Repository navigation
feat(runtime-host): enable ShellRun event waits - #6206
chinawch007 wants to merge 3 commits into
Conversation
Verify managed background Bash provenance and deliver terminal facts through the existing Goal and Root admission flow. Add bounded wait projections, precise cancellation, and Desktop/CLI controls without taking ownership of background processes. Cover provider parity, real Bash and PTY flows, cancellation, process recovery, and cross-layer projection behavior. Generated-by: OpenAI Codex
Move event wait cancellation notices and goal usage into the TUI copy catalog with English, Simplified Chinese, and Traditional Chinese resources. Remove the stale legacy usage allowance. Generated-by: OpenAI Codex
Record the Button used by the goal event wait cancellation control in the generated surface inventory. Generated-by: OpenAI Codex
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Reviewed b4bbc67faa5df6fca89fcf76ed9b06996e098cc3 against main dc4515cd6e7637e8267d59cd4f71ad53f403f9e7. I found no confirmed P0–P2 defect.
The core behavior is demonstrated with real managed Bash processes, SQLite, the existing Goal and Root admission components, and the production AI SDK backend with a scripted model. Pending observations do not invoke the work model or Goal evaluator. My additional expiry and terminal scenarios each admitted exactly one continuation and left the command start count at one; expiry left the background process running. Disabling terminal observation made the terminal scenario fail while expiry still worked, so the completion check depends on the source adapter. Original tests also exercised unsuccessful terminal states, PTY, stale cancellation, source revocation and recovery after killing the owning process. Both unmodified SIGKILL recovery cases passed under the repository's specified Node 24.18.1.
Validation included a fresh install, dependency patches and normal clean root build; Desktop, Host and CLI type checks; renderer and Storybook builds; 589 client tests (586 passed, three skipped); the Host/storage group (360 passed, one skipped, with the two Node 22 harness failures separately passing under Node 24); and 121 neighboring tests (116 passed, four skipped, with one unchanged PTY drain test passing on isolated rerun after an initial failure). All three wait states completed their actual Storybook play assertions at 600, 720 and 1280 pixels in light and dark themes, including the cancellation explanation and reachable button. Architecture, surface inventory, headers, localization, TUI copy, formatting and protocol checks passed. The current test and windows_acp CI checks are green.
P3 integration note: this branch, #6176 and #6195 currently all use protocol epoch 226 while main remains 225. The first merged protocol change takes that epoch; later branches need the next unused value and renewed compatibility checks before merging. A separate P3 comment covers local test portability on Node 22.
The tests used a scripted model rather than a paid provider. Native Windows/macOS GUI behavior, an authenticated remote Host and long-running production workloads were not exercised locally. The 360-pixel browser experiment is below the Desktop's enforced 600-pixel minimum and is not used as a product finding.
| process.execPath, | ||
| [ | ||
| '--test', | ||
| '--test-isolation=none', |
There was a problem hiding this comment.
[P3] Keep the crash harness runnable on the supported Node 22 line
The package still declares ^22.19.0 || >=24.0.0, but the new child process argument --test-isolation=none is rejected by Node 22.23.2 before either crash fixture runs. Both new crash-recovery tests fail locally for that reason; the same unmodified tests pass under Node 24.18.1. CI uses Node 24, so this does not invalidate its green result or establish a production runtime failure. Make the harness compatible with the declared contributor runtime, or state/enforce that these tests require Node 24.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Reviewed range: dc4515cd..b4bbc67f, i.e. merge-base with main (the current main tip) up to head b4bbc67faa5df6fca89fcf76ed9b06996e098cc3. That is three commits: c5a56cf feat, 3808356 CLI localization, b4bbc67 docs inventory. This is a first review, so there are no prior findings to track.
Verdict: I found no P0–P2 defects. There are four P3 items.
What I checked
-
Source authorization (
ShellRunEventWaitAccess). A wait is accepted only when all of these hold:- the ShellRun belongs to the same Session in the same Host stores, and its visibility is not
user; - the ShellRun has a source Run whose invocation is a runtime/ai-sdk route on the same Turn;
- exactly one immutable, non-partial Bash function_call, T1 dispatch and T2 outcome exist, in that order, with matching ids, args hash and operationId,
run_in_background: true, and ashell_runresult whose ref matches the wait.
The live record is re-read after the proof reads. Registration additionally rejects
code_modeand non-ai-sdk invocations. The wait never runs, restarts or kills a command. Cwd, env and sandbox stay owned by the original Bash dispatch, and expiry or cancellation leaves the task running, as documented. - the ShellRun belongs to the same Session in the same Host stores, and its visibility is not
-
Storage.
readToolProvenanceEventshas matching predicates in SQLite and Memory: in practice only function_call/function_response carrycontent.id, and both also matchrefs.toolCallIdandtoolDispatch.providerToolCallId. Both stores measure the budget before decoding, and both are covered by the dual-provider test. -
Restart behaviour. A task that was running when the Host died recovers as
orphaned, which maps tosource_unavailablewithout a re-dispatch. A task that was already terminal recovers assatisfied. The receipt key is a digest of the terminal fact, so delivery is idempotent through the existing #6147 delivery key. -
Interaction with #6147.
pausenow also requires the wait'swaitIdto still be the active gate, so a stale observer cannot pause a newer wait.- Admission-time revocation now persists a specific diagnostic.
goal.event-wait.cancelis serialized on the Session admission lane plus the Goal mutation lane. It is fenced on goalId, generation, waitId and goal revision, and it reportsalready_admitted/already_cancelledcorrectly.- The projection hides the wait once it is delivered or cancelled, or the Goal is no longer active.
- A plugin cannot shadow
WaitForEvent, and the tool is hidden unless the Goal is active or waiting and the mode is not code mode.
-
Output and prompt bounds. The continuation prompt carries only refs, never task output, and stays within 4000 chars / 12000 bytes after escaping. The projection's resourceId/reasonCode are truncated with
truncateUtf8, whose output (marker included) fits the decoder limits. The worst-case snapshot reservation includes the wait summary.
Findings
- P3 – protocol epoch collision. The epoch goes from 225 to 226. Open PRs #6176 and #6195 also use 226, so whichever merges later must re-bump. The change also adds no
// 226:history line above the 225 entry (packages/runtime-host/src/protocol/index.ts:111). - P3 – crash-recovery test fails on Node 22.
goal-root-authority.test.ts:3326spawns its fixture with--test-isolation=none. Node 22.23.2 rejects that flag (bad option), andengines.nodeallows^22.19.0.- Locally, both "ShellRun process crash recovery from {running,terminal}" tests fail on Node 22. They pass when the flag is switched to
--experimental-test-isolation=none. - CI uses Node 24 and is green. Pick the flag based on
process.version, or skip on Node 22.
- P3 – repeated provenance reads on every poll (performance; inferred from the code, not measured). Each observation calls
authorizeand thenadapter.read, and each of those runs the fullShellRunEventWaitAccess.read. That is per poll: readRunInvocation, twojson_extractscans over every row of the source Run, and tworeadShellRun. The proof is immutable once complete, so it could be verified once per wait and only the live ShellRun binding rechecked afterwards. At a 15 s poll interval this is acceptable, but cost grows with the size of the source Run's history. - P3 – misplaced JSDoc. In
packages/cli/src/session-driver.ts:249,cancelGoalEventWaitwas inserted between thecontrolGoalJSDoc andcontrolGoalitself, so that doc now attaches to the wrong member.
Tests run (local, Node 22.23.2, built core/storage/mcp/runtime/runtime-host only)
- runtime-host suites (shell-run-event-wait, goal-root-authority, event-wait-manager, goal-protocol, goal-coordinator, interactive-run-composer, canonical-session-projection): 411 pass, 1 skip, 2 fail. The 2 failures are finding 2 and pass with the flag adjusted.
- storage
tool-provenance-query: 2/2 pass. - runtime
shell-run-manager: one failure on the first run, then clean on rerun. I attribute this to load from a concurrent build.
CI and mergeability: test passed (22m37s) and windows_acp passed. The PR is mergeable; merge state is BLOCKED only on required review.
| // interoperability. Mismatches are rejected before domain commands are admitted. | ||
| // Re-check against main before merging; sibling PRs may use the same next epoch. | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 225 as const; | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 226 as const; |
| process.execPath, | ||
| [ | ||
| '--test', | ||
| '--test-isolation=none', |
There was a problem hiding this comment.
P3: Node 22 (allowed by engines as ^22.19.0) rejects --test-isolation=none with bad option, so both crash-recovery tests fail locally on Node 22.23.2. They pass with --experimental-test-isolation=none. Pick the flag based on process.version.
| invocation.opening.route.backendKind !== 'ai-sdk' | ||
| ) | ||
| return { kind: 'revoked', reason: 'shell_run_provenance_invalid' }; | ||
| const proof = await this.events.readToolProvenanceEvents( |
There was a problem hiding this comment.
P3 (performance, inferred from the code, not measured): every poll runs this full proof twice, once from authorize and once from adapter.read. Each run does two json_extract scans over the whole source Run. The proof is immutable once complete, so it could be cached per wait, with only the live ShellRun binding rechecked on later polls.
| * concurrent control action mid-flight — for clear that is the desired end | ||
| * state). Optional: drivers without a goal authority reject goal control. | ||
| */ | ||
| cancelGoalEventWait?( |
There was a problem hiding this comment.
P3: This member was inserted between the controlGoal JSDoc and controlGoal, so that doc now attaches to cancelGoalEventWait.
Heads-up on the Runtime Host compatibility epoch: #6176 has merged, so |
This branch has not been deployed
2 participants
Summary
Enables an active Goal using AI SDK direct tools to wait for managed background Bash without repeated work-model or Goal-evaluator calls. The source Turn yields, and persisted terminal facts resume the same Goal through existing Root admission.
Refs #5926 (PR4 implementation plan).
Refs the original discussion #5920.
Verification
format:checkincludes unrelated untracked local investigation scripts and fails on those files; they are excluded from this PR. Fullnpm test, Windows/Linux runs and Electron E2E were not run.Protocol compatibility
The new Goal projection and cancellation operation advance Runtime Host compatibility epoch 225 → 226. Snapshot preflight reserves the wait summary within the existing frame limit, reducing the remaining capacity for queued content. No storage schema migration is required.
AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex contributed implementation, tests, documentation and review assistance. The commit retains
Generated-by: OpenAI Codex.Checklist
The second item remains unchecked because of the root format-command limitation above; tracked-file formatting and the other listed checks passed.
Does this PR entail a change in behavior?