feat(delegation): add same-turn background result injection - #74378
feat(delegation): add same-turn background result injection#74378Xipong wants to merge 7 commits into
Conversation
b609d34 to
15d24a4
Compare
|
CI is currently blocked at Could a maintainer approve the workflow run? The focused delivery/async/gateway/CLI suite passes locally (75 tests), and the remaining repository-wide limitations are documented transparently in the PR body. |
15d24a4 to
e15e2cb
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the substantial durability and recovery work. The feature is still absent from current main, so this is not stale.
Problems
agent/delegation_inject.py:273-285appends a syntheticrole="user"message during an active turn. That directly conflicts with the repository's stated invariant inAGENTS.md:88-91: never inject a synthetic user message mid-loop. This needs an explicit maintainer-approved cache/alternation design before this path can merge.- The gateway receives a busy-parent deferral, but the TUI completion poller remains unchanged.
tui_gateway/server.py:8726-8737concurrently removes and requeues completion events while its session runs, so the loop has no deterministic reservation of aninjectevent. No TUI regression is included.
Suggested changes
- Resolve the mid-loop user-message invariant with maintainers, then encode the approved contract in tests.
- Add TUI-aware inject deferral/reservation and a deterministic TUI delivery test.
Automated hermes-sweeper review.
| event_ids = [item[3] for item in accepted] | ||
| try: | ||
| synthetic_message = { | ||
| "role": "user", |
There was a problem hiding this comment.
This injects a synthetic user message into an active parent turn. AGENTS.md:88-91 explicitly requires strict alternation and says never to inject a synthetic user message mid-loop; this needs an approved design exception or a different delivery mechanism before merge.
There was a problem hiding this comment.
Thanks — the current wording of the invariant is clear. Before replacing this transport, could you advise which representation you want if same-turn delivery itself is acceptable?
I chose a typed user tail because, at an established assistant/tool boundary, it is the only portable cross-provider role that represents new external information. An assistant message would impersonate model output; a tool message requires a matching unresolved assistant tool call; and injecting system/developer content mid-loop would change the stable system/prompt-cache contract. The message is explicitly tagged _synthetic_delegation_inject / display_kind=delegation_inject and is only appended at guarded safe boundaries.
I also traced the cache behavior rather than assuming it: the follow-up at 1bd9aba61 now has a real two-provider-call regression asserting that request 2 starts with request 1 as a byte-for-byte identical message prefix and that _cached_system_prompt is unchanged. The inject appears only in the new tail; existing system/history bytes are not rewritten. Unconsumed tails are removed by durable event identity across compression copies.
If that append-only/cache-safe contract is acceptable, would you prefer (1) a narrow documented exception to the invariant for typed internal delegation input, plus a test forbidding any other synthetic-user mid-loop path, or (2) a specific existing internal event/input representation? The current /steer seam only attaches input to a tool result and does not cover provisional-final or no-tool boundaries, so I do not want to guess an alternative that silently breaks provider role validity or cache stability.
There was a problem hiding this comment.
One product-level point I undersold above: this is not a second delegation implementation or a workaround around the legacy path. It is one delegation rail with a model-selected delivery policy.
The top-level model sees the explicit result_delivery: inject | after_turn enum and chooses only the result timing; top-level child execution remains asynchronous regardless of the deprecated background argument. Both modes use the same child construction, executor, async registry, durable ledger, completion queue, formatter, claim/lease, recovery, and late-delivery machinery. after_turn remains the backward-compatible default.
For a single child, the semantic difference is deliberately narrow: if the result is ready during the originating turn, inject makes it model-visible after a complete tool-call/result block and before the next model request; after_turn makes the same completed result visible in a separate synthetic turn. If the inject misses the bounded same-turn window, it automatically follows that same late after_turn path — no wait, no dropped result, and no unrelated future turn contamination. For a batch there is one intentional additional benefit: inject children publish independently as they finish, while after_turn preserves the consolidated all-siblings result.
That gives a frontier model a useful per-delegation choice: independent work can stay after_turn; an audit, reviewer, or dependency that can invalidate the work in progress can use inject and influence the current answer. The execution and durability semantics do not fork. The exact schema/forwarding, incremental-inject batch, and consolidated-after-turn tests all pass.
Given those properties plus the append-only/cache-prefix proof, I think this is a strong architecture and that a narrow documented invariant exception for typed internal delegation input would be preferable to replacing it with a less valid provider role or a cache-sensitive system mutation. Does that direction make sense to you? If yes, I can codify the exception and its guard test rather than redesigning a working delivery rail.
|
Follow-up lifecycle audit fixes pushed in
Verification: The fork workflow is still expected to require maintainer approval; could a maintainer approve the new run when available? |
b88a7e5 to
1bd9aba
Compare
|
Addressed the TUI inject race in The TUI poller and active conversation loop now share a bounded completion-routing lock covering only dequeue → ownership decision → requeue/claim. The lock is released before any agent/model turn. This means an inject event cannot be temporarily held outside the queue while I also moved Regression evidence:
The new fork CI run is blocked at |
1bd9aba to
d1a2f47
Compare
|
Fixed a blocking The previous path emitted only the final parent aggregate, so one fast child could be hidden behind a slow sibling indefinitely. The invariant is now: at each available turn boundary, deliver exactly all completed-but-undelivered children from one batch in one coalesced envelope; never wait for unfinished siblings. Thus Durability remains child-scoped ( Verification on a fresh isolated |
d1a2f47 to
e37a042
Compare
|
Closed the two remaining delivery races in
Also corrected the stale docs: both Post-rebase verification: 581 passed in the isolated delivery/async/gateway/CLI/TUI matrix; |
|
CI for head |
|
Thanks for the enormous amount of engineering here — the ledger/lease/two-phase-ack machinery and the test coverage are genuinely impressive work, and this was reviewed carefully against current Closing on architectural grounds, not quality: Two pieces of this are worth extracting into their own PRs, and we'd welcome them:
If maintainers ever decide to relax the alternation invariant, this PR is the reference implementation and we'll point back to it. Thanks again — your other delegation fixes (#74858, #66401) are moving forward. |
|
Implemented the close review as a three-part stack:
The central distinction is causal, not textual. This does not create a second delivery implementation. A parent may have dependency/review work and independent work in flight as parts of one orchestration graph. The rejected representation has also been removed rather than defended:
The new carrier appends a clearly delimited This is orchestration semantics, not new user guidance or a model-preset project. Weaker models may use same-turn evidence poorly or drift toward the child topic; that is a bounded model-quality risk. The safe baseline remains unchanged: Each PR body now states its owned commit and standalone effect. All three exact remote heads remain fully green; #76230 contains the complete carrier, failure-ordering, and compatibility rationale. |
Summary
result_delivery: "inject" | "after_turn"todelegate_task, preservingafter_turnas the compatibility default/backgroundbehaviorresult_deliverythrough both the direct and ToolRegistry fallback dispatch pathsVerification
origin/main..HEAD: passed (9 files scanned)git diff --check: passedorigin/mainunder the same sandbox (cua_no_overlay, container boot/chown, XAI migration, service manager/chown, and macOS launcher)origin/mainin the focused comparison runzeroed_state_db, lazy-deps target, search error guard, Termux API detection) passed when rerun in isolation;zeroed_state_dbpassed 4/4 on both PR andorigin/mainorigin/main; one Kanban file timed out on both. The other timeout files remain unclassified, so the full-suite result is reported as partial RED evidence rather than waivedorigin/mainreproduce the cold-import timeout; restoring the lock-defined environment produced the final 81/81 focused resultCrash/recovery audit hardening
The follow-up audit found and fixed three durable-delivery defects:
unknownresult.Deterministic regressions cover crash-before-parent-update rollback, complete and partial batch restart recovery, pre-response interrupt release/requeue, persisted-history restart deduplication, provider acknowledgement, duplicate queue entries, and repeated formatter failure.
Additional verification
tests/agent/test_delegation_inject.py: 18 passedpy_compileon all changed Python files: passedCurrent head:
e37a042debf72d6ae3dc627e680d974c83b09497.Lease/lifecycle audit follow-up
A second adversarial review identified and fixed the remaining long-provider and rollback windows:
complete_event_delivery()/release_event_delivery()now return the actual SQLite transition result. RAM claims are removed only after a successful completion or an explicitly observeddeliveredrow; events are requeued only after a successful release that is confirmedpending.delegation_id:event_key, not Python object identity, so copied message dicts produced by compression cannot survive rollback.task:0..N-1key set; extra/malformed task rows cannot make a partial batch appear complete.AIAgent.run_conversationtransport assembly, normalize/ack success, and inject -> copied pre-API compression -> provider error -> release/requeue with heartbeat shutdown.Follow-up verification
tests/agent/test_delegation_inject.py: 26 passedgit diff --check, focused Ruff, andpy_compile: passedOne delegation rail, model-selected delivery
This is not a second delegation implementation. The top-level model sees the explicit
result_delivery: inject | after_turnenum and chooses only when an already-completed result should become model-visible. Child construction, isolated execution, async registry, durable event ledger, completion queue, formatting, claims/leases, recovery, and late delivery are shared.after_turnremains the backward-compatible default for independent work. At each available turn boundary it groups every currently ready, undelivered child into one synthetic result turn; unfinished siblings never block that ready-set and arrive in a later grouped turn.injectis for auditors, reviewers, and dependencies that can change the work in progress. A single result re-enters after a complete tool-call/result block and before the next model request. Batch children may re-enter independently as they finish.after_turnpath.This gives frontier models a per-delegation dependency choice without forking execution or durability semantics.
Reviewer follow-up: TUI inject reservation
main(4a798f4bce29).session.running=True.py_compile, andgit diff --checkgreen.After-turn ready-set invariant
after_turnbatch delivery no longer joins every sibling before publishing. At each available turn boundary, the consumer snapshots one batch's completed-but-undelivered child rows under the shared routing lock and produces exactly one transient envelope for that ready-set:task:0of 3 is ready,task:0is delivered immediately after the foreground turn;task:1andtask:2become ready before a later boundary, they are coalesced into the next result turn;The durable ledger remains child-scoped (
delegation_id + task:N). Group claim, renewal, acknowledgement, release, and drop are transactional: a consumer owns the whole ready-set or none of it. Busy TUI/gateway sessions requeue a re-coalescible envelope, so children that finish meanwhile join the next boundary. Finalization publishes a fully-ready set under the same routing lock; crash recovery restores exact pending child keys without reviving the aggregate parent or duplicating delivered children. Legacy aggregate rows already on disk retain their existing delivery path.Deterministic regressions cover
2/3 now + 1/3 later, all-ready coalescing, atomic group claim/ack, attempt-cap pruning without poisoning a pending sibling, composite requeue plus a newly-ready child, inject/after-turn partial recovery parity, finalizer enqueue locking, idle/busy TUI, idle/busy gateway, CLI drain, and forced-sync fallback isolation.py_compile, andgit diff --check: passede37a042debf72d6ae3dc627e680d974c83b09497Gateway routing and live-lease restart hardening
Commit
e37a042decloses the two remaining delivery races without changing the intentional ready-set contract:completion_routing_lockacross the complete bounded routing reservation: dequeue → ready-set coalesce → active-parent classification → requeue. Formatting and adapter/network delivery remain outside the lock. A deterministic post-dequeue pause test proves the active conversation-loop drain cannot observe a temporary-empty queue.ProcessRegistryowns one lazy, deduplicated heap/condition scheduler. Terminal duplicates are discarded; pending rows wake after the remaining lease plus a small strict-comparison guard, with no immediate-requeue spin. Heartbeat-renewed leases are reclassified and deferred again if still live.after_turnenvelopes prune already delivered/dropped siblings before retry and retain only pending child rows.after_turncreates only a transient ready-set envelope. Legacy aggregate rows remain readable.Verification after rebasing onto
524ab539947aa7a092d749921e0e93913cb683de:tests/agent/test_delegation_inject.py: 36 passed;py_compile, andgit diff --check: GREEN;