Skip to content

feat(tool-delegate): return partial results on per-delegate timeout (k64 prereq 1 of 2, CONSUMER half) - #353

Merged
Brian Krabach (bkrabach) merged 1 commit into
mainfrom
lane/bp0-delegate-timeout-partial-consumer
Sep 3, 2026
Merged

feat(tool-delegate): return partial results on per-delegate timeout (k64 prereq 1 of 2, CONSUMER half)#353
Brian Krabach (bkrabach) merged 1 commit into
mainfrom
lane/bp0-delegate-timeout-partial-consumer

Conversation

@bkrabach

Copy link
Copy Markdown
Collaborator

This is HALF of a two-patch prerequisite

This is patch (1) of 2, the CONSUMER half, for model_performance-k64.

k64 remains BLOCKED until the app-cli producer half (model_performance-9w0)
also lands.
On its own this change makes partial_available observable but
always false, because nothing in amplifier-foundation registers a
session.partial capability. k64's gate G-D4 requires at least one timeout to
carry partial_available: true; run with only this half merged, it will
correctly record PARTIAL-PATH-NOT-EXERCISED and stop. That is the designed,
documented behaviour — it has not been faked.

Safe to land alone. Do not enable settings.timeout sweeps until the
producer half is in.


What this fixes

When a delegate exceeds settings.timeout, its own work was discarded. There
was no channel carrying what the sub-session produced before the deadline, and
no flag stating whether any such work existed.

  • Optional app-layer session.partial capability:
    (sub_session_id) -> {"text", "segments", "source"} | None, sync or async.
    Absent / empty / malformed / raising all degrade to
    partial_available: falsenever to an error. Raising out of the
    timeout handler is exactly what would propagate through asyncio.gather and
    discard completed siblings.
  • Both timeout paths (spawn and resume) now carry partial_available,
    partial_response, partial_segments, partial_source,
    partial_truncated, partial_chars_total, completed, guidance.
    Preserved text never lands on response, the success-only key.
  • settings.partial_max_chars (default 20000) caps preserved text, keeping
    the most recent tail.
  • elapsed_s emitted on delegate:error and in metadata — leg durations
    measured, not inferred.

⚠️ One breaking change, please review deliberately

status on the timeout result and on delegate:error is now "timeout",
was "timed_out". Single constant TIMEOUT_STATUS; reverting is one line.

Rationale: it is the literal string the downstream eval's gate observes
('every observed timeout result carries status "timeout"'), and "timed_out"
is not that string nor a substring of it. Blast radius measured before
changing:

$ grep -rn "timed_out" . | grep -v modules/tool-delegate     # foundation
(no matches)
$ grep -rn "timed_out" <installed amplifier_app_cli>
(no matches)

Only this module's own code, README and tests. Both channels changed together
so they cannot disagree.

Finding: gate G-D1 was already satisfied — reported, not staged

The item describes two harms. Only one still existed. 14d5a52 already
made the timeout handler return rather than raise, so completed siblings were
already surviving. Measured on the parent commit, unmodified:

--- C. G-D1 sibling survival ---
gather returned; completed siblings surviving = 2 of 2;
discarded-completed-sibling count = 0

There is therefore no fail-before test showing siblings being discarded,
because they are not. What this PR adds for G-D1 is a regression pin. The
previously measured 1.2% (4/533) harm rate predates 14d5a52 and should be
re-derived before being quoted as current.

Fail-before evidence

Parent 5ebf1da, git apply --check on the pre-existing patch —
all 10 source hunks failed, zero applied, nothing force-applied:

error: patch failed: modules/tool-delegate/amplifier_module_tool_delegate/__init__.py:20
error: ...: patch does not apply

Every hunk re-targeted by hand; the per-hunk record is in the DONE-NOTE §2.

New tests on the parent commit: 11 failed, 1 passed. The one that passes is
test_success_result_carries_no_partial_keys — the byte-identity guard, which
must pass on both sides.

FAILED test_timeout_is_not_success_on_either_channel
FAILED test_partial_text_never_lands_on_the_success_key
FAILED test_straggler_returns_rather_than_raises_so_siblings_survive
FAILED test_no_partial_capability_degrades_to_no_partial_not_to_error
FAILED test_partial_capability_raising_does_not_break_the_timeout_path
FAILED test_async_partial_capability_is_supported
FAILED test_partial_text_is_capped_and_keeps_the_tail
FAILED test_malformed_partial_payload_is_ignored
FAILED test_timeout_event_carries_elapsed_and_partial_flags
FAILED test_partial_max_chars_default_is_shipped_not_swept
FAILED test_resume_timeout_carries_the_same_partial_contract
11 failed, 1 passed

Normal completions are byte-identical — shown, not asserted

Same probe script, parent tree vs this one, diffed verbatim including the
exact serialized string the model receives:

$ diff <(sed -n "/^--- D./,$p" fail-before-probe.txt) \
       <(sed -n "/^--- D./,$p" after-probe.txt)
(no output -- IDENTICAL)

18dfd629b68861f7c11e2182a6d730f5e26cef39d48b8d4a9396b63b587a0ab1  D-before.txt
18dfd629b68861f7c11e2182a6d730f5e26cef39d48b8d4a9396b63b587a0ab1  D-after.txt

Structurally: the source diff removes 6 lines, every one a "timed_out"
literal or a timeout f-string. Zero lines removed from the success path.

Tests

uv run pytest -q                      # this branch: 1905 passed, 1 skipped
git stash -u && uv run pytest -q      # parent 5ebf1da: 1893 passed, 1 skipped
uv run pytest modules/tool-delegate/tests -q   # 189 passed

Delta is exactly +12, the new tests. The 1 warning is pre-existing in
tests/test_subprocess_runner.py, on both sides.

Reconciliation: the source patch's notes predicted "59 passed (48
pre-existing + 11 new)". Both numbers are stale — the module suite has 177
pre-existing tests at this base, so 177 + 12 = 189.

Note (pre-existing, not fixed here): CI runs pytest tests/, which
overrides testpaths and excludes modules/tool-delegate/tests entirely.
Every test in this PR, and all 177 pre-existing module tests, are invisible to
CI. Verified locally with the commands above.


Full record — per-hunk re-targeting, the eight re-targeting decisions and their
reasons, findings, and spend ($0.00 of $0.00 authorized) — is in
docs/lanes/bp0-delegate-timeout-partial-consumer/DONE-NOTE.md.

A delegate that exceeds settings.timeout already RETURNED rather than
raised (14d5a52), so completed siblings in the same asyncio.gather batch
already survived. What it still discarded was the straggler's own work:
there was no channel carrying what the sub-session produced before the
deadline, and no flag saying whether any such work existed.

- add an optional app-layer `session.partial` capability
  (sub_session_id) -> {"text", "segments", "source"}, sync or async.
  Absent, empty, malformed or raising degrades to
  partial_available: false -- never to an error, because raising out of
  the timeout handler is precisely what would discard the completed
  siblings this path protects.
- carry partial_available / partial_response / partial_segments /
  partial_source / partial_truncated / partial_chars_total / completed /
  guidance on both timeout paths (spawn and resume). Preserved text
  NEVER lands on `response`, the success-only key.
- settings.partial_max_chars (default 20000) caps preserved text,
  keeping the most recent tail.
- emit elapsed_s on delegate:error and in metadata, so leg durations are
  measured rather than inferred.
- BREAKING (small): output/event `status` is now "timeout", was
  "timed_out", via the single constant TIMEOUT_STATUS. Grep shows no
  consumer outside this module (not elsewhere in foundation, not in
  amplifier-app-cli). This is the literal string the downstream eval's
  gate observes.

Normal completions are byte-identical: same script run against the
parent tree and this one produces the same serialized output, sha256
18dfd629... on both sides. The source diff removes six lines, all of
them "timed_out" literals or timeout f-strings; nothing on the success
path is touched.

This is HALF of the prerequisite. partial_available is false for every
timeout until the app-cli producer half registers session.partial.
That is expected and documented, not a defect.

11 of 12 new tests fail on parent 5ebf1da; the one that passes is the
byte-identity guard, which must pass on both. Full suite: 1905 passed,
1 skipped (parent: 1893 passed, 1 skipped -- delta is exactly the 12
new tests).

Co-authored-by: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@bkrabach
Brian Krabach (bkrabach) marked this pull request as ready for review September 3, 2026 08:00
@bkrabach

Copy link
Copy Markdown
Collaborator Author

Manager verification (independent, scratch clone)

Head 26d5524, base 5ebf1dab = current origin/main, fast-forwardable. FIX → merging.

Full suite: 1905 passed, 1 skipped on the head (parent: 1893 — delta exactly +12, the new tests). CI green on all 6 matrix jobs + CLA.

Fail-before, reproduced independently. The test file alone fails at collection on the parent (ImportError: DEFAULT_PARTIAL_MAX_CHARS), which is weak evidence — so I ran this PR's own behavioural probe against an unmodified parent worktree instead:

parent 5ebf1da:   "status": "timed_out"        ← and NO partial_available key at all
PR head 26d5524:  "status": "timeout", "partial_available": true,
                  "partial_response", "completed": false, "guidance": ...

That is the defect and its repair, on the real code path, with no new symbols required to observe it.

Default behaviour unchanged — shown, not asserted. I diffed the probe's section D (a normal completion, including the exact serialized string the model receives) between parent and head: byte-identical. That is the deliverable most easily satisfied by assertion, and this PR earns it.

RT-2, the one breaking change, verified rather than trusted. status: "timed_out""timeout". I re-ran the grep myself: zero consumers of "timed_out" in foundation source (outside the module) and zero in amplifier-app-cli. The claim holds. It is behind a single TIMEOUT_STATUS constant, so reverting is one line if k64's harness prefers the incumbent string.

Scope: 11 files — the tool-delegate module, its tests, its README, and the lane's own artifact root. Nothing else.

Two findings that matter more than the diff

F-1 — half the stated harm no longer existed. Gate G-D1 (a straggler discarding completed siblings) was already satisfied on the parent: 14d5a52 had made the timeout handler RETURN rather than raise, after 37n cut its patch. The lane could not manufacture a fail-before for a defect that was gone, said so, and contributed a regression pin instead. The 1.2% (4/533) harm rate predates 14d5a52 and must not be re-quoted as current — I am recording that here so it does not leak into k64's writeup.

F-3 — this does not unblock k64, exactly as designed. partial_available is false for every real timeout in this repo until the app-cli producer half (model_performance-9w0) registers a session.partial capability. k64 run with only this half merged will correctly hit G-D4's PARTIAL-PATH-NOT-EXERCISED stop. Not faked, not papered over.

37n's patch did not apply at all — all 10 source hunks failed, and the "verified base" cc7e23aa is not even an object in this repo. Re-targeted by hand with a per-hunk table (RT-1…RT-8) and git apply --check output committed verbatim. Nothing force-applied over #350.

Squash + --admin (plain squash refused by the base-branch review policy, as with #350/#67/#58/#49).

@bkrabach
Brian Krabach (bkrabach) merged commit f42f48c into main Sep 3, 2026
7 checks passed
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