From 8800119b92a1cdc77903c30b8fef3f013ad22c4a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 19 Oct 2026 17:44:42 +0000 Subject: [PATCH 1/4] fix: sourced modules no longer overwrite the caller's SCRIPT_DIR Every module that src/main.sh sources (utils/logging.sh and all of diagnostics/ and repairs/) began by assigning the global SCRIPT_DIR to its own directory. After main.sh sourced utils/logging.sh, SCRIPT_DIR was src/utils, so its next line sourced src/utils/utils/privileges.sh and `network-repair --help` exited 1 on every distribution in the matrix and in Tests. Each module now resolves the directory it needs into a name of its own: NR_UTILS_DIR (always src/utils, whichever module sets it) and, in repairs/routing.sh, NR_REPAIRS_DIR for the two interfaces.sh re-sources its functions do at run time. Those used to work only because repairs/networkmanager.sh happened to be sourced last and left SCRIPT_DIR at src/repairs. Checked locally against main (eed2f99): - `network-repair --help` and `--version`: exit 1 -> exit 0 - tests/run-tests.sh: 20/25 -> 24/25 - tests/integration/test-safe-mode.sh: 2 checks pass -> 9 - `bash -n` on every .sh: clean - shellcheck -S warning on src/: the same two pre-existing findings (SC1010, SC2064), none added Still failing, unrelated to this change: `diagnose` exits 1 under set -e when an interface has no address (diagnostics/interfaces.sh, `ip=$(get_interface_ip ...)`), and test-utils.sh expects COLOR_RED to be non-empty when stdout is not a terminal. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Y6FD63UrriRBHqmhhEca9t --- src/diagnostics/connectivity.sh | 6 +++--- src/diagnostics/dns.sh | 6 +++--- src/diagnostics/firewall.sh | 8 ++++---- src/diagnostics/interfaces.sh | 6 +++--- src/diagnostics/networkmanager.sh | 6 +++--- src/diagnostics/routing.sh | 6 +++--- src/main.sh | 3 ++- src/repairs/dns.sh | 10 +++++----- src/repairs/interfaces.sh | 8 ++++---- src/repairs/networkmanager.sh | 8 ++++---- src/repairs/routing.sh | 15 +++++++++------ src/utils/logging.sh | 4 ++-- 12 files changed, 45 insertions(+), 41 deletions(-) diff --git a/src/diagnostics/connectivity.sh b/src/diagnostics/connectivity.sh index 6d2523b..debafb4 100644 --- a/src/diagnostics/connectivity.sh +++ b/src/diagnostics/connectivity.sh @@ -2,9 +2,9 @@ # Connectivity diagnostics module # Source utilities -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -source "${SCRIPT_DIR}/../utils/logging.sh" -source "${SCRIPT_DIR}/../utils/system.sh" +NR_UTILS_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/../utils" && pwd)" +source "${NR_UTILS_DIR}/logging.sh" +source "${NR_UTILS_DIR}/system.sh" # Test basic connectivity test_basic_connectivity() { diff --git a/src/diagnostics/dns.sh b/src/diagnostics/dns.sh index 51c5f09..3b23a7e 100644 --- a/src/diagnostics/dns.sh +++ b/src/diagnostics/dns.sh @@ -2,9 +2,9 @@ # DNS diagnostics module # Source utilities -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -source "${SCRIPT_DIR}/../utils/logging.sh" -source "${SCRIPT_DIR}/../utils/system.sh" +NR_UTILS_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/../utils" && pwd)" +source "${NR_UTILS_DIR}/logging.sh" +source "${NR_UTILS_DIR}/system.sh" # Check DNS configuration check_dns_config() { diff --git a/src/diagnostics/firewall.sh b/src/diagnostics/firewall.sh index afbbf9a..4cef14e 100644 --- a/src/diagnostics/firewall.sh +++ b/src/diagnostics/firewall.sh @@ -2,10 +2,10 @@ # Firewall diagnostics module # Source utilities -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -source "${SCRIPT_DIR}/../utils/logging.sh" -source "${SCRIPT_DIR}/../utils/system.sh" -source "${SCRIPT_DIR}/../utils/privileges.sh" +NR_UTILS_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/../utils" && pwd)" +source "${NR_UTILS_DIR}/logging.sh" +source "${NR_UTILS_DIR}/system.sh" +source "${NR_UTILS_DIR}/privileges.sh" # Check iptables rules check_iptables() { diff --git a/src/diagnostics/interfaces.sh b/src/diagnostics/interfaces.sh index dd429be..b2fbc46 100644 --- a/src/diagnostics/interfaces.sh +++ b/src/diagnostics/interfaces.sh @@ -2,9 +2,9 @@ # Network interface diagnostics module # Source utilities -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -source "${SCRIPT_DIR}/../utils/logging.sh" -source "${SCRIPT_DIR}/../utils/system.sh" +NR_UTILS_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/../utils" && pwd)" +source "${NR_UTILS_DIR}/logging.sh" +source "${NR_UTILS_DIR}/system.sh" # Check network interfaces check_interfaces() { diff --git a/src/diagnostics/networkmanager.sh b/src/diagnostics/networkmanager.sh index 89b9f7c..3ed6c5d 100644 --- a/src/diagnostics/networkmanager.sh +++ b/src/diagnostics/networkmanager.sh @@ -2,9 +2,9 @@ # NetworkManager diagnostics module # Source utilities -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -source "${SCRIPT_DIR}/../utils/logging.sh" -source "${SCRIPT_DIR}/../utils/system.sh" +NR_UTILS_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/../utils" && pwd)" +source "${NR_UTILS_DIR}/logging.sh" +source "${NR_UTILS_DIR}/system.sh" # Check NetworkManager status check_nm_status() { diff --git a/src/diagnostics/routing.sh b/src/diagnostics/routing.sh index 59ce834..b4cac2f 100644 --- a/src/diagnostics/routing.sh +++ b/src/diagnostics/routing.sh @@ -2,9 +2,9 @@ # Routing diagnostics module # Source utilities -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -source "${SCRIPT_DIR}/../utils/logging.sh" -source "${SCRIPT_DIR}/../utils/system.sh" +NR_UTILS_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/../utils" && pwd)" +source "${NR_UTILS_DIR}/logging.sh" +source "${NR_UTILS_DIR}/system.sh" # Check routing table check_routing_table() { diff --git a/src/main.sh b/src/main.sh index a971913..eba2df7 100755 --- a/src/main.sh +++ b/src/main.sh @@ -5,7 +5,8 @@ set -euo pipefail -# Script directory +# Script directory. The modules sourced below resolve their own directory +# into NR_UTILS_DIR / NR_REPAIRS_DIR and must not assign SCRIPT_DIR. SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" # Version diff --git a/src/repairs/dns.sh b/src/repairs/dns.sh index 174686d..1203142 100644 --- a/src/repairs/dns.sh +++ b/src/repairs/dns.sh @@ -2,11 +2,11 @@ # DNS repair procedures # Source utilities -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -source "${SCRIPT_DIR}/../utils/logging.sh" -source "${SCRIPT_DIR}/../utils/system.sh" -source "${SCRIPT_DIR}/../utils/privileges.sh" -source "${SCRIPT_DIR}/../utils/backup.sh" +NR_UTILS_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/../utils" && pwd)" +source "${NR_UTILS_DIR}/logging.sh" +source "${NR_UTILS_DIR}/system.sh" +source "${NR_UTILS_DIR}/privileges.sh" +source "${NR_UTILS_DIR}/backup.sh" # Repair DNS configuration repair_dns_config() { diff --git a/src/repairs/interfaces.sh b/src/repairs/interfaces.sh index 946e312..c7e50df 100644 --- a/src/repairs/interfaces.sh +++ b/src/repairs/interfaces.sh @@ -2,10 +2,10 @@ # Network interface repair procedures # Source utilities -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -source "${SCRIPT_DIR}/../utils/logging.sh" -source "${SCRIPT_DIR}/../utils/system.sh" -source "${SCRIPT_DIR}/../utils/privileges.sh" +NR_UTILS_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/../utils" && pwd)" +source "${NR_UTILS_DIR}/logging.sh" +source "${NR_UTILS_DIR}/system.sh" +source "${NR_UTILS_DIR}/privileges.sh" # Bring up network interface interface_up() { diff --git a/src/repairs/networkmanager.sh b/src/repairs/networkmanager.sh index af850fe..55a982c 100644 --- a/src/repairs/networkmanager.sh +++ b/src/repairs/networkmanager.sh @@ -2,10 +2,10 @@ # NetworkManager repair procedures # Source utilities -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -source "${SCRIPT_DIR}/../utils/logging.sh" -source "${SCRIPT_DIR}/../utils/system.sh" -source "${SCRIPT_DIR}/../utils/privileges.sh" +NR_UTILS_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/../utils" && pwd)" +source "${NR_UTILS_DIR}/logging.sh" +source "${NR_UTILS_DIR}/system.sh" +source "${NR_UTILS_DIR}/privileges.sh" # Restart NetworkManager restart_networkmanager() { diff --git a/src/repairs/routing.sh b/src/repairs/routing.sh index deee95d..551b594 100644 --- a/src/repairs/routing.sh +++ b/src/repairs/routing.sh @@ -2,10 +2,13 @@ # Routing repair procedures # Source utilities -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -source "${SCRIPT_DIR}/../utils/logging.sh" -source "${SCRIPT_DIR}/../utils/system.sh" -source "${SCRIPT_DIR}/../utils/privileges.sh" +NR_UTILS_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/../utils" && pwd)" +# Resolved now: the functions below source interfaces.sh after other +# modules have loaded. +NR_REPAIRS_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "${NR_UTILS_DIR}/logging.sh" +source "${NR_UTILS_DIR}/system.sh" +source "${NR_UTILS_DIR}/privileges.sh" # Add default route add_default_route() { @@ -134,7 +137,7 @@ repair_default_route() { log_step "Attempting to obtain gateway via DHCP" # Renew DHCP which should set up routes - source "${SCRIPT_DIR}/interfaces.sh" + source "${NR_REPAIRS_DIR}/interfaces.sh" if renew_dhcp "${primary}"; then sleep 2 @@ -171,7 +174,7 @@ flush_routing_table() { log_step "Rebuilding routes via DHCP" # Renew DHCP to rebuild routes - source "${SCRIPT_DIR}/interfaces.sh" + source "${NR_REPAIRS_DIR}/interfaces.sh" if renew_dhcp "${primary}"; then sleep 3 diff --git a/src/utils/logging.sh b/src/utils/logging.sh index 7f5b030..73efe5f 100644 --- a/src/utils/logging.sh +++ b/src/utils/logging.sh @@ -2,8 +2,8 @@ # Logging utilities for network repair tool # Source color utilities -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -source "${SCRIPT_DIR}/colors.sh" +NR_UTILS_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "${NR_UTILS_DIR}/colors.sh" # Log levels declare -gA LOG_LEVELS=( From 45da73dac990c44848e62c6f563df2b37418886c Mon Sep 17 00:00:00 2001 From: "coderabbitai[bot]" <136622811+coderabbitai[bot]@users.noreply.github.com> Date: Mon, 19 Oct 2026 20:30:32 +0000 Subject: [PATCH 2/4] docs(routing): clarify repair return statuses and side effects --- src/repairs/routing.sh | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/repairs/routing.sh b/src/repairs/routing.sh index 551b594..ac553d7 100644 --- a/src/repairs/routing.sh +++ b/src/repairs/routing.sh @@ -92,7 +92,16 @@ remove_duplicate_routes() { return 0 } -# Repair default route +# Remove duplicate default routes if the initial route query succeeds; +# otherwise try renewing DHCP on the primary interface. +# Returns 0 when the initial `ip route show default` succeeds, even if its +# output is empty or the gateway is unreachable. The DHCP fallback returns 0 +# only if renewal and a subsequent route query succeed; otherwise it returns 1, +# including when no primary interface is found. +# May remove routes and release/renew a DHCP lease. Requests root/sudo access +# and exits the shell if access cannot be obtained. With errexit active, +# unguarded failures such as interface lookup or sourcing helpers can terminate +# the caller before a return status is produced. repair_default_route() { log_section "Repair Default Route" @@ -151,7 +160,15 @@ repair_default_route() { return 1 } -# Flush and rebuild routing table +# Flush the main routing table and try to rebuild routes by renewing DHCP on +# the primary interface selected before the flush. May disconnect the network +# and release/renew a DHCP lease; no rollback is performed on failure. +# Returns 0 if DHCP renewal and the subsequent `ip route show default` succeed, +# even if the query output is empty. Returns 1 if no primary interface is found, +# renewal fails or the final query fails. +# Requests root/sudo access and exits the shell if access cannot be obtained. +# With errexit active, unguarded failures such as interface lookup, flushing +# routes or sourcing helpers can terminate the caller before returning. flush_routing_table() { log_section "Flush and Rebuild Routing Table" From 7895d9c3794b2804565ce7cc9cd15d33def88aa3 Mon Sep 17 00:00:00 2001 From: "coderabbitai[bot]" <136622811+coderabbitai[bot]@users.noreply.github.com> Date: Mon, 19 Oct 2026 20:34:42 +0000 Subject: [PATCH 3/4] fix(shell): handle diagnostic failures under strict mode and support nonterminal tests --- src/diagnostics/connectivity.sh | 2 +- src/diagnostics/interfaces.sh | 2 +- src/main.sh | 20 +++++++------------- src/utils/privileges.sh | 2 +- tests/run-tests.sh | 8 +++++--- tests/test-utils.sh | 18 ++++++++++++++---- 6 files changed, 29 insertions(+), 23 deletions(-) diff --git a/src/diagnostics/connectivity.sh b/src/diagnostics/connectivity.sh index debafb4..fb8f9f3 100644 --- a/src/diagnostics/connectivity.sh +++ b/src/diagnostics/connectivity.sh @@ -167,7 +167,7 @@ test_mtu() { # Subtract 28 bytes for IP + ICMP header local packet_size=$((size - 28)) - if ping -c 1 -W 2 -M do -s ${packet_size} 8.8.8.8 >/dev/null 2>&1; then + if ping -c 1 -W 2 -M 'do' -s ${packet_size} 8.8.8.8 >/dev/null 2>&1; then log_success " Packet size ${size} bytes: OK" break else diff --git a/src/diagnostics/interfaces.sh b/src/diagnostics/interfaces.sh index b2fbc46..788122d 100644 --- a/src/diagnostics/interfaces.sh +++ b/src/diagnostics/interfaces.sh @@ -187,7 +187,7 @@ check_interface_conflicts() { ip=$(get_interface_ip "${iface}") if [[ -n "${ip}" ]]; then - if [[ -n "${ip_map[${ip}]}" ]]; then + if [[ -n "${ip_map[${ip}]:-}" ]]; then log_error " Duplicate IP ${ip} on ${iface} and ${ip_map[${ip}]}" issues=$((issues + 1)) else diff --git a/src/main.sh b/src/main.sh index eba2df7..20fce16 100755 --- a/src/main.sh +++ b/src/main.sh @@ -124,29 +124,23 @@ run_all_diagnostics() { log_info "Network Manager: $(detect_network_manager)" echo "" - # Run diagnostics - diagnose_interfaces - total_issues=$((total_issues + $?)) + # Diagnostic failures report issues; collect them without triggering errexit. + diagnose_interfaces || total_issues=$((total_issues + $?)) echo "" - diagnose_routing - total_issues=$((total_issues + $?)) + diagnose_routing || total_issues=$((total_issues + $?)) echo "" - diagnose_dns - total_issues=$((total_issues + $?)) + diagnose_dns || total_issues=$((total_issues + $?)) echo "" - diagnose_connectivity - total_issues=$((total_issues + $?)) + diagnose_connectivity || total_issues=$((total_issues + $?)) echo "" - diagnose_firewall - total_issues=$((total_issues + $?)) + diagnose_firewall || total_issues=$((total_issues + $?)) echo "" - diagnose_networkmanager - total_issues=$((total_issues + $?)) + diagnose_networkmanager || total_issues=$((total_issues + $?)) echo "" # Summary diff --git a/src/utils/privileges.sh b/src/utils/privileges.sh index 4ca6a64..3f28093 100644 --- a/src/utils/privileges.sh +++ b/src/utils/privileges.sh @@ -43,7 +43,7 @@ require_root() { SUDO_KEEPER_PID=$! # Kill sudo keeper on exit - trap "kill ${SUDO_KEEPER_PID} 2>/dev/null || true" EXIT + trap 'kill "${SUDO_KEEPER_PID}" 2>/dev/null || true' EXIT } # Run command with appropriate privileges diff --git a/tests/run-tests.sh b/tests/run-tests.sh index cb2ff0c..c9a2ec7 100755 --- a/tests/run-tests.sh +++ b/tests/run-tests.sh @@ -252,9 +252,11 @@ test_safe_mode() { print_result "FAIL" "Repairs should be blocked in safe mode" fi - # Test that diagnose works without flags - if "${PROJECT_DIR}/network-repair" diagnose >/dev/null 2>&1 || \ - "${PROJECT_DIR}/network-repair" diagnose 2>&1 | grep -q "Diagnostic"; then + # Diagnose returns 1 for network issues, but must complete in safe mode. + # Capture once so pipefail and grep closing early cannot mask the result. + local diagnose_status=0 + output=$("${PROJECT_DIR}/network-repair" diagnose 2>&1) || diagnose_status=$? + if [[ ${diagnose_status} -le 1 && "${output}" == *"=== Diagnostic Summary ==="* ]]; then print_result "PASS" "Diagnose works in safe mode" else print_result "FAIL" "Diagnose should work in safe mode" diff --git a/tests/test-utils.sh b/tests/test-utils.sh index 98cd1fe..323c6bb 100755 --- a/tests/test-utils.sh +++ b/tests/test-utils.sh @@ -15,10 +15,20 @@ source "${PROJECT_DIR}/src/utils/system.sh" test_colors() { echo "Testing color utilities..." - # Test that color variables are defined - [[ -n "${COLOR_RED}" ]] || { echo "COLOR_RED not defined"; exit 1; } - [[ -n "${COLOR_GREEN}" ]] || { echo "COLOR_GREEN not defined"; exit 1; } - [[ -n "${COLOR_RESET}" ]] || { echo "COLOR_RESET not defined"; exit 1; } + # Colors are defined but may be empty when stdout is not a terminal. + [[ "${COLOR_RED+x}" == x ]] || { echo "COLOR_RED not defined"; exit 1; } + [[ "${COLOR_GREEN+x}" == x ]] || { echo "COLOR_GREEN not defined"; exit 1; } + [[ "${COLOR_RESET+x}" == x ]] || { echo "COLOR_RESET not defined"; exit 1; } + + # Explicitly disabled colors must stay empty regardless of terminal support. + ( + USE_COLORS=never + source "${PROJECT_DIR}/src/utils/colors.sh" + [[ -z "${COLOR_RED}${COLOR_GREEN}${COLOR_RESET}" ]] || { + echo "Colors should be empty when disabled" + exit 1 + } + ) echo "✓ Color utilities work" } From 4270e56b86c6c9139747c8b267c0ff1e6dd2ef96 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 19 Oct 2026 21:39:46 +0000 Subject: [PATCH 4/4] test: sourcing a module must leave the caller's SCRIPT_DIR alone test-utils.sh now sources logging.sh and every diagnostics and repairs module in a fresh shell, started from /, with SCRIPT_DIR preset, and fails if any module changes it. That is the bug this PR fixes, so the fix now has a test: on main the test fails at logging.sh. The test is the one CodeRabbit wrote for the same bug on #78 (fa99baa), moved here, where the fix lives. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Y6FD63UrriRBHqmhhEca9t --- tests/test-utils.sh | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/test-utils.sh b/tests/test-utils.sh index 323c6bb..4b439e4 100755 --- a/tests/test-utils.sh +++ b/tests/test-utils.sh @@ -70,6 +70,26 @@ test_system() { echo "✓ System utilities work" } +# Test that sourcing a module leaves the caller's SCRIPT_DIR alone. Each module +# loads in a fresh shell started from an unrelated directory. +test_module_loading() { + echo "Testing module loading..." + + local module + for module in "${PROJECT_DIR}/src/utils/logging.sh" \ + "${PROJECT_DIR}"/src/diagnostics/*.sh \ + "${PROJECT_DIR}"/src/repairs/*.sh; do + bash -euc ' + cd / + SCRIPT_DIR=caller-directory + source "$1" + [[ "${SCRIPT_DIR}" == caller-directory ]] + ' _ "${module}" || { echo "Sourcing ${module} changed SCRIPT_DIR"; exit 1; } + done + + echo "✓ Modules leave the caller's SCRIPT_DIR alone" +} + # Run all tests main() { echo "Running utility function tests..." @@ -78,6 +98,7 @@ main() { test_colors test_logging test_system + test_module_loading echo "" echo "All utility tests passed!"