Skip to content

fix(heartbeat): suppress no-op system event replies#73785

Open
pfrederiksen wants to merge 1 commit intoopenclaw:mainfrom
pfrederiksen:fix/73149-silent-noop-runtime
Open

fix(heartbeat): suppress no-op system event replies#73785
pfrederiksen wants to merge 1 commit intoopenclaw:mainfrom
pfrederiksen:fix/73149-silent-noop-runtime

Conversation

@pfrederiksen
Copy link
Copy Markdown
Contributor

@pfrederiksen pfrederiksen commented Apr 28, 2026

Summary

  • add centralized no-op sentinel detection for silent heartbeat/system-event replies
  • suppress exact HEARTBEAT_OK, NO_REPLY, NO_NEW_AUDIO, and SESSION_WATCHDOG_OK outputs before delivery
  • keep meaningful exec/system-event summaries deliverable instead of treating all system events as forced visible output

Fixes #73149.

Tests

  • pnpm exec oxlint src/auto-reply/tokens.ts src/infra/heartbeat-runner.ts src/infra/heartbeat-runner.respects-ackmaxchars-heartbeat-acks.test.ts
  • pnpm exec vitest run --config test/vitest/vitest.infra.config.ts src/infra/heartbeat-runner.respects-ackmaxchars-heartbeat-acks.test.ts
  • pnpm tsgo:core:test

Note: attempted full pnpm lint:core, but tsgolint was SIGKILLed in this constrained worktree before reporting code findings. Ran the focused lint/type/test gates above successfully after rebasing on current origin/main.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 28, 2026

Greptile Summary

This PR refactors heartbeat reply suppression by centralizing no-op token detection into isNoOpSentinelReplyText in tokens.ts, then applying it early in normalizeHeartbeatReply before further token stripping. The previous exec-completion exception (!hasExecCompletion in shouldSkipMain and the execFallbackText fallback) is replaced by simply treating system events with the "message" strip mode, so that HEARTBEAT_OK/NO_REPLY/etc. replies are suppressed even for exec events while meaningful summaries are still delivered.

Confidence Score: 5/5

Safe to merge — logic is straightforward, well-tested, and the behavioral change is intentional and correctly implemented.

No P0 or P1 issues found. The sentinel detection is correctly gated, the early-return path in normalizeHeartbeatReply is sound, the removal of the old execFallbackText workaround is clean, and three new test cases validate all key scenarios.

No files require special attention.

Reviews (1): Last reviewed commit: "fix(heartbeat): suppress no-op system ev..." | Re-trigger Greptile

@pfrederiksen pfrederiksen force-pushed the fix/73149-silent-noop-runtime branch 2 times, most recently from 7d4d936 to 4483377 Compare April 28, 2026 20:46
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented Apr 28, 2026 •
edited
Loading

Codex review: needs changes before merge.

Summary
The PR adds shared exact no-op sentinel detection for heartbeat/system-event replies, applies it in heartbeat reply normalization, and adds regression tests for silent sentinel outputs versus meaningful exec summaries.

Reproducibility: yes. source-level. Current main only suppresses heartbeat acks and the standard NO_REPLY path, so exact NO_NEW_AUDIO or SESSION_WATCHDOG_OK heartbeat replies remain ordinary deliverable text; the PR adds focused runHeartbeatOnce coverage for those cases.

Next step before merge
A repair worker can handle the mechanical rebase/adaptation and add the missing changelog entry without a product decision.

Security
Cleared: The PR changes TypeScript heartbeat/silent-token logic and tests only, with no dependency, workflow, credential, package metadata, or artifact execution changes.

Review findings

  • [P3] Add the required changelog entry — CHANGELOG.md:20
Review details

Best possible solution:

Land a rebased narrow exact-sentinel suppression with a changelog entry while preserving structured heartbeat_respond and relayable exec-output behavior; leave broader job-defined no-reply contracts to the canonical follow-up discussion.

Do we have a high-confidence way to reproduce the issue?

Yes, source-level. Current main only suppresses heartbeat acks and the standard NO_REPLY path, so exact NO_NEW_AUDIO or SESSION_WATCHDOG_OK heartbeat replies remain ordinary deliverable text; the PR adds focused runHeartbeatOnce coverage for those cases.

Is this the best way to solve the issue?

Yes for the narrow bug class, if rebased carefully. Central exact-sentinel detection before heartbeat delivery is maintainable, but it must preserve current main's structured heartbeat tool path and relayable exec-completion safeguards.

Full review comments:

  • [P3] Add the required changelog entry — CHANGELOG.md:20
    This patch fixes user-visible heartbeat/system-event delivery behavior, so repo policy requires a single-line entry under the active Unreleased/Fixes section. The PR currently changes only source and test files, leaving release notes incomplete.
    Confidence: 0.89

Overall correctness: patch is correct
Overall confidence: 0.82

Acceptance criteria:

  • pnpm exec oxlint src/auto-reply/tokens.ts src/infra/heartbeat-runner.ts src/infra/heartbeat-runner.respects-ackmaxchars-heartbeat-acks.test.ts
  • pnpm test src/infra/heartbeat-runner.respects-ackmaxchars-heartbeat-acks.test.ts
  • pnpm tsgo:core:test
  • pnpm check:changed

What I checked:

  • Current main sentinel helpers: Current main defines HEARTBEAT_OK and NO_REPLY helpers, but has no NO_OP_SENTINEL_REPLY_TOKENS or isNoOpSentinelReplyText, and source search found no NO_NEW_AUDIO or SESSION_WATCHDOG_OK handling in the relevant heartbeat/auto-reply paths. (src/auto-reply/tokens.ts:3, a4c1c28a1731)
  • Current main heartbeat normalization: normalizeHeartbeatReply strips the response prefix and then calls stripHeartbeatToken in heartbeat mode; it does not run a no-op sentinel detector before channel delivery. (src/infra/heartbeat-runner.ts:697, a4c1c28a1731)
  • Current main exec preservation: runHeartbeatOnce currently preserves relayable exec completion text when heartbeat token stripping empties normalized output, so a rebase must keep meaningful exec summaries deliverable. (src/infra/heartbeat-runner.ts:1655, a4c1c28a1731)
  • PR head implementation: The PR diff adds NO_OP_SENTINEL_REPLY_TOKENS/isNoOpSentinelReplyText, calls it from normalizeHeartbeatReply, and adds runHeartbeatOnce tests for NO_REPLY, NO_NEW_AUDIO, SESSION_WATCHDOG_OK, exec-event HEARTBEAT_OK suppression, and meaningful exec summaries. (src/auto-reply/tokens.ts:83, 2116d312abc1)
  • PR merge state: The GitHub PR API reports head 2116d31 with mergeable=false and mergeable_state=dirty, so the branch needs conflict resolution against current main.
  • Missing changelog entry: The PR file list contains only src/auto-reply/tokens.ts, src/infra/heartbeat-runner.ts, and the heartbeat runner test; current CHANGELOG.md has an active Unreleased/Fixes section for user-facing fixes. (CHANGELOG.md:20, a4c1c28a1731)

Likely related people:

  • steipete: Recent file history for src/infra/heartbeat-runner.ts and src/infra/heartbeat-events-filter.ts includes exec routing, metadata-only exec completion suppression, heartbeat response-prefix handling, and wake scheduling changes by this maintainer. (role: recent heartbeat and exec-event maintainer; confidence: high; commits: 9180173f9a79, 65c9eddae87e, 2d1523e573a8; files: src/infra/heartbeat-runner.ts, src/infra/heartbeat-events-filter.ts)
  • pashpashpash: Recent current-main history added structured heartbeat responses/Codex tool replies and then stopped heartbeat tool turns from asking for HEARTBEAT_OK, which directly overlaps the PR's sentinel versus structured no-reply boundary. (role: structured heartbeat response owner; confidence: high; commits: 439d8edf68e2, 8f4eaa9c00be; files: src/infra/heartbeat-runner.ts, src/infra/heartbeat-events-filter.ts, src/infra/heartbeat-runner.tool-response.test.ts)
  • obviyus: Recent src/auto-reply/tokens.ts history includes case-insensitive NO_REPLY detection and streaming/draft-boundary silent-token hardening, which is adjacent to the new shared sentinel helper proposed here. (role: adjacent silent-token maintainer; confidence: medium; commits: 16c608e3937e, 0817bf446fdc, 3d998828b930; files: src/auto-reply/tokens.ts)

Remaining risk / open question:

  • The PR head is currently merge-conflicted with main and must be rebased around newer heartbeat_respond and relayable exec-completion changes.
  • A naive rebase could suppress relayable command-completion output if it drops current main's hasRelayableExecCompletion preservation path.
  • The focused tests listed in the PR body were not rerun in this read-only review.

Codex review notes: model gpt-5.5, reasoning high; reviewed against a4c1c28a1731.

@pfrederiksen
fix(heartbeat): suppress no-op system event replies
2116d31 
Suppress exact silent/no-op sentinel outputs from heartbeat and system-event handoffs so HEARTBEAT_OK and NO_REPLY-style responses do not leak as visible messages. Keep meaningful exec completion summaries deliverable.\n\nFixes openclaw#73149.
@pfrederiksen pfrederiksen force-pushed the fix/73149-silent-noop-runtime branch from 4483377 to 2116d31 Compare April 28, 2026 21:07
This was referenced Apr 30, 2026
Closed
Open
Closed
@arcabotai arcabotai mentioned this pull request May 1, 2026
Open
@clawsweeper clawsweeper Bot mentioned this pull request May 3, 2026
Closed
@vincentkoc vincentkoc mentioned this pull request May 3, 2026
Merged
25 tasks
@clawsweeper clawsweeper Bot mentioned this pull request May 6, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

size: S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Cron/heartbeat no-op runs can leak visible messages despite silence/no-op semantics

1 participant

@pfrederiksen