-
Notifications
You must be signed in to change notification settings - Fork 1
feat(atxp): switch to /hooks/agent, remove HEARTBEAT.md logic#58
Conversation
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>
There was a problem hiding this 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):
-
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.
-
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.
There was a problem hiding this 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-configureHooksOnInstanceno longer logs anything on success; the previouschalk.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 ofenableNotifications(), before any API interaction. If session discovery is slow or fails silently, it silently sendschannels: undefined(omitted). Worth confirming the service handles a missingchannelsfield 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.logfor 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.
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.