fix(cron): de-duplicate main-session systemEvent in heartbeat model input#91287
Open
ZengWen-DT wants to merge 1 commit into
Open
fix(cron): de-duplicate main-session systemEvent in heartbeat model input#91287ZengWen-DT wants to merge 1 commit into
ZengWen-DT wants to merge 1 commit into
Conversation
…nput A sessionTarget:"main" cron systemEvent was surfaced to the model twice: the reply pipeline rendered it as a raw System: line and the heartbeat also wrapped it via buildCronEventPrompt. selectGenericSystemEvents excluded exec-completion events (which own a dedicated heartbeat prompt) but not cron events, which own buildCronEventPrompt. Exclude cron:<jobId>-tagged events so the heartbeat path is their single owner, symmetric with exec-completions. Closes openclaw#44922
Contributor
|
Codex review: needs maintainer review before merge. Reviewed June 7, 2026, 9:44 PM ET / 01:44 UTC. Summary PR surface: Source +13, Tests +28. Total +41 across 2 files. Reproducibility: yes. source-level. Current main enqueues Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land this narrow owner-boundary fix after normal maintainer review and checks, keeping heartbeat as the single owner for Do we have a high-confidence way to reproduce the issue? Yes, source-level. Current main enqueues Is this the best way to solve the issue? Yes. Filtering heartbeat-owned AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against b16a43597d49. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +13, Tests +28. Total +41 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
sessionTarget: "main"andpayload.kind: "systemEvent"surfaced its text to the model twice in the same turn — once as a rawSystem:line rendered by the reply pipeline, and again wrapped by the heartbeat (A scheduled reminder has been triggered…).delivery.mode: "none"only governs channel relay, so it did not suppress the duplicate.selectGenericSystemEvents(src/auto-reply/reply/session-system-events.ts) already excludes exec-completion events from the genericSystem:render because they own a dedicated heartbeat prompt (buildExecEventPrompt). Cron events own the same kind of dedicated prompt (buildCronEventPrompt) but were never added to that exclusion, so they were rendered raw and wrapped.cron:<jobId>(set at enqueue insrc/cron/service/timer.ts:1409) from the generic render, leaving the heartbeat path as the single owner that consumes and surfaces them — symmetric with exec-completions. The same latent double-surfacing on the auto-disabled notice (cron:<jobId>:auto-disabled) is fixed too.:heartbeatsession key, so only the wrapper ever surfaced). The isolated→main awareness mirror uses acron-direct-delivery:key, which does not matchcron:, so it still renders raw as before.startsWith("cron:")matches exactly the heartbeat-owned cron events and nothing that relies on the raw render.Linked context
Closes #44922
Related: none. Maintainer/owner requested: no (open
queueable-style bug; the prior commenter who claimed a fix never opened a PR).Real behavior proof (required for external PRs)
sessionTarget:"main"+payload.kind:"systemEvent"surfacing its text to the model twice (rawSystem:line + heartbeat reminder wrapper) — [Bug]: Cron job with sessionTarget: "main" triggers both systemEvent and reminder despite delivery.mode: "none" #44922.upstream/main2a21de6322with the patch applied; the repo'stsxrunner driving the real, unmocked assembly functionsdrainFormattedSystemEvents(reply-pipeline raw render) andbuildCronEventPrompt(heartbeat wrapper) — the exact two surfaces that build the model input. Node 22, Linux.node --import tsx repro-44922.mts.System:line is gone; the event stays queued so the heartbeat path remains its single owner.git stashof the one prod file).Tests and validation
pnpm tsgo,node scripts/run-oxlint.mjs <files>,pnpm format:check <files>,pnpm build, andnode scripts/run-vitest.mjsonsession.test.ts(104),heartbeat-runner.ghost-reminder.test.ts(16),system-events.test.ts(40),get-reply-run.media-only.test.ts(80) — all green.drainFormattedSystemEventstest insession.test.tsasserting acron:-tagged event is excluded from the genericSystem:render and left queued, while a non-cron event still renders.drainFormattedSystemEventspath rendered the cron event as a rawSystem:line in addition to the heartbeat wrapper (see Before evidence).