fix: use proper /hooks/wake endpoint for Gateway notifications#18
fix: use proper /hooks/wake endpoint for Gateway notifications#18KHAEntertainment merged 2 commits intomasterfrom
Conversation
- Add notifications.py module with correct webhook payload format - Use /hooks/wake endpoint with 'text' field instead of incorrect /hooks/ocbs-proceed endpoint that required custom hook mapping - Add NotificationManager class with proper authentication handling - Add file-based fallback when webhook fails - Update serve.py to use new notifications module - Add notification config methods to integration.py Fixes #2 - 'hook mapping requires message' error Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughA new notification system has been introduced that enables OCBS to send webhook notifications to an OpenClaw Gateway endpoint, with file-based fallback support. This includes a dedicated Changes
Sequence DiagramssequenceDiagram
participant OCBS as OCBS Service
participant NM as NotificationManager
participant GW as Gateway API
participant FS as File Storage
OCBS->>NM: notify_proceed(checkpoint_id, token)
NM->>NM: Build webhook URL<br/>(from env/config)
NM->>NM: Add Authorization header<br/>(if token available)
NM->>GW: POST /hooks/wake<br/>(JSON payload)
alt Webhook Success
GW-->>NM: 200 OK
NM-->>OCBS: return True
else Webhook Failure
GW-->>NM: Error or timeout
NM->>FS: Write proceed_notifications/<br/>token.json + metadata
NM-->>OCBS: return True (fallback)
end
sequenceDiagram
participant Config as Configuration
participant Integration as OCBSIntegration
participant NM as NotificationManager
participant GW as Gateway API
Config->>Integration: Load config file
Integration->>Integration: setup_notifications(enabled, host)
Integration->>Integration: Construct webhook_url<br/>from host + port 18789
Integration->>Config: save_config() persist settings
Integration-->>Config: return status message
activate NM
NM->>NM: Read notification_enabled,<br/>webhook_url from config
NM->>NM: Build full URL with<br/>token from env/file
deactivate NM
NM->>GW: POST with Authorization
NM-->>Integration: Notification delivery result
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ocbs/serve.py (2)
700-700:⚠️ Potential issue | 🔴 CriticalStructural issue:
do_POSTappears nested insidedo_GET.Similar to the earlier issue,
do_POSTat line 700 is indented insidedo_GET. These should be sibling methods on the HTTP handler class. This will cause HTTP POST requests to fail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ocbs/serve.py` at line 700, The do_POST method is incorrectly nested inside do_GET, making it a local function instead of a sibling handler; move the def do_POST(self): block out one indentation level so it is defined at the same class level as do_GET (i.e., under the HTTP handler class, not inside do_GET), ensuring any references to self and class attributes remain unchanged and tests for POST handling will execute.
59-98:⚠️ Potential issue | 🔴 CriticalCritical structural issue: Methods incorrectly nested inside
get_tailscale_ip()function.The methods
_generate_token(),_create_serve_record(), and others (lines 59–98) are indented at 4 spaces, placing them inside theget_tailscale_ip()function rather than at class level. Since these methods referenceself, they're intended as instance methods but are unreachable in their current location. This will causeAttributeErrorwhen the code tries to callself._generate_token()at runtime.These methods need to be dedented and placed at the proper class level—likely they belong to a server handler class that's missing or misplaced in the file structure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ocbs/serve.py` around lines 59 - 98, The methods _generate_token and _create_serve_record (and any other methods accidentally indented under get_tailscale_ip) are incorrectly nested inside the get_tailscale_ip() function; dedent these methods so they are defined at the class level (same indentation as get_tailscale_ip) so they can be accessed via self (e.g., self._generate_token(), self._create_serve_record()); ensure they remain part of the correct server/handler class, keep their references to self._active_tokens and self.core.db_path intact, and run a quick lint/flake8 check to confirm no other methods were unintentionally nested.
🧹 Nitpick comments (5)
src/ocbs/integration.py (1)
198-218:setup_notificationshardcodes port 18789 - consider parameterizing.The method constructs the webhook URL with a hardcoded port (line 212). If users need a different port, they'd have to manually edit the config after calling this method, which is a bit awkward.
Consider adding an optional
portparameter for flexibility:♻️ Suggested improvement
- def setup_notifications(self, enabled: bool = True, webhook_host: str = '127.0.0.1') -> str: + def setup_notifications(self, enabled: bool = True, webhook_host: str = '127.0.0.1', port: int = 18789) -> str: """Configure notification settings. Args: enabled: Enable webhook notifications webhook_host: Host where OpenClaw Gateway is running + port: Gateway port (default: 18789) Returns: Status message """ config = self.get_config() config['notification_enabled'] = enabled config['webhook_host'] = webhook_host - # Default port is 18789 (OpenClaw default) - config['webhook_url'] = f'http://{webhook_host}:18789/hooks/wake' + config['webhook_url'] = f'http://{webhook_host}:{port}/hooks/wake' self.save_config(config)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ocbs/integration.py` around lines 198 - 218, The setup_notifications method currently hardcodes port 18789 into config['webhook_url']; update the signature of setup_notifications (function name: setup_notifications) to accept an optional port parameter (e.g., port: int = 18789), use that port when constructing config['webhook_url'] (f'http://{webhook_host}:{port}/hooks/wake'), and update the docstring to document the new port parameter; keep using get_config and save_config so existing behavior and return messages ('Notifications enabled - Gateway webhook at ...' / 'Notifications disabled') remain unchanged.src/ocbs/notifications.py (4)
29-41: URL construction from environment variables - consider basic scheme validation.The URL is constructed from user-controlled environment variables (
OCBS_WEBHOOK_URL,OCBS_WEBHOOK_HOST). While unlikely to be exploited in this context, there's no validation that the URL useshttp://orhttps://schemes.If someone sets
OCBS_WEBHOOK_URL=file:///etc/passwd,urlopenwould happily try to read it. Think of it like accepting any address for a delivery - you'd want to at least verify it's a real address format.🛡️ Add scheme validation
def _get_gateway_url(self) -> str: """Get the Gateway webhook URL from environment or default.""" # Check for explicit URL first if os.environ.get("OCBS_WEBHOOK_URL"): - return os.environ["OCBS_WEBHOOK_URL"] + url = os.environ["OCBS_WEBHOOK_URL"] + if not url.startswith(("http://", "https://")): + logger.warning(f"Invalid URL scheme in OCBS_WEBHOOK_URL, using default") + else: + return url # Check for custom port port = os.environ.get("OPENCLAW_GATEWAY_PORT", self.DEFAULT_GATEWAY_PORT)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ocbs/notifications.py` around lines 29 - 41, The _get_gateway_url function returns a URL derived from OCBS_WEBHOOK_URL or constructed from OCBS_WEBHOOK_HOST and OPENCLAW_GATEWAY_PORT without validating the URI scheme; update _get_gateway_url to validate that any provided OCBS_WEBHOOK_URL has an http or https scheme (reject or ignore other schemes like file://) and, for the constructed URL, ensure you explicitly prepend "http://" or "https://" and validate the resulting URL/parsing (use urllib.parse.urlparse to check scheme and netloc) so only http/https addresses are returned and invalid values fall back to the safe default behavior.
36-36: Environment variable type coercion may cause issues.Line 36 gets
OPENCLAW_GATEWAY_PORTfrom environment but doesn't convert it to int. The f-string at line 41 will work fine with a string, but if this value is used elsewhere expecting an integer, it could cause issues.Minor, but worth noting for consistency:
♻️ Ensure port is integer
# Check for custom port - port = os.environ.get("OPENCLAW_GATEWAY_PORT", self.DEFAULT_GATEWAY_PORT) + port = int(os.environ.get("OPENCLAW_GATEWAY_PORT", self.DEFAULT_GATEWAY_PORT))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ocbs/notifications.py` at line 36, The environment-read for OPENCL_GATEWAY_PORT assigns a string to port (port = os.environ.get("OPENCLAW_GATEWAY_PORT", self.DEFAULT_GATEWAY_PORT)); change this to coerce to an int and handle bad values: attempt to int(os.environ.get(...)) and on ValueError fallback to self.DEFAULT_GATEWAY_PORT (or log a warning) so port is always an integer when used elsewhere; refer to the variable name port and constant self.DEFAULT_GATEWAY_PORT in your change.
281-290: Module singleton may have stale configuration if state_dir changes.The
_default_managersingleton is initialized with the defaultstate_diron first use. If different parts of the code need differentstate_dirvalues, the singleton won't reflect that.In
serve.py, you're correctly creating a newNotificationManager(state_dir=self.state_dir)instead of using the module functions, so this is handled. Just flagging that the module-level convenience functions assume a fixed default directory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ocbs/notifications.py` around lines 281 - 290, Module-level singleton _default_manager is initialized with a fixed default state_dir and can become stale if callers need a different directory; modify get_notification_manager to accept an optional state_dir parameter and ensure it creates or reinitializes _default_manager when the provided state_dir differs (compare against _default_manager.state_dir or store the last used state_dir), or add a set_notification_manager(state_dir: str) factory that constructs a new NotificationManager(state_dir=...) and assigns it to _default_manager; update references to call the new API (get_notification_manager(state_dir=...) or set_notification_manager) and ensure NotificationManager() construction uses the provided state_dir.
91-108: Good timeout and error handling, but consider narrowing the exception catch.The 5-second timeout is sensible for webhook delivery. However, catching bare
Exceptionat line 106 could mask programming errors (likeTypeErrorfrom bad arguments). The static analyzer flagged this too.For network code, it's usually better to catch specific exceptions you expect:
♻️ Narrow exception handling
except urllib.error.HTTPError as e: logger.warning(f"Webhook HTTP error: {e.code} - {e.reason}") return False except urllib.error.URLError as e: logger.warning(f"Webhook connection failed: {e.reason}") return False - except Exception as e: - logger.warning(f"Webhook unexpected error: {e}") + except (OSError, TimeoutError, ValueError) as e: + logger.warning(f"Webhook error: {e}") return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ocbs/notifications.py` around lines 91 - 108, The catch-all "except Exception as e" after the urllib.request.urlopen block should be removed to avoid masking programming errors; instead, extend the existing exception handling to explicitly catch only expected network-related errors (e.g., socket.timeout, ssl.SSLError, OSError and optionally ValueError for bad input) alongside the existing urllib.error.HTTPError and urllib.error.URLError handlers, and log the exception via the same logger (logger.warning) with context; update the try/except around urllib.request.Request/urllib.request.urlopen in the notification send routine so only those specific exceptions are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ocbs/integration.py`:
- Around line 181-196: save_notification_config writes webhook_token into the
OCBS config but NotificationManager._get_auth_token only checks env
OCBS_WEBHOOK_TOKEN and gateway.auth.token in the OpenClaw config, so tokens set
via save_notification_config are ignored; update
NotificationManager._get_auth_token to also load and check the OCBS config JSON
(where save_notification_config stores 'webhook_token') and return that token if
present (falling back to env and gateway.auth.token as before), ensuring the
code paths in NotificationManager._get_auth_token and save_notification_config
reference the same config key name 'webhook_token'.
In `@src/ocbs/serve.py`:
- Around line 214-225: The current flow in _send_webhook_notification delegates
to NotificationManager.notify_proceed(token, checkpoint_id) but notify_proceed
only writes the proceed-specific file when the webhook itself succeeds; when
notify_proceed falls back to a generic notification it returns True and no
proceed file is created. Modify notify_proceed (in notifications.py) so it
always creates the proceed file proceed_notifications/<token>.json (including
checkpoint_id) regardless of whether the delivery used the webhook or the
fallback path, or alternatively have _send_webhook_notification detect a
fallback return value (or add a return enum) and call
_write_proceed_notification(token, checkpoint_id) when needed; update
notify_proceed/_send_webhook_notification accordingly to ensure a proceed file
is always written.
---
Outside diff comments:
In `@src/ocbs/serve.py`:
- Line 700: The do_POST method is incorrectly nested inside do_GET, making it a
local function instead of a sibling handler; move the def do_POST(self): block
out one indentation level so it is defined at the same class level as do_GET
(i.e., under the HTTP handler class, not inside do_GET), ensuring any references
to self and class attributes remain unchanged and tests for POST handling will
execute.
- Around line 59-98: The methods _generate_token and _create_serve_record (and
any other methods accidentally indented under get_tailscale_ip) are incorrectly
nested inside the get_tailscale_ip() function; dedent these methods so they are
defined at the class level (same indentation as get_tailscale_ip) so they can be
accessed via self (e.g., self._generate_token(), self._create_serve_record());
ensure they remain part of the correct server/handler class, keep their
references to self._active_tokens and self.core.db_path intact, and run a quick
lint/flake8 check to confirm no other methods were unintentionally nested.
---
Nitpick comments:
In `@src/ocbs/integration.py`:
- Around line 198-218: The setup_notifications method currently hardcodes port
18789 into config['webhook_url']; update the signature of setup_notifications
(function name: setup_notifications) to accept an optional port parameter (e.g.,
port: int = 18789), use that port when constructing config['webhook_url']
(f'http://{webhook_host}:{port}/hooks/wake'), and update the docstring to
document the new port parameter; keep using get_config and save_config so
existing behavior and return messages ('Notifications enabled - Gateway webhook
at ...' / 'Notifications disabled') remain unchanged.
In `@src/ocbs/notifications.py`:
- Around line 29-41: The _get_gateway_url function returns a URL derived from
OCBS_WEBHOOK_URL or constructed from OCBS_WEBHOOK_HOST and OPENCLAW_GATEWAY_PORT
without validating the URI scheme; update _get_gateway_url to validate that any
provided OCBS_WEBHOOK_URL has an http or https scheme (reject or ignore other
schemes like file://) and, for the constructed URL, ensure you explicitly
prepend "http://" or "https://" and validate the resulting URL/parsing (use
urllib.parse.urlparse to check scheme and netloc) so only http/https addresses
are returned and invalid values fall back to the safe default behavior.
- Line 36: The environment-read for OPENCL_GATEWAY_PORT assigns a string to port
(port = os.environ.get("OPENCLAW_GATEWAY_PORT", self.DEFAULT_GATEWAY_PORT));
change this to coerce to an int and handle bad values: attempt to
int(os.environ.get(...)) and on ValueError fallback to self.DEFAULT_GATEWAY_PORT
(or log a warning) so port is always an integer when used elsewhere; refer to
the variable name port and constant self.DEFAULT_GATEWAY_PORT in your change.
- Around line 281-290: Module-level singleton _default_manager is initialized
with a fixed default state_dir and can become stale if callers need a different
directory; modify get_notification_manager to accept an optional state_dir
parameter and ensure it creates or reinitializes _default_manager when the
provided state_dir differs (compare against _default_manager.state_dir or store
the last used state_dir), or add a set_notification_manager(state_dir: str)
factory that constructs a new NotificationManager(state_dir=...) and assigns it
to _default_manager; update references to call the new API
(get_notification_manager(state_dir=...) or set_notification_manager) and ensure
NotificationManager() construction uses the provided state_dir.
- Around line 91-108: The catch-all "except Exception as e" after the
urllib.request.urlopen block should be removed to avoid masking programming
errors; instead, extend the existing exception handling to explicitly catch only
expected network-related errors (e.g., socket.timeout, ssl.SSLError, OSError and
optionally ValueError for bad input) alongside the existing
urllib.error.HTTPError and urllib.error.URLError handlers, and log the exception
via the same logger (logger.warning) with context; update the try/except around
urllib.request.Request/urllib.request.urlopen in the notification send routine
so only those specific exceptions are caught.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c1d5a4f-8107-478c-8101-b2222950f103
📒 Files selected for processing (3)
src/ocbs/integration.pysrc/ocbs/notifications.pysrc/ocbs/serve.py
| def _send_webhook_notification(self, token: str, checkpoint_id: str = None): | ||
| """Send webhook notification to gateway when user clicks proceed.""" | ||
| import urllib.request | ||
| import urllib.error | ||
|
|
||
| # Get webhook URL from environment or use default | ||
| webhook_url = os.environ.get('OCBS_WEBHOOK_URL', 'http://localhost:18789/hooks/ocbs-proceed') | ||
| webhook_token = os.environ.get('OCBS_WEBHOOK_TOKEN', 'ocbs-webhook-secret') | ||
|
|
||
| payload = json.dumps({ | ||
| "message": f"OCBS Proceed: User acknowledged checkpoint {checkpoint_id}. Token: {token}", | ||
| "token": token, | ||
| "checkpoint_id": checkpoint_id, | ||
| "action": "proceed" | ||
| }).encode('utf-8') | ||
|
|
||
| req = urllib.request.Request( | ||
| webhook_url, | ||
| data=payload, | ||
| headers={ | ||
| 'Content-Type': 'application/json', | ||
| 'Authorization': f'Bearer {webhook_token}' | ||
| }, | ||
| method='POST' | ||
| ) | ||
|
|
||
| try: | ||
| with urllib.request.urlopen(req, timeout=5) as response: | ||
| print(f"Webhook notification sent: {response.status}") | ||
| except urllib.error.URLError as e: | ||
| print(f"Webhook notification failed: {e}") | ||
| # Fall back to file notification | ||
| """Send webhook notification to gateway when user clicks proceed. | ||
|
|
||
| Uses the proper /hooks/wake endpoint with 'text' field format | ||
| instead of the incorrect /hooks/ocbs-proceed endpoint. | ||
| """ | ||
| notifier = NotificationManager(state_dir=self.state_dir) | ||
| success = notifier.notify_proceed(checkpoint_id, token) | ||
|
|
||
| if not success: | ||
| # Fall back to file notification if webhook fails | ||
| self._write_proceed_notification(token, checkpoint_id) |
There was a problem hiding this comment.
The delegation to NotificationManager looks good, but there's a subtle double-write risk.
The refactored code properly delegates to NotificationManager.notify_proceed(). However, when looking at the interaction with notify_proceed() in notifications.py (lines 254-266), that method already writes a proceed notification file when the webhook succeeds.
So if the webhook succeeds:
notify_proceed()writes toproceed_notifications/<token>.json(line 258-266 in notifications.py)- It returns
True _send_webhook_notificationexits without calling_write_proceed_notification
This is actually fine! But if the webhook fails and fallback is used:
notify()writes tonotifications/ocbs_<timestamp>.json(generic fallback)notify_proceed()returnsTrue(because fallback succeeded)- No proceed-specific file is written
The proceed notification file is only written on webhook success, but arguably it should also be written on fallback. You might want notify_proceed() to write the proceed file regardless of how the notification was delivered.
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 214-214: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ocbs/serve.py` around lines 214 - 225, The current flow in
_send_webhook_notification delegates to
NotificationManager.notify_proceed(token, checkpoint_id) but notify_proceed only
writes the proceed-specific file when the webhook itself succeeds; when
notify_proceed falls back to a generic notification it returns True and no
proceed file is created. Modify notify_proceed (in notifications.py) so it
always creates the proceed file proceed_notifications/<token>.json (including
checkpoint_id) regardless of whether the delivery used the webhook or the
fallback path, or alternatively have _send_webhook_notification detect a
fallback return value (or add a return enum) and call
_write_proceed_notification(token, checkpoint_id) when needed; update
notify_proceed/_send_webhook_notification accordingly to ensure a proceed file
is always written.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
1 participant
Summary
notifications.pymodule with correct OpenClaw Gateway webhook format/hooks/wakeendpoint withtextfield instead of incorrect/hooks/ocbs-proceedNotificationManagerclass with proper authentication and file-based fallbackProblem
OCBS was sending webhooks to
/hooks/ocbs-proceedendpoint withmessagefield, but:/hooks/wakewhich usestextfieldSolution
notifications.py: New module that sends to/hooks/wakewith proper{"text": "...", "mode": "now"}payloadserve.py: Updated to use newNotificationManagerinstead of inline webhook codeintegration.py: Added notification configuration methodsTest plan
notify_proceed()sends correct payload to/hooks/wakeFixes #2
Summary by CodeRabbit