feat(controller-peers-env-fallback): read XCE_CONTROLLER_PEERS ahead of XCEADMIN_CONTROLLER in every --controller option (task 4.2)

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

.kiro/specs/controller-peers-env-fallback/tasks.md

@@ -79,7 +79,7 @@
   - _Requirements: 2.2, 2.8_
   - _Boundary: xceadmin discovery and errors_
 
-- [ ] 4.2 (P) Read the new variable ahead of the legacy one in every `--controller` option
+- [x] 4.2 (P) Read the new variable ahead of the legacy one in every `--controller` option
   - Write the failing CLI tests first: the variable alone; both variables set (new one wins); explicit `--controller` overriding both; neither set (built-in default); each option's `--help` naming both variables
   - Add `monkeypatch.delenv` for the new variable to every existing test that asserts legacy-variable or no-variable behavior, so ambient state cannot affect them
   - Make the shared env-var setting an ordered list (new variable first, legacy second) and rewrite each option's help string and the precedence comment, since the help strings interpolate the setting today

src/xceadmin/src/xceadmin/cli.py

@@ -99,14 +99,25 @@ def _compose_version() -> str:
 
 _DEFAULT_OUTPUT = "cluster.bin"
 _DEFAULT_CONTROLLER = "localhost:9991"
-# Every --controller option below also reads this env var (click precedence:
+# Every --controller option below also reads these env vars, in order. click
-# explicit --controller flag > $XCEADMIN_CONTROLLER > _DEFAULT_CONTROLLER).
+# returns the first one set to a non-empty value (an empty one counts as
+# unset), so the precedence is: explicit --controller flag >
+# $XCE_CONTROLLER_PEERS (shared with the controller/scheduler/worker/harness;
+# may carry name=host:port entries, which resolve_controllers() strips) >
+# $XCEADMIN_CONTROLLER (legacy, still honored) > _DEFAULT_CONTROLLER.
 # _DEFAULT_CONTROLLER has no relationship to any REAL deployment's actual
 # ports -- it's a bare fallback for a totally bare invocation. A deployment
 # that knows its own real controller ports (e.g. /xce-deploy-ha's generated
-# scripts/xceadmin wrapper) should export this instead of requiring every
+# scripts/xceadmin wrapper) should export these instead of requiring every
 # command to be typed with an explicit --controller.
-_CONTROLLER_ENV_VAR = "XCEADMIN_CONTROLLER"
+_CONTROLLER_ENV_VAR = ["XCE_CONTROLLER_PEERS", "XCEADMIN_CONTROLLER"]
+_CONTROLLER_HELP = (
+    "host:port of any controller node (2.2), or a comma-separated list of "
+    "host:port candidates to try in order (failing over past unreachable/dead "
+    "ones, e.g. --controller host1:9991,host2:9991,host3:9991). Falls back to "
+    "$XCE_CONTROLLER_PEERS if set (preferred; accepts name=host:port entries), "
+    "then the legacy $XCEADMIN_CONTROLLER if set, then the bare default."
+)
 _STATUS_LABEL_WIDTH = 13  # len("started at:") + 2 -- requirements.md's "Status output" example
 
 
@@ -140,10 +151,7 @@ def config_build(configs_dir: str, output: str) -> None:
     "--controller",
     default=_DEFAULT_CONTROLLER,
     envvar=_CONTROLLER_ENV_VAR,
-    help=f"host:port of any controller node (2.2), or a comma-separated list of "
+    help=_CONTROLLER_HELP,
-    "host:port candidates to try in order (failing over past unreachable/dead "
-    "ones, e.g. --controller host1:9991,host2:9991,host3:9991). Falls back to "
-    f"${_CONTROLLER_ENV_VAR} if set, then the bare default.",
 )
 @click.option("--dry-run", is_flag=True, help="Probe acceptance without committing (Requirement 6).")
 def config_push(cluster_bin_path: str, controller: str, dry_run: bool) -> None:
@@ -273,10 +281,7 @@ def _run_scheduler_admin_command(controller: str, call: Callable[[], RecordResul
     "--controller",
     default=_DEFAULT_CONTROLLER,
     envvar=_CONTROLLER_ENV_VAR,
-    help=f"host:port of any controller node (2.2), or a comma-separated list of "
+    help=_CONTROLLER_HELP,
-    "host:port candidates to try in order (failing over past unreachable/dead "
-    "ones, e.g. --controller host1:9991,host2:9991,host3:9991). Falls back to "
-    f"${_CONTROLLER_ENV_VAR} if set, then the bare default.",
 )
 def scheduler_record_start(file: str | None, controller: str) -> None:
     """Start scheduler event recording, optionally to FILE (auto-generated when omitted)."""
@@ -289,10 +294,7 @@ def scheduler_record_start(file: str | None, controller: str) -> None:
     "--controller",
     default=_DEFAULT_CONTROLLER,
     envvar=_CONTROLLER_ENV_VAR,
-    help=f"host:port of any controller node (2.2), or a comma-separated list of "
+    help=_CONTROLLER_HELP,
-    "host:port candidates to try in order (failing over past unreachable/dead "
-    "ones, e.g. --controller host1:9991,host2:9991,host3:9991). Falls back to "
-    f"${_CONTROLLER_ENV_VAR} if set, then the bare default.",
 )
 def scheduler_record_stop(controller: str) -> None:
     """Stop scheduler event recording."""
@@ -305,10 +307,7 @@ def scheduler_record_stop(controller: str) -> None:
     "--controller",
     default=_DEFAULT_CONTROLLER,
     envvar=_CONTROLLER_ENV_VAR,
-    help=f"host:port of any controller node (2.2), or a comma-separated list of "
+    help=_CONTROLLER_HELP,
-    "host:port candidates to try in order (failing over past unreachable/dead "
-    "ones, e.g. --controller host1:9991,host2:9991,host3:9991). Falls back to "
-    f"${_CONTROLLER_ENV_VAR} if set, then the bare default.",
 )
 def scheduler_record_status(controller: str) -> None:
     """Report current scheduler event-recording state."""

src/xceadmin/tests/test_cli.py

@@ -12,6 +12,7 @@ import ssl
 import socket
 import threading
 from pathlib import Path
+from types import SimpleNamespace
 
 import pytest
 import yaml
@@ -597,6 +598,7 @@ def test_format_sched_admin_error_plain_when_not_recording() -> None:
 
 
 def test_scheduler_record_start_success_prints_formatted_result(monkeypatch) -> None:
+    monkeypatch.delenv("XCE_CONTROLLER_PEERS", raising=False)
     captured = {}
 
     def _fake_record_start(controller, filename):
@@ -650,6 +652,7 @@ def test_scheduler_record_start_passes_filename_and_controller_arguments(monkeyp
 
 
 def test_controller_option_falls_back_to_xceadmin_controller_env_var(monkeypatch) -> None:
+    monkeypatch.delenv("XCE_CONTROLLER_PEERS", raising=False)
     captured = {}
 
     def _fake_record_status(controller):
@@ -668,6 +671,7 @@ def test_controller_option_falls_back_to_xceadmin_controller_env_var(monkeypatch
 
 
 def test_controller_flag_overrides_xceadmin_controller_env_var(monkeypatch) -> None:
+    monkeypatch.delenv("XCE_CONTROLLER_PEERS", raising=False)
     captured = {}
 
     def _fake_record_status(controller):
@@ -687,6 +691,126 @@ def test_controller_flag_overrides_xceadmin_controller_env_var(monkeypatch) -> N
     assert captured["controller"] == "127.0.0.1:9999"
 
 
+# ---------------------------------------------------------------------------
+# --controller precedence with $XCE_CONTROLLER_PEERS (controller-peers-env-
+# fallback Requirement 2.1, 2.4-2.7, 4.3): explicit --controller >
+# $XCE_CONTROLLER_PEERS > $XCEADMIN_CONTROLLER > _DEFAULT_CONTROLLER, for
+# every command that takes --controller. Each case sets or deletes BOTH
+# variables itself, so ambient environment state cannot leak in.
+# ---------------------------------------------------------------------------
+
+_RECORD_RESULT = RecordResult(recording=False, file="", event_count=0, started_at=0)
+
+
+def _patch_controller_command(monkeypatch, command: str, tmp_path: Path) -> tuple[list[str], dict]:
+    """Patch the backend function behind ``command`` so it records the
+    controller value it was given; return the CLI args that invoke it."""
+    captured: dict = {}
+
+    def _record(controller, *_rest):
+        captured["controller"] = controller
+        return _RECORD_RESULT
+
+    if command == "config push":
+
+        def _fake_push_config(cluster_bin_path, controller):
+            captured["controller"] = controller
+            return SimpleNamespace(version_tag=1, acked_count=1, error="")
+
+        monkeypatch.setattr(cli_module, "push_config", _fake_push_config)
+        bin_path = tmp_path / "cluster.bin"
+        bin_path.write_bytes(b"unused -- push_config() is monkeypatched here")
+        return ["config", "push", str(bin_path)], captured
+
+    name = command.split()[-1]
+    monkeypatch.setattr(cli_module, f"record_{name}", _record)
+    return ["scheduler", "record", name], captured
+
+
+_CONTROLLER_COMMANDS = [
+    "config push",
+    "scheduler record start",
+    "scheduler record stop",
+    "scheduler record status",
+]
+
+
+@pytest.mark.parametrize("command", _CONTROLLER_COMMANDS)
+def test_controller_option_uses_xce_controller_peers_alone(monkeypatch, tmp_path, command) -> None:
+    monkeypatch.delenv("XCEADMIN_CONTROLLER", raising=False)
+    monkeypatch.setenv("XCE_CONTROLLER_PEERS", "ctrl-1=localhost:9921,ctrl-2=localhost:9922")
+    args, captured = _patch_controller_command(monkeypatch, command, tmp_path)
+
+    result = CliRunner().invoke(cli, args)
+
+    assert result.exit_code == 0, result.output
+    assert captured["controller"] == "ctrl-1=localhost:9921,ctrl-2=localhost:9922"
+
+
+@pytest.mark.parametrize("command", _CONTROLLER_COMMANDS)
+def test_controller_option_prefers_xce_controller_peers_over_legacy(monkeypatch, tmp_path, command) -> None:
+    monkeypatch.setenv("XCE_CONTROLLER_PEERS", "localhost:9921")
+    monkeypatch.setenv("XCEADMIN_CONTROLLER", "localhost:9901")
+    args, captured = _patch_controller_command(monkeypatch, command, tmp_path)
+
+    result = CliRunner().invoke(cli, args)
+
+    assert result.exit_code == 0, result.output
+    assert captured["controller"] == "localhost:9921"
+
+
+@pytest.mark.parametrize("command", _CONTROLLER_COMMANDS)
+def test_controller_option_empty_xce_controller_peers_falls_through_to_legacy(
+    monkeypatch, tmp_path, command
+) -> None:
+    monkeypatch.setenv("XCE_CONTROLLER_PEERS", "")
+    monkeypatch.setenv("XCEADMIN_CONTROLLER", "localhost:9901")
+    args, captured = _patch_controller_command(monkeypatch, command, tmp_path)
+
+    result = CliRunner().invoke(cli, args)
+
+    assert result.exit_code == 0, result.output
+    assert captured["controller"] == "localhost:9901"
+
+
+@pytest.mark.parametrize("command", _CONTROLLER_COMMANDS)
+def test_controller_flag_overrides_both_env_vars(monkeypatch, tmp_path, command) -> None:
+    monkeypatch.setenv("XCE_CONTROLLER_PEERS", "localhost:9921")
+    monkeypatch.setenv("XCEADMIN_CONTROLLER", "localhost:9901")
+    args, captured = _patch_controller_command(monkeypatch, command, tmp_path)
+
+    result = CliRunner().invoke(cli, [*args, "--controller", "127.0.0.1:9999"])
+
+    assert result.exit_code == 0, result.output
+    assert captured["controller"] == "127.0.0.1:9999"
+
+
+@pytest.mark.parametrize("command", _CONTROLLER_COMMANDS)
+def test_controller_option_uses_default_when_neither_env_var_set(monkeypatch, tmp_path, command) -> None:
+    monkeypatch.delenv("XCE_CONTROLLER_PEERS", raising=False)
+    monkeypatch.delenv("XCEADMIN_CONTROLLER", raising=False)
+    args, captured = _patch_controller_command(monkeypatch, command, tmp_path)
+
+    result = CliRunner().invoke(cli, args)
+
+    assert result.exit_code == 0, result.output
+    assert captured["controller"] == cli_module._DEFAULT_CONTROLLER
+
+
+@pytest.mark.parametrize("command", _CONTROLLER_COMMANDS)
+def test_controller_option_help_names_both_env_vars_in_order(command) -> None:
+    result = CliRunner().invoke(cli, [*command.split(), "--help"])
+
+    assert result.exit_code == 0, result.output
+    help_text = " ".join(result.output.split())
+    assert "$XCE_CONTROLLER_PEERS" in help_text
+    assert "$XCEADMIN_CONTROLLER" in help_text
+    assert help_text.index("$XCE_CONTROLLER_PEERS") < help_text.index("$XCEADMIN_CONTROLLER")
+    assert "name=host:port" in help_text
+    assert "['XCE" not in result.output
+    assert "$[" not in result.output
+
+
 def test_scheduler_record_stop_success_prints_formatted_result(monkeypatch) -> None:
     def _fake_record_stop(controller):
         return RecordResult(recording=False, file="final.replay", event_count=14273, started_at=1000)
@@ -778,6 +902,7 @@ def test_scheduler_record_status_connection_timeout_exits_nonzero_not_raw_traceb
 
 
 def test_scheduler_record_start_connection_refused_exits_nonzero_not_raw_traceback(monkeypatch) -> None:
+    monkeypatch.delenv("XCE_CONTROLLER_PEERS", raising=False)
     def _fake_record_start(controller, filename):
         raise ConnectionRefusedError("refused")