feat(api): MCP & OpenAPI implementation #1405
Conversation
- Reimplemented MCP Server and OpenAPI Spec Generator. - STRICTLY preserved legacy API return formats (Lists/404s) for existing endpoints to ensure zero breakage. - Added Pydantic schemas using RootModel for legacy List responses. - Added @validate_request decorator for input validation (returns 422). - Added comprehensive tests for MCP endpoints and legacy compatibility.
📝 WalkthroughWalkthroughThis PR implements a comprehensive API refactoring centered on OpenAPI/MCP (Model Context Protocol) compatibility. It introduces centralized request validation via Pydantic v2 schemas, a registry-driven OpenAPI spec generator, payload-driven endpoint architecture, MCP session management with SSE support, and extensive validation test coverage. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API as Flask API
participant Validator as `@validate_request` Decorator
participant Registry as MCP Registry
participant Spec as OpenAPI Spec
participant Tools as Tool Mapper
participant MCP as MCP SSE Handler
participant HTTP as Internal HTTP
Client->>API: POST /api/endpoint with payload
API->>Validator: Request received (payload=None)
Validator->>Validator: Validate payload against Pydantic model
alt Validation Success
Validator->>Registry: Payload valid, call endpoint
Registry->>Registry: Store in registry if needed
Validator->>API: Inject validated payload
API->>HTTP: Process business logic
HTTP-->>API: Response result
API-->>Client: 200 + response_model
else Validation Error
Validator-->>Client: 422 Unprocessable Entity
end
Client->>API: GET /openapi.json
API->>Spec: generate_openapi_spec(flask_app)
Spec->>Registry: get_registry()
Registry-->>Spec: List all registered endpoints
Spec->>Tools: map_openapi_to_mcp_tools(spec)
Tools-->>Spec: Convert to MCP tool definitions
Spec-->>Client: 200 + OpenAPI 3.1 document
Client->>MCP: GET /mcp/sse (establish session)
MCP->>Registry: create_session()
Registry-->>MCP: session_id
MCP->>MCP: Initialize SSE stream
MCP-->>Client: 200 + SSE stream (session_id)
Client->>MCP: POST /mcp/messages (send tool request)
MCP->>Registry: get_session(session_id)
MCP->>Tools: find_route_for_tool(tool_name)
Tools->>Registry: Lookup in operation registry
Registry-->>Tools: Route entry
Tools->>Tools: _execute_tool(route, args)
Tools->>HTTP: Call internal endpoint
HTTP-->>Tools: Result
Tools-->>MCP: Tool result
MCP->>MCP: Queue response to session
MCP-->>Client: SSE message event
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/api_server/api_server_start.py (1)
545-577: Add timeout parameter to subprocess.run() call.The Pydantic validator correctly rejects invalid MACs at the schema level (via
validate_mac()which enforcesMAC_PATTERN), so invalid MACs like"invalid-mac"are properly rejected beforewakeonlan()is reached.However, the
subprocess.run()call on line 47-48 is missing the requiredtimeoutparameter. The coding guidelines mandate explicit timeouts (default 60 seconds minimum) for all subprocess calls. Other subprocess calls in this same file (speedtest, traceroute) correctly includetimeout=60. Addtimeout=60to the wakeonlan subprocess call to prevent potential hangs.
🤖 Fix all issues with AI agents
In @schema_validation.py:
- Around line 29-35: The file currently sets DB_PATH from the imported
fullDbPath; change it to read the database path from the NETALERTX_DB
environment variable instead. Replace usage of fullDbPath with
os.getenv("NETALERTX_DB") and set DB_PATH to that value, and if the env var is
missing or empty log/print a clear error and exit (mirror existing error
handling style) so the script fails fast; ensure imports include os and remove
or stop relying on fullDbPath from the const module.
In @server/api_server/schemas.py:
- Around line 25-27: The current IP_PATTERN only matches IPv4 and causes
validate_ip() (used by TracerouteRequest and NslookupRequest) to reject IPv6;
update validation to accept both IPv4 and IPv6 by replacing the IP_PATTERN
usage: either change the field types in the request models to use Pydantic's
IPvAnyAddress (or IPv4Address/IPv6Address as appropriate) or replace IP_PATTERN
with a regex that matches IPv6 as well and use that in validate_ip(); locate and
update the validate_ip() function and any references in
TracerouteRequest/NslookupRequest to use the new IPvAnyAddress type or the
revised pattern so IPv6 targets are accepted.
In @server/api_server/spec_generator.py:
- Around line 483-505: The OpenAPI registration for the "/devices/by-status"
endpoint only lists method="POST" in the call to register_tool
(operation_id="list_devices_by_status"), but the Flask route accepts GET as
well; update the register_tool invocation for path="/devices/by-status" to
include the GET method (or change to method=["GET","POST"] / equivalent in your
register_tool API) so the spec matches the Flask route, or add a comment
explaining why GET should be omitted if intentional.
In @server/api_server/validation.py:
- Around line 91-100: The current try/except in the wrapper quietly swallows
TypeError and retries without 'payload', which can mask real bugs; instead,
detect whether the target function f actually accepts a 'payload' parameter
(e.g., inspect.signature(f) or use hasattr on its parameters) before calling and
if it does not, log a clear error including operation_id and raise a new
informative TypeError (or re-raise the original) rather than deleting kwargs and
retrying; remove the silent fallback that deletes kwargs['payload'] and ensure
the wrapper fails fast when a decorated view does not accept the expected
'payload' argument.
🧹 Nitpick comments (16)
requirements.txt (1)
35-36: Consider pinning pytest as a dev dependency.
pytestis typically a development/test dependency and may be better placed in a separaterequirements-dev.txtorpyproject.tomlunder[project.optional-dependencies]. Additionally, consider pinning a minimum version (e.g.,pytest>=7.0) for reproducibility.The
pydantic>=2.0constraint is appropriate for the v2 models introduced in this PR.server/api_server/validation.py (2)
50-53: Remove empty conditional block or implement Content-Type validation.This block checks if the request is not JSON but then does nothing (
pass). Either remove the dead code or add proper Content-Type validation that returns an error when mutation methods don't sendapplication/json.♻️ Proposed fix
- if request.method in ["POST", "PUT", "PATCH"]: - if not request.is_json and request.content_length: - # Allow empty body if optional? No, pydantic handles that. - pass + # For mutation methods, require JSON content-type if body is present + if request.method in ["POST", "PUT", "PATCH"]: + if request.content_length and not request.is_json: + return jsonify({ + "success": False, + "error": "Invalid Content-Type", + "message": "Content-Type must be application/json" + }), 415
11-13: Unused parameters reserved for future use.
summaryanddescriptionare currently unused (per static analysis). If these are intentionally reserved for Phase 1.2 OpenAPI registry sync (as mentioned in line 41), consider prefixing with underscore like the other unused params (_response_model,_tags, etc.) for consistency.♻️ Proposed fix for consistency
def validate_request( operation_id: str, - summary: str, - description: str, + _summary: str, + _description: str, request_model: Optional[Type[BaseModel]] = None,schema_validation.py (2)
122-125: Replace recursion with iteration for MAC collision handling.While statistically improbable, recursive retries could cause stack overflow. Use a loop instead.
♻️ Proposed fix using iteration
+def inject_test_device(): + max_attempts = 10 + for attempt in range(max_attempts): # Use locally administered MAC address range mac = "02:%02x:%02x:%02x:%02x:%02x" % ( random.randint(0, 255), random.randint(0, 255), random.randint(0, 255), random.randint(0, 255), random.randint(0, 255)) + + conn = None + cur = None + try: + conn = get_db_connection() + cur = conn.cursor() + cur.execute("SELECT devMAC FROM Devices WHERE devMAC = ?", (mac,)) + if cur.fetchone(): + continue # Try again with new MAC + # ... rest of insert logic + finally: + if cur: cur.close() + if conn: conn.close() + + raise RuntimeError(f"Failed to generate unique MAC after {max_attempts} attempts")
151-186: Module-level execution makes testing difficult.The initialization code (lines 152-186) runs at import time, which makes unit testing this module harder and can cause side effects during imports. Consider wrapping this in a
main()function with anif __name__ == "__main__":guard.♻️ Proposed structure
+def main(): + # Initialize + print("--- INITIALIZATION ---") + + parser = argparse.ArgumentParser(description='Rigorous Schema Validation') + parser.add_argument('--inject-test-device', action='store_true', help='Inject a test device if not present') + parser.add_argument('--cleanup', action='store_true', help='Remove test device after validation') + args = parser.parse_args() + + # ... rest of initialization and execution + +if __name__ == "__main__": + main()test/verify_runtime_validation.py (2)
21-31: Consider extracting shared wait_for_server utility.This function is nearly identical to the one in
test/test_wol_validation.py(shown in relevant snippets). Consider extracting to a shared test utilities module to reduce duplication.Also,
test_wol_validation.pyuses aSERVER_DELAYconstant for the sleep interval. For consistency, consider using the same pattern here.♻️ Suggested shared utility approach
# test/utils/server_helpers.py import os import time import requests def wait_for_server( base_url: str, retries: int = 5, delay: float = 1.0, endpoint: str = "/graphql" ) -> bool: """Wait for the backend to become ready with paced retries.""" for _ in range(retries): try: resp = requests.get(f"{base_url}{endpoint}", timeout=2) if 200 <= resp.status_code < 300: return True except requests.RequestException: pass time.sleep(delay) return False
38-48: Test assertions have redundant check.Line 47 asserts
status_code in (200, 404)and line 48 assertsstatus_code != 422. The second assertion is redundant since if the first passes, the status is already not 422.♻️ Simplified assertion
def test_search_valid(): """Valid payloads should return 200/404 but never 422.""" payload = {"query": "Router"} resp = requests.post( f"{BASE_URL}/devices/search", json=payload, headers=HEADERS, timeout=REQUEST_TIMEOUT, ) - assert resp.status_code in (200, 404), f"Unexpected status {resp.status_code}: {resp.text}" - assert resp.status_code != 422, f"Validation failed for valid payload: {resp.text}" + assert resp.status_code in (200, 404), ( + f"Expected 200/404 for valid payload, got {resp.status_code}: {resp.text}" + )test/api_endpoints/test_mcp_extended_endpoints.py (1)
23-29: Consider aligning fixture patterns with other test modules.The
auth_headershelper is implemented as a plain function requiring a token argument, while other test modules in the codebase use it as a pytest fixture (seetest_mcp_tools_endpoints.pyandtest_devices_endpoints.py). Additionally,api_tokenusessessionscope whileclientuses default (function) scope, which could cause test isolation issues if settings change between tests.♻️ Suggested alignment
@pytest.fixture(scope="session") def api_token(): return get_setting_value("API_TOKEN") -def auth_headers(token): - return {"Authorization": f"Bearer {token}"} +@pytest.fixture +def auth_headers(api_token): + return {"Authorization": f"Bearer {api_token}"}Then update test signatures to use
auth_headersdirectly as a fixture instead ofauth_headers(api_token).test/test_wol_validation.py (1)
17-27: Consider increasing the readiness probe timeout.The
wait_for_serverfunction uses a 1-second timeout for the readiness check (line 21), which may be too aggressive for slower CI environments or when the backend is under load. TheSERVER_DELAYis configurable but the internal probe timeout is hardcoded.♻️ Suggested fix
def wait_for_server() -> bool: """Wait for the GraphQL endpoint to become ready with paced retries.""" for _ in range(SERVER_RETRIES): try: - resp = requests.get(f"{BASE_URL}/graphql", timeout=1) + resp = requests.get(f"{BASE_URL}/graphql", timeout=REQUEST_TIMEOUT) if 200 <= resp.status_code < 300: return True except requests.RequestException: pass time.sleep(SERVER_DELAY) return Falsetest/test_mcp_disablement.py (1)
7-14: Avoid directly accessing private attributes in tests.The fixture accesses
spec_generator._disabled_toolsdirectly, which couples the test to internal implementation details. If the internal structure changes, these tests will break even if the public API remains stable.Consider using only public APIs (
clear_registry()already clears_disabled_toolsinternally per the relevant code snippet at lines 56-66 of spec_generator.py).♻️ Simplified fixture using only public API
@pytest.fixture(autouse=True) def reset_registry(): spec_generator.clear_registry() - spec_generator._disabled_tools.clear() yield spec_generator.clear_registry() - spec_generator._disabled_tools.clear()test/api_endpoints/test_mcp_openapi_spec.py (2)
31-32: Movesys.pathextension before imports that depend on it.The
sys.path.extend()call is placed after the imports fromapi_servermodules. If those modules depend on paths being available, this could cause import failures in certain environments. While it currently works (likely because the modules are already found), it's safer to extend the path before dependent imports.♻️ Suggested reordering
import sys import os import pytest + +INSTALL_PATH = os.getenv('NETALERTX_APP', '/app') +sys.path.extend([f"{INSTALL_PATH}/front/plugins", f"{INSTALL_PATH}/server"]) from pydantic import ValidationError from api_server.schemas import ( # ... imports ) # ... other imports -INSTALL_PATH = os.getenv('NETALERTX_APP', '/app') -sys.path.extend([f"{INSTALL_PATH}/front/plugins", f"{INSTALL_PATH}/server"])
118-121: Address static analysis hint for class attribute annotation.Per Ruff RUF012, mutable class attributes should be annotated with
typing.ClassVar. This is a minor typing improvement.♻️ Suggested fix
+from typing import ClassVar, Set + class TestOpenAPISpecGenerator: """Test the OpenAPI spec generator.""" - HTTP_METHODS = {"get", "post", "put", "patch", "delete", "options", "head", "trace"} + HTTP_METHODS: ClassVar[Set[str]] = {"get", "post", "put", "patch", "delete", "options", "head", "trace"}server/api_server/api_server_start.py (1)
5-6: Remove unusednoqadirective.Per Ruff RUF100, the
# flake8: noqa: E402directive is not suppressing any actual E402 violations on this line. If you intend to suppress E402 for the entire file, use a file-level comment at line 1, or remove it if not needed.♻️ Suggested fix
-# flake8: noqa: E402 -Or if needed for the entire file, move to line 1:
# flake8: noqa: E402 import threading ...server/api_server/mcp_endpoint.py (3)
682-682: Use environment variable for log directory path.The hardcoded
/tmp/logfallback is flagged by Ruff S108 as potentially insecure. While the code does useNETALERTX_LOGenvironment variable, per coding guidelines, prefer not hardcoding paths. The/tmpdirectory can be world-writable and shared.Consider using a more secure default or requiring the environment variable:
♻️ Suggested improvement
def _list_resources() -> List[Dict[str, Any]]: """List available MCP resources (read-only data like logs).""" resources = [] # Log files - log_dir = os.getenv("NETALERTX_LOG", "/tmp/log") + log_dir = os.getenv("NETALERTX_LOG") + if not log_dir: + mylog("verbose", ["[MCP] NETALERTX_LOG not set, skipping resource listing"]) + return resourcesApply similar change to
_read_resource.Also applies to: 717-717
290-291: Rename unused loop variables per static analysis.The
pathandmethodvariables inmap_openapi_to_mcp_toolsare not used within the loop body.♻️ Suggested fix
- for path, methods in spec["paths"].items(): - for method, details in methods.items(): + for _path, methods in spec["paths"].items(): + for _method, details in methods.items():
665-670: Consider narrowing the exception handler.The broad
except Exceptioncatch (Ruff BLE001) could mask unexpected errors. Consider catching more specific exceptions or at minimum re-raising after logging for debugging.♻️ Suggested improvement
except requests.Timeout: return { "content": [{"type": "text", "text": "Request timed out"}], "isError": True } - except Exception as e: + except requests.RequestException as e: mylog("none", [f"[MCP] Error executing tool {route['operation_id']}: {e}"]) return { - "content": [{"type": "text", "text": f"Error: {str(e)}"}], + "content": [{"type": "text", "text": f"Error: {e!s}"}], "isError": True }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
requirements.txtschema_validation.pyserver/api_server/api_server_start.pyserver/api_server/mcp_endpoint.pyserver/api_server/schemas.pyserver/api_server/spec_generator.pyserver/api_server/validation.pytest/api_endpoints/test_devices_endpoints.pytest/api_endpoints/test_mcp_extended_endpoints.pytest/api_endpoints/test_mcp_openapi_spec.pytest/api_endpoints/test_mcp_tools_endpoints.pytest/test_mcp_disablement.pytest/test_wol_validation.pytest/verify_runtime_validation.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Always include unit tests or validation steps immediately after writing functions; never leave code unvalidated
Always set explicit timeouts for subprocess calls; default to 60 seconds minimum unless plugin config specifies otherwise
Usenormalize_mac()fromfront/plugins/plugin_helper.pyto normalize MAC addresses before database writes
Usemylog(level, [message])for logging with levels: none/minimal/verbose/debug/trace; usenonefor critical messages that must always appear
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) for all file paths; never hardcode paths like/data/dbor use relative paths
Sanity-check and normalize all text input using sanitizer functions fromserver/helper.pybefore database writes
Files:
test/api_endpoints/test_mcp_openapi_spec.pytest/api_endpoints/test_mcp_tools_endpoints.pytest/test_wol_validation.pyserver/api_server/validation.pyschema_validation.pytest/api_endpoints/test_devices_endpoints.pytest/api_endpoints/test_mcp_extended_endpoints.pytest/verify_runtime_validation.pytest/test_mcp_disablement.pyserver/api_server/api_server_start.pyserver/api_server/mcp_endpoint.pyserver/api_server/schemas.pyserver/api_server/spec_generator.py
**/*.{py,js,php}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never use placeholders (e.g.,
// ... rest of code) in code implementations; output full, functional blocks to preserve context
Files:
test/api_endpoints/test_mcp_openapi_spec.pytest/api_endpoints/test_mcp_tools_endpoints.pytest/test_wol_validation.pyserver/api_server/validation.pyschema_validation.pytest/api_endpoints/test_devices_endpoints.pytest/api_endpoints/test_mcp_extended_endpoints.pytest/verify_runtime_validation.pytest/test_mcp_disablement.pyserver/api_server/api_server_start.pyserver/api_server/mcp_endpoint.pyserver/api_server/schemas.pyserver/api_server/spec_generator.py
server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/**/*.py: Never hardcode ports or secrets; useget_setting_value()to retrieve all configuration values from settings
Preferserver/db/db_helper.pyfunctions (e.g.,get_table_json, device condition helpers) over raw SQL in new code paths
Use datetime utilities fromserver/utils/datetime_utils.py(e.g.,timeNowDB()) for all timestamp operations
Files:
server/api_server/validation.pyserver/api_server/api_server_start.pyserver/api_server/mcp_endpoint.pyserver/api_server/schemas.pyserver/api_server/spec_generator.py
server/api_server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/api_server/**/*.py: All API responses must include"success": <true|false>and if false, include an"error"message field
All API routes must requireAuthorization: Bearer <API_TOKEN>header validation viaget_setting_value('API_TOKEN')
Files:
server/api_server/validation.pyserver/api_server/api_server_start.pyserver/api_server/mcp_endpoint.pyserver/api_server/schemas.pyserver/api_server/spec_generator.py
🧠 Learnings (2)
📚 Learning: 2026-01-10T04:32:10.552Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.552Z
Learning: Applies to **/*.py : Use environment variables (`NETALERTX_DB`, `NETALERTX_LOG`, etc.) for all file paths; never hardcode paths like `/data/db` or use relative paths
Applied to files:
test/api_endpoints/test_mcp_tools_endpoints.pytest/api_endpoints/test_devices_endpoints.pyserver/api_server/api_server_start.py
📚 Learning: 2025-11-05T04:34:52.339Z
Learnt from: jokob-sk
Repo: jokob-sk/NetAlertX PR: 1271
File: server/helper.py:50-71
Timestamp: 2025-11-05T04:34:52.339Z
Learning: In the NetAlertX codebase, `timeNowDB` function is intentionally duplicated in both `server/helper.py` and `server/logger.py` to prevent circular import issues, since `helper.py` imports from `logger.py` (`from logger import mylog, logResult`). This duplication is by design.
Applied to files:
server/api_server/api_server_start.py
🧬 Code graph analysis (10)
test/api_endpoints/test_mcp_openapi_spec.py (3)
server/api_server/schemas.py (7)
DeviceSearchRequest(101-117)DeviceSearchResponse(135-137)WakeOnLanRequest(293-315)TracerouteRequest(323-334)TriggerScanRequest(264-270)OpenPortsRequest(278-284)SetDeviceAliasRequest(172-179)server/api_server/spec_generator.py (6)
generate_openapi_spec(345-446)get_registry(163-166)register_tool(100-152)clear_registry(155-160)DuplicateOperationIdError(44-46)pydantic_to_json_schema(169-189)server/api_server/mcp_endpoint.py (1)
map_openapi_to_mcp_tools(272-354)
test/api_endpoints/test_mcp_tools_endpoints.py (2)
server/helper.py (1)
get_setting_value(200-257)server/db/db_helper.py (1)
get(245-256)
test/test_wol_validation.py (4)
test/verify_runtime_validation.py (1)
wait_for_server(21-31)test/api_endpoints/test_devices_endpoints.py (1)
auth_headers(30-31)test/api_endpoints/test_mcp_extended_endpoints.py (1)
auth_headers(28-29)test/api_endpoints/test_mcp_tools_endpoints.py (1)
auth_headers(20-21)
server/api_server/validation.py (1)
server/logger.py (1)
mylog(79-84)
schema_validation.py (1)
server/api_server/schemas.py (15)
DeviceSearchResponse(135-137)DeviceListResponse(148-150)GetDeviceResponse(167-169)DeviceInfo(120-132)DeviceTotalsResponse(182-184)DeviceExportResponse(195-198)NetworkTopologyResponse(372-375)BaseResponse(80-86)OpenPortsResponse(287-290)WakeOnLanResponse(318-320)TracerouteResponse(337-339)RecentEventsResponse(411-414)LastEventsResponse(417-419)DbQueryResponse(574-577)GetSettingResponse(617-619)
test/api_endpoints/test_devices_endpoints.py (1)
server/helper.py (1)
get_setting_value(200-257)
test/api_endpoints/test_mcp_extended_endpoints.py (5)
server/helper.py (1)
get_setting_value(200-257)test/api_endpoints/test_devices_endpoints.py (3)
client(19-21)api_token(14-15)auth_headers(30-31)test/api_endpoints/test_mcp_tools_endpoints.py (3)
client(15-17)api_token(10-11)auth_headers(20-21)test/test_wol_validation.py (1)
auth_headers(38-42)server/models/user_events_queue_instance.py (1)
add_event(81-116)
test/verify_runtime_validation.py (1)
test/test_wol_validation.py (1)
wait_for_server(17-27)
test/test_mcp_disablement.py (2)
server/api_server/spec_generator.py (6)
clear_registry(155-160)register_tool(100-152)is_tool_disabled(71-74)set_tool_disabled(49-68)get_tools_status(83-97)generate_openapi_spec(345-446)server/api_server/mcp_endpoint.py (2)
find_route_for_tool(357-373)_execute_tool(566-670)
server/api_server/api_server_start.py (3)
server/api_server/mcp_endpoint.py (1)
openapi_spec(244-265)server/api_server/validation.py (1)
validate_request(10-107)server/api_server/schemas.py (6)
DeviceSearchRequest(101-117)DeviceListRequest(140-145)WakeOnLanRequest(293-315)TriggerScanRequest(264-270)TracerouteRequest(323-334)RecentEventsRequest(395-408)
🪛 Ruff (0.14.10)
test/api_endpoints/test_mcp_openapi_spec.py
121-121: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
171-171: Loop control variable path not used within loop body
Rename unused path to _path
(B007)
server/api_server/validation.py
12-12: Unused function argument: summary
(ARG001)
13-13: Unused function argument: description
(ARG001)
72-72: Do not catch blind exception: Exception
(BLE001)
100-100: Use raise without specifying exception name
Remove exception name
(TRY201)
schema_validation.py
41-41: Avoid specifying long messages outside the exception class
(TRY003)
60-60: Do not catch blind exception: Exception
(BLE001)
80-80: Consider moving this statement to an else block
(TRY300)
81-81: Do not catch blind exception: Exception
(BLE001)
99-99: Do not catch blind exception: Exception
(BLE001)
111-111: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
111-111: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
111-111: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
112-112: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
112-112: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
139-139: Consider moving this statement to an else block
(TRY300)
141-141: Do not catch blind exception: Exception
(BLE001)
217-217: Consider moving this statement to an else block
(TRY300)
227-227: Do not catch blind exception: Exception
(BLE001)
server/api_server/api_server_start.py
5-5: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
server/api_server/mcp_endpoint.py
96-96: Do not catch blind exception: Exception
(BLE001)
232-232: Do not catch blind exception: Exception
(BLE001)
290-290: Loop control variable path not used within loop body
Rename unused path to _path
(B007)
291-291: Loop control variable method not used within loop body
Rename unused method to _method
(B007)
655-658: Consider moving this statement to an else block
(TRY300)
665-665: Do not catch blind exception: Exception
(BLE001)
668-668: Use explicit conversion flag
Replace with conversion flag
(RUF010)
682-682: Probable insecure usage of temporary file or directory: "/tmp/log"
(S108)
717-717: Probable insecure usage of temporary file or directory: "/tmp/log"
(S108)
735-735: Do not catch blind exception: Exception
(BLE001)
878-878: Do not catch blind exception: Exception
(BLE001)
server/api_server/schemas.py
57-57: Avoid specifying long messages outside the exception class
(TRY003)
64-64: Avoid specifying long messages outside the exception class
(TRY003)
71-71: Avoid specifying long messages outside the exception class
(TRY003)
255-255: Avoid specifying long messages outside the exception class
(TRY003)
424-424: Possible binding to all interfaces
(S104)
533-533: Avoid specifying long messages outside the exception class
(TRY003)
558-558: Avoid specifying long messages outside the exception class
(TRY003)
server/api_server/spec_generator.py
134-137: Avoid specifying long messages outside the exception class
(TRY003)
1265-1265: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (25)
server/api_server/schemas.py (2)
516-534: Excellent security pattern for dangerous operations.The
DbQueryRequestmodel withconfirm_dangerous_querysafety flag and model validator is a good security practice for privileged operations. This pattern ensures explicit acknowledgment of risk.
247-256: Good safety guard for bulk delete operations.The
DeleteDevicesRequestwithconfirm_delete_allflag and model validator prevents accidental deletion of all devices. This is a solid defensive pattern.test/api_endpoints/test_devices_endpoints.py (2)
9-10: Simplified imports look good.The direct imports from
helperandapi_server.api_server_startare cleaner than the previous dynamic path manipulation approach. This aligns with the broader refactoring in the PR.
117-148: Comprehensive export/import cycle test.Good test coverage for the base64 CSV import/export workflow. The test validates the full round-trip which is valuable for catching integration issues.
test/api_endpoints/test_mcp_tools_endpoints.py (4)
5-6: Import simplification consistent with other test files.The direct imports align with the pattern used in
test_devices_endpoints.pyand simplify the test setup.
168-171: Defensive access pattern for optional fields.Using
data.get("links", [])is safer than direct dictionary access and handles cases where the key might be missing.
258-260: Status code change reflects new validation layer.The change from 400 to 422 for invalid MAC addresses aligns with the new
validate_requestdecorator behavior, which returns 422 for PydanticValidationError.
432-436: Good test update for validation short-circuit.The test now correctly expects:
- HTTP 422 instead of 400 (validation error)
mock_traceroute.assert_not_called()- confirming the validation rejects before reaching the handlerThis properly tests the early validation rejection behavior.
test/verify_runtime_validation.py (1)
10-18: Good use of environment variables for configuration.Using
NETALERTX_BASE_URL,NETALERTX_REQUEST_TIMEOUT, andNETALERTX_SERVER_RETRIESenvironment variables with sensible defaults follows the coding guidelines for configuration management.test/api_endpoints/test_mcp_extended_endpoints.py (1)
1-494: Good comprehensive test coverage for MCP extended endpoints.The test suite covers a wide range of endpoints across devices, events, sessions, messaging, nettools, logs, sync, and dbquery domains. The mocking approach is appropriate, and the tests validate both response status codes and mock call arguments.
Consider adding negative test cases for error paths (e.g., 401/403 unauthorized, 422 validation errors) in a follow-up to improve coverage.
test/test_wol_validation.py (1)
45-78: LGTM - Good validation coverage for WOL endpoint.The tests properly validate:
- Valid MAC addresses are accepted (non-422 response)
- Valid IP addresses are accepted (non-422 response)
- Invalid MAC addresses are rejected with 422
The use of configurable timeouts via
REQUEST_TIMEOUTaligns with the coding guidelines for explicit timeout handling.test/test_mcp_disablement.py (1)
110-146: Good test coverage for disabled tool execution behavior.The test properly validates that:
- Enabled tools execute successfully via mocked HTTP
- Disabled tools return an error without making HTTP requests
- The
mock_get.call_count == 1assertion confirms the second call was short-circuitedtest/api_endpoints/test_mcp_openapi_spec.py (2)
65-88: Clarify or enable commented-out validation tests.Lines 72-73 and 87-88 have commented-out tests for invalid MAC and IP validation. If these validations are intentionally not enforced by
WakeOnLanRequestandTracerouteRequest, consider adding a brief comment explaining why. Otherwise, these may represent gaps in validation coverage.Are the MAC and IP validators intentionally permissive, or should these validation tests be enabled?
255-287: Good registry restoration pattern in test.The
try/finallyblock properly restores the registry state after the duplicate operationId test, ensuring test isolation. The call to_register_all_endpoints()re-populates the registry for subsequent tests.server/api_server/api_server_start.py (2)
433-451: LGTM - Good validation decorator integration with fallback.The
api_devices_by_statusendpoint correctly:
- Uses
@validate_requestfor Pydantic validation- Accepts optional
payloadparameter- Falls back to
request.argswhen payload is not provided- Maintains backward compatibility with GET requests
This pattern allows both validated POST requests and legacy GET query parameters.
914-927: Good defensive handling of hours parameter.The
api_events_default_24hfunction properly:
- Uses validated payload when available
- Falls back to query args with try/except for type conversion
- Defaults to 24 hours on any parsing error
This defensive approach prevents 500 errors from malformed query parameters.
server/api_server/mcp_endpoint.py (4)
719-726: Good path traversal protection implemented.The
_read_resourcefunction properly validates that the resolved file path stays within the log directory usingos.path.realpath()comparison. This prevents path traversal attacks via../sequences in the URI.
626-637: Explicit 60-second timeouts on all HTTP methods - good practice.All loopback HTTP calls (POST, PUT, PATCH, DELETE, GET) include explicit
timeout=60as required by the coding guidelines. This prevents indefinite blocking.
854-926: Solid SSE implementation with session management.The
mcp_ssefunction properly:
- Validates authorization before processing
- Handles both POST (stateless JSON-RPC) and GET (SSE stream) methods
- Creates sessions with unique IDs and cleanup on disconnect
- Includes keep-alive comments to prevent connection timeout
- Uses
stream_with_contextfor proper Flask context handling- Sets appropriate headers including
X-Accel-Buffering: nofor nginx
957-962: Good backpressure handling for bounded queue.The
mcp_messagesfunction properly handlesqueue.Fullexception with a 5-second timeout, returning HTTP 503 when the queue is full. This prevents memory exhaustion from slow consumers.server/api_server/spec_generator.py (5)
38-46: Well-designed thread-safe registry with duplicate detection.The module-level registry with threading lock, operation ID tracking, and disabled tools set provides a solid foundation. The
DuplicateOperationIdErrorensures operationId uniqueness at registration time, preventing silent overwrites.
169-189: Correct Pydantic v2 schema generation.The
pydantic_to_json_schemafunction correctly usesmodel_json_schema(mode="serialization")for Pydantic v2, which produces OpenAPI 3.1 compatible JSON Schema. The cleanup of empty$defsis a nice touch for cleaner output.
314-331: Good token optimization for LLM context.The minimal error responses (description only, no schema/content) is an intentional design choice to reduce OpenAPI spec size for LLM context windows. The comment on lines 314-316 clearly documents this rationale.
335-342: Placeholder function is acceptable for experimental feature.The
_flatten_defsfunction is documented as experimental and explicitly passes. This is acceptable for a first implementation, with the understanding that it may be enhanced later.
1262-1267: Resilient module initialization with graceful degradation.The try/except block around
_register_all_endpoints()allows the module to import successfully even if endpoint registration fails (e.g., due to missing dependencies). The warning is printed to stderr, which is appropriate for initialization-time issues.This pattern enables testing scenarios where schemas may not be available.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/api_server/api_server_start.py (2)
5-6: Remove unusednoqadirective.Static analysis indicates that
E402is not enabled in the linter configuration, making this directive unnecessary.Suggested fix
-# flake8: noqa: E402 -
1170-1187: Variableis_authorizedshadows the function name.The local variable
is_authorizedon line 1180 shadows the function name, which could cause confusion and potential issues if the function needs to be referenced later in this scope.Suggested fix
def is_authorized(): expected_token = get_setting_value('API_TOKEN') # Check Authorization header first (primary method) auth_header = request.headers.get("Authorization", "") header_token = auth_header.split()[-1] if auth_header.startswith("Bearer ") else "" # Also check query string token (for SSE and other streaming endpoints) query_token = request.args.get("token", "") - is_authorized = (header_token == expected_token) or (query_token == expected_token) + authorized = (header_token == expected_token) or (query_token == expected_token) - if not is_authorized: + if not authorized: msg = "[api] Unauthorized access attempt - make sure your GRAPHQL_PORT and API_TOKEN settings are correct." write_notification(msg, "alert") mylog("verbose", [msg]) - return is_authorized + return authorized
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/api_server/api_server_start.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Always include unit tests or validation steps immediately after writing functions; never leave code unvalidated
Always set explicit timeouts for subprocess calls; default to 60 seconds minimum unless plugin config specifies otherwise
Usenormalize_mac()fromfront/plugins/plugin_helper.pyto normalize MAC addresses before database writes
Usemylog(level, [message])for logging with levels: none/minimal/verbose/debug/trace; usenonefor critical messages that must always appear
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) for all file paths; never hardcode paths like/data/dbor use relative paths
Sanity-check and normalize all text input using sanitizer functions fromserver/helper.pybefore database writes
Files:
server/api_server/api_server_start.py
**/*.{py,js,php}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never use placeholders (e.g.,
// ... rest of code) in code implementations; output full, functional blocks to preserve context
Files:
server/api_server/api_server_start.py
server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/**/*.py: Never hardcode ports or secrets; useget_setting_value()to retrieve all configuration values from settings
Preferserver/db/db_helper.pyfunctions (e.g.,get_table_json, device condition helpers) over raw SQL in new code paths
Use datetime utilities fromserver/utils/datetime_utils.py(e.g.,timeNowDB()) for all timestamp operations
Files:
server/api_server/api_server_start.py
server/api_server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/api_server/**/*.py: All API responses must include"success": <true|false>and if false, include an"error"message field
All API routes must requireAuthorization: Bearer <API_TOKEN>header validation viaget_setting_value('API_TOKEN')
Files:
server/api_server/api_server_start.py
🧠 Learnings (2)
📚 Learning: 2026-01-10T04:32:10.552Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.552Z
Learning: Applies to **/*.py : Use environment variables (`NETALERTX_DB`, `NETALERTX_LOG`, etc.) for all file paths; never hardcode paths like `/data/db` or use relative paths
Applied to files:
server/api_server/api_server_start.py
📚 Learning: 2025-11-05T04:34:52.339Z
Learnt from: jokob-sk
Repo: jokob-sk/NetAlertX PR: 1271
File: server/helper.py:50-71
Timestamp: 2025-11-05T04:34:52.339Z
Learning: In the NetAlertX codebase, `timeNowDB` function is intentionally duplicated in both `server/helper.py` and `server/logger.py` to prevent circular import issues, since `helper.py` imports from `logger.py` (`from logger import mylog, logResult`). This duplication is by design.
Applied to files:
server/api_server/api_server_start.py
🧬 Code graph analysis (1)
server/api_server/api_server_start.py (5)
server/api_server/mcp_endpoint.py (5)
delete_session(172-179)create_session(117-147)mcp_sse(854-926)mcp_messages(929-964)openapi_spec(244-265)server/api_server/logs_endpoint.py (1)
clean_log(18-57)server/api_server/validation.py (1)
validate_request(10-107)server/api_server/schemas.py (5)
DeviceSearchRequest(101-117)DeviceListRequest(140-145)WakeOnLanRequest(293-315)TracerouteRequest(323-334)RecentEventsRequest(395-408)server/models/device_instance.py (2)
DeviceInstance(16-703)getByStatus(342-373)
🪛 Ruff (0.14.10)
server/api_server/api_server_start.py
5-5: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
🔇 Additional comments (9)
server/api_server/api_server_start.py (9)
58-65: LGTM!The new imports for MCP endpoints, validation decorator, and Pydantic schemas are properly organized and all appear to be used in the file.
94-94: LGTM!Adding CORS support for
/openapi.jsonis appropriate for an OpenAPI spec endpoint that external tools may need to access.
111-122: LGTM!The MCP SSE and messages endpoints properly check authorization before delegating to the handler functions, and the error responses follow the required format with
success: falseanderrorfields.
437-456: LGTM!The
@validate_requestdecorator is properly configured, and the fallback logic correctly preserves backward compatibility for clients using query parameters while supporting the new payload-based approach.
459-496: LGTM!The search endpoint correctly integrates the validation decorator while maintaining backward compatibility. The error responses properly include
success,message, anderrorfields as required by the coding guidelines.
548-582: LGTM!The Wake-on-LAN endpoint properly integrates validation while preserving backward compatibility. The MAC resolution from IP address and proper error handling enhance usability.
662-691: LGTM!The trigger scan endpoint properly validates the scan type against loaded plugins, preventing arbitrary plugin execution. The fallback to 'ARPSCAN' for GET requests maintains backward compatibility.
697-701: LGTM!The OpenAPI spec endpoint properly requires authorization and cleanly delegates to the handler function.
911-932: LGTM!The recent events endpoint correctly handles payload validation with robust fallback logic for query parameters, including proper error handling for invalid integer conversion.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @server/api_server/api_server_start.py:
- Around line 65-73: The multi-line import of DeviceSearchRequest,
DeviceListRequest, WakeOnLanRequest, TriggerScanRequest, TracerouteRequest, and
RecentEventsRequest is broken because the "# noqa: E402" comment is inserted
between "from .schemas import" and the opening parenthesis; move the noqa
comment to the end of the import statement (after the closing parenthesis) or
place it on the same line as the import keyword so the import reads as a valid
Python multi-line import and the flake8 suppression still applies.
- Around line 604-610: The code can raise AttributeError when request.json is
None; change the devLastIP extraction to safely handle missing/invalid JSON by
using a safe JSON read (e.g., request.get_json(silent=True)) or falling back to
an empty dict before calling .get so that when payload is falsy you do something
like: if payload: ip = payload.devLastIP else: json_body =
request.get_json(silent=True) or {} ; ip = json_body.get("devLastIP"). Update
the logic around payload and request.json in the same block where payload is
checked (the devLastIP extraction in api_server_start.py) to match the safer
pattern used in api_trigger_scan.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/api_server/api_server_start.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Always include unit tests or validation steps immediately after writing functions; never leave code unvalidated
Always set explicit timeouts for subprocess calls; default to 60 seconds minimum unless plugin config specifies otherwise
Usenormalize_mac()fromfront/plugins/plugin_helper.pyto normalize MAC addresses before database writes
Usemylog(level, [message])for logging with levels: none/minimal/verbose/debug/trace; usenonefor critical messages that must always appear
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) for all file paths; never hardcode paths like/data/dbor use relative paths
Sanity-check and normalize all text input using sanitizer functions fromserver/helper.pybefore database writes
Files:
server/api_server/api_server_start.py
**/*.{py,js,php}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never use placeholders (e.g.,
// ... rest of code) in code implementations; output full, functional blocks to preserve context
Files:
server/api_server/api_server_start.py
server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/**/*.py: Never hardcode ports or secrets; useget_setting_value()to retrieve all configuration values from settings
Preferserver/db/db_helper.pyfunctions (e.g.,get_table_json, device condition helpers) over raw SQL in new code paths
Use datetime utilities fromserver/utils/datetime_utils.py(e.g.,timeNowDB()) for all timestamp operations
Files:
server/api_server/api_server_start.py
server/api_server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/api_server/**/*.py: All API responses must include"success": <true|false>and if false, include an"error"message field
All API routes must requireAuthorization: Bearer <API_TOKEN>header validation viaget_setting_value('API_TOKEN')
Files:
server/api_server/api_server_start.py
🪛 GitHub Actions: Code checks
server/api_server/api_server_start.py
[error] 65-65: SyntaxError: invalid syntax during Python syntax check (py_compile) at line 65.
🪛 Ruff (0.14.10)
server/api_server/api_server_start.py
65-66: Expected one or more symbol names after import
(invalid-syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
🔇 Additional comments (5)
server/api_server/api_server_start.py (5)
446-464: LGTM!The decorator integration and backward-compatible fallback to query parameters are well implemented.
469-504: LGTM!Safe fallback pattern with
get_json(silent=True)ensures robustness when payload is absent.
558-590: LGTM!The fallback using
request.json or {}safely handles missing JSON bodies while preserving MAC resolution logic.
672-699: LGTM!Safe JSON parsing pattern, proper validation against loaded plugins, and correct queue usage.
921-940: LGTM!Robust handling of the
hoursparameter with proper try/except and sensible default fallback.
adamoutler
left a comment
adamoutler
left a comment
There was a problem hiding this comment.
review complete.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
server/api_server/api_server_start.py (3)
5-6: Remove unusednoqadirective.Static analysis indicates E402 is not enabled in this project's linter configuration, making this directive unnecessary.
Suggested fix
-# flake8: noqa: E402 -
62-72: Clean up unusednoqadirectives.Per static analysis, E402 is not enabled, so these directives have no effect.
Suggested fix
-from .mcp_endpoint import ( - mcp_sse, - mcp_messages, - openapi_spec -) # noqa: E402 [flake8 lint suppression] -# validation and schemas for MCP v2 -from .validation import validate_request # noqa: E402 [flake8 lint suppression] -from .schemas import ( # noqa: E402 [flake8 lint suppression] +from .mcp_endpoint import ( + mcp_sse, + mcp_messages, + openapi_spec +) +# validation and schemas for MCP v2 +from .validation import validate_request +from .schemas import ( DeviceSearchRequest, DeviceListRequest, WakeOnLanRequest, TriggerScanRequest, TracerouteRequest, RecentEventsRequest )
603-610: Misleading comment and defensive gap.The comment on line 609 states "Validation logic below handles None" but the next line simply calls
traceroute(ip)without any null check. While the@validate_requestdecorator should ensurepayloadis set for valid POST requests (making the else branch unreachable), the comment is inaccurate.Consider either:
- Adding an explicit null check before calling
traceroute(ip), or- Removing the misleading comment if the else branch is purely defensive.
Suggested fix (option 1 - add null check)
if payload: ip = payload.devLastIP else: ip = request.json.get("devLastIP") - # Validation logic below handles None (though Pydantic prevents None if required) + if not ip: + return jsonify({"success": False, "error": "Missing 'devLastIP' parameter"}), 400 + return traceroute(ip)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/api_server/api_server_start.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Always include unit tests or validation steps immediately after writing functions; never leave code unvalidated
Always set explicit timeouts for subprocess calls; default to 60 seconds minimum unless plugin config specifies otherwise
Usenormalize_mac()fromfront/plugins/plugin_helper.pyto normalize MAC addresses before database writes
Usemylog(level, [message])for logging with levels: none/minimal/verbose/debug/trace; usenonefor critical messages that must always appear
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) for all file paths; never hardcode paths like/data/dbor use relative paths
Sanity-check and normalize all text input using sanitizer functions fromserver/helper.pybefore database writes
Files:
server/api_server/api_server_start.py
**/*.{py,js,php}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never use placeholders (e.g.,
// ... rest of code) in code implementations; output full, functional blocks to preserve context
Files:
server/api_server/api_server_start.py
server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/**/*.py: Never hardcode ports or secrets; useget_setting_value()to retrieve all configuration values from settings
Preferserver/db/db_helper.pyfunctions (e.g.,get_table_json, device condition helpers) over raw SQL in new code paths
Use datetime utilities fromserver/utils/datetime_utils.py(e.g.,timeNowDB()) for all timestamp operations
Files:
server/api_server/api_server_start.py
server/api_server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/api_server/**/*.py: All API responses must include"success": <true|false>and if false, include an"error"message field
All API routes must requireAuthorization: Bearer <API_TOKEN>header validation viaget_setting_value('API_TOKEN')
Files:
server/api_server/api_server_start.py
🧬 Code graph analysis (1)
server/api_server/api_server_start.py (3)
server/api_server/mcp_endpoint.py (3)
mcp_sse(854-926)mcp_messages(929-964)openapi_spec(244-265)server/api_server/validation.py (1)
validate_request(10-107)server/api_server/schemas.py (6)
DeviceSearchRequest(101-117)DeviceListRequest(140-145)WakeOnLanRequest(293-315)TriggerScanRequest(264-270)TracerouteRequest(323-334)RecentEventsRequest(395-408)
🪛 Ruff (0.14.10)
server/api_server/api_server_start.py
5-5: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
62-62: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
64-64: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
65-65: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
🔇 Additional comments (8)
server/api_server/api_server_start.py (8)
101-101: LGTM!CORS configuration for the new OpenAPI spec endpoint follows the existing pattern and is necessary for MCP client access.
444-463: LGTM!The dual-path approach correctly preserves legacy query parameter behavior while enabling validated Pydantic payloads for MCP requests. Authorization check is in place.
466-503: LGTM!Validation integration is well-designed. The fallback path correctly handles legacy JSON body requests, and the is_mac routing logic is preserved. Response format follows the
success/errorguidelines.
555-589: LGTM!The implementation correctly handles both validated MCP payloads and legacy JSON requests. MAC validation at the schema level via
@field_validatoris a good practice, and the IP-to-MAC resolution fallback is preserved.
669-698: LGTM!The mixed GET/POST handling is correctly implemented. GET requests default to 'ARPSCAN' via the fallback path, while POST requests benefit from Pydantic validation. The secondary validation against
LOADED_PLUGINSensures only valid scan types are accepted.
704-708: LGTM!Clean wrapper that adds authorization before delegating to the OpenAPI spec generator. Follows the established pattern for MCP endpoints.
861-869: LGTM!The updated docstring clarifies the route disambiguation logic between MAC-based and days-based event deletion.
918-939: LGTM!The validation integration correctly handles multiple input sources: validated payloads take precedence, then query args with graceful error handling, then the default value. The delegation to
api_events_recent()maintains code reuse.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @schema_validation.py:
- Around line 33-46: The DB_PATH fallback logic is duplicated: remove the second
identical try/except block so there is only one fallback that attempts to set
DB_PATH from fullDbPath; keep the initial getenv checks (DB_PATH =
os.getenv("NETALERTX_DB") or os.getenv("NETALERTX_DB_PATH")) and the single
try/except that catches ImportError when referencing fullDbPath, and ensure any
necessary imports for fullDbPath are present so the code does not rely on a
duplicated block.
- Around line 162-198: The module does heavy side-effectful initialization
(argparse, get_setting calls, BASE_URL/HEADERS setup,
find_test_device/inject_test_device logic and sys.exit calls) at import time;
wrap that logic inside an if __name__ == "__main__": guard (or move into a
main() function called from that guard) so imports are side-effect free.
Specifically, relocate argument parsing (parser/args), TOKEN/PORT retrieval,
BASE_URL/HEADERS/REQUEST_TIMEOUT initialization, the MAC/IP lookup via
find_test_device(), injection via inject_test_device(), MUST_CLEANUP assignment
and the sys.exit/print flows into main(), leaving only pure function/class
definitions at module level so tests can import functions like find_test_device,
inject_test_device and validate without triggering execution.
In @server/api_server/api_server_start.py:
- Around line 444-454: The authorization check currently runs after the
validate_request decorator so unauthenticated calls get validation errors;
change the flow so is_authorized() runs before validation: remove or bypass the
@validate_request decorator on api_devices_by_status and perform an immediate
is_authorized() call at the top of api_devices_by_status (returning 403 on
failure), then invoke payload validation (e.g., via a helper like
validate_and_parse(DeviceListRequest, request) or by calling into the
validate_request logic) and proceed with the rest of the handler; alternatively,
modify the validate_request decorator to accept and run an auth callable
(is_authorized) before performing payload validation.
In @server/api_server/schemas.py:
- Around line 62-71: The validate_ip function currently catches
ipaddress.ip_address ValueError and re-raises a new ValueError without exception
chaining; update the except block in validate_ip to capture the original
exception (e.g., except ValueError as err) and re-raise the new ValueError using
"from err" so the original stack trace is preserved when the error is
propagated.
🧹 Nitpick comments (4)
schema_validation.py (2)
119-136: Unbounded recursion on MAC collision.While statistically unlikely, the recursive retry on MAC collision has no depth limit. Consider an iterative approach with a max retry count.
♻️ Proposed fix
def inject_test_device(): + MAX_RETRIES = 5 + for attempt in range(MAX_RETRIES): # Use locally administered MAC address range mac = "02:%02x:%02x:%02x:%02x:%02x" % ( random.randint(0, 255), random.randint(0, 255), random.randint(0, 255), random.randint(0, 255), random.randint(0, 255)) ip = "127.0.0.1" now = datetime.now().strftime("%Y-%m-%d %H:%M:%S") conn = None cur = None try: conn = get_db_connection() cur = conn.cursor() # Check if MAC exists (statistically impossible but good practice) cur.execute("SELECT devMAC FROM Devices WHERE devMAC = ?", (mac,)) if cur.fetchone(): - return inject_test_device() # Try again + continue # Try again with new MAC # ... rest of insertion logic + raise RuntimeError(f"Failed to generate unique MAC after {MAX_RETRIES} attempts")
200-240: Consider usingmylogfor logging consistency.Per coding guidelines, use
mylog(level, [message])for logging. This script usesprint()statements. While acceptable for a CLI validation tool, consider consistency with the rest of the codebase if this may run within the server context.server/api_server/spec_generator.py (1)
383-426: Consider lock scope optimization for high-concurrency scenarios.The lock is held during the entire spec generation loop. For high-concurrency scenarios, consider copying the registry snapshot first, then releasing the lock before building the spec. Current implementation is acceptable for typical usage.
server/api_server/api_server_start.py (1)
5-6: Remove unusednoqadirectives.Static analysis indicates these
noqa: E402directives are unused. Consider removing them to reduce noise.Also applies to: 62-72
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
schema_validation.pyserver/api_server/api_server_start.pyserver/api_server/schemas.pyserver/api_server/spec_generator.pytest/api_endpoints/test_nettools_endpoints.py
✅ Files skipped from review due to trivial changes (1)
- test/api_endpoints/test_nettools_endpoints.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Always include unit tests or validation steps immediately after writing functions; never leave code unvalidated
Always set explicit timeouts for subprocess calls; default to 60 seconds minimum unless plugin config specifies otherwise
Usenormalize_mac()fromfront/plugins/plugin_helper.pyto normalize MAC addresses before database writes
Usemylog(level, [message])for logging with levels: none/minimal/verbose/debug/trace; usenonefor critical messages that must always appear
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) for all file paths; never hardcode paths like/data/dbor use relative paths
Sanity-check and normalize all text input using sanitizer functions fromserver/helper.pybefore database writes
Files:
server/api_server/api_server_start.pyserver/api_server/spec_generator.pyschema_validation.pyserver/api_server/schemas.py
**/*.{py,js,php}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never use placeholders (e.g.,
// ... rest of code) in code implementations; output full, functional blocks to preserve context
Files:
server/api_server/api_server_start.pyserver/api_server/spec_generator.pyschema_validation.pyserver/api_server/schemas.py
server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/**/*.py: Never hardcode ports or secrets; useget_setting_value()to retrieve all configuration values from settings
Preferserver/db/db_helper.pyfunctions (e.g.,get_table_json, device condition helpers) over raw SQL in new code paths
Use datetime utilities fromserver/utils/datetime_utils.py(e.g.,timeNowDB()) for all timestamp operations
Files:
server/api_server/api_server_start.pyserver/api_server/spec_generator.pyserver/api_server/schemas.py
server/api_server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/api_server/**/*.py: All API responses must include"success": <true|false>and if false, include an"error"message field
All API routes must requireAuthorization: Bearer <API_TOKEN>header validation viaget_setting_value('API_TOKEN')
Files:
server/api_server/api_server_start.pyserver/api_server/spec_generator.pyserver/api_server/schemas.py
🧠 Learnings (1)
📚 Learning: 2026-01-10T04:32:10.552Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.552Z
Learning: Applies to **/*.py : Use environment variables (`NETALERTX_DB`, `NETALERTX_LOG`, etc.) for all file paths; never hardcode paths like `/data/db` or use relative paths
Applied to files:
schema_validation.py
🧬 Code graph analysis (2)
server/api_server/api_server_start.py (3)
server/api_server/validation.py (1)
validate_request(10-107)server/api_server/schemas.py (6)
DeviceSearchRequest(107-123)DeviceListRequest(146-151)WakeOnLanRequest(299-321)TriggerScanRequest(270-276)TracerouteRequest(329-340)RecentEventsRequest(401-414)server/models/device_instance.py (1)
getByStatus(342-373)
schema_validation.py (1)
server/api_server/schemas.py (13)
DeviceListResponse(154-156)GetDeviceResponse(173-175)DeviceInfo(126-138)DeviceTotalsResponse(188-190)DeviceExportResponse(201-204)NetworkTopologyResponse(378-381)BaseResponse(86-92)WakeOnLanResponse(324-326)TracerouteResponse(343-345)RecentEventsResponse(417-420)LastEventsResponse(423-425)DbQueryResponse(580-583)GetSettingResponse(623-625)
🪛 Ruff (0.14.10)
server/api_server/api_server_start.py
5-5: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
62-62: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
64-64: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
65-65: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
server/api_server/spec_generator.py
134-137: Avoid specifying long messages outside the exception class
(TRY003)
1283-1283: Do not catch blind exception: Exception
(BLE001)
schema_validation.py
52-52: Avoid specifying long messages outside the exception class
(TRY003)
71-71: Do not catch blind exception: Exception
(BLE001)
91-91: Consider moving this statement to an else block
(TRY300)
92-92: Do not catch blind exception: Exception
(BLE001)
110-110: Do not catch blind exception: Exception
(BLE001)
122-122: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
122-122: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
122-122: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
123-123: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
123-123: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
150-150: Consider moving this statement to an else block
(TRY300)
152-152: Do not catch blind exception: Exception
(BLE001)
228-228: Consider moving this statement to an else block
(TRY300)
238-238: Do not catch blind exception: Exception
(BLE001)
server/api_server/schemas.py
58-58: Avoid specifying long messages outside the exception class
(TRY003)
70-70: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
70-70: Avoid specifying long messages outside the exception class
(TRY003)
77-77: Avoid specifying long messages outside the exception class
(TRY003)
261-261: Avoid specifying long messages outside the exception class
(TRY003)
430-430: Possible binding to all interfaces
(S104)
539-539: Avoid specifying long messages outside the exception class
(TRY003)
564-564: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
🔇 Additional comments (11)
server/api_server/spec_generator.py (4)
38-41: LGTM - Thread-safe registry with proper locking.The module-level registry uses a lock for thread safety, which is appropriate given the Flask/MCP server context where multiple threads may access the registry.
49-97: LGTM - Well-designed tool management API.The enable/disable functions properly handle thread safety with appropriate lock usage. The
get_tools_statusfunction correctly creates a snapshot of disabled tools before iterating.
100-152: LGTM - Robust endpoint registration with duplicate detection.The function properly enforces unique
operation_idvalues and uses atomic lock acquisition for both the check and the append operation, preventing race conditions.
1280-1286: LGTM - Resilient module initialization.Catching the broad exception here is intentional to allow the module to be imported for testing even if schema imports fail. The warning is appropriately sent to stderr.
server/api_server/api_server_start.py (2)
706-710: LGTM - OpenAPI spec endpoint with proper authorization.The endpoint correctly checks authorization before returning the spec.
479-486: LGTM - Backward-compatible payload handling.The fallback from validated
payloadto rawrequest.jsonpreserves compatibility with legacy clients that don't send properly structured payloads.server/api_server/schemas.py (5)
85-93: LGTM - BaseResponse aligns with API response guidelines.The model correctly enforces the
successanderrorfields as required by coding guidelines.
253-262: LGTM - Good safety guard for bulk delete.The model validator prevents accidental deletion of all devices by requiring explicit confirmation when no MACs are provided.
299-321: LGTM - Well-designed request model with backward compatibility.The alias and
populate_by_name=Trueconfiguration allows bothdevLastIPandipas input field names, maintaining backward compatibility.
428-435: LGTM - Event request model.The
0.0.0.0default is a data placeholder, not a network binding. The static analysis warning (S104) is a false positive in this context.
543-565: LGTM - Strong validation for database updates.The combination of column name pattern validation, allowed tables restriction, and columns/values length matching provides good protection against SQL injection and data integrity issues.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @server/api_server/schemas.py:
- Around line 51-59: Replace the manual .upper() normalization in validate_mac
with the shared normalize_mac() helper: handle the "Internet" special-case first
(return "Internet" unchanged), then call normalize_mac(value) and return its
result; remove the regex/.upper() logic and ensure normalize_mac is imported
from front.plugins.plugin_helper so invalid inputs still raise via the helper.
- Around line 174-181: SetDeviceAliasRequest currently accepts alias without
sanitization which can lead to stored XSS when persisted; add input sanitization
by applying server/helper.py's sanitize_string() to the alias before database
write. Implement either a Pydantic validator on SetDeviceAliasRequest.alias that
replaces the value with sanitize_string(value) or call
sanitize_string(request.alias) in the handler right before calling
updateDeviceColumn(), ensuring the sanitized value is what gets passed to
updateDeviceColumn().
🧹 Nitpick comments (6)
server/api_server/schemas.py (6)
26-28:IP_PATTERNconstant is IPv4-only butvalidate_ipsupports IPv6.The regex pattern on line 27 only matches IPv4 addresses, but
validate_ip(lines 62-67) usesIPvAnyAddresswhich correctly handles both IPv4 and IPv6. If this constant is intended for documentation or used elsewhere, consider updating it to reflect actual validation behavior, or remove it to avoid confusion.
280-293: Missing validation ontargetfield.The
targetfield accepts "IP address or MAC address" per the description, but neither format is validated. Consider adding a validator that accepts either a valid IP (viavalidate_ip) or a valid MAC (viavalidate_mac).♻️ Suggested validator
class OpenPortsRequest(BaseModel): """Request for getting open ports.""" target: str = Field( ..., description="Target IP address or MAC address to check ports for", json_schema_extra={"examples": ["192.168.1.50", "00:11:22:33:44:55"]} ) + + @field_validator("target") + @classmethod + def validate_target(cls, v: str) -> str: + """Accept either valid IP or valid MAC.""" + try: + return validate_ip(v) + except ValueError: + return validate_mac(v)
302-310: Add IP validation fordevLastIPfield.The
devMacfield is validated, butdevLastIP(which is used to resolve the MAC if not provided) lacks validation. Consider adding a field validator to ensure it's a valid IP address when provided.♻️ Suggested addition
@field_validator("devMac") @classmethod def validate_mac_if_provided(cls, v: Optional[str]) -> Optional[str]: if v is not None: return validate_mac(v) return v + + @field_validator("devLastIP") + @classmethod + def validate_ip_if_provided(cls, v: Optional[str]) -> Optional[str]: + if v is not None: + return validate_ip(v) + return v
424-430: Consider validating theipfield inCreateEventRequest.The
ipfield has a default of"0.0.0.0"but no format validation. If a user provides a custom value, it should be validated as a proper IP address to prevent malformed data from entering the database.♻️ Suggested validator
class CreateEventRequest(BaseModel): """Request to create a device event.""" ip: Optional[str] = Field("0.0.0.0", description="Device IP") event_type: str = Field("Device Down", description="Event type") additional_info: Optional[str] = Field("", description="Additional info") pending_alert: int = Field(1, description="Pending alert flag") event_time: Optional[str] = Field(None, description="Event timestamp (ISO)") + + @field_validator("ip") + @classmethod + def validate_ip_address(cls, v: Optional[str]) -> Optional[str]: + if v is not None: + return validate_ip(v) + return v
449-462: Missing MAC/IP validators in session request schemas.
CreateSessionRequestandDeleteSessionRequesthavemacandipfields without validators, unlike other schemas (e.g.,GetDeviceRequest,CopyDeviceRequest) that validate these fields. Consider adding validators for consistency.♻️ Suggested validators for CreateSessionRequest
class CreateSessionRequest(BaseModel): """Request to create a session.""" mac: str = Field(..., description="Device MAC") ip: str = Field(..., description="Device IP") start_time: str = Field(..., description="Start time") end_time: Optional[str] = Field(None, description="End time") event_type_conn: str = Field("Connected", description="Connection event type") event_type_disc: str = Field("Disconnected", description="Disconnection event type") + + @field_validator("mac") + @classmethod + def validate_mac_address(cls, v: str) -> str: + return validate_mac(v) + + @field_validator("ip") + @classmethod + def validate_ip_address(cls, v: str) -> str: + return validate_ip(v) class DeleteSessionRequest(BaseModel): """Request to delete sessions for a MAC.""" mac: str = Field(..., description="Device MAC") + + @field_validator("mac") + @classmethod + def validate_mac_address(cls, v: str) -> str: + return validate_mac(v)
542-545: Consider constrainingidfield type.The
idfield isList[Any], which is very permissive and could accept unexpected types (nested dicts, None, etc.). If the database only expects primitive ID types, consider constraining toList[Union[int, str]]for type safety.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/api_server/schemas.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Always include unit tests or validation steps immediately after writing functions; never leave code unvalidated
Always set explicit timeouts for subprocess calls; default to 60 seconds minimum unless plugin config specifies otherwise
Usenormalize_mac()fromfront/plugins/plugin_helper.pyto normalize MAC addresses before database writes
Usemylog(level, [message])for logging with levels: none/minimal/verbose/debug/trace; usenonefor critical messages that must always appear
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) for all file paths; never hardcode paths like/data/dbor use relative paths
Sanity-check and normalize all text input using sanitizer functions fromserver/helper.pybefore database writes
Files:
server/api_server/schemas.py
**/*.{py,js,php}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never use placeholders (e.g.,
// ... rest of code) in code implementations; output full, functional blocks to preserve context
Files:
server/api_server/schemas.py
server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/**/*.py: Never hardcode ports or secrets; useget_setting_value()to retrieve all configuration values from settings
Preferserver/db/db_helper.pyfunctions (e.g.,get_table_json, device condition helpers) over raw SQL in new code paths
Use datetime utilities fromserver/utils/datetime_utils.py(e.g.,timeNowDB()) for all timestamp operations
Files:
server/api_server/schemas.py
server/api_server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/api_server/**/*.py: All API responses must include"success": <true|false>and if false, include an"error"message field
All API routes must requireAuthorization: Bearer <API_TOKEN>header validation viaget_setting_value('API_TOKEN')
Files:
server/api_server/schemas.py
🪛 Ruff (0.14.10)
server/api_server/schemas.py
58-58: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Avoid specifying long messages outside the exception class
(TRY003)
257-257: Avoid specifying long messages outside the exception class
(TRY003)
426-426: Possible binding to all interfaces
(S104)
535-535: Avoid specifying long messages outside the exception class
(TRY003)
560-560: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
🔇 Additional comments (6)
server/api_server/schemas.py (6)
82-96: LGTM!
BaseResponsecorrectly includessuccessanderrorfields as required by the coding guidelines. Theextra="allow"config provides flexibility for additional response data.
249-258: Good safety mechanism for bulk delete.The
model_validatorrequiring explicit confirmation to delete all devices when themacslist is empty is a solid defensive pattern that prevents accidental mass deletion.
469-493: LGTM!The notification schemas are well-defined with proper
NOTIFICATION_LEVELSLiteral constraint and sensible length limits on content.
518-536: Good safety pattern for dangerous operations.The
confirm_dangerous_queryflag requirement is a solid safeguard against accidental raw SQL execution. The explicit acknowledgment pattern helps prevent misuse.
587-593: LGTM!The
ALLOWED_LOG_FILESLiteral whitelist effectively prevents path traversal attacks by restricting log file operations to a predefined safe set.
1-12: Well-structured schema module.The module follows good practices with clear organization, comprehensive docstrings, and security-conscious design using Literal whitelists and confirmation flags. The "Code First, Spec Second" philosophy is well-executed.
adamoutler
force-pushed
the
openapi-mcp-
branch
from
b63c984 to
7524431
Compare
adamoutler
changed the title
- Implemented dynamic OpenAPI introspection: - Auto-registers Flask routes decorated with @validate_request. - Auto-introspects GraphQL schema Query fields. - Moves metadata from manual registry to 'living code'. - Added Swagger UI at /docs with unauthenticated spec access. - Enriched GraphQL types with descriptions. - Cleaned up API server start script and removed manual registry. - Consolidates validation logic into spec_generator for simplicity. - Groups /mcp/ routes under 'mcp' tag. - Fixed 100% of API unit tests.
adamoutler
force-pushed
the
openapi-mcp-
branch
from
7524431 to
6146ea3
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/api_server/api_server_start.py (1)
594-623: Fix decorator to skip JSON validation for multipart requests.The
@validate_requestdecorator unconditionally callsrequest.get_json()(line 97 of spec_generator.py) even when the request is multipart/form-data. For multipart requests, this raises aBadRequestexception (caught line 116) and returns 400 "Invalid JSON" before your handler runs.The line 93-94 check (
if not request.is_json and request.content_length: pass) exists but does nothing. The decorator should skip request body validation whennot request.is_jsonto allow the handler to process file uploads viarequest.files.
🤖 Fix all issues with AI agents
In `@schema_validation.py`:
- Around line 119-160: The inject_test_device function uses recursion to retry
on MAC collisions (returning inject_test_device()), which risks stack growth;
replace the recursion with a loop: wrap the MAC generation and existence check
in a while True (or for attempt in range(max_attempts)) loop that breaks when a
unique MAC is found, then proceed to the insertion; keep the existing error
handling and finally block (cur.close(), conn.close()) and ensure you return
(mac, ip) after successful insert or raise/exit after exhausting attempts.
- Around line 200-241: The validate function should explicitly catch JSON decode
errors thrown by r.json() before the generic Exception/ValidationError handlers:
add an except clause for requests.exceptions.JSONDecodeError (and fallbacks like
json.JSONDecodeError or ValueError) placed before the ValidationError except,
log a clear message e.g. "FAIL (JSON Decode Error): {e}" and print a safe
preview of the response body (e.g. r.text[:500]) to aid debugging, then return
False; ensure you reference the existing validate function and the r variable
when adding this handler.
In `@server/api_server/api_server_start.py`:
- Around line 220-233: The device endpoints have mismatched response_model
declarations vs actual JSON payloads; update each endpoint so the returned JSON
matches the declared model (or vice versa). Specifically, for the functions that
declare GetDeviceResponse (e.g., the /devices/latest handler) ensure the handler
returns the BaseResponse-wrapped shape {"success": true, "device": {...}}
instead of a raw list/object, and for functions that declare DeviceListResponse
(e.g., the /devices handler) either change the response_model to a wrapper that
includes "success" and "devices" or modify the return to {"success": true,
"devices": [...]}; apply the same fix to the other affected handlers (the ones
noted around lines 491-497, 641-660, 663-699, 701-722) so OpenAPI models and
actual jsonify() payloads are consistent and all responses include the required
"success" field.
In `@server/api_server/mcp_endpoint.py`:
- Around line 944-956: The error responses in this handler (when session_id
missing, when get_session(session_id) returns None, and when
request.get_json(silent=True) is falsy) currently return only {"error": "..."};
update each to include the required success field (e.g., return {"success":
False, "error": "..."}), preserving the existing status codes and messages;
locate the checks around session_id, the call to get_session(session_id) that
produces session, and the data = request.get_json(...) block to apply the
change.
In `@server/api_server/schemas.py`:
- Around line 143-149: DeviceListRequest.status currently omits the "offline"
option which causes POST payloads to fail validation; update the Literal on
DeviceListRequest.status to include "offline" so it matches the GraphQL filters
and the values handled by get_device_condition_by_status(), and update the Field
description to reflect the full set of supported statuses (e.g., "all", "my",
"connected", "favorites", "new", "down", "offline", "archived").
In `@server/api_server/spec_generator.py`:
- Around line 91-94: In the request_model handling block where you check
request.method in ["POST", "PUT", "PATCH"], replace the silent "pass" when
request.is_json is false but request.content_length is present with explicit
handling: detect the unexpected non-JSON content and return or raise an HTTP
error (e.g., 415 Unsupported Media Type or a BadRequest with a clear message) or
append a validation error indicating "Expected JSON content-type" so validation
is not skipped; update the logic around request_model / request.is_json /
request.content_length to ensure non-JSON bodies fail fast with a descriptive
error rather than falling through silently.
- Around line 611-615: The dynamic registry rebuild performed when flask_app is
present (calls to clear_registry(), introspect_graphql_schema(devicesSchema),
introspect_flask_app(flask_app)) must be performed while holding the same
registry lock used by generate_openapi_spec() to avoid race conditions; modify
the code so the clear + introspection sequence executes inside that lock (or
introduce and use a dedicated rebuild lock around the entire clear+introspect
sequence) to ensure no concurrent generate_openapi_spec() or other registry
readers see a partially rebuilt registry.
- Around line 146-149: The module redefines module-level variables _registry,
_registry_lock, _operation_ids, and _disabled_tools later in the file which
resets state; remove the duplicate declarations at the second occurrence so the
original module-level variables (the ones declared earlier) are the single
source of truth, and verify that usages like the validate_request decorator and
any register/unregister functions reference those original names (no new
shadowed definitions) to preserve registry state initialized during module
import.
In `@server/api_server/swagger.html`:
- Around line 8-15: Replace the CDN-only loading of swagger-ui assets in
swagger.html by either bundling and serving swagger-ui-dist CSS/JS from the
application image and updating the <link rel="stylesheet" href="..."> and
<script src="..."> references to local paths, or if continuing to use the CDN,
add Subresource Integrity and proper CORS by adding integrity="..." and
crossorigin="anonymous" attributes to both the <link> and <script> tags; ensure
the integrity hashes match the exact swagger-ui-dist@5.11.0 files and verify the
UI still mounts in the existing <div id="swagger-ui">/swagger-ui-bundle.js
initialization.
♻️ Duplicate comments (5)
schema_validation.py (2)
1-46: Fix DB path resolution: unconditionalfullDbPathimport + duplicated fallback block.
from const import fullDbPath(Line 8) will raise before your env-var logic runs, and Line 41-46 is a duplicated unreachable re-check.Proposed fix
-import sys -import os +import sys +import os import requests import sqlite3 import random import argparse from datetime import datetime -from const import fullDbPath from pydantic import ValidationError @@ # Database Config DB_PATH = os.getenv("NETALERTX_DB") or os.getenv("NETALERTX_DB_PATH") if not DB_PATH: try: - DB_PATH = fullDbPath - except ImportError: + from const import fullDbPath # local fallback import + DB_PATH = fullDbPath + except Exception: print("Error: NETALERTX_DB environment variable not set and const module unavailable") sys.exit(1) -if not DB_PATH: - try: - DB_PATH = fullDbPath - except ImportError: - print("Error: NETALERTX_DB environment variable not set and const module unavailable") - sys.exit(1)
162-279: Guard module-level execution withmain()/if __name__ == "__main__".
Right now importing this file triggers argparse + network calls +sys.exit(), which makes it hard to test/reuse.Suggested structure
+def main() -> int: + print("--- INITIALIZATION ---") + parser = argparse.ArgumentParser(description='Rigorous Schema Validation') + parser.add_argument('--inject-test-device', action='store_true', help='Inject a test device if not present') + parser.add_argument('--cleanup', action='store_true', help='Remove test device after validation') + args = parser.parse_args() + # ... move the current initialization + execution flow here ... + return 0 + +if __name__ == "__main__": + raise SystemExit(main())server/api_server/api_server_start.py (1)
641-653: Auth should run before request validation (information disclosure).
Because@validate_requestwraps the handler, unauthenticated callers can get 4xx validation details instead of 403. Either moveis_authorized()into the decorator, or add anauth_checkhook.server/api_server/schemas.py (2)
52-60: Usenormalize_mac()for MAC normalization (not just.upper()).
Per repo guidance, replace the manual normalization withfront/plugins/plugin_helper.py::normalize_mac()(keeping the"Internet"special case).
175-183: Sanitizealiasinput before DB write to reduce stored-XSS risk.
Either add a Pydantic validator usingserver/helper.pysanitizers or sanitize in the/device/<mac>/set-aliashandler right beforeupdateDeviceColumn(...).
🧹 Nitpick comments (5)
server/api_server/spec_generator.py (2)
136-142: Simplify payload removal usingpop.The TypeError handling can be simplified using dictionary
popmethod as suggested by static analysis.♻️ Proposed refactor
try: return f(*args, **kwargs) except TypeError as e: if "got an unexpected keyword argument 'payload'" in str(e): mylog("none", [f"[Validation] Endpoint {operation_id} does not accept 'payload' argument!"]) - if 'payload' in kwargs: - del kwargs['payload'] + kwargs.pop('payload', None) return f(*args, **kwargs) - raise e + raise
573-577: Consider logging when operationId suffix is added.When duplicate operationIds are encountered, the code silently generates unique IDs with suffixes. This could mask configuration issues where the same
operation_idis accidentally used on multiple endpoints.💡 Suggested improvement
unique_op_id = op_id count = 1 while unique_op_id in _operation_ids: unique_op_id = f"{op_id}_{count}" count += 1 + + if unique_op_id != op_id: + mylog("verbose", [f"[OpenAPI] Renamed duplicate operationId '{op_id}' to '{unique_op_id}'"])server/api_server/mcp_endpoint.py (1)
292-293: Consider renaming unused loop variables.The loop variables
pathandmethodare not used within the loop body. Prefixing with underscore clarifies intent.♻️ Suggested refactor
- for path, methods in spec["paths"].items(): - for method, details in methods.items(): + for _path, methods in spec["paths"].items(): + for _method, details in methods.items():server/api_server/swagger.html (1)
16-28: Make the OpenAPI URL work behind reverse proxies / subpaths.
Hardcodingurl: '/openapi.json'can break when NetAlertX is mounted under a prefix (e.g.,/netalertx/docs). Consider deriving fromwindow.locationor making it configurable via a small inlineconst specUrl = ....server/api_server/api_server_start.py (1)
150-162: Avoid reading full request bodies inbefore_requestlogging.
request.get_data(as_text=True)can load large payloads into memory. If you only need size, preferrequest.content_lengthand skip reading the body entirely (or cap + log a truncated prefix).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.devcontainer/resources/devcontainer-Dockerfileproject.mdschema_validation.pyserver/api_server/api_server_start.pyserver/api_server/graphql_endpoint.pyserver/api_server/mcp_endpoint.pyserver/api_server/schemas.pyserver/api_server/spec_generator.pyserver/api_server/swagger.htmltest/api_endpoints/test_nettools_endpoints.py
✅ Files skipped from review due to trivial changes (1)
- test/api_endpoints/test_nettools_endpoints.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Always include unit tests or validation steps immediately after writing functions; never leave code unvalidated
Always set explicit timeouts for subprocess calls; default to 60 seconds minimum unless plugin config specifies otherwise
Usenormalize_mac()fromfront/plugins/plugin_helper.pyto normalize MAC addresses before database writes
Usemylog(level, [message])for logging with levels: none/minimal/verbose/debug/trace; usenonefor critical messages that must always appear
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) for all file paths; never hardcode paths like/data/dbor use relative paths
Sanity-check and normalize all text input using sanitizer functions fromserver/helper.pybefore database writes
Files:
schema_validation.pyserver/api_server/api_server_start.pyserver/api_server/mcp_endpoint.pyserver/api_server/spec_generator.pyserver/api_server/graphql_endpoint.pyserver/api_server/schemas.py
**/*.{py,js,php}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never use placeholders (e.g.,
// ... rest of code) in code implementations; output full, functional blocks to preserve context
Files:
schema_validation.pyserver/api_server/api_server_start.pyserver/api_server/mcp_endpoint.pyserver/api_server/spec_generator.pyserver/api_server/graphql_endpoint.pyserver/api_server/schemas.py
.devcontainer/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Devcontainer philosophy is brutal simplicity: one user, everything writable, completely idempotent; never use
chmodorchownduring operations
Files:
.devcontainer/resources/devcontainer-Dockerfile
server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/**/*.py: Never hardcode ports or secrets; useget_setting_value()to retrieve all configuration values from settings
Preferserver/db/db_helper.pyfunctions (e.g.,get_table_json, device condition helpers) over raw SQL in new code paths
Use datetime utilities fromserver/utils/datetime_utils.py(e.g.,timeNowDB()) for all timestamp operations
Files:
server/api_server/api_server_start.pyserver/api_server/mcp_endpoint.pyserver/api_server/spec_generator.pyserver/api_server/graphql_endpoint.pyserver/api_server/schemas.py
server/api_server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/api_server/**/*.py: All API responses must include"success": <true|false>and if false, include an"error"message field
All API routes must requireAuthorization: Bearer <API_TOKEN>header validation viaget_setting_value('API_TOKEN')
Files:
server/api_server/api_server_start.pyserver/api_server/mcp_endpoint.pyserver/api_server/spec_generator.pyserver/api_server/graphql_endpoint.pyserver/api_server/schemas.py
🧠 Learnings (13)
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to **/*.py : Use environment variables (`NETALERTX_DB`, `NETALERTX_LOG`, etc.) for all file paths; never hardcode paths like `/data/db` or use relative paths
Applied to files:
schema_validation.py
📚 Learning: 2025-11-05T04:34:52.339Z
Learnt from: jokob-sk
Repo: jokob-sk/NetAlertX PR: 1271
File: server/helper.py:50-71
Timestamp: 2025-11-05T04:34:52.339Z
Learning: In the NetAlertX codebase, `timeNowDB` function is intentionally duplicated in both `server/helper.py` and `server/logger.py` to prevent circular import issues, since `helper.py` imports from `logger.py` (`from logger import mylog, logResult`). This duplication is by design.
Applied to files:
schema_validation.py
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to .devcontainer/** : Devcontainer philosophy is brutal simplicity: one user, everything writable, completely idempotent; never use `chmod` or `chown` during operations
Applied to files:
.devcontainer/resources/devcontainer-Dockerfile
📚 Learning: 2025-09-20T03:01:19.912Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/Dockerfile:18-19
Timestamp: 2025-09-20T03:01:19.912Z
Learning: In the NetAlertX repository, .devcontainer/Dockerfile is auto-generated and should not be reviewed directly. Review comments about dependencies and build steps should be directed at the root Dockerfile where the actual source commands are located.
Applied to files:
.devcontainer/resources/devcontainer-Dockerfile
📚 Learning: 2025-10-26T17:09:18.621Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1235
File: .devcontainer/scripts/setup.sh:146-148
Timestamp: 2025-10-26T17:09:18.621Z
Learning: In `.devcontainer/scripts/setup.sh` and other devcontainer setup scripts for NetAlertX, chmod 666 on /var/run/docker.sock is acceptable because devcontainer environments are single-user development contexts where convenience can take priority over strict permission hardening.
Applied to files:
.devcontainer/resources/devcontainer-Dockerfile
📚 Learning: 2025-09-20T02:56:24.501Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/devcontainer.json:5-8
Timestamp: 2025-09-20T02:56:24.501Z
Learning: In the NetAlertX devcontainer setup, the final .devcontainer/Dockerfile is generated by combining the root Dockerfile with .devcontainer/resources/devcontainer-Dockerfile using the generate-dockerfile.sh script. The devcontainer.json should reference the generated file, not the root Dockerfile.
Applied to files:
.devcontainer/resources/devcontainer-Dockerfile
📚 Learning: 2025-09-20T14:13:33.635Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/Dockerfile:11-16
Timestamp: 2025-09-20T14:13:33.635Z
Learning: In the NetAlertX repository with Alpine 3.22 base image, the `python -m venv` command works correctly in the devcontainer setup, likely due to symlink creation in the root Dockerfile that makes `python` available as an alias to `python3`.
Applied to files:
.devcontainer/resources/devcontainer-Dockerfile
📚 Learning: 2025-09-20T14:09:29.159Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/scripts/setup.sh:103-116
Timestamp: 2025-09-20T14:09:29.159Z
Learning: In NetAlertX devcontainer setup, the netalertx user has write permissions to /var/log/nginx/ directory as it's explicitly chowned to netalertx:www-data in the Dockerfile, so setup.sh can write to nginx log files without sudo.
Applied to files:
.devcontainer/resources/devcontainer-Dockerfile
📚 Learning: 2025-09-20T14:13:33.635Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/Dockerfile:11-16
Timestamp: 2025-09-20T14:13:33.635Z
Learning: In the NetAlertX devcontainer setup, the `python -m venv /opt/venv` command works successfully on Alpine 3.22 despite the typical Alpine behavior of not providing a /usr/bin/python symlink by default. The build completes successfully and pytest runs without issues.
Applied to files:
.devcontainer/resources/devcontainer-Dockerfile
📚 Learning: 2025-09-20T14:08:48.256Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/scripts/setup.sh:90-96
Timestamp: 2025-09-20T14:08:48.256Z
Learning: In the NetAlertX devcontainer setup, the setup.sh script intentionally removes user_notifications.json from the API directory during development environment initialization to prevent notification clutter that accumulates during container launches and development work.
Applied to files:
.devcontainer/resources/devcontainer-Dockerfile
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to server/api_server/**/*.py : All API routes must require `Authorization: Bearer <API_TOKEN>` header validation via `get_setting_value('API_TOKEN')`
Applied to files:
server/api_server/api_server_start.py
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to **/*.py : Use `normalize_mac()` from `front/plugins/plugin_helper.py` to normalize MAC addresses before database writes
Applied to files:
server/api_server/schemas.py
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to **/*.py : Sanity-check and normalize all text input using sanitizer functions from `server/helper.py` before database writes
Applied to files:
server/api_server/schemas.py
🧬 Code graph analysis (2)
schema_validation.py (1)
server/api_server/schemas.py (15)
DeviceSearchResponse(138-140)DeviceListResponse(151-153)GetDeviceResponse(170-172)DeviceInfo(123-135)DeviceTotalsResponse(185-187)DeviceExportResponse(198-201)NetworkTopologyResponse(375-378)BaseResponse(83-89)OpenPortsResponse(290-293)WakeOnLanResponse(321-323)TracerouteResponse(340-342)RecentEventsResponse(414-417)LastEventsResponse(420-422)DbQueryResponse(577-580)GetSettingResponse(620-622)
server/api_server/api_server_start.py (3)
server/api_server/mcp_endpoint.py (1)
mcp_sse(857-929)server/api_server/spec_generator.py (1)
validate_request(54-145)server/models/user_events_queue_instance.py (2)
UserEventsQueueInstance(9-116)add_event(81-116)
🪛 Ruff (0.14.11)
schema_validation.py
52-52: Avoid specifying long messages outside the exception class
(TRY003)
71-71: Do not catch blind exception: Exception
(BLE001)
91-91: Consider moving this statement to an else block
(TRY300)
92-92: Do not catch blind exception: Exception
(BLE001)
110-110: Do not catch blind exception: Exception
(BLE001)
122-122: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
122-122: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
122-122: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
123-123: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
123-123: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
150-150: Consider moving this statement to an else block
(TRY300)
152-152: Do not catch blind exception: Exception
(BLE001)
228-228: Consider moving this statement to an else block
(TRY300)
238-238: Do not catch blind exception: Exception
(BLE001)
server/api_server/api_server_start.py
5-5: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
62-62: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
64-64: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
65-65: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
server/api_server/mcp_endpoint.py
96-96: Do not catch blind exception: Exception
(BLE001)
233-233: Do not catch blind exception: Exception
(BLE001)
292-292: Loop control variable path not used within loop body
Rename unused path to _path
(B007)
293-293: Loop control variable method not used within loop body
Rename unused method to _method
(B007)
658-661: Consider moving this statement to an else block
(TRY300)
668-668: Do not catch blind exception: Exception
(BLE001)
671-671: Use explicit conversion flag
Replace with conversion flag
(RUF010)
685-685: Probable insecure usage of temporary file or directory: "/tmp/log"
(S108)
720-720: Probable insecure usage of temporary file or directory: "/tmp/log"
(S108)
738-738: Do not catch blind exception: Exception
(BLE001)
881-881: Do not catch blind exception: Exception
(BLE001)
server/api_server/spec_generator.py
123-123: Do not catch blind exception: Exception
(BLE001)
140-140: Use pop instead of key in dict followed by del dict[key]
Replace if statement with .pop(..., None)
(RUF051)
142-142: Use raise without specifying exception name
Remove exception name
(TRY201)
284-287: Avoid specifying long messages outside the exception class
(TRY003)
server/api_server/schemas.py
59-59: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Avoid specifying long messages outside the exception class
(TRY003)
258-258: Avoid specifying long messages outside the exception class
(TRY003)
427-427: Possible binding to all interfaces
(S104)
536-536: Avoid specifying long messages outside the exception class
(TRY003)
561-561: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (10)
.devcontainer/resources/devcontainer-Dockerfile (1)
56-56: LGTM! Simplifies venv permissions for devcontainer.The blanket
chmod -R 777aligns with the devcontainer philosophy of "brutal simplicity" as noted in the file comments (lines 6-8). This is acceptable for single-user development contexts where convenience outweighs strict permission hardening. Based on learnings.project.md (1)
1-47: Well-structured technical roadmap document.The project report clearly articulates the migration strategy from manual Pydantic-based registry to GraphQL-driven introspection. The phased approach (metadata migration → bridge building → rerouting/cleanup) provides a sensible implementation path.
server/api_server/mcp_endpoint.py (4)
186-195: LGTM! Authorization check follows coding guidelines.The
check_auth()function correctly validates theAuthorization: Bearer <API_TOKEN>header usingget_setting_value('API_TOKEN')as required by the coding guidelines for API routes.
629-640: Good practice: Explicit timeouts on all HTTP requests.All HTTP methods (POST, PUT, PATCH, DELETE, GET) correctly specify
timeout=60as required by the coding guidelines for subprocess and external calls.
726-729: Good security: Path traversal protection implemented correctly.The
realpathcheck properly prevents directory traversal attacks by ensuring the resolved path stays within the allowed log directory. This is the correct approach for securing file access.
869-870: API response format follows guidelines.The unauthorized response correctly returns
{"success": False, "error": "Unauthorized"}as required by the coding guidelines.server/api_server/graphql_endpoint.py (3)
48-88: LGTM! Comprehensive field documentation added.The Device object fields now have clear, informative descriptions that will enhance both the GraphQL introspection output and the generated OpenAPI spec. This aligns well with the PR's goal of using GraphQL as the "Single Source of Truth" for API documentation.
100-114: LGTM! Setting fields properly documented.Descriptions clearly explain each configuration field's purpose and data type.
138-167: LGTM! AppEvent fields documented for better API clarity.The event-related fields now have descriptions that will help API consumers understand the event structure.
server/api_server/schemas.py (1)
63-69:validate_ip()usingIPvAnyAddressis a good simplification.
This should accept IPv4/IPv6 and normalize canonical string output.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/api_server/api_server_start.py (1)
720-728: Response model mismatch:DeviceListResponseis RootModel but wrapped response returned.The endpoint declares
response_model=DeviceListResponse(aRootModelexpecting a bare list), but line 728 returnsjsonify([favorite])which wraps a single item. Additionally, returning a 404 for no favorites is inconsistent—an empty list would be more appropriate for a list endpoint.💡 Suggested fix
favorite = device_handler.getFavorite() - if not favorite: - return jsonify({"success": False, "message": "No devices found"}), 404 - return jsonify([favorite]) + favorites = favorite if isinstance(favorite, list) else [favorite] if favorite else [] + return jsonify(favorites)
🤖 Fix all issues with AI agents
In `@server/api_server/api_server_start.py`:
- Around line 1218-1235: The endpoint api_events_default_24h performs a manual
is_authorized() check inside the function despite being decorated with
`@validate_request`, which can cause validation to run before auth; update the
`@validate_request` decorator on api_events_default_24h to include
auth_callable=is_authorized (matching other endpoints) and remove the
in-function manual authorization block that returns the 403 so authentication is
enforced by the decorator and malformed requests will be handled in the correct
order.
♻️ Duplicate comments (6)
server/api_server/api_server_start.py (2)
699-707: Response model mismatch:GetDeviceResponsedeclared but raw list returned.The endpoint declares
response_model=GetDeviceResponse(which wraps device in{"success": true, "device": {...}}), but returnsjsonify([latest])(a bare list). This mismatch breaks OpenAPI spec accuracy.🐛 Proposed fix: Return wrapped response
if not latest: - return jsonify({"success": False, "message": "No devices found"}), 404 - return jsonify([latest]) + return jsonify({"success": False, "device": None, "message": "No devices found"}), 404 + return jsonify({"success": True, "device": latest})
485-497: Response model mismatch:DeviceListResponsevs actual wrapped response.The endpoint declares
response_model=DeviceListResponse(RootModel with bare list), but returns{"success": True, "devices": devices}. Either update the response model to match, or change the return to match the declared model.🐛 Option A: Update response to match RootModel
device_handler = DeviceInstance() devices = device_handler.getAll_AsResponse() - return jsonify({"success": True, "devices": devices}) + return jsonify(devices)🐛 Option B: Update response_model to DeviceSearchResponse or similar
`@validate_request`( operation_id="get_all_devices", summary="Get All Devices", description="Retrieve a list of all devices in the system.", - response_model=DeviceListResponse, + response_model=DeviceSearchResponse, # Uses {"success": bool, "devices": [...]} tags=["devices"],server/api_server/spec_generator.py (3)
155-158: Critical: Duplicate module-level variable declarations reset registry state.Lines 46-49 already declare
_registry,_registry_lock,_operation_ids, and_disabled_tools. These duplicate declarations at lines 155-158 will reset the registry after thevalidate_requestdecorator definition, potentially causing data loss if tools were registered during initialization.🐛 Proposed fix: Remove duplicate declarations
-_registry: List[Dict[str, Any]] = [] -_registry_lock = threading.Lock() -_operation_ids: set = set() -_disabled_tools: set = set() - - def introspect_graphql_schema(schema: graphene.Schema):
103-106: Silent pass on non-JSON content may cause confusing validation behavior.When
request_modelexpects JSON but the request has content that isn't JSON (line 105-106), the emptypasssilently proceeds. This could cause validation to be skipped for non-JSON POST requests with body content.💡 Suggested fix
if request.method in ["POST", "PUT", "PATCH"]: if not request.is_json and request.content_length: - pass + return jsonify({ + "success": False, + "error": "Content-Type must be application/json" + }), 415
619-624: Potential race condition during dynamic registry rebuild.When
flask_appis provided,clear_registry()followed by introspection occurs outside the main registry lock. Concurrent calls togenerate_openapi_spec()could observe incomplete registry states.🔒 Suggested fix: Use lock during rebuild
if flask_app: from .graphql_endpoint import devicesSchema - clear_registry() - introspect_graphql_schema(devicesSchema) - introspect_flask_app(flask_app) + with _registry_lock: + _registry.clear() + _operation_ids.clear() + _disabled_tools.clear() + introspect_graphql_schema(devicesSchema) + introspect_flask_app(flask_app)server/api_server/schemas.py (1)
151-156: Add"offline"toDeviceListRequest.statusLiteral to match GraphQL implementation.The GraphQL endpoint at
server/api_server/graphql_endpoint.pyexplicitly supports"offline"status filtering, but this schema's Literal doesn't include it. POST requests withstatus="offline"will fail with 422 validation error.🐛 Proposed fix
class DeviceListRequest(BaseModel): """Request for listing devices by status.""" - status: Optional[Literal["connected", "down", "favorites", "new", "archived", "all", "my"]] = Field( + status: Optional[Literal["connected", "down", "favorites", "new", "archived", "all", "my", "offline"]] = Field( None, - description="Filter devices by status (e.g. 'connected', 'down')" + description="Filter devices by status (e.g. 'connected', 'down', 'offline')" )
🧹 Nitpick comments (5)
server/api_server/spec_generator.py (2)
135-141: BareExceptioncatch is acceptable here but could be narrowed.The broad exception handler at line 135 catches malformed request bodies. While Ruff flags this (BLE001), it's reasonable as a fallback for unexpected parsing errors. However, consider catching
json.JSONDecodeErrorandTypeErrorexplicitly for clarity.♻️ Optional refinement
- except Exception as e: + except (json.JSONDecodeError, TypeError) as e: mylog("verbose", [f"[Validation] Malformed request for {operation_id}: {e}"])
143-150: Fail-fast behavior when endpoint doesn't acceptpayloadis correct but log level should benone.Per coding guidelines, use
mylog('none', [...])for critical messages that must always appear. A missingpayloadparameter is a developer error that should always be visible.♻️ Proposed fix
else: # Fail fast if decorated function doesn't accept payload (Coderabbit fix) - mylog("error", [f"[Validation] Endpoint {operation_id} does not accept 'payload' argument!"]) + mylog("none", [f"[Validation] Endpoint {operation_id} does not accept 'payload' argument!"]) raise TypeError(f"Function {f.__name__} (operationId: {operation_id}) does not accept 'payload' argument.")server/api_server/schemas.py (1)
438-444: Consider using a placeholder constant instead of hardcoded "0.0.0.0".Static analysis flags S104 for the default IP
"0.0.0.0". While this is likely intentional as a placeholder value, consider defining a constant likeDEFAULT_IP = "0.0.0.0"for clarity.server/api_server/api_server_start.py (2)
5-6: Optional: Remove unusednoqadirectives.Static analysis indicates these
noqa: E402comments are unused (Ruff RUF100). They can be safely removed to reduce noise.♻️ Cleanup suggestion
-# flake8: noqa: E402 - from flask import Flask, request, jsonify, ResponseAnd similar cleanup for lines 62-65.
Also applies to: 62-65
1284-1285: Avoid catching bareExceptionwithout specific handling.Line 1284 catches all exceptions, which could mask unexpected errors. Consider logging the full traceback or catching more specific exceptions.
♻️ Suggested improvement
except Exception as ex: + mylog("verbose", [f"[api_events_recent] Error: {ex}"]) return jsonify({"success": False, "error": str(ex)}), 500
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/api_server/api_server_start.pyserver/api_server/schemas.pyserver/api_server/spec_generator.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Always include unit tests or validation steps immediately after writing functions; never leave code unvalidated
Always set explicit timeouts for subprocess calls; default to 60 seconds minimum unless plugin config specifies otherwise
Usenormalize_mac()fromfront/plugins/plugin_helper.pyto normalize MAC addresses before database writes
Usemylog(level, [message])for logging with levels: none/minimal/verbose/debug/trace; usenonefor critical messages that must always appear
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) for all file paths; never hardcode paths like/data/dbor use relative paths
Sanity-check and normalize all text input using sanitizer functions fromserver/helper.pybefore database writes
Files:
server/api_server/spec_generator.pyserver/api_server/schemas.pyserver/api_server/api_server_start.py
**/*.{py,js,php}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never use placeholders (e.g.,
// ... rest of code) in code implementations; output full, functional blocks to preserve context
Files:
server/api_server/spec_generator.pyserver/api_server/schemas.pyserver/api_server/api_server_start.py
server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/**/*.py: Never hardcode ports or secrets; useget_setting_value()to retrieve all configuration values from settings
Preferserver/db/db_helper.pyfunctions (e.g.,get_table_json, device condition helpers) over raw SQL in new code paths
Use datetime utilities fromserver/utils/datetime_utils.py(e.g.,timeNowDB()) for all timestamp operations
Files:
server/api_server/spec_generator.pyserver/api_server/schemas.pyserver/api_server/api_server_start.py
server/api_server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/api_server/**/*.py: All API responses must include"success": <true|false>and if false, include an"error"message field
All API routes must requireAuthorization: Bearer <API_TOKEN>header validation viaget_setting_value('API_TOKEN')
Files:
server/api_server/spec_generator.pyserver/api_server/schemas.pyserver/api_server/api_server_start.py
🧠 Learnings (3)
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to **/*.py : Use `normalize_mac()` from `front/plugins/plugin_helper.py` to normalize MAC addresses before database writes
Applied to files:
server/api_server/schemas.py
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to **/*.py : Sanity-check and normalize all text input using sanitizer functions from `server/helper.py` before database writes
Applied to files:
server/api_server/schemas.py
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to server/api_server/**/*.py : All API routes must require `Authorization: Bearer <API_TOKEN>` header validation via `get_setting_value('API_TOKEN')`
Applied to files:
server/api_server/api_server_start.py
🧬 Code graph analysis (3)
server/api_server/spec_generator.py (1)
server/logger.py (1)
mylog(79-84)
server/api_server/schemas.py (1)
server/helper.py (1)
sanitize_string(535-539)
server/api_server/api_server_start.py (6)
server/api_server/mcp_endpoint.py (4)
mcp_sse(857-929)mcp_messages(932-967)openapi_spec(245-267)check_auth(186-195)server/api_server/spec_generator.py (1)
validate_request(55-154)server/api_server/schemas.py (10)
DeviceSearchRequest(112-128)DeviceSearchResponse(146-148)DeviceListRequest(151-156)DeviceListResponse(159-161)DeviceInfo(131-143)DeviceUpdateRequest(249-260)UpdateDeviceColumnRequest(243-246)CopyDeviceRequest(232-240)GetSettingResponse(633-635)SetDeviceAliasRequest(183-195)server/messaging/in_app.py (4)
write_notification(24-81)get_unread_notifications(197-211)mark_all_notifications_read(135-174)delete_notifications(177-194)server/models/device_instance.py (3)
setDeviceData(501-612)deleteDeviceByMAC(614-621)getByStatus(342-373)server/models/user_events_queue_instance.py (2)
UserEventsQueueInstance(9-116)add_event(81-116)
🪛 Ruff (0.14.11)
server/api_server/spec_generator.py
135-135: Do not catch blind exception: Exception
(BLE001)
149-149: Avoid specifying long messages outside the exception class
(TRY003)
293-296: Avoid specifying long messages outside the exception class
(TRY003)
server/api_server/schemas.py
63-63: Avoid specifying long messages outside the exception class
(TRY003)
76-76: Avoid specifying long messages outside the exception class
(TRY003)
82-82: Avoid specifying long messages outside the exception class
(TRY003)
271-271: Avoid specifying long messages outside the exception class
(TRY003)
440-440: Possible binding to all interfaces
(S104)
549-549: Avoid specifying long messages outside the exception class
(TRY003)
574-574: Avoid specifying long messages outside the exception class
(TRY003)
server/api_server/api_server_start.py
5-5: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
62-62: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
64-64: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
65-65: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
277-277: Unused function argument: payload
(ARG001)
303-303: Unused function argument: payload
(ARG001)
323-323: Unused function argument: payload
(ARG001)
343-343: Unused function argument: payload
(ARG001)
363-363: Unused function argument: payload
(ARG001)
379-379: Unused function argument: payload
(ARG001)
407-407: Unused function argument: payload
(ARG001)
441-441: Unused function argument: payload
(ARG001)
464-464: Unused function argument: payload
(ARG001)
494-494: Unused function argument: payload
(ARG001)
509-509: Unused function argument: payload
(ARG001)
529-529: Unused function argument: payload
(ARG001)
543-543: Unused function argument: payload
(ARG001)
571-571: Unused function argument: payload
(ARG001)
600-600: Unused function argument: payload
(ARG001)
628-628: Unused function argument: payload
(ARG001)
663-663: Unused function argument: payload
(ARG001)
699-699: Unused function argument: payload
(ARG001)
720-720: Unused function argument: payload
(ARG001)
741-741: Unused function argument: payload
(ARG001)
764-764: Unused function argument: payload
(ARG001)
795-795: Unused function argument: payload
(ARG001)
870-870: Unused function argument: payload
(ARG001)
1017-1017: Unused function argument: payload
(ARG001)
1040-1040: Unused function argument: payload
(ARG001)
1060-1060: Unused function argument: payload
(ARG001)
1099-1099: Unused function argument: payload
(ARG001)
1127-1127: Unused function argument: payload
(ARG001)
1143-1143: Unused function argument: payload
(ARG001)
1164-1164: Unused function argument: payload
(ARG001)
1185-1185: Unused function argument: payload
(ARG001)
1209-1209: Unused function argument: payload
(ARG001)
1224-1224: Unused function argument: payload
(ARG001)
1248-1248: Unused function argument: payload
(ARG001)
1270-1270: Unused function argument: payload
(ARG001)
1302-1302: Unused function argument: payload
(ARG001)
1329-1329: Unused function argument: payload
(ARG001)
1350-1350: Unused function argument: payload
(ARG001)
1371-1371: Unused function argument: payload
(ARG001)
1390-1390: Unused function argument: payload
(ARG001)
1407-1407: Unused function argument: payload
(ARG001)
1425-1425: Unused function argument: payload
(ARG001)
1443-1443: Unused function argument: payload
(ARG001)
1463-1463: Unused function argument: payload
(ARG001)
1476-1476: Unused function argument: payload
(ARG001)
1489-1489: Unused function argument: payload
(ARG001)
1507-1507: Unused function argument: payload
(ARG001)
1530-1530: Unused function argument: payload
(ARG001)
1550-1550: Unused function argument: payload
(ARG001)
1574-1574: Unused function argument: payload
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: docker_dev
- GitHub Check: docker-tests
🔇 Additional comments (16)
server/api_server/spec_generator.py (4)
55-66: LGTM: Decorator signature is well-designed.The
validate_requestdecorator properly supports operation metadata, optional request/response models, validation error codes, and anauth_callablefor authorization checks. Theauth_callableparameter addresses the past concern about authorization occurring after validation.
208-227: LGTM: Thread-safe tool enablement/disablement.The
set_tool_disabledfunction properly uses_registry_lockto ensure thread safety when modifying_disabled_tools, and correctly validates that theoperation_idexists before modification.
259-311: LGTM: Registry registration with duplicate detection.The
register_toolfunction properly enforces uniqueoperationIdvalues with a clear exception, uses thread-safe locking, and captures all necessary OpenAPI metadata.
378-405: LGTM: Recursive $defs extraction and $ref rewriting.The
_extract_definitionsfunction correctly handles nested schemas, rewrites$refpaths from#/$defs/to#/components/schemas/, and processes arrays recursively.server/api_server/schemas.py (7)
56-65: LGTM: MAC validation now usesnormalize_mac()andis_mac()helpers.The
validate_macfunction correctly handles the "Internet" special case and delegates to the shared helpers fromplugin_helper.py, ensuring consistent MAC normalization before database writes. This addresses the coding guidelines requirement.
68-76: LGTM: IP validation now supports IPv4/IPv6 with proper exception chaining.The
validate_ipfunction usesipaddress.ip_address()for robust validation and properly chains the exception withfrom err. This addresses both the IPv6 support requirement and exception chaining concern from past reviews.
192-195: LGTM: Alias sanitization usingsanitize_string()helper.The
sanitize_aliasfield validator appliessanitize_string()to prevent stored XSS, addressing the coding guidelines requirement to sanitize text input before database writes.
91-97: LGTM: BaseResponse provides consistent API response structure.The
BaseResponsemodel properly includessuccess,message, anderrorfields as per coding guidelines. Usingextra="allow"enables additional fields for flexibility.
263-272: LGTM: DeleteDevicesRequest includes safety validator for bulk delete.The
check_delete_all_safetymodel validator correctly prevents accidental deletion of all devices by requiring either a non-emptymacslist or explicitconfirm_delete_all=True.
532-550: Intentional raw SQL endpoint per project owner confirmation.The
DbQueryRequestschema withconfirm_dangerous_queryflag is intentionally designed for administrative use. Per past review discussion, this is acknowledged as intentional. The MCP disablement mechanism provides additional runtime protection.
309-331: LGTM: WakeOnLanRequest with dual-field flexibility.The schema properly supports both
devMacanddevLastIP(withipalias) usingpopulate_by_name=True, allowing clients to provide either field. The MAC validator handlesNonecorrectly.server/api_server/api_server_start.py (5)
131-148: LGTM: Centralized authorization function with proper logging.The
is_authorizedfunction correctly checks both Bearer token header and query string token (for SSE endpoints), logs unauthorized attempts withwrite_notification, and follows the coding guidelines for API_TOKEN validation.
240-256: LGTM: Settings endpoint with proper auth_callable integration.The
@validate_requestdecorator correctly passesauth_callable=is_authorizedto ensure authorization happens within the decorator flow, addressing the past concern about auth order.
883-898: LGTM:trigger_scanhelper with proper validation and mock handling.The helper validates scan types against an allowlist, uses the queue instance, and gracefully handles test mocks that don't return tuples. The response structure is consistent.
904-927: Public/openapi.jsonand/docsendpoints are documented exceptions.Per past review discussion, these endpoints intentionally allow unauthenticated access so Swagger UI can load the spec. The actual API endpoints remain protected via
auth_callable. This design is acceptable but should be documented in.github/copilot-instructions.md.Consider adding documentation for this exception in the coding guidelines:
Exception: `/openapi.json` and `/docs` are intentionally public to enable documentation discovery; actual API endpoints remain protected.
170-181: LGTM: Request logging filters sensitive headers.The
log_request_infofunction properly excludesauthorization,cookie, andx-api-keyheaders from debug logs, preventing credential leakage.
adamoutler
force-pushed
the
openapi-mcp-
branch
from
3486db6 to
7c47bed
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/api_server/api_server_start.py (1)
589-616: Multipart file uploads are blocked by JSON validation in the decorator.The
@validate_requestdecorator withrequest_model=DeviceImportRequestunconditionally callsrequest.get_json()(line 107 in spec_generator.py). For multipart requests, this returnsNone, resulting in validation of an empty dict{}againstDeviceImportRequest. Sincecontentis a required field with no default, validation fails with a 422 error before the handler executes.The handler's
request.files.get("file")path is unreachable. Either:
- Remove the request model validation if multipart upload is needed, or
- Make
contentoptional and add logic to skip validation when files are present, or- Use separate endpoints for JSON vs. multipart uploads.
🤖 Fix all issues with AI agents
In `@server/api_server/api_server_start.py`:
- Around line 633-650: The api_devices_by_status view will receive an injected
payload from the validate_request decorator on POST but the function signature
lacks that parameter, causing a TypeError at runtime; update
api_devices_by_status to accept the injected payload argument (use the name
payload), then extract status from payload first (fallback to request.args for
GET) and pass that status into DeviceInstance().getByStatus(status) as before so
both GET and POST work without errors.
- Around line 485-498: The declared OpenAPI response_models do not match the
actual return shapes for the three device endpoints, causing an incorrect
Swagger spec; update the validate_request decorators to use response models that
reflect the actual legacy payloads (or create new wrapper models) rather than
changing handler logic: for api_get_devices replace DeviceListResponse with a
model matching {"success": bool, "devices": List[Device]} (e.g.,
DeviceListWrapperResponse); for the /devices/latest endpoint update its
response_model to a model matching the actual return (raw device or {"success",
"device"} depending on the handler) or change the handler to return the declared
GetDeviceResponse shape; and for /devices/favorite replace DeviceListResponse
with a model matching the actual legacy list output (or change that handler to
return the declared RootModel). Ensure these new models are referenced in the
validate_request decorators for the functions handling /devices, /devices/latest
and /devices/favorite so the OpenAPI spec matches runtime responses.
- Around line 131-149: is_authorized currently treats empty expected_token as
valid because "" == ""; change it to fail closed by verifying expected_token is
non-empty before comparing: retrieve expected_token via
get_setting_value('API_TOKEN'), check that its length (or non-emptiness) is > 0
(same pattern as server/api.py's len(api_token_value) > 1), and only then
compare header_token or query_token to expected_token; if expected_token is
empty, treat as unauthorized, call write_notification/mylog and return False.
In `@server/api_server/schemas.py`:
- Around line 151-157: The DeviceListRequest.status field is missing the
"offline" literal (and other GraphQL-supported statuses), causing 422s; update
the Literal in DeviceListRequest.status to include "offline" (and optionally
"my_devices", "network_devices", "all_devices", or any other missing GraphQL
statuses) and adjust Field description accordingly, and modify
get_device_condition_by_status() to handle the "offline" case by returning the
condition devPresentLastScan=0 AND devIsArchived=0 (and map any newly added
status strings to their corresponding DB conditions) so REST validation and
backend filtering match GraphQL behavior.
In `@server/api_server/spec_generator.py`:
- Around line 619-625: The dynamic registry rebuild in generate_openapi_spec is
not atomic: clear_registry() is called then introspect_graphql_schema() and
introspect_flask_app(), allowing concurrent callers to observe a partially
rebuilt registry; fix by introducing a dedicated rebuild lock (e.g., a
module-level threading.Lock or asyncio.Lock depending on runtime) and wrap the
whole rebuild sequence (clear_registry(),
introspect_graphql_schema(devicesSchema), introspect_flask_app(flask_app) and
any similar rebuild blocks around lines 660-705) in a single critical section so
only one thread/coroutine can perform the clear+repopulate steps at once,
acquiring the lock before clear_registry() and releasing it after all
introspection/registration completes.
- Around line 148-150: The mylog call uses an invalid level "error"; change it
to one of the repo's allowed levels (e.g., "minimal" for error conditions) in
the mylog invocation that logs the missing 'payload' argument (the call
referencing operation_id), and ensure any other mylog calls in the same function
(or nearby logging for f.__name__/operation_id) are updated to use the approved
levels so logging remains consistent with the guidelines.
♻️ Duplicate comments (3)
server/api_server/api_server_start.py (2)
904-928: Public/openapi.jsonand/docslikely violate the “all routes require Bearer auth” guideline unless explicitly documented.If these are intentional public exceptions, document the exception in the repo guidelines and ensure the spec itself still declares BearerAuth for protected operations.
1218-1235: Passauth_callable=is_authorizedto the decorator and remove the manual auth check inside the handler.The
@validate_requestdecorator supportsauth_callableand checks authorization before validating the request body. Currently, this endpoint manually checks auth inside the handler, allowing validation errors (422) to be returned before authorization failures (403). This should be fixed to match the pattern already used in adjacent endpoints likeapi_get_events_totalsandget_last_events.Proposed fix
`@validate_request`( operation_id="get_recent_events", summary="Get Recent Events", description="Get recent events from the system.", - request_model=RecentEventsRequest + request_model=RecentEventsRequest, + auth_callable=is_authorized ) def api_events_default_24h(payload=None): - if not is_authorized(): - return jsonify({"success": False, "message": "ERROR: Not authorized", "error": "Forbidden"}), 403 - hours = 24server/api_server/spec_generator.py (1)
155-158: Remove duplicate module-level registry declarations (will reset state).Lines 155-158 re-declare
_registry,_registry_lock,_operation_ids,_disabled_tools, which can wipe already-registered tools depending on import/load order.Proposed fix
@@ - return wrapper - return decorator -_registry: List[Dict[str, Any]] = [] -_registry_lock = threading.Lock() -_operation_ids: set = set() -_disabled_tools: set = set() + return wrapper + return decorator
🧹 Nitpick comments (3)
server/api_server/spec_generator.py (1)
526-534: Consider removing placeholder functions (per guidelines).
_flatten_defs()and_register_all_endpoints()are placeholders (pass). If they’re not used, deleting them is cleaner; if they are required for compatibility, consider making them explicit no-ops with a comment explaining why they must exist.Also applies to: 730-733
server/api_server/schemas.py (1)
159-201: RootModel legacy list responses may conflict with API-server response guidelines.
DeviceListResponse/DeviceTotalsResponseare bare lists (legacy-compatible), butserver/api_server/**/*.pyguidelines say responses should include"success". If legacy list responses are intentional exceptions, consider documenting the exception (and ensure OpenAPI uses these models only where the endpoint truly returns a bare list).server/api_server/api_server_start.py (1)
1591-1604: Avoid hardcoding bind-all + debug in server start unless explicitly required.
app.run(host="0.0.0.0", debug=True)is risky outside dev environments; consider deriving these from settings.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/api_server/api_server_start.pyserver/api_server/schemas.pyserver/api_server/spec_generator.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Always include unit tests or validation steps immediately after writing functions; never leave code unvalidated
Always set explicit timeouts for subprocess calls; default to 60 seconds minimum unless plugin config specifies otherwise
Usenormalize_mac()fromfront/plugins/plugin_helper.pyto normalize MAC addresses before database writes
Usemylog(level, [message])for logging with levels: none/minimal/verbose/debug/trace; usenonefor critical messages that must always appear
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) for all file paths; never hardcode paths like/data/dbor use relative paths
Sanity-check and normalize all text input using sanitizer functions fromserver/helper.pybefore database writes
Files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.pyserver/api_server/schemas.py
**/*.{py,js,php}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never use placeholders (e.g.,
// ... rest of code) in code implementations; output full, functional blocks to preserve context
Files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.pyserver/api_server/schemas.py
server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/**/*.py: Never hardcode ports or secrets; useget_setting_value()to retrieve all configuration values from settings
Preferserver/db/db_helper.pyfunctions (e.g.,get_table_json, device condition helpers) over raw SQL in new code paths
Use datetime utilities fromserver/utils/datetime_utils.py(e.g.,timeNowDB()) for all timestamp operations
Files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.pyserver/api_server/schemas.py
server/api_server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/api_server/**/*.py: All API responses must include"success": <true|false>and if false, include an"error"message field
All API routes must requireAuthorization: Bearer <API_TOKEN>header validation viaget_setting_value('API_TOKEN')
Files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.pyserver/api_server/schemas.py
🧠 Learnings (3)
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to server/api_server/**/*.py : All API routes must require `Authorization: Bearer <API_TOKEN>` header validation via `get_setting_value('API_TOKEN')`
Applied to files:
server/api_server/api_server_start.py
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to **/*.py : Use `normalize_mac()` from `front/plugins/plugin_helper.py` to normalize MAC addresses before database writes
Applied to files:
server/api_server/schemas.py
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to **/*.py : Sanity-check and normalize all text input using sanitizer functions from `server/helper.py` before database writes
Applied to files:
server/api_server/schemas.py
🧬 Code graph analysis (3)
server/api_server/spec_generator.py (2)
server/logger.py (1)
mylog(79-84)server/api_server/api_server_start.py (1)
graphql_endpoint(209-231)
server/api_server/api_server_start.py (3)
server/api_server/spec_generator.py (1)
validate_request(55-154)server/api_server/schemas.py (36)
DeviceSearchRequest(112-128)DeviceListRequest(151-156)DeviceListResponse(159-161)DeviceInfo(131-143)GetDeviceResponse(178-180)BaseResponse(91-97)DeviceTotalsResponse(198-200)DeleteDevicesRequest(263-272)DeviceImportRequest(217-222)DeviceImportResponse(225-229)UpdateDeviceColumnRequest(243-246)CopyDeviceRequest(232-240)TriggerScanRequest(280-286)OpenPortsRequest(294-300)OpenPortsResponse(303-306)WakeOnLanRequest(309-331)TracerouteRequest(339-350)TracerouteResponse(353-355)NmapScanRequest(358-372)NslookupRequest(375-385)RecentEventsResponse(427-430)LastEventsResponse(433-435)NetworkTopologyResponse(388-391)CreateEventRequest(438-444)CreateSessionRequest(463-470)DeleteSessionRequest(473-475)CreateNotificationRequest(495-506)SyncPushRequest(514-518)SyncPullResponse(521-524)DbQueryRequest(532-550)DbQueryUpdateRequest(553-575)DbQueryDeleteRequest(578-587)AddToQueueRequest(617-619)GetSettingResponse(633-635)RecentEventsRequest(411-424)SetDeviceAliasRequest(183-195)server/models/user_events_queue_instance.py (2)
UserEventsQueueInstance(9-116)add_event(81-116)
server/api_server/schemas.py (2)
server/helper.py (1)
sanitize_string(535-539)front/plugins/plugin_helper.py (1)
is_mac(91-99)
🪛 Ruff (0.14.11)
server/api_server/spec_generator.py
135-135: Do not catch blind exception: Exception
(BLE001)
149-149: Avoid specifying long messages outside the exception class
(TRY003)
293-296: Avoid specifying long messages outside the exception class
(TRY003)
server/api_server/api_server_start.py
5-5: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
62-62: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
64-64: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
65-65: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
277-277: Unused function argument: payload
(ARG001)
303-303: Unused function argument: payload
(ARG001)
323-323: Unused function argument: payload
(ARG001)
343-343: Unused function argument: payload
(ARG001)
363-363: Unused function argument: payload
(ARG001)
379-379: Unused function argument: payload
(ARG001)
407-407: Unused function argument: payload
(ARG001)
441-441: Unused function argument: payload
(ARG001)
464-464: Unused function argument: payload
(ARG001)
494-494: Unused function argument: payload
(ARG001)
509-509: Unused function argument: payload
(ARG001)
529-529: Unused function argument: payload
(ARG001)
543-543: Unused function argument: payload
(ARG001)
571-571: Unused function argument: payload
(ARG001)
600-600: Unused function argument: payload
(ARG001)
628-628: Unused function argument: payload
(ARG001)
663-663: Unused function argument: payload
(ARG001)
699-699: Unused function argument: payload
(ARG001)
720-720: Unused function argument: payload
(ARG001)
741-741: Unused function argument: payload
(ARG001)
764-764: Unused function argument: payload
(ARG001)
795-795: Unused function argument: payload
(ARG001)
870-870: Unused function argument: payload
(ARG001)
1017-1017: Unused function argument: payload
(ARG001)
1040-1040: Unused function argument: payload
(ARG001)
1060-1060: Unused function argument: payload
(ARG001)
1099-1099: Unused function argument: payload
(ARG001)
1127-1127: Unused function argument: payload
(ARG001)
1143-1143: Unused function argument: payload
(ARG001)
1164-1164: Unused function argument: payload
(ARG001)
1185-1185: Unused function argument: payload
(ARG001)
1209-1209: Unused function argument: payload
(ARG001)
1224-1224: Unused function argument: payload
(ARG001)
1248-1248: Unused function argument: payload
(ARG001)
1270-1270: Unused function argument: payload
(ARG001)
1302-1302: Unused function argument: payload
(ARG001)
1329-1329: Unused function argument: payload
(ARG001)
1350-1350: Unused function argument: payload
(ARG001)
1371-1371: Unused function argument: payload
(ARG001)
1390-1390: Unused function argument: payload
(ARG001)
1407-1407: Unused function argument: payload
(ARG001)
1425-1425: Unused function argument: payload
(ARG001)
1443-1443: Unused function argument: payload
(ARG001)
1463-1463: Unused function argument: payload
(ARG001)
1476-1476: Unused function argument: payload
(ARG001)
1489-1489: Unused function argument: payload
(ARG001)
1507-1507: Unused function argument: payload
(ARG001)
1530-1530: Unused function argument: payload
(ARG001)
1550-1550: Unused function argument: payload
(ARG001)
1574-1574: Unused function argument: payload
(ARG001)
server/api_server/schemas.py
63-63: Avoid specifying long messages outside the exception class
(TRY003)
76-76: Avoid specifying long messages outside the exception class
(TRY003)
82-82: Avoid specifying long messages outside the exception class
(TRY003)
271-271: Avoid specifying long messages outside the exception class
(TRY003)
440-440: Possible binding to all interfaces
(S104)
549-549: Avoid specifying long messages outside the exception class
(TRY003)
574-574: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: docker-tests
- GitHub Check: docker_dev
🔇 Additional comments (2)
server/api_server/schemas.py (1)
56-77: MAC/IP validators look good and align with repo guidance.
validate_mac()usesis_mac()+normalize_mac(), andSetDeviceAliasRequestsanitizesaliasviasanitize_string()(good for preventing stored XSS before DB writes). Based on learnings/coding guidelines.server/api_server/api_server_start.py (1)
425-450: Cannot verify this review comment against the current codebase.The review references code patterns (
@validate_requestdecorator, Pydantic models likeSetDeviceAliasRequest, and specific handler functions with those exact implementations) that do not exist in the current repository. The decorator, models, and function signatures described cannot be found through comprehensive searches.This comment appears to be either: (1) reviewing code from an uncommitted pull request/diff, (2) referencing a planned refactor not yet committed, or (3) from a different branch/version of the codebase.
To properly verify this review, please provide:
- The actual diff or pull request context
- Confirmation that these changes are staged or in a specific branch
- The current state of the file being reviewed
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
adamoutler
force-pushed
the
openapi-mcp-
branch
from
7c47bed to
9390050
Compare
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 (1)
server/api_server/api_server_start.py (1)
603-629: Multipart file upload will fail due to validation in decorator.This endpoint intends to support both JSON (lines 619-620) and file uploads (lines 621-622), but the
@validate_requestdecorator withrequest_model=DeviceImportRequestwill return 415 for multipart requests before the endpoint code executes. This relates to the issue flagged in thespec_generator.pyreview.Consider adding
allow_multipart=Trueparameter to the decorator (once implemented) or removing the request_model for this endpoint and validating manually within the function.
🤖 Fix all issues with AI agents
In `@server/api_server/mcp_endpoint.py`:
- Around line 944-966: The queue.Full exception handler returns a response that
uses "status" and "message" instead of the project's standard "success":
<true|false> format; update the except queue.Full block (where
message_queue.put(response, timeout=5) is called) to return a JSON object
consistent with the others—e.g., jsonify({"success": False, "error": "Queue
full"})—and keep the same 503 status code so callers see a uniform "success"
field while preserving the existing log call to mylog.
- Around line 186-195: check_auth currently treats empty API_TOKEN as valid
because it builds an expected_token from an empty value; change it to fail
closed by first retrieving the raw API token via get_setting_value('API_TOKEN')
and returning False if that value is None or an empty string, then construct the
expected "Bearer {token}" header and compare to
request.headers.get("Authorization") (optionally strip whitespace) so that an
unset API_TOKEN never grants access; update the function check_auth to use the
raw token variable rather than assuming get_setting_value always returns a
configured token.
♻️ Duplicate comments (2)
server/api_server/api_server_start.py (1)
1302-1310: Inconsistent: manual auth check instead ofauth_callablein decorator.This endpoint performs a manual
is_authorized()check at lines 1309-1310 but lacksauth_callable=is_authorizedin the@validate_requestdecorator. This differs from other endpoints and means malformed requests will receive 422 (validation error) before 403 (forbidden).🐛 Proposed fix
`@validate_request`( operation_id="get_recent_events", summary="Get Recent Events", description="Get recent events from the system.", - request_model=RecentEventsRequest + request_model=RecentEventsRequest, + tags=["events"], + auth_callable=is_authorized ) def api_events_default_24h(payload=None): - if not is_authorized(): - return jsonify({"success": False, "message": "ERROR: Not authorized", "error": "Forbidden"}), 403 - hours = 24server/api_server/spec_generator.py (1)
105-143: Multipart file upload endpoints will still fail validation.The 415 response at lines 107-108 is an improvement over the silent
pass, but endpoints like/devices/importthat support both JSON and multipart file uploads will now receive a 415 error before the endpoint code can checkrequest.files. The endpoint implementation atapi_server_start.py:619-622shows intent to support both paths.Consider adding a parameter to skip JSON validation for multipart-capable endpoints, or handle this case in the decorator:
💡 Suggested approach
def validate_request( operation_id: str, # ... other params + allow_multipart: bool = False, ):Then in the wrapper:
if request_model: if request.method in ["POST", "PUT", "PATCH"]: - if not request.is_json and request.content_length: + if not request.is_json and request.content_length and not allow_multipart: return jsonify({"success": False, "error": "Invalid Content-Type", ...}), 415
🧹 Nitpick comments (4)
server/db/db_helper.py (1)
38-45: Update the docstring to document the new status options.The implementation is correct, but the docstring (lines 22-33) doesn't include the newly added statuses:
all_devices,my_devices,network_devices, andoffline. This will help future maintainers understand the available options.📝 Suggested docstring update
def get_device_condition_by_status(device_status): """ Return the SQL WHERE clause for filtering devices based on their status. Parameters: device_status (str): The status of the device. Possible values: - 'all' : All active devices - 'my' : Same as 'all' (active devices) + - 'all_devices': Same as 'all' (active devices) + - 'my_devices' : Same as 'all' (active devices) + - 'network_devices': Same as 'all' (active devices) - 'connected' : Devices that are active and present in the last scan - 'favorites' : Devices marked as favorite - 'new' : Devices marked as new - 'down' : Devices not present in the last scan but with alerts + - 'offline' : Devices not present in the last scan (regardless of alerts) - 'archived' : Devices that are archived Returns: str: SQL WHERE clause corresponding to the device status. Defaults to 'WHERE 1=0' for unrecognized statuses. """server/api_server/schemas.py (1)
30-32: Remove unusedIP_PATTERNconstant.The
IP_PATTERNregex (IPv4 only) is defined butvalidate_ip()usesipaddress.ip_address()for validation, which supports both IPv4 and IPv6. This unused pattern should be removed to avoid confusion about which validation method is canonical.server/api_server/spec_generator.py (1)
617-623: Rebuild lock addresses concurrency but spec read happens outside rebuild lock.The
_rebuild_lockwrapping the rebuild sequence (lines 619-623) addresses the atomic rebuild concern. However, the spec generation at lines 659-702 acquires only_registry_lock, not_rebuild_lock. If another request triggers a rebuild while this request is generating the spec, there could still be inconsistency.For most use cases this is likely acceptable since rebuilds are infrequent. If strict consistency is required, consider holding
_rebuild_lockas a read lock during spec generation, or using a readers-writer lock pattern.server/api_server/api_server_start.py (1)
964-988: Public/openapi.jsonand/docsendpoints are intentionally unauthenticated.The inline comment at lines 967-968 clearly documents this design decision: "Allow unauthenticated access to the spec itself so Swagger UI can load. The actual API endpoints remain protected."
As suggested in past reviews, consider adding this exception to
.github/copilot-instructions.mdunderserver/api_server/**/*.pyto document this architectural decision for future contributors.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.gemini/settings.json.github/copilot-instructions.mdserver/api_server/api_server_start.pyserver/api_server/mcp_endpoint.pyserver/api_server/schemas.pyserver/api_server/spec_generator.pyserver/api_server/swagger.htmlserver/db/db_helper.pytest/api_endpoints/test_dbquery_endpoints.pytest/api_endpoints/test_device_endpoints.pytest/api_endpoints/test_mcp_extended_endpoints.pytest/api_endpoints/test_mcp_tools_endpoints.py
✅ Files skipped from review due to trivial changes (1)
- .gemini/settings.json
🚧 Files skipped from review as they are similar to previous changes (2)
- test/api_endpoints/test_mcp_extended_endpoints.py
- server/api_server/swagger.html
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Always include unit tests or validation steps immediately after writing functions; never leave code unvalidated
Always set explicit timeouts for subprocess calls; default to 60 seconds minimum unless plugin config specifies otherwise
Usenormalize_mac()fromfront/plugins/plugin_helper.pyto normalize MAC addresses before database writes
Usemylog(level, [message])for logging with levels: none/minimal/verbose/debug/trace; usenonefor critical messages that must always appear
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) for all file paths; never hardcode paths like/data/dbor use relative paths
Sanity-check and normalize all text input using sanitizer functions fromserver/helper.pybefore database writes
Files:
server/db/db_helper.pytest/api_endpoints/test_dbquery_endpoints.pytest/api_endpoints/test_device_endpoints.pytest/api_endpoints/test_mcp_tools_endpoints.pyserver/api_server/spec_generator.pyserver/api_server/schemas.pyserver/api_server/api_server_start.pyserver/api_server/mcp_endpoint.py
**/*.{py,js,php}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never use placeholders (e.g.,
// ... rest of code) in code implementations; output full, functional blocks to preserve context
Files:
server/db/db_helper.pytest/api_endpoints/test_dbquery_endpoints.pytest/api_endpoints/test_device_endpoints.pytest/api_endpoints/test_mcp_tools_endpoints.pyserver/api_server/spec_generator.pyserver/api_server/schemas.pyserver/api_server/api_server_start.pyserver/api_server/mcp_endpoint.py
server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/**/*.py: Never hardcode ports or secrets; useget_setting_value()to retrieve all configuration values from settings
Preferserver/db/db_helper.pyfunctions (e.g.,get_table_json, device condition helpers) over raw SQL in new code paths
Use datetime utilities fromserver/utils/datetime_utils.py(e.g.,timeNowDB()) for all timestamp operations
Files:
server/db/db_helper.pyserver/api_server/spec_generator.pyserver/api_server/schemas.pyserver/api_server/api_server_start.pyserver/api_server/mcp_endpoint.py
server/api_server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/api_server/**/*.py: All API responses must include"success": <true|false>and if false, include an"error"message field
All API routes must requireAuthorization: Bearer <API_TOKEN>header validation viaget_setting_value('API_TOKEN')
Files:
server/api_server/spec_generator.pyserver/api_server/schemas.pyserver/api_server/api_server_start.pyserver/api_server/mcp_endpoint.py
🧠 Learnings (8)
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to server/**/*.py : Prefer `server/db/db_helper.py` functions (e.g., `get_table_json`, device condition helpers) over raw SQL in new code paths
Applied to files:
server/db/db_helper.py
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to server/api_server/**/*.py : All API routes must require `Authorization: Bearer <API_TOKEN>` header validation via `get_setting_value('API_TOKEN')`
Applied to files:
.github/copilot-instructions.mdserver/api_server/api_server_start.py
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to server/api_server/**/*.py : All API responses must include `"success": <true|false>` and if false, include an `"error"` message field
Applied to files:
.github/copilot-instructions.mdserver/api_server/mcp_endpoint.py
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to **/*.py : Use environment variables (`NETALERTX_DB`, `NETALERTX_LOG`, etc.) for all file paths; never hardcode paths like `/data/db` or use relative paths
Applied to files:
test/api_endpoints/test_mcp_tools_endpoints.py
📚 Learning: 2025-10-26T19:36:26.482Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1235
File: server/api_server/nettools_endpoint.py:13-34
Timestamp: 2025-10-26T19:36:26.482Z
Learning: In server/api_server/nettools_endpoint.py, the use of print() for module-level initialization warnings is acceptable and should be reviewed by the primary maintainer. The logger.mylog guideline may be specific to plugin code rather than core server code.
Applied to files:
server/api_server/spec_generator.py
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to **/*.py : Use `mylog(level, [message])` for logging with levels: none/minimal/verbose/debug/trace; use `none` for critical messages that must always appear
Applied to files:
server/api_server/spec_generator.py
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to **/*.py : Use `normalize_mac()` from `front/plugins/plugin_helper.py` to normalize MAC addresses before database writes
Applied to files:
server/api_server/schemas.py
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to **/*.py : Sanity-check and normalize all text input using sanitizer functions from `server/helper.py` before database writes
Applied to files:
server/api_server/schemas.py
🧬 Code graph analysis (6)
test/api_endpoints/test_dbquery_endpoints.py (3)
test/api_endpoints/test_mcp_extended_endpoints.py (3)
client(17-20)auth_headers(28-29)api_token(24-25)test/api_endpoints/test_mcp_tools_endpoints.py (3)
client(15-17)auth_headers(20-21)api_token(10-11)test/api_endpoints/test_messaging_in_app_endpoints.py (3)
client(25-27)auth_headers(30-31)api_token(20-21)
test/api_endpoints/test_device_endpoints.py (7)
test/api_endpoints/test_dbquery_endpoints.py (1)
test_mac(27-29)test/api_endpoints/test_devices_endpoints.py (1)
test_mac(25-27)test/api_endpoints/test_graphq_endpoints.py (1)
test_mac(24-26)test/api_endpoints/test_history_endpoints.py (1)
test_mac(25-27)test/api_endpoints/test_events_endpoints.py (1)
test_mac(27-29)test/api_endpoints/test_settings_endpoints.py (1)
test_mac(25-27)test/api_endpoints/test_sessions_endpoints.py (1)
test_mac(27-29)
test/api_endpoints/test_mcp_tools_endpoints.py (1)
server/helper.py (1)
get_setting_value(200-257)
server/api_server/spec_generator.py (1)
server/logger.py (1)
mylog(79-84)
server/api_server/schemas.py (2)
server/helper.py (1)
sanitize_string(535-539)front/plugins/plugin_helper.py (1)
is_mac(91-99)
server/api_server/api_server_start.py (8)
server/api_server/mcp_endpoint.py (4)
mcp_sse(857-929)mcp_messages(932-967)openapi_spec(245-267)check_auth(186-195)server/api_server/spec_generator.py (1)
validate_request(56-156)server/api_server/schemas.py (6)
DeviceSearchRequest(112-128)DeviceSearchResponse(146-148)DeviceListRequest(151-159)DeviceListResponse(162-164)DeviceListWrapperResponse(167-169)BaseResponse(91-97)server/helper.py (1)
get_setting_value(200-257)server/messaging/in_app.py (3)
write_notification(24-81)mark_all_notifications_read(135-174)delete_notifications(177-194)server/models/plugin_object_instance.py (1)
getByStatus(69-72)server/api_server/nettools_endpoint.py (1)
traceroute(67-113)server/api_server/sync_endpoint.py (1)
handle_sync_get(12-38)
🪛 Ruff (0.14.11)
test/api_endpoints/test_dbquery_endpoints.py
93-93: Possible SQL injection vector through string-based query construction
(S608)
104-104: Possible SQL injection vector through string-based query construction
(S608)
115-115: Possible SQL injection vector through string-based query construction
(S608)
server/api_server/spec_generator.py
137-137: Do not catch blind exception: Exception
(BLE001)
151-151: Avoid specifying long messages outside the exception class
(TRY003)
291-294: Avoid specifying long messages outside the exception class
(TRY003)
server/api_server/schemas.py
63-63: Avoid specifying long messages outside the exception class
(TRY003)
76-76: Avoid specifying long messages outside the exception class
(TRY003)
82-82: Avoid specifying long messages outside the exception class
(TRY003)
284-284: Avoid specifying long messages outside the exception class
(TRY003)
475-475: Possible binding to all interfaces
(S104)
584-584: Avoid specifying long messages outside the exception class
(TRY003)
609-609: Avoid specifying long messages outside the exception class
(TRY003)
server/api_server/api_server_start.py
5-5: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
62-62: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
64-64: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
65-65: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
291-291: Unused function argument: payload
(ARG001)
317-317: Unused function argument: payload
(ARG001)
337-337: Unused function argument: payload
(ARG001)
357-357: Unused function argument: payload
(ARG001)
377-377: Unused function argument: payload
(ARG001)
393-393: Unused function argument: payload
(ARG001)
421-421: Unused function argument: payload
(ARG001)
455-455: Unused function argument: payload
(ARG001)
478-478: Unused function argument: payload
(ARG001)
508-508: Unused function argument: payload
(ARG001)
523-523: Unused function argument: payload
(ARG001)
543-543: Unused function argument: payload
(ARG001)
557-557: Unused function argument: payload
(ARG001)
585-585: Unused function argument: payload
(ARG001)
614-614: Unused function argument: payload
(ARG001)
642-642: Unused function argument: payload
(ARG001)
685-685: Unused function argument: payload
(ARG001)
721-721: Unused function argument: payload
(ARG001)
742-742: Unused function argument: payload
(ARG001)
763-763: Unused function argument: payload
(ARG001)
786-786: Unused function argument: payload
(ARG001)
819-819: Unused function argument: payload
(ARG001)
837-837: Unused function argument: payload
(ARG001)
852-852: Unused function argument: payload
(ARG001)
876-876: Unused function argument: payload
(ARG001)
899-899: Unused function argument: payload
(ARG001)
912-912: Unused function argument: payload
(ARG001)
928-928: Unused function argument: payload
(ARG001)
1003-1003: Unused function argument: payload
(ARG001)
1023-1023: Unused function argument: payload
(ARG001)
1043-1043: Unused function argument: payload
(ARG001)
1074-1074: Unused function argument: payload
(ARG001)
1101-1101: Unused function argument: payload
(ARG001)
1124-1124: Unused function argument: payload
(ARG001)
1144-1144: Unused function argument: payload
(ARG001)
1183-1183: Unused function argument: payload
(ARG001)
1211-1211: Unused function argument: payload
(ARG001)
1227-1227: Unused function argument: payload
(ARG001)
1248-1248: Unused function argument: payload
(ARG001)
1269-1269: Unused function argument: payload
(ARG001)
1293-1293: Unused function argument: payload
(ARG001)
1308-1308: Unused function argument: payload
(ARG001)
1332-1332: Unused function argument: payload
(ARG001)
1354-1354: Unused function argument: payload
(ARG001)
1386-1386: Unused function argument: payload
(ARG001)
1413-1413: Unused function argument: payload
(ARG001)
1434-1434: Unused function argument: payload
(ARG001)
1455-1455: Unused function argument: payload
(ARG001)
1474-1474: Unused function argument: payload
(ARG001)
1491-1491: Unused function argument: payload
(ARG001)
1509-1509: Unused function argument: payload
(ARG001)
1527-1527: Unused function argument: payload
(ARG001)
1547-1547: Unused function argument: payload
(ARG001)
1560-1560: Unused function argument: payload
(ARG001)
1573-1573: Unused function argument: payload
(ARG001)
1591-1591: Unused function argument: payload
(ARG001)
1614-1614: Unused function argument: payload
(ARG001)
1635-1635: Unused function argument: payload
(ARG001)
1648-1648: Unused function argument: payload
(ARG001)
1664-1664: Unused function argument: payload
(ARG001)
server/api_server/mcp_endpoint.py
96-96: Do not catch blind exception: Exception
(BLE001)
233-233: Do not catch blind exception: Exception
(BLE001)
292-292: Loop control variable path not used within loop body
Rename unused path to _path
(B007)
293-293: Loop control variable method not used within loop body
Rename unused method to _method
(B007)
658-661: Consider moving this statement to an else block
(TRY300)
668-668: Do not catch blind exception: Exception
(BLE001)
671-671: Use explicit conversion flag
Replace with conversion flag
(RUF010)
685-685: Probable insecure usage of temporary file or directory: "/tmp/log"
(S108)
720-720: Probable insecure usage of temporary file or directory: "/tmp/log"
(S108)
738-738: Do not catch blind exception: Exception
(BLE001)
881-881: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: docker_dev
- GitHub Check: docker-tests
🔇 Additional comments (41)
server/db/db_helper.py (1)
35-48: LGTM on the condition logic.The new status mappings are consistent with existing patterns. The distinction between
offline(any device not present in last scan) anddown(devices not present AND have alerts enabled) is a sensible addition that provides more flexible filtering options.test/api_endpoints/test_dbquery_endpoints.py (5)
15-33: LGTM! Test fixtures and helper functions follow established patterns.The fixtures are appropriately scoped (
sessionforapi_tokenandtest_mac, function-scoped forclient), matching patterns from other test files in the codebase.
44-61: Test correctly includesconfirm_dangerous_queryflag for raw SQL execution.The updated test aligns with the
DbQueryRequestschema's safety validator that requires explicit acknowledgment of dangerous operations.
64-74: LGTM! Read test properly validates response structure.Assertions correctly verify
success,resultsfields matching the API response contract.
77-100: LGTM! Update test covers both write and verification read.Good test pattern: perform update, then verify with a read query.
103-121: LGTM! Delete test properly verifies cleanup.The test correctly verifies that the device is removed by checking for an empty results array.
.github/copilot-instructions.md (1)
51-52: LGTM! Documentation correctly clarifies public endpoints exception.This accurately documents that
/openapi.jsonand/docsare intentionally public for API discovery, while actual API endpoints remain protected. This aligns with standard OpenAPI/Swagger best practices.server/api_server/schemas.py (9)
21-24: LGTM! Internal helper imports properly leverage existing codebase utilities.Good adherence to coding guidelines by reusing
sanitize_stringfromserver/helper.pyandnormalize_mac/is_macfromfront/plugins/plugin_helper.py.
56-66: LGTM! MAC validation properly delegates to shared helpers.The validator correctly handles the "Internet" special case and uses
normalize_mac()for consistent MAC formatting per coding guidelines.
68-76: LGTM! IP validation uses stdlib ipaddress for proper IPv4/IPv6 support.Exception chaining with
from errproperly preserves the stack trace for debugging.
151-159: LGTM! DeviceListRequest now includes comprehensive status options.The expanded
Literaltype properly includes"offline","my_devices","network_devices", and"all_devices"to match GraphQL endpoint capabilities.
196-208: LGTM! SetDeviceAliasRequest properly sanitizes input.The
sanitize_aliasfield validator appliessanitize_string()to prevent stored XSS, addressing the previous review concern.
276-285: Good safety guard for mass deletion operations.The model validator ensures that either specific MACs are provided or
confirm_delete_all=Trueis explicitly set, preventing accidental deletion of all devices.
371-386: Good security practice: NMAP modes are whitelisted.The
ALLOWED_NMAP_MODESliteral prevents arbitrary NMAP arguments that could be dangerous, while still allowing legitimate scan types.
567-585: Safety guard for raw SQL execution is appropriate.The
confirm_dangerous_queryrequirement provides an explicit acknowledgment mechanism. Per previous discussion with maintainers, this is intentional for administrative use cases, with MCP tool disablement providing additional control.
588-610: Good parameterized query support with column validation.The
DbQueryUpdateRequestproperly validates column identifiers and enforces columns/values length parity, supporting safer parameterized queries over raw SQL when applicable.test/api_endpoints/test_device_endpoints.py (3)
52-61: LGTM! Test assertions updated to match new wrapped response structure.The assertions correctly validate the
successflag and accessdevMacthrough the nesteddeviceobject, aligning withGetDeviceResponseschema.
95-125: LGTM! Copy device test properly validates wrapped response.Good test pattern: creates source device, copies to target, verifies target exists with correct MAC, and cleans up both devices.
146-154: Good negative test case for non-existent device.The test correctly verifies that updating a non-existent device returns 404 with
success: False, validating the API's error handling.test/api_endpoints/test_mcp_tools_endpoints.py (6)
5-6: LGTM! Cleaner imports directly from modules.The direct imports from
api_server.api_server_startandhelperare cleaner than the previous sys.path manipulation pattern used in some other test files.
168-171: Good defensive access pattern for optional response fields.Using
data.get("links", [])preventsKeyErrorif the topology response contains no links, making the test more robust.
195-196: LGTM! Test verifies expected response structure.Assertions correctly check for
countandeventsfields in the recent alerts response.
251-260: LGTM! 422 status code correctly reflects Pydantic validation error.The change from 400 to 422 aligns with RFC 7231 and Pydantic's convention for validation failures (Unprocessable Entity).
286-291: LGTM! Latest device test validates wrapped response structure.Test correctly verifies
successflag and accesses device fields through the nesteddeviceobject.
423-437: LGTM! Traceroute test correctly validates early validation failure.The test properly verifies that:
- Missing required field returns 422 (validation error)
- The traceroute function is never called when validation fails
This confirms that Pydantic validation prevents invalid requests from reaching business logic.
server/api_server/spec_generator.py (5)
45-50: LGTM! Registry variables and locks are properly structured.The thread-safe registry with separate
_registry_lockand_rebuild_lockaddresses the previously flagged race condition concerns. The duplicate declaration issue from past reviews has been resolved.
159-198: LGTM! GraphQL schema introspection is well-implemented.The function properly bridges GraphQL fields to OpenAPI registry entries with appropriate descriptions and tags.
240-254: LGTM! Thread-safe registry management with proper locking.The
get_tools_statusfunction correctly takes a snapshot of_disabled_toolsbefore iterating, avoiding potential issues from holding the lock too long.
326-346: LGTM! Correct Pydantic v2 schema generation.The use of
model_json_schema(mode="serialization")is the correct Pydantic v2 API for generating OpenAPI-compatible JSON schemas.
534-598: LGTM! Flask app introspection handles edge cases well.The function properly handles wrapped functions, path parameter conversion, and operationId uniqueness. The tag manipulation for
/mcp/paths is a nice touch for organization.server/api_server/mcp_endpoint.py (7)
80-103: LGTM! Robust session cleanup with graceful shutdown.The cleanup thread properly handles the stop event with small sleep increments, allowing for graceful shutdown. The double-checked locking pattern for thread initialization is correctly implemented.
125-136: Good defensive handling of MCP_QUEUE_MAXSIZE setting.The fallback to 1000 on invalid values and the logging for configuration issues is appropriate. Note:
queue.Queue(maxsize=0)creates an unbounded queue, which matches the comment's intent.
207-242: LGTM! OpenAPI spec caching with proper error fallback.The caching logic correctly handles custom servers (bypasses cache) and force refresh. The minimal spec fallback on error prevents complete failures.
292-355: LGTM! Tool mapping correctly extracts OpenAPI operation details.The function properly builds MCP
inputSchemafrom both request bodies and parameters, including handling of nested Pydantic models via$defs. The Ruff B007 warnings about unused loop variables are cosmetic - the iteration structure is semantically correct.
382-554: LGTM! JSON-RPC 2.0 compliant request processing.The method routing is well-organized with correct JSON-RPC error codes (-32601 for method not found, -32602 for invalid params). Notifications correctly return
None.
628-640: LGTM! HTTP requests use explicit 60-second timeouts.The loopback HTTP calls correctly use
timeout=60for all methods, aligning with the coding guideline for explicit timeouts. Authorization header forwarding maintains the auth chain properly.
726-729: Good path traversal protection in resource reading.The
os.path.realpath()comparison correctly prevents directory traversal attacks by ensuring the resolved path stays within the allowed log directory.server/api_server/api_server_start.py (4)
135-160: Past review issue addressed:is_authorizednow fails closed on empty token.The function now correctly checks for empty or short API_TOKEN (lines 140-144) and denies access by default. The exception for
PYTEST_CURRENT_TESTis a reasonable accommodation for the test suite.
275-299: LGTM! Device endpoints follow consistent pattern with proper auth ordering.The
auth_callable=is_authorizedparameter ensures authorization happens before validation, addressing the past review concern. Response format includes required"success"field. The unusedpayloadparameter is intentional for decorator injection.
647-671: Past review issue addressed:api_devices_by_statusnow acceptspayloadparameter.The function signature now includes
payload=None(line 658), preventing the TypeError that would have occurred on POST requests. The fallback logic properly handles both payload and query parameter sources.
943-958: LGTM!trigger_scanhelper with proper validation and mock handling.The function validates scan types against an allowed list and handles test mock edge cases gracefully. The structured response format is consistent with other endpoints.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
adamoutler
force-pushed
the
openapi-mcp-
branch
from
9390050 to
b7da593
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/api_server/api_server_start.py (2)
603-630:/devices/import: unsaferequest.json.get(...)usage; use safe JSON parsing.Even with
request.is_json,request.jsoncan beNone(or raise on malformed JSON), and.get("content")can throw. Userequest.get_json(silent=True) or {}.Proposed diff
def api_import_csv(payload=None): @@ - if request.is_json and request.json.get("content"): - json_content = request.json.get("content") + data = request.get_json(silent=True) if request.is_json else None + if data and data.get("content"): + json_content = data.get("content") else: file_storage = request.files.get("file")
732-751: Adderrorfield to 404 response to comply with API guidelines.The endpoint currently returns
{"success": False, "message": "No devices found"}on 404, but API guidelines require anerrorfield. All other error responses in the codebase use this pattern. While this endpoint intentionally returns a bare list on success (legacy behavior per the existing test), the error response must still follow the standard error shape.Proposed fix
if not favorite: - return jsonify({"success": False, "message": "No devices found"}), 404 + return jsonify({"success": False, "error": "No devices found"}), 404 return jsonify([favorite])
🤖 Fix all issues with AI agents
In `@coderabbit.txt`:
- Around line 1-94: coderabbit.txt is a tracking/notes file that should not be
committed; remove it from the PR and either add the items as GitHub Issues or
move the content to a non-source file (e.g., IMPROVEMENTS.md) in docs if you
intend to keep it. Specifically, delete coderabbit.txt from the branch (or
revert its addition), or replace it with a properly named markdown file checked
into a docs/ or .github/ folder, and if you want to preserve history create
issues for each checklist item and reference them in the new file; ensure the
commit message explains the relocation/removal so reviewers know this was
intentional.
In `@server/api_server/api_server_start.py`:
- Around line 711-730: The 404 response from api_devices_latest is missing the
required "error" field; update the 404 return in the api_devices_latest function
(the route handlers for '/mcp/sse/devices/latest' and '/devices/latest' that
call DeviceInstance.getLatest()) to include an "error" key (e.g. {"success":
False, "message": "No devices found", "error": "No devices found"} or a concise
error string) so the response conforms to the guideline requiring "success":
false responses to include "error".
- Around line 647-672: The handler api_devices_by_status supports a ?status=...
query but validate_request currently doesn't declare it so OpenAPI and
validation miss it; update the validate_request call on api_devices_by_status to
declare the status query parameter (e.g., add a query_params/parameters entry
for "status" with its type/allowed values) or adjust DeviceListRequest to
include a query-scoped "status" field so the decorator generates the OpenAPI
parameter and enforces validation; ensure the parameter name matches "status"
used in the function and that DeviceInstance.getByStatus still receives the
validated value.
In `@server/api_server/mcp_endpoint.py`:
- Around line 688-752: The code currently falls back to a hardcoded "/tmp/log"
via os.getenv("NETALERTX_LOG", "/tmp/log"); update _list_resources and
_read_resource to never hardcode a path: read NETALERTX_LOG strictly from the
environment (os.environ["NETALERTX_LOG"]) or obtain it from the app
config/settings module and raise/return an explicit error if it is unset, and
centralize this logic into a helper (e.g., get_log_dir()) used by both functions
so they consistently fail fast when the env/config is missing instead of using
"/tmp/log".
- Around line 117-147: The create_session function currently allows
MCP_QUEUE_MAXSIZE to be 0 which makes queue.Queue(0) unbounded; clamp the parsed
queue_maxsize so 0 (or any non-positive value) is treated as the safe default
(e.g., 1000) or at least a minimum of 1 before constructing message_queue;
update the logic around get_setting_value('MCP_QUEUE_MAXSIZE') and the
conditional that currently only checks for < 0 to instead handle <= 0 (or
explicitly treat 0 as default), ensuring message_queue =
queue.Queue(maxsize=queue_maxsize) cannot receive 0.
- Around line 186-204: check_auth currently only checks the Authorization header
so MCP handlers (mcp_sse, mcp_messages) reject requests that used ?token=...
like api_server_start.is_authorized allows; update check_auth to also accept a
query-string token by extracting request.args.get("token") (or equivalent) and
treating it as valid when it matches the same expected_token logic (compose
expected_token = f"Bearer {raw_token}" or compare raw tokens consistently with
is_authorized), preserve the existing short-token/pytest guard and logging, and
ensure the same change is applied to the other occurrences noted (lines with
similar logic).
In `@server/api_server/schemas.py`:
- Around line 262-286: DeviceUpdateRequest is missing field validators to
sanitize text inputs; add `@field_validator` methods on DeviceUpdateRequest for
the string fields (devName, devOwner, devType, devVendor, devGroup, devLocation,
devComments and any other Optional[str] fields) that call sanitize_string from
server/helper.py (same pattern as SetDeviceAliasRequest.alias) and return the
sanitized value so inserts/updates from DeviceInstance.setDeviceData() use
cleaned strings; ensure validators accept None and preserve non-string values if
present.
In `@server/api_server/spec_generator.py`:
- Around line 56-160: The decorator validate_request currently just "pass"es
when request.files is present, causing validated_instance to remain None and
handlers that expect a payload (checked via accepts_payload) to receive
payload=None silently; fix by making multipart behavior explicit: add a
decorator flag (e.g., allow_multipart_payload: bool = False or multipart_inject:
bool = False) to validate_request, update the wrapper branch that checks
request.files to either (a) if flag true, construct validated_instance from
request.form/FILES (or inject raw files+form data) so accepts_payload handlers
receive a payload, or (b) if flag false, return a clear error or log and raise
TypeError (or set payload to None but log) so callers must opt-in; modify the
code paths around request.files, validated_instance, and accepts_payload to use
this flag and add a small comment explaining multipart bypass semantics.
- Around line 163-202: introspect_graphql_schema currently registers per-field
GET endpoints via register_tool (creating /graphql/{field_name}) but no Flask
routes exist to serve them, causing 404s; either remove the per-field
registrations from introspect_graphql_schema so only the single POST /graphql
remains, or implement matching Flask route handlers (e.g., a dynamic GET handler
that maps /graphql/<field_name> to executing the corresponding GraphQL query
against the schema) and wire them into the existing Flask app; modify
introspect_graphql_schema, register_tool usage, or the Flask routing setup
accordingly to keep registry and actual routes in sync.
In `@test/api_endpoints/test_mcp_extended_endpoints.py`:
- Around line 467-521: The two tests test_db_update and test_db_delete are
currently indented inside test_db_write which makes them local functions and
prevents pytest discovery; move the `@patch-decorated` definitions of
test_db_update and test_db_delete out to top-level (unindent them to the same
level as test_db_write) so their decorators apply to top-level test functions,
and remove the excessive blank lines between statements to restore normal
formatting.
♻️ Duplicate comments (1)
server/api_server/api_server_start.py (1)
1302-1319: Auth must run before validation on/events/recent(decorator missing auth_callable).
@validate_request(request_model=RecentEventsRequest)runs before the function, butis_authorized()is checked inside the function. This reintroduces the “422 before 403” information disclosure and inconsistent behavior across endpoints.Proposed diff
`@validate_request`( operation_id="get_recent_events", summary="Get Recent Events", description="Get recent events from the system.", - request_model=RecentEventsRequest + request_model=RecentEventsRequest, + auth_callable=is_authorized, + tags=["events"], + response_model=RecentEventsResponse, ) def api_events_default_24h(payload=None): - if not is_authorized(): - return jsonify({"success": False, "message": "ERROR: Not authorized", "error": "Forbidden"}), 403 - hours = 24
🧹 Nitpick comments (3)
server/db/db_helper.py (1)
38-45: Update docstring to document new statuses.The implementation is correct and the new aliases provide good API flexibility for the MCP/OpenAPI endpoints. However, the docstring (lines 22-30) should be updated to include the new statuses:
all_devices,my_devices,network_devices, andoffline.📝 Suggested docstring update
Parameters: device_status (str): The status of the device. Possible values: - 'all' : All active devices - 'my' : Same as 'all' (active devices) + - 'all_devices': Same as 'all' (active devices) + - 'my_devices' : Same as 'all' (active devices) + - 'network_devices': Same as 'all' (active devices) - 'connected' : Devices that are active and present in the last scan - 'favorites' : Devices marked as favorite - 'new' : Devices marked as new - 'down' : Devices not present in the last scan but with alerts + - 'offline' : Devices not present in the last scan - 'archived' : Devices that are archivedserver/api_server/spec_generator.py (2)
330-351: Use Pydantic “validation” schema mode for request bodies (serialization mode can be wrong).
pydantic_to_json_schema(..., mode="serialization")is then used for request bodies too. For OpenAPI, requestBody schemas typically should reflect validation constraints (e.g., required fields) rather than serialization. Consider:
- request models:
mode="validation"- response models:
mode="serialization"(and you already strip validation for tokens)This impacts Swagger UI correctness and MCP input schemas derived from OpenAPI.
Proposed diff
-def pydantic_to_json_schema(model: Type[BaseModel]) -> Dict[str, Any]: +def pydantic_to_json_schema(model: Type[BaseModel], *, mode: Literal["validation", "serialization"] = "validation") -> Dict[str, Any]: @@ - schema = model.model_json_schema(mode="serialization") + schema = model.model_json_schema(mode=mode)def _build_request_body(model: Optional[Type[BaseModel]], definitions: Dict[str, Any]) -> Optional[Dict[str, Any]]: @@ - schema = pydantic_to_json_schema(model) + schema = pydantic_to_json_schema(model, mode="validation")- schema = _strip_validation(pydantic_to_json_schema(response_model)) + schema = _strip_validation(pydantic_to_json_schema(response_model, mode="serialization"))Also applies to: 410-426
528-536: Remove or implement_flatten_defs()stub (avoid placeholder code).
_flatten_defs()is currently a no-oppass. If it’s intentionally unused, consider deleting it (or at least raisingNotImplementedErrorwith a short message) to avoid “placeholder implementation” drift.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.gemini/settings.json.github/copilot-instructions.mdcoderabbit.txtserver/api_server/api_server_start.pyserver/api_server/mcp_endpoint.pyserver/api_server/schemas.pyserver/api_server/spec_generator.pyserver/api_server/swagger.htmlserver/db/db_helper.pytest/api_endpoints/test_dbquery_endpoints.pytest/api_endpoints/test_device_endpoints.pytest/api_endpoints/test_mcp_extended_endpoints.pytest/api_endpoints/test_mcp_tools_endpoints.py
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/copilot-instructions.md
- server/api_server/swagger.html
- .gemini/settings.json
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Always include unit tests or validation steps immediately after writing functions; never leave code unvalidated
Always set explicit timeouts for subprocess calls; default to 60 seconds minimum unless plugin config specifies otherwise
Usenormalize_mac()fromfront/plugins/plugin_helper.pyto normalize MAC addresses before database writes
Usemylog(level, [message])for logging with levels: none/minimal/verbose/debug/trace; usenonefor critical messages that must always appear
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) for all file paths; never hardcode paths like/data/dbor use relative paths
Sanity-check and normalize all text input using sanitizer functions fromserver/helper.pybefore database writes
Files:
test/api_endpoints/test_device_endpoints.pytest/api_endpoints/test_mcp_extended_endpoints.pyserver/api_server/spec_generator.pyserver/db/db_helper.pytest/api_endpoints/test_mcp_tools_endpoints.pytest/api_endpoints/test_dbquery_endpoints.pyserver/api_server/schemas.pyserver/api_server/api_server_start.pyserver/api_server/mcp_endpoint.py
**/*.{py,js,php}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never use placeholders (e.g.,
// ... rest of code) in code implementations; output full, functional blocks to preserve context
Files:
test/api_endpoints/test_device_endpoints.pytest/api_endpoints/test_mcp_extended_endpoints.pyserver/api_server/spec_generator.pyserver/db/db_helper.pytest/api_endpoints/test_mcp_tools_endpoints.pytest/api_endpoints/test_dbquery_endpoints.pyserver/api_server/schemas.pyserver/api_server/api_server_start.pyserver/api_server/mcp_endpoint.py
server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/**/*.py: Never hardcode ports or secrets; useget_setting_value()to retrieve all configuration values from settings
Preferserver/db/db_helper.pyfunctions (e.g.,get_table_json, device condition helpers) over raw SQL in new code paths
Use datetime utilities fromserver/utils/datetime_utils.py(e.g.,timeNowDB()) for all timestamp operations
Files:
server/api_server/spec_generator.pyserver/db/db_helper.pyserver/api_server/schemas.pyserver/api_server/api_server_start.pyserver/api_server/mcp_endpoint.py
server/api_server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/api_server/**/*.py: All API responses must include"success": <true|false>and if false, include an"error"message field
All API routes must requireAuthorization: Bearer <API_TOKEN>header validation viaget_setting_value('API_TOKEN')
Files:
server/api_server/spec_generator.pyserver/api_server/schemas.pyserver/api_server/api_server_start.pyserver/api_server/mcp_endpoint.py
🧠 Learnings (8)
📚 Learning: 2025-10-26T19:36:26.482Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1235
File: server/api_server/nettools_endpoint.py:13-34
Timestamp: 2025-10-26T19:36:26.482Z
Learning: In server/api_server/nettools_endpoint.py, the use of print() for module-level initialization warnings is acceptable and should be reviewed by the primary maintainer. The logger.mylog guideline may be specific to plugin code rather than core server code.
Applied to files:
server/api_server/spec_generator.py
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to **/*.py : Use `mylog(level, [message])` for logging with levels: none/minimal/verbose/debug/trace; use `none` for critical messages that must always appear
Applied to files:
server/api_server/spec_generator.py
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to server/**/*.py : Prefer `server/db/db_helper.py` functions (e.g., `get_table_json`, device condition helpers) over raw SQL in new code paths
Applied to files:
server/db/db_helper.py
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to **/*.py : Use environment variables (`NETALERTX_DB`, `NETALERTX_LOG`, etc.) for all file paths; never hardcode paths like `/data/db` or use relative paths
Applied to files:
test/api_endpoints/test_mcp_tools_endpoints.py
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to **/*.py : Use `normalize_mac()` from `front/plugins/plugin_helper.py` to normalize MAC addresses before database writes
Applied to files:
server/api_server/schemas.py
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to **/*.py : Sanity-check and normalize all text input using sanitizer functions from `server/helper.py` before database writes
Applied to files:
server/api_server/schemas.py
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to server/api_server/**/*.py : All API routes must require `Authorization: Bearer <API_TOKEN>` header validation via `get_setting_value('API_TOKEN')`
Applied to files:
server/api_server/api_server_start.pyserver/api_server/mcp_endpoint.py
📚 Learning: 2026-01-10T04:32:10.567Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-10T04:32:10.567Z
Learning: Applies to server/api_server/**/*.py : All API responses must include `"success": <true|false>` and if false, include an `"error"` message field
Applied to files:
server/api_server/mcp_endpoint.py
🧬 Code graph analysis (5)
test/api_endpoints/test_mcp_extended_endpoints.py (4)
test/api_endpoints/test_dbquery_endpoints.py (3)
client(21-23)api_token(16-17)auth_headers(32-33)test/api_endpoints/test_mcp_tools_endpoints.py (3)
client(15-17)api_token(10-11)auth_headers(20-21)test/api_endpoints/test_devices_endpoints.py (3)
client(19-21)api_token(14-15)auth_headers(30-31)test/test_wol_validation.py (1)
auth_headers(38-42)
test/api_endpoints/test_dbquery_endpoints.py (1)
test/api_endpoints/test_mcp_tools_endpoints.py (3)
client(15-17)auth_headers(20-21)api_token(10-11)
server/api_server/schemas.py (2)
server/helper.py (1)
sanitize_string(535-539)front/plugins/plugin_helper.py (1)
is_mac(91-99)
server/api_server/api_server_start.py (2)
server/api_server/mcp_endpoint.py (1)
check_auth(186-203)server/api_server/spec_generator.py (1)
validate_request(56-160)
server/api_server/mcp_endpoint.py (2)
server/helper.py (1)
get_setting_value(200-257)server/api_server/spec_generator.py (3)
generate_openapi_spec(604-728)get_registry(324-327)is_tool_disabled(232-235)
🪛 Ruff (0.14.11)
server/api_server/spec_generator.py
141-141: Do not catch blind exception: Exception
(BLE001)
155-155: Avoid specifying long messages outside the exception class
(TRY003)
295-298: Avoid specifying long messages outside the exception class
(TRY003)
test/api_endpoints/test_dbquery_endpoints.py
93-93: Possible SQL injection vector through string-based query construction
(S608)
104-104: Possible SQL injection vector through string-based query construction
(S608)
115-115: Possible SQL injection vector through string-based query construction
(S608)
server/api_server/schemas.py
63-63: Avoid specifying long messages outside the exception class
(TRY003)
76-76: Avoid specifying long messages outside the exception class
(TRY003)
82-82: Avoid specifying long messages outside the exception class
(TRY003)
284-284: Avoid specifying long messages outside the exception class
(TRY003)
475-475: Possible binding to all interfaces
(S104)
584-584: Avoid specifying long messages outside the exception class
(TRY003)
609-609: Avoid specifying long messages outside the exception class
(TRY003)
server/api_server/api_server_start.py
5-5: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
62-62: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
64-64: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
65-65: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
291-291: Unused function argument: payload
(ARG001)
317-317: Unused function argument: payload
(ARG001)
337-337: Unused function argument: payload
(ARG001)
357-357: Unused function argument: payload
(ARG001)
377-377: Unused function argument: payload
(ARG001)
393-393: Unused function argument: payload
(ARG001)
421-421: Unused function argument: payload
(ARG001)
455-455: Unused function argument: payload
(ARG001)
478-478: Unused function argument: payload
(ARG001)
508-508: Unused function argument: payload
(ARG001)
523-523: Unused function argument: payload
(ARG001)
543-543: Unused function argument: payload
(ARG001)
557-557: Unused function argument: payload
(ARG001)
585-585: Unused function argument: payload
(ARG001)
614-614: Unused function argument: payload
(ARG001)
642-642: Unused function argument: payload
(ARG001)
685-685: Unused function argument: payload
(ARG001)
721-721: Unused function argument: payload
(ARG001)
742-742: Unused function argument: payload
(ARG001)
763-763: Unused function argument: payload
(ARG001)
786-786: Unused function argument: payload
(ARG001)
819-819: Unused function argument: payload
(ARG001)
837-837: Unused function argument: payload
(ARG001)
852-852: Unused function argument: payload
(ARG001)
876-876: Unused function argument: payload
(ARG001)
899-899: Unused function argument: payload
(ARG001)
912-912: Unused function argument: payload
(ARG001)
928-928: Unused function argument: payload
(ARG001)
1003-1003: Unused function argument: payload
(ARG001)
1023-1023: Unused function argument: payload
(ARG001)
1043-1043: Unused function argument: payload
(ARG001)
1074-1074: Unused function argument: payload
(ARG001)
1101-1101: Unused function argument: payload
(ARG001)
1124-1124: Unused function argument: payload
(ARG001)
1144-1144: Unused function argument: payload
(ARG001)
1183-1183: Unused function argument: payload
(ARG001)
1211-1211: Unused function argument: payload
(ARG001)
1227-1227: Unused function argument: payload
(ARG001)
1248-1248: Unused function argument: payload
(ARG001)
1269-1269: Unused function argument: payload
(ARG001)
1293-1293: Unused function argument: payload
(ARG001)
1308-1308: Unused function argument: payload
(ARG001)
1332-1332: Unused function argument: payload
(ARG001)
1354-1354: Unused function argument: payload
(ARG001)
1386-1386: Unused function argument: payload
(ARG001)
1413-1413: Unused function argument: payload
(ARG001)
1434-1434: Unused function argument: payload
(ARG001)
1455-1455: Unused function argument: payload
(ARG001)
1474-1474: Unused function argument: payload
(ARG001)
1491-1491: Unused function argument: payload
(ARG001)
1509-1509: Unused function argument: payload
(ARG001)
1527-1527: Unused function argument: payload
(ARG001)
1547-1547: Unused function argument: payload
(ARG001)
1560-1560: Unused function argument: payload
(ARG001)
1573-1573: Unused function argument: payload
(ARG001)
1591-1591: Unused function argument: payload
(ARG001)
1614-1614: Unused function argument: payload
(ARG001)
1635-1635: Unused function argument: payload
(ARG001)
1648-1648: Unused function argument: payload
(ARG001)
1664-1664: Unused function argument: payload
(ARG001)
server/api_server/mcp_endpoint.py
96-96: Do not catch blind exception: Exception
(BLE001)
241-241: Do not catch blind exception: Exception
(BLE001)
300-300: Loop control variable path not used within loop body
Rename unused path to _path
(B007)
301-301: Loop control variable method not used within loop body
Rename unused method to _method
(B007)
666-669: Consider moving this statement to an else block
(TRY300)
676-676: Do not catch blind exception: Exception
(BLE001)
679-679: Use explicit conversion flag
Replace with conversion flag
(RUF010)
693-693: Probable insecure usage of temporary file or directory: "/tmp/log"
(S108)
728-728: Probable insecure usage of temporary file or directory: "/tmp/log"
(S108)
746-746: Do not catch blind exception: Exception
(BLE001)
889-889: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: docker_dev
- GitHub Check: docker-tests
🔇 Additional comments (19)
test/api_endpoints/test_device_endpoints.py (2)
60-61: LGTM!The assertions correctly validate the new API response structure with a top-level
successflag and nesteddeviceobject containingdevMac. This aligns with the updated API response shape mentioned in the PR objectives.
120-121: LGTM!Consistent with the changes in
test_get_device, the assertions properly validate the new wrapped response structure for the copied device verification.test/api_endpoints/test_dbquery_endpoints.py (4)
52-56: LGTM!The addition of
confirm_dangerous_query: Truealigns with the new validation requirements for DB query endpoints. This explicit confirmation flag provides an additional safety check for dangerous operations.
66-70: LGTM!Consistent addition of the
confirm_dangerous_queryflag for the read operation.
83-98: LGTM!The
confirm_dangerous_queryflag is correctly added to both the UPDATE and verification SELECT queries.Regarding the static analysis SQL injection warning (S608): This is a false positive in test code since
test_macis a pytest fixture generating random test data internally, not external user input.
105-120: LGTM!The
confirm_dangerous_queryflag is correctly added to both the DELETE and verification SELECT queries. The static analysis warnings for SQL injection are false positives as noted above.test/api_endpoints/test_mcp_tools_endpoints.py (7)
168-171: LGTM!Using
data.get("links", [])instead of direct indexing is more robust and preventsKeyErrorif the response doesn't includelinks. Good defensive programming.
195-196: LGTM!The test now validates that the response includes the
countandeventsfields, aligning with the updated API response structure.
258-258: LGTM!The status code change from 400 to 422 is correct. HTTP 422 (Unprocessable Entity) is the standard response for validation errors when using Pydantic, which aligns with the PR's validation implementation.
271-271: Minor: Comment could be more descriptive.The comment
# API uses getLatest() which calls _fetchoneprovides useful context about the internal implementation.
288-291: LGTM!The test correctly validates the new wrapped response structure with a top-level
successflag and nesteddeviceobject, consistent with the changes in other test files.
433-433: LGTM!The test correctly expects:
- HTTP 422 for validation failure (missing required
devLastIPfield)- The mock
traceroutefunction should not be called since validation fails before reaching the handler logicThis validates the
@validate_requestdecorator properly rejects invalid payloads early.Also applies to: 437-438
5-6: Add missingsys.pathconfiguration to enable imports.The test file is missing the path configuration that all other test files in
test/api_endpoints/include. Add the following at the top of the file after the existing imports:import sys import os INSTALL_PATH = os.getenv("NETALERTX_APP", "/app") sys.path.extend([f"{INSTALL_PATH}/front/plugins", f"{INSTALL_PATH}/server"])Without this, the imports
from api_server.api_server_start import appandfrom helper import get_setting_valuewill fail with ModuleNotFoundError.⛔ Skipped due to learnings
Learnt from: CR Repo: jokob-sk/NetAlertX PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2026-01-10T04:32:10.567Z Learning: Applies to server/**/*.py : Never hardcode ports or secrets; use `get_setting_value()` to retrieve all configuration values from settingstest/api_endpoints/test_mcp_extended_endpoints.py (4)
16-20: LGTM!The test fixture correctly sets up the Flask test client with
TESTINGmode enabled.
36-48: LGTM!The test correctly mocks
setDeviceDataand validates the endpoint behavior for device updates.
451-465: LGTM!The
test_db_readandtest_db_writetests correctly include theconfirm_dangerous_query: Truefield, consistent with the other DB query test file.
122-135: Test expectations match actual API behavior.The endpoint
api_devices_favorite()explicitly wraps thegetFavorite()result withjsonify([favorite]), intentionally creating the double-nested list structure[[{...}]]the test expects. The "legacy: wrapped in a list" comment accurately documents this behavior.server/api_server/schemas.py (1)
56-77: MAC/IP validators look good (normalization + exception chaining).
validate_mac()usingnormalize_mac()andvalidate_ip()usingipaddresswithraise ... from erris the right direction.server/api_server/api_server_start.py (1)
964-988: No changes needed. The/openapi.jsonand/docspublic endpoints are already explicitly documented as intentional exceptions in.github/copilot-instructions.mdunder "API/Endpoints quick map" with the note: "Exception:/openapi.jsonand/docsare intentionally public to enable documentation discovery; actual API endpoints remain protected." The spec also properly declares BearerAuth, as expected.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
adamoutler
force-pushed
the
openapi-mcp-
branch
from
649e0dd to
cdc18b0
Compare
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 (1)
server/api_server/api_server_start.py (1)
271-279: Includesuccesson the 404 response.
api_get_devicereturns{"error": ...}withoutsuccess:false. Align with the API response rule. As per coding guidelines, error responses must include"success": falseand"error".🐛 Proposed fix
- if device_data is None: - return jsonify({"error": "Device not found"}), 404 + if device_data is None: + return jsonify({ + "success": False, + "error": "Device not found", + "message": "Device not found" + }), 404
🤖 Fix all issues with AI agents
In `@server/api_server/api_server_start.py`:
- Around line 103-109: The CORS setup uses a wildcard origin with
supports_credentials=True which is invalid; update the CORS call so that origins
is set to a configured allowlist (use get_setting_value("CORS_ORIGINS")) and
keep supports_credentials=True, or if you must keep a wildcard origin set
supports_credentials=False; modify the CORS(...) invocation (the CORS
constructor call) to use either get_setting_value("CORS_ORIGINS") for origins or
set supports_credentials to False while preserving the existing allow_headers
and methods values.
In `@server/api_server/spec_generator.py`:
- Around line 1-50: The spec_generator.py file exceeds the 500-line limit and
must be split into smaller modules: extract the thread-safe registry, locking
and registration logic (move _registry, _registry_lock, _rebuild_lock,
_operation_ids, _disabled_tools and functions like register_tool,
deregister_tool, list_tools) into a new registry.py; move Pydantic/GraphQL
request/response validation and error handling (ValidationError handling,
validate_request, parse_response, any decorators/wrappers that touch
request/response conversion) into validation.py; and move OpenAPI assembly and
JSON Schema conversion logic (generate_openapi_spec, any helper builders or
schema collectors, and the code that converts Pydantic models to JSON Schema)
into openapi_builder.py; ensure shared symbols are imported from these new
modules in spec_generator.py (or expose a small facade module) and preserve
threading locks and logger usages (mylog) so behavior is identical; update
imports and any tests to use the new module paths.
♻️ Duplicate comments (7)
server/api_server/api_server_start.py (6)
708-731: Adderrorfield to “No devices found” 404s.
api_devices_latestandapi_devices_favoritereturn 404s without anerrorfield. As per coding guidelines,"success": falsemust include"error".🐛 Proposed fix
- if not latest: - return jsonify({"success": False, "message": "No devices found"}), 404 + if not latest: + return jsonify({"success": False, "message": "No devices found", "error": "No devices found"}), 404 @@ - if not favorite: - return jsonify({"success": False, "message": "No devices found"}), 404 + if not favorite: + return jsonify({"success": False, "message": "No devices found", "error": "No devices found"}), 404
1672-1675: Avoiddebug=Truein production; derive from settings.
debug=Trueexposes Werkzeug debugger and is unsafe. Useget_setting_value("FLASK_DEBUG")and coerce to boolean. As per coding guidelines, config should come from settings.🔒 Proposed fix
thread = threading.Thread( target=lambda: app.run( - host="0.0.0.0", port=graphql_port, debug=True, use_reloader=False + host="0.0.0.0", + port=graphql_port, + debug=str(get_setting_value("FLASK_DEBUG")).lower() in ("1", "true", "yes"), + use_reloader=False ) )
119-140: Fail closed whenAPI_TOKENis unset.If
API_TOKENis empty,"" == ""authorizes every request. Add an explicit check to refuse all requests when the token is missing. As per coding guidelines, all routes must enforce Authorization.🐛 Proposed fix
def is_authorized(): @@ - expected_token = get_setting_value('API_TOKEN') + expected_token = get_setting_value('API_TOKEN') + if not expected_token: + mylog("minimal", ["[api] API_TOKEN is not configured; refusing all requests."]) + return False
800-802: Guard againstNonefromrequest.get_json(silent=True).
request.get_json(silent=True)can returnNone, so.get(...)can raiseAttributeErrorwhenpayloadis absent.🐛 Proposed fix
def api_traceroute(payload: TracerouteRequest = None): - ip = payload.devLastIP if payload else request.get_json(silent=True).get("devLastIP") + if payload: + ip = payload.devLastIP + else: + data = request.get_json(silent=True) or {} + ip = data.get("devLastIP") return traceroute(ip)
1286-1295: Move auth intovalidate_requestfor/events/recent.Manual auth inside the handler means validation may run before authorization. Use
auth_callable=is_authorizedin the decorator and remove the inline check. As per coding guidelines, auth should be enforced consistently at the route layer.🐛 Proposed fix
`@validate_request`( operation_id="get_recent_events", summary="Get Recent Events", description="Get recent events from the system.", - request_model=RecentEventsRequest + request_model=RecentEventsRequest, + auth_callable=is_authorized ) def api_events_default_24h(payload=None): - if not is_authorized(): - return jsonify({"success": False, "message": "ERROR: Not authorized", "error": "Forbidden"}), 403 - hours = 24
944-967: Confirm/openapi.jsonand/docsauth exception is documented or enforce auth.These endpoints are public, but the guideline requires Bearer auth for API routes. Either add auth checks or document the explicit exception in the coding guidelines to avoid policy drift. As per coding guidelines, all API routes should require Authorization.
server/api_server/spec_generator.py (1)
107-116: Propagateallow_multipart_payloadinto the registry and requestBody.
allow_multipart_payloadis not stored in_openapi_metadata/registry and_build_request_bodyalways emitsapplication/json, so/docswon’t showmultipart/form-datafor endpoints like/devices/import. This regresses OpenAPI accuracy.♻️ Proposed fix
- f._openapi_metadata = { + f._openapi_metadata = { "operation_id": operation_id, "summary": summary, "description": description, "request_model": request_model, "response_model": response_model, "tags": tags, "path_params": path_params, - "query_params": query_params + "query_params": query_params, + "allow_multipart_payload": allow_multipart_payload }def register_tool( path: str, method: Literal["GET", "POST", "PUT", "DELETE", "PATCH"], operation_id: str, summary: str, description: str, request_model: Optional[Type[BaseModel]] = None, response_model: Optional[Type[BaseModel]] = None, path_params: Optional[List[Dict[str, Any]]] = None, query_params: Optional[List[Dict[str, Any]]] = None, tags: Optional[List[str]] = None, deprecated: bool = False, - original_operation_id: Optional[str] = None + original_operation_id: Optional[str] = None, + allow_multipart_payload: bool = False ) -> None: @@ _registry.append({ "path": path, "method": method.upper(), "operation_id": operation_id, "original_operation_id": original_operation_id, "summary": summary, "description": description, "request_model": request_model, "response_model": response_model, "path_params": path_params or [], "query_params": query_params or [], "tags": tags or ["default"], - "deprecated": deprecated + "deprecated": deprecated, + "allow_multipart_payload": allow_multipart_payload })register_tool( path=path, method=method, operation_id=unique_op_id, original_operation_id=original_op_id if unique_op_id != original_op_id else None, summary=metadata["summary"], description=metadata["description"], request_model=metadata.get("request_model"), response_model=metadata.get("response_model"), path_params=metadata.get("path_params"), query_params=metadata.get("query_params"), - tags=tags + tags=tags, + allow_multipart_payload=metadata.get("allow_multipart_payload", False) )-def _build_request_body(model: Optional[Type[BaseModel]], definitions: Dict[str, Any]) -> Optional[Dict[str, Any]]: +def _build_request_body( + model: Optional[Type[BaseModel]], + definitions: Dict[str, Any], + allow_multipart_payload: bool = False +) -> Optional[Dict[str, Any]]: @@ - return { + content = { + "application/json": { + "schema": schema + } + } + if allow_multipart_payload: + content["multipart/form-data"] = { + "schema": schema + } + return { "required": True, - "content": { - "application/json": { - "schema": schema - } - } + "content": content }- request_body = _build_request_body(entry["request_model"], definitions) + request_body = _build_request_body( + entry["request_model"], + definitions, + entry.get("allow_multipart_payload", False) + )Also applies to: 295-350, 447-462, 628-640, 735-738
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/api_server/api_server_start.pyserver/api_server/spec_generator.pyserver/api_server/sse_endpoint.pyserver/db/db_helper.pytest/ui/test_ui_maintenance.py
💤 Files with no reviewable changes (1)
- test/ui/test_ui_maintenance.py
🚧 Files skipped from review as they are similar to previous changes (2)
- server/api_server/sse_endpoint.py
- server/db/db_helper.py
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{py,js,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{py,js,ts,tsx}: Write a test case or validation step immediately after writing a function to verify correctness
Never use placeholders such as// ... rest of codeor ellipsis; output full, functional blocks to preserve context
Files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.py
server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/**/*.py: Usemylog(level, [message])for logging with levels: none/minimal/verbose/debug/trace; do NOT useerroras level
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) for file paths; never hardcode/data/dbor use relative paths
Useserver/db/db_helper.pyfunctions (e.g.,get_table_json, device condition helpers) instead of raw SQL for database operations
When adding new functionality, implement it in existing handlers (e.g.,DeviceInstanceinserver/models/device_instance.py) or create a new one if appropriate; do not access the database from other application layers
Validate MACs before database writes usingnormalize_mac()fromfront/plugins/plugin_helper.py
Files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.py
**/*.{py,php}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{py,php}: Never hardcode ports or secrets; useget_setting_value()to retrieve configuration values
Code files must not exceed 500 lines of code
Files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.py
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Always set explicit timeouts on subprocess calls; default to 60s minimum unless plugin config specifies otherwise
Files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.py
server/api_server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/api_server/**/*.py: All API responses must return"success": <true|false>; iffalse, include an"error"field with the error message
Use Flask app routes inserver/api_server/api_server_start.pyfor API endpoints and ensure all routes expectAuthorization: Bearer <API_TOKEN>header viaget_setting_value('API_TOKEN')
Files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.py
**/*.{py,php,js,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow the DRY (Don't Repeat Yourself) principle; reuse existing methods where possible or refactor to use a common method called multiple times
Files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.py
🧠 Learnings (6)
📚 Learning: 2026-01-15T08:45:01.464Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-15T08:45:01.464Z
Learning: Applies to server/**/*.py : Use `mylog(level, [message])` for logging with levels: none/minimal/verbose/debug/trace; do NOT use `error` as level
Applied to files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.py
📚 Learning: 2025-10-26T19:36:26.482Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1235
File: server/api_server/nettools_endpoint.py:13-34
Timestamp: 2025-10-26T19:36:26.482Z
Learning: In server/api_server/nettools_endpoint.py, the use of print() for module-level initialization warnings is acceptable and should be reviewed by the primary maintainer. The logger.mylog guideline may be specific to plugin code rather than core server code.
Applied to files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.py
📚 Learning: 2026-01-15T08:45:01.465Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-15T08:45:01.465Z
Learning: Applies to server/api_server/**/*.py : Use Flask app routes in `server/api_server/api_server_start.py` for API endpoints and ensure all routes expect `Authorization: Bearer <API_TOKEN>` header via `get_setting_value('API_TOKEN')`
Applied to files:
server/api_server/api_server_start.py
📚 Learning: 2026-01-15T08:45:01.464Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-15T08:45:01.464Z
Learning: Applies to server/api_server/**/*.py : All API responses must return `"success": <true|false>`; if `false`, include an `"error"` field with the error message
Applied to files:
server/api_server/api_server_start.py
📚 Learning: 2026-01-15T08:45:01.464Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-15T08:45:01.464Z
Learning: Applies to server/**/*.py : Use environment variables (`NETALERTX_DB`, `NETALERTX_LOG`, etc.) for file paths; never hardcode `/data/db` or use relative paths
Applied to files:
server/api_server/api_server_start.py
📚 Learning: 2026-01-15T08:45:01.464Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-15T08:45:01.464Z
Learning: Applies to **/*.{py,php} : Never hardcode ports or secrets; use `get_setting_value()` to retrieve configuration values
Applied to files:
server/api_server/api_server_start.py
🧬 Code graph analysis (1)
server/api_server/spec_generator.py (1)
server/logger.py (1)
mylog(86-91)
🪛 Ruff (0.14.11)
server/api_server/spec_generator.py
140-140: Do not catch blind exception: Exception
(BLE001)
170-170: Do not catch blind exception: Exception
(BLE001)
186-186: Do not catch blind exception: Exception
(BLE001)
196-196: Avoid specifying long messages outside the exception class
(TRY003)
331-334: Avoid specifying long messages outside the exception class
(TRY003)
server/api_server/api_server_start.py
5-5: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
62-62: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
64-64: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
65-65: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
271-271: Unused function argument: payload
(ARG001)
297-297: Unused function argument: payload
(ARG001)
317-317: Unused function argument: payload
(ARG001)
337-337: Unused function argument: payload
(ARG001)
357-357: Unused function argument: payload
(ARG001)
373-373: Unused function argument: payload
(ARG001)
401-401: Unused function argument: payload
(ARG001)
435-435: Unused function argument: payload
(ARG001)
458-458: Unused function argument: payload
(ARG001)
488-488: Unused function argument: payload
(ARG001)
503-503: Unused function argument: payload
(ARG001)
523-523: Unused function argument: payload
(ARG001)
537-537: Unused function argument: payload
(ARG001)
565-565: Unused function argument: payload
(ARG001)
595-595: Unused function argument: payload
(ARG001)
623-623: Unused function argument: payload
(ARG001)
666-666: Unused function argument: payload
(ARG001)
702-702: Unused function argument: payload
(ARG001)
723-723: Unused function argument: payload
(ARG001)
744-744: Unused function argument: payload
(ARG001)
767-767: Unused function argument: payload
(ARG001)
814-814: Unused function argument: payload
(ARG001)
873-873: Unused function argument: payload
(ARG001)
886-886: Unused function argument: payload
(ARG001)
902-902: Unused function argument: payload
(ARG001)
983-983: Unused function argument: payload
(ARG001)
1003-1003: Unused function argument: payload
(ARG001)
1023-1023: Unused function argument: payload
(ARG001)
1054-1054: Unused function argument: payload
(ARG001)
1081-1081: Unused function argument: payload
(ARG001)
1104-1104: Unused function argument: payload
(ARG001)
1124-1124: Unused function argument: payload
(ARG001)
1163-1163: Unused function argument: payload
(ARG001)
1191-1191: Unused function argument: payload
(ARG001)
1207-1207: Unused function argument: payload
(ARG001)
1228-1228: Unused function argument: payload
(ARG001)
1253-1253: Unused function argument: payload
(ARG001)
1277-1277: Unused function argument: payload
(ARG001)
1292-1292: Unused function argument: payload
(ARG001)
1316-1316: Unused function argument: payload
(ARG001)
1338-1338: Unused function argument: payload
(ARG001)
1370-1370: Unused function argument: payload
(ARG001)
1397-1397: Unused function argument: payload
(ARG001)
1418-1418: Unused function argument: payload
(ARG001)
1439-1439: Unused function argument: payload
(ARG001)
1458-1458: Unused function argument: payload
(ARG001)
1475-1475: Unused function argument: payload
(ARG001)
1493-1493: Unused function argument: payload
(ARG001)
1511-1511: Unused function argument: payload
(ARG001)
1531-1531: Unused function argument: payload
(ARG001)
1544-1544: Unused function argument: payload
(ARG001)
1557-1557: Unused function argument: payload
(ARG001)
1575-1575: Unused function argument: payload
(ARG001)
1598-1598: Unused function argument: payload
(ARG001)
1619-1619: Unused function argument: payload
(ARG001)
1632-1632: Unused function argument: payload
(ARG001)
1648-1648: Unused function argument: payload
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
🔇 Additional comments (1)
server/api_server/spec_generator.py (1)
232-293: Disablement helpers look solid.Thread-safe snapshots and original-operationId checks are clear and consistent.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/db/db_helper.py (1)
22-30: Update docstring to includeofflinestatus.The docstring lists supported statuses but omits the newly added
offlinemapping, which can mislead callers. Addofflineto the documented list or clarify its semantics.Also applies to: 35-43
server/api_server/api_server_start.py (1)
1-20: Split api_server_start.py to respect the 500‑line limit.This module is ~1.7k lines and should be decomposed by domain (devices, events, sessions, nettools, etc.) to meet the size constraint and improve maintainability.
As per coding guidelines, code files must not exceed 500 lines.
🤖 Fix all issues with AI agents
In `@server/api_server/api_server_start.orig.py`:
- Around line 1-20: Delete the legacy unused module api_server_start.orig.py
from the repository; it duplicates the real entrypoint and only contains unused
imports and symbols (e.g., Flask imports, DeviceInstance, updateState,
devicesSchema, delete_online_history, get_metric_stats) so remove the file and
run a repo-wide search to confirm no remaining references, then run tests/lint
to ensure nothing else depended on it.
In `@server/api_server/api_server_start.py`:
- Around line 1227-1235: The failure branch in api_get_events omits the required
"error" field; update the except block in function api_get_events to include an
"error" key in the JSON response (e.g., set "error": str(e) or a structured
error object) alongside "success": False and "message", and keep returning the
500 status; locate the except handling for (ValueError, RuntimeError) in
api_get_events and modify the jsonify payload accordingly.
In `@server/api_server/spec_generator.py`:
- Around line 133-179: The DELETE method is not being validated or added to the
OpenAPI requestBody when request_model is provided; update the validation branch
that checks request.method in ["POST","PUT","PATCH"] to also include "DELETE" so
DELETE requests follow the same multipart/json validation flow
(variables/functions to touch: request_model, allow_multipart_payload,
operation_id, validated_instance, _handle_validation_error,
validation_error_code, and the multipart handling block), and mirror this change
in the OpenAPI spec generation code that currently only handles
POST/PUT/PATCH/GET (the block around where requestBody is emitted) so DELETE
endpoints emit a requestBody when request_model is present.
♻️ Duplicate comments (7)
server/api_server/api_server_start.py (6)
701-709: Add requirederrorfield on 404 responses.The 404 paths return
success=Falsewithout anerrorfield, which violates the response contract.As per coding guidelines, `"success": false` responses must include an `"error"` field.🔧 Suggested fix
def api_devices_latest(payload=None): @@ if not latest: - return jsonify({"success": False, "message": "No devices found"}), 404 + return jsonify({"success": False, "message": "No devices found", "error": "No devices found"}), 404 @@ def api_devices_favorite(payload=None): @@ if not favorite: - return jsonify({"success": False, "message": "No devices found"}), 404 + return jsonify({"success": False, "message": "No devices found", "error": "No devices found"}), 404Also applies to: 726-730
1664-1674: Avoid hard‑codeddebug=Truewhen starting Flask.Running with debug enabled is unsafe in production. Use a settings-driven flag.
🔧 Suggested fix
def start_server(graphql_port, app_state): """Start the GraphQL server in a background thread.""" @@ - thread = threading.Thread( - target=lambda: app.run( - host="0.0.0.0", port=graphql_port, debug=True, use_reloader=False - ) - ) + debug_mode = get_setting_value("FLASK_DEBUG") in (True, "True", "true", 1, "1") + thread = threading.Thread( + target=lambda: app.run( + host="0.0.0.0", port=graphql_port, debug=debug_mode, use_reloader=False + ) + )
102-108: CORS wildcard + credentials is invalid and weakens protections.
origins="*"withsupports_credentials=Trueis rejected by browsers/Flask-CORS and can create a false sense of security. Use an explicit allowlist (e.g.,CORS_ORIGINS) or disable credentials when using a wildcard.🔧 Suggested fix
-CORS( - app, - resources={r"/*": {"origins": "*"}}, - supports_credentials=True, - allow_headers=["Authorization", "Content-Type", "Accept", "Origin", "X-Requested-With"], - methods=["GET", "POST", "PUT", "DELETE", "OPTIONS"] -) +origins = get_setting_value("CORS_ORIGINS") or "*" +supports_creds = False if origins == "*" else True +CORS( + app, + resources={r"/*": {"origins": origins}}, + supports_credentials=supports_creds, + allow_headers=["Authorization", "Content-Type", "Accept", "Origin", "X-Requested-With"], + methods=["GET", "POST", "PUT", "DELETE", "OPTIONS"] +)Flask-CORS supports_credentials wildcard origins behavior
118-139: Fail closed when API_TOKEN is unset.
get_setting_value('API_TOKEN')returns an empty string when missing, so empty header/query tokens will authorize requests. Add an explicit guard to deny access when the token is unset.🔧 Suggested fix
def is_authorized(): @@ - expected_token = get_setting_value('API_TOKEN') + expected_token = get_setting_value('API_TOKEN') + if not expected_token: + msg = "[api] API_TOKEN is not configured; refusing all requests." + write_notification(msg, "alert") + mylog("verbose", [msg]) + return False
799-801: Guard againstNonefromrequest.get_json()in traceroute fallback.
request.get_json(silent=True)can returnNone; calling.get(...)on it will raiseAttributeError.🔧 Suggested fix
def api_traceroute(payload: TracerouteRequest = None): - ip = payload.devLastIP if payload else request.get_json(silent=True).get("devLastIP") + if payload: + ip = payload.devLastIP + else: + json_data = request.get_json(silent=True) or {} + ip = json_data.get("devLastIP") return traceroute(ip)
1285-1294: Make auth consistent by usingauth_callableon the decorator.This endpoint still performs manual auth inside the handler, so validation can run before auth. Move auth to the decorator for consistency with other routes.
🔧 Suggested fix
`@validate_request`( operation_id="get_recent_events", summary="Get Recent Events", description="Get recent events from the system.", - request_model=RecentEventsRequest + request_model=RecentEventsRequest, + auth_callable=is_authorized ) def api_events_default_24h(payload=None): - if not is_authorized(): - return jsonify({"success": False, "message": "ERROR: Not authorized", "error": "Forbidden"}), 403 - hours = 24server/api_server/spec_generator.py (1)
1-50: Split this module to meet the 500‑line cap.This file is well over the 500‑line limit and should be broken into smaller modules (e.g., registry.py, validation.py, openapi_builder.py).
As per coding guidelines, code files must not exceed 500 lines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/api_server/api_server_start.orig.pyserver/api_server/api_server_start.pyserver/api_server/spec_generator.pyserver/db/db_helper.py
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{py,js,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{py,js,ts,tsx}: Write a test case or validation step immediately after writing a function to verify correctness
Never use placeholders such as// ... rest of codeor ellipsis; output full, functional blocks to preserve context
Files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.pyserver/db/db_helper.pyserver/api_server/api_server_start.orig.py
server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/**/*.py: Usemylog(level, [message])for logging with levels: none/minimal/verbose/debug/trace; do NOT useerroras level
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) for file paths; never hardcode/data/dbor use relative paths
Useserver/db/db_helper.pyfunctions (e.g.,get_table_json, device condition helpers) instead of raw SQL for database operations
When adding new functionality, implement it in existing handlers (e.g.,DeviceInstanceinserver/models/device_instance.py) or create a new one if appropriate; do not access the database from other application layers
Validate MACs before database writes usingnormalize_mac()fromfront/plugins/plugin_helper.py
Files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.pyserver/db/db_helper.pyserver/api_server/api_server_start.orig.py
**/*.{py,php}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{py,php}: Never hardcode ports or secrets; useget_setting_value()to retrieve configuration values
Code files must not exceed 500 lines of code
Files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.pyserver/db/db_helper.pyserver/api_server/api_server_start.orig.py
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Always set explicit timeouts on subprocess calls; default to 60s minimum unless plugin config specifies otherwise
Files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.pyserver/db/db_helper.pyserver/api_server/api_server_start.orig.py
server/api_server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/api_server/**/*.py: All API responses must return"success": <true|false>; iffalse, include an"error"field with the error message
Use Flask app routes inserver/api_server/api_server_start.pyfor API endpoints and ensure all routes expectAuthorization: Bearer <API_TOKEN>header viaget_setting_value('API_TOKEN')
Files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.pyserver/api_server/api_server_start.orig.py
**/*.{py,php,js,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow the DRY (Don't Repeat Yourself) principle; reuse existing methods where possible or refactor to use a common method called multiple times
Files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.pyserver/db/db_helper.pyserver/api_server/api_server_start.orig.py
🧠 Learnings (7)
📚 Learning: 2026-01-15T08:45:01.464Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-15T08:45:01.464Z
Learning: Applies to server/**/*.py : Use `mylog(level, [message])` for logging with levels: none/minimal/verbose/debug/trace; do NOT use `error` as level
Applied to files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.py
📚 Learning: 2025-10-26T19:36:26.482Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1235
File: server/api_server/nettools_endpoint.py:13-34
Timestamp: 2025-10-26T19:36:26.482Z
Learning: In server/api_server/nettools_endpoint.py, the use of print() for module-level initialization warnings is acceptable and should be reviewed by the primary maintainer. The logger.mylog guideline may be specific to plugin code rather than core server code.
Applied to files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.pyserver/api_server/api_server_start.orig.py
📚 Learning: 2026-01-15T08:45:01.465Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-15T08:45:01.465Z
Learning: Applies to server/api_server/**/*.py : Use Flask app routes in `server/api_server/api_server_start.py` for API endpoints and ensure all routes expect `Authorization: Bearer <API_TOKEN>` header via `get_setting_value('API_TOKEN')`
Applied to files:
server/api_server/api_server_start.pyserver/api_server/api_server_start.orig.py
📚 Learning: 2026-01-15T08:45:01.464Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-15T08:45:01.464Z
Learning: Applies to server/api_server/**/*.py : All API responses must return `"success": <true|false>`; if `false`, include an `"error"` field with the error message
Applied to files:
server/api_server/api_server_start.py
📚 Learning: 2026-01-15T08:45:01.464Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-15T08:45:01.464Z
Learning: Applies to server/**/*.py : Use environment variables (`NETALERTX_DB`, `NETALERTX_LOG`, etc.) for file paths; never hardcode `/data/db` or use relative paths
Applied to files:
server/api_server/api_server_start.py
📚 Learning: 2026-01-15T08:45:01.464Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-15T08:45:01.464Z
Learning: Applies to **/*.{py,php} : Never hardcode ports or secrets; use `get_setting_value()` to retrieve configuration values
Applied to files:
server/api_server/api_server_start.py
📚 Learning: 2026-01-15T08:45:01.464Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-15T08:45:01.464Z
Learning: Applies to server/**/*.py : Use `server/db/db_helper.py` functions (e.g., `get_table_json`, device condition helpers) instead of raw SQL for database operations
Applied to files:
server/db/db_helper.py
🧬 Code graph analysis (2)
server/api_server/spec_generator.py (1)
server/logger.py (1)
mylog(86-91)
server/api_server/api_server_start.py (3)
server/api_server/api_server_start.orig.py (1)
is_authorized(1090-1107)server/helper.py (1)
get_setting_value(200-257)server/models/device_instance.py (1)
DeviceInstance(16-703)
🪛 ast-grep (0.40.5)
server/api_server/api_server_start.orig.py
[warning] 1121-1123: Running flask app with host 0.0.0.0 could expose the server publicly.
Context: app.run(
host="0.0.0.0", port=graphql_port, debug=True, use_reloader=False
)
Note: [CWE-668]: Exposure of Resource to Wrong Sphere [OWASP A01:2021]: Broken Access Control [REFERENCES]
https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid_app_run_with_bad_host-python)
[warning] 1121-1123: Detected Flask app with debug=True. Do not deploy to production with this flag enabled as it will leak sensitive information. Instead, consider using Flask configuration variables or setting 'debug' using system environment variables.
Context: app.run(
host="0.0.0.0", port=graphql_port, debug=True, use_reloader=False
)
Note: [CWE-489] Active Debug Code. [REFERENCES]
- https://labs.detectify.com/2015/10/02/how-patreon-got-hacked-publicly-exposed-werkzeug-debugger/
(debug-enabled-python)
🪛 Ruff (0.14.11)
server/api_server/spec_generator.py
142-142: Do not catch blind exception: Exception
(BLE001)
172-172: Do not catch blind exception: Exception
(BLE001)
188-188: Do not catch blind exception: Exception
(BLE001)
198-198: Avoid specifying long messages outside the exception class
(TRY003)
335-338: Avoid specifying long messages outside the exception class
(TRY003)
server/api_server/api_server_start.py
5-5: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
62-62: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
64-64: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
65-65: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
270-270: Unused function argument: payload
(ARG001)
296-296: Unused function argument: payload
(ARG001)
316-316: Unused function argument: payload
(ARG001)
336-336: Unused function argument: payload
(ARG001)
356-356: Unused function argument: payload
(ARG001)
372-372: Unused function argument: payload
(ARG001)
400-400: Unused function argument: payload
(ARG001)
434-434: Unused function argument: payload
(ARG001)
457-457: Unused function argument: payload
(ARG001)
487-487: Unused function argument: payload
(ARG001)
502-502: Unused function argument: payload
(ARG001)
522-522: Unused function argument: payload
(ARG001)
536-536: Unused function argument: payload
(ARG001)
564-564: Unused function argument: payload
(ARG001)
594-594: Unused function argument: payload
(ARG001)
622-622: Unused function argument: payload
(ARG001)
665-665: Unused function argument: payload
(ARG001)
701-701: Unused function argument: payload
(ARG001)
722-722: Unused function argument: payload
(ARG001)
743-743: Unused function argument: payload
(ARG001)
766-766: Unused function argument: payload
(ARG001)
813-813: Unused function argument: payload
(ARG001)
872-872: Unused function argument: payload
(ARG001)
885-885: Unused function argument: payload
(ARG001)
901-901: Unused function argument: payload
(ARG001)
982-982: Unused function argument: payload
(ARG001)
1002-1002: Unused function argument: payload
(ARG001)
1022-1022: Unused function argument: payload
(ARG001)
1053-1053: Unused function argument: payload
(ARG001)
1080-1080: Unused function argument: payload
(ARG001)
1103-1103: Unused function argument: payload
(ARG001)
1123-1123: Unused function argument: payload
(ARG001)
1162-1162: Unused function argument: payload
(ARG001)
1190-1190: Unused function argument: payload
(ARG001)
1206-1206: Unused function argument: payload
(ARG001)
1227-1227: Unused function argument: payload
(ARG001)
1252-1252: Unused function argument: payload
(ARG001)
1276-1276: Unused function argument: payload
(ARG001)
1291-1291: Unused function argument: payload
(ARG001)
1315-1315: Unused function argument: payload
(ARG001)
1337-1337: Unused function argument: payload
(ARG001)
1369-1369: Unused function argument: payload
(ARG001)
1396-1396: Unused function argument: payload
(ARG001)
1417-1417: Unused function argument: payload
(ARG001)
1438-1438: Unused function argument: payload
(ARG001)
1457-1457: Unused function argument: payload
(ARG001)
1474-1474: Unused function argument: payload
(ARG001)
1492-1492: Unused function argument: payload
(ARG001)
1510-1510: Unused function argument: payload
(ARG001)
1530-1530: Unused function argument: payload
(ARG001)
1543-1543: Unused function argument: payload
(ARG001)
1556-1556: Unused function argument: payload
(ARG001)
1574-1574: Unused function argument: payload
(ARG001)
1597-1597: Unused function argument: payload
(ARG001)
1618-1618: Unused function argument: payload
(ARG001)
1631-1631: Unused function argument: payload
(ARG001)
1647-1647: Unused function argument: payload
(ARG001)
server/api_server/api_server_start.orig.py
6-6: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
13-13: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
14-14: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
15-15: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
16-16: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
18-18: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
19-19: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
20-20: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
21-21: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
29-29: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
38-38: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
39-39: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
40-40: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
41-41: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
43-43: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
45-45: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
48-48: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
56-56: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
61-61: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
134-134: Unused function argument: error
(ARG001)
781-781: Possible binding to all interfaces
(S104)
885-885: Do not catch blind exception: Exception
(BLE001)
1123-1123: Possible binding to all interfaces
(S104)
1123-1123: Use of debug=True in Flask app detected
(S201)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: docker-tests
- GitHub Check: docker_dev
🔇 Additional comments (1)
server/db/db_helper.py (1)
165-168: Header formatting looks good.This change cleanly fixes the header alignment using
.format().
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…hql_endpoint Restores the 'my_devices', 'network_devices', and 'all_devices' status filtering logic that was removed. This ensures the 'UI_MY_DEVICES' setting is respected and related device filtering works as expected in the GraphQL endpoint. Also restores 'get_setting_value' import.
…eference Reverts the 'offline' status filter to only check for 'devPresentLastScan == 0', removing the 'devIsArchived == 0' check. This aligns with the developer's instruction to only change descriptions while keeping existing logic.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/api_server/api_server_start.py (3)
271-279: 404 response is missing the required success/error envelope.
api_get_device()returns only{"error": ...}on miss. The contract requires"success": falseand an"error"field.🔧 Proposed fix
if device_data is None: - return jsonify({"error": "Device not found"}), 404 + return jsonify({ + "success": False, + "message": "Device not found", + "error": "Device not found" + }), 404As per coding guidelines, failures must include
"success": falseand"error".
723-731: Adderrorfield and fix nested list wrapping.Per API guidelines, failures with
success: falsemust include an"error"field. Additionally,getFavorite()returns a list (via_fetchall()), so wrapping it in brackets creates an incorrect nested structure.🔧 Proposed fixes
if not favorite: - return jsonify({"success": False, "message": "No devices found"}), 404 - return jsonify([favorite]) + return jsonify({"success": False, "error": "No devices found"}), 404 + return jsonify(favorite)
1665-1676: Do not hardcodedebug=Truefor the background server.Running Flask with
debug=Truecan expose the Werkzeug debugger. Useget_setting_value('FLASK_DEBUG')instead, which is already available in the codebase.🔧 Proposed fix
def start_server(graphql_port, app_state): """Start the GraphQL server in a background thread.""" + debug_mode = get_setting_value("FLASK_DEBUG") if app_state.graphQLServerStarted == 0: mylog("verbose", [f"[graphql endpoint] Starting on port: {graphql_port}"]) # Start Flask app in a separate thread thread = threading.Thread( target=lambda: app.run( - host="0.0.0.0", port=graphql_port, debug=True, use_reloader=False + host="0.0.0.0", port=graphql_port, debug=debug_mode, use_reloader=False ) )
♻️ Duplicate comments (4)
server/api_server/api_server_start.py (4)
706-710: Adderrorfield to/devices/latest404.The 404 payload is missing the required
"error"field.🔧 Proposed fix
if not latest: - return jsonify({"success": False, "message": "No devices found"}), 404 + return jsonify({"success": False, "message": "No devices found", "error": "No devices found"}), 404As per coding guidelines, failures must include an
"error"field.
800-802: Guard againstNonefromrequest.get_json()in traceroute.
request.get_json(silent=True)can returnNone, causingAttributeErroron.get(...).🔧 Proposed fix
def api_traceroute(payload: TracerouteRequest = None): - ip = payload.devLastIP if payload else request.get_json(silent=True).get("devLastIP") + if payload: + ip = payload.devLastIP + else: + data = request.get_json(silent=True) or {} + ip = data.get("devLastIP") return traceroute(ip)
1286-1295: Move auth enforcement into the decorator to avoid 422-before-403.
api_events_default_24hvalidates before authorization. This can leak schema details to unauthenticated callers. Let@validate_requestrunis_authorized()first.🔧 Proposed fix
`@validate_request`( operation_id="get_recent_events", summary="Get Recent Events", description="Get recent events from the system.", - request_model=RecentEventsRequest + request_model=RecentEventsRequest, + auth_callable=is_authorized ) def api_events_default_24h(payload=None): - if not is_authorized(): - return jsonify({"success": False, "message": "ERROR: Not authorized", "error": "Forbidden"}), 403 - hours = 24Based on learnings, auth should be enforced via the decorator’s
auth_callable.
119-140: Fail closed whenAPI_TOKENis empty.If
API_TOKENis unset, empty tokens will match and grant access. Add an explicit check to refuse all requests when the token is missing.🔒 Proposed fix
def is_authorized(): # Allow OPTIONS requests (preflight) without auth if request.method == "OPTIONS": return True expected_token = get_setting_value('API_TOKEN') + if not expected_token: + mylog("verbose", ["[api] API_TOKEN is not configured; refusing all requests."]) + return FalseBased on learnings, API_TOKEN must be enforced for all protected routes.
🧹 Nitpick comments (1)
server/api_server/spec_generator.py (1)
160-178: Remove the unreachable duplicateValidationErrorhandler.The second
except ValidationErrorblock is never hit because the same exception is already caught above it.♻️ Proposed fix
except ValidationError as e: return _handle_validation_error(e, operation_id, validation_error_code) except BadRequest as e: mylog("verbose", [f"[Validation] Invalid JSON for {operation_id}: {e}"]) return jsonify({ "success": False, "error": "Invalid JSON", "message": "Request body must be valid JSON" }), 400 - except ValidationError as e: - mylog("verbose", [f"[Validation] Malformed request for {operation_id}: {e}"]) - return jsonify({ - "success": False, - "error": "Invalid Request", - "message": "Unable to process request body" - }), 400
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/api_server/api_server_start.pyserver/api_server/graphql_endpoint.pyserver/api_server/spec_generator.py
🚧 Files skipped from review as they are similar to previous changes (1)
- server/api_server/graphql_endpoint.py
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{py,js,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{py,js,ts,tsx}: Write a test case or validation step immediately after writing a function to verify correctness
Never use placeholders such as// ... rest of codeor ellipsis; output full, functional blocks to preserve context
Files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.py
server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/**/*.py: Usemylog(level, [message])for logging with levels: none/minimal/verbose/debug/trace; do NOT useerroras level
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) for file paths; never hardcode/data/dbor use relative paths
Useserver/db/db_helper.pyfunctions (e.g.,get_table_json, device condition helpers) instead of raw SQL for database operations
When adding new functionality, implement it in existing handlers (e.g.,DeviceInstanceinserver/models/device_instance.py) or create a new one if appropriate; do not access the database from other application layers
Validate MACs before database writes usingnormalize_mac()fromfront/plugins/plugin_helper.py
Files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.py
**/*.{py,php}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{py,php}: Never hardcode ports or secrets; useget_setting_value()to retrieve configuration values
Code files must not exceed 500 lines of code
Files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.py
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Always set explicit timeouts on subprocess calls; default to 60s minimum unless plugin config specifies otherwise
Files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.py
server/api_server/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
server/api_server/**/*.py: All API responses must return"success": <true|false>; iffalse, include an"error"field with the error message
Use Flask app routes inserver/api_server/api_server_start.pyfor API endpoints and ensure all routes expectAuthorization: Bearer <API_TOKEN>header viaget_setting_value('API_TOKEN')
Files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.py
**/*.{py,php,js,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow the DRY (Don't Repeat Yourself) principle; reuse existing methods where possible or refactor to use a common method called multiple times
Files:
server/api_server/spec_generator.pyserver/api_server/api_server_start.py
🧠 Learnings (6)
📚 Learning: 2026-01-15T08:45:01.464Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-15T08:45:01.464Z
Learning: Applies to server/**/*.py : Use `mylog(level, [message])` for logging with levels: none/minimal/verbose/debug/trace; do NOT use `error` as level
Applied to files:
server/api_server/spec_generator.py
📚 Learning: 2025-10-26T19:36:26.482Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1235
File: server/api_server/nettools_endpoint.py:13-34
Timestamp: 2025-10-26T19:36:26.482Z
Learning: In server/api_server/nettools_endpoint.py, the use of print() for module-level initialization warnings is acceptable and should be reviewed by the primary maintainer. The logger.mylog guideline may be specific to plugin code rather than core server code.
Applied to files:
server/api_server/spec_generator.py
📚 Learning: 2026-01-15T08:45:01.465Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-15T08:45:01.465Z
Learning: Applies to server/api_server/**/*.py : Use Flask app routes in `server/api_server/api_server_start.py` for API endpoints and ensure all routes expect `Authorization: Bearer <API_TOKEN>` header via `get_setting_value('API_TOKEN')`
Applied to files:
server/api_server/api_server_start.py
📚 Learning: 2026-01-15T08:45:01.464Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-15T08:45:01.464Z
Learning: Applies to server/api_server/**/*.py : All API responses must return `"success": <true|false>`; if `false`, include an `"error"` field with the error message
Applied to files:
server/api_server/api_server_start.py
📚 Learning: 2026-01-15T08:45:01.464Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-15T08:45:01.464Z
Learning: Applies to server/**/*.py : Use environment variables (`NETALERTX_DB`, `NETALERTX_LOG`, etc.) for file paths; never hardcode `/data/db` or use relative paths
Applied to files:
server/api_server/api_server_start.py
📚 Learning: 2026-01-15T08:45:01.464Z
Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-15T08:45:01.464Z
Learning: Applies to **/*.{py,php} : Never hardcode ports or secrets; use `get_setting_value()` to retrieve configuration values
Applied to files:
server/api_server/api_server_start.py
🧬 Code graph analysis (2)
server/api_server/spec_generator.py (1)
server/logger.py (1)
mylog(86-91)
server/api_server/api_server_start.py (5)
server/api_server/mcp_endpoint.py (1)
check_auth(186-208)server/api_server/spec_generator.py (1)
validate_request(77-203)server/helper.py (1)
get_setting_value(200-257)server/messaging/in_app.py (2)
write_notification(24-81)get_unread_notifications(197-211)server/models/user_events_queue_instance.py (2)
UserEventsQueueInstance(9-116)add_event(81-116)
🪛 Ruff (0.14.11)
server/api_server/spec_generator.py
172-172: try-except block with duplicate exception ValidationError
(B025)
188-188: Do not catch blind exception: Exception
(BLE001)
198-198: Avoid specifying long messages outside the exception class
(TRY003)
335-338: Avoid specifying long messages outside the exception class
(TRY003)
server/api_server/api_server_start.py
6-6: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
63-63: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
65-65: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
66-66: Unused noqa directive (non-enabled: E402)
Remove unused noqa directive
(RUF100)
271-271: Unused function argument: payload
(ARG001)
297-297: Unused function argument: payload
(ARG001)
317-317: Unused function argument: payload
(ARG001)
337-337: Unused function argument: payload
(ARG001)
357-357: Unused function argument: payload
(ARG001)
373-373: Unused function argument: payload
(ARG001)
401-401: Unused function argument: payload
(ARG001)
435-435: Unused function argument: payload
(ARG001)
458-458: Unused function argument: payload
(ARG001)
488-488: Unused function argument: payload
(ARG001)
503-503: Unused function argument: payload
(ARG001)
523-523: Unused function argument: payload
(ARG001)
537-537: Unused function argument: payload
(ARG001)
565-565: Unused function argument: payload
(ARG001)
595-595: Unused function argument: payload
(ARG001)
623-623: Unused function argument: payload
(ARG001)
666-666: Unused function argument: payload
(ARG001)
702-702: Unused function argument: payload
(ARG001)
723-723: Unused function argument: payload
(ARG001)
744-744: Unused function argument: payload
(ARG001)
767-767: Unused function argument: payload
(ARG001)
814-814: Unused function argument: payload
(ARG001)
873-873: Unused function argument: payload
(ARG001)
886-886: Unused function argument: payload
(ARG001)
902-902: Unused function argument: payload
(ARG001)
983-983: Unused function argument: payload
(ARG001)
1003-1003: Unused function argument: payload
(ARG001)
1023-1023: Unused function argument: payload
(ARG001)
1054-1054: Unused function argument: payload
(ARG001)
1081-1081: Unused function argument: payload
(ARG001)
1104-1104: Unused function argument: payload
(ARG001)
1124-1124: Unused function argument: payload
(ARG001)
1163-1163: Unused function argument: payload
(ARG001)
1191-1191: Unused function argument: payload
(ARG001)
1207-1207: Unused function argument: payload
(ARG001)
1228-1228: Unused function argument: payload
(ARG001)
1253-1253: Unused function argument: payload
(ARG001)
1277-1277: Unused function argument: payload
(ARG001)
1292-1292: Unused function argument: payload
(ARG001)
1316-1316: Unused function argument: payload
(ARG001)
1338-1338: Unused function argument: payload
(ARG001)
1370-1370: Unused function argument: payload
(ARG001)
1397-1397: Unused function argument: payload
(ARG001)
1418-1418: Unused function argument: payload
(ARG001)
1439-1439: Unused function argument: payload
(ARG001)
1458-1458: Unused function argument: payload
(ARG001)
1475-1475: Unused function argument: payload
(ARG001)
1493-1493: Unused function argument: payload
(ARG001)
1511-1511: Unused function argument: payload
(ARG001)
1531-1531: Unused function argument: payload
(ARG001)
1544-1544: Unused function argument: payload
(ARG001)
1557-1557: Unused function argument: payload
(ARG001)
1575-1575: Unused function argument: payload
(ARG001)
1598-1598: Unused function argument: payload
(ARG001)
1619-1619: Unused function argument: payload
(ARG001)
1632-1632: Unused function argument: payload
(ARG001)
1648-1648: Unused function argument: payload
(ARG001)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/api_server/api_server_start.py (2)
280-283: Missing"success": falsewrapper in 404 response.When device is not found, the response returns
{"error": "Device not found"}without the required"success": falsefield per coding guidelines.🐛 Proposed fix
if device_data is None: - return jsonify({"error": "Device not found"}), 404 + return jsonify({"success": False, "error": "Device not found", "message": "Device not found"}), 404As per coding guidelines, all API responses must return
"success": <true|false>; iffalse, include an"error"field.
732-735: Missing"error"field in 404 response for/devices/favorite.The 404 response includes
"success": Falsebut omits the required"error"field per coding guidelines.🐛 Proposed fix
if not favorite: - return jsonify({"success": False, "message": "No devices found"}), 404 + return jsonify({"success": False, "message": "No devices found", "error": "No devices found"}), 404 return jsonify([favorite])
♻️ Duplicate comments (1)
server/api_server/api_server_start.py (1)
1679-1681: Security:debug=Truehardcoded in production server start.The Flask server is started with
debug=Truehardcoded, which exposes the Werkzeug debugger in production. This was flagged in a previous review and appears to still be present. The debugger can allow remote code execution if accessible.🔒 Proposed fix
thread = threading.Thread( target=lambda: app.run( - host="0.0.0.0", port=graphql_port, debug=True, use_reloader=False + host="0.0.0.0", port=graphql_port, debug=False, use_reloader=False ) )Or use a configuration setting:
debug_mode = str(get_setting_value("FLASK_DEBUG")).lower() in ("true", "1", "yes")
🧹 Nitpick comments (3)
server/api_server/spec_generator.py (2)
186-190: Silentpassafter GET query param validation exception may mask errors.When validation fails for reasons other than
ValidationError(line 188-190), the exception is logged but then silently ignored withpass. This could hide unexpected issues like type conversion failures or malformed query strings, causing the endpoint to proceed withvalidated_instance = None.Consider returning an error response instead of silently continuing:
💡 Suggested fix
except ValidationError as e: return _handle_validation_error(e, operation_id, validation_error_code) except Exception as e: mylog("verbose", [f"[Validation] Query param validation failed for {operation_id}: {e}"]) - pass + return jsonify({ + "success": False, + "error": "Invalid query parameters", + "message": str(e) + }), 400
172-178: BroadExceptioncatch is acceptable here but could be narrowed.The
except Exceptionat line 172 catches all exceptions during JSON parsing/validation. While this provides a safety net, it could mask unexpected errors. If you want to be more precise, consider catching(TypeError, KeyError, AttributeError)alongside the explicitValidationErrorandBadRequestalready handled.This is a minor refinement—the current implementation is functional and provides a reasonable fallback response.
server/api_server/api_server_start.py (1)
1354-1359: BroadExceptioncatch without specific error handling.The
except Exception as exat line 1358 catches all exceptions and returns a 500 error. While this prevents crashes, it could mask specific issues that should be handled differently. Consider catching more specific exceptions or at minimum logging the exception type.💡 Suggested improvement
except Exception as ex: - return jsonify({"success": False, "error": str(ex)}), 500 + mylog("verbose", [f"[api_events_recent] Unexpected error: {type(ex).__name__}: {ex}"]) + return jsonify({"success": False, "error": "Internal server error", "message": str(ex)}), 500
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/api_server/api_server_start.py (1)
1646-1662: Dead code: Theelsebranch can never execute.The route decorator at line 1646 only allows
methods=["GET"], sorequest.methodwill always be"GET"when this function executes. Theelsebranch (lines 1658-1662) is unreachable dead code.🐛 Proposed fix
`@validate_request`( operation_id="check_auth", summary="Check Authentication", description="Check if the current API token is valid.", response_model=BaseResponse, tags=["auth"], auth_callable=is_authorized ) def check_auth(payload=None): - if request.method == "GET": - return jsonify({"success": True, "message": "Authentication check successful"}), 200 - else: - msg = "[sync endpoint] Method Not Allowed" - write_notification(msg, "alert") - mylog("verbose", [msg]) - return jsonify({"success": False, "message": "ERROR: No allowed", "error": "Method Not Allowed"}), 405 + return jsonify({"success": True, "message": "Authentication check successful"}), 200
🧹 Nitpick comments (2)
server/api_server/spec_generator.py (2)
186-194: Consider narrowing the blindExceptioncatch for better error visibility.The bare
Exceptioncatch at line 188 could mask unexpected infrastructure errors (e.g., database failures, memory issues). While returning 400 for query param issues is reasonable, catching specific exceptions would improve debugging.♻️ Suggested refinement
except ValidationError as e: return _handle_validation_error(e, operation_id, validation_error_code) - except Exception as e: + except (TypeError, ValueError, KeyError) as e: mylog("verbose", [f"[Validation] Query param validation failed for {operation_id}: {e}"]) return jsonify({ "success": False, "error": "Invalid query parameters", "message": str(e) }), 400
585-592: Empty stub function could be removed.
_flatten_defsis an empty function marked as "experimental" that does nothing. Consider removing it until the functionality is implemented to reduce code clutter.
2 participants
set_tool_disabled(operation_id, disabled=True): Used to mark a specific endpoint (by its operationId) as disabled or enabled.is_tool_disabled(operation_id): Checks the current status of an endpoint.get_disabled_tools(): Returns a list of all currently disabled operationIds.get_tools_status(): Returns a complete list of all registered tools along with their summary and disabled status. This is intended for relaying information to the frontend.Note: this PR is extremely extensive due to mid-PR requirements change. I had broken it up to migrating docs into code, OpenAPI and MCP server, and then merging the OpenAPI directly into legacy API code, along with modernization. However, it was decided this must be done at the same time. This lead to significant CodeRabbit reviews on legacy code which was not intended to be part of the original PR for this reason. This PR was originally intended to be an augmentation, but turned into API modernization and on-the-fly adaptation to coderabbit issues. Please note this was not intended.
Phase2:
Summary by CodeRabbit
Release Notes
New Features
/openapi.json) and interactive Swagger UI documentation (/docs) now available for API discovery.Bug Fixes
Configuration
✏️ Tip: You can customize this high-level summary in your review settings.