Skip to content

feat(atxp): auto-discover channels and write HEARTBEAT.md for multi-c…#57

Merged
R-M-Naveen merged 6 commits intomainfrom
naveen/agent-push-notifications
Mar 6, 2026
Merged

feat(atxp): auto-discover channels and write HEARTBEAT.md for multi-c…#57
R-M-Naveen merged 6 commits intomainfrom
naveen/agent-push-notifications

Conversation

@R-M-Naveen
Copy link
Member

…hannel notification relay

Instead of injecting instructions via /hooks/wake, directly write relay instructions to HEARTBEAT.md and configure heartbeat delivery target from discovered session channels. Adds sms.received to help text.

…hannel notification relay

Instead of injecting instructions via /hooks/wake, directly write relay
instructions to HEARTBEAT.md and configure heartbeat delivery target from
discovered session channels. Adds sms.received to help text.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

AI Code Review - Recommendation: COMMENT. This PR replaces injecting HEARTBEAT.md instructions via /hooks/wake HTTP with a direct file-based approach: discovering channels from the session store and writing relay instructions to HEARTBEAT.md at enable-time. Three suggestions: (1) notifications.ts:27 - hardcoded sessions and workspace paths should be extracted as named constants. (2) notifications.ts:148 - the regex replacing the relay section interpolates the header string into RegExp without escaping; the end-anchor lookahead may also fail on trailing-newline files - consider a split-on-header approach or a distinct end-marker. (3) notifications.ts:121 - the else-if only resets heartbeat target to last when channels is empty AND no target exists; if previously-set channels disappear, the stale target persists. Overall the refactor is clean: the changed flag avoids unnecessary restarts, Set-based dedup and webchat filter are sensible, and removing the HTTP call to /hooks/wake improves reliability since disk writes do not require the agent to be running.

…ace, reset stale target

- Extract hardcoded paths as named constants
- Replace regex section replacement with split-on-header approach
- Reset stale heartbeat target when channels disappear
- Add error handling for fetch, getAccountId, and configureHooksOnInstance

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

AI Code Review - COMMENT

Summary: This PR replaces the previous one-shot /hooks/wake webhook approach with a more robust mechanism: auto-discovering active messaging channels from the local session store and writing relay instructions directly into HEARTBEAT.md.

Actionable Feedback (3 items):

  1. notifications.ts ~line 90 -- channel and to values parsed from session keys are written verbatim into HEARTBEAT.md as LLM instructions. If sessions.json were compromised (e.g. a session key with a newline in the to segment), extra lines could be injected into the prompt. Sanitise channel and to values (strip newlines, limit length) before embedding them in buildHeartbeatMd.
  2. notifications.ts ~line 138 (section-replace branch) -- When before is non-empty and does not end with a newline, before+section concatenates without a separator. Add a newline between before and section when before does not already end with one.
  3. notifications.ts ~line 180 -- Extra blank line after the closing brace of configureHooksOnInstance. Minor nit.

Code Quality: The overall refactor is cleaner than the previous approach. Using a changed flag to gate file writes and gateway restarts is good practice. The session-key regex is clear, skipping webchat is sensible, and wrapping fetch and getAccountInfo in try/catch makes the flow more resilient.

Security: The primary concern is prompt injection -- channel and to values from the session file are embedded directly into LLM instructions in HEARTBEAT.md. The attack surface is limited (requires write access to /data on the Fly instance), but a newline in a to value could allow arbitrary instruction injection. Stripping newlines from channel and to before writing them to the file would close this. No secrets are logged or exposed.

Suggestions: The removal of sendHeartbeatInstruction means a currently-running agent will only pick up the new HEARTBEAT.md on its next heartbeat tick -- worth a brief comment so future maintainers understand the timing.

Positive Notes: Conditional gateway restart (only when changed===true) avoids unnecessary churn. Graceful fallback to hb.target=last when no channels are discovered is thoughtful. The catch(err) block now surfaces actual error messages, far more debuggable than the original silent swallow. Adding sms.received to the help text is a useful documentation improvement.

…ve extra blank line

- Strip newlines/control chars and limit length on channel/to values
  parsed from session keys to prevent prompt injection via HEARTBEAT.md
- Ensure newline separator between preceding content and replaced section
- Remove extra blank line after configureHooksOnInstance

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

AI Code Review

Recommendation: COMMENT

Summary
This PR refactors notification relay setup from a one-time webhook injection (via /hooks/wake) to directly writing LLM instructions into HEARTBEAT.md and auto-discovering messaging channels from the local session store. It also adds proper error handling for network/config failures and documents sms.received support.

Actionable Feedback

  • notifications.ts:buildHeartbeatMd -- Channel/peer ID values from session keys are embedded verbatim (after control-char stripping) into an LLM-read prompt. A crafted peer ID containing double-quotes or equals signs could perform attribute-injection in the generated tool-call lines since HEARTBEAT.md is read and followed by an LLM. Consider wrapping values in backticks or expanding the sanitizer to cover these characters.
  • notifications.ts:configureHooksOnInstance -- HEARTBEAT.md is unconditionally rewritten on every notifications enable call even when changed === false. Consider comparing the proposed section content to what is already on disk before writing.
  • notifications.ts:buildHeartbeatMd -- The section-replace path writes section.trimEnd() + after where after starts with a newline. This is subtle: if buildHeartbeatMd ever gains a trailing newline the trim compensates silently. Consider making the separator explicit rather than relying on trimEnd().
  • General: No tests cover discoverConnectedChannels or buildHeartbeatMd. The session-key regex and section-replace logic are non-trivial; unit tests with edge-case inputs (empty sessions file, existing section, next heading immediately after) would prevent regressions.

Code Quality
Clear improvement over the previous approach. Replacing the fire-and-forget webhook injection with a deterministic file write is more reliable and easier to reason about. The changed flag avoiding unnecessary config writes and gateway restarts is a nice optimization. Extracting constants (OPENCLAW_CONFIG_PATH, SESSIONS_PATH, HEARTBEAT_SECTION_HEADER) improves readability and centralizes paths. The section-replace logic (indexOf + search for next top-level heading) is correct but somewhat fragile -- the comment about avoiding regex edge cases is appreciated, but the replacement itself has subtle edge cases worth covering with tests.

Security

  • Sanitization of session keys: control-character stripping prevents newline injection -- good baseline.
  • Prompt injection via peer IDs: values are embedded as unquoted attribute strings in lines an LLM reads and follows. A peer ID containing double-quotes or equals signs could mutate the intended channel/target. Consider wrapping values in backticks in the generated Markdown.
  • Path hardcoding: appropriate for this deployment context; try/catch fallbacks handle missing files gracefully.
  • No user-controlled content flows into execSync -- the pkill invocation is safe.

Suggestions

  • The getAccountId try/catch and the fetch error wrapping with process.exit(1) are both good defensive improvements.
  • Consider a sanitizeChannelValue helper to make sanitization reusable and independently testable.

Positive Notes

  • Removing sendHeartbeatInstruction eliminates an HTTP round-trip and a timing dependency on the agent being awake -- strictly more reliable.
  • The webchat exclusion and dedup logic in discoverConnectedChannels show careful attention to edge cases.
  • Error messages in the updated catch block are actionable: warning, detail, and fallback hint.
  • The sms.received help text addition keeps documentation in sync with new capabilities.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

AI Code Review - COMMENT - This PR refactors notification setup to auto-discover messaging channels from the session store and write relay instructions to HEARTBEAT.md, replacing the /hooks/wake injection approach. Also adds sms.received to help text. Issues: (1) notifications.ts:44 - sanitize lambda is redefined each loop iteration, hoist it. (2) notifications.ts:68 - const sessionsPath = SESSIONS_PATH is a redundant alias, use SESSIONS_PATH directly. (3) notifications.ts:93 - LLM prompt injection: channel/peer-ID values from sessions.json go verbatim into HEARTBEAT.md instructions; control chars are stripped but quote chars are not, so a tampered sessions.json could inject arbitrary LLM instructions. Backtick-wrap values and strip backticks from inputs. Positives: writing to HEARTBEAT.md directly is more auditable than live webhook injection, dedup via Set is correct, webchat skip shows domain understanding, error handling improved across the board, the changed flag avoids unnecessary restarts.

… values

- Hoist sanitizeSessionValue to module scope (out of loop)
- Also strip quote/backtick chars to prevent LLM prompt injection
- Remove redundant sessionsPath alias, use SESSIONS_PATH directly
- Backtick-wrap channel/target values in HEARTBEAT.md output

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

AI Code Review

Recommendation: COMMENT

Summary

This PR replaces the old /hooks/wake webhook injection approach with a more direct strategy: auto-discovering connected messaging channels from the local session store and writing relay instructions to HEARTBEAT.md. It also sets the heartbeat delivery target in openclaw.json from the first discovered channel and adds sms.received to the help text.

Actionable Feedback (3 items)

  • notifications.ts:260 - Trailing blank line left inside enableNotifications after removing the sendHeartbeatInstruction call. Minor style nit.

  • notifications.ts:100-109 - The next-heading search regex requires a newline before the hash character. If a subsequent top-level heading happened to start at position 0 of afterHeader (no leading newline), it would be missed. Low probability in practice, but worth a brief inline comment acknowledging the assumption.

  • notifications.ts:51 - sanitizeSessionValue strips control chars and backticks/quotes but leaves markdown-special characters like square brackets unescaped. Since these values are written into HEARTBEAT.md (an LLM-read file), a crafted session key could inject markdown-formatted prompt text. Given the trusted Fly instance context the risk is low, but worth stripping brackets as a precaution.

Code Quality

The refactor is clean and well-structured. Extracting discoverConnectedChannels and buildHeartbeatMd as separate functions with clear JSDoc comments makes intent easy to follow. The deduplication via Set, the webchat exclusion, and the changed-flag guard before writing config and restarting the gateway are all good practices that reduce unnecessary I/O. The HEARTBEAT.md section-replace logic (split-on-header rather than regex) is a reasonable approach.

getAccountId error wrapping and the fetch try/catch with process.exit(1) are solid defensive improvements over the original.

Security

Sanitization of session values covers the most dangerous characters (control chars, backticks, double-quotes). The main gap is unescaped markdown syntax characters as noted above. Since the session file lives on the Fly instance local filesystem and is written by the openclaw runtime, practical exploitability is low. No credentials are logged; the hooks token is only written to the on-instance config file.

Positive Notes

  • Replacing the fire-and-forget webhook injection with a deterministic file write is a meaningful reliability improvement -- the old approach could silently fail if the agent was not ready.
  • Conditional gateway restart (only when changed === true) avoids unnecessary disruption.
  • Adding sms.received to help text keeps docs in sync with actual supported event types.
  • Improved error visibility: the new catch block in configureHooksOnInstance logs a warning with the error message rather than silently swallowing it.

…k line, add heading comment

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

AI Code Review: APPROVE. This PR refactors notification relay from an indirect HTTP-based mechanism to direct file I/O: auto-discovering connected messaging channels from the session store and writing relay instructions to HEARTBEAT.md. The change is a clear improvement - more reliable, better error handling, and a cleaner trust boundary. Minor suggestions: (1) the HEARTBEAT.md write runs outside the changed guard unconditionally - if intentional, a comment would help; (2) sanitizeSessionValue does not strip markdown chars like asterisk/underscore/angle-brackets, which is fine given backtick-wrapping in the template but worth documenting; (3) the section-replacement logic around line 121 is non-obvious and would benefit from a brief inline comment. Overall the code is clean and well-structured. Removing sendHeartbeatInstruction eliminates a fragile network-dependent step. Error handling improvements are substantial. The webchat exclusion and channel deduplication are well-considered.

@R-M-Naveen R-M-Naveen merged commit d48a097 into main Mar 6, 2026
2 checks passed
@R-M-Naveen R-M-Naveen deleted the naveen/agent-push-notifications branch March 6, 2026 21:00
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