Code review findings: Future improvements for session management and config loading

[mischadiehm]

Context

During code review of PR #31 (code cleanup for multi-inventory feature), several valid improvement suggestions were identified that don't block the current PR but should be addressed in future work.

Findings from Security Review

1. Exception logging may expose sensitive info (Medium Priority)

Location: src/network_toolkit/api/run.py:259-262

logger.debug(
    "Session for %s failed, attempting fresh connection: %s",
    device_name,
    first_error,  # Could contain credential info in some error types
)

Risk: If first_error contains credential information (e.g., from certain auth failures), it will be logged.

Recommendation: Log only exception type, not full message:

logger.debug(
    "Session for %s failed, attempting fresh connection: %s",
    device_name,
    type(first_error).__name__,
)

2. .env file permission checks (Medium Priority)

Location: src/network_toolkit/config.py:77-114

The load_dotenv_files() function doesn't verify that .env files have secure permissions (should be 0600).

Recommendation: Add permission check on Unix-like systems:

if hasattr(os, 'stat') and platform.system() != 'Windows':
    stat_info = env_file.stat()
    if stat_info.st_mode & 0o077:
        logging.warning("Insecure permissions on .env file")

Findings from Code Quality Review

3. Redundant logging in session pool (Low Priority)

Location: src/network_toolkit/session_pool.py:91-108

Currently logs both individual failures AND a summary. Consider picking one to reduce log noise.

4. Redundant logging before exceptions (Low Priority)

Location: src/network_toolkit/inventory/nornir_simple.py:504-510, 539-542

Warning logs contain the same information that's already in the ConfigurationError details. The exception will be logged by the caller, making the warning redundant.

Recommendation: Consider using logger.debug() instead of logger.warning() for detailed information.

5. Missing test coverage (Low Priority)

Location: tests/test_session_pool.py

Missing tests for:

• Session staleness detection
• Retry behavior when using sessions from the pool
• What happens when a session is in-use while another thread calls close_all()
• Integration test showing actual use case: multiple devices, parallel execution, session reuse

6. Config refactor incomplete (Low Priority)

Location: src/network_toolkit/config.py

The main load_modular_config() function is still ~300+ lines after extracting helpers. The _compile_one() nested function (lines 1385-1486) could be extracted, but it modifies closure state heavily.

Recommendation: Consider restructuring to use a builder pattern or dataclass to pass state, allowing full extraction.

Priority Summary

Issue	Priority	Effort
Exception message sanitization	Medium	Low
.env permission checks	Medium	Low
Redundant logging cleanup	Low	Low
Additional test coverage	Low	Medium
Further config refactoring	Low	High

Labels

• tech-debt
• security
• good-first-issue (for logging cleanup items)