Skip to content

Fix heartbeat async exec delivery leaks#67273

Closed
kenrolzjc wants to merge 2 commits into
openclaw:mainfrom
kenrolzjc:fix/heartbeat-async-exec-delivery-main-base
Closed

Fix heartbeat async exec delivery leaks#67273
kenrolzjc wants to merge 2 commits into
openclaw:mainfrom
kenrolzjc:fix/heartbeat-async-exec-delivery-main-base

Conversation

@kenrolzjc
Copy link
Copy Markdown

PR-ready package: Fix heartbeat async exec delivery leaks

Suggested PR title

Fix heartbeat async exec delivery leaks

Suggested PR summary

This backports the heartbeat async-completion fix that was previously applied only as a live dist patch.

Problem

Even after exec-event prompts were switched to internal-only, async command completion could still leak into user-visible chat because the heartbeat delivery path continued to special-case exec completion.

Root cause

The remaining leak was in src/infra/heartbeat-runner.ts, not just in prompt wording:

  • execFallbackText could revive text already suppressed by heartbeat token stripping
  • shouldSkipMain = normalized.shouldSkip && !normalized.hasMedia && !hasExecCompletion excluded exec completion from the silent main-delivery path
  • canAttemptHeartbeatOk and reasoning delivery were still allowed to flow outward for exec completion events

Fix

  • make exec completion prompt internal-only in src/infra/heartbeat-events-filter.ts
  • remove exec-completion text revival (execFallbackText)
  • force exec completion into the silent main-delivery path
  • prevent outward HEARTBEAT_OK delivery for exec completion
  • prevent outward reasoning payload delivery for exec completion

Tests

Updated / added regression coverage in:

  • src/infra/heartbeat-events-filter.test.ts
  • src/infra/heartbeat-runner.returns-default-unset.test.ts

Regression assertions now cover:

  • internal-only exec prompt regardless of deliverToUser
  • no external HEARTBEAT_OK delivery for exec completion
  • no external reasoning payload delivery for exec completion

Local validation already completed

  • corepack pnpm install --frozen-lockfile
  • git diff --check
  • node scripts/run-vitest.mjs run --config test/vitest/vitest.unit.config.ts src/infra/heartbeat-events-filter.test.ts src/infra/heartbeat-runner.returns-default-unset.test.ts
    • exit code: 0

Local source branch

  • fix/heartbeat-async-exec-delivery-backport

Commit

  • f83d4a15ddf2ca6c6d96c56f4baf27d1dc557c59

Transfer artifacts

  • patch diff: heartbeat-async-upstream-backport.patch
  • format-patch: heartbeat-async-upstream-backport-formatpatch.patch
  • result note: heartbeat-async-upstream-backport-result.md

Cherry-pick instruction

git cherry-pick f83d4a15ddf2ca6c6d96c56f4baf27d1dc557c59

Push / PR note

No external push was performed in this step. This package is prepared for manual push / PR creation once explicitly approved.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 15, 2026

Greptile Summary

This PR closes three exec-completion delivery leak paths in the heartbeat runner: the prompt is now always internal-only (ignoring deliverToUser), HEARTBEAT_OK is blocked from external delivery for exec events, and reasoning payloads are suppressed for exec events. The old execFallbackText revival logic is removed, and shouldSkipMain is restructured to unconditionally force exec completions into the silent path. Regression tests cover all three new guards.

Confidence Score: 5/5

Safe to merge; changes are tightly scoped to exec-completion silencing with no behavioral change to regular heartbeat or cron paths.

All findings are P2 style suggestions. The logic for the three new guards is consistent — since includeReasoning is already false for exec completions, reasoningPayloads is always empty, so shouldSkipMain short-circuits cleanly. The only nit is a dead deliverToUser argument at the call site.

No files require special attention.

Comments Outside Diff (1)

  1. src/infra/heartbeat-runner.ts, line 690 (link)

    P2 Dead argument after prompt change

    buildExecEventPrompt now ignores its _opts parameter entirely, so { deliverToUser: params.canRelayToUser } is a dead argument here. It signals to future readers that deliverToUser still influences exec prompts, which is no longer true. Dropping the argument (or passing no args) would make the intent obvious.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: src/infra/heartbeat-runner.ts
    Line: 690
    
    Comment:
    **Dead argument after prompt change**
    
    `buildExecEventPrompt` now ignores its `_opts` parameter entirely, so `{ deliverToUser: params.canRelayToUser }` is a dead argument here. It signals to future readers that `deliverToUser` still influences exec prompts, which is no longer true. Dropping the argument (or passing no args) would make the intent obvious.
    
    
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/infra/heartbeat-runner.ts
Line: 690

Comment:
**Dead argument after prompt change**

`buildExecEventPrompt` now ignores its `_opts` parameter entirely, so `{ deliverToUser: params.canRelayToUser }` is a dead argument here. It signals to future readers that `deliverToUser` still influences exec prompts, which is no longer true. Dropping the argument (or passing no args) would make the intent obvious.

```suggestion
    ? buildExecEventPrompt()
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "Fix heartbeat async exec delivery leaks" | Re-trigger Greptile

@prtags
Copy link
Copy Markdown

prtags Bot commented Apr 23, 2026

Related work from PRtags group modern-barnacle-fdko

Title: heartbeat: internal-only async exec transcript leak

Number Title
#67273* Fix heartbeat async exec delivery leaks
#70519 fix(heartbeat): hide internal-only exec transcript turns (Fixes #70458)

* This PR

@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented Apr 27, 2026

Closing this as duplicate or superseded after Codex automated review.

Close as superseded. This PR proposes making all async exec completion delivery internal-only, but current main still intentionally preserves user-relayable exec completions, and the newer related PR #72253 is the active, narrower implementation candidate for the remaining benign completion/SIGTERM leak while preserving real failure delivery.

Best possible solution:

Close this PR as superseded and continue review on #72253 or an equivalent focused heartbeat patch that keeps benign structured exec completions internal while preserving user-visible delivery for real async command failures.

What I checked:

So I’m closing this here and keeping the remaining discussion on the canonical linked item.

Codex Review notes: model gpt-5.5, reasoning high; reviewed against 7c0fdae9b95b.

@clawsweeper clawsweeper Bot closed this Apr 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant