feat(cli): show session state in classic terminal title - #74654
feat(cli): show session state in classic terminal title#74654konsisumer wants to merge 1 commit into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for splitting the classic-CLI implementation into a focused current-main change. The gap is real: current main has terminal-title handling in the TUI, not in the classic prompt_toolkit CLI.
Problems
hermes_cli/terminal_title.py:60writes tosys.stdout, but the classic interactive loop runs underpatch_stdout(cli.py:17185).cli.py:3060explicitly documents that raw ANSI through itsStdoutProxyis swallowed, so the new OSC updates may never reach the terminal in the path this PR targets.hermes_cli/terminal_title.py:49-61has no native-Windows branch. The existing TUI title hook usesprocess.titleon Windows because classic conhost does not support OSC (ui-tui/packages/hermes-ink/src/ink/hooks/use-terminal-title.ts:15-31).
Suggested changes
- Route the OSC writer through a prompt_toolkit-safe real-terminal/raw-output path and add a regression test for that interactive path.
- Add a native-Windows fallback or safe no-op, with coverage.
This is an automated hermes-sweeper review.
| if output is None or not output.isatty(): | ||
| return False | ||
| clean_title = sanitize_terminal_title(title) | ||
| if not clean_title: |
There was a problem hiding this comment.
The classic interactive loop is inside patch_stdout (cli.py:17185), and cli.py:3060 documents that raw ANSI written to its StdoutProxy is swallowed. Please write this sequence through the real terminal/raw prompt_toolkit output path and add a regression test; otherwise the title lifecycle can be invisible in the primary classic-CLI path.
| """Emit OSC 1/2 title updates when stdout is an interactive terminal. | ||
|
|
||
| OSC 1 updates a terminal icon/tab label and OSC 2 updates the window title. | ||
| Unsupported terminals safely ignore both sequences. The writer deliberately |
There was a problem hiding this comment.
This emits OSC for native Windows TTYs too. The existing TUI title hook explicitly uses process.title on Windows because classic conhost does not support OSC (ui-tui/packages/hermes-ink/src/ink/hooks/use-terminal-title.ts:15-31). Please add a Windows fallback or explicit safe no-op with coverage.
|
Thanks for the review. Addressed in this push:
Verified with |
cf1210a to
f10dd8c
Compare
Review note (Windows user / VS Code + Git Bash)Thanks for the focused classic-CLI path and for addressing the earlier Remaining gaps on the platform this feature is meant to helpThese are the environments where “title doesn’t feel alive” shows up most for me:
What already looks solid
Happy to re-test on Win11 + VS Code + Git Bash if you add the dual-write path. Not asking to expand into a second product (spinner templates live in #36698) — just the Windows tab-actually-moves case. (Human review from a Windows daily driver; not the sweeper bot.) |
f10dd8c to
e9a67ac
Compare
|
Thanks for the detailed Windows report. Addressed in this push:
Verified with focused Ruff checks and |
SummaryFour PRs address or reference #5505: #4834 introduced OSC title updates inside a large unrelated bundle, #5318 and #8497 carried the same stale and unsafe classic-CLI patch, and #74654 is the focused current-main implementation covering the classic CLI lifecycle, session names, busy state, opt-out, and tests. Related pull requests
Duplicates#5318 and #8497 are effectively the same change; #4834 contains an earlier version of the same terminal-title feature within a much broader bundle. All three are superseded for this issue by the focused #74654 implementation. Suggested consolidationKeep #74654 open with a salvage path: retain its focused current-main title helper, lifecycle hooks, prompt_toolkit raw-output transport, Windows dual transport, opt-out, and tests, then obtain contributor re-review of the two previously blocking transport findings and validate the requested Windows Terminal, VS Code, conhost, and Git Bash matrix. Keep #4834 closed as superseded by #74654, keep #5318 closed as superseded by #74654, and keep #8497 closed as a duplicate of #5318 in the same superseded chain. Complex graphflowchart 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
I5505(["issue #5505 (open)"])
subgraph Dup4834 ["PRs duplicating each other"]
P4834["PR #4834 (closed)"]
P5318["PR #5318 (closed)"]
P8497["PR #8497 (closed)"]
P74654["PR #74654 (open)"]
end
P74654 -->|best fix| I5505
class I5505 open
class P4834 closed
class P5318 closed
class P8497 closed
class P74654 open
class P4834 best
class P74654 best
class P74654 target
click I5505 "https://github.com/NousResearch/hermes-agent/issues/5505"
click P4834 "https://github.com/NousResearch/hermes-agent/pull/4834"
click P5318 "https://github.com/NousResearch/hermes-agent/pull/5318"
click P8497 "https://github.com/NousResearch/hermes-agent/pull/8497"
click P74654 "https://github.com/NousResearch/hermes-agent/pull/74654"
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: 228 kB of PR diffs, 4 kB of issue/PR text, 10 kB of discussion (17 comments), 5 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
|
I built the same feature against the classic CLI locally last week, so I ran my own test cases against this branch (head
Two ordering problems did show up, both from the delayed auto-title callback. 1. The session guard is checked outside the write lock. In
My harness ended with 2. The callback clears the busy marker of the turn in flight. Neither needs an unusual setup, but both need the titler to race a fast session switch or a quick second turn, so I would call them polish rather than blockers. I am happy to send a patch for either if that helps. This branch also covers ground mine did not: the config opt-out, the |
|
One more from the same testing pass, and it is the one I would fix first. The title write reaches
data = "".join(self._buffer)
self._buffer = []
flush_stdout(self.stdout, data)A title write landing between the join and the rebind is dropped by the rebind. A title write that flushes while the renderer is inside its own flush can put the same bytes on the terminal twice. Forcing the interleaving deterministically (blocking inside the join, then writing a title from a second thread) gave me: Prompt text and title sequence both emitted twice. That repro is synthetic in its timing, but the shared mutable buffer and the unsynchronized Scheduling the write onto the application event loop with |
|
Thanks for the detailed concurrency repros. Addressed in this push:
This is a two-file, 139-line follow-up limited to the reported concurrency behavior. Verified with focused Ruff checks and |
e9a67ac to
6bf58d5
Compare
|
Verified the new head (6bf58d5) against my earlier repros. The scheduled-write path resolves all three findings:
Nothing further from my side. |
6bf58d5 to
d5dc6f8
Compare
|
Rebased onto current Verified: Ruff on all six changed Python files; The 6-file, 408-line diff is the original terminal-title feature scope after rebase, not repair scope expansion; this repair added no files and only reconciles its lifecycle integration with current main. |
d5dc6f8 to
a536833
Compare
|
Rebased onto current Verified: focused terminal-title tests (9 passed), Ruff on all changed Python files, the Windows-footguns diff scan, |
What changed and why
Per the follow-up triage, this adds the missing, current-main terminal-title lifecycle to the classic prompt_toolkit CLI. It emits sanitized OSC 1/2 updates only for interactive terminals, uses the active skin's response symbol, includes the session name, marks active work with an hourglass, and keeps response panel labels aligned with named sessions.
display.terminal_title: falsedisables the behavior.The lifecycle refreshes at startup, thinking transitions, completion, manual and automatic titles, new sessions, resumes, and branches. The automatic-title callback is session-scoped so a delayed title cannot overwrite a newly selected session's tab.
Addressing maintainer feedback
The maintainer identified #10013 and #5318 as related open work. This is a new, atomic current-main implementation for #5505 rather than modifying either existing PR branch: it supplies the classic-CLI OSC writer and lifecycle integration called for by the follow-up triage, while leaving #10013 and #5318 untouched for maintainers to consolidate as appropriate.
How to test
/title Release prep, and verify the tab/window shows the active skin symbol plusRelease prep.⏳while Hermes is working, then returns to idle when the turn completes./new,/resume, and/branch; each should retitle the tab for the active session.display.terminal_title: falseinconfig.yamland verify no OSC title updates are emitted./opt/homebrew/bin/timeout -k 30 480 sh -c 'pytest tests/ -q -x --timeout=60 "$@"' shWhat platforms tested on
Fixes #5505