Add router restart capability by kshk123 · Pull Request #9 · kshk123/monitoring

kshk123 · 2026-01-02T15:43:16Z

No description provided.

Copilot

Pull request overview

This pull request adds router restart capability to the internet speed monitoring service. The change introduces automatic FRITZ!Box router restart functionality when internet speeds fall below configured thresholds.

Key Changes

Renamed SpeedTest.py to speed_test.py and refactored for package structure
Added RouterRestartManager for automatic router restart based on speed thresholds
Added PrometheusManager for managing Prometheus server lifecycle
Introduced YAML-based configuration system replacing command-line arguments
Added comprehensive test coverage for new functionality

Reviewed changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 18 comments.

Show a summary per file

File	Description
network_speed/uv.lock	Added dependencies: certifi, charset-normalizer, fritzconnection, idna, pyyaml, requests, urllib3
network_speed/src/speed_test.py	New main application with YAML config support, router restart integration, and WSGI compatibility
network_speed/src/router_restart.py	New router management module with FritzBox integration and metrics tracking
network_speed/src/prometheus_manager.py	New Prometheus lifecycle management module
network_speed/src/init.py	Package marker for proper Python package structure
network_speed/tests/*.py	Comprehensive test suite for all new functionality
network_speed/config.yaml	New YAML configuration file replacing CLI arguments
network_speed/pyproject.toml	Updated entry point from SpeedTest to speed_test, added new dependencies
network_speed/README.md	Updated documentation reflecting new configuration system and features

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-02T15:46:16Z

network_speed/tests/test_router_restart.py

+            manager1 = RouterRestartManager(config)
+            manager2 = RouterRestartManager(config)
+            manager3 = RouterRestartManager(config)


Variable manager1 is not used.

Suggested change

manager1 = RouterRestartManager(config)

manager2 = RouterRestartManager(config)

manager3 = RouterRestartManager(config)

for _ in range(3):

RouterRestartManager(config)

Copilot · 2026-01-02T15:46:17Z

network_speed/tests/test_router_restart.py

+            manager1 = RouterRestartManager(config)
+            manager2 = RouterRestartManager(config)
+            manager3 = RouterRestartManager(config)
+
+            # Count file handlers after
+            final_file_handlers = [


Variable manager2 is not used.

Suggested change

manager1 = RouterRestartManager(config)

manager2 = RouterRestartManager(config)

manager3 = RouterRestartManager(config)

# Count file handlers after

final_file_handlers = [

for _ in range(3):

RouterRestartManager(config)

# Count file handlers after

final_file_handlers = [

final_file_handlers = [

Copilot · 2026-01-02T15:46:17Z

network_speed/tests/test_router_restart.py

+            manager1 = RouterRestartManager(config)
+            manager2 = RouterRestartManager(config)
+            manager3 = RouterRestartManager(config)


Variable manager3 is not used.

Suggested change

manager1 = RouterRestartManager(config)

manager2 = RouterRestartManager(config)

manager3 = RouterRestartManager(config)

for _ in range(3):

RouterRestartManager(config)

Copilot · 2026-01-02T15:46:17Z

network_speed/tests/test_router_restart.py

+                "logging": {"enabled": False}
+            })
+
+            manager = RouterRestartManager(config)


Variable manager is not used.

Suggested change

manager = RouterRestartManager(config)

RouterRestartManager(config)

Copilot · 2026-01-02T15:46:18Z

network_speed/tests/test_speedtest.py

+                        # Metric line: name{labels} value or name value
+                        parts = line.split(' ')
+                        self.assertEqual(len(parts), 2, f"Invalid metric line: {line}")
+                        metric_name_with_labels = parts[0]


Variable metric_name_with_labels is not used.

Suggested change

metric_name_with_labels = parts[0]

Copilot · 2026-01-02T15:46:20Z

network_speed/tests/test_prometheus_manager.py

+"""Tests for PrometheusManager startup/shutdown behavior."""
+
+import unittest
+from unittest.mock import Mock, patch, MagicMock


Import of 'MagicMock' is not used.

Suggested change

from unittest.mock import Mock, patch, MagicMock

from unittest.mock import Mock, patch

Copilot · 2026-01-02T15:46:20Z

network_speed/tests/test_router_restart.py

+"""Tests for RouterRestartManager including config resolution, state handling, and time windows."""
+
+import unittest
+from unittest.mock import Mock, patch, MagicMock, mock_open


Import of 'MagicMock' is not used.
Import of 'mock_open' is not used.

Suggested change

from unittest.mock import Mock, patch, MagicMock, mock_open

from unittest.mock import Mock, patch

Copilot · 2026-01-02T15:46:21Z

network_speed/tests/test_wsgi_mode.py

+"""Tests for WSGI mode where main() is never called."""
+
+import unittest
+from unittest.mock import Mock, patch, MagicMock


Import of 'MagicMock' is not used.

Suggested change

from unittest.mock import Mock, patch, MagicMock

from unittest.mock import Mock, patch

Copilot · 2026-01-02T15:46:21Z

network_speed/src/prometheus_manager.py

+                logging.error(f"Error stopping Prometheus: {e}")
+            try:
+                self._process.kill()
+            except:


Except block directly handles BaseException.

Suggested change

except:

except Exception:

Copilot · 2026-01-02T15:46:21Z

network_speed/src/prometheus_manager.py

+                logging.error(f"Error stopping Prometheus: {e}")
+            try:
+                self._process.kill()
+            except:


'except' clause does nothing but pass and there is no explanatory comment.

Suggested change

except:

except Exception:

# Intentionally ignore errors while forcefully terminating the process during cleanup.

Copilot

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-02T21:33:57Z

network_speed/src/router_restart.py

+    def _get_fritzbox_password(self):
+        """Retrieve FRITZ!Box password from 1Password."""
+        try:
+            result = subprocess.run(
+                ["op", "read", self.onepassword_ref],
+                capture_output=True,
+                text=True,
+                check=True
+            )
+            return result.stdout.strip()
+        except subprocess.CalledProcessError as e:
+            logging.error(f"Failed to retrieve password from 1Password: {e}")
+            raise


Password retrieval from 1Password using subprocess without input validation could be a security risk. Consider adding validation for the onepassword_ref format to prevent command injection, even though the op CLI should handle this safely.

Copilot · 2026-01-02T21:33:57Z

network_speed/src/router_restart.py

+    def _restart_fritzbox(self):
+        """Restart the FRITZ!Box router."""
+        try:
+            password = self._get_fritzbox_password()
+
+            logging.info(f"Connecting to FRITZ!Box at {self.fritzbox_ip}...")
+            fc = FritzConnection(
+                address=self.fritzbox_ip,
+                user=self.fritzbox_username if self.fritzbox_username else None,
+                password=password
+            )
+
+            logging.info("Sending reboot command to FRITZ!Box...")
+            fc.call_action("DeviceConfig1", "Reboot")
+
+            logging.info("FRITZ!Box reboot initiated successfully")
+
+            # Record successful restart in instance metrics
+            with self._metrics_lock:
+                self._restart_total += 1
+                self._restart_last_timestamp = datetime.now().timestamp()
+
+            return True
+
+        except Exception as e:
+            logging.error(f"Failed to restart FRITZ!Box: {e}")
+
+            # Record failed restart attempt in instance metrics
+            with self._metrics_lock:
+                self._restart_failures_total += 1
+
+            return False


The password retrieved from 1Password is logged in case of errors and passed to FritzConnection. While the password itself isn't logged directly, ensure that any exception messages from FritzConnection don't expose the password in traceback information.

Copilot · 2026-01-02T21:33:57Z

network_speed/tests/test_router_restart.py

+                "logging": {"enabled": False}
+            })
+
+            manager = RouterRestartManager(config)


Variable manager is not used.

Copilot

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-02T21:42:00Z

network_speed/src/router_restart.py

+        try:
+            result = subprocess.run(
+                ["op", "read", self.onepassword_ref],
+                capture_output=True,
+                text=True,
+                check=True
+            )


The subprocess call passes user-configurable input (self.onepassword_ref) to a shell command. While there's validation via _validate_onepassword_ref(), the regex pattern allows spaces and backslashes which could potentially be exploited. Consider using a more restrictive pattern or additional sanitization to prevent command injection risks.

Copilot · 2026-01-02T21:42:01Z

network_speed/src/router_restart.py

+
+    def _validate_onepassword_ref(self):
+        """Validate the 1Password reference format to prevent unsafe inputs."""
+        pattern = r"^op://[A-Za-z0-9 ._\\-]+/[A-Za-z0-9 ._\\-]+/[A-Za-z0-9 ._\\-]+$"


The validation regex on line 243 allows backslashes in the 1Password reference format, which is unusual and potentially risky. The pattern [A-Za-z0-9 ._\\-]+ includes escaped backslash \\. Consider whether backslashes are actually needed in 1Password references, as they could be used in path traversal or injection attacks.

Suggested change

pattern = r"^op://[A-Za-z0-9 ._\\-]+/[A-Za-z0-9 ._\\-]+/[A-Za-z0-9 ._\\-]+$"

pattern = r"^op://[A-Za-z0-9 ._-]+/[A-Za-z0-9 ._-]+/[A-Za-z0-9 ._-]+$"

kshk123 added 2 commits December 29, 2025 22:24

Added router restart capability and some refactoring

00c7559

Some further adjustments

18e16b8

Copilot AI review requested due to automatic review settings January 2, 2026 15:43

Copilot started reviewing on behalf of kshk123 January 2, 2026 15:43 View session

Copilot AI reviewed Jan 2, 2026

View reviewed changes

kshk123 added 2 commits January 2, 2026 22:24

Code review comments addressed

43f45c2

Trying to fix build errors

b344606

Copilot AI review requested due to automatic review settings January 2, 2026 21:29

Copilot started reviewing on behalf of kshk123 January 2, 2026 21:30 View session

Update lockfile with dependency-groups

99c9cbd

Copilot AI reviewed Jan 2, 2026

View reviewed changes

kshk123 added 2 commits January 2, 2026 22:34

Fixed test case

1d57d72

Added some security hardening

86979c2

Copilot AI review requested due to automatic review settings January 2, 2026 21:37

Copilot started reviewing on behalf of kshk123 January 2, 2026 21:38 View session

Copilot AI reviewed Jan 2, 2026

View reviewed changes

More review comments addressed

d8cd7a9

kshk123 merged commit 4393275 into main Jan 2, 2026
1 check passed

kshk123 deleted the add-router-restart-capability branch January 2, 2026 21:51

	manager = RouterRestartManager(config)
	RouterRestartManager(config)

	from unittest.mock import Mock, patch, MagicMock
	from unittest.mock import Mock, patch

	except:
	except Exception:
	# Intentionally ignore errors while forcefully terminating the process during cleanup.

	pattern = r"^op://[A-Za-z0-9 ._\\-]+/[A-Za-z0-9 ._\\-]+/[A-Za-z0-9 ._\\-]+$"
	pattern = r"^op://[A-Za-z0-9 ._-]+/[A-Za-z0-9 ._-]+/[A-Za-z0-9 ._-]+$"

Conversation

kshk123 commented Jan 2, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant