From b97633e1e7303f7b5e90837cfdf2b98729281b27 Mon Sep 17 00:00:00 2001
From: Oliver Boehmer <oboehmer@cisco.com>
Date: Sun, 22 Mar 2026 19:42:22 +0100
Subject: [PATCH 01/11] refactor(tests): remove fragile string-based config
 tests and improve credential check

- Remove 3 moot unit tests that checked config strings (fragile, not behavioral)
- Remove duplicate integration test for config creation failure
- Move credential sentinel test imports to module level
- Run credential check for all scenarios, not just PyATS
- Improve error message for PyATS config creation failure
---
 nac_test/pyats_core/orchestrator.py           |  6 +++-
 tests/e2e/test_e2e_scenarios.py               | 10 ++----
 .../test_subprocess_runner_integration.py     | 32 ++---------------
 .../execution/test_subprocess_runner.py       | 34 -------------------
 4 files changed, 9 insertions(+), 73 deletions(-)

diff --git a/nac_test/pyats_core/orchestrator.py b/nac_test/pyats_core/orchestrator.py
index cb6f425d..d17b6ecb 100644
--- a/nac_test/pyats_core/orchestrator.py
+++ b/nac_test/pyats_core/orchestrator.py
@@ -629,7 +629,11 @@ async def _run_tests_async(self) -> PyATSResults:
             )
         except ConfigFileCreationError as e:
             error_msg = str(e)
-            print(terminal.error(f"PyATS initialization failed: {error_msg}"))
+            print(
+                terminal.error(
+                    f"Fatal error: PyATS config creation failed: {error_msg}"
+                )
+            )
             api_result = TestResults.from_error(error_msg) if api_tests else None
             d2d_result = TestResults.from_error(error_msg) if d2d_tests else None
             return PyATSResults(api=api_result, d2d=d2d_result)
diff --git a/tests/e2e/test_e2e_scenarios.py b/tests/e2e/test_e2e_scenarios.py
index a82af3b0..6da15ca4 100644
--- a/tests/e2e/test_e2e_scenarios.py
+++ b/tests/e2e/test_e2e_scenarios.py
@@ -20,6 +20,7 @@
 import logging
 import re
 import xml.etree.ElementTree as ET
+import zipfile
 
 import pytest
 
@@ -39,7 +40,7 @@
 )
 from nac_test.robot.reporting.robot_output_parser import RobotResultParser
 from tests.conftest import assert_is_link_to
-from tests.e2e.conftest import E2EResults
+from tests.e2e.conftest import TEST_CREDENTIAL_SENTINEL, E2EResults
 from tests.e2e.html_helpers import (
     assert_combined_stats,
     assert_report_stats,
@@ -960,13 +961,6 @@ def test_no_credentials_in_output_artifacts(self, results: E2EResults) -> None:
         Regression test for #689 - EnvironmentDebugPlugin was exposing env vars
         including passwords in env.txt files within PyATS archives.
         """
-        import zipfile
-
-        from tests.e2e.conftest import TEST_CREDENTIAL_SENTINEL
-
-        if not results.has_pyats_results:
-            pytest.skip("No PyATS results in this scenario")
-
         offending_files: list[str] = []
 
         for file_path in results.output_dir.rglob("*"):
diff --git a/tests/pyats_core/execution/test_subprocess_runner_integration.py b/tests/pyats_core/execution/test_subprocess_runner_integration.py
index c2913d09..d7687dcb 100644
--- a/tests/pyats_core/execution/test_subprocess_runner_integration.py
+++ b/tests/pyats_core/execution/test_subprocess_runner_integration.py
@@ -6,9 +6,8 @@
 Tests the subprocess execution logic:
 1. Config file content verification (git_info = false for macOS fork() safety)
 2. Command construction includes all required PyATS flags
-3. Error handling when config file creation fails
-4. Return code interpretation (0 = success, 1 = test failures, >1 = error)
-5. Output processing and progress event parsing
+3. Return code interpretation (0 = success, 1 = test failures, >1 = error)
+4. Output processing and progress event parsing
 
 See tests/unit/pyats_core/execution/test_subprocess_runner.py for
 additional unit tests covering subprocess crash handling,
@@ -23,7 +22,6 @@
 import pytest
 
 from nac_test.pyats_core.execution.subprocess_runner import (
-    ConfigFileCreationError,
     SubprocessRunner,
 )
 
@@ -223,32 +221,6 @@ def test_execute_job_with_testbed_includes_testbed_and_config_flags(
         assert "--pyats-configuration" in cmd
 
 
-class TestConfigCreationFailure:
-    """Tests error handling when config file creation fails."""
-
-    def test_init_raises_config_file_creation_error_on_failure(
-        self, tmp_path: Path
-    ) -> None:
-        """Verify __init__ raises ConfigFileCreationError when config file write fails.
-
-        Config files are now created in __init__() rather than execute_job().
-        If creation fails, we raise immediately rather than proceeding with
-        execution that would use default PyATS settings (causing fork() crashes on macOS).
-        """
-        with (
-            patch("sysconfig.get_path", return_value=str(tmp_path)),
-            patch.object(Path, "exists", return_value=True),
-            patch.object(Path, "write_text", side_effect=OSError("disk full")),
-        ):
-            with pytest.raises(ConfigFileCreationError) as exc_info:
-                SubprocessRunner(
-                    output_dir=tmp_path,
-                    output_handler=lambda line: None,
-                )
-
-            assert "disk full" in str(exc_info.value)
-
-
 class TestReturnCodeHandling:
     """Tests subprocess return code interpretation."""
 
diff --git a/tests/unit/pyats_core/execution/test_subprocess_runner.py b/tests/unit/pyats_core/execution/test_subprocess_runner.py
index 7b1fc92c..6f25d8c0 100644
--- a/tests/unit/pyats_core/execution/test_subprocess_runner.py
+++ b/tests/unit/pyats_core/execution/test_subprocess_runner.py
@@ -28,8 +28,6 @@
     PYATS_PLUGIN_CONFIG_FILENAME,
 )
 from nac_test.pyats_core.execution.subprocess_runner import (
-    PLUGIN_CONFIG,
-    PYATS_CONFIG,
     ConfigFileCreationError,
     SubprocessRunner,
 )
@@ -327,35 +325,6 @@ def test_build_command_includes_config_files(
     assert cmd[pyats_config_idx + 1] == str(runner._pyats_config_file)
 
 
-# --- Config constants tests (security and configuration) ---
-
-
-def test_plugin_config_disables_environment_debug_plugin() -> None:
-    """Test that PLUGIN_CONFIG disables EnvironmentDebugPlugin to prevent credential exposure.
-
-    This is a security requirement (issue #689) - the EnvironmentDebugPlugin writes
-    all environment variables (including credentials) to env.txt files in archives.
-    """
-    assert "EnvironmentDebugPlugin:" in PLUGIN_CONFIG
-    assert "enabled: False" in PLUGIN_CONFIG
-
-
-def test_plugin_config_enables_progress_reporter_plugin() -> None:
-    """Test that PLUGIN_CONFIG enables ProgressReporterPlugin for real-time progress."""
-    assert "ProgressReporterPlugin:" in PLUGIN_CONFIG
-    assert "enabled: True" in PLUGIN_CONFIG
-    assert "module: nac_test.pyats_core.progress.plugin" in PLUGIN_CONFIG
-
-
-def test_pyats_config_disables_git_info() -> None:
-    """Test that PYATS_CONFIG disables git_info to prevent macOS fork() crashes.
-
-    This prevents CoreFoundation lock corruption in get_git_info() on macOS with
-    Python 3.12+.
-    """
-    assert "git_info = false" in PYATS_CONFIG
-
-
 def test_init_creates_config_files_in_output_dir(
     temp_output_dir: Path, mock_output_handler: Mock
 ) -> None:
@@ -372,9 +341,6 @@ def test_init_creates_config_files_in_output_dir(
     assert runner._plugin_config_file.name == PYATS_PLUGIN_CONFIG_FILENAME
     assert runner._pyats_config_file.name == PYATS_CONFIG_FILENAME
 
-    assert runner._plugin_config_file.read_text() == PLUGIN_CONFIG
-    assert runner._pyats_config_file.read_text() == PYATS_CONFIG
-
 
 def test_init_raises_config_file_creation_error_on_write_failure(
     temp_output_dir: Path, mock_output_handler: Mock

From 208f60dccbbc682f603fbd29cc77afeec9561179 Mon Sep 17 00:00:00 2001
From: Oliver Boehmer <oboehmer@cisco.com>
Date: Sun, 22 Mar 2026 23:13:18 +0100
Subject: [PATCH 02/11] test(pyats): add test for ConfigFileCreationError
 handling in PyATSOrchestrator

Verify that when SubprocessRunner fails to create config files (e.g., disk full),
PyATSOrchestrator returns PyATSResults with from_error() for discovered test types.
---
 .../test_orchestrator_config_error.py         | 73 +++++++++++++++++++
 1 file changed, 73 insertions(+)
 create mode 100644 tests/pyats_core/test_orchestrator_config_error.py

diff --git a/tests/pyats_core/test_orchestrator_config_error.py b/tests/pyats_core/test_orchestrator_config_error.py
new file mode 100644
index 00000000..c5600070
--- /dev/null
+++ b/tests/pyats_core/test_orchestrator_config_error.py
@@ -0,0 +1,73 @@
+# SPDX-License-Identifier: MPL-2.0
+# Copyright (c) 2025 Daniel Schmidt
+
+"""Tests for PyATSOrchestrator handling of ConfigFileCreationError."""
+
+from pathlib import Path
+from unittest.mock import patch
+
+import pytest
+
+from nac_test.pyats_core.orchestrator import PyATSOrchestrator
+
+from .conftest import PyATSTestDirs
+
+
+class TestOrchestratorConfigFileCreationError:
+    """Tests for ConfigFileCreationError handling in PyATSOrchestrator."""
+
+    @pytest.mark.parametrize(
+        ("has_api", "has_d2d"),
+        [
+            (True, False),
+            (False, True),
+            (True, True),
+        ],
+    )
+    def test_config_file_creation_error_returns_from_error_results(
+        self,
+        aci_controller_env: None,
+        pyats_test_dirs: PyATSTestDirs,
+        has_api: bool,
+        has_d2d: bool,
+    ) -> None:
+        """OSError in SubprocessRunner._create_config_files returns from_error results."""
+        api_tests = [Path("/fake/tests/api/test_one.py")] if has_api else []
+        d2d_tests = [Path("/fake/tests/d2d/test_two.py")] if has_d2d else []
+
+        orchestrator = PyATSOrchestrator(
+            data_paths=[pyats_test_dirs.output_dir.parent / "data"],
+            test_dir=pyats_test_dirs.test_dir,
+            output_dir=pyats_test_dirs.output_dir,
+            merged_data_filename="merged.yaml",
+        )
+
+        with (
+            patch.object(
+                orchestrator.test_discovery, "discover_pyats_tests"
+            ) as mock_discover,
+            patch.object(
+                orchestrator.test_discovery, "categorize_tests_by_type"
+            ) as mock_categorize,
+            patch.object(orchestrator, "validate_environment"),
+            patch.object(Path, "write_text", side_effect=OSError("disk full")),
+        ):
+            mock_discover.return_value = (api_tests + d2d_tests, [])
+            mock_categorize.return_value = (api_tests, d2d_tests)
+            result = orchestrator.run_tests()
+
+        if has_api:
+            assert result.api is not None
+            assert result.api.has_error is True
+            assert result.api.reason is not None
+            assert "disk full" in result.api.reason
+        else:
+            assert result.api is None
+
+        if has_d2d:
+            assert result.d2d is not None
+            assert result.d2d.has_error is True
+            assert result.d2d.reason is not None
+            assert "disk full" in result.d2d.reason
+        else:
+            assert result.d2d is None

From 265d9c32fd4036a0eadcc58894b11285a4135399 Mon Sep 17 00:00:00 2001
From: Oliver Boehmer <oboehmer@cisco.com>
Date: Mon, 23 Mar 2026 20:35:36 +0100
Subject: [PATCH 03/11] refactor(pyats): replace ConfigFileCreationError with
 RuntimeError, fix cleanup leak

- Delete ConfigFileCreationError; _create_config_files now raises
  RuntimeError, consistent with the pyats-not-found failure in __init__
- Catch RuntimeError in orchestrator so both init failures populate
  api/d2d results correctly instead of falling through to the broad
  except-Exception handler
- Call subprocess_runner.cleanup() in the report-generation failure
  branch so config files are removed regardless of outcome
- Assert EnvironmentDebugPlugin disabled in plugin config integration
  test using regex; convert all config-content checks to re.search
- Remove leftover try/finally unlink() blocks in TestConfigFileContent
  (files live in tmp_path, cleaned up by pytest)
---
 .../pyats_core/execution/subprocess_runner.py | 12 ++------
 nac_test/pyats_core/orchestrator.py           | 18 ++++++------
 .../test_subprocess_runner_integration.py     | 28 +++++++------------
 .../test_orchestrator_config_error.py         | 10 +++----
 .../execution/test_subprocess_runner.py       |  9 +++---
 5 files changed, 29 insertions(+), 48 deletions(-)

diff --git a/nac_test/pyats_core/execution/subprocess_runner.py b/nac_test/pyats_core/execution/subprocess_runner.py
index 13aa1b34..ff1c9cca 100644
--- a/nac_test/pyats_core/execution/subprocess_runner.py
+++ b/nac_test/pyats_core/execution/subprocess_runner.py
@@ -27,12 +27,6 @@
 logger = logging.getLogger(__name__)
 
 
-class ConfigFileCreationError(Exception):
-    """Raised when PyATS config files cannot be created."""
-
-    pass
-
-
 PLUGIN_CONFIG = textwrap.dedent("""\
     plugins:
         ProgressReporterPlugin:
@@ -82,7 +76,7 @@ def _create_config_files(self) -> None:
         """Create config files for PyATS execution in the output directory.
 
         Raises:
-            ConfigFileCreationError: If file creation fails
+            RuntimeError: If file creation fails
         """
         plugin_config_file = self.output_dir / PYATS_PLUGIN_CONFIG_FILENAME
         pyats_config_file = self.output_dir / PYATS_CONFIG_FILENAME
@@ -91,9 +85,7 @@ def _create_config_files(self) -> None:
             plugin_config_file.write_text(PLUGIN_CONFIG)
             pyats_config_file.write_text(PYATS_CONFIG)
         except OSError as e:
-            raise ConfigFileCreationError(
-                f"Failed to create PyATS config files: {e}"
-            ) from e
+            raise RuntimeError(f"Failed to create PyATS config files: {e}") from e
 
         self._plugin_config_file = plugin_config_file
         self._pyats_config_file = pyats_config_file
diff --git a/nac_test/pyats_core/orchestrator.py b/nac_test/pyats_core/orchestrator.py
index d365e95b..aee61df9 100644
--- a/nac_test/pyats_core/orchestrator.py
+++ b/nac_test/pyats_core/orchestrator.py
@@ -36,7 +36,6 @@
 )
 from nac_test.pyats_core.execution.device import DeviceExecutor
 from nac_test.pyats_core.execution.device.testbed_generator import TestbedGenerator
-from nac_test.pyats_core.execution.subprocess_runner import ConfigFileCreationError
 from nac_test.pyats_core.progress import ProgressReporter
 from nac_test.pyats_core.reporting.multi_archive_generator import (
     MultiArchiveReportGenerator,
@@ -622,13 +621,9 @@ async def _run_tests_async(self) -> PyATSResults:
                 output_handler=self.output_processor.process_line,
                 loglevel=self.loglevel,
             )
-        except ConfigFileCreationError as e:
+        except RuntimeError as e:
+            # pyats entrypoint not found or configfile creation failed
             error_msg = str(e)
-            print(
-                terminal.error(
-                    f"Fatal error: PyATS config creation failed: {error_msg}"
-                )
-            )
             api_result = TestResults.from_error(error_msg) if api_tests else None
             d2d_result = TestResults.from_error(error_msg) if d2d_tests else None
             return PyATSResults(api=api_result, d2d=d2d_result)
@@ -758,6 +753,9 @@ async def _generate_html_reports_async(
         )
         result = await generator.generate_reports_from_archives(archive_paths)
 
+        # Determine whether to keep artifacts (debug mode or explicit env var)
+        keep_artifacts = DEBUG_MODE or os.environ.get("NAC_TEST_PYATS_KEEP_REPORT_DATA")
+
         if result["status"] in ["success", "partial"]:
             # Log report generation timing (procedural info)
             duration_str = format_duration(result["duration"])
@@ -786,9 +784,6 @@ async def _generate_html_reports_async(
 
             # Clean up archives after successful extraction and report generation
             # (unless in debug mode or user wants to keep data)
-            keep_artifacts = DEBUG_MODE or os.environ.get(
-                "NAC_TEST_PYATS_KEEP_REPORT_DATA"
-            )
             if not keep_artifacts:
                 for archive_path in archive_paths:
                     try:
@@ -831,4 +826,7 @@ async def _generate_html_reports_async(
             print(f"\n{terminal.error('Failed to generate reports')}")
             if result.get("error"):
                 print(f"Error: {result['error']}")
+            # Clean up PyATS config files; report generation failed so no stats to return
+            if self.subprocess_runner and not keep_artifacts:
+                self.subprocess_runner.cleanup()
             return PyATSResults()
diff --git a/tests/pyats_core/execution/test_subprocess_runner_integration.py b/tests/pyats_core/execution/test_subprocess_runner_integration.py
index d7687dcb..57569510 100644
--- a/tests/pyats_core/execution/test_subprocess_runner_integration.py
+++ b/tests/pyats_core/execution/test_subprocess_runner_integration.py
@@ -15,6 +15,7 @@
 """
 
 import asyncio
+import re
 from pathlib import Path
 from typing import Any
 from unittest.mock import AsyncMock, patch
@@ -124,12 +125,9 @@ def test_execute_job_writes_git_info_false_to_pyats_config(
         config_idx = cmd.index("--pyats-configuration")
         config_path = Path(cmd[config_idx + 1])
 
-        try:
-            content = config_path.read_text()
-            assert "[report]" in content
-            assert "git_info = false" in content
-        finally:
-            config_path.unlink(missing_ok=True)
+        content = config_path.read_text()
+        assert re.search(r"\[report\]", content)
+        assert re.search(r"git_info\s*=\s*false", content)
 
     def test_execute_job_with_testbed_writes_git_info_false_to_pyats_config(
         self, runner: SubprocessRunner
@@ -149,12 +147,9 @@ def test_execute_job_with_testbed_writes_git_info_false_to_pyats_config(
         config_idx = cmd.index("--pyats-configuration")
         config_path = Path(cmd[config_idx + 1])
 
-        try:
-            content = config_path.read_text()
-            assert "[report]" in content
-            assert "git_info = false" in content
-        finally:
-            config_path.unlink(missing_ok=True)
+        content = config_path.read_text()
+        assert re.search(r"\[report\]", content)
+        assert re.search(r"git_info\s*=\s*false", content)
 
     def test_execute_job_writes_plugin_config_with_progress_reporter(
         self, runner: SubprocessRunner
@@ -167,12 +162,9 @@ def test_execute_job_writes_plugin_config_with_progress_reporter(
         config_idx = cmd.index("--configuration")
         config_path = Path(cmd[config_idx + 1])
 
-        try:
-            content = config_path.read_text()
-            assert "ProgressReporterPlugin" in content
-            assert "enabled: True" in content
-        finally:
-            config_path.unlink(missing_ok=True)
+        content = config_path.read_text()
+        assert re.search(r"ProgressReporterPlugin:\s+enabled:\s+True", content)
+        assert re.search(r"EnvironmentDebugPlugin:\s+enabled:\s+False", content)
 
 
 class TestCommandConstruction:
diff --git a/tests/pyats_core/test_orchestrator_config_error.py b/tests/pyats_core/test_orchestrator_config_error.py
index c5600070..493e681b 100644
--- a/tests/pyats_core/test_orchestrator_config_error.py
+++ b/tests/pyats_core/test_orchestrator_config_error.py
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: MPL-2.0
 # Copyright (c) 2025 Daniel Schmidt
 
-"""Tests for PyATSOrchestrator handling of ConfigFileCreationError."""
+"""Tests for PyATSOrchestrator handling of SubprocessRunner init failures."""
 
 from pathlib import Path
 from unittest.mock import patch
@@ -13,8 +13,8 @@
 from .conftest import PyATSTestDirs
 
 
-class TestOrchestratorConfigFileCreationError:
-    """Tests for ConfigFileCreationError handling in PyATSOrchestrator."""
+class TestOrchestratorSubprocessRunnerInitError:
+    """Tests for RuntimeError handling in PyATSOrchestrator when SubprocessRunner cannot initialize."""
 
     @pytest.mark.parametrize(
         ("has_api", "has_d2d"),
@@ -24,14 +24,14 @@ class TestOrchestratorConfigFileCreationError:
             (True, True),
         ],
     )
-    def test_config_file_creation_error_returns_from_error_results(
+    def test_subprocess_runner_init_error_returns_from_error_results(
         self,
         aci_controller_env: None,
         pyats_test_dirs: PyATSTestDirs,
         has_api: bool,
         has_d2d: bool,
     ) -> None:
-        """OSError in SubprocessRunner._create_config_files returns from_error results."""
+        """OSError in SubprocessRunner._create_config_files raises RuntimeError, returns from_error results."""
         api_tests = [Path("/fake/tests/api/test_one.py")] if has_api else []
         d2d_tests = [Path("/fake/tests/d2d/test_two.py")] if has_d2d else []
 
diff --git a/tests/unit/pyats_core/execution/test_subprocess_runner.py b/tests/unit/pyats_core/execution/test_subprocess_runner.py
index 6f25d8c0..fec7d989 100644
--- a/tests/unit/pyats_core/execution/test_subprocess_runner.py
+++ b/tests/unit/pyats_core/execution/test_subprocess_runner.py
@@ -28,7 +28,6 @@
     PYATS_PLUGIN_CONFIG_FILENAME,
 )
 from nac_test.pyats_core.execution.subprocess_runner import (
-    ConfigFileCreationError,
     SubprocessRunner,
 )
 from nac_test.utils.logging import LogLevel
@@ -342,12 +341,12 @@ def test_init_creates_config_files_in_output_dir(
     assert runner._pyats_config_file.name == PYATS_CONFIG_FILENAME
 
 
-def test_init_raises_config_file_creation_error_on_write_failure(
+def test_init_raises_runtime_error_on_write_failure(
     temp_output_dir: Path, mock_output_handler: Mock
 ) -> None:
-    """Test that ConfigFileCreationError is raised when config file write fails."""
+    """Test that RuntimeError is raised when config file write fails."""
     with patch.object(Path, "write_text", side_effect=OSError("disk full")):
-        with pytest.raises(ConfigFileCreationError, match="disk full"):
+        with pytest.raises(RuntimeError, match="disk full"):
             SubprocessRunner(temp_output_dir, mock_output_handler)
 
 
@@ -361,7 +360,7 @@ def test_write_failure_leaves_attributes_none_and_cleanup_is_safe(
     runner._pyats_config_file = None
 
     with patch.object(Path, "write_text", side_effect=OSError("disk full")):
-        with pytest.raises(ConfigFileCreationError):
+        with pytest.raises(RuntimeError):
             runner._create_config_files()
 
     assert runner._plugin_config_file is None

From 16211ccb055e8ad3b7ed79773dae185672f15702 Mon Sep 17 00:00:00 2001
From: Oliver Boehmer <oboehmer@cisco.com>
Date: Tue, 24 Mar 2026 12:15:25 +0100
Subject: [PATCH 04/11] use textwrap.dedent() also for PYATS_CONFIG

---
 nac_test/pyats_core/execution/subprocess_runner.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/nac_test/pyats_core/execution/subprocess_runner.py b/nac_test/pyats_core/execution/subprocess_runner.py
index ff1c9cca..83bf6083 100644
--- a/nac_test/pyats_core/execution/subprocess_runner.py
+++ b/nac_test/pyats_core/execution/subprocess_runner.py
@@ -37,7 +37,10 @@
             enabled: False
     """)
 
-PYATS_CONFIG = "[report]\ngit_info = false\n"
+PYATS_CONFIG = textwrap.dedent("""\
+    [report]
+    git_info = false
+    """)
 
 
 class SubprocessRunner:

From 23481f26d8e69835458e059aa82e07f8ba2244ca Mon Sep 17 00:00:00 2001
From: Oliver Boehmer <oboehmer@cisco.com>
Date: Tue, 24 Mar 2026 16:57:50 +0100
Subject: [PATCH 05/11] address review feedback from #698

---
 nac_test/pyats_core/execution/subprocess_runner.py | 2 ++
 tests/e2e/conftest.py                              | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/nac_test/pyats_core/execution/subprocess_runner.py b/nac_test/pyats_core/execution/subprocess_runner.py
index 83bf6083..301fcec3 100644
--- a/nac_test/pyats_core/execution/subprocess_runner.py
+++ b/nac_test/pyats_core/execution/subprocess_runner.py
@@ -27,6 +27,8 @@
 logger = logging.getLogger(__name__)
 
 
+# disable EnvironmentDebugPlugin to prevent sensitive environment vars
+# from being logged by PyATS
 PLUGIN_CONFIG = textwrap.dedent("""\
     plugins:
         ProgressReporterPlugin:
diff --git a/tests/e2e/conftest.py b/tests/e2e/conftest.py
index 09ea8772..fdcb31a5 100644
--- a/tests/e2e/conftest.py
+++ b/tests/e2e/conftest.py
@@ -44,7 +44,7 @@
 
 # Sentinel value for credential exposure detection (#689)
 # All test passwords use this value so we can detect if credentials leak into artifacts
-TEST_CREDENTIAL_SENTINEL = "CRED_SENTINEL_MUST_NOT_APPEAR_IN_ARTIFACTS"
+TEST_CREDENTIAL_SENTINEL: str = "CRED_SENTINEL_MUST_NOT_APPEAR_IN_ARTIFACTS"
 
 
 @dataclass

From 96b6569ee3705d90402dcd3d0cb8774cb1f176f3 Mon Sep 17 00:00:00 2001
From: Oliver Boehmer <oboehmer@cisco.com>
Date: Tue, 24 Mar 2026 19:13:13 +0100
Subject: [PATCH 06/11] fix: clean up orphaned config file on partial write
 failure, replace asserts

Review feedback from PR #697:

- Fix bug: if first config file writes successfully but second fails,
  clean up the first file to avoid orphaned files on disk
- Replace assert statements with RuntimeError in _build_command() since
  asserts are stripped in optimized mode (-O)
- Add clarifying comments explaining why cleanup isn't needed on
  SubprocessRunner init failure (attributes only set after success)
- Add docstring note about planned nac-test-wide cleanup mechanism
- Add test for partial write failure cleanup behavior
---
 .../pyats_core/execution/subprocess_runner.py | 16 +++++--
 nac_test/pyats_core/orchestrator.py           |  5 ++-
 .../execution/test_subprocess_runner.py       | 44 +++++++++++++++++++
 3 files changed, 61 insertions(+), 4 deletions(-)

diff --git a/nac_test/pyats_core/execution/subprocess_runner.py b/nac_test/pyats_core/execution/subprocess_runner.py
index 301fcec3..415f52e5 100644
--- a/nac_test/pyats_core/execution/subprocess_runner.py
+++ b/nac_test/pyats_core/execution/subprocess_runner.py
@@ -90,6 +90,9 @@ def _create_config_files(self) -> None:
             plugin_config_file.write_text(PLUGIN_CONFIG)
             pyats_config_file.write_text(PYATS_CONFIG)
         except OSError as e:
+            # Clean up any successfully written files before raising
+            plugin_config_file.unlink(missing_ok=True)
+            pyats_config_file.unlink(missing_ok=True)
             raise RuntimeError(f"Failed to create PyATS config files: {e}") from e
 
         self._plugin_config_file = plugin_config_file
@@ -98,7 +101,12 @@ def _create_config_files(self) -> None:
         logger.debug(f"Created pyats_config {self._pyats_config_file}")
 
     def cleanup(self) -> None:
-        """Remove config files created during initialization."""
+        """Remove config files created during initialization.
+
+        Note: A more robust, nac-test-wide cleanup mechanism is planned that would
+        handle cleanup on unexpected exits. For now, cleanup is best-effort hygiene
+        since the config file contents are not sensitive.
+        """
         for config_file in [self._plugin_config_file, self._pyats_config_file]:
             if config_file is not None:
                 config_file.unlink(missing_ok=True)
@@ -130,8 +138,10 @@ def _build_command(
         if testbed_file_path is not None:
             cmd.extend(["--testbed-file", str(testbed_file_path)])
 
-        assert self._plugin_config_file is not None
-        assert self._pyats_config_file is not None
+        if self._plugin_config_file is None or self._pyats_config_file is None:
+            raise RuntimeError(
+                "Config files not initialized. This indicates a bug in SubprocessRunner."
+            )
         cmd.extend(
             [
                 "--configuration",
diff --git a/nac_test/pyats_core/orchestrator.py b/nac_test/pyats_core/orchestrator.py
index aee61df9..a49ed57f 100644
--- a/nac_test/pyats_core/orchestrator.py
+++ b/nac_test/pyats_core/orchestrator.py
@@ -622,7 +622,10 @@ async def _run_tests_async(self) -> PyATSResults:
                 loglevel=self.loglevel,
             )
         except RuntimeError as e:
-            # pyats entrypoint not found or configfile creation failed
+            # pyats entrypoint not found or config file creation failed.
+            # No cleanup needed: SubprocessRunner._create_config_files() only sets
+            # file path attributes AFTER successful writes, so partial failures
+            # leave attributes as None and cleanup() would be a no-op.
             error_msg = str(e)
             api_result = TestResults.from_error(error_msg) if api_tests else None
             d2d_result = TestResults.from_error(error_msg) if d2d_tests else None
diff --git a/tests/unit/pyats_core/execution/test_subprocess_runner.py b/tests/unit/pyats_core/execution/test_subprocess_runner.py
index fec7d989..7be94d1d 100644
--- a/tests/unit/pyats_core/execution/test_subprocess_runner.py
+++ b/tests/unit/pyats_core/execution/test_subprocess_runner.py
@@ -19,6 +19,7 @@
 import asyncio
 import logging
 from pathlib import Path
+from typing import Any
 from unittest.mock import AsyncMock, MagicMock, Mock, patch
 
 import pytest
@@ -371,6 +372,49 @@ def test_write_failure_leaves_attributes_none_and_cleanup_is_safe(
         mock_unlink.assert_not_called()
 
 
+def test_partial_write_failure_cleans_up_first_file(
+    temp_output_dir: Path,
+) -> None:
+    """Test that partial write failure cleans up successfully written files.
+
+    If the first config file is written successfully but the second fails,
+    the first file should be cleaned up to avoid orphaned files.
+    """
+    runner = object.__new__(SubprocessRunner)
+    runner.output_dir = temp_output_dir
+    runner._plugin_config_file = None
+    runner._pyats_config_file = None
+
+    # Track which file is being written
+    write_count = {"count": 0}
+    original_write_text = Path.write_text
+
+    def write_text_fail_on_second(
+        self: Path, content: str, *args: Any, **kwargs: Any
+    ) -> None:
+        write_count["count"] += 1
+        if write_count["count"] == 1:
+            # First write succeeds
+            original_write_text(self, content, *args, **kwargs)
+        else:
+            # Second write fails
+            raise OSError("disk full on second file")
+
+    with patch.object(Path, "write_text", write_text_fail_on_second):
+        with pytest.raises(RuntimeError, match="disk full"):
+            runner._create_config_files()
+
+    # Attributes should still be None (not set until both writes succeed)
+    assert runner._plugin_config_file is None
+    assert runner._pyats_config_file is None
+
+    # The first file that was successfully written should have been cleaned up
+    plugin_config_path = temp_output_dir / PYATS_PLUGIN_CONFIG_FILENAME
+    assert not plugin_config_path.exists(), (
+        "First config file should be cleaned up after second write fails"
+    )
+
+
 # --- Cleanup tests ---
 
 

From eb2f353f2cb73ea5a43457e310c3bd76e8c373ad Mon Sep 17 00:00:00 2001
From: Oliver Boehmer <oboehmer@cisco.com>
Date: Tue, 24 Mar 2026 21:44:29 +0100
Subject: [PATCH 07/11] address PR #697 review findings: __del__, DRY cleanup,
 test improvements

- Add SubprocessRunner.__del__ for opportunistic cleanup on unexpected exits
  (best-effort: CPython GC only, not guaranteed on SIGKILL; ref #677)
- Mark unreachable None-guard with # pragma: no cover and explanatory comment
- Update cleanup() docstring to reflect dual call sites (__del__ + orchestrator)
- Extract _cleanup_subprocess_runner() helper in orchestrator to DRY up two
  identical call sites; documents that __del__ covers unexpected exit paths
- Cast keep_artifacts to bool() to avoid bool | str | None union type
- Remove stale comment in except RuntimeError block (made obsolete by __del__)
- Strengthen test_cleanup_is_idempotent with file-existence assertions
- Add test_del_calls_cleanup to verify __del__ removes config files
---
 .../pyats_core/execution/subprocess_runner.py | 27 +++++++++++++++----
 nac_test/pyats_core/orchestrator.py           | 23 ++++++++++------
 .../execution/test_subprocess_runner.py       | 23 +++++++++++++++-
 3 files changed, 59 insertions(+), 14 deletions(-)

diff --git a/nac_test/pyats_core/execution/subprocess_runner.py b/nac_test/pyats_core/execution/subprocess_runner.py
index 415f52e5..8ee23fb4 100644
--- a/nac_test/pyats_core/execution/subprocess_runner.py
+++ b/nac_test/pyats_core/execution/subprocess_runner.py
@@ -103,15 +103,28 @@ def _create_config_files(self) -> None:
     def cleanup(self) -> None:
         """Remove config files created during initialization.
 
-        Note: A more robust, nac-test-wide cleanup mechanism is planned that would
-        handle cleanup on unexpected exits. For now, cleanup is best-effort hygiene
-        since the config file contents are not sensitive.
+        Called explicitly by the orchestrator after normal execution. Also called
+        opportunistically from __del__ for unexpected exits (best-effort only).
         """
         for config_file in [self._plugin_config_file, self._pyats_config_file]:
             if config_file is not None:
                 config_file.unlink(missing_ok=True)
                 logger.debug(f"Cleaned up config file: {config_file}")
 
+    def __del__(self) -> None:
+        """Opportunistic cleanup on garbage collection.
+
+        Not guaranteed: CPython-specific, not called on SIGKILL or interpreter shutdown.
+        Handles unexpected exits without complicating call sites in the orchestrator.
+        A more robust cleanup mechanism will be implemented as part of #677 (which
+        primarily targets a different file, but config file cleanup will be included) —
+        until then, leaked config files are acceptable (contents not sensitive, files small).
+        """
+        try:
+            self.cleanup()
+        except Exception:
+            pass  # Best-effort: never raise from __del__
+
     def _build_command(
         self,
         job_file_path: Path,
@@ -138,9 +151,13 @@ def _build_command(
         if testbed_file_path is not None:
             cmd.extend(["--testbed-file", str(testbed_file_path)])
 
-        if self._plugin_config_file is None or self._pyats_config_file is None:
+        # Unreachable in practice: _create_config_files() always sets these in __init__,
+        # or raises before __init__ completes. Guard exists for mypy type narrowing only.
+        if (
+            self._plugin_config_file is None or self._pyats_config_file is None
+        ):  # pragma: no cover
             raise RuntimeError(
-                "Config files not initialized. This indicates a bug in SubprocessRunner."
+                "Config files not initialized — this is a bug in SubprocessRunner."
             )
         cmd.extend(
             [
diff --git a/nac_test/pyats_core/orchestrator.py b/nac_test/pyats_core/orchestrator.py
index a49ed57f..393d1865 100644
--- a/nac_test/pyats_core/orchestrator.py
+++ b/nac_test/pyats_core/orchestrator.py
@@ -623,9 +623,6 @@ async def _run_tests_async(self) -> PyATSResults:
             )
         except RuntimeError as e:
             # pyats entrypoint not found or config file creation failed.
-            # No cleanup needed: SubprocessRunner._create_config_files() only sets
-            # file path attributes AFTER successful writes, so partial failures
-            # leave attributes as None and cleanup() would be a no-op.
             error_msg = str(e)
             api_result = TestResults.from_error(error_msg) if api_tests else None
             d2d_result = TestResults.from_error(error_msg) if d2d_tests else None
@@ -718,6 +715,16 @@ async def _run_tests_async(self) -> PyATSResults:
 
         return pyats_results
 
+    def _cleanup_subprocess_runner(self, keep_artifacts: bool) -> None:
+        """Explicitly clean up config files created by SubprocessRunner.
+
+        Called after report generation completes (success or failure paths).
+        Unexpected exit paths are handled opportunistically via SubprocessRunner.__del__
+        until a more robust mechanism is implemented as part of #677.
+        """
+        if self.subprocess_runner and not keep_artifacts:
+            self.subprocess_runner.cleanup()
+
     async def _generate_html_reports_async(
         self,
     ) -> PyATSResults:
@@ -757,7 +764,9 @@ async def _generate_html_reports_async(
         result = await generator.generate_reports_from_archives(archive_paths)
 
         # Determine whether to keep artifacts (debug mode or explicit env var)
-        keep_artifacts = DEBUG_MODE or os.environ.get("NAC_TEST_PYATS_KEEP_REPORT_DATA")
+        keep_artifacts: bool = bool(
+            DEBUG_MODE or os.environ.get("NAC_TEST_PYATS_KEEP_REPORT_DATA")
+        )
 
         if result["status"] in ["success", "partial"]:
             # Log report generation timing (procedural info)
@@ -816,8 +825,7 @@ async def _generate_html_reports_async(
                         logger.debug(f"Could not remove directory {type_dir}: {e}")
 
             # Clean up PyATS config files created by SubprocessRunner
-            if self.subprocess_runner and not keep_artifacts:
-                self.subprocess_runner.cleanup()
+            self._cleanup_subprocess_runner(keep_artifacts)
 
             # Extract and return test statistics
             if result.get("pyats_stats"):
@@ -830,6 +838,5 @@ async def _generate_html_reports_async(
             if result.get("error"):
                 print(f"Error: {result['error']}")
             # Clean up PyATS config files; report generation failed so no stats to return
-            if self.subprocess_runner and not keep_artifacts:
-                self.subprocess_runner.cleanup()
+            self._cleanup_subprocess_runner(keep_artifacts)
             return PyATSResults()
diff --git a/tests/unit/pyats_core/execution/test_subprocess_runner.py b/tests/unit/pyats_core/execution/test_subprocess_runner.py
index 7be94d1d..4f2ae967 100644
--- a/tests/unit/pyats_core/execution/test_subprocess_runner.py
+++ b/tests/unit/pyats_core/execution/test_subprocess_runner.py
@@ -440,6 +440,27 @@ def test_cleanup_is_idempotent(
 ) -> None:
     """Test that cleanup() can be called multiple times safely."""
     runner = SubprocessRunner(temp_output_dir, mock_output_handler)
+    assert runner._plugin_config_file is not None
+    assert runner._pyats_config_file is not None
 
     runner.cleanup()
-    runner.cleanup()
+    assert not runner._plugin_config_file.exists()
+    assert not runner._pyats_config_file.exists()
+
+    runner.cleanup()  # Second call must not raise
+    assert not runner._plugin_config_file.exists()
+    assert not runner._pyats_config_file.exists()
+
+
+def test_del_calls_cleanup(temp_output_dir: Path, mock_output_handler: Mock) -> None:
+    """Test that __del__ triggers opportunistic cleanup of config files."""
+    runner = SubprocessRunner(temp_output_dir, mock_output_handler)
+    assert runner._plugin_config_file is not None
+    assert runner._pyats_config_file is not None
+    assert runner._plugin_config_file.exists()
+    assert runner._pyats_config_file.exists()
+
+    runner.__del__()
+
+    assert not runner._plugin_config_file.exists()
+    assert not runner._pyats_config_file.exists()

From f42ae1cd0d9fb32294bd330c53f0110a9caa593e Mon Sep 17 00:00:00 2001
From: Oliver Boehmer <oboehmer@cisco.com>
Date: Tue, 24 Mar 2026 21:50:33 +0100
Subject: [PATCH 08/11] don't re-export new constants

---
 nac_test/pyats_core/constants.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/nac_test/pyats_core/constants.py b/nac_test/pyats_core/constants.py
index dcfa7c99..460855ac 100644
--- a/nac_test/pyats_core/constants.py
+++ b/nac_test/pyats_core/constants.py
@@ -133,8 +133,6 @@
     "DEFAULT_CPU_MULTIPLIER",
     "LOAD_AVERAGE_THRESHOLD",
     "AUTH_CACHE_DIR",
-    "PYATS_PLUGIN_CONFIG_FILENAME",
-    "PYATS_CONFIG_FILENAME",
     "PYATS_POST_DISCONNECT_WAIT_SECONDS",
     "PYATS_GRACEFUL_DISCONNECT_WAIT_SECONDS",
     # Multi-job execution

From bb4b76c9406084306d31c9e01c03b4676216101d Mon Sep 17 00:00:00 2001
From: Oliver Boehmer <oboehmer@cisco.com>
Date: Wed, 15 Apr 2026 12:08:34 +0200
Subject: [PATCH 09/11] refactor: use CleanupManager for config file cleanup in
 SubprocessRunner
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Replace manual cleanup()/\__del__() with centralized CleanupManager
registration (keep_if_debug=True). This removes SubprocessRunner.cleanup(),
SubprocessRunner.__del__(), and PyATSOrchestrator._cleanup_subprocess_runner()
along with both call sites in _generate_html_reports_async().

Addresses PR #697 review comments #1 (cleanup gap on early return),
#5 (test_del_calls_cleanup wrong contract), and #8 (missing policy tests
for _cleanup_subprocess_runner — method now deleted).
---
 .../pyats_core/execution/subprocess_runner.py | 33 ++-----
 nac_test/pyats_core/orchestrator.py           | 15 ---
 .../execution/test_subprocess_runner.py       | 91 ++++++++-----------
 3 files changed, 45 insertions(+), 94 deletions(-)

diff --git a/nac_test/pyats_core/execution/subprocess_runner.py b/nac_test/pyats_core/execution/subprocess_runner.py
index 8ee23fb4..99e4a6d6 100644
--- a/nac_test/pyats_core/execution/subprocess_runner.py
+++ b/nac_test/pyats_core/execution/subprocess_runner.py
@@ -22,6 +22,7 @@
     PYATS_OUTPUT_BUFFER_LIMIT,
     PYATS_PLUGIN_CONFIG_FILENAME,
 )
+from nac_test.utils.cleanup import get_cleanup_manager
 from nac_test.utils.logging import DEFAULT_LOGLEVEL, LogLevel
 
 logger = logging.getLogger(__name__)
@@ -97,33 +98,15 @@ def _create_config_files(self) -> None:
 
         self._plugin_config_file = plugin_config_file
         self._pyats_config_file = pyats_config_file
-        logger.debug(f"Created plugin_config {self._plugin_config_file}")
-        logger.debug(f"Created pyats_config {self._pyats_config_file}")
 
-    def cleanup(self) -> None:
-        """Remove config files created during initialization.
+        # Register for automatic cleanup on exit (atexit, SIGTERM, SIGINT).
+        # keep_if_debug=True retains files when NAC_TEST_DEBUG is set.
+        cleanup_mgr = get_cleanup_manager()
+        cleanup_mgr.register(self._plugin_config_file, keep_if_debug=True)
+        cleanup_mgr.register(self._pyats_config_file, keep_if_debug=True)
 
-        Called explicitly by the orchestrator after normal execution. Also called
-        opportunistically from __del__ for unexpected exits (best-effort only).
-        """
-        for config_file in [self._plugin_config_file, self._pyats_config_file]:
-            if config_file is not None:
-                config_file.unlink(missing_ok=True)
-                logger.debug(f"Cleaned up config file: {config_file}")
-
-    def __del__(self) -> None:
-        """Opportunistic cleanup on garbage collection.
-
-        Not guaranteed: CPython-specific, not called on SIGKILL or interpreter shutdown.
-        Handles unexpected exits without complicating call sites in the orchestrator.
-        A more robust cleanup mechanism will be implemented as part of #677 (which
-        primarily targets a different file, but config file cleanup will be included) —
-        until then, leaked config files are acceptable (contents not sensitive, files small).
-        """
-        try:
-            self.cleanup()
-        except Exception:
-            pass  # Best-effort: never raise from __del__
+        logger.debug(f"Created plugin_config {self._plugin_config_file}")
+        logger.debug(f"Created pyats_config {self._pyats_config_file}")
 
     def _build_command(
         self,
diff --git a/nac_test/pyats_core/orchestrator.py b/nac_test/pyats_core/orchestrator.py
index ad8e0918..3fae885b 100644
--- a/nac_test/pyats_core/orchestrator.py
+++ b/nac_test/pyats_core/orchestrator.py
@@ -725,16 +725,6 @@ async def _run_tests_async(self) -> PyATSResults:
 
         return pyats_results
 
-    def _cleanup_subprocess_runner(self, keep_artifacts: bool) -> None:
-        """Explicitly clean up config files created by SubprocessRunner.
-
-        Called after report generation completes (success or failure paths).
-        Unexpected exit paths are handled opportunistically via SubprocessRunner.__del__
-        until a more robust mechanism is implemented as part of #677.
-        """
-        if self.subprocess_runner and not keep_artifacts:
-            self.subprocess_runner.cleanup()
-
     async def _generate_html_reports_async(
         self,
     ) -> PyATSResults:
@@ -834,9 +824,6 @@ async def _generate_html_reports_async(
                     except Exception as e:
                         logger.debug(f"Could not remove directory {type_dir}: {e}")
 
-            # Clean up PyATS config files created by SubprocessRunner
-            self._cleanup_subprocess_runner(keep_artifacts)
-
             # Extract and return test statistics
             if result.get("pyats_stats"):
                 return self._extract_pyats_stats(result["pyats_stats"])
@@ -847,6 +834,4 @@ async def _generate_html_reports_async(
             print(f"\n{terminal.error('Failed to generate reports')}")
             if result.get("error"):
                 print(f"Error: {result['error']}")
-            # Clean up PyATS config files; report generation failed so no stats to return
-            self._cleanup_subprocess_runner(keep_artifacts)
             return PyATSResults()
diff --git a/tests/unit/pyats_core/execution/test_subprocess_runner.py b/tests/unit/pyats_core/execution/test_subprocess_runner.py
index 4f2ae967..a8d2e0ec 100644
--- a/tests/unit/pyats_core/execution/test_subprocess_runner.py
+++ b/tests/unit/pyats_core/execution/test_subprocess_runner.py
@@ -351,25 +351,14 @@ def test_init_raises_runtime_error_on_write_failure(
             SubprocessRunner(temp_output_dir, mock_output_handler)
 
 
-def test_write_failure_leaves_attributes_none_and_cleanup_is_safe(
+def test_write_failure_leaves_attributes_none(
     temp_output_dir: Path,
+    mock_output_handler: Mock,
 ) -> None:
-    """Test that write failure leaves attributes None and cleanup handles it safely."""
-    runner = object.__new__(SubprocessRunner)
-    runner.output_dir = temp_output_dir
-    runner._plugin_config_file = None
-    runner._pyats_config_file = None
-
+    """Test that write failure leaves config file attributes as None."""
     with patch.object(Path, "write_text", side_effect=OSError("disk full")):
         with pytest.raises(RuntimeError):
-            runner._create_config_files()
-
-    assert runner._plugin_config_file is None
-    assert runner._pyats_config_file is None
-
-    with patch.object(Path, "unlink") as mock_unlink:
-        runner.cleanup()
-        mock_unlink.assert_not_called()
+            SubprocessRunner(temp_output_dir, mock_output_handler)
 
 
 def test_partial_write_failure_cleans_up_first_file(
@@ -415,52 +404,46 @@ def write_text_fail_on_second(
     )
 
 
-# --- Cleanup tests ---
+# --- CleanupManager registration tests ---
 
 
-def test_cleanup_removes_config_files(
+def test_init_registers_config_files_with_cleanup_manager(
     temp_output_dir: Path, mock_output_handler: Mock
 ) -> None:
-    """Test that cleanup() removes config files."""
-    runner = SubprocessRunner(temp_output_dir, mock_output_handler)
-
-    assert runner._plugin_config_file is not None
-    assert runner._pyats_config_file is not None
-    assert runner._plugin_config_file.exists()
-    assert runner._pyats_config_file.exists()
-
-    runner.cleanup()
-
-    assert not runner._plugin_config_file.exists()
-    assert not runner._pyats_config_file.exists()
+    """Test that __init__ registers both config files with CleanupManager."""
+    with patch(
+        "nac_test.pyats_core.execution.subprocess_runner.get_cleanup_manager"
+    ) as mock_get_cm:
+        mock_cm = MagicMock()
+        mock_get_cm.return_value = mock_cm
+
+        runner = SubprocessRunner(temp_output_dir, mock_output_handler)
+
+    assert mock_cm.register.call_count == 2
+    registered_paths = {call.args[0] for call in mock_cm.register.call_args_list}
+    assert runner._plugin_config_file in registered_paths
+    assert runner._pyats_config_file in registered_paths
+    # Both should use keep_if_debug=True
+    for call in mock_cm.register.call_args_list:
+        assert call.kwargs.get("keep_if_debug") is True or (
+            len(call.args) > 1 and call.args[1] is True
+        )
 
 
-def test_cleanup_is_idempotent(
+def test_write_failure_does_not_register_with_cleanup_manager(
     temp_output_dir: Path, mock_output_handler: Mock
 ) -> None:
-    """Test that cleanup() can be called multiple times safely."""
-    runner = SubprocessRunner(temp_output_dir, mock_output_handler)
-    assert runner._plugin_config_file is not None
-    assert runner._pyats_config_file is not None
-
-    runner.cleanup()
-    assert not runner._plugin_config_file.exists()
-    assert not runner._pyats_config_file.exists()
-
-    runner.cleanup()  # Second call must not raise
-    assert not runner._plugin_config_file.exists()
-    assert not runner._pyats_config_file.exists()
-
-
-def test_del_calls_cleanup(temp_output_dir: Path, mock_output_handler: Mock) -> None:
-    """Test that __del__ triggers opportunistic cleanup of config files."""
-    runner = SubprocessRunner(temp_output_dir, mock_output_handler)
-    assert runner._plugin_config_file is not None
-    assert runner._pyats_config_file is not None
-    assert runner._plugin_config_file.exists()
-    assert runner._pyats_config_file.exists()
+    """Test that write failure does NOT register files with CleanupManager."""
+    with (
+        patch.object(Path, "write_text", side_effect=OSError("disk full")),
+        patch(
+            "nac_test.pyats_core.execution.subprocess_runner.get_cleanup_manager"
+        ) as mock_get_cm,
+    ):
+        mock_cm = MagicMock()
+        mock_get_cm.return_value = mock_cm
 
-    runner.__del__()
+        with pytest.raises(RuntimeError):
+            SubprocessRunner(temp_output_dir, mock_output_handler)
 
-    assert not runner._plugin_config_file.exists()
-    assert not runner._pyats_config_file.exists()
+    mock_cm.register.assert_not_called()

From 130f3495ae2d59dff6299974328c85d589ea0f49 Mon Sep 17 00:00:00 2001
From: Oliver Boehmer <oboehmer@cisco.com>
Date: Wed, 15 Apr 2026 12:09:14 +0200
Subject: [PATCH 10/11] fix: address PR #697 review comments on test quality
 and exports

- Add missing __all__ entries in constants.py (review #3)
- Narrow Path.write_text patches to module scope in test_subprocess_runner
  instead of global patch.object (review #4)
- Rewrite test_partial_write_failure and test_orchestrator_config_error to
  remove fragile object.__new__ coupling and update to current API (review #7)
---
 nac_test/pyats_core/constants.py              |  4 ++
 .../test_orchestrator_config_error.py         | 35 ++++++++++-----
 .../execution/test_subprocess_runner.py       | 45 +++++++++----------
 3 files changed, 47 insertions(+), 37 deletions(-)

diff --git a/nac_test/pyats_core/constants.py b/nac_test/pyats_core/constants.py
index 9ef1799f..d9c9e3a1 100644
--- a/nac_test/pyats_core/constants.py
+++ b/nac_test/pyats_core/constants.py
@@ -143,6 +143,8 @@
     "DEFAULT_CPU_MULTIPLIER",
     "LOAD_AVERAGE_THRESHOLD",
     "AUTH_CACHE_DIR",
+    "PYATS_PLUGIN_CONFIG_FILENAME",
+    "PYATS_CONFIG_FILENAME",
     "PYATS_POST_DISCONNECT_WAIT_SECONDS",
     "PYATS_GRACEFUL_DISCONNECT_WAIT_SECONDS",
     # Connection broker protocol limits
@@ -165,4 +167,6 @@
     "OVERFLOW_QUEUE_SIZE",
     "OVERFLOW_MEMORY_LIMIT_MB",
     "OVERFLOW_DIR_OVERRIDE",
+    # Environment variable name
+    "ENV_TEST_DIR",
 ]
diff --git a/tests/pyats_core/test_orchestrator_config_error.py b/tests/pyats_core/test_orchestrator_config_error.py
index 493e681b..e00c8056 100644
--- a/tests/pyats_core/test_orchestrator_config_error.py
+++ b/tests/pyats_core/test_orchestrator_config_error.py
@@ -4,7 +4,7 @@
 """Tests for PyATSOrchestrator handling of SubprocessRunner init failures."""
 
 from pathlib import Path
-from unittest.mock import patch
+from unittest.mock import MagicMock, patch
 
 import pytest
 
@@ -32,28 +32,39 @@ def test_subprocess_runner_init_error_returns_from_error_results(
         has_d2d: bool,
     ) -> None:
         """OSError in SubprocessRunner._create_config_files raises RuntimeError, returns from_error results."""
-        api_tests = [Path("/fake/tests/api/test_one.py")] if has_api else []
-        d2d_tests = [Path("/fake/tests/d2d/test_two.py")] if has_d2d else []
+        api_test_paths = [Path("/fake/tests/api/test_one.py")] if has_api else []
+        d2d_test_paths = [Path("/fake/tests/d2d/test_two.py")] if has_d2d else []
 
         orchestrator = PyATSOrchestrator(
             data_paths=[pyats_test_dirs.output_dir.parent / "data"],
             test_dir=pyats_test_dirs.test_dir,
             output_dir=pyats_test_dirs.output_dir,
-            merged_data_filename="merged.yaml",
         )
 
+        # Build a mock DiscoveryResult
+        mock_discovery_result = MagicMock()
+        mock_discovery_result.total_count = len(api_test_paths) + len(d2d_test_paths)
+        mock_discovery_result.api_tests = [MagicMock(path=p) for p in api_test_paths]
+        mock_discovery_result.d2d_tests = [MagicMock(path=p) for p in d2d_test_paths]
+        mock_discovery_result.api_paths = api_test_paths
+        mock_discovery_result.d2d_paths = d2d_test_paths
+        mock_discovery_result.all_tests = (
+            mock_discovery_result.api_tests + mock_discovery_result.d2d_tests
+        )
+        mock_discovery_result.filtered_by_tags = False
+
         with (
             patch.object(
-                orchestrator.test_discovery, "discover_pyats_tests"
-            ) as mock_discover,
-            patch.object(
-                orchestrator.test_discovery, "categorize_tests_by_type"
-            ) as mock_categorize,
+                orchestrator.test_discovery,
+                "discover_pyats_tests",
+                return_value=mock_discovery_result,
+            ),
             patch.object(orchestrator, "validate_environment"),
-            patch.object(Path, "write_text", side_effect=OSError("disk full")),
+            patch(
+                "nac_test.pyats_core.execution.subprocess_runner.Path.write_text",
+                side_effect=OSError("disk full"),
+            ),
         ):
-            mock_discover.return_value = (api_tests + d2d_tests, [])
-            mock_categorize.return_value = (api_tests, d2d_tests)
             result = orchestrator.run_tests()
 
         if has_api:
diff --git a/tests/unit/pyats_core/execution/test_subprocess_runner.py b/tests/unit/pyats_core/execution/test_subprocess_runner.py
index a8d2e0ec..552f6b9d 100644
--- a/tests/unit/pyats_core/execution/test_subprocess_runner.py
+++ b/tests/unit/pyats_core/execution/test_subprocess_runner.py
@@ -346,7 +346,10 @@ def test_init_raises_runtime_error_on_write_failure(
     temp_output_dir: Path, mock_output_handler: Mock
 ) -> None:
     """Test that RuntimeError is raised when config file write fails."""
-    with patch.object(Path, "write_text", side_effect=OSError("disk full")):
+    with patch(
+        "nac_test.pyats_core.execution.subprocess_runner.Path.write_text",
+        side_effect=OSError("disk full"),
+    ):
         with pytest.raises(RuntimeError, match="disk full"):
             SubprocessRunner(temp_output_dir, mock_output_handler)
 
@@ -356,25 +359,23 @@ def test_write_failure_leaves_attributes_none(
     mock_output_handler: Mock,
 ) -> None:
     """Test that write failure leaves config file attributes as None."""
-    with patch.object(Path, "write_text", side_effect=OSError("disk full")):
+    with patch(
+        "nac_test.pyats_core.execution.subprocess_runner.Path.write_text",
+        side_effect=OSError("disk full"),
+    ):
         with pytest.raises(RuntimeError):
             SubprocessRunner(temp_output_dir, mock_output_handler)
 
 
 def test_partial_write_failure_cleans_up_first_file(
     temp_output_dir: Path,
+    mock_output_handler: Mock,
 ) -> None:
     """Test that partial write failure cleans up successfully written files.
 
     If the first config file is written successfully but the second fails,
     the first file should be cleaned up to avoid orphaned files.
     """
-    runner = object.__new__(SubprocessRunner)
-    runner.output_dir = temp_output_dir
-    runner._plugin_config_file = None
-    runner._pyats_config_file = None
-
-    # Track which file is being written
     write_count = {"count": 0}
     original_write_text = Path.write_text
 
@@ -383,19 +384,16 @@ def write_text_fail_on_second(
     ) -> None:
         write_count["count"] += 1
         if write_count["count"] == 1:
-            # First write succeeds
             original_write_text(self, content, *args, **kwargs)
         else:
-            # Second write fails
             raise OSError("disk full on second file")
 
-    with patch.object(Path, "write_text", write_text_fail_on_second):
+    with patch(
+        "nac_test.pyats_core.execution.subprocess_runner.Path.write_text",
+        write_text_fail_on_second,
+    ):
         with pytest.raises(RuntimeError, match="disk full"):
-            runner._create_config_files()
-
-    # Attributes should still be None (not set until both writes succeed)
-    assert runner._plugin_config_file is None
-    assert runner._pyats_config_file is None
+            SubprocessRunner(temp_output_dir, mock_output_handler)
 
     # The first file that was successfully written should have been cleaned up
     plugin_config_path = temp_output_dir / PYATS_PLUGIN_CONFIG_FILENAME
@@ -420,14 +418,8 @@ def test_init_registers_config_files_with_cleanup_manager(
         runner = SubprocessRunner(temp_output_dir, mock_output_handler)
 
     assert mock_cm.register.call_count == 2
-    registered_paths = {call.args[0] for call in mock_cm.register.call_args_list}
-    assert runner._plugin_config_file in registered_paths
-    assert runner._pyats_config_file in registered_paths
-    # Both should use keep_if_debug=True
-    for call in mock_cm.register.call_args_list:
-        assert call.kwargs.get("keep_if_debug") is True or (
-            len(call.args) > 1 and call.args[1] is True
-        )
+    mock_cm.register.assert_any_call(runner._plugin_config_file, keep_if_debug=True)
+    mock_cm.register.assert_any_call(runner._pyats_config_file, keep_if_debug=True)
 
 
 def test_write_failure_does_not_register_with_cleanup_manager(
@@ -435,7 +427,10 @@ def test_write_failure_does_not_register_with_cleanup_manager(
 ) -> None:
     """Test that write failure does NOT register files with CleanupManager."""
     with (
-        patch.object(Path, "write_text", side_effect=OSError("disk full")),
+        patch(
+            "nac_test.pyats_core.execution.subprocess_runner.Path.write_text",
+            side_effect=OSError("disk full"),
+        ),
         patch(
             "nac_test.pyats_core.execution.subprocess_runner.get_cleanup_manager"
         ) as mock_get_cm,

From 96b3bc9394d9ac0d0781ee8dc873cd2421386f36 Mon Sep 17 00:00:00 2001
From: Oliver Boehmer <oboehmer@cisco.com>
Date: Wed, 15 Apr 2026 12:46:19 +0200
Subject: [PATCH 11/11] refactor: move test_orchestrator_config_error.py to
 tests/unit/ (#541)

Move test file from tests/pyats_core/ to tests/unit/pyats_core/ to reduce
placement debt per review comment #9. Add PyATSTestDirs and pyats_test_dirs
fixture to tests/unit/conftest.py (intentional duplication until #541 fully
consolidates the two conftest files).
---
 tests/unit/conftest.py                        | 30 ++++++++++++++++++-
 .../test_orchestrator_config_error.py         |  2 +-
 2 files changed, 30 insertions(+), 2 deletions(-)
 rename tests/{ => unit}/pyats_core/test_orchestrator_config_error.py (98%)

diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py
index bdc05ee3..137ad0f5 100644
--- a/tests/unit/conftest.py
+++ b/tests/unit/conftest.py
@@ -4,7 +4,8 @@
 """Shared fixtures for unit tests."""
 
 import os
-from typing import Any
+from pathlib import Path
+from typing import Any, NamedTuple
 from unittest.mock import AsyncMock, Mock
 
 import pytest
@@ -18,6 +19,15 @@
 
 CONTROLLER_ENV_PREFIXES = ("ACI_", "SDWAN_", "CC_", "MERAKI_", "FMC_", "ISE_")
 
+
+class PyATSTestDirs(NamedTuple):
+    """Directory structure for PyATS orchestrator tests."""
+
+    test_dir: Path
+    output_dir: Path
+    merged_file: Path
+
+
 AUTH_SUCCESS = AuthCheckResult(
     success=True,
     reason=AuthOutcome.SUCCESS,
@@ -125,3 +135,21 @@ def cc_controller_env(monkeypatch: MonkeyPatch) -> None:
     monkeypatch.setenv("CC_URL", "https://cc.test.com")
     monkeypatch.setenv("CC_USERNAME", "admin")
     monkeypatch.setenv("CC_PASSWORD", "test_pass")
+
+
+@pytest.fixture()
+def pyats_test_dirs(tmp_path: Path) -> PyATSTestDirs:
+    """Create standard directory structure for PyATS orchestrator tests.
+
+    Returns:
+        PyATSTestDirs with test_dir, output_dir, and merged_file paths.
+    """
+    test_dir = tmp_path / "tests"
+    test_dir.mkdir()
+    output_dir = tmp_path / "output"
+    output_dir.mkdir()
+    merged_file = output_dir / "merged.yaml"
+    merged_file.write_text("test: data")
+    return PyATSTestDirs(
+        test_dir=test_dir, output_dir=output_dir, merged_file=merged_file
+    )
diff --git a/tests/pyats_core/test_orchestrator_config_error.py b/tests/unit/pyats_core/test_orchestrator_config_error.py
similarity index 98%
rename from tests/pyats_core/test_orchestrator_config_error.py
rename to tests/unit/pyats_core/test_orchestrator_config_error.py
index e00c8056..190ed89c 100644
--- a/tests/pyats_core/test_orchestrator_config_error.py
+++ b/tests/unit/pyats_core/test_orchestrator_config_error.py
@@ -10,7 +10,7 @@
 
 from nac_test.pyats_core.orchestrator import PyATSOrchestrator
 
-from .conftest import PyATSTestDirs
+from ..conftest import PyATSTestDirs
 
 
 class TestOrchestratorSubprocessRunnerInitError: