Skip to content

feat(coding-agent): auto-deliver async bash job results to chat - #1622

Merged
flora131 merged 2 commits into
mainfrom
feat/async-bash-result-delivery
Jul 4, 2026
Merged

feat(coding-agent): auto-deliver async bash job results to chat#1622
flora131 merged 2 commits into
mainfrom
feat/async-bash-result-delivery

Conversation

@flora131

@flora131 flora131 commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

Background bash jobs (bash({ async: true })) previously finished silently: the job record was updated in a module-level map, and the result only reached the conversation if the model happened to poll __atomic_bash_job <id>. This ports oh-my-pi's AsyncJobManager (reference: can1357/oh-my-pi src/async/job-manager.ts + sdk.ts wiring) into Atomic so completed job output is automatically delivered back into the chat.

Design

  • New src/core/async/ module (job-manager.ts, session-manager.ts, format.ts, types.ts): delivery queue + retry loop, isDeliverySuppressed/acknowledgeDeliveries, max-running-job bounding, and upstream's process-singleton ownership semantics (first top-level session owns the manager; in-process sessions share it; dispose releases without clobbering other live sessions).
  • Delivery adapted to Atomic's session primitives: instead of porting oh-my-pi's yieldQueue, completions deliver via sendCustomMessage({ customType: "async-job-result", ... }, { deliverAs: "followUp", triggerTurn: true }) — queued at the next boundary while streaming, and starting a new turn when idle.
  • Duplicate suppression: polling __atomic_bash_job <id>, cancelling, and parent aborts acknowledge the delivery so the same completion is not auto-delivered twice.
  • Large output handling: results over 12KB inline a 4KB preview and persist the full output (job fullOutputPath).
  • The bash tool's async branch is extracted from bash.ts into a new bash-async-execution.ts (startAsyncBashCommand), so BashToolOptions now threads an optional asyncJobManager, asyncJobDeliveryHandler, and asyncJobSessionId through to it; existing agent-facing contract (start message, poll, cancel, details.async, TTL/retention) is preserved.
  • Registration-failure hardening: if registerBashJob fails after a managed job entry was created (disposed manager/session, or a capacity race mid-flight), the entry is now discarded instead of lingering as a permanently "running" zombie visible to __atomic_bash_job polls; the triggering tool-call error still surfaces unchanged.
  • Docs (docs/tools.md) and CHANGELOG.md updated; 20 new tests across async-job-manager.test.ts and agent-session-async-bash.test.ts.

Key changes

  • src/core/async/job-manager.tsAsyncJobManager: registration, capacity bounding, completion queue, delivery retry loop, suppression/acknowledgement tracking
  • src/core/async/session-manager.ts — process-singleton ownership: which session owns the shared manager, shared-manager lifecycle across forked/subagent sessions, safe disposal
  • src/core/async/format.ts, types.tsasync-job-result message formatting (inline preview vs. persisted full output) and shared types
  • src/core/tools/bash-async-execution.ts — extracted startAsyncBashCommand, now job-manager-aware (capacity check, registration, delivery handler wiring, abort acknowledgement); discards the managed job entry when registration fails
  • src/core/tools/bash.ts — async branch now delegates to startAsyncBashCommand; polling/cancelling a job acknowledges its delivery to suppress duplicate auto-delivery
  • src/core/agent-session*.ts — wiring to construct/share the AsyncJobManager per session and register the delivery handler
  • packages/coding-agent/docs/tools.md, CHANGELOG.md — documented the new auto-delivery behavior and the registration-failure fix

Validation

Implemented via a 6-loop ralph run with a 3-model review panel; final panel verdict was a unanimous "patch is correct" with stop_review_loop: true (confidences 0.93 / 0.86 / 0.90):

  • bun run typecheck, bun run lint, bun run check:file-length — pass
  • bun run test:unit — 2823 pass / 0 fail (the 2 bash-pty-native failures reproduce identically on clean origin/main; pre-existing native-PTY environment issue)
  • Live tmux TUI E2E (performed independently by two reviewers): started an async bash job, agent went idle, and the [async-job-result] follow-up auto-delivered the output and triggered a new turn

Follow-up addressed in this PR

A P3 edge case flagged in review — a zombie "running" job entry could linger until overflow eviction if registerBashJob threw after createManagedBashJob inserted into the module map (e.g. session disposed mid-flight) — is now fixed: the managed job entry is discarded when registration fails.

🤖 Generated with Atomic (Claude Fable 5)

Port oh-my-pi's AsyncJobManager (delivery loop, retry, suppression/
acknowledge, job bounding, process-singleton ownership) into
src/core/async/ so background bash jobs (bash({async:true})) propagate
their final output into the conversation as an async-job-result
follow-up (deliverAs: followUp, triggerTurn when idle) instead of
finishing silently until manually polled.

Polling __atomic_bash_job, cancelling, and parent aborts acknowledge
delivery to suppress duplicate follow-ups; results over 12KB inline a
preview and persist full output to the job's temp file. AgentSession
owns the singleton manager, releases it on dispose without clobbering
other live sessions, and the bash tool async branch is extracted to
bash-async-execution.ts.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@mintlify

mintlify Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
bastani 🟢 Ready View Preview Jul 4, 2026, 1:26 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@claude claude Bot changed the title feat(coding-agent): auto-deliver async bash job results back to the chat feat(coding-agent): auto-deliver async bash job results to chat Jul 4, 2026
…ails

registerBashJob can throw after createManagedBashJob has already
inserted the job into the module-level map (disposed manager/session
mid-flight, or a capacity race), leaving a never-executed zombie entry
permanently reported as "running" to __atomic_bash_job polls — TTL
cleanup only evicts settled jobs, so it lingered until max-jobs
overflow eviction.

Discard the managed entry (including any temp output) when
registration fails and rethrow the original error. Addresses the P3
review finding from the async-delivery review panel.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 4, 2026

Copy link
Copy Markdown

Review: auto-deliver async bash job results

Solid, well-tested port. The delivery/suppression semantics (poll acknowledges, cancel acknowledges, parent abort acknowledges, re-check at the streaming boundary) are carefully thought through, the extraction of startAsyncBashCommand out of bash.ts is a real readability win, and the 20 new tests cover the tricky paths (streaming boundary, stale-after-poll, shared-singleton routing, retention bounds, retry). Docs and CHANGELOG are updated. Note this is a static review — I could not check out the branch in this environment, so I did not re-run typecheck/tests myself.

Findings, roughly by severity:

1. The new running-job cap is process-global and shared across sessions

DEFAULT_MAX_RUNNING_JOBS = 15 (src/core/async/job-manager.ts) is enforced on the shared singleton, so all sessions, forks, and subagents in one process share a single 15-job budget — previously async jobs had no running cap at all. Worse, releaseSession suppresses a disposed session's running jobs but doesn't abort them, and atCapacity still counts them, so a disposed session's hung job (timeout can be up to 3600s) blocks other sessions from starting background jobs for up to an hour. Consider per-session accounting for the cap, or aborting (not just suppressing) running jobs on releaseSession, or at minimum documenting that the limit is process-wide.

2. Zombie "running" job when registerBashJob throws (acknowledged in PR body)

In startAsyncBashCommand (bash-async-execution.ts), createManagedBashJob inserts into the module-level managedBashJobs map before registerBashJob can throw (disposed session/manager). Since there are no awaits between them the window is narrow, but if it fires, the job's exec never starts and the entry stays "running" forever — TTL pruning skips running jobs, so it's only evicted at >100 overflow, and it stays pollable as perpetually "running". Cheap fix while you're here: call registerBashJob first, or wrap it in try/catch and delete the module-map entry on throw. Since you're already tracking this as a follow-up, fine to defer, but it's a two-line fix.

3. Owner fallback handler pins the first session and skips its disposal check

In createSessionAsyncJobManager (async/session-manager.ts), the singleton's onJobComplete is createSessionAsyncDeliveryHandler(session, manager)without a sessionId. Two consequences:

  • The static singleton's closure retains the first AgentSession (and its whole agent graph) for the manager's lifetime, even after that session is disposed, as long as any other session keeps the manager alive.
  • The fallback's isStale() never checks whether the owner session is disposed, so a job registered without a per-job handler can deliver into a disposed session.

In the current AgentSession wiring every job supplies a per-job handler, so this is latent — but SDK users who pass only asyncJobManager (no asyncJobDeliveryHandler) hit exactly this path. Passing the owner's sessionId into the fallback handler would close the staleness gap.

4. fullOutputPath in a delivered follow-up can point at a deleted file

The module map's cleanupManagedBashJobs deletes the temp file on TTL/overflow independently of the manager's delivery queue. A delivery that has been retrying toward the 30-minute TTL (retry backoff caps at 30s and never gives up while the job is retained), or an overflow eviction at >100 jobs, can land an async-job-result whose Full output: <path> no longer exists. Low likelihood, but the two retention systems (managedBashJobs vs the manager's #jobs) are now duplicated state with independent cleanup — worth a comment or eventual unification.

5. Delivery failures are swallowed silently

#startDelivery's .catch(() => { ... }) discards the error entirely. A permanently failing sendCustomMessage retries every ≤30s for up to 30 minutes with zero diagnostics, then the result is silently dropped when the job TTL-expires. A debug-level log on retry/give-up would make field issues debuggable.

Smaller points

  • 10ms polling for the streaming boundary (STREAMING_DELIVERY_POLL_MS): each pending delivery for a streaming session spins a 10ms setTimeout loop for the whole stream duration. Timers are unref'd so it's benign, but an event-driven wait (agent event subscription / turn-end hook) would be cleaner than polling.
  • Cross-session poll suppression: jobs live in the process-global managedBashJobs map, so session A polling session B's completed job acknowledges it and suppresses B's follow-up. The global map is pre-existing, but suppression makes it observable — worth knowing.
  • Churn in bash.ts: existing doc comments on unrelated BashToolOptions fields were rewritten or dropped, and declarations were comma-combined (const BASH_PREVIEW_LINES = 5, BASH_UPDATE_THROTTLE_MS = 100). This package tracks upstream pi, so gratuitous diffs cost merge friction later. Same for the "./session-manager.ts""./session-manager.js" import-extension flips in agent-session.ts/agent-session-methods.ts while sibling imports stay .ts.
  • truncateText slices by UTF-16 code units and can split a surrogate pair at the preview boundary, producing a lone surrogate in the message.
  • CHANGELOG: the entry is comprehensive (good, per CLAUDE.md) but is one ~150-word run-on sentence — splitting it into bullets would make it actually readable for users.

Security

Completed background-job output now auto-injects a follow-up message that triggers a model turn without the model asking. That modestly widens the prompt-injection surface: output of a long-running command (which may include fetched remote content) initiates agent action unprompted rather than arriving via an explicit poll. The suppression-on-cancel/abort behavior is the right mitigation for unwanted deliveries; just flagging the design tradeoff for awareness — it's inherent to the feature.

Test coverage

Coverage is strong overall. Gaps worth closing in the follow-up: the registerBashJob-throws zombie path (#2), retry give-up when the job TTL-expires mid-retries, and fallback-to-owner delivery after owner disposal (#3). Also, several tests rely on real 60–120ms sleeps to assert non-delivery, which can flake on slow CI — asserting on deliveryState()/retentionState() where possible would be more robust.

None of the above is blocking on its own; #1 (process-global cap semantics) is the one I would want a deliberate decision on before merge.

@flora131

flora131 commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the known P3 follow-up in 512b687: registerBashJob failures after the managed-map insert (disposed manager/session mid-flight, capacity race) now discard the never-executed job entry (including temp output) and rethrow, so no permanently-"running" zombie can linger in managedBashJobs. Added a regression test (discards the managed job when async registration fails on a disposed manager/session") plus a listManagedBashJobIds()observability helper, and a CHANGELOG entry. Validation: async-job-manager tests 18/18, agent-session-async-bash + bash parity + tools suites pass,typecheck/lint/check:file-length/root test:unit` (2823) all green.

@flora131
flora131 merged commit 8863da8 into main Jul 4, 2026
10 checks passed
@flora131
flora131 deleted the feat/async-bash-result-delivery branch July 4, 2026 01:33
@claude

claude Bot commented Jul 4, 2026

Copy link
Copy Markdown

Review: auto-deliver async bash job results

Overall this is a solid, carefully-adapted port. The delivery/suppression semantics are well thought through (poll/cancel/parent-abort acknowledgement, streaming-boundary staleness recheck, owner-vs-later-session lifecycle), the zombie-job discard fix is a nice catch, and the 20 new tests cover an impressive range of lifecycle edge cases. Docs and changelog are genuinely descriptive. Findings below, roughly by severity.

1. Delivery retries are unbounded and can duplicate the follow-up message

AsyncJobManager.#startDelivery (src/core/async/job-manager.ts:245-251) has no max-attempt cap — a persistently failing handler retries every ≤30s until the job falls out of the 30-minute TTL (~60 attempts).

More importantly, retry conflates "delivery failed" with "turn failed". On the idle path, the handler awaits session.sendCustomMessage(..., { triggerTurn: true }), which awaits _runAgentPromptagent.prompt(...) — i.e. the entire agent turn. If the turn rejects after the async-job-result message has been appended to conversation state (e.g. an auth/stream error mid-turn), the .catch re-enqueues and a retry appends the same message a second time and triggers another turn. Suggestions:

  • Cap attempts (upstream-style small constant) and mark the job acknowledged after the cap.
  • Consider treating "message appended to session" as delivery success, decoupled from the turn outcome — e.g. have the handler resolve once the message is enqueued/appended rather than when the turn completes.

2. Two jobs completing while idle can race into concurrent agent.prompt on the same session

#runDeliveryLoop (job-manager.ts:221-227) starts all ready deliveries concurrently. If two jobs complete while a session is idle, both handlers pass waitForStreamingBoundary (their continuations land on adjacent microtasks with isStreaming === false) and both take the idle triggerTurn branch in sendCustomMessage → concurrent _runAgentPrompt calls. This is only safe if agent.state.isStreaming flips synchronously inside agent.prompt() before its first await — worth verifying against pi-agent-core, and even then the window is delicate. Upstream's yieldQueue was inherently serial; here I'd serialize deliveries (per session, or globally — volume is low) rather than firing them all in parallel. A test with two simultaneous completions delivering to one idle session would pin this down.

3. Stale disposed singleton can throw inside the AgentSession constructor

createSessionAsyncJobManager (src/core/async/session-manager.ts:63-64) returns AsyncJobManager.instance() without checking disposed. The instance is only cleared when disposal goes through disposeSessionAsyncJobManager; any path that calls manager.dispose() / releaseSession() directly (your own afterEach in async-job-manager.test.ts:45 does exactly this — it works only because resetForTests() follows) leaves a disposed instance behind, and the next new AgentSession(...) throws from existing.registerSession(). Cheap hardening:

const existing = AsyncJobManager.instance();
if (existing && !existing.disposed) return { manager: existing, owns: false, sessionId: existing.registerSession() };

4. Fallback onJobComplete stays bound to the first session forever

The manager's default handler (session-manager.ts:66-68) closes over the owning session with no sessionId, so its staleness check can never detect that the owner was disposed. If the owner releases while later sessions keep the manager alive, any job registered without a per-job handler (the #deliveryHandlers.get(jobId) ?? this.#onJobComplete fallback at job-manager.ts:236) delivers into a disposed session. Today the tool registry always passes a per-job handler, so this is latent — but the fallback path targets a dead session by construction. Consider passing the owner's sessionId into the fallback handler, or dropping the fallback entirely and requiring a handler at registration.

Minor

  • Silent queue drop: #pruneRetention (job-manager.ts:202) shifts the oldest queued deliveries over maxRetainedJobs without suppressing or logging — the job then looks "delivered" to nobody. Unlikely at a cap of 100, but an acknowledge + debug note would make it deterministic.
  • Duplicated error text: in format.ts, a failed job with empty output renders the error twice — jobOutput returns (no output)\n\n${job.error} and statusLine adds Error: ${job.error}.
  • exitCode: null (signal-killed) reports completed: bash-async-execution.ts:69 carries over the pre-existing exitCode && exitCode !== 0 logic, so a signal-killed process auto-delivers as "completed" with no status line. Pre-existing, but the new auto-delivery makes it more visible; fine to defer.
  • 10ms polling in waitForStreamingBoundary: sendCustomMessage already queues deliverAs: "followUp" at the next boundary while streaming (agent-session-message-queue.ts:96-97), so the poll loop is mostly belt-and-braces for the fresh-turn-when-idle behavior. Works, but an event/subscription would avoid an indefinite (if cheap, unref'd) poll per pending delivery.
  • Cross-session suppression: job IDs live in a process-global map, so session B polling session A's settled job acknowledges (suppresses) A's pending auto-delivery. Consistent with the shared-manager model, just worth being aware of.
  • Unrelated churn in bash.ts: the rewording of existing BashToolOptions doc comments and the comma-joined const declarations (const BASH_PREVIEW_LINES = 5, BASH_UPDATE_THROTTLE_MS = 100;, similarly the asyncJobManager/... line) inflate the diff and read against the file's prior style; splitting declarations back out would keep blame history cleaner.

Test coverage

Coverage is strong on lifecycle/suppression. Gaps worth adding: (a) two simultaneous completions to one idle session (finding 2), (b) retry exhaustion behavior once a cap exists (finding 1), (c) polling a still-running job does not suppress later auto-delivery — the current tests only exercise the settled-poll suppression path.

Security / performance

No new security surface: outputs go to tmpdir with random suffixes (pre-existing pattern), delivered content is the same command output already reachable via polling. Capacity bounding (15 running jobs process-wide) is shared across parent + subagent sessions via the singleton — a subagent fan-out can starve on the parent's jobs; that's a defensible design choice but worth a note in docs/tools.md if intentional.

Note: I could not run bun install/tests in my sandboxed review environment, so this is a static review; the PR reports typecheck/lint/2823 unit tests green in CI. All new files are comfortably under the 500-line gate, vitest-style imports match this package's existing test convention, and the changelog follows the repo's [Unreleased] rules.

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