fix: skip controller check when in --render-only mode

[oboehmer]

There is no need for any controller check when we only rendering robot files. This is also for backward-compatibility reasons as this was never required and affects current workflows

I also disabled the DEBUG_MODE treatment, don't see a need for this here.

Fixes #508

[aitestino]

@oboehmer — Thanks for tackling the render-only mode controller check bypass. The core fix is solid and addresses issue #508 directly. A few observations for discussion:

1. Sentinel Value Pattern

The current implementation uses an empty string as the sentinel:

self.controller_type: str = ""
if not self.render_only:
    # ... detection logic

Would you consider using str | None = None instead? It's more explicit about "not set" vs "detected but empty" and aligns with Python's idiomatic pattern for optional values. The test assertion assert orchestrator.controller_type == "" would become assert orchestrator.controller_type is None, which reads more naturally.

2. DEBUG_MODE Removal

I noticed the DEBUG_MODE constant was removed. While the original implementation had some rough edges, the underlying concept of progressive disclosure (clean output for customers, full context for developers) has value for supportability — in fact, this pattern helped with root-causing the macOS issue recently. This feels like it might warrant its own discussion or issue rather than being bundled here. What prompted this change?

3. Test Coverage Gap

The test test_render_only_mode_does_not_instantiate_pyats_orchestrator does excellent work verifying PyATSOrchestrator isn't instantiated, but it mocks TestDiscovery which means we're not actually verifying the real discovery path. More importantly, consider adding an explicit assertion that detect_controller_type() itself is never called in render-only mode:

with patch("nac_test.combined_orchestrator.detect_controller_type") as mock_detect:
    orchestrator = CombinedOrchestrator(..., render_only=True)
    mock_detect.assert_not_called()

This would directly verify the fix for issue #508. I know you mentioned earlier this week you're working on a larger testing PR as we move from MVP to production — totally fine if you want to track this in a separate issue. Happy to open it for you if that helps, but didn't want to not call it out.

4. Guard Clause Logic

In run_tests(), the dev_pyats_only check has and not self.render_only:

if self.dev_pyats_only and not self.render_only:

This prevents the early return when both flags are set, causing fall-through to production mode. Is this intentional? If someone passes --pyats --render-only, should that be an error, or should render-only take precedence?

---

None of these are blockers — the core fix is correct and the tests cover the main scenario. Happy to discuss any of these points.

---

P.S. — This comment was drafted using voice-to-text via Claude Code. If the tone comes across as overly direct or terse, please know that's just how it tends to phrase things. No offense or criticism is intended — this is purely an objective technical review of the PR. Thanks for understanding! 🙂

[oboehmer]

Thanks for the review! I replied via comments in the code, making it easier for tracking,the conversations. Do you prefer otherwise, @aitestino ?

[oboehmer]

converting to draft, waiting for #470 to be merged

[oboehmer]

@aitestino , please re-review this one

[aitestino]

@oboehmer — the core fix looks good, and I can see you addressed the earlier feedback on the sentinel value (str | None) and the test assertions.

One thing I'd like to see added: an integration test that validates the backward compatibility claim end-to-end. The current unit test correctly verifies control flow (PyATS orchestrator not instantiated, controller detection not called), but since render_only actually renders both Robot templates and the merged data model, we have an opportunity to test the real thing without any mocking - and without needing controller credentials:

@pytest.mark.integration
def test_render_only_without_controller_credentials(tmp_path: Path) -> None:
    """Render-only mode works without controller environment variables."""
    data_file = tmp_path / "data.yaml"
    data_file.write_text("device: Router1\nip: 192.168.1.1")

    template = tmp_path / "templates" / "test.robot.j2"
    template.parent.mkdir(parents=True)
    template.write_text(
        "*** Test Cases ***\nVerify {{ device }}\n    Log    IP: {{ ip }}"
    )

    runner = CliRunner()
    result = runner.invoke(app, [
        "-d", str(data_file),
        "-t", str(template.parent),
        "-o", str(tmp_path / "output"),
        "--render-only",
    ])

    assert result.exit_code == 0
    output = (tmp_path / "output" / "test.robot").read_text()
    assert "Verify Router1" in output
    assert "{{" not in output  # Jinja variables resolved

This would catch regressions where controller detection accidentally gets re-enabled in render-only mode - which is the exact scenario from #508. Happy to open a separate issue for this if you'd prefer to track it outside this PR.

---

P.S. — I'm sure you already know this, but just in case it helps: when a PR depends on another that's still in review, you don't have to convert to draft and wait for the base to merge. You can branch directly off the in-flight branch (git checkout -b feature-B feature-A) and stack PRs - GitHub handles the diff correctly once the base merges. I noticed you were blocked here waiting on #470, and similarly on #470 waiting for #407 - stacking would have let you keep moving on all three in parallel. Just a workflow tip! This comment was drafted using voice-to-text via Claude Code. If the tone comes across as overly direct or terse, please know that's just how it tends to phrase things. No offense or criticism is intended — this is purely an objective technical review of the PR. Thanks for understanding! 🙂

[oboehmer]

requested changes implemented via 5a2a9ea , please re-review

And P.S: I knew about stacking PRs, I just wasn't sure if you liked the reporting refactoring in 470 (which also changed quite a bit when it came to results collection), so didn't want to touch this code until I had more visibility.. avoiding double-work..

[aitestino]

@oboehmer Thanks! LGTM.