feat(runtime): observe background Bash through runtime refs - #547
Conversation
# Conflicts: # apps/desktop/src/main/main.ts
# Conflicts: # apps/desktop/src/main/main.ts # packages/cli/src/cli.ts
# Conflicts: # packages/runtime/src/ai-sdk-backend.ts
Astro-Han
left a comment
There was a problem hiding this comment.
Strong work on this. I ran the full merge-check against current main (typecheck, build, core/storage/runtime 878, desktop 2060) and it's all green. The implementation follows the #486 RFC invariants closely: runtime-owned lifecycle, bounded and redaction-safe output, conservative orphaned recovery, desktop and headless sharing one model. The yield-based auto-background plus the turn-tail summary is a nice touch. Short commands stay foreground transparently; long ones become observable without the model having to decide up front. I thumbs-up'd both #486 and #544. The direction is right and this is a solid realization of it.
The thing I want to settle before this merges is the tool surface. #486 Question 2 left the agent-facing surface open on purpose, and this PR picks the heaviest of the three options it listed: Bash plus three always-on control tools (ShellStatus, ShellWait, ShellCancel).
Tool schemas are a recurring tax, not a one-time cost. Every one of these sits in the system prompt for the whole session. Maka's pitch is economy, and we already built the load_tools / deferred-tools seam for exactly this reason: to keep the always-loaded surface small. Adding three shell-control tools to the default set cuts against that. We can't defer them either: if the model had to load_tools before it could check on a backgrounded run, the flow breaks (Bash backgrounds a run, then the model has to load tools before it can look at it).
Claude Code hit the same problem and solved it with one Bash tool and one boolean parameter, run_in_background. The rest reuses things that already exist: background output is a file, read with the existing Read tool; completion is a passive notification ("you'll be notified when it finishes, don't poll"); cancel goes through a shared TaskStop tool that the agent and task system already use. Net new shell-specific schema: roughly zero. No ShellStatus, because Read covers it. No ShellWait, because passive notification covers it, and saves the model from burning turns polling.
My suggestion for maka: keep this PR's good parts (yield-based auto-background, turn-tail summary that already nudges the model about running ShellRuns each turn) and drop the three control tools. Read covers status. The existing event stream plus the turn-tail summary cover "what's still running" and "when did it finish" without a dedicated wait tool. Cancel still needs a tool, but it can be one shared "stop background task" tool that future agent/cron/eval tasks will also want, instead of a shell-specific ShellCancel. That's three schemas down to one, or two if you'd rather keep a dedicated read-only observe tool. If we keep the current surface, several of the smaller issues below get worse; if we shrink it, most of them disappear.
Separate from the surface, one fix I'd want before this lands regardless. The old Bash tool defaulted to 120000ms. The new background path only installs a timeout timer when timeout_ms is passed explicitly, so a Bash call with no timeout_ms runs unbounded in the background until cancel or session end. One line to restore the default: timeout_ms ?? 120_000. Real regression from the old safety bound, cheap to fix.
Smaller things, as follow-ups. Several are moot if the surface shrinks:
- Abort during the initial yield window backgrounds the run instead of cancelling it. Old behavior killed the process. Worth a deliberate call. Probably kill during the yield window, keep after it backgrounds.
- ShellStatus lists only the first 8 actionable runs. Runs 9 through 64 have no discoverable id, and there's no limit/offset. Goes away if Status becomes Read.
- ShellWait doesn't pass the abort signal, so a long wait blocks up to 300s even if the user hits stop. Goes away if Wait becomes passive notification.
- Scope is broad: 39 files, including UI stories, a desktop fixture, and headless wiring. The headless share is minimal and probably has to ship with this; the UI and fixture bits might split out.
- PR body says 28 tests, the suite actually has 32. Minor.
Happy to pair on the surface reshape. The direction is right; this is about how much schema we pay for it.
|
@Astro-Han I agree with the main concern here: the runtime lifecycle model is useful, but the always-loaded agent-facing surface is probably too heavy. After re-reading the implementation and your comment, I think the PR should be reshaped around a smaller surface:
For observation, I am considering reusing
The Bash result for a backgrounded run would stay compact: status/cwd/command/ref, without stdout/stderr tail. The benefit here is context economy and explicit observation: long command output enters model context only when the model asks to read it. Internally I would keep For stop/cancel, I do not plan to introduce a new shared stop tool in this PR. Given #545/#558 are also actively shaping the broader automation/background-task model, it seems better not to preempt that shared contract here. I would rather leave cancellation out of this PR than ship another shell-specific tool that we already know should become shared. Scope-wise, I would keep this to desktop and CLI/TUI where Does this direction match what you had in mind? The two parts I would especially like your take on are the |
# Conflicts: # apps/desktop/src/main/main.ts # packages/cli/src/__tests__/runtime-bootstrap.test.ts # packages/cli/src/runtime-bootstrap.ts # packages/headless/src/__tests__/tools.test.ts # packages/runtime/src/__tests__/builtin-tools.test.ts # packages/runtime/src/builtin-tools.ts # packages/runtime/src/index.ts # packages/runtime/src/runtime-kernel.ts # packages/runtime/src/tool-runtime.ts
|
@M4n5ter This matches what I had in mind. Drop the three control tools, keep yield-based backgrounding, observe through Read. On your two questions: Q1 — Read resource-ref. Agree, and it's better than the disk-file idea we floated. I checked: Two things to pin down. One Read must return status + the bounded tail together; if stdout/stderr need a second hop we've rebuilt ShellStatus. And Skip the list ref for now. The turn-tail summary already surfaces actionable runs with refs each turn. Q2 — shared stop. I'd rather not defer this. The fallbacks don't hold: a timeout only fires at the deadline, so a run the model wants to stop now (wrong command, runaway loop, But your concern is fair. Compromise: ship the stop tool now with the generic shape, scoped to shell runs. With yield-window-abort-kills, every phase has a stop path. And #545/#558 are scheduling (timers injecting turns); the overlap with a live process is mostly "cancel by id," which is stable, so committing to the shape now is lower-risk than it looks. Net surface: |
|
@Astro-Han I reshaped the PR around your suggested surface. The current version removes I added coverage for a finalization edge case in this shape: a just-finished process should be observed as completed, not orphaned, even if the durable terminal write is still settling. Headless, subagent background control, PTY, and stdin remain out of scope here. |
|
@Astro-Han I want to explicitly call out one bounded-discovery tradeoff in the current The current turn tail lists only a bounded subset of actionable background tasks. Each The most likely cases are:
I think this is acceptable for the initial implementation. It is a bounded discoverability tradeoff rather than a data-loss or process-control issue: the initial If real usage shows that overflow discovery matters, the lowest-regret follow-up seems to be adding a bounded list resource under the same runtime namespace, without changing the existing single-task |
Astro-Han
left a comment
There was a problem hiding this comment.
LGTM. Verified the reshape against what we agreed: 120s default restored, abort-during-yield kills with a regression test, Read(ref) returns status + tail in one call, StopBackgroundTask is the generic stop path, and the just-finished finalization race has coverage with a delayed store. maka://runtime/* stays out of UI nav and subagent Read fails closed without the controller.
On the bounded-discovery tradeoff: agree it's acceptable for the initial cut. No data loss, no runaway (stop + default timeout + ref returned inline), and the list-resource follow-up doesn't change the Read/Stop contract.
Summary
Refs #486.
This PR keeps long-running non-interactive
Bashwork in the existing ShellRun lifecycle, but narrows the agent-facing surface to the runtime-resource shape discussed in review:Bashremains the single start path. Commands that finish insideyield_time_msreturn a normal terminal result.yield_time_mscontinue as background tasks and return a runtime ref such asmaka://runtime/background-tasks/<id>.Read(ref)observes a background task and returns status plus bounded stdout/stderr tails together.StopBackgroundTask({ ref })stops a background task by runtime ref.ShellStatus,ShellWait, andShellCancelare not part of the tool surface.Design
coreShellRunRecordas the durable internal model and uses ashell_runtool result with a requiredref.storageruntimeRead(ref)rendering, and generic stop-by-ref for shell background tasks.SessionManager/RuntimeKernelorphanedand injects bounded background-task metadata into the next turn tail.Behavior
Bashremains permission-gated.120_000mswhen no explicittimeout_msis supplied.yield_time_mswindow returns cancels the process as foreground work instead of leaving it behind.Read(ref)treats runtime refs as whole resources; file pagination options do not slice task metadata away from stdout/stderr tails.maka://router continues to rejectmaka://runtime/*.Scope Boundaries
Not included in this PR:
orphaned.Bash.Verification
Validated with runtime, CLI/TUI, headless, UI, and desktop URI-focused coverage relevant to this change.