Fix history ranges starting past the end by AryanRG · Pull Request #1778 · python-cmd2/cmd2

AryanRG · 2026-10-17T05:04:27Z

History ranges starting past the end no longer select the last command. Invalid indices, including zero, and bounded ranges selecting no items report No history items in range before replay, editing or file output.

Empty action selections also stop before any side effects, including unbounded ranges, omitted selectors and search misses. Displaying an empty selection still succeeds, and the no-argument history --run safety refusal is unchanged.

History.get() and span() raise HistoryIndexError, caught once by _get_history(). Unexpected IndexErrors are not swallowed. Valid negative-range clamping and persisted-history selections are preserved. The API change is documented under 4.4.0.

Added regression coverage, including persisted-file reloads. make check, make test and make docs-test pass locally on macOS / Python 3.12. Hosted CI is rerunning.

Closes #1777

codecov · 2026-10-17T05:05:26Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.75%. Comparing base (1ff9293) to head (9ad0cf8).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1778   +/-   ##
=======================================
  Coverage   99.75%   99.75%           
=======================================
  Files          23       23           
  Lines        6474     6493   +19     
=======================================
+ Hits         6458     6477   +19     
  Misses         16       16

Flag	Coverage Δ
unittests	`99.75% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

tleonhardt

@AryanRG Thanks for the PR!

While I think the 1-line change in cmd2/history.py in this PR is a clever solution for handling the problem where a range past the end of the history inappropriately treating that range as equal to the last command, I think as-is this results in some behavior which feels off an non-intuitive.

As a user, I would expect and prefer the command to fail fast with a clear error message informing me that I have selected an invalid range instead of it silently behaving in a way that at least for some history commands is non-intuitive using an empty-range.

For example, take history -r 5: with only 3 items now does nothing and still reports success. -e opens an empty editor and -o writes an empty file. Before this branch, the same input acted on the last command, which also feels inappropriate.

I think the user should now get an error such as "No history items in range" instead of using an empty range. Fixing this will likely require an edit to cmd2.py somewhere around line 5505. This overall review comment is the important one. The two others in my review are minor.

…/history-range-1778 # Conflicts: # CHANGELOG.md

AryanRG · 2026-10-20T05:02:58Z

@tleonhardt Thanks, that makes sense. Empty indices and ranges now fail with No history items in range before replay, editing or file output. I also removed the redundant clamp and left persisted-history behavior unchanged for a separate discussion.

tleonhardt

The bounds checks live in three places: the integer-index path in _get_history, the IndexError from span(), and the exemption for ranges with no start.

I suggest having History.get() raise the same IndexError("No history items in range"), or better yet a custom exception subclass derived from this, and catching it once in _get_history. That should also fix the history 0 problem, and other callers of get() would get a clear message instead of "list index out of range".

The history command is by far the most complex command built into cmd2. There are a LOT of features which results in a LOT of possible edge cases.

tleonhardt

Ok, it feels like we are making progress! The edge cases I'm finding now are starting to decrease in severity I think.

tleonhardt · 2026-10-22T00:36:02Z

+                index = len(self.history) + int_arg + 1 if int_arg < 0 else int_arg
+                return {index: item}

            if ".." in args.arg or ":" in args.arg:


Searches containing : or .. never run: history /a..b/, history /one:/ and history http://x are sent to span() instead of being searched. They fail with "...may not be zero. The first command in history is command 1." This predates the branch, but the new clean error output now makes it look intended.

The fix is to check for /regex/ first, send only spanpattern matches to span(), and treat everything else as a string search.

tleonhardt · 2026-10-22T00:41:49Z

-            start = min(self._zero_based_index(start_token), len(self) - 1)
+            start = self._zero_based_index(start_token)
            if start < 0:
                start = max(0, len(self) + start)


A typo can replay all history: history -- -100 is rejected, but history -r -- -100: silently clamps to command 1 and runs everything, including persisted commands. A mistyped -30: meant as -3: does the same. The "Cowardly refusing to run all" guard only checks for an empty argument, so it doesn't catch this.

Rejecting a negative start that falls before the first command, as is already done for past-end starts, would fixt this edge case.

tleonhardt · 2026-10-22T00:45:05Z

                start = max(0, len(self) + start)
        else:
-            start = 0 if include_persisted else self.session_start_index
+            start = 0 if include_persisted or end_token else self.session_start_index


history -r :N runs commands from earlier sessions: this is a side effect of the end-only fix. In a fresh session, history -r :3 now runs persisted commands 1–3 without --all, but history : still shows only this session. The behavior is arguably intended, but it's a hazard for -r/-e.

This one is a judgement call. We could keep it as-is or require --all for persisted items with -r/-e. Regardless, we should make sure whatever behavior we leave in place is carefully documented.

@AryanRG @kmvanbrunt If either of you have an opinion on this, please weigh in.

tleonhardt · 2026-10-22T00:54:20Z

            history = self.history.span(":", args.all)
+
+        # An empty display is harmless, but actions must have something to run, edit, or save.
+        if not history and (args.run or args.edit or args.output_file):


Empty-selection check is in the wrong function: it lives in _get_history as a HistoryIndexError. With an empty session, history -o out.txt and no selector reports "No matching history items" even though nothing was searched.

Moving the check to do_history would fix that and leave _get_history returning a plain dict.

tleonhardt · 2026-10-22T00:55:33Z

        if not results:
            # our regex doesn't match the input, bail out
-            raise ValueError("History indices must be positive or negative integers, and may not be zero.")
+            raise HistoryFormatError(f"{INVALID_HISTORY_RANGE} {FIRST_HISTORY_INDEX}")


Misleading "command 1" hint: HistoryFormatError always appends "The first command in history is command 1.", even for input like 1:2:3 or abc: that has no zero in it.

tleonhardt · 2026-10-22T00:56:46Z

        """
        if args.arg:
            try:
                int_arg = int(args.arg)


int() is too permissive: it accepts underscores, a leading +, whitespace and non-ASCII digits. For example, history 1_0 selects command 10, and history -r ١ (an Arabic-Indic digit one) runs command 1.

Checking re.fullmatch(r"-?\d+", arg) first would stop that.

tleonhardt · 2026-10-22T00:57:52Z

        )

        history_arg_help = (
            "empty               all history items\n"


Stale in-app help: help history still says that no argument means "all history items". It also doesn't mention that bounded ranges use the full history, while docs/features/history.md was updated.

tleonhardt · 2026-10-22T00:58:30Z

                pass
+            else:
+                item = self.history.get(int_arg)
+                index = len(self.history) + int_arg + 1 if int_arg < 0 else int_arg


Duplicated index arithmetic: the negative-to-positive conversion is now in three places.

self.history.span(f"{n}:{n}") already handles it.

Fix history ranges starting past the end

7193751

AryanRG requested review from kmvanbrunt and tleonhardt as code owners October 17, 2026 05:04

tleonhardt requested changes Oct 20, 2026

View reviewed changes

Comment thread cmd2/history.py

Comment thread cmd2/history.py Outdated

AryanRG added 2 commits October 20, 2026 10:06

Merge commit '1ff9293d9caaf5ac1cce0e17b9e8d3f4824598fd' into followup…

1ae4e16

…/history-range-1778 # Conflicts: # CHANGELOG.md

Reject empty history selections before performing actions

b1c9466

AryanRG requested a review from tleonhardt October 20, 2026 05:03

AryanRG added 2 commits October 20, 2026 10:38

Use explicit positional separator in history range tests

d1aa374

Clarify empty history selection error in changelog

7f98eb4

tleonhardt requested changes Oct 20, 2026

View reviewed changes

Comment thread cmd2/cmd2.py Outdated

Comment thread cmd2/cmd2.py Outdated

Comment thread cmd2/history.py

Distinguish invalid history ranges from empty sessions

ad622ad

tleonhardt requested changes Oct 20, 2026

View reviewed changes

Comment thread cmd2/history.py Outdated

Comment thread cmd2/cmd2.py Outdated

Comment thread cmd2/cmd2.py Outdated

Centralize history selection errors

feefbf4

tleonhardt requested changes Oct 20, 2026

View reviewed changes

Comment thread cmd2/cmd2.py Outdated

Reject empty history actions before side effects

6b48b75

tleonhardt requested changes Oct 21, 2026

View reviewed changes

Comment thread cmd2/cmd2.py Outdated

Comment thread cmd2/history.py

Comment thread cmd2/history.py Outdated

Comment thread cmd2/cmd2.py Outdated

Comment thread cmd2/cmd2.py Outdated

Comment thread cmd2/history.py Outdated

Comment thread CHANGELOG.md

Comment thread cmd2/cmd2.py Outdated

Make history selection errors and boundaries consistent

9ad0cf8

tleonhardt requested changes Oct 22, 2026

View reviewed changes

Conversation

AryanRG commented Oct 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Oct 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tleonhardt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AryanRG commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tleonhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tleonhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AryanRG commented Oct 17, 2026 •

edited

Loading

codecov Bot commented Oct 17, 2026 •

edited

Loading

tleonhardt left a comment •

edited

Loading

AryanRG commented Oct 20, 2026 •

edited

Loading