Naveen/agent push notifications

[R-M-Naveen]

No description provided.

[github-actions]

AI Code Review

Recommendation: COMMENT

Summary

This PR adds a notifications command to the atxp CLI that enables push notifications via a machine-ID-based webhook registration system. It also refactors whoami.ts to export a shared getAccountInfo() helper used by the new webhook command.

Actionable Feedback (4 items)

• packages/atxp/src/commands/whoami.ts:68 - The refactor drops granular HTTP error handling. The original code distinguished 401 errors (expired token) from other errors with a specific message; after the refactor, getAccountInfo() returns null for all failures and whoamiCommand() shows a generic "Could not fetch account info" message. Consider preserving the 401 distinction in getAccountInfo() (e.g., return an error discriminant) or re-adding the status check in whoamiCommand().

• packages/atxp/src/commands/webhook.ts:4790 - Unsafe cast of API response without null checks. data.instance as { webhookUrl: string; hooksToken: string } is cast and immediately destructured/passed to configureHooksOnInstance(hooksToken) — if the server response shape changes or the key is missing, hooksToken will be undefined, silently writing a broken config. Add a null/undefined guard before using these values.

• packages/atxp/src/commands/webhook.ts:4737 - sendHeartbeatInstruction() silently sends a prompt-injection-style instruction to the running agent telling it to save content to HEARTBEAT.md and "broadcast the notification to every connected channel." This happens as a side effect of notifications enable without any user confirmation. Consider logging what's happening (e.g., "Sending notification handling instructions to your agent...") and/or making it an opt-in step so users understand the agent's behavior will be modified.

• packages/atxp/src/commands/webhook.ts:4750 - getMachineId() falls back to os.hostname(). In non-Fly environments (e.g., a developer's laptop), this registers the developer's hostname as a machine ID against their account. Add a warning or fail fast if neither FLY_MACHINE_ID is set nor the hostname looks like a Fly machine ID (e.g., doesn't match the expected format), to avoid silently registering incorrect machine IDs.

Detailed Review

Code Quality

The overall structure is clean and well-organized. The webhook.ts file has a consistent pattern for subcommands and clear separation of concerns. Backward-compatibility aliases (add → enable, ls → list) are a nice touch. The dynamic import('./whoami.js') in getEmailUserId() avoids potential circular dependency issues.

One inconsistency: enableNotifications() re-implements its own fetch + error-handling logic instead of reusing the apiRequest() helper. This is intentional (the /notifications/enable endpoint doesn't require Bearer auth), but the duplicated error handling pattern adds maintenance surface. A comment explaining why it doesn't use apiRequest() would help future readers.

Security

• execSync('pkill -f openclaw-gateway') is gated behind process.env.FLY_MACHINE_ID, limiting blast radius to Fly instances. The pattern openclaw-gateway is reasonably specific.
• The connection_token extracted from the URL is used as a Bearer token — this is consistent with the rest of the codebase.
• No credential leakage or injection vectors identified beyond the heartbeat instruction concern noted above.

Suggestions

• The getArgValue() helper reading raw process.argv works but is fragile if a flag value contains spaces or if argument order varies. Worth noting for future extension.
• The package-lock.json changes (removing "peer": true from many entries) appear to be a package manager version artifact rather than intentional changes. If possible, regenerate it cleanly to minimize noise in the diff.

Positive Notes

• configureHooksOnInstance() is well-guarded and non-fatal (the try/catch prevents it from blocking the enable flow on config errors).
• The help text is comprehensive and well-formatted.
• SKILL.md documentation is clear and complete.
• The getAccountInfo() refactor is a good extraction of shared logic, reducing duplication even if it needs a small tweak for error specificity.

[github-actions]

AI Code Review - Recommendation: COMMENT. This PR adds a notifications command to the atxp CLI for webhook-based push notifications, and refactors whoami.ts to expose a reusable getAccountInfo() function. Feedback: (1) package-lock.json:13957 - create-atxp dependency on atxp downgraded from 1.22.1 to 1.21.1, looks like an accidental regression, please verify. (2) webhook.ts:enableNotifications - raw fetch with no Authorization header unlike all other calls that use apiRequest() with Bearer auth; if intentional add a comment explaining why. (3) whoami.ts:whoamiCommand - refactoring loses 401-specific error message that was more actionable for users with expired tokens. (4) webhook.ts:listFailures - the since arg from process.argv is interpolated into the URL without encodeURIComponent which could corrupt the query string. Positive notes: zero-config auto-discovery is a nice UX touch, backward compat aliases are thoughtful, getMachineId hostname fallback is well-documented, parallel fetch in whoamiCommand is preserved.

[github-actions]

AI Code Review

Recommendation: COMMENT

Summary

This PR adds a notifications command to the atxp CLI for managing push notification webhooks (enable/list/remove/test/re-enable/rotate-secret/failures/replay). It also refactors whoami.ts to extract a reusable getAccountInfo() helper.

Actionable Feedback (5 items)

• webhook.ts:261 - The --since value is interpolated directly into the URL without encoding. Use encodeURIComponent(since) to prevent characters like & from injecting extra query parameters.

• webhook.ts:230,245,253,269,277,288 - Webhook/delivery IDs from user input are interpolated directly into URL paths. Applying encodeURIComponent(id) would prevent accidental path mangling if an ID contains / or ?.

• whoami.ts - Refactoring into getAccountInfo() loses the 401-specific error message. Previously whoamiCommand printed a targeted "try npx atxp login --force" hint on a 401; now any failure shows a generic message. Consider returning the HTTP status or preserving that hint.

• webhook.ts (filename) - The file is named webhook.ts but implements the notifications command. Consider renaming to notifications.ts for consistency with the other command files.

• index.ts:124 - The help-flag exclusion list is now 10 commands long on one line. Consider a Set to make this more maintainable as commands are added.

Detailed Review

Code Quality

Overall the code is clean and well-structured. The apiRequest() helper consolidates HTTP and error handling nicely. The configureHooksOnInstance() function correctly gates on FLY_MACHINE_ID and handles errors non-fatally. Using pkill -f openclaw-gateway is appropriate for the Fly deployment context.

Security

• No auth on /notifications/enable: enableNotifications() intentionally skips getNotificationsAuth() and sends only machine_id/email_user_id by design. Confirm the server validates machine ownership server-side.
• sendHeartbeatInstruction writes to agent memory: This sends a SYSTEM-prefixed instruction to the agent's /hooks/wake endpoint to modify HEARTBEAT.md. The hooksToken auth header mitigates abuse - verify the server enforces token validation strictly, since a token leak would allow an attacker to overwrite the agent's behavior instructions.
• URL encoding (see above): low severity in a CLI context, but encodeURIComponent is a cheap improvement.

Suggestions

• sendHeartbeatInstruction silently catches all errors. Logging under a --verbose flag would ease troubleshooting.
• The getArgValue helper could be moved to a shared utils module if other commands need it.

Positive Notes

• Clean extraction of getAccountInfo() and AccountInfo interface from whoami.ts - good reuse; the whoamiCommand refactor is noticeably simpler.
• Help text and SKILL.md documentation are thorough and consistent.
• The os.hostname() fallback for FLY_MACHINE_ID is a practical fix for nested shell environments.
• Error handling in apiRequest() gracefully surfaces details[] arrays from the API response.

[github-actions]

AI Code Review - COMMENT. This PR adds push notification support via a new notifications command and refactors whoami.ts to export getAccountInfo(). Feedback: (1) webhook.ts should be renamed notifications.ts for consistency. (2) The whoami refactor loses 401-specific error messaging. (3) The --since flag is interpolated into a query string without validation - add a regex guard. (4) data.instance/data.webhook have no runtime shape validation - add presence guards. Positive: clean extraction of getAccountInfo(), graceful non-fatal error handling, FLY_MACHINE_ID guard is appropriate, good backward-compat aliases and clear help text.

[github-actions]

AI Code Review - COMMENT

Summary: This PR adds a notifications enable command that registers the current Fly machine with a push notification service, configures the local OpenClaw gateway with a hooks token, and sends a one-time setup instruction to the agent.

Actionable Feedback (4 items):

1. webhook.ts:75-77 - getMachineId() returns os.hostname() as a fallback, which virtually always succeeds. The guard if (!machineId) in enableNotifications is effectively unreachable on most machines. Local developer machines will pass the check and accidentally register their hostname with the notifications service. Consider checking FLY_MACHINE_ID directly, or at minimum warn when falling back to hostname.

2. webhook.ts:97-98 - The API response is cast to typed objects without runtime validation. If the server returns an unexpected shape (e.g. instance is null/undefined), the code will throw an unhandled TypeError. A simple existence check before accessing nested properties would prevent a confusing crash.

3. whoami.ts:80-83 - getAccountInfo() swallows all errors (network failures, 401, 403, 5xx) into a single null. The original whoamiCommand surfaced a specific message for HTTP 401. The new code collapses everything to 'Your token may be invalid or expired' which is misleading for transient network errors.

4. File naming: webhook.ts exports notificationsCommand and is imported as ./commands/webhook.js. Consider renaming to notifications.ts to match the command name and codebase conventions.

Code Quality: Clean structure following existing patterns. Extracting getAccountInfo() as a shared export avoids code duplication. The configureHooksOnInstance guard on FLY_MACHINE_ID is sensible but the getMachineId() fallback to os.hostname() undermines it. The pkill call is safe since no user input flows into it.

Security: No credential leakage or injection vulnerabilities found. The sendHeartbeatInstruction call intentionally seeds a system-level prompt into the agent via webhook. Ensure the webhook endpoint validates the Authorization header before acting on message content.

Suggestions: The 'add' backward-compat alias is undocumented; either document it or remove it if never publicly released. The _positionalArg parameter is accepted but unused; consider removing it.

Positive Notes: Good use of Promise.all to parallelize fetches. Silent failure in sendHeartbeatInstruction is appropriate for a best-effort step. SKILL.md docs are concise and accurate. Help output is consistent with existing commands.

[github-actions]

AI Code Review - Recommendation: COMMENT

Summary: This PR adds a notifications enable command that registers a push notification webhook with the Clowdbot notifications service, auto-configures hooks on the running Fly.io instance, and documents the feature in help text and SKILL.md. It also refactors whoami.ts to extract a reusable getAccountInfo() function.

Actionable Feedback (3 items):

1. notifications.ts:74 - require('os').hostname() is a CommonJS require call inside an ES module file. Use a top-level ESM import (import os from 'os') and call os.hostname() instead for consistency.

2. whoami.ts refactor - The original whoamiCommand had specific 401 handling ('Invalid or expired connection token.'), but getAccountInfo() now returns null for any HTTP error. The caller loses this distinction and shows a generic message. Consider returning an error type or throwing on 401 so callers can give a precise message.

3. notifications.ts:55 - sendHeartbeatInstruction() embeds a hardcoded prompt instructing the agent to overwrite its HEARTBEAT.md file with specific text. This is effectively prompt injection into the agent memory store and should be clearly commented so future maintainers understand the intent and trust boundary.

Code Quality: Overall clean and readable. JSDoc comments on internal helpers are a nice touch. The refactoring of whoami.ts to expose getAccountInfo() as a shared utility is the right design. The configureHooksOnInstance() guard on FLY_MACHINE_ID is a good safety valve. The require('os') call is the main style inconsistency. execSync pkill is hardcoded and only runs inside a Fly instance, so blocking the event loop is acceptable for this one-time CLI config call.

Security: hooksToken and webhook config are written to /data/.openclaw/openclaw.json only inside Fly instances with no user-controlled input touching the path. sendHeartbeatInstruction() sends a hardcoded system prompt to the agent webhook asking it to write to HEARTBEAT.md - intentional by design but functionally prompt injection into agent memory, and should be documented as intentional. No credentials or secrets are logged.

Suggestions: (a) Consider adding notifications disable or status subcommands as a follow-up. (b) NOTIFICATIONS_BASE_URL is hardcoded; an env var override would help with local dev and testing.

Positive Notes: The zero-config story for OpenClaw instances is well-executed. getEmailUserId() gracefully handles missing accounts without failing the enable flow. Good separation of concerns across functions. Help text and SKILL.md updated in the same PR.

[github-actions]

AI Code Review — COMMENT

Summary: Adds a notifications enable command that registers a push notification webhook for agent instances, auto-configuring openclaw.json on Fly machines. Also refactors whoami.ts to expose a reusable getAccountInfo() function.

Actionable Feedback

1. whoami.ts:81 — Minor UX regression: original code differentiated 401 from other HTTP errors with distinct messages. Refactored path returns null for all failures so whoamiCommand now shows a single generic message. Consider returning a structured error or re-exposing the response status so the caller can show context-appropriate messages.

2. notifications.ts:72-77 — getMachineId() falls back to os.hostname() when FLY_MACHINE_ID is unset. On non-Fly machines this returns the local hostname, passing the machine-ID guard and submitting a bogus ID to the API. Consider accepting only FLY_MACHINE_ID, or validating the hostname format.

3. notifications.ts:105-111 — Multiple unchecked type casts on the API response (data.instance, data.webhook, etc.). If the shape changes these produce confusing runtime errors. A minimal existence check before casting would improve resilience.

Code Quality: Clean and well-structured. Extracting getAccountInfo() removes duplication. Dynamic import of whoami.js in notifications.ts avoids a circular dependency. Functions are well-separated across configureHooksOnInstance, sendHeartbeatInstruction, and enableNotifications.

Security: No significant concerns. Auth tokens passed via Authorization headers. configureHooksOnInstance is gated behind FLY_MACHINE_ID. The execSync gateway-kill is low-risk in the controlled Fly environment.

Positive Notes: The add alias for backward compat is a thoughtful touch. Documentation updated in help.ts and SKILL.md alongside the code. AccountInfo interface is well-defined and immediately reusable across commands.

[github-actions]

AI Code Review -- Recommendation: COMMENT. This PR adds a notifications enable command that registers a Fly machine with a push-notification service and refactors whoami.ts to expose reusable getAccountInfo/fetchAccountInfo helpers. Issues found: (1) whoami.ts:91-94 - The login hint is now shown for all error cases including network failures, not just 401. Previously it was scoped to the 401 branch. Restore the conditional so it only appears when re-login would actually help. (2) whoami.ts:93 - The HTTP status code was replaced by a generic message, making non-401 failures harder to diagnose. Preserve the status code in the non-401 branch. (3) notifications.ts:103 - res.json() is not inside a try/catch. A non-JSON response body will throw an unhandled exception. Add try/catch or check Content-Type first. (4) notifications.ts:104-108 - Response shape is only validated via type assertions. A runtime guard would make the null-check at line 112 reachable for real-world failure modes. Overall: clean and idiomatic code. The getAccountInfo/fetchAccountInfo refactoring is solid. The trust-boundary comment on sendHeartbeatInstruction is helpful. No injection risks. Parallel Promise.all is preserved correctly. Documentation in SKILL.md and help.ts is complete.

[github-actions]

AI Code Review

Recommendation: COMMENT

Summary: Adds a notifications enable command that registers a Fly machine with the push notification service and auto-configures the OpenClaw gateway webhook token. Also refactors whoami.ts to expose reusable getAccountInfo/fetchAccountInfo helpers.

Actionable Feedback

1. notifications.ts line 57 (sendHeartbeatInstruction): silently swallows non-OK HTTP responses. A 401/403 from token mismatch looks identical to success. The catch block should log a warning on failure so operators know the setup instruction did not land.

2. notifications.ts line 133: (webhook.eventTypes as string[]).join will throw if the server omits eventTypes or returns null. Guard with optional chaining and a fallback empty string.

3. notifications.ts lines 95-109: the webhook response is typed as Record<string, unknown> with repeated casts. A local WebhookInfo interface would eliminate the casts and catch server schema drift at compile time.

4. General/Security: sendHeartbeatInstruction deliberately injects a system-level instruction into the agent HEARTBEAT.md via /hooks/wake. The inline comment documents the trust boundary well. Worth also noting the risk if the notifications backend itself were compromised - the hardcoded string is benign today, but this remote-service-to-agent-memory pathway warrants a threat model note.

Positive Notes

• Zero-config UX is elegant - notifications enable auto-discovers webhook URL and token.
• Idempotency check in configureHooksOnInstance (token === hooksToken and enabled) prevents unnecessary gateway restarts.
• Help text and SKILL.md are kept in sync with the new command.
• whoami.ts refactor is clean: the discriminated return type from fetchAccountInfo makes caller error handling explicit, and the improved error messages (HTTP status codes, network vs auth errors) are a UX win.

[github-actions]

AI Code Review - Recommendation: COMMENT

Summary: This PR adds a notifications enable command that registers a webhook with a push notification service and auto-configures the running OpenClaw instance to receive events via POST to /hooks/wake. It also refactors whoami.ts to export reusable getAccountInfo() and fetchAccountInfo() helpers.

Actionable Feedback:

1. notifications.ts:43 - execSync with pkill -f blocks the Node.js event loop inside an async function. Consider using exec from child_process with a promise wrapper.

2. notifications.ts:43 - pkill -f matches the full command line and could kill unintended processes. Using pkill -x (exact match) or a PID file would be safer.

3. notifications.ts:57-73 - sendHeartbeatInstruction injects a SYSTEM-prefixed instruction into agent memory via the webhook endpoint. If webhookUrl or hooksToken were leaked or the notification service compromised, an attacker could inject arbitrary instructions into HEARTBEAT.md. A dedicated /configure-notifications endpoint would be more robust than a free-form POST to /hooks/wake.

Code Quality: The whoami.ts refactor is clean. Extracting getAccountInfo() and fetchAccountInfo() into exportable helpers with a well-typed AccountInfo interface improves reusability. The fetchAccountInfo return type cleanly distinguishes HTTP errors from network failures without throwing. The notifications module follows existing command patterns. configureHooksOnInstance and sendHeartbeatInstruction could run in parallel via Promise.all since they are independent - minor optimization.

Security: The main concern is sendHeartbeatInstruction, which POSTs free-form text to an AI agent endpoint to modify HEARTBEAT.md. The code comment acknowledges this is intentional prompt injection with hardcoded non-user-controlled text. Risk is manageable if /hooks/wake validates the Bearer token, but a dedicated configuration endpoint would narrow the attack surface. No credentials are logged beyond writing hooksToken to openclaw.json.

Suggestions: help.ts says Manage push notifications but only enable is implemented - consider renaming or planning disable/list subcommands. instance.webhookUrl is used in fetch() without URL validation; a URL check verifying HTTPS would add defense in depth.

Positive Notes: Zero-config design via FLY_MACHINE_ID auto-detection is excellent developer experience. Graceful degradation throughout - non-Fly environments get a clear error, missing emailUserId silently skips, and sendHeartbeatInstruction failures are non-fatal. The trust boundary doc comment shows good security awareness. SKILL.md documentation is clear and consistent.

[github-actions]

AI Code Review

Recommendation: COMMENT

Summary: Adds a notifications enable command that registers a webhook with the notifications service, auto-configures hooks on the running Fly instance, and sends a one-time HEARTBEAT.md instruction to the agent. Also refactors whoami.ts to export reusable getAccountInfo() and fetchAccountInfo() helpers.

Actionable Feedback (3 items):

1. notifications.ts:44 - pkill with -f flag matches any process whose full command line contains openclaw-gateway. On a shared host this could kill unrelated processes. Consider pkill -x openclaw-gateway or a PID file for more targeted process management.

2. notifications.ts:100 - getEmailUserId() splits the email on @ without first checking it contains one. A non-standard email value without @ would cause split to return the entire string as email_user_id. Adding a contains-@ guard before splitting would be defensive.

3. notifications.ts:1 - NOTIFICATIONS_BASE_URL is hardcoded with no env-var override path, making local integration testing hard. Consider checking process.env.NOTIFICATIONS_BASE_URL as a fallback for testability.

Code Quality: Clean, well-typed TypeScript throughout. The separation between getAccountInfo (simple null-returning wrapper) and fetchAccountInfo (returns HTTP status for callers that need it) is a good pattern, arrived at deliberately per the commit history. The configureHooksOnInstance function swallows all outer-catch errors silently; a chalk.gray debug log before returning would help operators diagnose misconfiguration.

Security: The sendHeartbeatInstruction trust-boundary comment is good - it documents that the webhook endpoint must validate the Authorization header before acting on message content. The hardcoded instruction text is controlled by the CLI (not user input), limiting prompt-injection risk. The hostname fallback in getMachineId is gated on the Fly machine ID regex, preventing accidental registration from developer machines. Webhook secret is shown once with a clear save-it warning and HMAC-SHA256 verification guidance.

Positive Notes: Multiple rounds of cleanup visible in the commit history - removing ~200 lines of dead code, improving error messages, adding type safety - the PR landed in noticeably better shape than it started. The whoami.ts refactor is clean and backward-compatible. SKILL.md docs are concise and accurate for what is actually implemented.