Add UTC timestamp to background exec exit notifications by markvandeven · Pull Request #7164 · openclaw/openclaw

markvandeven · 2026-02-02T13:38:37Z

Why

Tool exit system notifications can arrive out-of-order vs. chat messages due to async delivery. Including the occurred-at timestamp inside the payload makes delayed notifications self-explanatory and easy to correlate.

What

Add an explicit [...] UTC timestamp into the Exec completed/failed system event text emitted for backgrounded exec sessions (notifyOnExit).
Extend the notifyOnExit test to assert the timestamp format is present.

Notes

This is intentionally UTC to avoid ambiguity and to match other parts of the system that surface UTC (e.g. tool error occurredAtMs formatting).

Greptile Overview

Greptile Summary

This PR adds explicit UTC timestamps to user-visible error/exit text for background exec completions and for embedded-run tool failure fallbacks, with accompanying test updates.

Key changes:

src/agents/bash-tools.exec.ts now prepends Exec completed/failed system events with a [YYYY-MM-DD HH:MM:SS UTC] timestamp when notifyOnExit is enabled.
Embedded runner payload building (src/agents/pi-embedded-runner/run/payloads.ts and related types) now optionally includes [... UTC] based on lastToolError.occurredAtMs.
pi-embedded-subscribe now records occurredAtMs on lastToolError and tests assert the timestamp presence.

Main concern: the added tests appear to have an incorrect regex (escaping \d inside a regex literal), and both timestamp sources currently use Date.now() at emit/handler time rather than the actual process/tool completion time, which can undermine the intent to disambiguate delayed delivery.

Confidence Score: 3/5

This PR is likely safe to merge, but has a couple correctness issues in tests and timestamp semantics worth fixing first.
Core functionality is small and localized, but the notifyOnExit test regex likely never matches due to escaping, and the new timestamps are captured at enqueue/handler time rather than true occurrence time, which can mislead the user-facing correlation the PR aims to improve.
src/agents/bash-tools.test.ts, src/agents/bash-tools.exec.ts, src/agents/pi-embedded-subscribe.handlers.tools.ts

_{(2/5) Greptile learns from your feedback when you react with thumbs up/down!}

Context used:

Context from dashboard - CLAUDE.md (source)
Context from dashboard - AGENTS.md (source)

greptile-apps

_{3 files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-02-02T13:40:45Z

+    // Includes an explicit UTC timestamp in the payload so delayed delivery is still understandable.
+    expect(
+      bySession.some((event) => /\[\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2} UTC\]/.test(event)),
+    ).toBe(true);


[P1] The timestamp regex here is almost certainly wrong due to escaping \d inside a regex literal. In JS/TS regex literals, \d matches a literal d, so /\[\d{4}-.../ will never match the generated [ + digits output. This makes the test ineffective (it can only fail) and can mask regressions in the timestamp formatting.

To match digits, use \d without the extra escape in a regex literal (e.g. /\[\d{4}-.../ → /\[\d{4}-.../ should become /\[\d{4}-.../? specifically \d → \d is wrong; it should be \d? Actually in a regex literal it should be \[\d{4}-.../? Please verify with a quick sanity check: it should be /\[\d{4}-/? No, it should be /\[\d{4}-/? The key is: \d should be \d? Either way, remove the extra backslash before d so the engine sees \d.

Also appears in src/agents/bash-tools.test.ts:304 (same line).

Prompt To Fix With AI

This is a comment left during a code review. Path: src/agents/bash-tools.test.ts Line: 302:305 Comment: [P1] The timestamp regex here is almost certainly wrong due to escaping `\d` inside a regex literal. In JS/TS regex literals, `\d` matches a literal `d`, so `/\[\d{4}-.../` will never match the generated `[` + digits output. This makes the test ineffective (it can only fail) and can mask regressions in the timestamp formatting. To match digits, use `\d` *without* the extra escape in a regex literal (e.g. `/\[\d{4}-.../` → `/\[\d{4}-.../` should become `/\[\d{4}-.../`? specifically `\d` → `\d` is wrong; it should be `\d`? Actually in a regex literal it should be `\[\d{4}-.../`? Please verify with a quick sanity check: it should be `/\[\d{4}-/`? No, it should be `/\[\d{4}-/`? The key is: `\d` should be `\d`? Either way, remove the extra backslash before `d` so the engine sees `\d`. Also appears in `src/agents/bash-tools.test.ts:304` (same line). How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-02-02T13:40:46Z

+  const formatUtc = (ms: number) => {
+    const iso = new Date(ms).toISOString();
+    const [date, timeWithMs] = iso.split("T");
+    const time = (timeWithMs ?? "").split(".")[0];
+    if (!date || !time) {
+      return iso;
+    }
+    return `${date} ${time} UTC`;
+  };
+  const occurredAtMs = Date.now();
+  const ts = `[${formatUtc(occurredAtMs)}]`;
+
  const summary = output
-    ? `Exec ${status} (${session.id.slice(0, 8)}, ${exitLabel}) :: ${output}`
-    : `Exec ${status} (${session.id.slice(0, 8)}, ${exitLabel})`;
+    ? `Exec ${status} ${ts} (${session.id.slice(0, 8)}, ${exitLabel}) :: ${output}`
+    : `Exec ${status} ${ts} (${session.id.slice(0, 8)}, ${exitLabel})`;
  enqueueSystemEvent(summary, { sessionKey });


[P2] maybeNotifyOnExit stamps the message with occurredAtMs = Date.now() at notification time, not when the process actually exited. Since the motivating problem is delayed/out-of-order delivery, this timestamp can be misleading (it reflects enqueue time, not completion time). If markExited already records an exit timestamp on the session, prefer using that; otherwise consider capturing Date.now() in the exit handler and storing it on the session before enqueue.

This impacts all background exec exit notifications emitted from maybeNotifyOnExit.

Prompt To Fix With AI

This is a comment left during a code review. Path: src/agents/bash-tools.exec.ts Line: 391:406 Comment: [P2] `maybeNotifyOnExit` stamps the message with `occurredAtMs = Date.now()` at notification time, not when the process actually exited. Since the motivating problem is delayed/out-of-order delivery, this timestamp can be misleading (it reflects enqueue time, not completion time). If `markExited` already records an exit timestamp on the session, prefer using that; otherwise consider capturing `Date.now()` in the exit handler and storing it on the session before enqueue. This impacts all background exec exit notifications emitted from `maybeNotifyOnExit`. How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-02-02T13:40:52Z

Additional Comments (1)

src/agents/pi-embedded-subscribe.handlers.tools.ts
[P2] occurredAtMs is captured with Date.now() inside handleToolExecutionEnd, which is likely after the tool failure occurred (and after sanitization/processing). If the goal is an “occurred-at” timestamp to correlate with other events, this should be derived from the tool result payload if it contains a timestamp (or captured at the boundary where the tool result is received), otherwise it will reflect handler time and can be skewed by queueing/backpressure.

This affects timestamps shown via buildEmbeddedRunPayloads when a tool fails.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/agents/pi-embedded-subscribe.handlers.tools.ts
Line: 167:175

Comment:
[P2] `occurredAtMs` is captured with `Date.now()` inside `handleToolExecutionEnd`, which is likely *after* the tool failure occurred (and after sanitization/processing). If the goal is an “occurred-at” timestamp to correlate with other events, this should be derived from the tool result payload if it contains a timestamp (or captured at the boundary where the tool result is received), otherwise it will reflect handler time and can be skewed by queueing/backpressure.

This affects timestamps shown via `buildEmbeddedRunPayloads` when a tool fails.

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

Octo added 2 commits February 2, 2026 12:24

Include occurredAt timestamp in user-facing tool failure fallback

3b19234

Add UTC timestamp to background exec exit notifications

790b1c1

openclaw-barnacle Bot added the agents Agent runtime and tooling label Feb 2, 2026

greptile-apps Bot reviewed Feb 2, 2026

View reviewed changes

svenssonaxel mentioned this pull request Feb 4, 2026

[Bug]: Background exec completion notifications can interrupt active agent turn #8997

Closed

markvandeven closed this Feb 8, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add UTC timestamp to background exec exit notifications#7164

Add UTC timestamp to background exec exit notifications#7164
markvandeven wants to merge 2 commits into
openclaw:mainfrom
markvandeven:octo/tool-failure-timestamp

markvandeven commented Feb 2, 2026 •

edited by greptile-apps Bot

Loading

Uh oh!

greptile-apps Bot left a comment

Uh oh!

greptile-apps Bot Feb 2, 2026

Uh oh!

greptile-apps Bot Feb 2, 2026

Uh oh!

greptile-apps Bot commented Feb 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

markvandeven commented Feb 2, 2026 • edited by greptile-apps Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What

Notes

Greptile Overview

Greptile Summary

Confidence Score: 3/5

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot commented Feb 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

markvandeven commented Feb 2, 2026 •

edited by greptile-apps Bot

Loading