feat(controller-peers-env-fallback): name the environment source in xceadmin's controller errors (task 4.3)

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

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

@@ -87,7 +87,7 @@
   - _Requirements: 2.1, 2.4, 2.5, 2.6, 2.7, 4.3_
   - _Boundary: xceadmin CLI_
 
-- [ ] 4.3 Name the environment source in xceadmin's controller errors
+- [x] 4.3 Name the environment source in xceadmin's controller errors
   - Write the failing CLI tests first: in-order failover over an env-derived named list (patching the per-candidate request so the first fails and the second answers); unreachable values from each variable and from an explicit `--controller` (source note only for the two variables); a missing cluster-bin file with the variable set (no source note); a malformed entry from an explicit `--controller` and from the variable (entry quoted, note only for the variable). Use `config push` with a real cluster-bin file, or patch out the record commands' retry pause
   - Add a helper that returns "(from $NAME)" when the value came from the environment, using the same first-non-empty rule click applies
   - Append it to the "cannot connect" messages only for network errors (not local file errors), and to the controller-reachability and controller-value errors in the existing handlers; leave other errors unchanged

src/xceadmin/src/xceadmin/cli.py

@@ -27,14 +27,23 @@ traceback.
 
 from __future__ import annotations
 
+import os
 from collections.abc import Callable
 from datetime import datetime, timezone
 from importlib.metadata import PackageNotFoundError
 from importlib.metadata import version as _package_version
 
 import click
+from click.core import ParameterSource
 
-from xcecommon.errors import SchedAdminError, XceAdminError
+from xcecommon.errors import (
+    ConnectionTimeoutError,
+    ControllerValueError,
+    NoPrimaryError,
+    SchedAdminError,
+    TooManyRedirectsError,
+    XceAdminError,
+)
 from xceadmin.build import build_cluster_config, dump_cluster_config, write_output
 from xceadmin.push import probe_config, push_config
 from xceadmin.scheduler import RecordResult, dump, record_start, record_status, record_stop
@@ -118,9 +127,46 @@ _CONTROLLER_HELP = (
     "$XCE_CONTROLLER_PEERS if set (preferred; accepts name=host:port entries), "
     "then the legacy $XCEADMIN_CONTROLLER if set, then the bare default."
 )
+# Errors about the --controller value itself (malformed, or nothing reachable
+# at it): when that value came from the environment, their message names the
+# variable (controller-peers-env-fallback 2.8). Every other XceAdminError
+# (e.g. a bad cluster-bin file, a SchedAdminError reply) keeps its text.
+_CONTROLLER_ERRORS = (ControllerValueError, ConnectionTimeoutError, TooManyRedirectsError, NoPrimaryError)
 _STATUS_LABEL_WIDTH = 13  # len("started at:") + 2 -- requirements.md's "Status output" example
 
 
+def _controller_source_note() -> str:
+    """``" (from $NAME)"`` when the current command's ``--controller`` value
+    came from the environment, ``""`` otherwise (explicit flag or default).
+    NAME is the first variable in ``_CONTROLLER_ENV_VAR`` set to a non-empty
+    value -- the same rule click applies when it reads the envvar list.
+    """
+    ctx = click.get_current_context(silent=True)
+    if ctx is None or ctx.get_parameter_source("controller") is not ParameterSource.ENVIRONMENT:
+        return ""
+    for name in _CONTROLLER_ENV_VAR:
+        if os.environ.get(name):
+            return f" (from ${name})"
+    return ""
+
+
+def _controller_error_message(e: XceAdminError) -> str:
+    """``e.message``, plus the env source note for a controller-value error."""
+    if isinstance(e, _CONTROLLER_ERRORS):
+        return e.message + _controller_source_note()
+    return e.message
+
+
+def _cannot_connect_message(controller: str, e: OSError) -> str:
+    """The "cannot connect" text for an ``OSError``; the env source note is
+    added only for a network error -- a local file error (missing or
+    unreadable cluster-bin file) carries ``filename`` and is not the
+    controller value's fault.
+    """
+    note = _controller_source_note() if e.filename is None else ""
+    return f"cannot connect to {controller}: {e}{note}"
+
+
 @click.group()
 @click.version_option(version=_compose_version(), prog_name="xceadmin")
 def cli() -> None:
@@ -172,12 +218,12 @@ def config_push(cluster_bin_path: str, controller: str, dry_run: bool) -> None:
                 raise click.ClickException(result.error or "push rejected (ackedCount == 0)")  # 2.7
             # 2.6: positive ackedCount and no error -- fall through to exit 0.
     except XceAdminError as e:
-        raise click.ClickException(e.message) from e
+        raise click.ClickException(_controller_error_message(e)) from e
     except OSError as e:
         # ConnectionRefusedError and other bare OS-level failures propagate
         # unwrapped out of xcecommon.wire by design (see wire.py's module
         # docstring) -- this is the layer that formats them (7.1).
-        raise click.ClickException(f"cannot connect to {controller}: {e}") from e
+        raise click.ClickException(_cannot_connect_message(controller, e)) from e
     except KeyboardInterrupt:
         raise click.ClickException("cancelled") from None
 
@@ -265,12 +311,12 @@ def _run_scheduler_admin_command(controller: str, call: Callable[[], RecordResul
     except SchedAdminError as e:
         raise click.ClickException(_format_sched_admin_error(e)) from e
     except XceAdminError as e:
-        raise click.ClickException(e.message) from e
+        raise click.ClickException(_controller_error_message(e)) from e
     except OSError as e:
         # Same rationale as config_push's own OSError branch: bare OS-level
         # failures (ConnectionRefusedError etc.) propagate unwrapped out of
         # xcecommon.wire by design -- this is the layer that formats them.
-        raise click.ClickException(f"cannot connect to {controller}: {e}") from e
+        raise click.ClickException(_cannot_connect_message(controller, e)) from e
     except KeyboardInterrupt:
         raise click.ClickException("cancelled") from None
 

src/xceadmin/tests/test_cli.py

@@ -967,3 +967,221 @@ def test_scheduler_dump_missing_file_exits_nonzero_with_clean_message(monkeypatc
     assert result.exit_code != 0
     assert not isinstance(result.exception, KeyError | AttributeError | TypeError | FileNotFoundError)
     assert "does-not-exist.replay" in result.output
+
+
+# ---------------------------------------------------------------------------
+# Controller errors name an environment source (controller-peers-env-fallback
+# Requirements 2.3, 2.8): a controller value taken from $XCE_CONTROLLER_PEERS
+# or $XCEADMIN_CONTROLLER gets " (from $NAME)" appended to its unreachable /
+# malformed-entry error; an explicit --controller value and errors unrelated
+# to the controller value (a missing cluster-bin file) get no note.
+# ---------------------------------------------------------------------------
+
+
+def _closed_port() -> int:
+    listener = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+    listener.bind(("127.0.0.1", 0))
+    port = listener.getsockname()[1]
+    listener.close()
+    return port
+
+
+def _error_line(result) -> str:
+    """The ``Error: ...`` line click prints for a ClickException (stderr
+    lines from the failover loop may precede it in ``result.output``)."""
+    lines = [line for line in result.output.splitlines() if line.startswith("Error: ")]
+    assert len(lines) == 1, result.output
+    return lines[0]
+
+
+def _assert_clean_failure(result) -> None:
+    assert result.exit_code == 1, result.output
+    assert isinstance(result.exception, SystemExit), repr(result.exception)
+    assert "Traceback" not in result.output
+
+
+def test_config_push_fails_over_env_candidates_in_order(monkeypatch, cluster_bin, wrap_reply) -> None:
+    """2.3: an env-derived named list is tried in env order with name=
+    stripped; an unreachable first candidate fails over to the second, and
+    the third is never needed."""
+    from xceadmin import discovery
+
+    monkeypatch.delenv("XCEADMIN_CONTROLLER", raising=False)
+    monkeypatch.setenv("XCE_CONTROLLER_PEERS", "a=127.0.0.1:1,b=127.0.0.1:2,127.0.0.1:3")
+
+    mod = load_schema()
+    reply = mod.RequestReply.new_message()
+    reply.status = 0
+    upgrade_reply = reply.data.init("upgradeconfigreplymsg")
+    upgrade_reply.versionTag = 7
+    upgrade_reply.ackedCount = 2
+    upgrade_reply.error = ""
+    reply_bytes = wrap_reply(reply)
+
+    tried: list[tuple[str, int]] = []
+
+    def _fake_follow_redirects(host, port, send_fn):
+        tried.append((host, port))
+        if (host, port) == ("127.0.0.1", 1):
+            raise ConnectionRefusedError("refused")
+        return host, port, reply_bytes
+
+    monkeypatch.setattr(discovery, "follow_redirects", _fake_follow_redirects)
+
+    result = CliRunner().invoke(cli, ["config", "push", cluster_bin])
+
+    assert result.exit_code == 0, result.output
+    assert tried == [("127.0.0.1", 1), ("127.0.0.1", 2)]
+    assert "versionTag=7 ackedCount=2" in result.output
+
+
+@pytest.mark.parametrize("dry_run", [False, True], ids=["push", "dry-run"])
+@pytest.mark.parametrize(
+    ("env_name", "note"),
+    [
+        ("XCE_CONTROLLER_PEERS", " (from $XCE_CONTROLLER_PEERS)"),
+        ("XCEADMIN_CONTROLLER", " (from $XCEADMIN_CONTROLLER)"),
+        (None, ""),
+    ],
+    ids=["xce-controller-peers", "xceadmin-controller", "explicit"],
+)
+def test_config_push_unreachable_controller_names_env_source(
+    monkeypatch, cluster_bin, env_name, note, dry_run
+) -> None:
+    """2.8: an unreachable controller value is a clean error ending with
+    the variable it came from; an explicit --controller gets no note."""
+    monkeypatch.delenv("XCE_CONTROLLER_PEERS", raising=False)
+    monkeypatch.delenv("XCEADMIN_CONTROLLER", raising=False)
+    value = f"127.0.0.1:{_closed_port()}"
+    args = ["config", "push", cluster_bin] + (["--dry-run"] if dry_run else [])
+    if env_name is None:
+        args += ["--controller", value]
+    else:
+        monkeypatch.setenv(env_name, value)
+
+    result = CliRunner().invoke(cli, args)
+
+    _assert_clean_failure(result)
+    line = _error_line(result)
+    assert value in line
+    assert "no primary available" in line
+    if note:
+        assert line.endswith(note), line
+    else:
+        assert "(from $" not in line, line
+
+
+@pytest.mark.parametrize("command", _CONTROLLER_COMMANDS)
+@pytest.mark.parametrize(
+    "error",
+    [
+        ConnectionRefusedError("refused"),
+        ConnectionTimeoutError("timed out connecting to 127.0.0.1:9", "127.0.0.1", 9),
+    ],
+    ids=["os-error", "connection-timeout"],
+)
+def test_controller_errors_name_env_source_in_every_command(
+    monkeypatch, tmp_path, command, error
+) -> None:
+    """2.8: every --controller command appends the env source note to a
+    network OSError ("cannot connect to ...") and to a controller
+    reachability error raised by its backend call."""
+    monkeypatch.delenv("XCEADMIN_CONTROLLER", raising=False)
+    monkeypatch.setenv("XCE_CONTROLLER_PEERS", "localhost:9921")
+    args, _captured = _patch_controller_command(monkeypatch, command, tmp_path)
+
+    def _raise(*_args, **_kwargs):
+        raise error
+
+    target = "push_config" if command == "config push" else f"record_{command.split()[-1]}"
+    monkeypatch.setattr(cli_module, target, _raise)
+
+    result = CliRunner().invoke(cli, args)
+
+    _assert_clean_failure(result)
+    line = _error_line(result)
+    assert line.endswith(" (from $XCE_CONTROLLER_PEERS)"), line
+    if isinstance(error, OSError):
+        assert line.startswith("Error: cannot connect to localhost:9921: refused"), line
+
+
+def test_scheduler_record_sched_admin_error_gets_no_env_source_note(monkeypatch) -> None:
+    """A SchedAdminError is the scheduler's own reply, not a controller
+    value problem: its text stays unchanged even with the variable set."""
+    monkeypatch.setenv("XCE_CONTROLLER_PEERS", "localhost:9921")
+
+    def _fake_record_stop(controller):
+        raise SchedAdminError(
+            "scheduler admin request failed: not recording",
+            error="not recording",
+            recording=False,
+            file="",
+            event_count=0,
+            started_at=0,
+        )
+
+    monkeypatch.setattr(cli_module, "record_stop", _fake_record_stop)
+
+    result = CliRunner().invoke(cli, ["scheduler", "record", "stop"])
+
+    _assert_clean_failure(result)
+    assert _error_line(result) == "Error: not recording"
+
+
+def test_config_push_missing_cluster_bin_gets_no_env_source_note(monkeypatch, tmp_path) -> None:
+    """2.8: a local file error (FileNotFoundError carries a filename) is
+    not blamed on the variable even when the controller value came from
+    it."""
+    monkeypatch.delenv("XCEADMIN_CONTROLLER", raising=False)
+    monkeypatch.setenv("XCE_CONTROLLER_PEERS", "localhost:9921")
+    missing = tmp_path / "does-not-exist.bin"
+
+    result = CliRunner().invoke(cli, ["config", "push", str(missing)])
+
+    _assert_clean_failure(result)
+    line = _error_line(result)
+    assert str(missing) in line
+    assert "(from $" not in line, line
+
+
+def test_config_push_malformed_explicit_controller_entry_is_clean_error_without_note(
+    monkeypatch, cluster_bin
+) -> None:
+    """2.8: a malformed entry from an explicit --controller is quoted in a
+    clean error with no source note, even with both variables set."""
+    monkeypatch.setenv("XCE_CONTROLLER_PEERS", "localhost:9921")
+    monkeypatch.setenv("XCEADMIN_CONTROLLER", "localhost:9901")
+
+    result = CliRunner().invoke(cli, ["config", "push", cluster_bin, "--controller", "a=h:x"])
+
+    _assert_clean_failure(result)
+    line = _error_line(result)
+    assert "'a=h:x'" in line
+    assert "(from $" not in line, line
+
+
+@pytest.mark.parametrize("command", _CONTROLLER_COMMANDS)
+@pytest.mark.parametrize(
+    "env_name", ["XCE_CONTROLLER_PEERS", "XCEADMIN_CONTROLLER"]
+)
+def test_malformed_env_controller_entry_is_clean_error_naming_variable(
+    monkeypatch, cluster_bin, command, env_name
+) -> None:
+    """2.8: a malformed entry from either variable is quoted in a clean
+    error (no traceback) ending with the variable's name, in every
+    --controller command (backends not patched: the real
+    resolve_controllers() raises before any network I/O)."""
+    monkeypatch.delenv("XCE_CONTROLLER_PEERS", raising=False)
+    monkeypatch.delenv("XCEADMIN_CONTROLLER", raising=False)
+    monkeypatch.setenv(env_name, "a=h:x")
+    if command == "config push":
+        args = ["config", "push", cluster_bin]
+    else:
+        args = command.split()
+
+    result = CliRunner().invoke(cli, args)
+
+    _assert_clean_failure(result)
+    line = _error_line(result)
+    assert "'a=h:x'" in line
+    assert line.endswith(f" (from ${env_name})"), line