Skip to content

fix(cli): split newlines before wrapping in approval/sudo/clarify panels (#72580) - #72601

Open
JonthanaHanh wants to merge 1 commit into
NousResearch:mainfrom
JonthanaHanh:fix/approval-panel-heredoc-72580
Open

fix(cli): split newlines before wrapping in approval/sudo/clarify panels (#72580)#72601
JonthanaHanh wants to merge 1 commit into
NousResearch:mainfrom
JonthanaHanh:fix/approval-panel-heredoc-72580

Conversation

@JonthanaHanh

Copy link
Copy Markdown
Contributor

Summary

Fixes #72580.

_wrap_panel_text() (3 copies: approval panel, sudo panel, clarify panel) passes multi-line commands directly to textwrap.wrap(), which treats newlines as whitespace. A heredoc command gets collapsed into a few long lines with literal \n characters, making it unreadable.

Changes

  • cli.py: All 3 copies of _wrap_panel_text now split text on newlines first, then wrap each segment individually. This preserves the original line structure.

Testing

  • py_compile: OK
  • ruff check: All checks passed

…els (NousResearch#72580)

_wrap_panel_text() (3 copies: approval panel, sudo panel, clarify
panel) passes multi-line commands directly to textwrap.wrap(), which
treats newlines as whitespace.  A 50-line heredoc gets collapsed into
4-5 long lines with literal \n characters, making the command
unreadable and pushing approve/deny choices off-screen.

Fix: split text on newlines first, then wrap each segment individually.
This preserves the original line structure while still wrapping long
lines within each segment.

Fixes NousResearch#72580
@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 27, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for identifying a real rendering defect: current main still sends the full approval command through textwrap.wrap() at cli.py:13070, and renders that result at cli.py:13119.

Problems

  • The PR does not cover all active wrappers it describes. The clarify wrapper remains unsplit at cli.py:16100 and is used for the displayed question at cli.py:16278.
  • The first hunk is in _get_slash_confirm_display_fragments (cli.py:8648), not the sudo UI; its detail is already split before wrapping at cli.py:8685 and cli.py:8695. The sudo display is the fixed password panel at cli.py:16350.
  • No regression test is included. tests/cli/test_cli_approval_ui.py:158 exercises a long single-line command, not newline preservation.

Suggested changes

  • Apply the same split-before-wrap behavior to the clarify wrapper while preserving its distinct textwrap options.
  • Add rendering tests for multiline approval and clarify text, plus a one-line control case.

Automated hermes-sweeper review.

@GottZ GottZ left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was generated by AI during triage.

Summary

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

Related pull requests

  • #72601 partial — (+28/-16) — duplicate of #75207: the diff fixes slash confirmation and approval but leaves clarify unchanged and adds no regression tests. Despite the keep_open review on #72601, #75207 covers all three wrapper paths, preserves blank lines, and tests the shared behavior.
  • #72614 best fix — (+78/-24) — duplicate of #75207: the diff correctly fixes slash confirmation, approval, and clarify while adding approval heredoc and one-line regressions, but retains three copies and lacks multiline clarify coverage. Despite the recorded best-fix and keep_open verdicts on #72614, #75207 implements the same correction centrally, retains clarify's distinct wrapping options, and tests clarify delegation and empty lines.
  • #73584 duplicate — (+139/-149) — duplicate of #75207: its approval-only hunk renders an empty source line as visible '', has no regression tests, and is mixed with unrelated tool-hook and Himalaya documentation changes. Despite the keep_open review on #73584, the contributor-identified blank-line defect remains in its diff, while #75207 uses "" and tests blank-line preservation.
  • #75207 best fix — (+152/-29) — keep open with a salvage path: consistent with its recorded best-fix and keep_open verdicts, the diff centralizes newline-preserving wrapping for slash confirmation and approval, delegates clarify with its distinct prose options, preserves empty lines, and adds focused regressions. The remaining salvage step is a true clarify-panel rendering-path test, because the amended tests call the delegation helper directly rather than exercising the panel binding.

Duplicates

#72601, #72614, and #73584 substantially duplicate the split-before-wrap correction in #75207; #75207 subsumes their relevant #72580 changes 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 actual panel binding. Close #72601 and #73584 as duplicates of #75207; despite #72614's recorded best-fix and keep_open verdicts, close it as a duplicate because #75207 contains the same complete wrapper fix with centralized logic and broader empty-line and clarify-delegation coverage. Keep #73584's unrelated tool-hook and Himalaya changes independently reviewable.

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
    P72601 -.->|partial| I72580
    class I72580 open
    class P72601 open
    class P72614 open
    class P73584 open
    class P75207 open
    class P72614 best
    class P75207 best
    class P72601 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.

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 P3 Low — cosmetic, nice to have 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