Skip to content

refactor: extract cli_config, cli_git, cli_display from cli.py (−2,158 lines) - #12355

Open
xandgate wants to merge 1 commit into
NousResearch:mainfrom
xandgate:refactor/cli-modularisation
Open

refactor: extract cli_config, cli_git, cli_display from cli.py (−2,158 lines)#12355
xandgate wants to merge 1 commit into
NousResearch:mainfrom
xandgate:refactor/cli-modularisation

Conversation

@xandgate

@xandgate xandgate commented Apr 19, 2026

Copy link
Copy Markdown

Extracts three coherent clusters from the 16,280-line cli.py into focused modules, per the god-file modularization goal in AGENTS.md:

  • cli_config.py (609 lines): config loading/saving (load_cli_config, save_config_value), prefill message loading and precedence resolution, reasoning/service-tier parsing
  • cli_git.py (701 lines): git worktree isolation — synchronized-base _setup_worktree, lock classification, stale-worktree pruning, orphaned-branch cleanup; active-worktree state behind get_active_worktree()/set_active_worktree()
  • cli_display.py (1,015 lines): ANSI/skin helpers, light-mode detection/remap, output-history record/replay, file-drop and attachment detection, ChatConsole, banners, slash-command detection

cli.py: 16,280 → 14,122 lines (−13.3%). The HermesCLI class and main() are untouched.

All moves are verbatim from the current head. cli.py re-imports every moved name, so existing from cli import X call sites (mixins, tools/delegate_tool.py, tests) keep working. Tests that monkeypatched moved module globals now patch the owning module; the full tests/cli + tests/hermes_cli suites show zero new failures vs a clean main baseline.

History note: originally opened in April against a 10,536-line cli.py; re-extracted from the current head in July per the hermes-sweeper review below (preserving the sync_base worktree setup, lock-classification/pruning safety logic, prefill precedence, and light-mode paths that landed in between).

🤖 Generated with Claude Code

@mxnstrexgl

Copy link
Copy Markdown

🤖 Automated Refactor Review

Summary

Clean extraction of 1,327 lines from cli.py into 3 focused modules (cli_config.py, cli_git.py, cli_display.py). Good modularization effort.

✅ Imports & Dependencies

  • All imports properly preserved
  • No circular import risks detected
  • Module boundaries are clean

✅ API Compatibility

  • Public APIs maintained (, , worktree helpers)
  • No breaking changes to callers

✅ State Management

  • Shared state properly handled via with get/set accessors
  • Clean separation of concerns

⚠️ Issue Found: Missing Variable Reference

cli_config.py line ~98: references which is defined in cli.py, not in the new module. This will cause when called.

Fix: Either:

  1. Pass as parameter to the function, OR
  2. Import/get it via inside the function

✅ Security Review

  • Config handling: No secrets exposed in new structure - API keys still handled via env vars
  • Git operations: Credential handling preserved correctly - worktree operations use subprocess, no credential leaks
  • Display module: No info disclosure - only UI helpers and ANSI formatting

Verdict

NEEDS CHANGES - The reference needs fixing before merge. Otherwise excellent refactor.

@mxnstrexgl

Copy link
Copy Markdown

Automated Refactor Review - PR #12355

Summary

Clean extraction of 1,327 lines from cli.py into 3 focused modules. Good modularization.

Critical Issue

cli_config.py: _load_prefill_messages() uses _hermes_home variable that is NOT defined in the module. This variable was in cli.py but wasn't passed to the new module. Will cause NameError.

Security Review: PASS

  • Config handling: No secrets exposed
  • Git operations: Credential handling preserved
  • Display module: No info disclosure

Verdict: NEEDS CHANGES

Fix the _hermes_home reference before merge. Suggest using get_hermes_home() import inside the function.

@xandgate

Copy link
Copy Markdown
Author

Fixed in 477b7f2.

_load_prefill_messages resolved relative paths against _hermes_home (a module-level var in cli.py, undefined in the extracted module). Replaced with get_hermes_home() — consistent with how load_cli_config and save_config_value already handle it.

# before
path = _hermes_home / path   # NameError in extracted module

# after  
path = get_hermes_home() / path   # consistent with rest of cli_config.py

Good catch — I had patched the two obvious _hermes_home usages in load_cli_config and save_config_value but missed this third one in _load_prefill_messages. All three are now get_hermes_home() calls. Syntax verified clean.

@xandgate
xandgate force-pushed the refactor/cli-modularisation branch from 477b7f2 to 9d31271 Compare April 19, 2026 03:48
@xandgate

Copy link
Copy Markdown
Author

Rebased on current main (9d3127) and conflicts resolved.

What conflicted: Two upstream commits landed while the PR was open:

  • fix(cli): strip all reasoning tag variants from /resume recap — improved _strip_reasoning_tags with re.IGNORECASE, orphan close-tag stripping, and added "thought" to _REASONING_TAGS for Gemma 4
  • fix(agent): strip <think> blocks from stored assistant content — related content processing fix

How resolved:

  • cli.py: kept our extracted import structure (discarded the re-inlined function definitions that came back via upstream)
  • cli_config.py: absorbed all upstream improvements — updated _REASONING_TAGS to match ("thought" added, "THINKING" removed since re.IGNORECASE handles it), and replaced _strip_reasoning_tags with the improved version including re.IGNORECASE flags and orphan close-tag stripping

All four files syntax-verified clean post-rebase.

@alt-glitch alt-glitch added type/refactor Code restructuring, no behavior change P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard labels Apr 23, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused modularization work. This aligns with the repository’s stated goal of extracting coherent clusters from cli.py (AGENTS.md:65-70), but the patch needs a current-head salvage rather than a mechanical conflict resolution.

Problems

  • The proposed cli_git.py has _setup_worktree(repo_root) only, while current main calls _setup_worktree(sync_base=_sync_base) (cli.py:15854) and implements synchronized-base setup at cli.py:1423-1588.
  • Current worktree safety logic must be retained: lock classification is at cli.py:1643-1702, and pruning uses it at cli.py:1846-1949 (added by 74809b4e9).
  • Preserve later behavior in the extracted config/display clusters: prefill precedence is resolved at cli.py:318-334 and used at cli.py:3914; the compact banner fast-start path is at cli.py:3496-3502.

Suggested changes

  • Re-extract the current-head implementations and retain the existing cli.py compatibility exports.
  • Cover the relocated surface with the existing worktree and prefill tests.

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:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 12, 2026
Re-extraction of PR NousResearch#12355 from the current head, per the sweeper
review: the previous branch predated the synchronized-base worktree
setup, worktree lock classification/pruning (74809b4), prefill
precedence resolution, and the light-mode display path, so the modules
are re-cut from today's implementations instead of rebasing the stale
copies.

- cli_config.py: config loading/saving, prefill message loading and
  precedence resolution, reasoning/service-tier parsing
- cli_git.py: worktree isolation incl. sync_base setup, lock
  classification, stale-worktree pruning, orphaned-branch cleanup;
  active-worktree state now lives here behind
  get_active_worktree()/set_active_worktree()
- cli_display.py: ANSI/skin helpers, light-mode detection/remap,
  output-history record/replay, file-drop and attachment detection,
  ChatConsole, banners, slash-command detection

cli.py re-imports every moved name, so existing `from cli import X`
call sites (mixins, tools, tests) keep working. Tests that
monkeypatched moved module globals (_hermes_home, _LIGHT_MODE_CACHE,
_pt_print, _OUTPUT_HISTORY) now patch the module that owns them.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@xandgate
xandgate force-pushed the refactor/cli-modularisation branch from 9d31271 to f469af6 Compare July 12, 2026 20:31
@xandgate

Copy link
Copy Markdown
Author

Re-extracted from the current head per the review above (branch force-pushed; the previous April snapshot is superseded).

Addressing each point:

  • _setup_worktree signaturecli_git.py now carries the current implementation: _setup_worktree(repo_root=None, sync_base=True) including the synchronized-base setup, plus _resolve_worktree_base and the _worktree_has_unpushed_commits / _worktree_is_dirty helpers. The sync_base=_sync_base call site in cli.py is unchanged.
  • Worktree safety logic — lock classification (_worktree_lock_is_live) and the pruning that consumes it (from 74809b4) moved intact into cli_git.py.
  • Later config/display behavior_resolve_prefill_messages_file (prefill precedence) is in cli_config.py; the compact-banner fast-start path and the light-mode detection/remap block are in cli_display.py.
  • Compatibility exports — every moved name is re-imported in cli.py, so from cli import X call sites (mixins, tools/delegate_tool.py, tests) keep working. Active-worktree session state lives in cli_git behind get_active_worktree() / set_active_worktree().
  • Test coverage — the existing worktree suites (test_worktree.py, test_worktree_security.py, test_worktree_sync_base.py) and test_prefill_config.py pass against the relocated surface. Tests that monkeypatched moved module globals (cli._hermes_home, _LIGHT_MODE_CACHE, _pt_print, _OUTPUT_HISTORY) now patch the owning module. The full tests/cli + tests/hermes_cli run has failure parity with a clean main baseline (identical pre-existing flakes, zero new failures).

All moves are verbatim from today's cli.py; the only non-move changes are the re-import shims, the two accessor functions, and dropping imports that became dead in cli.py. Conflicts are resolved — the PR is mergeable again.

@xandgate xandgate changed the title refactor: extract cli_config, cli_git, cli_display from cli.py (−1,327 lines) refactor: extract cli_config, cli_git, cli_display from cli.py (−2,158 lines) Jul 12, 2026
@alt-glitch alt-glitch removed sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Jul 12, 2026
@andrexibiza

Copy link
Copy Markdown
Contributor

Coordination from the god-file decomposition campaign (epic #78647, cli.py target #55136):

This PR overlaps cli.py's R1 region's two biggest extraction candidates:

  • C2 (reasoning/prefill, window 234–407) — this PR removes the whole window
  • C5 (git worktree cluster, windows 1402–2115 + 2213–2497) — same

Our double-blind consensus (2 independent analysts + adjudication, 2026-08-05) found this PR during the open-PR sweep and blocked both candidates on it — no R1 slice will be filed while this is open. If this merges, R1's C2/C5 work is pre-done and we re-derive the remaining cluster map from its diff (per the consensus execution order §4.1).

Standing coordination: the campaign will re-scan the merge state before every R1 scheduling decision. If you'd like the full consensus file for reference: it's summarized at #78647 progress comments; the blocking map is in the campaign's R1-CONSENSUS.md.

No conflicts intended — the extraction wave is proceeding on R2–R5 (windows 5613+), which this PR does not touch.

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-broad Sweeper blast radius: broad — a core path most sessions hit type/refactor Code restructuring, no behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants