Skip to content

Conversation

@jeffmarcilliat
Copy link

@jeffmarcilliat jeffmarcilliat commented Jan 10, 2026

Summary

Implements a comprehensive network configuration validator for Netplan and NetworkManager as requested in #445.

Features

  • YAML Syntax Validation - Catches formatting errors before apply with line number tracking
  • Semantic Correctness Checks - Validates IPs, routes, gateways, DNS, MACs
  • Duplicate IP Detection - Detects IP conflicts across interfaces
  • DHCP + Static IP Conflict Detection - Warns when both are configured
  • File Permission Security Checks - Ensures config files have proper permissions
  • Diff Display - Shows colorized diff of proposed changes
  • Dry-run Mode - Applies config temporarily with automatic revert timer (using netplan try)
  • Plain English Error Messages - User-friendly messages like "The gateway '10.0.0.1' isn't in the same network as your IP '192.168.1.100'"

New Files

  • cortex/network_config_validator.py (~1,168 lines) - Complete validator implementation
  • tests/test_network_config_validator.py (~1,132 lines) - 73 comprehensive tests

CLI Commands Added

  • cortex netconfig validate - Validate all network config files
  • cortex netconfig diff <file> <new> - Show diff between configs
  • cortex netconfig try [--timeout N] - Apply temporarily with auto-revert
  • cortex netconfig summary - Show current network configuration

Testing

  • All 73 new tests pass
  • All existing tests pass
  • Ruff linting passes

Closes #445


Generated by Freelance Agent using Claude Code

Summary by CodeRabbit

  • New Features

    • New "netconfig" CLI command: validate, diff, try (dry-run), and summary actions.
    • Supports Netplan and NetworkManager on Debian/Ubuntu with detailed, categorized validation reports and actionable suggestions.
    • Shows configuration diffs and summaries; dry-run mode with timeout and automatic revert guidance.
    • Emits file-permission warnings and user-friendly error messages.
  • Tests

    • Extensive test coverage for validation, diffing, dry-run, reporting, and CLI behaviors.

✏️ Tip: You can customize this high-level summary in your review settings.

Implements a comprehensive network configuration validator that addresses
the common pain point of Netplan's strict YAML formatting causing network
outages from simple typos.

Features:
- YAML syntax validation before apply with clear error messages
- Semantic correctness checks (valid IPs, routes, gateways, DNS, MACs)
- Diff display showing what will change before applying
- Dry-run mode with automatic revert timer using netplan try
- Plain English error messages with actionable suggestions

CLI commands:
- cortex netconfig validate - Validate all network config files
- cortex netconfig diff <file> <new> - Show diff between configs
- cortex netconfig try [--timeout N] - Apply temporarily with auto-revert
- cortex netconfig summary - Show current network configuration

Includes 73 comprehensive tests covering:
- IP/CIDR/MAC address validation
- Gateway reachability checking
- Netplan-specific validation rules
- File permission security checks
- Diff generation and display
- Dry-run revert mechanism

Closes cortexlinux#445

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 10, 2026

📝 Walkthrough

Walkthrough

Adds a network configuration validator and CLI integration that validates YAML and semantic correctness for Netplan/NetworkManager, shows diffs, supports dry-run with automatic revert timers, and exposes utilities for validation, diffing, and dry-run operations.

Changes

Cohort / File(s) Summary
CLI Integration
cortex/cli.py
Adds CortexCLI.netconfig() handler and wires a netconfig subparser with actions validate, diff, try, and summary plus args: --verbose, --file, --new-content, --timeout. Routes CLI to validation/diff/dry-run helpers and handles dependency/file errors.
Network Config Validator Core
cortex/network_config_validator.py
New comprehensive validator implementing enums (ValidationSeverity, ConfigType), dataclasses (ValidationIssue, ValidationReport, ConfigDiff), validators (IP/CIDR/MAC/gateway), NetworkConfigValidator (detects system, collects files, YAML + semantic checks, diffing, dry-run with backup/revert timers), and CLI helpers (run_validation, run_dry_run, show_config_diff).
Tests
tests/test_network_config_validator.py
Adds extensive unit/integration tests covering validators, error messages, data models, Netplan/NetworkManager detection and validation, diff/dry-run flows, backup/revert mechanics, permission checks, and CLI helper behavior.

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant CLI as CLI Handler
    participant Validator as NetworkConfigValidator
    participant FS as File System
    participant ConfigSys as Config System (Netplan/NetworkManager)

    User->>CLI: cortex netconfig validate --file /etc/netplan/01.yaml --verbose
    CLI->>Validator: run_validation(verbose=True, file=/etc/netplan/01.yaml)
    Validator->>FS: detect_config_system()
    FS-->>Validator: system type
    Validator->>FS: collect_config_files()
    FS-->>Validator: config files content
    Validator->>Validator: _validate_yaml_syntax(file)
    alt YAML valid
        Validator->>Validator: semantic checks (IPs, CIDR, MAC, routes, DNS)
        Validator->>Validator: build ValidationReport
    else YAML invalid
        Validator-->>CLI: ValidationReport (syntax error)
    end
    Validator-->>CLI: ValidationReport
    CLI-->>User: formatted report + exit code
Loading
sequenceDiagram
    actor User
    participant CLI as CLI Handler
    participant Validator as NetworkConfigValidator
    participant FS as File System
    participant ConfigSys as Netplan
    participant Timer as Auto-Revert Timer

    User->>CLI: cortex netconfig try --timeout 120 --file /etc/netplan/01.yaml
    CLI->>Validator: run_dry_run(timeout=120, file=...)
    Validator->>FS: _backup_configs_to_memory()
    FS-->>Validator: backups
    Validator->>ConfigSys: netplan try (apply temporarily)
    ConfigSys-->>Validator: applied
    Validator->>Timer: _auto_revert(120s)
    Timer-->>User: countdown (visible)
    alt User confirms before timeout
        User->>Validator: confirm
        Validator->>ConfigSys: netplan apply (persist)
        Validator->>Timer: _cancel_revert_timer()
    else timeout or cancel
        Timer->>Validator: revert trigger
        Validator->>FS: _revert_configs()
        FS->>ConfigSys: restore previous configs
    end
    Validator-->>CLI: result status
    CLI-->>User: operation complete
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 I nibble lines of YAML with care,

I patch and diff each network snare,
Dry-run timers tick in the air,
Revert if chaos dares to flare,
Hopping safe, your packets fare.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: adding a network configuration validator for Netplan and NetworkManager. It is concise and specific.
Description check ✅ Passed The PR description follows the template structure with a summary, features, new files, CLI commands, and testing information. It addresses the linked issue and provides comprehensive details about the implementation.
Linked Issues check ✅ Passed All primary objectives from issue #445 are met: YAML syntax validation, semantic correctness checks, diff display, dry-run mode with revert timer, and plain-English error messages are all implemented.
Out of Scope Changes check ✅ Passed All changes are directly related to implementing the network config validator as specified in issue #445. No out-of-scope modifications detected beyond the validator feature and its tests.
Docstring Coverage ✅ Passed Docstring coverage is 99.08% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link

CLA Verification Failed

The following contributors have not signed the Contributor License Agreement:

  • @jeffmarcilliat (SF Hodl, jeffmarcilliat@mac.com)
  • Claude Opus 4.5 (noreply@anthropic.com)

How to Sign

  1. Read the CLA document
  2. Open a CLA signature request
  3. A maintainer will add you to the signers list
  4. Comment recheck on this PR to re-run verification

This check runs automatically. Maintainers can update .github/cla-signers.json to add signers.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @jeffmarcilliat, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request delivers a robust network configuration validator designed to prevent network issues by proactively identifying errors in Netplan and NetworkManager configurations. It provides a suite of tools for syntax and semantic validation, allowing users to safely preview and test network changes before applying them, thereby enhancing system stability and reducing potential downtime.

Highlights

  • Comprehensive Network Configuration Validation: Introduces a new cortex netconfig command-line utility to validate and manage network configurations for both Netplan and NetworkManager, addressing issue Network Config Validator (Netplan/NetworkManager) #445.
  • Advanced Validation Checks: Includes YAML syntax validation, semantic correctness checks for IPs, routes, gateways, DNS, and MAC addresses, duplicate IP detection, and warnings for DHCP + static IP conflicts.
  • Security and Usability Features: Incorporates file permission security checks, a colorized diff display for proposed changes, a dry-run mode with an automatic revert timer (using netplan try), and user-friendly, plain English error messages.
  • New CLI Commands: Adds cortex netconfig validate, cortex netconfig diff <file> <new>, cortex netconfig try [--timeout N], and cortex netconfig summary for various network configuration tasks.
  • Extensive Test Coverage: A new test file with 73 comprehensive tests has been added to ensure the reliability and correctness of the network configuration validator.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a comprehensive network configuration validator, which is a fantastic addition. The implementation is extensive and accompanied by a thorough test suite, demonstrating high code quality. The use of modern Python features and user-friendly error messages is commendable.

I have identified a critical issue in the dry_run implementation related to its handling of an interactive command and a redundant, risky custom revert mechanism. I've also found a medium-severity bug that could cause a crash when printing the network summary. My review provides suggestions to simplify the dry_run logic for better safety and robustness, and to fix the potential IndexError. Addressing these points will make this a solid and valuable feature.

Comment on lines 860 to 928
def dry_run(self, timeout_seconds: int = 120) -> bool:
"""
Apply configuration in dry-run mode with automatic revert.

This applies the configuration temporarily and automatically reverts
after the timeout unless the user confirms the changes.

Args:
timeout_seconds: Seconds before automatic revert (default: 120)

Returns:
True if configuration was applied and confirmed, False otherwise
"""
config_type = self.detect_config_system()

if config_type != ConfigType.NETPLAN:
console.print(
"[yellow]Dry-run mode is currently only supported for Netplan[/yellow]"
)
return False

# Backup current configs
if not self._backup_configs_to_memory():
console.print("[red]Failed to backup current configuration[/red]")
return False

console.print(
f"\n[yellow]Applying configuration in dry-run mode...[/yellow]"
f"\n[yellow]Configuration will automatically revert in {timeout_seconds} seconds[/yellow]"
f"\n[yellow]if you don't confirm the changes.[/yellow]\n"
)

# Start revert timer
self._revert_timer = threading.Timer(
timeout_seconds, self._auto_revert, args=[config_type]
)
self._revert_timer.start()

# Apply configuration
try:
result = subprocess.run(
["sudo", "netplan", "try", f"--timeout={timeout_seconds}"],
capture_output=True,
text=True,
timeout=timeout_seconds + 30,
)

if result.returncode == 0:
console.print("[green]Configuration applied successfully![/green]")
self._cancel_revert_timer()
self._backup_configs.clear()
return True
else:
console.print(f"[red]Failed to apply configuration:[/red]\n{result.stderr}")
self._revert_configs()
return False

except subprocess.TimeoutExpired:
console.print("[yellow]Timeout expired, configuration reverted[/yellow]")
return False
except FileNotFoundError:
console.print("[red]netplan command not found. Is Netplan installed?[/red]")
self._cancel_revert_timer()
return False
except subprocess.CalledProcessError as e:
console.print(f"[red]Error applying configuration: {e}[/red]")
self._revert_configs()
return False

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The dry_run implementation has a few critical issues:

  1. Incorrect subprocess Usage: It uses capture_output=True with netplan try, which is an interactive command that prompts the user for confirmation. This will hide the prompt from the user, causing the command to hang until it times out.
  2. Redundant Revert Logic: It implements a custom revert mechanism using threading.Timer and manual file backups. This is redundant because netplan try is designed to be atomic and has its own safe, built-in revert mechanism.
  3. Race Condition Risk: The custom timer-based revert could potentially race with netplan try's own operations, leading to an inconsistent state.

The method should be simplified to be a direct wrapper around netplan try, letting it handle interactivity and safe reverts. This would also allow for the removal of the now-unnecessary helper methods: _backup_configs_to_memory, _auto_revert, _revert_configs, and _cancel_revert_timer, significantly simplifying the code.

    def dry_run(self, timeout_seconds: int = 120) -> bool:
        """
        Apply configuration in dry-run mode with automatic revert using `netplan try`.

        This command is atomic and will automatically revert if not confirmed.

        Args:
            timeout_seconds: Seconds to wait for confirmation before automatic revert.

        Returns:
            True if configuration was applied and confirmed, False otherwise.
        """
        config_type = self.detect_config_system()

        if config_type != ConfigType.NETPLAN:
            console.print(
                "[yellow]Dry-run mode is currently only supported for Netplan[/yellow]"
            )
            return False

        console.print(
            f"\n[yellow]Applying configuration using 'netplan try'...[/yellow]"
            f"\n[yellow]You will be prompted to confirm the new configuration.[/yellow]"
            f"\n[yellow]If not confirmed, it will automatically revert after {timeout_seconds} seconds.[/yellow]\n"
        )

        try:
            # `netplan try` is interactive, so we should not capture output.
            result = subprocess.run(
                ["sudo", "netplan", "try", f"--timeout={timeout_seconds}"],
                # The subprocess timeout should be longer than the netplan timeout
                # to allow for user interaction and revert time.
                timeout=timeout_seconds + 60,
            )

            if result.returncode == 0:
                console.print("[green]Configuration applied and confirmed successfully![/green]")
                return True
            else:
                console.print(f"[red]Failed to apply configuration. 'netplan try' exited with code {result.returncode}.[/red]")
                console.print("[yellow]Configuration should have been reverted automatically by netplan.[/yellow]")
                return False

        except subprocess.TimeoutExpired:
            console.print("[yellow]Command timed out. Configuration should have been reverted by netplan.[/yellow]")
            return False
        except FileNotFoundError:
            console.print("[red]'netplan' command not found. Is Netplan installed?[/red]")
            return False
        except Exception as e:
            console.print(f"[red]An unexpected error occurred during 'netplan try': {e}[/red]")
            return False

for addr in addresses:
console.print(f" IP: {addr}")

gw = iface_config.get("gateway4") or iface_config.get("routes", [{}])[0].get("via")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This line can cause an IndexError if the routes key exists in the configuration but is an empty list (e.g., routes: []). In that case, iface_config.get("routes", [{}]) returns the empty list [], and accessing [0] fails. A safer approach is needed to prevent the command from crashing.

                                gw = iface_config.get("gateway4") or (iface_config.get("routes") or [{}])[0].get("via")

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
cortex/network_config_validator.py (2)

860-928: Consider thread safety for early exceptions in dry_run.

The revert timer is started at line 893-896 before the subprocess call. If the subprocess raises an unexpected exception not caught by the existing handlers (e.g., an unhandled error), the timer could continue running after the method returns. However, the current exception handling is comprehensive (TimeoutExpired, FileNotFoundError, CalledProcessError), so this is a minor edge case.

The implementation correctly uses netplan try with a matching timeout parameter, which is the proper approach for safe configuration testing.

🔧 Optional: Wrap subprocess in try-finally for timer cleanup
         # Start revert timer
         self._revert_timer = threading.Timer(
             timeout_seconds, self._auto_revert, args=[config_type]
         )
         self._revert_timer.start()

         # Apply configuration
         try:
             result = subprocess.run(
                 ["sudo", "netplan", "try", f"--timeout={timeout_seconds}"],
                 capture_output=True,
                 text=True,
                 timeout=timeout_seconds + 30,
             )

             if result.returncode == 0:
                 console.print("[green]Configuration applied successfully![/green]")
                 self._cancel_revert_timer()
                 self._backup_configs.clear()
                 return True
             else:
                 console.print(f"[red]Failed to apply configuration:[/red]\n{result.stderr}")
                 self._revert_configs()
                 return False

         except subprocess.TimeoutExpired:
             console.print("[yellow]Timeout expired, configuration reverted[/yellow]")
             return False
         except FileNotFoundError:
             console.print("[red]netplan command not found. Is Netplan installed?[/red]")
             self._cancel_revert_timer()
             return False
         except subprocess.CalledProcessError as e:
             console.print(f"[red]Error applying configuration: {e}[/red]")
             self._revert_configs()
             return False
+        except Exception:
+            self._cancel_revert_timer()
+            raise

18-29: Unused import: time module.

The time module is imported at line 24 but does not appear to be used anywhere in the file.

🧹 Remove unused import
 import subprocess
 import tempfile
 import threading
-import time
 from dataclasses import dataclass, field
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fd7922b and 7532c09.

📒 Files selected for processing (3)
  • cortex/cli.py
  • cortex/network_config_validator.py
  • tests/test_network_config_validator.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs

Files:

  • tests/test_network_config_validator.py
  • cortex/cli.py
  • cortex/network_config_validator.py
tests/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

Maintain >80% test coverage for pull requests

Files:

  • tests/test_network_config_validator.py
🧬 Code graph analysis (1)
tests/test_network_config_validator.py (1)
cortex/network_config_validator.py (31)
  • ConfigDiff (136-142)
  • ConfigType (52-57)
  • NetworkConfigValidator (301-1103)
  • ValidationIssue (64-85)
  • ValidationReport (89-132)
  • ValidationSeverity (43-49)
  • get_error_message (211-217)
  • run_dry_run (1130-1151)
  • run_validation (1109-1127)
  • show_config_diff (1154-1168)
  • validate_cidr (240-254)
  • validate_gateway_reachable (274-295)
  • validate_ip_address (223-237)
  • validate_mac_address (257-271)
  • info_count (99-101)
  • warning_count (104-106)
  • error_count (109-111)
  • critical_count (114-116)
  • detect_config_system (332-363)
  • get_config_files (365-391)
  • validate_yaml_syntax (395-481)
  • validate_netplan_config (485-572)
  • validate (749-784)
  • show_diff (810-845)
  • print_diff (847-856)
  • dry_run (860-927)
  • print_report (984-1053)
  • print_config_summary (1055-1065)
  • _check_file_permissions (786-806)
  • _backup_configs_to_memory (929-942)
  • _cancel_revert_timer (976-980)
🔇 Additional comments (16)
cortex/cli.py (3)

384-461: LGTM! Clean CLI integration for netconfig.

The lazy import pattern correctly handles the optional PyYAML dependency, and the error handling is comprehensive (ImportError, PermissionError, OSError). The implementation follows existing CLI patterns in this file.


2301-2336: LGTM! Argparse configuration is well-structured.

The subparser setup correctly defines all netconfig actions with appropriate arguments. Default timeout of 120 seconds aligns with the implementation.


2646-2647: LGTM!

Command routing follows the established pattern.

tests/test_network_config_validator.py (6)

33-129: LGTM! Comprehensive validation function tests.

Good coverage of edge cases including valid/invalid inputs, different formats (colon vs dash for MAC), and boundary conditions. The comment at line 81-84 helpfully explains the CIDR without prefix behavior.


131-231: LGTM! Data class and error message tests.

Tests appropriately verify object creation, attribute access, serialization, and computed properties (severity counts).


233-346: LGTM! Initialization and detection tests.

Proper use of temporary directories for filesystem isolation and mocking for subprocess calls. Tests cover the main code paths for system detection.


348-659: LGTM! Thorough YAML and Netplan validation tests.

Excellent coverage of validation scenarios including edge cases like DHCP+static conflicts, duplicate IPs, unreachable gateways, and route validation. Temporary files are properly cleaned up.


661-875: LGTM! Workflow and display tests.

Good coverage of end-to-end validation, diff generation, and dry-run failure scenarios. Display method tests appropriately verify no exceptions are raised.


877-1132: LGTM! CLI, permissions, revert, and integration tests.

Good coverage including CLI helper functions, file permission checks with proper chmod calls, revert timer mechanics, and end-to-end integration scenarios with multiple config files.

cortex/network_config_validator.py (7)

43-143: LGTM! Well-designed data structures.

The dataclasses are properly typed with computed properties for severity counts. The to_dict methods correctly serialize enum values for JSON output.


147-296: LGTM! User-friendly error messages and robust validation.

Plain English error messages align with issue #445 requirements. Validation functions use standard library ipaddress module correctly, with strict=False appropriately allowing host bits in CIDR notation.


301-392: LGTM! Detection and file retrieval methods.

Proper timeout handling for subprocess calls and appropriate exception handling. The detection priority (Netplan → NetworkManager → systemd fallback) is sensible for Ubuntu systems.


395-746: LGTM! Comprehensive validation logic.

YAML syntax validation gracefully handles the optional PyYAML dependency and extracts line numbers from errors. Netplan validation thoroughly checks structure, IPs, gateways, routes, DNS, and MAC addresses. The duplicate IP detection using a set is efficient.


749-807: LGTM! Full validation workflow.

The validity determination correctly considers only ERROR and CRITICAL severity issues. File permission checks appropriately issue warnings (not errors) for world-readable configs.


984-1104: LGTM! Rich-formatted display methods.

The report and summary display methods use Rich library effectively for colorized, structured output. Exception handling in _print_netplan_summary ensures display continues even if individual files have issues.


1109-1168: LGTM! CLI integration functions.

The run_dry_run function correctly validates the configuration before attempting to apply it, preventing accidental application of broken configs. All functions return appropriate exit codes for CLI usage.

- Remove capture_output=True from netplan try (interactive command)
- Simplify dry_run to rely on netplan try's native timeout/revert
- Fix IndexError when routes list is empty
- Remove unused time import

Addresses feedback from @gemini-code-assist and @coderabbitai

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
13 Security Hotspots

See analysis details on SonarQube Cloud

@jeffmarcilliat
Copy link
Author

Review Feedback Addressed

I've pushed a commit addressing the review feedback:

Changes Made

  1. Fixed critical dry_run issue (@gemini-code-assist)

    • Removed capture_output=True since netplan try is an interactive command
    • Simplified implementation to rely on netplan try's native timeout and revert behavior
    • Removed redundant custom backup/revert logic from the dry_run flow
  2. Fixed IndexError (@gemini-code-assist)

    • Safe access to routes list - no longer crashes when routes: []
  3. Removed unused import (@coderabbitai)

    • Removed unused time module import

CLA Signed

I've submitted a CLA signature request in #553.


Updated by Freelance Agent using Claude Code

@jeffmarcilliat
Copy link
Author

recheck

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 10, 2026

Tip

For best results, initiate chat on the files or code changes.

@jeffmarcilliat Thank you for the comprehensive update and for addressing all the review feedback! 🎉

Great work on the fixes:

  1. dry_run fix - Smart simplification to leverage netplan try's built-in interactive behavior and timeout/revert functionality. This is the right approach for an interactive command.

  2. IndexError fix - Good defensive programming to handle empty routes lists safely.

  3. Unused import - Thanks for removing the time module import!

  4. CLA submission - Appreciate you taking care of CLA Signature Request: jeffmarcilliat #553 as well.

The changes look solid. Feel free to let me know if you'd like me to review the updated code!


If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🤖 Fix all issues with AI agents
In @cortex/network_config_validator.py:
- Around line 331-363: detect_config_system() and get_config_files() only glob
for "*.yaml", missing ".yml" and causing Netplan files to be ignored; update the
Netplan discovery logic to check for both "*.yaml" and "*.yml" (or a single
pattern that matches both) when listing files under netplan_dir and when
deciding Netplan is present, and ensure any code that builds the file list
(e.g., netplan_files = list(self.netplan_dir.glob("*.yaml"))) uses the new
pattern(s) so Netplan configs with .yml are detected and validated.
- Around line 484-572: The duplicate-IP detection currently creates seen_ips
inside _validate_interfaces, so duplicates across interface types in the same
Netplan file aren’t detected; fix by creating a file-scoped seen_ips set in
validate_netplan_config and pass it into _validate_interfaces (or change
_validate_interfaces signature to accept an optional seen_ips set), then use
that shared set when iterating iface types
("ethernets","wifis","bonds","bridges","vlans"); if you want global detection
across files, lift the set further up into validate() and propagate it
similarly.
- Around line 312-328: The __init__ of NetworkConfigValidator is missing an
explicit return type; update the constructor signature for the method __init__
in class NetworkConfigValidator to include the return type "-> None" (i.e.,
change "def __init__(...):" to "def __init__(...) -> None:"), keeping the
existing parameters and body unchanged so it conforms to the project's
type-hinting guidelines.
- Around line 748-765: The validate() function currently sends all non-NETPLAN
files to validate_yaml_syntax(), causing false positives for
ConfigType.NETWORKMANAGER; update validate() to detect ConfigType.NETWORKMANAGER
and call a new validate_networkmanager_syntax(config_file) instead of
validate_yaml_syntax(config_file). Implement validate_networkmanager_syntax to
perform basic INI/keyfile syntax checks (use Python's configparser to attempt
parsing and return ValidationIssue entries on parse errors) and ensure
validate_networkmanager_syntax returns the same list[ValidationIssue] shape as
validate_yaml_syntax so validate() can extend all_issues uniformly.
- Around line 859-913: The dry_run() function currently has an unreachable
subprocess.CalledProcessError except block and treats any non-zero
result.returncode as "Configuration was reverted", which hides real errors;
remove the subprocess.CalledProcessError handler, and change the non-zero-exit
branch (the else after subprocess.run) to print a clear failure message that
includes result.returncode and any captured output (e.g.,
result.stdout/result.stderr if available) so callers can distinguish an actual
error from a revert; keep the TimeoutExpired and FileNotFoundError handlers
as-is and ensure you do not switch to check=True since netplan try is
interactive.
🧹 Nitpick comments (1)
cortex/network_config_validator.py (1)

914-966: Dead-code risk: backup/revert timer helpers appear unused after switching to native netplan try.

_backup_configs_to_memory(), _auto_revert(), _revert_configs(), _cancel_revert_timer() and _revert_timer state aren’t used by dry_run() anymore. Consider removing or re-wiring to avoid maintaining two revert strategies.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7532c09 and c02503b.

📒 Files selected for processing (2)
  • cortex/network_config_validator.py
  • tests/test_network_config_validator.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/test_network_config_validator.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs

Files:

  • cortex/network_config_validator.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*install*.py : Dry-run by default for all installations in command execution

Applied to files:

  • cortex/network_config_validator.py
🔇 Additional comments (1)
cortex/network_config_validator.py (1)

809-856: Diff/report/CLI wrappers are clean and consistent.

show_diff() + print_diff(), print_report(), and CLI functions (run_validation, run_dry_run, show_config_diff) are straightforward and nicely structured for CLI consumption.

Also applies to: 969-1039, 1095-1154

Comment on lines +312 to +328
def __init__(
self,
netplan_dir: str = "/etc/netplan",
nm_dir: str = "/etc/NetworkManager",
):
"""
Initialize the network configuration validator.

Args:
netplan_dir: Path to Netplan configuration directory
nm_dir: Path to NetworkManager configuration directory
"""
self.netplan_dir = Path(netplan_dir)
self.nm_dir = Path(nm_dir)
self._revert_timer: threading.Timer | None = None
self._backup_configs: dict[str, str] = {}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add missing return type on __init__ (type-hints guideline).

Per **/*.py guidelines, add -> None to NetworkConfigValidator.__init__.

Proposed diff
     def __init__(
         self,
         netplan_dir: str = "/etc/netplan",
         nm_dir: str = "/etc/NetworkManager",
-    ):
+    ) -> None:
🤖 Prompt for AI Agents
In @cortex/network_config_validator.py around lines 312 - 328, The __init__ of
NetworkConfigValidator is missing an explicit return type; update the
constructor signature for the method __init__ in class NetworkConfigValidator to
include the return type "-> None" (i.e., change "def __init__(...):" to "def
__init__(...) -> None:"), keeping the existing parameters and body unchanged so
it conforms to the project's type-hinting guidelines.

Comment on lines +331 to +363
def detect_config_system(self) -> ConfigType:
"""
Detect which network configuration system is in use.

Returns:
ConfigType enum indicating the active system
"""
# Check for Netplan first (common on Ubuntu 18.04+)
if self.netplan_dir.exists():
netplan_files = list(self.netplan_dir.glob("*.yaml"))
if netplan_files:
return ConfigType.NETPLAN

# Check for NetworkManager
if self.nm_dir.exists():
nm_conf = self.nm_dir / "NetworkManager.conf"
if nm_conf.exists():
return ConfigType.NETWORKMANAGER

# Also check via systemd
try:
result = subprocess.run(
["systemctl", "is-active", "NetworkManager"],
capture_output=True,
timeout=5,
)
if result.returncode == 0:
return ConfigType.NETWORKMANAGER
except (FileNotFoundError, subprocess.TimeoutExpired):
pass

return ConfigType.UNKNOWN

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Netplan discovery ignores .yml files (can mis-detect system / skip validation).

detect_config_system() / get_config_files() only glob *.yaml. Netplan commonly uses .yaml, but .yml exists in the wild; missing it can make the validator report UNKNOWN and skip key checks.

Proposed diff
     def detect_config_system(self) -> ConfigType:
@@
-        if self.netplan_dir.exists():
-            netplan_files = list(self.netplan_dir.glob("*.yaml"))
+        if self.netplan_dir.is_dir():
+            netplan_files = list(self.netplan_dir.glob("*.yaml")) + list(
+                self.netplan_dir.glob("*.yml")
+            )
             if netplan_files:
                 return ConfigType.NETPLAN
@@
     def get_config_files(self) -> list[Path]:
@@
         if config_type == ConfigType.NETPLAN:
-            if self.netplan_dir.exists():
-                return sorted(self.netplan_dir.glob("*.yaml"))
+            if self.netplan_dir.is_dir():
+                return sorted(
+                    list(self.netplan_dir.glob("*.yaml")) + list(self.netplan_dir.glob("*.yml"))
+                )
             return []

Also applies to: 364-390

🤖 Prompt for AI Agents
In @cortex/network_config_validator.py around lines 331 - 363,
detect_config_system() and get_config_files() only glob for "*.yaml", missing
".yml" and causing Netplan files to be ignored; update the Netplan discovery
logic to check for both "*.yaml" and "*.yml" (or a single pattern that matches
both) when listing files under netplan_dir and when deciding Netplan is present,
and ensure any code that builds the file list (e.g., netplan_files =
list(self.netplan_dir.glob("*.yaml"))) uses the new pattern(s) so Netplan
configs with .yml are detected and validated.

Comment on lines +484 to +572
def validate_netplan_config(self, file_path: Path) -> list[ValidationIssue]:
"""
Validate a Netplan configuration file.

Args:
file_path: Path to the Netplan YAML file

Returns:
List of validation issues found
"""
issues: list[ValidationIssue] = []

# First check YAML syntax
yaml_issues = self.validate_yaml_syntax(file_path)
if any(i.severity == ValidationSeverity.ERROR for i in yaml_issues):
return yaml_issues # Can't continue if YAML is broken

issues.extend(yaml_issues)

try:
import yaml
except ImportError:
return issues

try:
with open(file_path, encoding="utf-8") as f:
config = yaml.safe_load(f)

if not config:
return issues

# Check for network key
if "network" not in config:
issues.append(
ValidationIssue(
rule_id="NP001",
message=get_error_message("missing_version"),
severity=ValidationSeverity.ERROR,
file_path=str(file_path),
suggestion="Add 'network:\\n version: 2' at the start of your config",
)
)
return issues

network = config["network"]

# Check version
if "version" not in network:
issues.append(
ValidationIssue(
rule_id="NP002",
message="Missing version number in network configuration.",
severity=ValidationSeverity.WARNING,
file_path=str(file_path),
suggestion="Add 'version: 2' under the 'network:' key",
)
)

# Check renderer
if "renderer" not in network:
issues.append(
ValidationIssue(
rule_id="NP003",
message=get_error_message("missing_renderer"),
severity=ValidationSeverity.INFO,
file_path=str(file_path),
)
)

# Validate each interface type
for iface_type in ["ethernets", "wifis", "bonds", "bridges", "vlans"]:
if iface_type in network:
iface_issues = self._validate_interfaces(
network[iface_type], iface_type, str(file_path)
)
issues.extend(iface_issues)

except (OSError, yaml.YAMLError) as e:
issues.append(
ValidationIssue(
rule_id="NP099",
message=f"Error reading config: {e}",
severity=ValidationSeverity.ERROR,
file_path=str(file_path),
)
)

return issues

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Duplicate IP detection is not global across interface types (and likely not across files).

seen_ips is created inside _validate_interfaces(), so duplicates across ethernets vs wifis (etc.) in the same file won’t be flagged. If you want “across interfaces” across the whole config, this should be tracked at the file level (and optionally across all Netplan files during validate()).

Proposed diff (file-level global set across iface types)
     def validate_netplan_config(self, file_path: Path) -> list[ValidationIssue]:
@@
-            # Validate each interface type
+            # Validate each interface type
+            seen_ips: set[str] = set()
             for iface_type in ["ethernets", "wifis", "bonds", "bridges", "vlans"]:
                 if iface_type in network:
                     iface_issues = self._validate_interfaces(
-                        network[iface_type], iface_type, str(file_path)
+                        network[iface_type], iface_type, str(file_path), seen_ips
                     )
                     issues.extend(iface_issues)
@@
     def _validate_interfaces(
-        self, interfaces: dict[str, Any], iface_type: str, file_path: str
+        self,
+        interfaces: dict[str, Any],
+        iface_type: str,
+        file_path: str,
+        seen_ips: set[str],
     ) -> list[ValidationIssue]:
@@
-        seen_ips: set[str] = set()
-
         for iface_name, iface_config in interfaces.items():

Also applies to: 573-706

🤖 Prompt for AI Agents
In @cortex/network_config_validator.py around lines 484 - 572, The duplicate-IP
detection currently creates seen_ips inside _validate_interfaces, so duplicates
across interface types in the same Netplan file aren’t detected; fix by creating
a file-scoped seen_ips set in validate_netplan_config and pass it into
_validate_interfaces (or change _validate_interfaces signature to accept an
optional seen_ips set), then use that shared set when iterating iface types
("ethernets","wifis","bonds","bridges","vlans"); if you want global detection
across files, lift the set further up into validate() and propagate it
similarly.

Comment on lines +573 to +706
def _validate_interfaces(
self, interfaces: dict[str, Any], iface_type: str, file_path: str
) -> list[ValidationIssue]:
"""Validate interface configurations."""
issues: list[ValidationIssue] = []

if not isinstance(interfaces, dict):
return issues

seen_ips: set[str] = set()

for iface_name, iface_config in interfaces.items():
if not isinstance(iface_config, dict):
continue

# Check for DHCP + static IP conflict
dhcp4 = iface_config.get("dhcp4", False)
dhcp6 = iface_config.get("dhcp6", False)
addresses = iface_config.get("addresses", [])

if (dhcp4 or dhcp6) and addresses:
issues.append(
ValidationIssue(
rule_id="NP010",
message=get_error_message("dhcp_with_static"),
severity=ValidationSeverity.WARNING,
file_path=file_path,
context=f"Interface: {iface_name}",
)
)

# Validate addresses
for addr in addresses:
if isinstance(addr, str):
is_valid, error = validate_cidr(addr)
if not is_valid:
issues.append(
ValidationIssue(
rule_id="NP011",
message=error or f"Invalid address: {addr}",
severity=ValidationSeverity.ERROR,
file_path=file_path,
context=f"Interface: {iface_name}",
)
)
else:
# Check for duplicate IPs
ip_only = addr.split("/")[0]
if ip_only in seen_ips:
issues.append(
ValidationIssue(
rule_id="NP012",
message=get_error_message("duplicate_ip", value=ip_only),
severity=ValidationSeverity.ERROR,
file_path=file_path,
context=f"Interface: {iface_name}",
)
)
seen_ips.add(ip_only)

# Validate gateway (legacy gateway4/gateway6)
for gw_key in ["gateway4", "gateway6"]:
if gw_key in iface_config:
gateway = iface_config[gw_key]
is_valid, error = validate_ip_address(str(gateway))
if not is_valid:
issues.append(
ValidationIssue(
rule_id="NP013",
message=get_error_message("invalid_gateway", value=gateway),
severity=ValidationSeverity.ERROR,
file_path=file_path,
context=f"Interface: {iface_name}",
)
)
elif addresses:
# Check gateway reachability
for addr in addresses:
if isinstance(addr, str):
is_reach, reach_error = validate_gateway_reachable(
str(gateway), addr
)
if not is_reach:
issues.append(
ValidationIssue(
rule_id="NP014",
message=reach_error or "Gateway unreachable",
severity=ValidationSeverity.WARNING,
file_path=file_path,
context=f"Interface: {iface_name}",
)
)

# Validate routes
routes = iface_config.get("routes", [])
for route in routes:
if isinstance(route, dict):
route_issues = self._validate_route(route, iface_name, file_path)
issues.extend(route_issues)

# Validate nameservers
nameservers = iface_config.get("nameservers", {})
if isinstance(nameservers, dict):
dns_addresses = nameservers.get("addresses", [])
for dns in dns_addresses:
is_valid, error = validate_ip_address(str(dns))
if not is_valid:
issues.append(
ValidationIssue(
rule_id="NP015",
message=get_error_message("invalid_dns", value=dns),
severity=ValidationSeverity.ERROR,
file_path=file_path,
context=f"Interface: {iface_name}",
)
)

# Validate MAC address
macaddress = iface_config.get("macaddress")
if macaddress:
is_valid, error = validate_mac_address(str(macaddress))
if not is_valid:
issues.append(
ValidationIssue(
rule_id="NP016",
message=error or f"Invalid MAC: {macaddress}",
severity=ValidationSeverity.ERROR,
file_path=file_path,
context=f"Interface: {iface_name}",
)
)

return issues

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Harden addresses/routes type handling to avoid silent mis-validation.

addresses = iface_config.get("addresses", []) and routes = iface_config.get("routes", []) assume lists. If a user supplies a scalar (string/dict) you’ll iterate characters / keys and produce confusing results (or skip validation).

Proposed diff (normalize/guard types)
             addresses = iface_config.get("addresses", [])
+            if isinstance(addresses, str):
+                addresses = [addresses]
+            elif not isinstance(addresses, list):
+                addresses = []
@@
             routes = iface_config.get("routes", [])
+            if not isinstance(routes, list):
+                routes = []

Comment on lines +748 to +765
def validate(self) -> ValidationReport:
"""
Run full validation on all detected configuration files.

Returns:
ValidationReport with all issues found
"""
config_type = self.detect_config_system()
config_files = self.get_config_files()
all_issues: list[ValidationIssue] = []

for config_file in config_files:
if config_type == ConfigType.NETPLAN:
issues = self.validate_netplan_config(config_file)
else:
issues = self.validate_yaml_syntax(config_file)
all_issues.extend(issues)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

NetworkManager configs are validated as YAML (will produce false errors / misses NM semantics).

For ConfigType.NETWORKMANAGER, validate() routes files to validate_yaml_syntax(), but NetworkManager files are typically INI/keyfile format, not YAML. At minimum, skip YAML validation for NM and implement a basic parser (e.g., configparser) for syntax checks.

Minimal “stop false positives” diff
         for config_file in config_files:
             if config_type == ConfigType.NETPLAN:
                 issues = self.validate_netplan_config(config_file)
-            else:
+            elif config_type == ConfigType.NETWORKMANAGER:
+                issues = []
+            else:
                 issues = self.validate_yaml_syntax(config_file)
             all_issues.extend(issues)
🤖 Prompt for AI Agents
In @cortex/network_config_validator.py around lines 748 - 765, The validate()
function currently sends all non-NETPLAN files to validate_yaml_syntax(),
causing false positives for ConfigType.NETWORKMANAGER; update validate() to
detect ConfigType.NETWORKMANAGER and call a new
validate_networkmanager_syntax(config_file) instead of
validate_yaml_syntax(config_file). Implement validate_networkmanager_syntax to
perform basic INI/keyfile syntax checks (use Python's configparser to attempt
parsing and return ValidationIssue entries on parse errors) and ensure
validate_networkmanager_syntax returns the same list[ValidationIssue] shape as
validate_yaml_syntax so validate() can extend all_issues uniformly.

Comment on lines +785 to +806
def _check_file_permissions(self, file_path: Path) -> list[ValidationIssue]:
"""Check if file permissions are secure."""
issues: list[ValidationIssue] = []

try:
mode = file_path.stat().st_mode
# Check if file is world-readable (others can read)
if mode & 0o004:
issues.append(
ValidationIssue(
rule_id="SEC001",
message=get_error_message("file_permission", file=str(file_path)),
severity=ValidationSeverity.WARNING,
file_path=str(file_path),
suggestion=f"Run: sudo chmod 600 {file_path}",
)
)
except OSError:
pass

return issues

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

File permission check only flags “other-readable”; also check group-readable/writable bits.

If the goal is “root-only readable” (600), you likely also want to flag 0o040 (group read) and writable bits (0o022) depending on policy.

Proposed diff
             mode = file_path.stat().st_mode
-            # Check if file is world-readable (others can read)
-            if mode & 0o004:
+            # Check if file is readable by group/others, or writable by group/others
+            if mode & 0o044 or mode & 0o022:
                 issues.append(
                     ValidationIssue(
                         rule_id="SEC001",
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def _check_file_permissions(self, file_path: Path) -> list[ValidationIssue]:
"""Check if file permissions are secure."""
issues: list[ValidationIssue] = []
try:
mode = file_path.stat().st_mode
# Check if file is world-readable (others can read)
if mode & 0o004:
issues.append(
ValidationIssue(
rule_id="SEC001",
message=get_error_message("file_permission", file=str(file_path)),
severity=ValidationSeverity.WARNING,
file_path=str(file_path),
suggestion=f"Run: sudo chmod 600 {file_path}",
)
)
except OSError:
pass
return issues
def _check_file_permissions(self, file_path: Path) -> list[ValidationIssue]:
"""Check if file permissions are secure."""
issues: list[ValidationIssue] = []
try:
mode = file_path.stat().st_mode
# Check if file is readable by group/others, or writable by group/others
if mode & 0o044 or mode & 0o022:
issues.append(
ValidationIssue(
rule_id="SEC001",
message=get_error_message("file_permission", file=str(file_path)),
severity=ValidationSeverity.WARNING,
file_path=str(file_path),
suggestion=f"Run: sudo chmod 600 {file_path}",
)
)
except OSError:
pass
return issues

Comment on lines +859 to +913
def dry_run(self, timeout_seconds: int = 120) -> bool:
"""
Apply configuration in dry-run mode with automatic revert.

This applies the configuration temporarily using `netplan try`, which
handles the timeout and automatic revert natively. The user must press
Enter to confirm the changes, or the configuration will be reverted
after the timeout.

Args:
timeout_seconds: Seconds before automatic revert (default: 120)

Returns:
True if configuration was applied and confirmed, False otherwise
"""
config_type = self.detect_config_system()

if config_type != ConfigType.NETPLAN:
console.print(
"[yellow]Dry-run mode is currently only supported for Netplan[/yellow]"
)
return False

console.print(
f"\n[yellow]Applying configuration in dry-run mode...[/yellow]"
f"\n[yellow]Configuration will automatically revert in {timeout_seconds} seconds[/yellow]"
f"\n[yellow]Press Enter to confirm the changes, or wait for timeout to revert.[/yellow]\n"
)

# Use netplan try which handles timeout and revert natively
# Do NOT use capture_output=True as this is an interactive command
try:
result = subprocess.run(
["sudo", "netplan", "try", f"--timeout={timeout_seconds}"],
text=True,
timeout=timeout_seconds + 30,
)

if result.returncode == 0:
console.print("[green]Configuration applied and confirmed![/green]")
return True
else:
console.print("[yellow]Configuration was reverted.[/yellow]")
return False

except subprocess.TimeoutExpired:
console.print("[yellow]Timeout expired, configuration reverted[/yellow]")
return False
except FileNotFoundError:
console.print("[red]netplan command not found. Is Netplan installed?[/red]")
return False
except subprocess.CalledProcessError as e:
console.print(f"[red]Error applying configuration: {e}[/red]")
return False

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, locate and read the file to verify the code matches
find . -name "network_config_validator.py" -type f

Repository: cortexlinux/cortex

Length of output: 98


🏁 Script executed:

# Check if we can read the file and verify the relevant lines
cat -n cortex/network_config_validator.py | sed -n '859,913p'

Repository: cortexlinux/cortex

Length of output: 2699


🌐 Web query:

subprocess.run CalledProcessError check=True Python

💡 Result:

Short explanation

  • subprocess.run(..., check=True) raises subprocess.CalledProcessError if the child exits with a non‑zero return code. The exception contains attributes like returncode, cmd, and (if captured) output and stderr. [1][2]

Example (capture output and handle error)

  • Python:
    try:
    res = subprocess.run(
    ["ls", "nonexistent"],
    check=True,
    capture_output=True,
    text=True
    )
    print("stdout:", res.stdout)
    except subprocess.CalledProcessError as e:
    print("command failed:", e.cmd)
    print("return code:", e.returncode)
    print("stdout:", e.output) # check_output compatibility
    print("stderr:", e.stderr)

Notes

  • Use capture_output=True (or stdout=PIPE/stderr=PIPE) to get output/stderr in the exception. [1]
  • check=True behavior is equivalent to the older check_call/check_output patterns (check_call -> run(..., check=True); check_output -> run(..., check=True, stdout=PIPE)). [1][2]

Sources

  • Python subprocess docs. [1][2]

🌐 Web query:

netplan try exit codes return values

💡 Result:

Short answer: netplan try follows standard UNIX conventions — it returns 0 on success and a non‑zero exit code on error. The netplan docs/manpage do not enumerate special numeric codes for netplan try; errors you see are typically from underlying commands (systemctl, subprocess) and will be the non‑zero return codes those commands produce. [1][2]

Notes / evidence:

  • netplan try documented behavior (no exit‑code table; describes try/revert behavior). [1]
  • Multiple bug reports / tracebacks show netplan raising subprocess.CalledProcessError when systemctl returns non‑zero, i.e. netplan propagates non‑zero return codes from the underlying commands. [2][3]

If you need exact numeric codes for a particular failure, run netplan try and inspect the command's exit code ($?); for root cause check the netplan output and system logs (journalctl) to see which underlying command failed. [1][2]

Sources:
[1] netplan try manpage / documentation.
[2] netplan bug reports / tracebacks showing subprocess.CalledProcessError when systemctl returns non‑zero.


dry_run() catches unreachable CalledProcessError and misclassifies non-zero exits as "reverted".

Because subprocess.run() doesn't use check=True, the subprocess.CalledProcessError handler at line 910 will never execute. Additionally, all non-zero return codes are printed as "Configuration was reverted", which masks actual failures that may differ from timeout revert behavior. Remove the unreachable exception handler and clarify the message for non-zero exits.

Proposed diff
         try:
             result = subprocess.run(
                 ["sudo", "netplan", "try", f"--timeout={timeout_seconds}"],
                 text=True,
                 timeout=timeout_seconds + 30,
             )
 
             if result.returncode == 0:
                 console.print("[green]Configuration applied and confirmed![/green]")
                 return True
-            else:
-                console.print("[yellow]Configuration was reverted.[/yellow]")
-                return False
+            console.print(
+                "[yellow]netplan try exited non-zero (reverted or failed). "
+                "Re-run with netplan debug output to confirm the cause.[/yellow]"
+            )
+            return False
 
         except subprocess.TimeoutExpired:
             console.print("[yellow]Timeout expired, configuration reverted[/yellow]")
             return False
         except FileNotFoundError:
             console.print("[red]netplan command not found. Is Netplan installed?[/red]")
             return False
-        except subprocess.CalledProcessError as e:
-            console.print(f"[red]Error applying configuration: {e}[/red]")
-            return False
🤖 Prompt for AI Agents
In @cortex/network_config_validator.py around lines 859 - 913, The dry_run()
function currently has an unreachable subprocess.CalledProcessError except block
and treats any non-zero result.returncode as "Configuration was reverted", which
hides real errors; remove the subprocess.CalledProcessError handler, and change
the non-zero-exit branch (the else after subprocess.run) to print a clear
failure message that includes result.returncode and any captured output (e.g.,
result.stdout/result.stderr if available) so callers can distinguish an actual
error from a revert; keep the TimeoutExpired and FileNotFoundError handlers
as-is and ensure you do not switch to check=True since netplan try is
interactive.

Copy link
Collaborator

@Anshgrover23 Anshgrover23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeffmarcilliat Follow contributing.md guidelines(i.e PR should contain tests file, docs file, demonstrating video in the pr description itself.) Making it draft until then.

@Anshgrover23 Anshgrover23 marked this pull request as draft January 10, 2026 06:17
@Anshgrover23
Copy link
Collaborator

@divanshu-go is already assigned to this Issue before, Feel free to choose any other Issue, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Network Config Validator (Netplan/NetworkManager)

2 participants