Skip to content

fix(cli): keep session alive when a destructive slash confirm is cancelled - #40733

Open
entropy-0x wants to merge 1 commit into
NousResearch:mainfrom
entropy-0x:fix/destructive-slash-cancel-keeps-session
Open

fix(cli): keep session alive when a destructive slash confirm is cancelled#40733
entropy-0x wants to merge 1 commit into
NousResearch:mainfrom
entropy-0x:fix/destructive-slash-cancel-keeps-session

Conversation

@entropy-0x

Copy link
Copy Markdown
Contributor

process_command() is contracted to return bool ("True to continue, False to
exit"), and the REPL loop exits the session when the call is falsy:
if not self.process_command(user_input): self._should_exit = True; app.exit().

The cancel branches of the destructive session commands (/clear, /new, /reset,
/undo) did a bare return (None) when _confirm_destructive_slash() returned
None. Since not None is True, picking "Cancel — keep current conversation"
exited the entire interactive session and dropped the live agent state — the
opposite of the user's intent. The /undo invalid-count branch had the same
bare return. The falsy path also reaches the inline dispatch site.

Return True from those branches so cancelling keeps the REPL running.

What does this PR do?

Fixes a bug where cancelling the confirmation prompt for a destructive session
slash command (/clear, /new, /reset, /undo) exited the whole CLI session
instead of returning to the prompt with the conversation intact. The cancel
branches returned None, which the REPL loop treats as a request to exit.

Related Issue

N/A

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • cli.py: changed the bare return to return True in the cancel branches
    of /clear, /new, and /undo, and in the /undo invalid-count branch
    inside process_command(), honoring its -> bool contract.
  • tests/cli/test_destructive_slash_cancel_keeps_session.py: added a
    regression test asserting process_command() returns True (and does not run
    new_session/undo_last) when the destructive-confirm prompt is cancelled.

How to Test

  1. Run hermes to enter the interactive CLI.
  2. Type /clear (or /new, /reset, /undo) and select
    "Cancel — keep current conversation" at the prompt.
  3. Before the fix the session exits; after the fix the prompt returns with the
    conversation intact.
  4. Automated: pytest tests/cli/test_destructive_slash_cancel_keeps_session.py -q.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15.5

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

…elled

process_command() is contracted to return bool ("True to continue, False to
exit"), and the REPL loop exits the session when the call is falsy:
`if not self.process_command(user_input): self._should_exit = True; app.exit()`.

The cancel branches of the destructive session commands (/clear, /new, /reset,
/undo) did a bare `return` (None) when _confirm_destructive_slash() returned
None. Since `not None` is True, picking "Cancel — keep current conversation"
exited the entire interactive session and dropped the live agent state — the
opposite of the user's intent. The /undo invalid-count branch had the same
bare return. The falsy path also reaches the inline dispatch site.

Return True from those branches so cancelling keeps the REPL running.

## What does this PR do?

Fixes a bug where cancelling the confirmation prompt for a destructive session
slash command (/clear, /new, /reset, /undo) exited the whole CLI session
instead of returning to the prompt with the conversation intact. The cancel
branches returned None, which the REPL loop treats as a request to exit.

## Related Issue

N/A

## Type of Change

- [x] 🐛 Bug fix (non-breaking change that fixes an issue)
- [ ] ✨ New feature (non-breaking change that adds functionality)
- [ ] 🔒 Security fix
- [ ] 📝 Documentation update
- [ ] ✅ Tests (adding or improving test coverage)
- [ ] ♻️ Refactor (no behavior change)
- [ ] 🎯 New skill (bundled or hub)

## Changes Made

- `cli.py`: changed the bare `return` to `return True` in the cancel branches
  of `/clear`, `/new`, and `/undo`, and in the `/undo` invalid-count branch
  inside `process_command()`, honoring its `-> bool` contract.
- `tests/cli/test_destructive_slash_cancel_keeps_session.py`: added a
  regression test asserting `process_command()` returns True (and does not run
  new_session/undo_last) when the destructive-confirm prompt is cancelled.

## How to Test

1. Run `hermes` to enter the interactive CLI.
2. Type `/clear` (or `/new`, `/reset`, `/undo`) and select
   "Cancel — keep current conversation" at the prompt.
3. Before the fix the session exits; after the fix the prompt returns with the
   conversation intact.
4. Automated: `pytest tests/cli/test_destructive_slash_cancel_keeps_session.py -q`.

## Checklist

### Code

- [x] I've read the [Contributing Guide](https://github.com/NousResearch/hermes-agent/blob/main/CONTRIBUTING.md)
- [x] My commit messages follow [Conventional Commits](https://www.conventionalcommits.org/) (`fix(scope):`, `feat(scope):`, etc.)
- [x] I searched for [existing PRs](https://github.com/NousResearch/hermes-agent/pulls) to make sure this isn't a duplicate
- [x] My PR contains **only** changes related to this fix/feature (no unrelated commits)
- [x] I've run `pytest tests/ -q` and all tests pass
- [x] I've added tests for my changes (required for bug fixes, strongly encouraged for features)
- [x] I've tested on my platform: macOS 15.5

### Documentation & Housekeeping

- [x] I've updated relevant documentation (README, `docs/`, docstrings) — or N/A
- [x] I've updated `cli-config.yaml.example` if I added/changed config keys — or N/A
- [x] I've updated `CONTRIBUTING.md` or `AGENTS.md` if I changed architecture or workflows — or N/A
- [x] I've considered cross-platform impact (Windows, macOS) per the [compatibility guide](https://github.com/NousResearch/hermes-agent/blob/main/CONTRIBUTING.md#cross-platform-compatibility) — or N/A
- [x] I've updated tool descriptions/schemas if I changed tool behavior — or N/A
@daimon-nous daimon-nous Bot added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists duplicate This issue or pull request already exists labels Jun 6, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for identifying the process_command() boolean-contract issue. The destructive-confirm cancellation guarantee is already on current main via #40583 (136dae779ec80b33c2ad823f0e9ec2f42435fdf9; cli.py:8496, cli.py:8631, cli.py:8679).

Problems

  • The three confirmation-cancel production hunks therefore overlap current main and will conflict during salvage.

Suggested changes

  • Preserve the separate /undo notanumber fix: current main still has a bare return at cli.py:8666, while the interactive loop exits on a falsy process_command() result at cli.py:15236.
  • Keep or adapt the regression test to cover that invalid-count branch; the confirmation-cancel assertions remain useful regression coverage even though their production fix has already landed.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/sessions Session lifecycle, resume, persistence, history labels Jul 14, 2026

@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

Two PRs address the same falsy-return defect in process_command(): both change destructive-confirm cancellation and invalid /undo count paths from None to True, with regression coverage. Current main already contains the cancellation fixes via #40583, leaving only the invalid-count branch as the production defect to salvage.

Related pull requests

  • #40733 related — (+52/-4) — salvage and merge: The diff fixes all four bare returns and adds fixture-based coverage for cancellation plus invalid /undo counts; as the contributor review notes, the three cancellation production hunks overlap current main, so #40733 should be rebased to retain only the invalid-count fix while preserving useful regression tests.
  • #41019 duplicate — (+97/-4) — duplicate of #40733: Its production diff is identical, while its tests cover the same behavior through a SimpleNamespace stand-in. Despite the keep_open review on #41019, the complete diff shows no distinct production fix beyond #40733, and the review itself requires the same reduction to the remaining invalid-count branch.

Duplicates

#40733 and #41019 contain identical production changes and substantially duplicate regression coverage for the same process_command() return-value defect.

Suggested consolidation

Merge #40733 after reconciling it with current main: retain the /undo invalid-count return True fix and adapt the regression coverage, while dropping the three production hunks already implemented by #40583. Close #41019 as a duplicate of #40733; this does not disregard its keep_open review, because that review identifies the same salvageable invalid-count fix but the diff provides no independent implementation to preserve.

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
    subgraph Dup40733 ["PRs duplicating each other"]
        P40733["PR #40733 (open)"]
        P41019["PR #41019 (open)"]
    end
    class P40733 open
    class P41019 open
    class P40733 target
    click P40733 "https://github.com/NousResearch/hermes-agent/pull/40733"
    click P41019 "https://github.com/NousResearch/hermes-agent/pull/41019"
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 or no verify verdict yet (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 9 kB of PR diffs, 7 kB of issue/PR text, 2 kB of discussion (2 comments), 1 verify verdict. 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

area/sessions Session lifecycle, resume, persistence, history comp/cli CLI entry point, hermes_cli/, setup wizard duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants