Skip to content

feat(atxp): switch to /hooks/agent, remove HEARTBEAT.md logic#58

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

feat(atxp): switch to /hooks/agent, remove HEARTBEAT.md logic#58
R-M-Naveen merged 2 commits intomainfrom
naveen/agent-push-notifications

Conversation

@R-M-Naveen
Copy link
Member

Replace /hooks/wake + HEARTBEAT.md approach with direct /hooks/agent channel targeting. Discovered channels are now sent to the notifications service which stores them and uses them for delivery.

  • Remove HEARTBEAT.md read/write/section-replace logic (~100 lines)
  • Simplify configureHooksOnInstance to only set hooks.token
  • Send discovered channels in the enable API request

R-M-Naveen and others added 2 commits March 6, 2026 17:02
Replace /hooks/wake + HEARTBEAT.md approach with direct /hooks/agent
channel targeting. Discovered channels are now sent to the notifications
service which stores them and uses them for delivery.

- Remove HEARTBEAT.md read/write/section-replace logic (~100 lines)
- Simplify configureHooksOnInstance to only set hooks.token
- Send discovered channels in the enable API request

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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: APPROVE

Summary

This PR replaces the HEARTBEAT.md-based notification relay approach with direct channel discovery sent to the notifications service API, removing roughly 100 lines of complex file-manipulation logic in favor of a simpler architecture.

Actionable Feedback (2 items):

  1. notifications.ts line 73 - configureHooksOnInstance no longer logs on success or gateway restart. Consider adding a brief chalk.gray log after fs.writeFile and the pkill call to restore the observability that was present in the old code.

  2. notifications.ts line 76 - The pkill catch block swallows all errors silently. A narrow error-code check or a chalk.gray log would help distinguish the expected "not running" case from an actual failure.

Detailed Review:

Code Quality: The refactor is clean and well-motivated. Removing the HEARTBEAT.md read/write/section-replace logic (split-on-header indexing, regex searches, separator edge cases) eliminates a meaningful source of fragility. The early-return guard in configureHooksOnInstance is a nice simplification over the previous changed-flag pattern. Moving discoverConnectedChannels() into enableNotifications is the right call: channels are now a concern of the API request rather than local instance configuration. The body type widening from Record<string,string> to Record<string,unknown> is correct given that channels is an array.

Security: No new security concerns introduced. The execSync command is fully hardcoded with no user-controlled input, the config path is a constant, and the existing sanitizeSessionValue function remains intact.

Positive Notes: Net -94 lines with no loss of correctness is a great outcome. The PR description clearly explains the motivation and what was removed, making the change easy to reason about.

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: APPROVE

Summary

This PR replaces the HEARTBEAT.md-based notification relay approach with direct /hooks/agent channel targeting, moving channel discovery to the enableNotifications() call and sending channels to the notifications service instead. Net result: ~100 lines removed, logic significantly simplified.

Actionable Feedback (2 items)
  • notifications.ts:~78 - configureHooksOnInstance no longer logs anything on success; the previous chalk.gray('Hooks and heartbeat configured in openclaw.json') was removed without a replacement. A brief success log (e.g. chalk.gray('Hooks token configured')) would help with observability when debugging on a Fly instance.
  • notifications.ts:~113 - discoverConnectedChannels() is now called unconditionally at the start of enableNotifications(), before any API interaction. If session discovery is slow or fails silently, it silently sends channels: undefined (omitted). Worth confirming the service handles a missing channels field gracefully (no-op vs. error).
Detailed Review

Code Quality

The simplification is well-executed. The early-return guard in configureHooksOnInstance (if (config.hooks.token === hooksToken && config.hooks.enabled === true) return;) is cleaner than the previous changed flag approach. Removing the split-on-header HEARTBEAT.md section-replacement logic eliminates a subtle edge-case-prone code path. The type widening of body from Record<string, string> to Record<string, unknown> is the correct fix for passing the channels array.

Security

No concerns. Token handling is unchanged; the pkill call uses a fixed string pattern with no user input.

Suggestions

  • The removed console.log for gateway restart (chalk.gray('Gateway restarting to apply config...')) was useful for debugging — consider keeping it even in the simplified path.

Positive Notes

  • Clean, focused PR — does exactly what the description says and nothing more.
  • Moving channel discovery to enableNotifications() is the right design: the service owns delivery targeting rather than a local markdown file.
  • SKILL.md update is consistent with the implementation change.

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