Skip to content

[BUG]: Run binding gaps in worker runs, the HITL gate, and withheld scratchpad output #791

Description

@justintime4tea

Summary

PR #719 (#628) moved per-run state onto the run, reached by tools and wrappers through the prepared agent's BoundRun slot. Review after merge found a few places where that binding is loose. None is reachable through the server today, because every caller prepares an agent per request and begins one run. Each is small, and together they close the gaps before anything reuses a prepared agent (#787).

  • A worker's run shares its parent's cancel token. RunContext::child hands the child parent.cancel.clone() rather than a child_token(), so cancelling a worker's run cancels the whole orchestration. Nothing calls stream on a worker or coordinator Agent today; if something did, that stream's drop guard would end the sibling workers and the client's stream.
  • The HITL gate reads its slot twice. HitlApprovalWrapper::pre_call takes run from the slot, then reads the slot again for the approval's request_id. If the slot is rebound in between, the approval carries one run's id but waits on another run's token. RequestApprovalTool::call uses one run for both.
  • Withheld output tells the model to retry. With no run bound, ScratchpadWrapper::transform_output withholds the output and tells the model to retry. The tool call has already run by then, so a side-effecting tool would run twice, and the retry is withheld too because the slot is still empty. read_artifact gives the same retry advice; the read has no side effects, but the retry is withheld as well.
  • Worker configs carry the coordinator's skill recorder. create_worker no longer clears worker_config.skill_recorder. Workers skip recording only because begin_run_within is passed None, so anything that built a worker through Agent::new or build_streaming_agent_with_tools would write task-scoped skill loads into the session.
  • A worker's HITL gate isn't bound by begin_run_within. A worker's gate and request_approval tool keep the run that was in scope when create_worker built them, because the worker's PreparedAgent has hitl_gate: None and hitl_approval_tool: None. A worker built outside the orchestration run would raise approvals that reach no observer. The warning logged then carries no context: it used to log the request id from the gate, and that id now comes from the run, which is what is missing.

Reported in the review of PR #719: #719 (comment) (items 5, 6, 7, 8), and Mike's questions on hitl/gate.rs:103 and read_artifact.rs:72.

Reproduction

Through the server, none of these can be reached today. Each can be reached in a unit test:

  1. Cancel token: RunContext::child(&parent, …), then child.cancel_token().cancel(). The parent is cancelled too.
  2. Double read: bind run A to a gate's slot, call pre_call, and rebind run B between the two reads. The approval carries B's id and waits on A's token.
  3. Retry advice: call a scratchpad-flagged tool through ScratchpadWrapper with an empty slot. The output is "…withheld… Retry the tool call." after the call has run.
  4. Recorder: read worker_config.skill_recorder in create_worker on a config that has a recorder. It is Some.
  5. Worker gate: build a worker outside any with_run scope with a conversational park route, then make a gated call. The approval events reach no observer, and the warning names nothing.

Additional Context

Proposed fix, one PR:

  • RunContext::child uses parent.cancel.child_token(). Nothing cancels a worker's run to stop its parent; the gate and tool only wait on the token. Extend a_child_shares_its_parents_identity_and_keeps_its_own_state: cancelling the child leaves the parent running.
  • HitlApprovalWrapper::pre_call takes request_id from the run it captured, as RequestApprovalTool::call does.
  • The no-run messages in ScratchpadWrapper::transform_output and read_artifact say the call ran and its output was withheld, with no retry instruction.
  • create_worker sets worker_config.skill_recorder = None again, with the reason; begin_run_within(…, None) stays as well.
  • create_worker sets hitl_gate and hitl_approval_tool on the worker's PreparedAgent, so start_run binds them as it does for a single agent. Approvals behave the same: the worker's run has the orchestration's id and observer, and is cancelled with it. Test: build a worker outside any run scope, begin its run, and check that a parked call's approval_requested reaches the orchestration run's observer.
  • For a gate used with no run at all, HitlApprovalWrapper::emit's warning logs the scope (run_id, task, session_id) and agent_name. emit is only called from park_pre_call, so the scope is always a worker's.

Once a worker's gate reads presence from the worker's own run (#786), child runs must share the parent's presence and claim; #781 covers that. Nothing reads those atomics yet, so this doesn't wait on it.

Searched Issues

  • No similar issues found

Code of Conduct

  • I agree to follow this project's Code of Conduct

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions