From 2ccaa66239b186ccd36589b4d70bb48d52b3c391 Mon Sep 17 00:00:00 2001 From: Thomas Christory Date: Sat, 24 Oct 2026 23:45:06 +0200 Subject: [PATCH 1/2] feat(output): render branch merge/sync conflicts as a table on a terminal (#169) A `conflict` envelope printed details.conflicts as a raw, truncated Python dict. On rich stderr, render one row per conflicting field with the object and its original/branch/main values (shape from netbox-branching 1.2.2). Unexpected payloads keep the raw details line; JSON output is unchanged. Also drop soft_wrap from the error panel console: inside a Panel it cropped long lines, hiding e.g. "re-run with --acknowledge-conflicts". Claude-Session: https://claude.ai/code/session_01RD8NgCVtp3yYFd6NSZpbUb --- CHANGELOG.md | 11 +++ docs/guides/branching.md | 17 +++- nsc/output/errors.py | 54 +++++++++++- tests/output/test_errors_conflicts.py | 114 ++++++++++++++++++++++++++ 4 files changed, 189 insertions(+), 7 deletions(-) create mode 100644 tests/output/test_errors_conflicts.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 6cac205..14d3173 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,8 +4,18 @@ 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. - **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 +210,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..587b438 100644 --- a/nsc/output/errors.py +++ b/nsc/output/errors.py @@ -6,12 +6,17 @@ from __future__ import annotations +import json 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,7 +104,9 @@ 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. body_lines = [ @@ -117,9 +124,48 @@ def render_to_rich_stderr(env: ErrorEnvelope, *, stream: TextIO, color: bool = F 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")) + 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: {escape(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")) + + +_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: object names and values are server data, never markup. + table.add_row(*(Text(c) for c in (obj if i == 0 else "", name, *values))) + 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..7354ee5 --- /dev/null +++ b/tests/output/test_errors_conflicts.py @@ -0,0 +1,114 @@ +"""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 From 654e77741dbccac836d5353be36c4c73ead37b23 Mon Sep 17 00:00:00 2001 From: Thomas Christory Date: Sat, 24 Oct 2026 23:50:46 +0200 Subject: [PATCH 2/2] fix(output): neutralize terminal control sequences in error panels Rich's markup escape() and Text() don't strip ESC, so server-sourced text in the TTY error panel (message, endpoint, op id, and the new conflict table cells) could carry terminal escape sequences, e.g. an object name that retitles or clears the terminal. Show control and format characters as visible \x.. escapes. Flagged by the automated security review on #205. Claude-Session: https://claude.ai/code/session_01RD8NgCVtp3yYFd6NSZpbUb --- CHANGELOG.md | 4 ++++ nsc/output/errors.py | 34 ++++++++++++++++++++------- tests/output/test_errors_conflicts.py | 28 ++++++++++++++++++++++ 3 files changed, 58 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 14d3173..8a37c99 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,10 @@ All notable changes to netbox-super-cli are tracked here. Format follows [Keep a - **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 diff --git a/nsc/output/errors.py b/nsc/output/errors.py index 587b438..36a7200 100644 --- a/nsc/output/errors.py +++ b/nsc/output/errors.py @@ -7,6 +7,7 @@ from __future__ import annotations import json +import unicodedata from enum import Enum, StrEnum from typing import Any, Literal, TextIO @@ -108,34 +109,50 @@ def render_to_rich_stderr(env: ErrorEnvelope, *, stream: TextIO, color: bool = F # 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)}") + 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: {escape(str(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") @@ -156,8 +173,9 @@ def _conflict_table(raw: Any) -> Table | None: 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: object names and values are server data, never markup. - table.add_row(*(Text(c) for c in (obj if i == 0 else "", name, *values))) + # 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 diff --git a/tests/output/test_errors_conflicts.py b/tests/output/test_errors_conflicts.py index 7354ee5..e398224 100644 --- a/tests/output/test_errors_conflicts.py +++ b/tests/output/test_errors_conflicts.py @@ -112,3 +112,31 @@ def test_long_error_message_wraps_instead_of_being_cropped() -> None: ) 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