🤖 fix: deliver terminal wakes for kernel-launched background workflow runs - #3990
🤖 fix: deliver terminal wakes for kernel-launched background workflow runs#3990ibetitsmike wants to merge 38 commits into
Conversation
|
@codex review |
This comment has been minimized.
This comment has been minimized.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9665489578
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a19c5a304c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30a3437d0b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89263dd6b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59ce349492
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76e6e8070d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…defer drains on indeterminate currentness
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 046286a0d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
This comment has been minimized.
This comment has been minimized.
|
@codex review |
The whole coalesced prompt is handled under the single agentId passed to sendMessage, so batching current runs from different initiating agents would hand a restricted agent's attacker-influenced output to another agent's tool grants. Deliver one initiating-agent group per drain (newest launch first) and keep the other groups pending on the re-armed retry drain.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1b1954808
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two round-24 findings: (1) mixed drains applied the workflow group's initiating agent to coalesced workspace-turn and sub-agent sections, so those now resume under the conversation's own identity while agent-bound workflow groups defer to their own wake; (2) the restriction walk ignored synthetic launch rows carrying only a strict-agent pin, so the pin and the policy now resolve independently: the newest pin-bearing or manual row defines the pin, the newest policy-bearing or manual row defines the policy.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb313080a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This comment has been minimized.
This comment has been minimized.
Three round-25 findings: (1) an explicit background launch now fails loudly (run stays pending and resumable) when the pre-launch sidecar write fails, instead of starting a runner whose terminal wake would be permanently superseded; (2) transient run-store read failures (non-ENOENT fs errors) defer the terminal wake for retry instead of tombstoning it, while missing or unparseable runs stay superseded; (3) the launch turn's strict-agent pin is persisted with the run reference (null for verified-unpinned) and the wake re-pins the selected group's own provenance, because the newest pin-bearing history row can belong to a different group's wake and a mismatched pin would reject the wake on every retry.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: 8e1bcd6550
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
xum/src/node/services/taskService.ts
Lines 1728 to 1731 in d53ac1a
Use boundary IDs when rediscovering resumed workflows
When the clock moves backward after the latest manual/reset row and a kernel workflow_resume subsequently re-records provenance, the reference timestamp can remain at or below the row's timestamp, so this filter hides the active run even though its new afterBoundaryMessageId proves it was resumed after that boundary. At the stream-end callers this can let a task finalize while its workflow is still active. Fresh evidence beyond the removed retry-ordering path is that this remaining rediscovery gate still compares wall-clock values instead of the deterministic boundary identity.
AGENTS.md reference: AGENTS.md:L211-L211
xum/src/node/services/taskService.ts
Lines 7613 to 7614 in d53ac1a
Retain indeterminate startup wakes for retry
When startup finds a terminal run but its invocation currentness is temporarily indeterminate because history or the sidecar cannot be read, this boolean helper maps that state to false and the one-shot recovery skips creating an outbox notification. No defer timer exists without that notification, so recovery of the storage does not deliver the result until another app restart happens to rerun this scan. Use the tri-state currentness result here and retain indeterminate runs for retry.
AGENTS.md reference: AGENTS.md:L108-L113
xum/src/node/services/taskService.ts
Line 7616 in d53ac1a
Contain terminal-outbox failures during startup
When any workspace's terminal-attention path is unreadable or unwritable, enqueueIfAbsent throws here and the exception escapes recoverTerminalWorkflowRunAttentionNotifications, then TaskService.initialize and ServiceContainer.initialize. A single corrupt directory or permission failure can therefore prevent the application from starting; catch failures per run as the adjacent workspace-turn recovery does and continue startup.
AGENTS.md reference: AGENTS.md:L113-L113
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd1b200807
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: 578d72bf2b
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 578d72bf2b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
…sh-resumed services
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56171b714c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (this.onRunCrashResumed != null) { | ||
| try { | ||
| await this.onRunCrashResumed({ workspaceId: run.workspaceId, runId: run.id }); |
There was a problem hiding this comment.
When a kernel run remains active across a downgrade, the older build strips its boundary fields and can let the run become terminal—and tombstone its then-unrecognized wake—before the user upgrades again. Fresh evidence beyond the earlier downgrade thread is that getCrashRecoverableRun rejects this now-terminal run before this repair hook executes, so neither its provenance nor its stale outbox tombstone is repaired and the result remains permanently silent. Add an upgrade recovery path for already-terminal stripped references, including revival of the obsolete superseded notification.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6776f483e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce9085a89c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| onBackgroundRunTerminal: | ||
| options.onBackgroundRunTerminal ?? | ||
| (async (event) => { |
There was a problem hiding this comment.
When workflows.resume restarts a run whose prior workflow_run:<runId> notification is already delivered or superseded, this new default callback cannot enqueue the next terminal wake because enqueueIfAbsent preserves every existing status. Unlike the AIService workflow service, this router service's onRunStatusChanged only emits activity and never calls resetWorkflowRunTerminalAttention when the run becomes nonterminal, so the resumed result is silently omitted; reset the stable notification when the router-managed run restarts.
Useful? React with 👍 / 👎.
| await this.onRunCrashResumed({ workspaceId: run.workspaceId, runId: run.id }); | ||
| } catch (error) { | ||
| // Best-effort: an unrepaired reference defers its wake as indeterminate rather than | ||
| // losing it, so a failed repair must not block the resume itself. | ||
| console.error("Workflow crash-resume provenance repair failed:", error); |
There was a problem hiding this comment.
When the crash-resume boundary repair fails transiently, this catch restarts the runner anyway without retaining any repair work. Once the run becomes terminal, the repair hook no longer runs and terminal drains only reread the still-boundaryless reference, so they remain indeterminate every 30 seconds even after storage recovers. Unlike the fixed terminal-outbox enqueue retry, this provenance write still has no retry path; schedule a contained repair retry while the run is active rather than permanently stranding its wake.
AGENTS.md reference: AGENTS.md:L110-L113
Useful? React with 👍 / 👎.
| for (const message of messages) { | ||
| if (message.role !== "user") { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
When a context reset is followed by a synthetic turn with no caller restrictions—such as a scheduled heartbeat that launches a kernel workflow—this unbounded walk skips the assistant-role reset boundary and continues into the discarded context. The terminal wake then restores the pre-reset manual row's toolPolicy or disableWorkspaceAgents, so tools intentionally available to the new synthetic context can remain disabled and prevent the workflow result from being handled. Stop the restriction walk at the newest reset boundary and use fresh defaults when no post-reset row defines them.
Useful? React with 👍 / 👎.
1 participant
Summary
Background workflow runs launched from the code_execution kernel (
xum.workflow_run(...)) never delivered their terminal wake to the owning workspace: the run failed or completed, the notify_on_terminal attention was enqueued, and then silently dropped as "superseded". The owner stayed idle until a human prodded it. This PR makes the invocation-currency gate recognize kernel-launched runs via their durableagent-workflow-runs.jsonsidecar provenance, persists terminal-result consumption for kernel-nestedworkflow_resume, and stops dropping workflow wakes silently.Background
Incident: a 16-repo audit workflow launched from the kernel hit the runner-level
Execution interruptedfailure ~11 minutes in.WorkflowService.notifyBackgroundRunTerminalfired andTaskServiceenqueued the terminal attention, butbuildWorkflowTerminalPromptgates delivery onworkspaceService.isWorkflowInvocationCurrent, which only recognized:workflow_run/workflow_resumedynamic-tool parts (WORKFLOW_RUN_EMITTING_TOOL_NAMES), andworkflow-run-card-displaysynthetic messages, appended only by the slash-command path.A kernel launch leaves neither in history (the runId sits at
output.result.runIdof acode_executionpart, and only when the script happens to return it), so the backward history walk hit the prior real user message first and classified the run as superseded.drainTerminalAttentionthen marked the notification superseded without any log. The same gate is used by the aiService fallback sender and restart recovery, so nothing ever retried. Ironically,recordBackgroundWorkflowRunReferencealready persists exactly the needed provenance, documented as existing so a run's "terminal result re-engages the agent", but the gate never consulted it.Implementation
isWorkflowInvocationCurrentkeeps its backward walk (direct tool parts and card-display messages still win), and the sidecar fallback decides by row identity instead of wall-clock order:recordBackgroundWorkflowRunReferencesnapshots the message ID of the newest invocation-decision row (manual user/reset supersession, consumed terminal result for the run, or direct invocation part) intoagent-workflow-runs.json, and the run is current exactly when that row is still the newest decision row. One shared walk (findWorkflowInvocationDecisionRow) classifies rows for both record and check, so a backward clock correction can neither strand a legitimate wake nor let a pre-supersession reference outrank a newer boundary. Decision-free histories honor a verified-empty snapshot (null), so kernel launches from a new or fully cleared workspace keep their wake; entries without a snapshot (pre-upgrade, or a launch-time history read failure) stay indeterminate (deferred wake on the bounded retry cadence) rather than being ordered by wall clock, so a clock rollback can never promote them past a newer manual supersession.createdAtMs, the write path clamps future-dated timestamps, the parser rejects implausibly future-dated or boundary-corrupt entries and collapses duplicates to the newest sane record, and non-ENOENT read failures propagate (mapping to indeterminate currentness) instead of flattening to an empty list, so a transient fault can neither tombstone a wake nor let the record path clobber other runs' provenance.current/not_current/indeterminate; the terminal-attention drain defers indeterminate notifications on a bounded per-owner retry timer (an already-idle owner produces no further drain trigger on its own) and logs before marking a workflow notification superseded, so the next dropped wake is diagnosable.workflow_resume. Returning an already-completed run's result, or a foreground dispatch finishing terminal, persistsmarkWorkflowRunTerminalAttentionConsumed(astask_awaitalready does). Foreground consumption derives from the dispatch result itself, so a transient post-dispatch refresh failure cannot leave the delivered result armed for re-injection. Background dispatches never consume: their refresh can still observe the stale pre-dispatch terminal snapshot.disableWorkspaceAgentsflag from the newest restriction-defining user row, walking full history backward (a long assistant/synthetic tail cannot silently lift restrictions), and fails closed by deferring the wake when history is unreadable.workflow_resume/task_awaitconsumes the run.notifyBackgroundRunTerminalswallows callback rejections), so a transient outbox write failure would otherwise silence the wake until the next restart's sweep.enqueueWorkflowRunTerminalAttentionretains failed params and retries them in-process on the deferred-drain cadence;resetWorkflowRunTerminalAttentiondrops the retained entry so a reset run's stale wake cannot be resurrected by the retry.Review history note
Rounds 11-14 of the review cycle accreted a detached retry/repair layer around the sidecar (record retries with lifecycle generations, boundary repair timers, corrupt-node self-heal, removal drains, admission staleness probes, and persisted per-row delivery provenance). Each layer fixed the previous round's finding and produced new ones. Per maintainer decision, 95ca943 removed that machinery wholesale and keeps the core identity-based design above; the residual windows it covered (one possible redelivery of a genuine result after a crash between send acceptance and the outbox delivered mark, a wake lost only if the launch-time history read fails and no resume re-records, unbounded-but-tiny sidecar growth until a full clear, and manual repair for an externally corrupted sidecar path, and a foreground kernel resume whose terminal tombstone lands before the tool result persists: a crash in that window loses only the proactive wake, while the completed run stays retrievable via workflow_resume or task_await) are accepted and documented in the resolved review threads.
Validation
workflow_resumeconsumption marking each fail exactly the tests that guard them.disableWorkspaceAgentsrestoration, and reverting foreground consumption to the refreshed-record gate each fail exactly the new tests ("wake restriction restore walks past a long synthetic tail and carries the agent disable flag", "consumes the foreground terminal result even when the refresh read fails").make static-checkandmake typecheckpass; targeted suites (taskService, workspaceService, agentWorkflowRunReferences, workflow_resume, workflow_run, messageQueue, agentSession, workspaceRemoval) run clean apart from failures re-confirmed as pre-existing on this host at the unmodified base commit (bash-monitor-wake reconciliation and terminal-recovery flakes with tmp chmod/chown noise).Risks
Low-moderate. The gate change only widens delivery in cases that previously returned false at a boundary; direct-tool semantics are unchanged, and the widened path requires a sidecar reference, which only background
workflow_run/workflow_resumetool executions write. The machinery strip trades hardening for simplicity in windows that require a crash or storage fault at a precise moment; each accepted residual is fail-safe (redeliver a genuine result once, defer, or drop a wake without injecting anything into a superseding conversation).Generated with
xum• Model:anthropic:claude-fable-5• Thinking:xhigh