diff --git a/docs/lanes/yiy-partial-guidance-kind/DONE-NOTE.md b/docs/lanes/yiy-partial-guidance-kind/DONE-NOTE.md new file mode 100644 index 0000000..3deb230 --- /dev/null +++ b/docs/lanes/yiy-partial-guidance-kind/DONE-NOTE.md @@ -0,0 +1,181 @@ +# DONE-NOTE — `model_performance-yiy` + +**Lane:** `yiy-partial-guidance-kind` · **Repo:** `microsoft/amplifier-foundation` · +**Branch:** `lane/yiy-partial-guidance-kind` · **Parent commit:** `5d8db2fa4715e4dfe7d3b6604d725de4c291ec9d` + +**Outcome: branch A — RESOLVED.** Every deliverable is DONE. Nothing was +NOT-POSSIBLE; nothing was dropped. Spend: **$0.00** against an authority of +**$0.00** (see §Spend). + +--- + +## What was wrong, in one paragraph + +`modules/tool-delegate` picked its timeout guidance string from `bool(text)` +alone, so a partial recovered from the **reasoning** channel received the +sentence written for unfinished **prose**: *"the text in 'partial_response' is +unfinished work salvaged from the agent mid-flight — it has NOT been checked, +concluded, or self-reviewed by that agent."* That is true of prose the agent was +writing for a reader and did not finish. It is false of raw private reasoning, +which was never addressed to a reader at all — and framing it as unreviewed +draft output invites the calling model to read it as a draft answer. + +This became reachable only after app-cli `8c83a9b` (PR #298) widened the +accumulator to recover `thinking` + `tool_call` traces when no assistant text +exists. Before that commit the case could not occur; after it, it is the common +one (k64's 18 legs: recoverable window 0.05% → 82.2% of a leg). + +## The change + +`modules/tool-delegate/amplifier_module_tool_delegate/__init__.py`, **+64 / −2**, +one file, no other file in the repo touched: + +- new constant `_REASONING_PARTIAL_GUIDANCE` +- new constant `_REASONING_PARTIAL_SOURCES = frozenset({"spawn-accumulator:reasoning"})` +- new selector `_guidance_for(text, source)` — three cases, in order: + no text → `_NO_PARTIAL_GUIDANCE`; recognised reasoning source → + `_REASONING_PARTIAL_GUIDANCE`; everything else → `_PARTIAL_GUIDANCE` +- `_partial_output_fields` reads `source` once and calls the selector + +eem scoped this as *"two lines and a constant"*. The executable change is +exactly that; the remaining lines are the two guidance/selection docstrings and +the comment recording **why** exact-match was chosen (below). + +### The one judgement call, and why + +**Exact match on `"spawn-accumulator:reasoning"`, not a `:reasoning` suffix +test.** The deliverable requires that an unknown or absent `partial_source` +*must not* silently receive the reasoning frame. The producer is a separate repo +on its own release cadence, so exact match is the reading that satisfies that +literally: an unrecognised value degrades to the incumbent behaviour rather than +inheriting a frame that may be wrong for it. **The cost is stated rather than +hidden:** if app-cli ever renames the source, the reasoning frame silently +reverts to today's prose frame — i.e. back to this defect. Recorded here and in +a source comment so the next reader sees the tradeoff rather than rediscovering +it. No human decision was waited on. + +`source` is typed `Any` and compared, never parsed, so a non-string value +(`None`, `int`, `list`, `dict`) cannot raise on the timeout path — the one path +where raising would discard every completed sibling in a parallel batch. + +## Deliverables + +| # | Deliverable | State | Evidence | +|---|---|---|---| +| 1 | Reasoning-kind partial gets a guidance string describing what it actually is | **DONE** | `_REASONING_PARTIAL_GUIDANCE`; `after.txt`; 3 tests | +| 2 | Text case BYTE-IDENTICAL to today | **DONE** | `byte-identity.txt` — sha256 `b1d9796d1a9adf29`, len 416, parent == now | +| 3 | No-partial case unchanged | **DONE** | `byte-identity.txt` — sha256 `d73f51f164c545d3`, len 245, parent == now | +| 4 | Fail-before test, run against the parent, both outputs pasted | **DONE** | `fail-before.txt` (3 failed / 16 passed) → `after.txt` (19 passed) | +| 5 | Unknown/absent `partial_source` does not crash, does not get the reasoning frame | **DONE** | 11 parametrised cases + absent-key case, all green | +| 6 | Full foundation suite green, before/after counts stated | **DONE** | 1939 → **1958** passed, 1 skipped both sides | +| 7 | DRAFT PR on origin naming app-cli `8c83a9b` | **DONE** | see `publication` in `DONE.json` | +| 8 | DONE-NOTE at the lane artifact root | **DONE** | this file | + +## Fail-before → after (deliverable 4) + +Run on the parent commit `5d8db2f` with the module **unchanged** and only the new +test file present. Full transcripts in `fail-before.txt` and `after.txt`. + +``` +=== FAIL-BEFORE: parent commit 5d8db2fa4715e4dfe7d3b6604d725de4c291ec9d, module UNCHANGED === +$ uv run pytest tests/test_partial_guidance_kind_yiy.py -q +FAILED tests/test_partial_guidance_kind_yiy.py::test_reasoning_partial_is_not_framed_as_unfinished_prose +FAILED tests/test_partial_guidance_kind_yiy.py::test_reasoning_guidance_says_what_the_payload_actually_is +FAILED tests/test_partial_guidance_kind_yiy.py::test_reasoning_kind_is_honoured_on_the_resume_path_too +3 failed, 16 passed in 0.31s + +=== AFTER: with the fix applied === +$ uv run pytest tests/test_partial_guidance_kind_yiy.py -q +19 passed in 0.26s +``` + +**The 16 that passed on the parent are the point of the design.** They are the +byte-identity, no-partial, and unknown-source pins — they must pass on *both* +sides, or the file would merely be asserting the new behaviour everywhere and +would prove nothing about what stayed still. Exactly the 3 reasoning-kind tests +move. + +## Byte-identity, checked honestly (deliverables 2 and 3) + +Not asserted from the module's own constants (that would be tautological). The +parent blob was extracted with `git show HEAD:…`, its two constants parsed out +with `ast.literal_eval`, and compared byte-for-byte against this build — +**and** the parent's selector was re-implemented and run against this build's +actual output for every case meant to be unchanged. `byte-identity.txt`: + +``` +_PARTIAL_GUIDANCE parent sha256=b1d9796d1a9adf29 len=416 | now sha256=b1d9796d1a9adf29 len=416 | BYTE-IDENTICAL = True +_NO_PARTIAL_GUIDANCE parent sha256=d73f51f164c545d3 len=245 | now sha256=d73f51f164c545d3 len=245 | BYTE-IDENTICAL = True + +runtime: this build's guidance vs what the PARENT selector would return + text partial (spawn-accumulator) unchanged-vs-parent = True + text partial (source absent) unchanged-vs-parent = True + text partial (unknown source) unchanged-vs-parent = True + text partial (non-str source) unchanged-vs-parent = True + no partial unchanged-vs-parent = True + reasoning partial (CHANGES) unchanged-vs-parent = False +``` + +Exactly one case changed. That case is the deliverable. + +The test file additionally spells both incumbent strings out as **literals** +rather than importing the constants, so a future reword of a constant fails the +test instead of silently redefining "unchanged". app-cli's +`test_guidance_string_is_unchanged_for_the_text_case` pins the same bytes from +the producer side; the two now fail together. + +## Test suite (deliverable 6) + +| Run | Command | Result | +|---|---|---| +| Before (parent `5d8db2f`, clean tree) | `uv run pytest -q` | **1939 passed, 1 skipped** | +| After (this branch) | `uv run pytest -q` | **1958 passed, 1 skipped** | +| After, CI's own subset | `uv run pytest tests/ -q --tb=short` | **1769 passed, 1 skipped** | + ++19 = exactly the 19 new tests. Zero pre-existing tests changed state. The +before figure matches the goal's KNOWN section (1939/1 at `5d8db2f`). + +**Test placement is deliberate.** The new file is `tests/test_partial_guidance_kind_yiy.py`, +not `modules/tool-delegate/tests/`. CI runs `uv run pytest tests/ -q --tb=short` +(`.github/workflows/*.yml:35`), which excludes the module's own test directory — +a test placed only there would never run in CI. Same reasoning 67u used. The +local `testpaths` covers both, which is why the full-suite figure is larger than +the CI figure. + +## Spend + +**$0.00 spent against an authority of $0.00.** The goal's arithmetic — +`0 runs × 0 arms × $0 / 1.00 = $0.00`, slack $0.00 — closes trivially and +correctly: this is a pure source change with no container, no DTU, and no eval +spend. No API calls were bought. No infrastructure was created, so no row was +added to the infra ledger and no teardown was run. The cap never bound and no +deliverable was reduced by it. + +## Deviations and scope decisions + +1. **The `timeout_msg` error string was left alone.** Both timeout call sites + (spawn `:2276`, resume `:2676`) also branch on `partial.get("text")` to + compose the human/log message *"Partial output was preserved … it is + UNFINISHED, not a result."* That sentence is **true of reasoning too** and + does not contain the offending "not checked, concluded, or self-reviewed" + clause, so it is not the defect. Changing it would have exceeded eem's scope + and put the byte-identity pin at risk in passing. Left as-is, deliberately. +2. **`amplifier-app-cli` was not modified**, per scope-out. Its half is merged. + Note for the record: the app-cli copy installed on this host predates + `8c83a9b` (its `get_partial_output` returns only `"spawn-accumulator"`), so + the producer values used here are the ones the item and goal specify, not + ones read off this host's installed copy. This is a consumer-side change and + is fully exercised by its own tests; no cross-repo runtime check was in + scope or was performed. +3. **No repo-root `DONE-NOTE.md`** was created or modified (item kez). All lane + artifacts are under `docs/lanes/yiy-partial-guidance-kind/`. +4. Nothing was merged; nothing outside this lane's owned paths was touched. + +## Artifacts in this directory + +| File | What it is | +|---|---| +| `DONE-NOTE.md` | this note | +| `fail-before.txt` | verbatim pytest output on the parent commit, module unchanged | +| `after.txt` | verbatim pytest output with the fix applied | +| `byte-identity.txt` | verbatim output of the parent-vs-now constant and selector comparison | diff --git a/docs/lanes/yiy-partial-guidance-kind/after.txt b/docs/lanes/yiy-partial-guidance-kind/after.txt new file mode 100644 index 0000000..53311a7 --- /dev/null +++ b/docs/lanes/yiy-partial-guidance-kind/after.txt @@ -0,0 +1,4 @@ +=== AFTER: with the fix applied === +$ uv run pytest tests/test_partial_guidance_kind_yiy.py -q +................... [100%] +19 passed in 0.26s diff --git a/docs/lanes/yiy-partial-guidance-kind/byte-identity.txt b/docs/lanes/yiy-partial-guidance-kind/byte-identity.txt new file mode 100644 index 0000000..ba0b48f --- /dev/null +++ b/docs/lanes/yiy-partial-guidance-kind/byte-identity.txt @@ -0,0 +1,18 @@ +parent commit under comparison: 5d8db2fa4715e4dfe7d3b6604d725de4c291ec9d + +_PARTIAL_GUIDANCE + parent sha256=b1d9796d1a9adf29 len=416 + now sha256=b1d9796d1a9adf29 len=416 + BYTE-IDENTICAL = True +_NO_PARTIAL_GUIDANCE + parent sha256=d73f51f164c545d3 len=245 + now sha256=d73f51f164c545d3 len=245 + BYTE-IDENTICAL = True + +runtime: this build's guidance vs what the PARENT selector would return + text partial (spawn-accumulator) unchanged-vs-parent = True + text partial (source absent) unchanged-vs-parent = True + text partial (unknown source) unchanged-vs-parent = True + text partial (non-str source) unchanged-vs-parent = True + no partial unchanged-vs-parent = True + reasoning partial (CHANGES) unchanged-vs-parent = False diff --git a/docs/lanes/yiy-partial-guidance-kind/fail-before.txt b/docs/lanes/yiy-partial-guidance-kind/fail-before.txt new file mode 100644 index 0000000..e2c7dd8 --- /dev/null +++ b/docs/lanes/yiy-partial-guidance-kind/fail-before.txt @@ -0,0 +1,62 @@ +=== FAIL-BEFORE: parent commit 5d8db2fa4715e4dfe7d3b6604d725de4c291ec9d (5d8db2f), module UNCHANGED === +$ uv run pytest tests/test_partial_guidance_kind_yiy.py -q + + guidance = result.output["guidance"] + assert result.output["partial_source"] == REASONING_SOURCE +> assert PROSE_CLAUSE not in guidance +E AssertionError: assert 'unfinished ...t mid-flight' not in 'INCOMPLETE:... resumption.' +E +E 'unfinished work sa...he agent mid-flight' is contained here: +E ? ^^^^^^^^^^ +E ponse' is unfinished work salvaged from the agent mid-flight -- it has NOT been checked, concluded, or self-reviewed by that agent. Do not report it as a completed result and do not treat its conclusions as final. Re-delegate a narrower task or complete the work yourself; see metadata.recovery_message before considering this session for resumption. +E ? ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +tests/test_partial_guidance_kind_yiy.py:194: AssertionError +------------------------------ Captured log call ------------------------------- +WARNING amplifier_module_tool_delegate:__init__.py:2282 Agent 'test-agent' timed out after 0.01s (delegate tool session-level timeout; elapsed 0.01s). Partial output was preserved and is returned under 'partial_response' -- it is UNFINISHED, not a result. Child cancellation cleanup is still in progress; do not resume this session until cleanup and persistence complete. +__________ test_reasoning_guidance_says_what_the_payload_actually_is ___________ + + @pytest.mark.asyncio + async def test_reasoning_guidance_says_what_the_payload_actually_is(): + """It is evidence of what the agent was doing -- not a draft answer.""" + tool = _make_tool( + timeout=0.01, + partial_fn=_partial("thinking, then a tool call", REASONING_SOURCE), + ) + guidance = (await _spawn(tool)).output["guidance"] + + lowered = guidance.lower() +> assert "reasoning" in lowered +E assert 'reasoning' in "incomplete: this delegate did not finish. the text in 'partial_response' is unfinished work salvaged from the agent m...ower task or complete the work yourself; see metadata.recovery_message before considering this session for resumption." + +tests/test_partial_guidance_kind_yiy.py:209: AssertionError +------------------------------ Captured log call ------------------------------- +WARNING amplifier_module_tool_delegate:__init__.py:2282 Agent 'test-agent' timed out after 0.01s (delegate tool session-level timeout; elapsed 0.01s). Partial output was preserved and is returned under 'partial_response' -- it is UNFINISHED, not a result. Child cancellation cleanup is still in progress; do not resume this session until cleanup and persistence complete. +____________ test_reasoning_kind_is_honoured_on_the_resume_path_too ____________ + + @pytest.mark.asyncio + async def test_reasoning_kind_is_honoured_on_the_resume_path_too(): + """The resume path is a second timeout call site; it must not diverge.""" + tool = _make_tool( + timeout=0.01, + partial_fn=_partial("resumed reasoning trace", REASONING_SOURCE), + ) + result = await _resume(tool) + + assert result.output["partial_source"] == REASONING_SOURCE +> assert PROSE_CLAUSE not in result.output["guidance"] +E AssertionError: assert 'unfinished ...t mid-flight' not in 'INCOMPLETE:... resumption.' +E +E 'unfinished work sa...he agent mid-flight' is contained here: +E ? ^^^^^^^^^^ +E ponse' is unfinished work salvaged from the agent mid-flight -- it has NOT been checked, concluded, or self-reviewed by that agent. Do not report it as a completed result and do not treat its conclusions as final. Re-delegate a narrower task or complete the work yourself; see metadata.recovery_message before considering this session for resumption. +E ? ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +tests/test_partial_guidance_kind_yiy.py:227: AssertionError +------------------------------ Captured log call ------------------------------- +WARNING amplifier_module_tool_delegate:__init__.py:2682 Resumed agent 'test-agent' timed out after 0.01s (delegate tool session-level timeout; elapsed 0.01s). Partial output was preserved and is returned under 'partial_response' -- it is UNFINISHED, not a result. Child cancellation cleanup is still in progress; do not resume this session until cleanup and persistence complete. +=========================== short test summary info ============================ +FAILED tests/test_partial_guidance_kind_yiy.py::test_reasoning_partial_is_not_framed_as_unfinished_prose +FAILED tests/test_partial_guidance_kind_yiy.py::test_reasoning_guidance_says_what_the_payload_actually_is +FAILED tests/test_partial_guidance_kind_yiy.py::test_reasoning_kind_is_honoured_on_the_resume_path_too +3 failed, 16 passed in 0.31s diff --git a/modules/tool-delegate/amplifier_module_tool_delegate/__init__.py b/modules/tool-delegate/amplifier_module_tool_delegate/__init__.py index eaee897..34439b2 100644 --- a/modules/tool-delegate/amplifier_module_tool_delegate/__init__.py +++ b/modules/tool-delegate/amplifier_module_tool_delegate/__init__.py @@ -124,6 +124,67 @@ "considering this session for resumption." ) +# Guidance for a partial recovered from the REASONING channel: the producer +# found no assistant text at all and fell back to the agent's own thinking +# blocks and tool-call trace. +# +# _PARTIAL_GUIDANCE above is wrong for this payload, and wrong in a way that +# does harm. "Unfinished work ... not checked, concluded, or self-reviewed" +# describes prose the agent was writing FOR a reader and did not get to +# finish. Private reasoning was never addressed to a reader at all; framing +# it as unreviewed draft output invites the caller to read it as a draft +# answer, which is the one thing it is not. +# +# The payload itself is already self-labelled at head and tail by the +# producer. What this constant fixes is the frame this repo puts around it. +_REASONING_PARTIAL_GUIDANCE = ( + "INCOMPLETE: this delegate did not finish, and it produced no answer text " + "at all before the deadline. What is in 'partial_response' is the agent's " + "own private reasoning and the trace of the tool calls it made -- evidence " + "of what it was doing and what it had looked at. It was never addressed to " + "a reader and is never a draft answer, so do not quote it, summarize it as " + "a result, or treat any statement in it as a conclusion. Use it only to " + "decide what to do next: re-delegate a narrower task informed by what it " + "had already covered, or complete the work yourself; see " + "metadata.recovery_message before considering this session for resumption." +) + +#: ``partial.source`` values that denote the reasoning channel rather than +#: recovered assistant prose. Produced by app-cli ``8c83a9b`` +#: (``amplifier_app_cli/session_spawner.py::get_partial_output``), which +#: returns ``"spawn-accumulator"`` whenever assistant text exists and +#: ``"spawn-accumulator:reasoning"`` only when it does not. +#: +#: EXACT MATCH, deliberately. The producer is a separate repo on its own +#: release cadence, so a value this code has never seen must degrade to the +#: incumbent behaviour rather than inherit a frame that may be wrong for it. +#: A prefix or suffix test would hand the reasoning frame to any future +#: producer that merely happens to spell its source similarly. +_REASONING_PARTIAL_SOURCES = frozenset({"spawn-accumulator:reasoning"}) + + +def _guidance_for(text: str, source: Any) -> str: + """Pick the timeout guidance by the KIND of partial, not by ``bool(text)``. + + Three cases, in order: + + * nothing recovered -> ``_NO_PARTIAL_GUIDANCE`` (unchanged) + * recovered from the reasoning channel -> ``_REASONING_PARTIAL_GUIDANCE`` + * anything else, including an unknown or non-string ``source`` -> + ``_PARTIAL_GUIDANCE``, byte-identical to what shipped before this + branch existed. app-cli's round-trip test + ``test_guidance_string_is_unchanged_for_the_text_case`` asserts the + same bytes from the producer side. + + ``source`` is typed ``Any`` on purpose: it arrives from another repo and + is compared, never parsed, so a non-string can never raise here. + """ + if not text: + return _NO_PARTIAL_GUIDANCE + if isinstance(source, str) and source in _REASONING_PARTIAL_SOURCES: + return _REASONING_PARTIAL_GUIDANCE + return _PARTIAL_GUIDANCE + def _partial_output_fields(partial: dict[str, Any]) -> dict[str, Any]: """The additive timeout-result keys describing recovered partial work. @@ -134,15 +195,16 @@ def _partial_output_fields(partial: dict[str, Any]) -> dict[str, Any]: ``partial_available`` states plainly whether any exists. """ text = partial.get("text") or "" + source = partial.get("source", "none") return { "completed": False, "partial_available": bool(text), "partial_response": text or None, "partial_segments": partial.get("segments", 0), - "partial_source": partial.get("source", "none"), + "partial_source": source, "partial_truncated": bool(partial.get("truncated")), "partial_chars_total": partial.get("chars_total", len(text)), - "guidance": _PARTIAL_GUIDANCE if text else _NO_PARTIAL_GUIDANCE, + "guidance": _guidance_for(text, source), } diff --git a/tests/test_partial_guidance_kind_yiy.py b/tests/test_partial_guidance_kind_yiy.py new file mode 100644 index 0000000..6b0ada6 --- /dev/null +++ b/tests/test_partial_guidance_kind_yiy.py @@ -0,0 +1,351 @@ +"""Pins the answer to model_performance-yiy. + +THE DEFECT. ``modules/tool-delegate`` picked its timeout guidance string from +``bool(text)`` alone. Both kinds of recovered partial therefore got the same +sentence: + + "INCOMPLETE: ... The text in 'partial_response' is unfinished work salvaged + from the agent mid-flight -- it has NOT been checked, concluded, or + self-reviewed by that agent." + +That is true of unfinished assistant prose. It is NOT true of raw private +reasoning. Prose is at least addressed to a reader; reasoning never was. +Framing an agent's own reasoning as "unfinished work ... not self-reviewed" +invites the caller to read it as a draft answer -- which is precisely what it +is not. + +WHY THIS BECAME REACHABLE. app-cli ``8c83a9b`` (PR #298) widened its partial +accumulator: when a timed-out delegate emitted no assistant text at all, it now +recovers the agent's ``thinking`` and ``tool_call`` trace instead. Measured on +k64's 18 legs the recoverable window went 0.05% -> 82.2% of a leg. Before that +change this case could not occur; after it, it is the common one. + +THE FIELD TO SELECT ON. The producer distinguishes the two kinds for us: +``source`` is ``"spawn-accumulator:reasoning"`` for the reasoning channel and +``"spawn-accumulator"`` for a text partial. The consumer branches on that -- +never on the prose, which is exactly what the field exists to avoid. + +WHAT THESE TESTS PIN, and why each matters: + + 1. Reasoning kind gets a guidance string that describes what it actually is, + and does NOT carry the prose sentence. (Fails on the parent commit.) + 2. The TEXT case is BYTE-IDENTICAL to before. The literal is spelled out + here rather than compared to the module constant, so a reword of the + constant fails this test instead of silently redefining "unchanged". + app-cli's round-trip test ``test_guidance_string_is_unchanged_for_the_text_case`` + asserts the same bytes from the producer side. + 3. The no-partial case is unchanged, pinned the same literal way. + 4. An unknown, absent, or non-string ``source`` must not crash and must not + silently receive the reasoning frame -- it degrades to the pre-existing + behaviour. The producer is a different repo on its own release cadence; + a value this code has never seen has to be safe. + +These tests live under ``tests/`` rather than ``modules/tool-delegate/tests/`` +deliberately: CI runs ``pytest tests/`` only, so a test placed only in the +module directory would never run there. +""" + +from __future__ import annotations + +import asyncio +from typing import Any +from unittest.mock import AsyncMock +from unittest.mock import MagicMock + +import pytest +from amplifier_module_tool_delegate import DelegateTool + +# --------------------------------------------------------------------------- +# The two guidance strings that must not move, quoted here as literals. +# +# Copying them rather than importing the constants is the point: importing +# would make the assertion tautological, and "byte-identical to today" is only +# a real pin if today's bytes are written down somewhere a reword must edit. +# --------------------------------------------------------------------------- + +TEXT_GUIDANCE_AS_SHIPPED = ( + "INCOMPLETE: this delegate did not finish. The text in 'partial_response' " + "is unfinished work salvaged from the agent mid-flight -- it has NOT been " + "checked, concluded, or self-reviewed by that agent. Do not report it as a " + "completed result and do not treat its conclusions as final. Re-delegate a " + "narrower task or complete the work yourself; see metadata.recovery_message " + "before considering this session for resumption." +) + +NO_PARTIAL_GUIDANCE_AS_SHIPPED = ( + "INCOMPLETE: this delegate did not finish and no partial output could be " + "recovered. Nothing here is a result. Re-delegate a narrower task or " + "complete the work yourself; see metadata.recovery_message before " + "considering this session for resumption." +) + +#: The producer's own values, verbatim (app-cli ``8c83a9b``, +#: ``amplifier_app_cli/session_spawner.py::get_partial_output``). +REASONING_SOURCE = "spawn-accumulator:reasoning" +TEXT_SOURCE = "spawn-accumulator" + +#: The sentence that must never appear over recovered reasoning. It is the +#: whole defect in one clause. +PROSE_CLAUSE = "unfinished work salvaged from the agent mid-flight" + +_ABSENT = object() + + +# --------------------------------------------------------------------------- +# Harness: a DelegateTool over a fake coordinator whose spawn never finishes, +# so the real timeout path runs. Mirrors +# ``modules/tool-delegate/tests/test_delegate_timeout_partial.py`` so both +# files exercise the same code, not a reimplementation of it. +# --------------------------------------------------------------------------- + + +def _make_tool(*, timeout: float, partial_fn: Any = _ABSENT) -> DelegateTool: + coordinator = MagicMock() + coordinator.session_id = "parent-session-yiy" + coordinator.config = {"agents": {"test-agent": {}}} + coordinator.session_state = {} + coordinator._tool_dispatch_context = {} + coordinator._tool_dispatch_contexts = {} + + capabilities: dict[str, Any] = { + "session.spawn": AsyncMock(side_effect=_never_finishes), + "session.resume": AsyncMock(side_effect=_never_finishes), + "self_delegation_depth": 0, + } + if partial_fn is not _ABSENT: + capabilities["session.partial"] = partial_fn + coordinator.get_capability = lambda name: capabilities.get(name) + coordinator.get = MagicMock(return_value=None) + + parent_session = MagicMock() + parent_session.config = {"session": {"orchestrator": {}}} + coordinator.session = parent_session + + return DelegateTool( + coordinator, + {"features": {}, "settings": {"exclude_tools": [], "timeout": timeout}}, + ) + + +async def _never_finishes(**_kwargs): + await asyncio.Future() + + +def _hooks() -> MagicMock: + hooks = MagicMock() + hooks.emit = AsyncMock() + return hooks + + +async def _spawn(tool: DelegateTool) -> Any: + return await tool._spawn_new_session( + agent_name="test-agent", + instruction="Do something", + context_depth="none", + context_scope="conversation", + context_turns=5, + provider_preferences=None, + hooks=_hooks(), + tool_call_id="call-yiy", + parallel_group_id="parallel-yiy", + ) + + +async def _resume(tool: DelegateTool) -> Any: + return await tool._resume_existing_session( + session_id="child-session-yiy_test-agent", + instruction="Continue", + hooks=_hooks(), + tool_call_id="call-yiy-resume", + parallel_group_id="parallel-yiy-resume", + ) + + +def _partial(text: str, source: Any = _ABSENT) -> Any: + payload: dict[str, Any] = {"text": text, "segments": 2} + if source is not _ABSENT: + payload["source"] = source + return lambda _sid: payload + + +# --------------------------------------------------------------------------- +# 1. The defect: reasoning must not be framed as unfinished prose. +# THIS IS THE FAIL-BEFORE TEST. +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_reasoning_partial_is_not_framed_as_unfinished_prose(): + """FAILS on the parent commit (5d8db2f), passes after. + + On the parent, guidance is selected by ``bool(text)`` alone, so a + reasoning partial receives ``_PARTIAL_GUIDANCE`` verbatim -- including + the "unfinished work ... not been checked, concluded, or self-reviewed" + clause, which is false of raw reasoning. + """ + tool = _make_tool( + timeout=0.01, + partial_fn=_partial("I should check the config first...", REASONING_SOURCE), + ) + result = await _spawn(tool) + + guidance = result.output["guidance"] + assert result.output["partial_source"] == REASONING_SOURCE + assert PROSE_CLAUSE not in guidance + assert "self-reviewed" not in guidance + assert guidance != TEXT_GUIDANCE_AS_SHIPPED + + +@pytest.mark.asyncio +async def test_reasoning_guidance_says_what_the_payload_actually_is(): + """It is evidence of what the agent was doing -- not a draft answer.""" + tool = _make_tool( + timeout=0.01, + partial_fn=_partial("thinking, then a tool call", REASONING_SOURCE), + ) + guidance = (await _spawn(tool)).output["guidance"] + + lowered = guidance.lower() + assert "reasoning" in lowered + assert "tool call" in lowered or "tool-call" in lowered + # It still has to say, unmistakably, that the delegate did not finish. + assert guidance.startswith("INCOMPLETE:") + # And it must not invite the caller to treat reasoning as an answer. + assert "not a" in lowered or "never" in lowered + + +@pytest.mark.asyncio +async def test_reasoning_kind_is_honoured_on_the_resume_path_too(): + """The resume path is a second timeout call site; it must not diverge.""" + tool = _make_tool( + timeout=0.01, + partial_fn=_partial("resumed reasoning trace", REASONING_SOURCE), + ) + result = await _resume(tool) + + assert result.output["partial_source"] == REASONING_SOURCE + assert PROSE_CLAUSE not in result.output["guidance"] + + +# --------------------------------------------------------------------------- +# 2. The text case is byte-identical to today. +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_text_partial_guidance_is_byte_identical_to_today(): + tool = _make_tool( + timeout=0.01, + partial_fn=_partial("half an answer", TEXT_SOURCE), + ) + result = await _spawn(tool) + + assert result.output["partial_source"] == TEXT_SOURCE + assert result.output["guidance"] == TEXT_GUIDANCE_AS_SHIPPED + + +@pytest.mark.asyncio +async def test_text_partial_guidance_is_byte_identical_on_resume_too(): + tool = _make_tool( + timeout=0.01, + partial_fn=_partial("half an answer", TEXT_SOURCE), + ) + result = await _resume(tool) + + assert result.output["guidance"] == TEXT_GUIDANCE_AS_SHIPPED + + +# --------------------------------------------------------------------------- +# 3. The no-partial case is unchanged. +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_no_partial_guidance_is_byte_identical_to_today(): + tool = _make_tool(timeout=0.01) # no session.partial capability at all + result = await _spawn(tool) + + assert result.output["partial_available"] is False + assert result.output["partial_source"] == "none" + assert result.output["guidance"] == NO_PARTIAL_GUIDANCE_AS_SHIPPED + + +@pytest.mark.asyncio +async def test_empty_text_with_a_reasoning_source_still_reads_as_no_partial(): + """No recovered characters means nothing was recovered, whatever the kind. + + ``partial_available`` is False here, so the reasoning frame would be + describing an empty payload. The no-partial guidance is the honest one. + """ + tool = _make_tool(timeout=0.01, partial_fn=_partial("", REASONING_SOURCE)) + result = await _spawn(tool) + + assert result.output["partial_available"] is False + assert result.output["guidance"] == NO_PARTIAL_GUIDANCE_AS_SHIPPED + + +# --------------------------------------------------------------------------- +# 4. An unrecognised source degrades; it never crashes and never gets the +# reasoning frame by accident. +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "source", + [ + "capability", + "store", + "spawn-accumulator:something-invented-later", + "SPAWN-ACCUMULATOR:REASONING", # case matters; this is not the value + "spawn-accumulator:reasoning ", # trailing space; not the value either + "", + None, + 123, + ["spawn-accumulator:reasoning"], + {"kind": "reasoning"}, + ], +) +async def test_unknown_source_degrades_to_the_text_guidance(source: Any): + """A value this repo has never seen must be safe and must not be reasoning. + + The producer ships on its own cadence. Selecting by exact match on the one + value it documents means an unrecognised value falls back to the + pre-existing behaviour rather than inheriting a frame that may be wrong + for it. + """ + tool = _make_tool(timeout=0.01, partial_fn=_partial("some recovered text", source)) + result = await _spawn(tool) + + assert result.success is False + assert result.output["status"] == "timeout" + assert result.output["partial_available"] is True + assert result.output["guidance"] == TEXT_GUIDANCE_AS_SHIPPED + + +@pytest.mark.asyncio +async def test_absent_source_key_degrades_to_the_text_guidance(): + """The payload need not carry ``source`` at all.""" + tool = _make_tool(timeout=0.01, partial_fn=_partial("some recovered text")) + result = await _spawn(tool) + + assert result.output["guidance"] == TEXT_GUIDANCE_AS_SHIPPED + + +@pytest.mark.asyncio +async def test_the_other_partial_fields_are_untouched_for_the_reasoning_kind(): + """Only the guidance frame changes. The payload contract does not move.""" + tool = _make_tool( + timeout=0.01, + partial_fn=_partial("reasoning text", REASONING_SOURCE), + ) + result = await _spawn(tool) + + assert "response" not in result.output + assert result.output["completed"] is False + assert result.output["status"] == "timeout" + assert result.output["partial_available"] is True + assert result.output["partial_response"] == "reasoning text" + assert result.output["partial_segments"] == 2 + assert result.output["partial_truncated"] is False + assert result.output["partial_chars_total"] == len("reasoning text")