diff --git a/CHANGELOG.md b/CHANGELOG.md index 6cac205..8a37c99 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,8 +4,22 @@ All notable changes to netbox-super-cli are tracked here. Format follows [Keep a ## Unreleased +### Added + +- **Branch merge/sync conflicts render as a table on a terminal** ([#169]). A + `conflict` error (exit 10) used to print `details.conflicts` as a raw, + truncated Python dict. It now shows one row per conflicting field with the + object and its original, branch and main values. JSON output is unchanged. + ### Fixed +- **Long error messages wrap inside the terminal error panel** instead of being + cut off. For example, the conflict error's closing + `re-run with --acknowledge-conflicts` hint used to disappear. +- **Server text in terminal error panels can't send terminal control + sequences.** The error message, endpoint, operation id and conflict values + were printed raw, so an ESC sequence in an object name could retitle or clear + the terminal. Control and bidi characters now show as visible `\x..` escapes. - **Plugin endpoints get one command group per resource** ([#202]). NetBox tags every plugin endpoint `plugins`, so all of a plugin's endpoints were merged into one `nsc plugins ` resource whose verbs overwrote each other. For @@ -200,6 +214,7 @@ notices, and a fix for `nsc login` reporting the wrong user. [#1]: https://github.com/thomaschristory/netbox-super-cli/issues/1 [#166]: https://github.com/thomaschristory/netbox-super-cli/issues/166 [#170]: https://github.com/thomaschristory/netbox-super-cli/issues/170 +[#169]: https://github.com/thomaschristory/netbox-super-cli/issues/169 [#182]: https://github.com/thomaschristory/netbox-super-cli/issues/182 [#186]: https://github.com/thomaschristory/netbox-super-cli/issues/186 [#189]: https://github.com/thomaschristory/netbox-super-cli/issues/189 diff --git a/docs/guides/branching.md b/docs/guides/branching.md index 24da642..5052dc0 100644 --- a/docs/guides/branching.md +++ b/docs/guides/branching.md @@ -125,9 +125,20 @@ policy blocks the merge. If main and the branch changed the same fields of an object, `sync` and `merge` fail with exit **10** (`conflict`), and `details.conflicts` lists each -object together with its original, branch and main values. After reviewing -them, re-run with `--acknowledge-conflicts` to proceed; the branch's values -win. +object together with its original, branch and main values. On a terminal they +render as a table, one row per conflicting field: + +```text +╭────────────────────┬─────────────┬──────────┬─────────────┬───────────╮ +│ object │ field │ original │ branch │ main │ +├────────────────────┼─────────────┼──────────┼─────────────┼───────────┤ +│ extras.tag edge │ description │ │ from-branch │ from-main │ +╰────────────────────┴─────────────┴──────────┴─────────────┴───────────╯ +``` + +With `-o json` (or when piped), the envelope carries the raw list unchanged. +After reviewing them, re-run with `--acknowledge-conflicts` to proceed; the +branch's values win. ## For agents diff --git a/nsc/output/errors.py b/nsc/output/errors.py index 5262969..36a7200 100644 --- a/nsc/output/errors.py +++ b/nsc/output/errors.py @@ -6,12 +6,18 @@ from __future__ import annotations +import json +import unicodedata from enum import Enum, StrEnum from typing import Any, Literal, TextIO from pydantic import BaseModel, ConfigDict, Field +from rich import box +from rich.console import Group, RenderableType from rich.markup import escape from rich.panel import Panel +from rich.table import Table +from rich.text import Text from nsc.config.models import OutputFormat from nsc.model.command_model import HttpMethod @@ -99,27 +105,85 @@ def select_render_target(*, output_format: OutputFormat, stdout_is_tty: bool) -> def render_to_rich_stderr(env: ErrorEnvelope, *, stream: TextIO, color: bool = False) -> None: - console = make_console(stream, color=color, soft_wrap=True) + # No soft_wrap: inside a Panel it crops long lines instead of wrapping them, + # which cut off e.g. the "re-run with --acknowledge-conflicts" hint. + console = make_console(stream, color=color) # env.error/endpoint/operation_id/audit_log_path/details carry server- and - # user-sourced text; escape it so stray [..] is not parsed as Rich markup. + # user-sourced text: `_safe` neutralizes Rich markup and terminal control + # sequences (an object name must not retitle or clear the terminal). body_lines = [ - f"[bold red]{env.type.value}[/]: {escape(env.error)}", + f"[bold red]{env.type.value}[/]: {_safe(env.error)}", ] if env.endpoint: - body_lines.append(f"endpoint: {escape(env.endpoint)}") + body_lines.append(f"endpoint: {_safe(env.endpoint)}") if env.method is not None: body_lines.append(f"method: {env.method.value}") if env.status_code is not None: body_lines.append(f"status: {env.status_code}") if env.operation_id: - body_lines.append(f"op: {escape(env.operation_id)}") + body_lines.append(f"op: {_safe(env.operation_id)}") if env.attempt_n is not None: body_lines.append(f"attempt: {env.attempt_n}") if env.audit_log_path: - body_lines.append(f"audit: {escape(env.audit_log_path)}") - if env.details: - body_lines.append(f"details: {escape(str(env.details))}") - console.print(Panel("\n".join(body_lines), title="nsc error", border_style="red")) + body_lines.append(f"audit: {_safe(env.audit_log_path)}") + details = dict(env.details) + conflicts = _conflict_table(details.get("conflicts")) + if conflicts is not None: + del details["conflicts"] + if details: + body_lines.append(f"details: {_safe(str(details))}") + body: RenderableType = "\n".join(body_lines) + if conflicts is not None: + body = Group(body, conflicts) + console.print(Panel(body, title="nsc error", border_style="red")) + + +def _printable(text: str) -> str: + """Show control and format characters (ESC, BEL, bidi overrides, ...) as + visible escapes so server text cannot drive the terminal.""" + return "".join( + ch + if ch in "\n\t" or not unicodedata.category(ch).startswith("C") + else ch.encode("unicode_escape").decode("ascii") + for ch in text + ) + + +def _safe(text: str) -> str: + return escape(_printable(text)) + + +_CONFLICT_SIDES = ("original", "branch", "main") + + +def _conflict_table(raw: Any) -> Table | None: + """Issue #169: one row per conflicting field of a Branching merge/sync 409. + + Returns None (keeping the raw `details:` line) unless every entry is a dict, + so an unexpected plugin payload is still shown rather than dropped. + """ + if not isinstance(raw, list) or not raw or not all(isinstance(c, dict) for c in raw): + return None + table = Table("object", "field", *_CONFLICT_SIDES, box=box.ROUNDED) + for conflict in raw: + obj = f"{conflict.get('object_type') or ''} {conflict.get('object_repr') or ''}".strip() + data = conflict.get("conflicting_data") + sides = data if isinstance(data, dict) else {} + fields = conflict.get("conflicts") + names = [str(f) for f in fields] if isinstance(fields, list) and fields else [""] + for i, name in enumerate(names): + values = [_conflict_value(sides.get(side), name) for side in _CONFLICT_SIDES] + # Text cells are never parsed as markup; `_printable` handles control codes. + cells = (obj if i == 0 else "", name, *values) + table.add_row(*(Text(_printable(c)) for c in cells)) + return table + + +def _conflict_value(side: Any, field: str) -> str: + if not isinstance(side, dict) or field not in side: + return "" + value = side[field] + return value if isinstance(value, str) else json.dumps(value, default=str) class ClientError(Exception): diff --git a/tests/output/test_errors_conflicts.py b/tests/output/test_errors_conflicts.py new file mode 100644 index 0000000..e398224 --- /dev/null +++ b/tests/output/test_errors_conflicts.py @@ -0,0 +1,142 @@ +"""Branch merge/sync conflicts render as a table on rich stderr (#169).""" + +from __future__ import annotations + +import io +import json +from typing import Any + +from nsc.output.errors import ErrorEnvelope, ErrorType, render_to_json, render_to_rich_stderr + + +def _conflict(**over: Any) -> dict[str, Any]: + # Shape returned by netbox-branching 1.2.2 on NetBox 4.7.3 for a 409 merge. + data: dict[str, Any] = { + "id": 1, + "object_type": "extras.tag", + "object_id": 1, + "object_repr": "edge", + "action": {"value": "update", "label": "Updated"}, + "conflicts": ["color", "description"], + "conflicting_data": { + "original": {"color": "9e9e9e", "description": ""}, + "branch": {"color": "ff0000", "description": "from-branch"}, + "main": {"color": "00ff00", "description": "from-main"}, + }, + "last_updated": "2026-10-24T21:31:56.653578Z", + } + data.update(over) + return data + + +def _envelope(*conflicts: Any) -> ErrorEnvelope: + return ErrorEnvelope( + error="branch 'my-change' conflicts with main", + type=ErrorType.CONFLICT, + details={"branch": "my-change", "conflicts": list(conflicts)}, + ) + + +def _render(env: ErrorEnvelope) -> str: + buf = io.StringIO() + render_to_rich_stderr(env, stream=buf) + return buf.getvalue() + + +def _rows(out: str) -> list[list[str]]: + rows = [] + for line in out.splitlines(): + inner = line.strip()[1:-1].strip() # drop the panel's own border + if not (inner.startswith("│") and inner.endswith("│")): + continue + cells = [c.strip() for c in inner[1:-1].split("│")] + if len(cells) == 5: + rows.append(cells) + return rows + + +def test_conflicts_render_one_row_per_field_with_all_three_values() -> None: + out = _render(_envelope(_conflict())) + assert _rows(out) == [ + ["object", "field", "original", "branch", "main"], + ["extras.tag edge", "color", "9e9e9e", "ff0000", "00ff00"], + ["", "description", "", "from-branch", "from-main"], + ] + assert "{'id'" not in out + + +def test_other_details_still_shown_without_raw_conflicts() -> None: + out = _render(_envelope(_conflict())) + details = [line for line in out.splitlines() if "details:" in line] + assert details, out + assert "my-change" in details[0] + assert "conflicting_data" not in out + + +def test_conflict_without_conflicting_data_lists_fields() -> None: + out = _render(_envelope(_conflict(conflicting_data=None, conflicts=["name"]))) + assert _rows(out)[1] == ["extras.tag edge", "name", "", "", ""] + + +def test_non_string_values_render_as_json() -> None: + data = { + "original": {"tags": []}, + "branch": {"tags": [{"id": 1}]}, + "main": {"tags": None}, + } + out = _render(_envelope(_conflict(conflicts=["tags"], conflicting_data=data))) + assert _rows(out)[1] == ["extras.tag edge", "tags", "[]", '[{"id": 1}]', "null"] + + +def test_unreadable_conflicts_fall_back_to_details_line() -> None: + out = _render(_envelope()) + assert _rows(out) == [] + assert "conflicts" in out + out = _render(_envelope("not-a-dict")) + assert _rows(out) == [] + assert "not-a-dict" in out + + +def test_json_envelope_is_unchanged() -> None: + env = _envelope(_conflict()) + assert json.loads(render_to_json(env))["details"]["conflicts"] == [_conflict()] + + +def test_long_error_message_wraps_instead_of_being_cropped() -> None: + env = ErrorEnvelope( + error=( + "branch 'my-change' has 1 unacknowledged conflict(s); review them and " + "re-run with --acknowledge-conflicts" + ), + type=ErrorType.CONFLICT, + ) + flat = " ".join(c for line in _render(env).splitlines() for c in line.strip("│ ").split()) + assert "re-run with --acknowledge-conflicts" in flat + + +def test_server_text_cannot_inject_terminal_escapes() -> None: + evil = "x\x1b]0;pwned\x07\x1b[2J\u202e" + env = ErrorEnvelope( + error=f"err {evil}", + type=ErrorType.CONFLICT, + endpoint=f"https://nb/{evil}", + operation_id=evil, + details={ + "conflicts": [ + _conflict( + object_repr=evil, + conflicts=["f"], + conflicting_data={ + "original": {"f": evil}, + "branch": {"f": "a"}, + "main": {"f": "b"}, + }, + ) + ] + }, + ) + out = _render(env) + assert "\x1b" not in out + assert "\x07" not in out + assert "\u202e" not in out + assert "\\x1b]0;pwned" in out