fix(tool-delegate): select the partial guidance string on the KIND of partial, not bool(text) - #356
Conversation
…ot bool(text)
`_partial_output_fields` picked its timeout guidance from `bool(text)` alone,
so a partial recovered from the REASONING channel got 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"
True of unfinished assistant prose. 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.
Reachable only since app-cli 8c83a9b (PR #298) widened the accumulator to
recover `thinking` + `tool_call` traces when no assistant text exists (k64:
recoverable window 0.05% -> 82.2% of a leg). That half is the producer; this is
the consumer.
Branches on `partial.source`, never on the prose:
no text -> _NO_PARTIAL_GUIDANCE (byte-identical)
"spawn-accumulator:reasoning"-> _REASONING_PARTIAL_GUIDANCE (new)
anything else -> _PARTIAL_GUIDANCE (byte-identical)
Exact match, deliberately: an unknown or non-string source degrades to the
incumbent behaviour rather than inheriting a frame that may be wrong for it.
`source` is compared, never parsed, so it cannot raise on the timeout path --
the one path where raising discards every completed sibling in a batch.
Byte-identity verified against the parent blob, not against this module's own
constants: _PARTIAL_GUIDANCE sha256 b1d9796d1a9adf29 (416 B) and
_NO_PARTIAL_GUIDANCE sha256 d73f51f164c545d3 (245 B) are unchanged, and the
parent's selector re-run against this build agrees on every case except the
reasoning one.
Tests land in tests/ (not modules/tool-delegate/tests/) because CI runs
`pytest tests/` only. Fail-before on 5d8db2f: 3 failed / 16 passed; after:
19 passed. Full suite 1939 -> 1958 passed, 1 skipped.
Refs: model_performance-yiy
Manager verification — byte-identity confirmed from the parent blob, and the frame is honest. Merging.Head Byte-identity — I extracted both constants from the parent blob myselfNot from the module's own constants, and not from the marker: So the text case and the no-partial case are provably unmoved, and app-cli's The new frame says what the content actually is
This is the judgement I most wanted to check, and it is right. It names the content correctly (private reasoning + tool-call trace), states the negative claim explicitly rather than implying it (never a draft answer; do not quote, summarise, or treat as a conclusion), and gives the correct positive affordance — use it to choose the next move, informed by what was already covered. It does not smuggle in the old sentence's implication that this is unfinished work product. Nothing in it overstates what a killed agent's scratch reasoning is. Safe degradation, which the goal required and is easy to get wrongif not text: return _NO_PARTIAL_GUIDANCE
if isinstance(source, str) and source in _REASONING_PARTIAL_SOURCES:
return _REASONING_PARTIAL_GUIDANCE
return _PARTIAL_GUIDANCEExact-match membership behind an Gates
The three failures are precisely the reasoning-frame assertions — including Pairs with app-cli |
Pairs with the producer half: app-cli
8c83a9b(microsoft/amplifier-app-cli PR #298).That commit widened app-cli's partial accumulator so a timed-out delegate that emitted
no assistant text now recovers the agent's
thinkingandtool_calltrace instead.This PR is the consumer half, in this repo. Neither half is complete without the other.
The defect
modules/tool-delegate's_partial_output_fieldspicked its timeout guidance frombool(text)alone. So a recovered-reasoning partial received the sentence writtenfor unfinished prose:
True of unfinished assistant prose. Not true of raw private reasoning: prose is at
least addressed to a reader; reasoning never was. Handing a model its own unreviewed
reasoning under that frame invites it to treat the reasoning as a draft answer — the one
thing it is not.
Before
8c83a9bthis case could not occur. After it, it is the common one: measured onk64's 18 legs, the recoverable window went 0.05% → 82.2% of a leg.
The change
One file, +64 / −2 (
modules/tool-delegate/amplifier_module_tool_delegate/__init__.py):_REASONING_PARTIAL_GUIDANCE— describes the payload as what it is: the agent's ownprivate reasoning and tool-call trace, evidence of what it was doing and what it had
looked at, never a draft answer.
_REASONING_PARTIAL_SOURCES = frozenset({"spawn-accumulator:reasoning"})_guidance_for(text, source)— three cases, in order:_NO_PARTIAL_GUIDANCE— byte-identicalsource == "spawn-accumulator:reasoning"_REASONING_PARTIAL_GUIDANCE— newsource)_PARTIAL_GUIDANCE— byte-identicalBranches on
partial_source. Never parses the prose — that is precisely what thefield exists to avoid.
Why exact match, not a
:reasoningsuffix testThe producer is a separate repo on its own release cadence, so a value this code has
never seen must be safe: it 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 the prose frame. That
tradeoff is recorded in a source comment and in the lane note, not left to be
rediscovered.
sourceis typedAnyand compared, never parsed, so a non-string value cannot raiseon the timeout path — the one path where raising discards every completed sibling in a
parallel batch.
Evidence
Fail-before → after, run against the parent commit
5d8db2fwith the moduleunchanged and only the new test file present:
The 16 that already pass on the parent are the point of the design: they are the
byte-identity, no-partial, and unknown-source pins, and they must pass on both sides.
Exactly the 3 reasoning-kind tests move.
Byte-identity, checked against the parent blob rather than against this module's own
constants (which would be tautological) —
git show HEAD:…+ast.literal_eval, plusthe parent's selector re-run against this build:
Exactly one case changed. app-cli's round-trip test
test_guidance_string_is_unchanged_for_the_text_casepins the same bytes from theproducer side, so a regression here fails there too. The new test file spells both
incumbent strings out as literals rather than importing the constants, so a reword
fails the test instead of silently redefining "unchanged".
Test suite
uv run pytest -q— before (parent5d8db2f)uv run pytest -q— this branchuv run pytest tests/ -q --tb=short(what CI runs)+19 = exactly the new tests; zero pre-existing tests changed state.
Test placement is deliberate. The new file is
tests/test_partial_guidance_kind_yiy.py,not
modules/tool-delegate/tests/— CI runspytest tests/only, so a test placed onlyin the module directory would never run there.
Scope
timeout_msgat both call sites (:2276,:2676) is unchanged. It says "it isUNFINISHED, not a result", which is true of reasoning too and does not carry the
offending "not checked, concluded, or self-reviewed" clause. Not the defect; not
touched.
amplifier-app-cliis not modified. Its half is merged;partial_sourceis thisrepo's input.
nothing recovered.
Lane note with the full record:
docs/lanes/yiy-partial-guidance-kind/DONE-NOTE.md.Refs
model_performance-yiy. Spend: $0.00.