Skip to content

fix(cli): split panel text on newlines before wrapping - #75207

Open
AKAZIK-py wants to merge 3 commits into
NousResearch:mainfrom
AKAZIK-py:fix/approval-panel-newline-wrap
Open

fix(cli): split panel text on newlines before wrapping#75207
AKAZIK-py wants to merge 3 commits into
NousResearch:mainfrom
AKAZIK-py:fix/approval-panel-newline-wrap

Conversation

@AKAZIK-py

@AKAZIK-py AKAZIK-py commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Bug

When a dangerous command contains embedded newlines (e.g. a python3 << 'EOF' ... EOF heredoc), the TUI approval panel renders it as a few long lines with literal \n characters, making the command unreadable and pushing the approve/deny choices off-screen.

Fixes #72580.

Root Cause

_wrap_panel_text in cli.py (3 copies: approval panel, sudo panel, clarify panel) called textwrap.wrap() directly on the full command string. textwrap.wrap treats \n as ordinary whitespace — it does not split on newlines. A multi-line heredoc script gets collapsed into a handful of long lines with embedded newlines.

Fix

  • Extracted the triplicated wrapper (slash-confirmation panel, approval panel, clarify panel — the sudo panel uses a fixed password display and is unaffected) into one module-level _wrap_panel_text in cli.py that splits on \n first, then wraps each line individually (preserving empty lines, the width>=8 floor, and subsequent_indent semantics).
  • The slash-confirm and approval panels (identical copies) now resolve to the shared helper directly.
  • The clarify panel's copy had intentionally different textwrap kwargs (break_long_words=False, break_on_hyphens=False, default whitespace handling — prose wrapping). It now binds to _wrap_clarify_panel_text, a module-level delegator with those kwargs, so its wrapping behaviour is byte-identical to before; it only gains the newline split.
  • Note: the Ink TUI already splits command lines itself (ui-tui/src/components/prompts.tsx:102-104); this fix covers the prompt_toolkit CLI path.

No signature changes for callers; the nested call sites are untouched.

Regression Tests

tests/cli/test_wrap_panel_text.py (11 tests): per-source-line preservation for a heredoc, no embedded newlines in output, empty-line preservation, single-line behaviour unchanged, empty input, subsequent_indent on continuations only, the width floor, the no-word-break kwargs — plus 3 rendering-path tests for the clarify delegation (_wrap_clarify_panel_text: multiline question split, prose no-word-break, empty lines). Verified to fail against the pre-fix implementation (3 behaviour failures; the module functions obviously don't exist pre-fix at all).

Existing tests/cli/test_cli_approval_ui.py (15 tests) passes unchanged.

Checklist

  • Reproduced the bug (in the issue body, and via the new tests against the old implementation)
  • Fix addresses the root cause (split before wrap), applied to all 3 panel copies
  • Clarify panel's divergent wrapping kwargs preserved byte-for-byte
  • Regression tests added
  • No new dependencies

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused regression fix. Current main still sends the complete approval command through textwrap.wrap() at cli.py:13070-13078, then uses that output to budget and render the command at cli.py:13118-13205; splitting before wrapping addresses the reported behavior.

Suggested changes

  • Add a rendering-path regression for multiline clarify content. The clarify wrapper has distinct prose settings at cli.py:16100-16108; the new helper tests verify options directly but do not exercise that panel delegation.
  • Please correct the scope wording: the first removed wrapper is slash confirmation (cli.py:8648), not sudo; sudo has a fixed password display (cli.py:16350-16365). The Ink TUI already splits command lines at ui-tui/src/components/prompts.tsx:102-104.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 31, 2026
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have labels Jul 31, 2026
@AKAZIK-py
AKAZIK-py force-pushed the fix/approval-panel-newline-wrap branch from fa9934c to 7a1c8aa Compare July 31, 2026 05:37
@AKAZIK-py

Copy link
Copy Markdown
Contributor Author

Both points addressed in the amended commit:

  1. Clarify rendering-path regression: the clarify delegation is now a module-level _wrap_clarify_panel_text (the panel binds it via a one-line alias), and 3 tests exercise that path directly: multiline question splitting, prose no-word-break preservation, and empty-line handling. 26/26 in tests/cli/test_wrap_panel_text.py + test_cli_approval_ui.py pass.
  2. Scope wording corrected: the three copies are slash-confirmation (_get_slash_confirm_display_fragments), approval, and clarify — verified on main. The sudo panel (fixed password display) is unaffected, and the PR body now notes the Ink TUI already splits command lines at prompts.tsx:102-104, so this covers the prompt_toolkit path. Commit message and PR description updated accordingly.

@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

Four PRs address #72580 with split-before-wrap changes: #72601 fixes slash confirmation and approval only, #72614 also fixes clarify and adds approval regressions, #73584 changes only approval while mishandling blank lines and bundling unrelated work, and #75207 centralizes all three wrappers while preserving clarify-specific semantics and adding the broadest focused coverage.

Related pull requests

Duplicates

#72601, #72614, and the relevant approval hunk of #73584 substantially duplicate the split-before-wrap correction in #75207; #75207 subsumes their #72580 work without #73584's blank-line defect or unrelated files.

Suggested consolidation

Keep #75207 open with a salvage path: retain its shared newline-preserving helpers and focused tests, then add a clarify-panel rendering regression that exercises the panel binding itself. Close #72601 and #73584 as duplicates of #75207; despite #72614's recorded best-fix and keep_open verdicts, close #72614 as a duplicate because #75207 contains the same complete wrapper correction with centralized logic and broader empty-line and clarify-delegation coverage.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I72580(["issue #72580 (open)"])
    subgraph Dup72601 ["PRs duplicating each other"]
        P72601["PR #72601 (open)"]
        P72614["PR #72614 (open)"]
        P73584["PR #73584 (open)"]
        P75207["PR #75207 (open)"]
    end
    P75207 -->|best fix| I72580
    class I72580 open
    class P72601 open
    class P72614 open
    class P73584 open
    class P75207 open
    class P72614 best
    class P75207 best
    class P75207 target
    click I72580 "https://github.com/NousResearch/hermes-agent/issues/72580"
    click P72601 "https://github.com/NousResearch/hermes-agent/pull/72601"
    click P72614 "https://github.com/NousResearch/hermes-agent/pull/72614"
    click P73584 "https://github.com/NousResearch/hermes-agent/pull/73584"
    click P75207 "https://github.com/NousResearch/hermes-agent/pull/75207"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 4 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 34 kB of PR diffs, 7 kB of issue/PR text, 3 kB of discussion (5 comments), 7 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@AKAZIK-py

Copy link
Copy Markdown
Contributor Author

Addressed the salvage step from the 2026-08-03 triage: clarify-panel rendering regression that exercises the actual panel binding.

The clarify renderer was a run()-scoped closure (not directly testable), so I lifted it into an instance method _get_clarify_display_fragments — mirroring the existing _get_approval_display_fragments — and bound the widget to self._get_clarify_display_fragments. Two new regressions call the real renderer with a populated _clarify_state:

  1. test_clarify_panel_binding_renders_all_choices_with_multiline_question — a multi-line question keeps one display line per source line (the fix: approval panel mangles multi-line heredoc commands, pushing approve/deny buttons off-screen #72580 newline-split fix) and all choices + the Other row still render.
  2. test_clarify_panel_binding_drops_question_on_tiny_terminal — when the choices alone overflow the viewport, the question is dropped entirely instead of clipping the choices off-screen (HSplit overflow guard).

Branch rebased onto latest main (8f27127). 28/28 pass in tests/cli/test_wrap_panel_text.py + tests/cli/test_cli_approval_ui.py; py_compile clean. Diff remains 2 files: cli.py + tests/cli/test_wrap_panel_text.py.

@alt-glitch alt-glitch removed the needs-decision Awaiting maintainer decision before any implementation label Aug 6, 2026
@AKAZIK-py
AKAZIK-py force-pushed the fix/approval-panel-newline-wrap branch from 7e62154 to 14f9646 Compare August 7, 2026 04:41
…72580)

textwrap.wrap treats embedded newlines as ordinary whitespace, so a
multi-line command pending approval (e.g. a heredoc) collapsed into a
few unreadable long lines that pushed the approve/deny choices
off-screen. Extract the triplicated _wrap_panel_text (slash-confirm,
approval, clarify panels) into one module-level helper that splits on
newlines first, then wraps each line individually. The clarify panel
keeps its prose-oriented kwargs (break_long_words=False,
break_on_hyphens=False) via _wrap_clarify_panel_text. The sudo panel
is unaffected (fixed password display).
…visible (NousResearch#72580)

Triage salvage step for NousResearch#75207: lift the clarify panel renderer from a
run()-scoped closure into an instance method (_get_clarify_display_fragments,
mirroring _get_approval_display_fragments) so the actual panel binding is
exercisable. Add two rendering regressions through the real renderer:
- multiline question keeps one display line per source line and all
  choices + Other still render (newline-split fix, NousResearch#72580)
- tiny terminal drops the question entirely rather than clipping choices
@AKAZIK-py
AKAZIK-py force-pushed the fix/approval-panel-newline-wrap branch from 14f9646 to 46c1daa Compare August 12, 2026 07:10
@AKAZIK-py
AKAZIK-py force-pushed the fix/approval-panel-newline-wrap branch from 46c1daa to e6985e3 Compare August 12, 2026 07:15
@alt-glitch alt-glitch added P2 Medium — degraded but workaround exists needs-decision Awaiting maintainer decision before any implementation and removed P3 Low — cosmetic, nice to have labels Aug 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: approval panel mangles multi-line heredoc commands, pushing approve/deny buttons off-screen

4 participants