You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.
Use the following commands to manage reviews:
@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.
Use the checkboxes below for quick actions:
▶️ Resume reviews
🔍 Trigger review
📝 Walkthrough
Walkthrough
The change adds Wake-on-LAN packet transmission with input validation, exposes it through ExternalIntegration, and adds an authenticated POST /api/integration/v1/network/wake endpoint. Tests cover packet formats, validation, source ports, successful responses, and error propagation.
Changes
Wake-on-LAN feature
Layer / File(s)
Summary
Packet construction and transmission server/lib/external-integration/externalIntegration.wakeOnLan.js, server/test/lib/external-integration/externalIntegration.wakeOnLan.test.js
The implementation validates MAC addresses, IPv4 addresses, ports, options, and permissions. It builds and sends UDP/IPv4 magic packets. Tests cover packet contents, MAC formats, source ports, invalid options, permission failures, and socket errors.
ExternalIntegration API wiring server/lib/external-integration/index.js
wakeOnLan is imported and exposed on the ExternalIntegration prototype.
The controller forwards request options and returns { success: true }. The authenticated POST route is registered. Tests cover route registration, successful calls, and error propagation.
I’m a rabbit with packets to send,
A magic burst hops round the bend.
MACs line up, ports glow bright,
The network wakes before moonlight.
Errors return, tests cheer: “All clear!”
Sequence Diagram(s)
sequenceDiagram
participant Client
participant IntegrationRoute
participant IntegrationHostController
participant ExternalIntegration
participant UDPSocket
Client->>IntegrationRoute: POST /api/integration/v1/network/wake
IntegrationRoute->>IntegrationHostController: authenticate and dispatch request
IntegrationHostController->>ExternalIntegration: wakeOnLan(service, options)
ExternalIntegration->>UDPSocket: bind and send magic packet
UDPSocket-->>ExternalIntegration: report completion or error
ExternalIntegration-->>IntegrationHostController: resolve or reject
IntegrationHostController-->>Client: success response or propagated error
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Thanks for this — the core idea matches a real Host API gap (bridge containers cannot emit LAN broadcasts, so WoL has to be mediated by the core, same family as B.16).
Requesting changes before merge:
Living spec missing — docs/specs/external-integrations.md is the source of truth for Host API contracts. This PR adds POST /api/integration/v1/network/wake and must document it in the same diff (C.3 at minimum; C.1/C.8 if a manifest/SDK surface is introduced).
No authorization contract — sibling host-mediated capabilities (network_discovery, location) are declared in the manifest, shown at install time, and enforced server-side with 403. This endpoint is available to every authenticated external integration with no declaration, no disclosure, and no rate bound, while emitting UDP from the core’s network=host namespace to an arbitrary IPv4/port. That needs a deliberate design choice (likely a manifest flag + gate, and probably a rate limit), not an ungated free-for-all.
Invalid MAC returns a generic Error → HTTP 500 instead of BadParameters → 400.
UDP socket leak on the error path when bind/send fails before close().
Magic-packet construction, MAC format support, and the unit tests look solid. No device categories/types involved.
Flagged needs:human-review and requested @Pierre-Gilles — this is a Host API / external-integration authorization-contract question and should align with the living-spec philosophy before landing.
✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.49%. Comparing base (5416580) to head (6f4dab9). ⚠️ Report is 1 commits behind head on master.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/lib/external-integration/externalIntegration.wakeOnLan.js`:
- Around line 15-19: Ensure every malformed Wake-on-LAN request returns
BadParameters: update normalizeMac to throw BadParameters, validate the options
object before destructuring in wakeOnLan, and have integrationHost.controller
pass req.body to the service. Update the specified tests to assert BadParameters
for malformed MACs, null options, and other invalid options; apply changes at
server/lib/external-integration/externalIntegration.wakeOnLan.js lines 15-19 and
49-52, server/api/controllers/integrationHost.controller.js lines 142-148, and
the corresponding test ranges at lines 133-148 and 215-233.
- Around line 72-93: Ensure every UDP socket is closed on failure: in
externalIntegration.wakeOnLan.js, close the production socket when bind emits an
error; in
server/test/lib/external-integration/externalIntegration.wakeOnLan.test.js lines
13-24, close each test receiver in finally; and in lines 41-130, make
waitForUdpMessage remove its message listener on timeout or abort and close
receivers in finally, including failed sends, timeouts, and assertion failures.
In `@server/test/lib/external-integration/externalIntegration.wakeOnLan.test.js`:
- Around line 235-307: Move the networkWake success and error controller tests
from server/test/lib/external-integration/externalIntegration.wakeOnLan.test.js
lines 235-307 to server/test/api/controllers/integrationHost.controller.test.js,
preserving coverage of option forwarding, success responses, and error
propagation. Add a mirrored route-registration test for server/api/routes.js
lines 673-677 that verifies externalIntegrationAuth is true and the route
handler is integrationHostController.networkWake.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d4826bf3-5058-44bb-92ab-28581375f9f1
📥 Commits
Reviewing files that changed from the base of the PR and between 12bc5f2 and a585e5d.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/lib/external-integration/externalIntegration.wakeOnLan.js`:
- Line 56: Validate the address in the options-handling flow before calling
dgram.Socket#send, using net.isIPv4(address) and rejecting non-IPv4 values,
including hostnames and “invalid-ip”, with BadParameters. Add or update the
relevant test to assert that invalid-ip produces BadParameters.
In `@server/test/lib/external-integration/externalIntegration.wakeOnLan.test.js`:
- Around line 6-10: Add a denial test in the external integration suite using a
service fixture whose manifest omits network_wake, then invoke the relevant
wake-on-LAN operation and assert that it rejects with ForbiddenError. Keep the
existing permitted-service fixture and success coverage unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 562c5040-abeb-4b51-846e-9a8a0c1dcd6a
📥 Commits
Reviewing files that changed from the base of the PR and between a585e5d and e7fd19a.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/test/lib/external-integration/externalIntegration.wakeOnLan.test.js`:
- Line 3: Replace the shared Sinon singleton import in the test file with a
per-file Sinon sandbox, and update the tests to create, use, and restore that
sandbox for stubs and cleanup.
- Around line 283-342: Update the external integration test file to use a
per-file Sinon sandbox by creating it from require('sinon').createSandbox(), and
use that sandbox for stubbing and restoration. Add coverage for the socket
error-event path in wakeOnLan: capture the error listener registered through
socket.once, invoke it while the operation is pending, assert the original error
is rejected, and verify socket.close is called.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: daa34bac-bb40-4ef0-ae3c-171694c53fa1
📥 Commits
Reviewing files that changed from the base of the PR and between a5df4b3 and 25e29b0.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Re-reviewed at 21d53300 after the follow-up commits. Thanks for addressing the earlier BadParameters MAC path and the UDP socket close-on-error leak — those look good.
Still requesting changes before merge:
Authorization contract is only half-wired. Runtime now gates on service.manifest.network_wake with ForbiddenError, which is the right direction — but network_wake is not in MANIFEST_FIELDS / manifest.schema.json, is not validated as a boolean, is not shown on the install screen, and is not documented in docs/specs/external-integrations.md. Because unknown top-level manifest keys are rejected (network_wake: unknown field), a real integration cannot declare the capability through the normal install/update path, so the Host API is unreachable in production while also skipping the user-consent surface that location / network_discovery / webhooks provide. Please complete the full contract in this PR (schema + validator + install disclosure + living spec C.1/C.3; SDK follow-up in C.8 is fine later).
Missing/non-string mac still becomes HTTP 500.{} / { mac: null } skip the options-object guard, pass the permission check, then crash in normalizeMac on .replace.
Codecov patch coverage is not 100%. CI reports uncovered lines 88-89 and 106-107 (the if (settled) return branches). Gladys requires 100% patch coverage on changed server lines — add a socket error-event / double-settle test (CodeRabbit already flagged the missing error path).
Magic-packet construction, MAC format support, IPv4/port validation, controller wiring, and the unit tests that exist remain solid. No device categories/types involved.
Server test is red on this run, but the failure is the known parallel t_session schema-drift flake (table t_session has 15 columns but 14 values were supplied), not the WoL suite (all externalIntegration.wakeOnLan / networkWake cases passed).
Keeping needs:human-review + @Pierre-Gilles — Host API authorization-contract / LAN-emission philosophy (including whether arbitrary unicast destinations and unbounded wake rate are acceptable vs B.16’s broadcast-only + rate-bound active scan). Not tagging risk:high: fixed 102-byte WoL payload, not a general UDP proxy.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Re-reviewed at 7b7f8f3d after the follow-up commits. Thanks — the previous blockers are largely fixed:
network_wake is now in MANIFEST_FIELDS / manifest.schema.json, validated as boolean, shown on the install screen, and gated with ForbiddenError
missing/non-string mac → BadParameters
socket close-on-error + double-settle coverage
CI is green (including codecov/patch)
Still requesting changes before merge:
German install consent text is wrong.de.jsonnetworkWakeText still describes network discovery (“Netzwerkankündigungen … erfassen”), not Wake-on-LAN. That breaks the authorization-contract disclosure for German users — please translate the EN/FR meaning.
Living-spec C.3 entry is unfinished. The new POST /network/wake line is truncated (100% reliable wake-on-LAN,) and overclaims reliability. Please finish it in the style of GET /house / camera: required network_wake: true → 403, optional address/port/sourcePort defaults, fixed 102-byte magic packet. Also add network_wake to the C.1 prose list of authorization contracts (containers, network_discovery, webhooks, location, …).
Soft / non-blocking:
Rename NetworkWakeSumamary.jsx → NetworkWakeSummary.jsx (typo in the filename).
Prefer service.manifest && service.manifest.network_wake === true like getHouses / location.
SDK helper (C.8) can land as a follow-up once the Host API contract is settled.
Keeping needs:human-review + @Pierre-Gilles for the Host API philosophy question left open: arbitrary unicast destinations + no per-integration rate bound vs B.16’s broadcast-only + 1/10s active-scan guardrails. Not tagging risk:high: fixed 102-byte WoL payload, not a general UDP proxy. No device categories/types involved.
CI is green (Front/Server/Cypress/Docker/codecov patch+project).
Approving from a code/contract standpoint. Soft nits left inline (spec punctuation; tests asserting Error instead of BadParameters).
Keeping needs:human-review + @Pierre-Gilles for the Host API philosophy still open vs B.16: arbitrary unicast destinations and no per-integration rate bound. Not tagging risk:high — fixed magic-packet payload, not a general UDP proxy. No device categories/types involved. SDK helper (C.8) can land as a follow-up.
The reason will be displayed to describe this comment to others. Learn more.
Re-reviewed at 1d9c787. The two leftover nits from the previous approval are fixed — thanks:
C.3 now matches sibling Host API entries (em dash, \u2192, capitalized “Sends”)
Invalid destination/source port tests assert BadParameters (so a generic Error → HTTP 500 would fail the suite)
The Host API contract remains complete: network_wake in C.1 + schema/validator, install-screen disclosure (en/fr/de), strict network_wake === true gate, fixed 102-byte magic packet, MAC/IPv4/port validation → BadParameters.
Approving again from a code/contract standpoint (previous approval was dismissed by this push).
Keeping needs:human-review + @Pierre-Gilles for the Host API philosophy still open vs B.16: arbitrary unicast destinations and no per-integration rate bound. Not tagging risk:high — fixed magic-packet payload, not a general UDP proxy. No device categories/types involved.
Current Server test red is unrelated: zigbee2mqttManager.publish “should log the topic and the message published” (AssertError on logger.debug). All WoL / networkWake cases passed (5672 passing). Worth a re-run; not caused by this diff.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
github-actions
Bot
added
area:server
Pierre-Gilles
added
the
needs:human-review
Cursor
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Description
Add Wake-on-LAN support to the external integration Host API.
External integrations can now request Gladys Core to send a Wake-on-LAN magic packet from its network namespace through:
POST /api/integration/v1/network/wake
The API supports:
This allows external integrations running in isolated Docker networks to wake LAN devices through Gladys Core.
Tests cover magic packet generation, MAC address formats, source port handling, input validation and the external integration Host API controller.
Forum
Forum: https://community.gladysassistant.com/t/api-sdk-wake-on-lan-via-le-coeur-gladys/10522
Checklist
cd server && npm run coverage(Codecov requires 100% coverage on changed lines) and Cypress (npm run cypress:run) if the UI changednpm run eslint,npm run prettier)Summary by CodeRabbit