Skip to content

🤖 fix: deliver terminal wakes for kernel-launched background workflow runs - #3990

Open
ibetitsmike wants to merge 18 commits into
mainfrom
mike/workflow-kernel-terminal-wake
Open

🤖 fix: deliver terminal wakes for kernel-launched background workflow runs#3990
ibetitsmike wants to merge 18 commits into
mainfrom
mike/workflow-kernel-terminal-wake

Conversation

@ibetitsmike

@ibetitsmike ibetitsmike commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

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 durable agent-workflow-runs.json sidecar provenance, persists terminal-result consumption for kernel-nested workflow_resume, and stops dropping workflow wakes silently.

Background

Incident: a 16-repo audit workflow launched from the kernel hit the runner-level Execution interrupted failure ~11 minutes in. WorkflowService.notifyBackgroundRunTerminal fired and TaskService enqueued the terminal attention, but buildWorkflowTerminalPrompt gates delivery on workspaceService.isWorkflowInvocationCurrent, which only recognized:

  1. direct workflow_run / workflow_resume dynamic-tool parts (WORKFLOW_RUN_EMITTING_TOOL_NAMES), and
  2. workflow-run-card-display synthetic messages, appended only by the slash-command path.

A kernel launch leaves neither in history (the runId sits at output.result.runId of a code_execution part, 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. drainTerminalAttention then 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, recordBackgroundWorkflowRunReference already 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

  • isWorkflowInvocationCurrent keeps 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: recordBackgroundWorkflowRunReference snapshots the message ID of the newest invocation-decision row (manual user/reset supersession or consumed terminal result for the run) into the sidecar, 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. Equality against a consumed-result row means a background resume/retry was recorded after the prior delivery, whose next terminal wake must not be dropped. Decision-free histories honor a verified-empty snapshot: a reference recorded with a null boundary against a still decision-free history is current, so kernel launches from a new or fully cleared workspace (e.g. a heartbeat turn) keep their wake; every other surviving reference fails safe, and a full clear (truncateHistory), which removes every row without appending a reset boundary, now also retires the sidecar durably BEFORE the truncation (like the r41 pre-truncation retry discard), so no crash window exists in which the transcript is gone but the sidecar survives; a retirement failure aborts the clear with the transcript intact, a corrupt directory at the sidecar path is removed recursively instead of bricking every clear, so a pre-clear null snapshot cannot masquerade as a fresh empty-history launch and inject its result into the cleared conversation (a post-clear resume re-records provenance). Entries without a boundary snapshot (written by pre-upgrade versions, or after a record-time history read failure) take a wall-clock migration fallback against a datable boundary so an in-flight run keeps its wake across the upgrade; the fallback population shrinks as new records and resumes snapshot identities.
  • recordAgentWorkflowRunReference keeps the newest createdAtMs across re-records (was oldest) so a kernel workflow_resume issued after a newer user message re-establishes provenance, matching how a direct workflow_resume tool part already re-establishes currency through the history walk. createdAtMs now feeds only the pre-existing listAgentReferencedWorkflowRunIds rediscovery listing, which does not gate terminal-result injection: the write path clamps future-dated input to Date.now(), the parser rejects persisted entries beyond a bounded future skew (1 hour) as corruption while keeping within-tolerance entries, and duplicate per-run entries collapse to the newest sane record so order-sensitive consumers cannot pick a stale duplicate.
  • workflow_resume durably persists terminal-result consumption (markWorkflowRunTerminalAttentionConsumed, as task_await already does) when it returns an already-completed run's result or a foreground dispatch finishes terminal, so a kernel-nested resume cannot trigger a redundant second terminal-result turn. Foreground-only: a background dispatch can still observe the stale pre-dispatch terminal snapshot, and consuming it would tombstone the retried run's future wake.
  • Read failures are never persisted or tombstoned as supersession: the boundary snapshot throws on an unreadable history (recording a rediscovery-only entry) instead of persisting a verified-empty boundary, currentness is three-state (current / not_current / indeterminate), and the terminal-attention drain defers indeterminate notifications for a later drain trigger rather than permanently marking them superseded. A deferred drain arms a bounded per-owner retry timer, because an already-idle owner produces no further drain trigger on its own. Sidecar reads distinguish failure from absence: non-ENOENT read failures propagate and map to indeterminate instead of flattening to an empty list (parse corruption stays self-healing), the record path refuses to rewrite contents it could not read so other runs' provenance survives a transient fault (a failed record schedules bounded background retries that supersede older provenance but never a strictly newer record; a full clear or workspace removal cancels and drains them, a lifecycle generation refuses chains rescheduled during the drain, and a failed empty-history boundary snapshot gets a bounded repair that waits for the entry to land and persists null only while history is still verified-empty), and the parser rejects entries whose boundary snapshot is present but invalid rather than migrating them into the legacy wall-clock fallback. The drain persists deliveredWorkflowRunIds onto the accepted row (a new MuxMetadata field threaded through send/queue dispatch), and the currentness walk recognizes consumption from that backend-controlled provenance only, so a crash between durable acceptance and the outbox delivery mark cannot replay the result, and user-controlled synthetic text quoting a run ID cannot spoof consumption. Rediscovery listings propagate an unreadable sidecar so completion-gating callers (turn blockers, stream-end reconciliation, report finalization) defer instead of finalizing while a kernel workflow may still run. Startup recovery consults three-state currentness and skips only not_current, so a transiently unreadable history/sidecar at restart enqueues the notification and lets the drain defer with its bounded retry instead of stranding the run until another restart. Drain sends carry an admissionStale probe bound to the context-mutation epoch captured before prompt validation, so a full clear racing the drain refuses the stale send and the next drain re-evaluates.
  • drainTerminalAttention logs a warning before marking a workflow notification superseded, so the next dropped wake is diagnosable from logs.

Validation

  • Red-green on every behavior independently: neutering the identity comparison, dropping the record-side boundary snapshot, treating legacy entries as current, restoring consumed-is-final, reverting newest-record-wins to oldest, removing the write-path clamp, replacing future-entry rejection with a per-read clamp, dropping the backward-clock skew tolerance, preserving duplicate sidecar entries, letting decision-free history accept sidecar references without a verified-empty snapshot, treating decision-free history as unconditionally stale, accepting present-but-invalid boundary snapshots as legacy, flattening sidecar read failures to empty, dropping the defer-retry arming, and neutering the workflow_resume consumption marking each fail exactly the tests that guard them. The new tests include the clock-rollback scenario in both directions (a skewed future reference with a matching boundary row still delivers; a newer boundary with a smaller timestamp suppresses), the indeterminate/defer path (unreadable history keeps the notification pending; the boundary snapshot throws instead of persisting null), legacy migration (pre-snapshot entries deliver via the wall-clock fallback, newer boundaries supersede, undatable boundaries fail safe), the sidecar fault path (a directory at the sidecar path yields indeterminate, not superseded), the bounded defer retry (a transient fault defers the first drain and the armed retry delivers), full-clear retirement (skipping the sidecar retirement makes the stale reference read current again), retirement ordering (a failing post-clear carryover discard leaves the reference retired), record-path preservation (an owner-unreadable sidecar rejects the record instead of clobbering peers), coalesced-consumption recognition (the accepted drain prompt supersedes the pending wake; a different run's payload does not), record retries (the reference lands once storage recovers, preserving peers), indeterminate startup recovery (the notification stays enqueued instead of being skipped), retry lifecycle governance (a resume record survives a pending stale retry; a full clear leaves the sidecar empty through the retry window), pre-truncation retirement (a failing retirement aborts the clear with history undeleted), the drain admission probe (false at the captured epoch, true after it advances), boundary repair (a still-empty history regains its null snapshot; a decision row at repair time stays fail-safe; a missing entry stays retryable until the record retry lands it), removal drain (a pending retry cannot recreate a deleted session directory), corrupt-sidecar self-heal (a directory at the sidecar path clears successfully), retry supersession (older provenance is replaced, strictly newer wins), the drain-window generation latch (a pre-cancel chain never re-arms), sidecar-gated finalization (an unreadable sidecar reports blockers), provenance-based consumption (spoofed text and foreign run IDs do not count; persisted provenance does), and caller tool-policy restoration (a restricted launch turn's policy reaches the wake send; a policy-less newest manual row restores nothing).
  • Failing tests already failing at origin/main on this host (bash monitor wakes reconciliation in workspaceService.test.ts, 3 terminal-recovery tests in taskService.test.ts) were re-confirmed as pre-existing by running both suites at the base commit; this diff adds no new failures.

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_resume tool executions write. The consumed-boundary override is conditioned on a strictly newer reference, and duplicate delivery after in-turn consumption is suppressed by the durable consumption tombstone. The Math.minMath.max change also affects listAgentReferencedWorkflowRunIds: a resumed run's provenance now survives a newer user message, which makes stream-end rediscovery consistent with the equivalent direct-tool flow but slightly broadens which runs are auto-awaited after supersession.


Generated with xum • Model: anthropic:claude-fable-5 • Thinking: xhigh

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/node/services/workspaceService.ts Outdated
Comment thread src/node/services/agentWorkflowRunReferences.ts
Comment thread src/node/services/workspaceService.ts Outdated

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/node/services/agentWorkflowRunReferences.ts Outdated

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/node/services/workspaceService.ts Outdated
Comment thread src/node/services/workspaceService.ts Outdated

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/node/services/agentWorkflowRunReferences.ts Outdated
Comment thread src/node/services/workspaceService.ts Outdated
Comment thread src/node/services/tools/workflow_resume.ts
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/node/services/agentWorkflowRunReferences.ts
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/node/services/workspaceService.ts Outdated
Comment thread src/node/services/workspaceService.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/node/services/taskService.ts Outdated
Comment thread src/node/services/workspaceService.ts Outdated
Comment thread src/node/services/workspaceService.ts
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 60bb82d89f

ℹ️ 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".

Comment thread src/node/services/workspaceService.ts
- Reject sidecar entries whose afterBoundaryMessageId is present but
  invalid (empty string or non-string) instead of migrating them into
  the wall-clock legacy fallback, where they could outrank a newer
  boundary during tolerated clock skew.
- Propagate non-ENOENT sidecar read failures instead of flattening
  them to an empty list; workflow currentness reports indeterminate so
  the terminal drain defers rather than tombstoning the wake, while
  rediscovery listings skip the pass and record keeps its atomic
  rewrite.
- Arm a bounded per-owner retry timer when a drain defers on
  indeterminate currentness: an already-idle owner otherwise produces
  no further drain trigger until restart.
A verified-empty (null) boundary snapshot recorded before a full clear
is indistinguishable from one recorded after it, because the clear
removes every row without appending a reset boundary. A pre-clear
reference could therefore inject its workflow result into the freshly
cleared conversation. Retire the sidecar with the transcript, durably
like the post-compaction carryover discard; a post-clear resume
re-records provenance.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 983d3c5eac

ℹ️ 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".

Comment thread src/node/services/workspaceService.ts Outdated
Comment thread src/node/services/agentWorkflowRunReferences.ts Outdated
Comment thread src/node/services/taskService.ts
… clobber, coalesced replay

- Retire kernel workflow run references immediately after the
  truncation commits, before post-clear steps (goal acknowledgment,
  carryover discard) that can fail and return early while leaving the
  transcript deleted.
- Propagate a sidecar read failure out of recordAgentWorkflowRunReference
  instead of treating the file as empty: the atomic rewrite would
  replace valid-but-unreadable contents with only the new run,
  destroying every other run's durable provenance. The failed record is
  retryable; parse corruption still self-heals.
- Recognize coalesced terminal-attention prompts as consumption during
  currentness checks: the drain's synthetic user row carries no
  workflow-result metadata, so a crash between durable acceptance and
  the outbox delivery mark would otherwise replay the same terminal
  result after restart. Payload blocks are parsed back and matched on
  the exact workflow.runId the builder wrote.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 56c789b44d

ℹ️ 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".

Comment thread src/node/services/agentWorkflowRunReferences.ts
Comment thread src/node/services/workspaceService.ts
Comment thread src/node/services/workspaceService.ts Outdated
…ar-emit ordering

- A failed provenance record now schedules bounded background retries
  (1s/10s/60s, reusing the launch-time boundary snapshot): the only
  natural re-record sites are a new dispatch and workflow_resume, which
  an untouched active run never hits, so a single failed write would
  permanently supersede its wake once storage recovers.
- Startup recovery keeps indeterminate runs: it now consults three-state
  currentness and skips only not_current, because no pending
  notification exists yet to arm the drain's defer retry; the drain
  re-evaluates and defers or supersedes with full context.
- A failed sidecar retirement after a committed full clear now emits the
  DeleteMessage before returning the cleanup error, so the renderer
  does not keep showing a transcript that no longer exists on disk.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 521fba2b77

ℹ️ 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".

Comment thread src/node/services/tools/toolUtils.ts Outdated
Comment thread src/node/services/workspaceService.ts Outdated
Comment thread src/node/services/taskService.ts
- Detached record retries are lifecycle-governed: they only fill
  absence (onlyIfAbsent), so a later dispatch or workflow_resume record
  wins, and a full history clear cancels the sidecar path's pending
  retries (with a registry-identity guard against a raced timer), so a
  stale retry can neither overwrite newer provenance nor resurrect a
  retired reference.
- Sidecar retirement now happens durably BEFORE the truncation, like
  the r41 retry discard: no crash window exists in which the transcript
  is gone but the sidecar survives, and a retirement failure aborts the
  clear with the transcript intact instead of returning a partial
  cleanup error after commit.
- Terminal-attention drain sends carry an admissionStale probe bound to
  the context-mutation epoch captured before prompt validation, so a
  full clear between the currentness check and send admission refuses
  the stale workflow result instead of injecting it into the cleared
  conversation; the refused send leaves notifications pending.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3eb19a5229

ℹ️ 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".

Comment thread src/node/services/tools/toolUtils.ts
Comment thread src/node/services/agentWorkflowRunReferences.ts Outdated
Comment thread src/node/services/agentWorkflowRunReferences.ts Outdated
… drain, corrupt-dir self-heal

- A failed record-time boundary snapshot on a decision-free launch now
  schedules a bounded background repair: if history is still verified
  empty at repair time it persists the null snapshot the decision-free
  currentness branch requires (rows cannot disappear outside a full
  clear, which retires the sidecar), while a decision row seen at
  repair time keeps the entry boundary-less and fail safe because it
  may postdate the launch.
- Workspace removal cancels and drains detached sidecar maintenance
  before deleting the session directory, so a late retry cannot mkdir
  it back into existence; the registry now tracks in-flight writes and
  the full clear drains through the same path.
- clearAgentWorkflowRunReferences removes a directory at the known
  sidecar path recursively: force alone refuses directories, which
  previously failed every subsequent full clear identically with no
  self-heal.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 62dd29073a

ℹ️ 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".

Comment thread src/node/services/agentWorkflowRunReferences.ts Outdated
Comment thread src/node/services/agentWorkflowRunReferences.ts
Comment thread src/node/services/tools/toolUtils.ts
Comment thread src/node/services/taskService.ts Outdated
Comment thread src/node/services/workspaceService.ts Outdated
…tion latch, repair ordering, sidecar-gated finalization, persisted drain provenance

- Detached record retries skip only when a STRICTLY NEWER record exists,
  so a failed workflow_resume re-record can supersede the stale dispatch
  entry it was meant to replace while a newer successful record still
  wins.
- Cancellation bumps a per-path lifecycle generation before clearing
  timers; registration refuses stale generations, so a retry rescheduled
  from a failing write's catch handler during the cancellation drain
  cannot re-arm and recreate retired state.
- Boundary repair treats a missing reference as retryable (the record
  retry lands it later) instead of a satisfied no-op that would leave
  the eventual entry permanently boundary-less.
- listAgentReferencedWorkflowRunIds propagates an unreadable sidecar;
  the three completion-gating callers defer (assume blockers, skip
  stream-end reconciliation, keep the task running) instead of
  finalizing while a kernel workflow may still run.
- The drain persists deliveredWorkflowRunIds onto the accepted row
  (MuxMetadata) and currentness recognizes consumption from that
  provenance instead of row text, which user-controlled synthetic
  content (e.g. a heartbeat body) could spoof to suppress a real wake.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Security Review

Here are some automated security review suggestions for this pull request.

Reviewed commit: 6866c5bcb2

ℹ️ 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.

Comment thread src/node/services/workspaceService.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6866c5bcb2

ℹ️ 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".

Comment on lines +3550 to +3552
...(internal?.deliveredWorkflowRunIds != null && internal.deliveredWorkflowRunIds.length > 0
? { deliveredWorkflowRunIds: internal.deliveredWorkflowRunIds }
: {}),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Carry delivery provenance through on-send compaction

When a terminal wake triggers on-send compaction, the annotated userMessage is explicitly not persisted; instead, a compaction request containing CompactionFollowUpRequest is written and the prompt is redispatched later. That follow-up does not carry deliveredWorkflowRunIds, so a crash after the compaction request is durable but before onAccepted marks the notification delivered leaves both a pending notification and a durable follow-up. Restart recovery can consequently deliver the same workflow result twice. Preserve this provenance in the durable follow-up and restore it into the redispatched send.

AGENTS.md reference: AGENTS.md:L110-L112

Useful? React with 👍 / 👎.

Comment on lines +99 to +100
/** Persisted onto the dispatched user row; see MuxMetadata.deliveredWorkflowRunIds. */
deliveredWorkflowRunIds?: string[];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Retain delivery provenance across queue dispatch

When the terminal drain's fallback send races another turn and is queued, this option is accepted here but is never copied into QueueEntry or reconstructed by dequeueNext(). The dispatched user row therefore lacks deliveredWorkflowRunIds; if the process exits after that row is appended but before the queued onAccepted callback marks the outbox delivered, restart currentness cannot recognize consumption and replays the terminal result. Store and return the field with the queued entry.

AGENTS.md reference: AGENTS.md:L110-L112

Useful? React with 👍 / 👎.

Comment on lines +8292 to +8294
const admissionEpoch = this.workspaceService.getContextMutationEpoch(ownerWorkspaceId);
const sendAdmissionStale = () =>
this.workspaceService.getContextMutationEpoch(ownerWorkspaceId) !== admissionEpoch;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Revalidate after intervening manual turns

If a short or pre-stream-failing manual turn starts after workflow currentness is checked and finishes before this synthetic send reaches admission, the busy/preflight guards no longer see it and this probe remains false because manual sends do not advance contextMutationEpoch. The stale workflow result is then admitted after the manual user row that should supersede it. Fresh evidence beyond the prior full-clear thread is that this generation covers only context-discarding mutations, not manual supersession rows; recheck workflow currentness at admission or include accepted manual turns in the generation.

AGENTS.md reference: AGENTS.md:L150-L150

Useful? React with 👍 / 👎.

Comment on lines +290 to +294
if (
input.skipIfNewerRecordExists === true &&
previous != null &&
previous.createdAtMs > createdAtMs
) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Order provenance retries independently of wall clock

When an initial provenance write fails, the clock rolls backward, and a later workflow_resume successfully records a corrected boundary, that newer record can have a smaller createdAtMs than the scheduled older retry. This comparison then treats the retry as newer and overwrites the resume boundary with stale launch provenance, causing the resumed run's terminal wake to be dropped. Fresh evidence beyond the earlier retry-ordering thread is that the final gate still relies on wall-clock timestamps despite explicitly supporting backward-clock corrections; use a monotonic generation or expected-record identity instead.

AGENTS.md reference: AGENTS.md:L150-L150

Useful? React with 👍 / 👎.

Comment on lines +487 to +490
function isCoalescedWorkflowResultMessage(message: MuxMessage, runId: string): boolean {
return (
message.role === "user" && message.metadata?.deliveredWorkflowRunIds?.includes(runId) === true
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Validate persisted delivery provenance before calling includes

When a persisted row has malformed deliveredWorkflowRunIds metadata such as a number or object, this calls a nonexistent includes method and aborts the full-history walk. Currentness then remains indeterminate, so the terminal notification is retried forever and one corrupt metadata field can prevent the workspace from self-healing. Check Array.isArray and filter string IDs before consulting the provenance.

AGENTS.md reference: AGENTS.md:L108-L113

Useful? React with 👍 / 👎.

Comment on lines +250 to +253
// Foreground only: a background dispatch can still observe the stale pre-dispatch terminal
// status, and consuming it would tombstone the retried run's future terminal wake.
if (!isBackgroundDispatch && refreshedRun != null) {
await markTerminalAttentionConsumed(refreshedRun);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Consume foreground results when the refresh read fails

When a kernel-nested foreground resume or checkpoint retry finishes terminally but the post-dispatch run refresh encounters a transient read failure, WorkflowService.getRun collapses that failure to null, so this guard skips the consumption tombstone even though dispatched is returned to the model with the terminal result. Because the nested invocation is invisible in top-level history, its pending terminal attention can later inject the same result again. Derive terminal consumption from the successful dispatch result or keep refresh failures retryable rather than treating null as no consumption.

AGENTS.md reference: AGENTS.md:L110-L112

Useful? React with 👍 / 👎.

Comment on lines +209 to +215
scheduleBoundarySnapshotRepair({
workspaceSessionDir,
runId,
createdAtMs,
getBoundary: () => getBoundaryMessageId(workspaceId, runId),
retryDelaysMs,
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Defer wake supersession until boundary repair completes

When the launch-time history read fails for a workflow started from empty history, this schedules the first boundary repair one second later but immediately persists a boundary-less reference. If the workflow completes and the owner becomes idle before that timer fires, currentness sees decision-free history plus afterBoundaryMessageId === undefined, classifies the wake not_current, and permanently marks its outbox record superseded; the later repair to null cannot resurrect that tombstone. Fresh evidence beyond the earlier boundary-repair thread is that the detached repair is not coordinated with terminal-drain supersession, so keep currentness indeterminate while maintenance is pending or revive the notification when repair succeeds.

AGENTS.md reference: AGENTS.md:L110-L112

Useful? React with 👍 / 👎.

The ordinary in-stream workflow continuation carries the live turn's
effectiveToolPolicy; the terminal-attention drain's synthetic send
starts a fresh turn and omitted it, so a workflow wake could regain
tools the caller disabled, with attacker-influenced workflow output
choosing the timing. The drain now restores the newest manual user
row's persisted caller policy (synthetic rows without one are skipped),
fails closed by deferring the wake when history is unreadable, and the
agent-level policy recomposes from agentId at send resolution.
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e14574d8bf

ℹ️ 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".

private async resolveTerminalWakeCallerToolPolicy(
ownerWorkspaceId: string
): Promise<ToolPolicy | undefined> {
const historyResult = await this.historyService.getLastMessages(ownerWorkspaceId, 50);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Scan past the fixed history tail for restricted tool policy

When more than 50 assistant or synthetic rows have accumulated since the manual turn that launched the workflow, this bounded read never sees that turn and returns undefined; the wake is then sent without toolPolicy, restoring the unrestricted default despite the caller having disabled tools. Fresh evidence after the prior tool-policy fix is this hard 50-row limit in the new resolver; walk history until the newest manual user row is found, or fail closed when the bounded tail contains none.

Useful? React with 👍 / 👎.

Comment on lines 8419 to 8425
const sendOptions = {
model: resumeOptions.model,
agentId: resumeOptions.agentId,
thinkingLevel: resumeOptions.thinkingLevel,
reasoningMode: resumeOptions.reasoningMode,
...(wakeToolPolicy != null ? { toolPolicy: wakeToolPolicy } : {}),
...(workspaceTurnMuxMetadata != null ? { muxMetadata: workspaceTurnMuxMetadata } : {}),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve the workspace-agent disable flag on terminal wakes

When the initiating turn has disableWorkspaceAgents: true, this synthetic continuation restores its tool policy but omits that separately persisted restriction. Send resolution consequently uses the default false value, making repository-local agents and skills available again on a wake generated from workflow output; restore the newest manual turn's disableWorkspaceAgents value alongside its tool policy.

Useful? React with 👍 / 👎.

@@ -85,3 +320,68 @@ export async function recordAgentWorkflowRunReference(input: {
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Retire settled workflow references

In a long-lived workspace, every background workflow run remains in byRunId until the user performs a full history clear, including runs whose terminal attention was already delivered or consumed. Each later launch atomically rewrites the entire accumulated array, while stream-end and currentness paths repeatedly read and parse it, so workflow-heavy workspaces incur steadily increasing latency and storage; remove settled/superseded entries or compact the sidecar to references that can still affect delivery.

AGENTS.md reference: AGENTS.md:L10-L10

Useful? React with 👍 / 👎.

Comment on lines +225 to +230
// For kernel-launched runs this file is the only durable invocation evidence, and callers
// deciding wake delivery must distinguish "no reference" from "cannot know right now":
// flattening a transient read failure into [] would let the terminal drain tombstone the
// run's wake. Corrupted contents below stay self-healing because rereading cannot repair
// them, while a failed read can succeed later.
throw error;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Quarantine corrupt sidecar nodes during normal reads

When the known agent-workflow-runs.json path is a directory or another permanently invalid filesystem node, every read deterministically throws rather than recovering, so workflow currentness remains indeterminate and the terminal notification rearms its 30-second retry forever; subsequent provenance writes also cannot repair the path. Fresh evidence after the prior full-clear corruption fix is that only the clear path removes such directories, while this normal read path still classifies them as transient; quarantine or remove the invalid node before returning an empty sanitized state.

AGENTS.md reference: AGENTS.md:L108-L110

Useful? React with 👍 / 👎.

@chatgpt-codex-connector

This comment has been minimized.

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