From 7193751141742c7bb345d6b4ed8a9e6d5860b3c9 Mon Sep 17 00:00:00 2001 From: AryanRG Date: Sat, 17 Oct 2026 10:34:05 +0530 Subject: [PATCH 01/11] Fix history ranges starting past the end --- CHANGELOG.md | 5 +++++ cmd2/history.py | 2 +- tests/test_history.py | 27 +++++++++++++++++++++++++++ 3 files changed, 33 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f7e7ccd88..12ba87ffb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +## 4.3.1 (TBD) + +- Fixed history ranges starting past the end selecting the last command, including when using + `history --run`. + ## 4.3.0 (October 13, 2026) - Stabilized features diff --git a/cmd2/history.py b/cmd2/history.py index c99a8aac5..7fb3b4317 100644 --- a/cmd2/history.py +++ b/cmd2/history.py @@ -256,7 +256,7 @@ def span(self, span: str, include_persisted: bool = False) -> dict[int, "History start_token = results.group("start") if start_token: - start = min(self._zero_based_index(start_token), len(self) - 1) + start = min(self._zero_based_index(start_token), len(self)) if start < 0: start = max(0, len(self) + start) else: diff --git a/tests/test_history.py b/tests/test_history.py index ca19d2364..6c2a40636 100644 --- a/tests/test_history.py +++ b/tests/test_history.py @@ -228,6 +228,19 @@ def test_history_class_span(hist) -> None: hist.span(tryit) +@pytest.mark.parametrize("suffix", [":", ":999", ":-1", "..", "..999", "..-1"]) +def test_history_span_past_end(hist, suffix) -> None: + assert hist.span(f"{len(hist) + 1}{suffix}") == {} + + +@pytest.mark.parametrize( + ("span", "indices"), + [("4:", [4]), ("1:999", [1, 2, 3, 4]), ("-999:-1", [1, 2, 3, 4]), ("999:1", [])], +) +def test_history_span_boundaries(hist, span, indices) -> None: + assert list(hist.span(span)) == indices + + def test_persisted_history_span(persisted_hist) -> None: span = persisted_hist.span("2..") assert len(span) == 5 @@ -644,6 +657,20 @@ def test_history_with_span_end(base_app) -> None: verify_hi_last_result(base_app, 2) +@pytest.mark.parametrize("span", ["2:", "2:3", "2..", "2..3"]) +def test_history_run_span_past_end(base_app, span) -> None: + run_cmd(base_app, "set debug true") + assert len(base_app.history) == 1 + base_app.debug = False + + out, err = run_cmd(base_app, f"history --run {span}") + + assert base_app.debug is False + assert out == [] + assert err == [] + assert len(base_app.history) == 1 + + def test_history_with_span_index_error(base_app) -> None: run_cmd(base_app, "help") run_cmd(base_app, "help history") From b1c946613927b40db6b72edd70a0494b5daa4c18 Mon Sep 17 00:00:00 2001 From: AryanRG Date: Tue, 20 Oct 2026 10:31:14 +0530 Subject: [PATCH 02/11] Reject empty history selections before performing actions --- cmd2/cmd2.py | 18 +++++- cmd2/history.py | 2 +- tests/test_history.py | 141 ++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 155 insertions(+), 6 deletions(-) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index 94138cb32..f43e96d1d 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -5501,6 +5501,18 @@ def do_history(self, args: argparse.Namespace) -> bool | None: # If an argument was supplied, then retrieve partial contents of the history, otherwise retrieve it all history = self._get_history(args) + # Fail fast on an index or range which selects nothing, before running, editing, or saving anything. + # A string or regex search which finds nothing is not an error. + if not history and args.arg: + try: + int(args.arg) + selects_by_index = True + except ValueError: + selects_by_index = ".." in args.arg or ":" in args.arg + if selects_by_index: + self.perror("No history items in range") + return None + if args.run: if not args.arg: self.perror("Cowardly refusing to run all previously entered commands.") @@ -5555,9 +5567,13 @@ def _get_history(self, args: argparse.Namespace) -> dict[int, HistoryItem]: if args.arg: try: int_arg = int(args.arg) - return {int_arg: self.history.get(int_arg)} except ValueError: pass + else: + # An index past either end selects nothing. Index 0 still raises its own error from get(). + if abs(int_arg) > len(self.history): + return {} + return {int_arg: self.history.get(int_arg)} if ".." in args.arg or ":" in args.arg: # Get a slice of history diff --git a/cmd2/history.py b/cmd2/history.py index 7fb3b4317..1b80356b4 100644 --- a/cmd2/history.py +++ b/cmd2/history.py @@ -256,7 +256,7 @@ def span(self, span: str, include_persisted: bool = False) -> dict[int, "History start_token = results.group("start") if start_token: - start = min(self._zero_based_index(start_token), len(self)) + start = self._zero_based_index(start_token) if start < 0: start = max(0, len(self) + start) else: diff --git a/tests/test_history.py b/tests/test_history.py index 6c2a40636..d47cb9c4b 100644 --- a/tests/test_history.py +++ b/tests/test_history.py @@ -657,18 +657,151 @@ def test_history_with_span_end(base_app) -> None: verify_hi_last_result(base_app, 2) -@pytest.mark.parametrize("span", ["2:", "2:3", "2..", "2..3"]) -def test_history_run_span_past_end(base_app, span) -> None: +# Each of these selects nothing when history holds a single item +EMPTY_HISTORY_SPANS = ["2", "5", "999", "-2", "2:", "2:3", "2..", "2..3", "2:1"] + + +@pytest.fixture +def debug_history_app(base_app) -> cmd2.Cmd: + """App whose only history item is 'set debug true', with debug reset so a rerun is detectable""" run_cmd(base_app, "set debug true") assert len(base_app.history) == 1 base_app.debug = False + return base_app + + +@pytest.mark.parametrize("span", EMPTY_HISTORY_SPANS) +def test_history_empty_span(debug_history_app, span) -> None: + out, err = run_cmd(debug_history_app, f"history {span}") + + assert out == [] + assert err == ["No history items in range"] + assert debug_history_app.last_result is False + + +@pytest.mark.parametrize("span", EMPTY_HISTORY_SPANS) +def test_history_run_empty_span(debug_history_app, span) -> None: + out, err = run_cmd(debug_history_app, f"history --run {span}") + + assert out == [] + assert err == ["No history items in range"] + assert debug_history_app.last_result is False + assert debug_history_app.debug is False + assert len(debug_history_app.history) == 1 + + +@pytest.mark.parametrize("span", EMPTY_HISTORY_SPANS) +def test_history_edit_empty_span(debug_history_app, monkeypatch, span) -> None: + mkstemp_mock = mock.MagicMock(name="mkstemp", wraps=tempfile.mkstemp) + monkeypatch.setattr("cmd2.cmd2.tempfile.mkstemp", mkstemp_mock) + edit_mock = mock.MagicMock(name="run_editor") + monkeypatch.setattr("cmd2.Cmd.run_editor", edit_mock) + run_script_mock = mock.MagicMock(name="do_run_script") + monkeypatch.setattr("cmd2.Cmd.do_run_script", run_script_mock) + + out, err = run_cmd(debug_history_app, f"history --edit {span}") + + assert out == [] + assert err == ["No history items in range"] + assert debug_history_app.last_result is False + mkstemp_mock.assert_not_called() + edit_mock.assert_not_called() + run_script_mock.assert_not_called() + assert debug_history_app.debug is False + + +@pytest.mark.parametrize("span", EMPTY_HISTORY_SPANS) +def test_history_output_file_empty_span(debug_history_app, tmp_path, span) -> None: + existing_file = tmp_path / "existing.txt" + existing_file.write_text("keep me\n") + missing_file = tmp_path / "missing.txt" + + for output_file in (existing_file, missing_file): + out, err = run_cmd(debug_history_app, f'history --output-file "{output_file}" {span}') - out, err = run_cmd(base_app, f"history --run {span}") + assert out == [] + assert err == ["No history items in range"] + assert debug_history_app.last_result is False + assert existing_file.read_text() == "keep me\n" + assert not missing_file.exists() + + +@pytest.mark.parametrize("action", ["", "--run", "--edit", "--output-file"]) +def test_history_index_past_end_with_three_items(base_app, monkeypatch, tmp_path, action) -> None: + run_cmd(base_app, "help") + run_cmd(base_app, "shortcuts") + run_cmd(base_app, "set debug true") + base_app.debug = False + edit_mock = mock.MagicMock(name="run_editor") + monkeypatch.setattr("cmd2.Cmd.run_editor", edit_mock) + output_file = tmp_path / "existing.txt" + output_file.write_text("keep me\n") + if action == "--output-file": + action = f'--output-file "{output_file}"' + + out, err = run_cmd(base_app, f"history {action} 5") + + assert out == [] + assert err == ["No history items in range"] + assert base_app.last_result is False + assert len(base_app.history) == 3 assert base_app.debug is False + edit_mock.assert_not_called() + assert output_file.read_text() == "keep me\n" + + +@pytest.mark.parametrize("span", ["1", "-1", "1:", "-1:"]) +def test_history_run_valid_span(debug_history_app, span) -> None: + _out, err = run_cmd(debug_history_app, f"history --run {span}") + + assert err == [] + assert debug_history_app.last_result is True + assert debug_history_app.debug is True + + +@pytest.mark.parametrize("span", ["1", "-1", "1:", "-1:"]) +def test_history_output_file_valid_span(debug_history_app, tmp_path, span) -> None: + output_file = tmp_path / "out.txt" + output_file.write_text("old contents\n") + + run_cmd(debug_history_app, f'history --output-file "{output_file}" {span}') + + assert debug_history_app.last_result is True + assert output_file.read_text() == "set debug true\n" + + +@pytest.mark.parametrize("span", ["1", "-1", "1:"]) +def test_history_edit_valid_span(debug_history_app, monkeypatch, span) -> None: + edited = [] + + def fake_editor(_self, fname) -> None: + with open(fname) as f: + edited.append(f.read()) + + monkeypatch.setattr("cmd2.Cmd.run_editor", fake_editor) + run_script_mock = mock.MagicMock(name="do_run_script") + monkeypatch.setattr("cmd2.Cmd.do_run_script", run_script_mock) + + _out, err = run_cmd(debug_history_app, f"history --edit {span}") + + assert err == [] + assert edited == ["set debug true\n"] + run_script_mock.assert_called_once() + + +@pytest.mark.parametrize("search", ["no_such_command", "/no_such_regex/"]) +def test_history_search_without_match_is_not_an_error(debug_history_app, search) -> None: + out, err = run_cmd(debug_history_app, f"history {search}") + assert out == [] assert err == [] - assert len(base_app.history) == 1 + assert debug_history_app.last_result == {} + + +def test_history_index_zero_error_unchanged(debug_history_app) -> None: + with pytest.raises(IndexError, match=r"The first command in history is command 1\."): + debug_history_app.onecmd("history 0") def test_history_with_span_index_error(base_app) -> None: From d1aa37483e64b38d7272a8b9d42c605f5f9ec96e Mon Sep 17 00:00:00 2001 From: AryanRG Date: Tue, 20 Oct 2026 10:38:08 +0530 Subject: [PATCH 03/11] Use explicit positional separator in history range tests --- tests/test_history.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/test_history.py b/tests/test_history.py index d47cb9c4b..e1e9e3bd6 100644 --- a/tests/test_history.py +++ b/tests/test_history.py @@ -753,7 +753,7 @@ def test_history_index_past_end_with_three_items(base_app, monkeypatch, tmp_path @pytest.mark.parametrize("span", ["1", "-1", "1:", "-1:"]) def test_history_run_valid_span(debug_history_app, span) -> None: - _out, err = run_cmd(debug_history_app, f"history --run {span}") + _out, err = run_cmd(debug_history_app, f"history --run -- {span}") assert err == [] assert debug_history_app.last_result is True @@ -765,8 +765,9 @@ def test_history_output_file_valid_span(debug_history_app, tmp_path, span) -> No output_file = tmp_path / "out.txt" output_file.write_text("old contents\n") - run_cmd(debug_history_app, f'history --output-file "{output_file}" {span}') + _out, err = run_cmd(debug_history_app, f'history --output-file "{output_file}" -- {span}') + assert err == [] assert debug_history_app.last_result is True assert output_file.read_text() == "set debug true\n" From 7f98eb484a884447e1beb5cf9decb83316846d1f Mon Sep 17 00:00:00 2001 From: AryanRG Date: Tue, 20 Oct 2026 16:01:13 +0530 Subject: [PATCH 04/11] Clarify empty history selection error in changelog --- CHANGELOG.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 05d96ad96..a110ca8f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,8 +3,9 @@ - Bug Fixes - Fixed `@with_annotated` command `--help` output bypassing the application's output stream, leaving redirected help files empty and preventing help from being captured by embedded apps. - - Fixed history ranges starting past the end selecting the last command, including when using - `history --run`. + - Fixed history ranges starting past the end selecting the last command. A `history` index or + range which selects no items now reports `No history items in range` instead of running, + editing, or saving nothing. ## 4.3.0 (October 13, 2026) From ad622ad84a0bcb812419189106339d4b069d3e4a Mon Sep 17 00:00:00 2001 From: AryanRG Date: Tue, 20 Oct 2026 23:47:36 +0530 Subject: [PATCH 05/11] Distinguish invalid history ranges from empty sessions --- CHANGELOG.md | 15 ++++-- cmd2/cmd2.py | 29 +++++------ cmd2/history.py | 7 ++- tests/test_history.py | 117 +++++++++++++++++++++++++++++++++++++++--- 4 files changed, 142 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a110ca8f2..d167923cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,11 +1,20 @@ +## 4.4.0 (TBD) + +- Breaking Changes + - `History.span()` now raises `IndexError` when a range with an explicit start selects no + history items, rather than returning an empty dictionary. Open-start ranges can still return + an empty dictionary, and negative starts still clamp to the beginning of history. +- Bug Fixes + - Fixed history ranges starting past the end selecting the last command. Invalid indices and + explicit-start ranges selecting no items report `No history items in range` before running, + editing, or saving commands. Open-start ranges in an empty session behave like plain + `history`. + ## 4.3.1 (TBD) - Bug Fixes - Fixed `@with_annotated` command `--help` output bypassing the application's output stream, leaving redirected help files empty and preventing help from being captured by embedded apps. - - Fixed history ranges starting past the end selecting the last command. A `history` index or - range which selects no items now reports `No history items in range` instead of running, - editing, or saving nothing. ## 4.3.0 (October 13, 2026) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index f43e96d1d..383c5cc91 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -5501,17 +5501,8 @@ def do_history(self, args: argparse.Namespace) -> bool | None: # If an argument was supplied, then retrieve partial contents of the history, otherwise retrieve it all history = self._get_history(args) - # Fail fast on an index or range which selects nothing, before running, editing, or saving anything. - # A string or regex search which finds nothing is not an error. - if not history and args.arg: - try: - int(args.arg) - selects_by_index = True - except ValueError: - selects_by_index = ".." in args.arg or ":" in args.arg - if selects_by_index: - self.perror("No history items in range") - return None + if history is None: + return None if args.run: if not args.arg: @@ -5559,10 +5550,11 @@ def do_history(self, args: argparse.Namespace) -> bool | None: self.last_result = history return None - def _get_history(self, args: argparse.Namespace) -> dict[int, HistoryItem]: + def _get_history(self, args: argparse.Namespace) -> dict[int, HistoryItem] | None: """If an argument was supplied, then retrieve partial contents of the history; otherwise retrieve entire history. - This function returns a dictionary with history items keyed by their 1-based index in ascending order. + This function returns a dictionary with history items keyed by their 1-based index in ascending order, + or None after reporting an index or range which selects no items. """ if args.arg: try: @@ -5570,14 +5562,19 @@ def _get_history(self, args: argparse.Namespace) -> dict[int, HistoryItem]: except ValueError: pass else: - # An index past either end selects nothing. Index 0 still raises its own error from get(). + # Index 0 still raises its own error from get(). if abs(int_arg) > len(self.history): - return {} + self.perror("No history items in range") + return None return {int_arg: self.history.get(int_arg)} if ".." in args.arg or ":" in args.arg: # Get a slice of history - history = self.history.span(args.arg, args.all) + try: + history = self.history.span(args.arg, args.all) + except IndexError as ex: + self.perror(str(ex)) + return None elif args.arg.startswith(r"/") and args.arg.endswith(r"/"): history = self.history.regex_search(args.arg, args.all) else: diff --git a/cmd2/history.py b/cmd2/history.py index 1b80356b4..51b0642c2 100644 --- a/cmd2/history.py +++ b/cmd2/history.py @@ -229,7 +229,9 @@ def span(self, span: str, include_persisted: bool = False) -> dict[int, "History :param span: string containing an index or a slice :param include_persisted: if True, then retrieve full results including from persisted history :return: a dictionary of history items keyed by their 1-based index in ascending order, - or an empty dictionary if no results were found + or an empty dictionary if an open-start range has no results + :raises ValueError: if the range format is invalid or an index is zero + :raises IndexError: if a range with an explicit start selects no history items This method can accommodate input in any of these forms: @@ -270,6 +272,9 @@ def span(self, span: str, include_persisted: bool = False) -> dict[int, "History else: end = len(self) + if start_token and (start >= len(self) or end <= start): + raise IndexError("No history items in range") + return self._build_result_dictionary(start, end) def str_search(self, search: str, include_persisted: bool = False) -> dict[int, "HistoryItem"]: diff --git a/tests/test_history.py b/tests/test_history.py index e1e9e3bd6..0a0432709 100644 --- a/tests/test_history.py +++ b/tests/test_history.py @@ -230,17 +230,40 @@ def test_history_class_span(hist) -> None: @pytest.mark.parametrize("suffix", [":", ":999", ":-1", "..", "..999", "..-1"]) def test_history_span_past_end(hist, suffix) -> None: - assert hist.span(f"{len(hist) + 1}{suffix}") == {} + with pytest.raises(IndexError, match="No history items in range"): + hist.span(f"{len(hist) + 1}{suffix}") @pytest.mark.parametrize( ("span", "indices"), - [("4:", [4]), ("1:999", [1, 2, 3, 4]), ("-999:-1", [1, 2, 3, 4]), ("999:1", [])], + [("4:", [4]), ("1:999", [1, 2, 3, 4]), ("-999:-1", [1, 2, 3, 4])], ) def test_history_span_boundaries(hist, span, indices) -> None: assert list(hist.span(span)) == indices +@pytest.mark.parametrize("span", ["999:1", "3:1", "2:-4", "1:-999"]) +def test_history_span_explicit_start_without_matches(hist, span) -> None: + with pytest.raises(IndexError, match="No history items in range"): + hist.span(span) + + +@pytest.mark.parametrize("span", ["1:", "-1:"]) +def test_history_span_explicit_start_on_empty_history(hist, span) -> None: + hist.clear() + with pytest.raises(IndexError, match="No history items in range"): + hist.span(span) + + +@pytest.mark.parametrize("span", [":", "..", ":2", ":-999"]) +@pytest.mark.parametrize("empty_history", [False, True]) +def test_history_span_open_start_without_matches(hist, span, empty_history) -> None: + if empty_history: + hist.clear() + hist.start_session() + assert hist.span(span) == {} + + def test_persisted_history_span(persisted_hist) -> None: span = persisted_hist.span("2..") assert len(span) == 5 @@ -661,6 +684,88 @@ def test_history_with_span_end(base_app) -> None: EMPTY_HISTORY_SPANS = ["2", "5", "999", "-2", "2:", "2:3", "2..", "2..3", "2:1"] +@pytest.mark.parametrize("selector", ["", ":", "..", ":2"]) +@pytest.mark.parametrize("empty_history", [False, True]) +def test_history_open_start_on_fresh_session(tmp_path, selector, empty_history) -> None: + history_file = str(tmp_path / "history.dat") + previous_app = cmd2.Cmd(persistent_history_file=history_file) + if not empty_history: + run_cmd(previous_app, "help") + run_cmd(previous_app, "shortcuts") + previous_app._persist_history() + app = cmd2.Cmd(persistent_history_file=history_file) + assert app.history.session_start_index == len(app.history) + + out, err = run_cmd(app, f"history {selector}") + + assert out == [] + assert err == [] + assert app.last_result == {} + + +@pytest.mark.parametrize("action", ["--run", "--edit", "--output-file"]) +@pytest.mark.parametrize("selector", [":", "..", ":2"]) +def test_history_open_start_empty_actions(base_app, hist, monkeypatch, tmp_path, action, selector) -> None: + base_app.history = hist + hist.start_session() + output_file = tmp_path / "out.txt" + output_file.write_text("old contents\n") + edit_mock = mock.MagicMock(name="run_editor") + monkeypatch.setattr(base_app, "run_editor", edit_mock) + run_script_mock = mock.MagicMock(name="do_run_script") + monkeypatch.setattr(base_app, "do_run_script", run_script_mock) + run_mock = mock.MagicMock(name="runcmds_plus_hooks") + monkeypatch.setattr(base_app, "runcmds_plus_hooks", run_mock) + options = f'--output-file "{output_file}"' if action == "--output-file" else action + + _out, err = run_cmd(base_app, f"history {options} {selector}") + + assert err == [] + if action == "--run": + run_mock.assert_called_once_with([]) + assert base_app.last_result is True + elif action == "--edit": + edit_mock.assert_called_once() + run_script_mock.assert_called_once() + else: + assert output_file.read_text() == "" + assert base_app.last_result is True + + +@pytest.mark.parametrize("span", ["3:1", "2:-4", "1:-999"]) +@pytest.mark.parametrize("action", ["", "--run", "--edit", "--output-file"]) +@pytest.mark.parametrize("debug", [False, True]) +def test_history_reversed_span_has_no_side_effects(base_app, hist, monkeypatch, tmp_path, span, action, debug) -> None: + base_app.history = hist + base_app.debug = debug + mkstemp_mock = mock.MagicMock(name="mkstemp") + monkeypatch.setattr("cmd2.cmd2.tempfile.mkstemp", mkstemp_mock) + edit_mock = mock.MagicMock(name="run_editor") + monkeypatch.setattr(base_app, "run_editor", edit_mock) + run_script_mock = mock.MagicMock(name="do_run_script") + monkeypatch.setattr(base_app, "do_run_script", run_script_mock) + run_mock = mock.MagicMock(name="runcmds_plus_hooks") + monkeypatch.setattr(base_app, "runcmds_plus_hooks", run_mock) + existing_file = tmp_path / "existing.txt" + existing_file.write_text("keep me\n") + missing_file = tmp_path / "missing.txt" + + for output_file in (existing_file, missing_file): + options = f'--output-file "{output_file}"' if action == "--output-file" else action + out, err = run_cmd(base_app, f"history {options} {span}") + assert out == [] + assert err == ["No history items in range"] + assert base_app.last_result is False + + mkstemp_mock.assert_not_called() + edit_mock.assert_not_called() + run_script_mock.assert_not_called() + run_mock.assert_not_called() + assert len(base_app.history) == 4 + assert existing_file.read_text() == "keep me\n" + assert not missing_file.exists() + + @pytest.fixture def debug_history_app(base_app) -> cmd2.Cmd: """App whose only history item is 'set debug true', with debug reset so a rerun is detectable""" @@ -751,7 +856,7 @@ def test_history_index_past_end_with_three_items(base_app, monkeypatch, tmp_path assert output_file.read_text() == "keep me\n" -@pytest.mark.parametrize("span", ["1", "-1", "1:", "-1:"]) +@pytest.mark.parametrize("span", ["1", "-1", "1:", "-1:", "-999:"]) def test_history_run_valid_span(debug_history_app, span) -> None: _out, err = run_cmd(debug_history_app, f"history --run -- {span}") @@ -760,7 +865,7 @@ def test_history_run_valid_span(debug_history_app, span) -> None: assert debug_history_app.debug is True -@pytest.mark.parametrize("span", ["1", "-1", "1:", "-1:"]) +@pytest.mark.parametrize("span", ["1", "-1", "1:", "-1:", "-999:"]) def test_history_output_file_valid_span(debug_history_app, tmp_path, span) -> None: output_file = tmp_path / "out.txt" output_file.write_text("old contents\n") @@ -772,7 +877,7 @@ def test_history_output_file_valid_span(debug_history_app, tmp_path, span) -> No assert output_file.read_text() == "set debug true\n" -@pytest.mark.parametrize("span", ["1", "-1", "1:"]) +@pytest.mark.parametrize("span", ["1", "-1", "1:", "-999:"]) def test_history_edit_valid_span(debug_history_app, monkeypatch, span) -> None: edited = [] @@ -784,7 +889,7 @@ def fake_editor(_self, fname) -> None: run_script_mock = mock.MagicMock(name="do_run_script") monkeypatch.setattr("cmd2.Cmd.do_run_script", run_script_mock) - _out, err = run_cmd(debug_history_app, f"history --edit {span}") + _out, err = run_cmd(debug_history_app, f"history --edit -- {span}") assert err == [] assert edited == ["set debug true\n"] From feefbf4e013d5cbcb03052383e51f6ba198532d9 Mon Sep 17 00:00:00 2001 From: AryanRG Date: Wed, 21 Oct 2026 02:54:01 +0530 Subject: [PATCH 06/11] Centralize history selection errors --- CHANGELOG.md | 13 ++--- cmd2/cmd2.py | 43 ++++++++-------- cmd2/history.py | 17 ++++--- tests/test_history.py | 112 +++++++++++++++++++++++++++++++++++------- 4 files changed, 131 insertions(+), 54 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d167923cf..470616c55 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,14 +1,15 @@ ## 4.4.0 (TBD) - Breaking Changes - - `History.span()` now raises `IndexError` when a range with an explicit start selects no - history items, rather than returning an empty dictionary. Open-start ranges can still return - an empty dictionary, and negative starts still clamp to the beginning of history. + - `History.get()` and `History.span()` now raise `HistoryIndexError`, an `IndexError` subclass, + for invalid indices and bounded ranges selecting no history items. This includes end-only + ranges. Unbounded ranges (`:` and `..`) can still return an empty dictionary, and negative + starts still clamp to the beginning of history. Invalid indices, including zero, now use the + message `No history items in range`. - Bug Fixes - Fixed history ranges starting past the end selecting the last command. Invalid indices and - explicit-start ranges selecting no items report `No history items in range` before running, - editing, or saving commands. Open-start ranges in an empty session behave like plain - `history`. + bounded ranges selecting no items report `No history items in range` before running, editing, + or saving commands. Unbounded ranges in an empty session behave like plain `history`. ## 4.3.1 (TBD) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index 383c5cc91..d1de3775c 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -154,6 +154,7 @@ ) from .history import ( History, + HistoryIndexError, HistoryItem, ) from .parsing import ( @@ -5556,32 +5557,28 @@ def _get_history(self, args: argparse.Namespace) -> dict[int, HistoryItem] | Non This function returns a dictionary with history items keyed by their 1-based index in ascending order, or None after reporting an index or range which selects no items. """ - if args.arg: - try: - int_arg = int(args.arg) - except ValueError: - pass - else: - # Index 0 still raises its own error from get(). - if abs(int_arg) > len(self.history): - self.perror("No history items in range") - return None - return {int_arg: self.history.get(int_arg)} - - if ".." in args.arg or ":" in args.arg: - # Get a slice of history + try: + if args.arg: try: + int_arg = int(args.arg) + except ValueError: + pass + else: + return {int_arg: self.history.get(int_arg)} + + if ".." in args.arg or ":" in args.arg: + # Get a slice of history history = self.history.span(args.arg, args.all) - except IndexError as ex: - self.perror(str(ex)) - return None - elif args.arg.startswith(r"/") and args.arg.endswith(r"/"): - history = self.history.regex_search(args.arg, args.all) + elif args.arg.startswith(r"/") and args.arg.endswith(r"/"): + history = self.history.regex_search(args.arg, args.all) + else: + history = self.history.str_search(args.arg, args.all) else: - history = self.history.str_search(args.arg, args.all) - else: - # Get a copy of the history so it doesn't get mutated while we are using it - history = self.history.span(":", args.all) + # Get a copy of the history so it doesn't get mutated while we are using it + history = self.history.span(":", args.all) + except HistoryIndexError as ex: + self.perror(str(ex)) + return None return history def _initialize_history(self, hist_file: str) -> None: diff --git a/cmd2/history.py b/cmd2/history.py index 51b0642c2..9ae3fffbd 100644 --- a/cmd2/history.py +++ b/cmd2/history.py @@ -131,6 +131,10 @@ def from_dict(source_dict: dict[str, Any]) -> "HistoryItem": return HistoryItem(Statement.from_dict(statement_dict)) +class HistoryIndexError(IndexError): + """Raised when a history index is invalid or a bounded range selects no items.""" + + class History(list[HistoryItem]): """A list of [HistoryItem][cmd2.history.HistoryItem] objects with additional methods for searching and managing the list. @@ -190,9 +194,10 @@ def get(self, index: int) -> HistoryItem: :param index: optional item to get :return: a single [cmd2.history.HistoryItem][] + :raises HistoryIndexError: if index is zero or outside the history """ - if index == 0: - raise IndexError("The first command in history is command 1.") + if index == 0 or abs(index) > len(self): + raise HistoryIndexError("No history items in range") if index < 0: return self[index] return self[index - 1] @@ -229,9 +234,9 @@ def span(self, span: str, include_persisted: bool = False) -> dict[int, "History :param span: string containing an index or a slice :param include_persisted: if True, then retrieve full results including from persisted history :return: a dictionary of history items keyed by their 1-based index in ascending order, - or an empty dictionary if an open-start range has no results + or an empty dictionary if an unbounded range (``:`` or ``..``) has no results :raises ValueError: if the range format is invalid or an index is zero - :raises IndexError: if a range with an explicit start selects no history items + :raises HistoryIndexError: if a range with an explicit start or end selects no history items This method can accommodate input in any of these forms: @@ -272,8 +277,8 @@ def span(self, span: str, include_persisted: bool = False) -> dict[int, "History else: end = len(self) - if start_token and (start >= len(self) or end <= start): - raise IndexError("No history items in range") + if (start_token or end_token) and (start >= len(self) or end <= start): + raise HistoryIndexError("No history items in range") return self._build_result_dictionary(start, end) diff --git a/tests/test_history.py b/tests/test_history.py index 0a0432709..a4f486fc8 100644 --- a/tests/test_history.py +++ b/tests/test_history.py @@ -10,6 +10,7 @@ import pytest import cmd2 +from cmd2.history import HistoryIndexError from .conftest import ( normalize, @@ -230,7 +231,7 @@ def test_history_class_span(hist) -> None: @pytest.mark.parametrize("suffix", [":", ":999", ":-1", "..", "..999", "..-1"]) def test_history_span_past_end(hist, suffix) -> None: - with pytest.raises(IndexError, match="No history items in range"): + with pytest.raises(HistoryIndexError, match="No history items in range"): hist.span(f"{len(hist) + 1}{suffix}") @@ -244,26 +245,36 @@ def test_history_span_boundaries(hist, span, indices) -> None: @pytest.mark.parametrize("span", ["999:1", "3:1", "2:-4", "1:-999"]) def test_history_span_explicit_start_without_matches(hist, span) -> None: - with pytest.raises(IndexError, match="No history items in range"): + with pytest.raises(HistoryIndexError, match="No history items in range"): hist.span(span) @pytest.mark.parametrize("span", ["1:", "-1:"]) def test_history_span_explicit_start_on_empty_history(hist, span) -> None: hist.clear() - with pytest.raises(IndexError, match="No history items in range"): + with pytest.raises(HistoryIndexError, match="No history items in range"): hist.span(span) -@pytest.mark.parametrize("span", [":", "..", ":2", ":-999"]) +@pytest.mark.parametrize("span", [":", ".."]) @pytest.mark.parametrize("empty_history", [False, True]) -def test_history_span_open_start_without_matches(hist, span, empty_history) -> None: +def test_history_span_unbounded_without_matches(hist, span, empty_history) -> None: if empty_history: hist.clear() hist.start_session() assert hist.span(span) == {} +@pytest.mark.parametrize("span", [":2", "..2", "..-3", ":-999"]) +@pytest.mark.parametrize("empty_history", [False, True]) +def test_history_span_end_only_without_matches(hist, span, empty_history) -> None: + if empty_history: + hist.clear() + hist.start_session() + with pytest.raises(HistoryIndexError, match="No history items in range"): + hist.span(span) + + def test_persisted_history_span(persisted_hist) -> None: span = persisted_hist.span("2..") assert len(span) == 5 @@ -320,14 +331,33 @@ def test_history_class_get(hist) -> None: assert hist.get(3).statement.raw == "third" assert hist.get(-2) == hist[-2] assert hist.get(-1).statement.raw == "fourth" + assert hist.get(len(hist)) == hist[-1] + assert hist.get(-len(hist)) == hist[0] - with pytest.raises(IndexError): + with pytest.raises(HistoryIndexError, match="No history items in range"): hist.get(0) - with pytest.raises(IndexError): + with pytest.raises(HistoryIndexError, match="No history items in range"): hist.get(5) +@pytest.mark.parametrize("index", [0, 5, -5, 10**100, -(10**100)]) +@pytest.mark.parametrize("empty_history", [False, True]) +def test_history_get_invalid_index_has_clear_error(hist, index, empty_history) -> None: + if empty_history: + hist.clear() + with pytest.raises(HistoryIndexError, match="No history items in range") as error: + hist.get(index) + assert isinstance(error.value, IndexError) + + +@pytest.mark.parametrize("index", [1, -1]) +def test_history_get_on_empty_history(hist, index) -> None: + hist.clear() + with pytest.raises(HistoryIndexError, match="No history items in range"): + hist.get(index) + + def test_history_str_search(hist) -> None: items = hist.str_search("ir") assert len(items) == 2 @@ -681,12 +711,14 @@ def test_history_with_span_end(base_app) -> None: # Each of these selects nothing when history holds a single item -EMPTY_HISTORY_SPANS = ["2", "5", "999", "-2", "2:", "2:3", "2..", "2..3", "2:1"] +EMPTY_HISTORY_SPANS = ["0", "2", "5", "999", "-2", "2:", "2:3", "2..", "2..3", "2:1", ":-999"] -@pytest.mark.parametrize("selector", ["", ":", "..", ":2"]) +@pytest.mark.parametrize( + ("selector", "empty_selection"), [("", False), (":", False), ("..", False), (":2", True), ("..-3", True)] +) @pytest.mark.parametrize("empty_history", [False, True]) -def test_history_open_start_on_fresh_session(tmp_path, selector, empty_history) -> None: +def test_history_selections_on_fresh_session(tmp_path, selector, empty_selection, empty_history) -> None: history_file = str(tmp_path / "history.dat") previous_app = cmd2.Cmd(persistent_history_file=history_file) if not empty_history: @@ -699,13 +731,17 @@ def test_history_open_start_on_fresh_session(tmp_path, selector, empty_history) out, err = run_cmd(app, f"history {selector}") assert out == [] - assert err == [] - assert app.last_result == {} + if empty_selection: + assert err == ["No history items in range"] + assert app.last_result is False + else: + assert err == [] + assert app.last_result == {} @pytest.mark.parametrize("action", ["--run", "--edit", "--output-file"]) -@pytest.mark.parametrize("selector", [":", "..", ":2"]) -def test_history_open_start_empty_actions(base_app, hist, monkeypatch, tmp_path, action, selector) -> None: +@pytest.mark.parametrize("selector", [":", ".."]) +def test_history_unbounded_empty_actions(base_app, hist, monkeypatch, tmp_path, action, selector) -> None: base_app.history = hist hist.start_session() output_file = tmp_path / "out.txt" @@ -732,11 +768,29 @@ def test_history_open_start_empty_actions(base_app, hist, monkeypatch, tmp_path, assert base_app.last_result is True -@pytest.mark.parametrize("span", ["3:1", "2:-4", "1:-999"]) +@pytest.mark.parametrize("span", [":2", "..2", "..-3"]) +def test_history_end_only_with_all_includes_persisted(base_app, hist, span) -> None: + base_app.history = hist + hist.start_session() + + out, err = run_cmd(base_app, f"history --all {span}") + + assert len(out) == 2 + assert err == [] + assert list(base_app.last_result) == [1, 2] + + +@pytest.mark.parametrize( + ("span", "session_start"), + [("3:1", 0), ("2:-4", 0), ("1:-999", 0), (":1", 2), ("..2", 2), ("..-3", 2), (":2", 4), ("..2", 4), ("..-3", 4)], +) @pytest.mark.parametrize("action", ["", "--run", "--edit", "--output-file"]) @pytest.mark.parametrize("debug", [False, True]) -def test_history_reversed_span_has_no_side_effects(base_app, hist, monkeypatch, tmp_path, span, action, debug) -> None: +def test_history_empty_bounded_span_has_no_side_effects( + base_app, hist, monkeypatch, tmp_path, span, session_start, action, debug +) -> None: base_app.history = hist + hist.session_start_index = session_start base_app.debug = debug mkstemp_mock = mock.MagicMock(name="mkstemp") monkeypatch.setattr("cmd2.cmd2.tempfile.mkstemp", mkstemp_mock) @@ -905,9 +959,29 @@ def test_history_search_without_match_is_not_an_error(debug_history_app, search) assert debug_history_app.last_result == {} -def test_history_index_zero_error_unchanged(debug_history_app) -> None: - with pytest.raises(IndexError, match=r"The first command in history is command 1\."): - debug_history_app.onecmd("history 0") +@pytest.mark.parametrize("debug", [False, True]) +def test_history_index_zero_has_clean_error(debug_history_app, debug) -> None: + debug_history_app.debug = debug + out, err = run_cmd(debug_history_app, "history 0") + assert out == [] + assert err == ["No history items in range"] + assert debug_history_app.last_result is False + + +@pytest.mark.parametrize("selector", ["1", "1:"]) +@pytest.mark.parametrize("debug", [False, True]) +def test_history_does_not_hide_unexpected_index_errors(base_app, hist, monkeypatch, selector, debug) -> None: + base_app.history = hist + base_app.debug = debug + monkeypatch.setattr(type(hist), "__getitem__", mock.Mock(side_effect=IndexError("unexpected lookup failure"))) + + with pytest.raises(IndexError, match="unexpected lookup failure"): + base_app.onecmd(f"history {selector}") + + out, err = run_cmd(base_app, f"history {selector}") + assert out == [] + assert err != ["No history items in range"] + assert any("unexpected lookup failure" in line for line in err) def test_history_with_span_index_error(base_app) -> None: From 6b48b75e74e0c14bcf99b3ecbe97c77ea6a410ec Mon Sep 17 00:00:00 2001 From: AryanRG Date: Wed, 21 Oct 2026 12:13:24 +0530 Subject: [PATCH 07/11] Reject empty history actions before side effects --- CHANGELOG.md | 5 +- cmd2/cmd2.py | 22 +++++---- tests/test_history.py | 107 +++++++++++++++++++++++++++++++++++++++--- 3 files changed, 119 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 470616c55..8463c9201 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,10 @@ - Bug Fixes - Fixed history ranges starting past the end selecting the last command. Invalid indices and bounded ranges selecting no items report `No history items in range` before running, editing, - or saving commands. Unbounded ranges in an empty session behave like plain `history`. + or saving commands. Empty selections from unbounded ranges, omitted selectors, or searches + with no match also fail before replay, editing, or file output. Displaying an empty unbounded + selection or search still prints nothing without an error. The no-argument `history --run` + safety refusal is unchanged. ## 4.3.1 (TBD) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index d1de3775c..4f7cf5f76 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -5499,21 +5499,27 @@ def do_history(self, args: argparse.Namespace) -> bool | None: return None + if args.run and not args.arg: + self.perror("Cowardly refusing to run all previously entered commands.") + self.perror("If this is what you want to do, specify '1:' as the range of history.") + return None + # If an argument was supplied, then retrieve partial contents of the history, otherwise retrieve it all history = self._get_history(args) if history is None: return None + # 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): + self.perror("No history items in range") + return None + if args.run: - if not args.arg: - self.perror("Cowardly refusing to run all previously entered commands.") - self.perror("If this is what you want to do, specify '1:' as the range of history.") - else: - stop = self.runcmds_plus_hooks(list(history.values())) - self.last_result = True - return stop - elif args.edit: + stop = self.runcmds_plus_hooks(list(history.values())) + self.last_result = True + return stop + if args.edit: fd, fname = tempfile.mkstemp(suffix=".txt", text=True) fobj: TextIO with os.fdopen(fd, "w") as fobj: diff --git a/tests/test_history.py b/tests/test_history.py index a4f486fc8..1a296a38f 100644 --- a/tests/test_history.py +++ b/tests/test_history.py @@ -740,34 +740,129 @@ def test_history_selections_on_fresh_session(tmp_path, selector, empty_selection @pytest.mark.parametrize("action", ["--run", "--edit", "--output-file"]) -@pytest.mark.parametrize("selector", [":", ".."]) -def test_history_unbounded_empty_actions(base_app, hist, monkeypatch, tmp_path, action, selector) -> None: +@pytest.mark.parametrize( + ("selector", "empty_history", "include_persisted"), + [ + (selector, empty_history, include_persisted) + for selector in ("", '""', ":", "..", '" : "', '" "', "no_such_command", "/no_such_regex/") + for empty_history in (False, True) + for include_persisted in (False, True) + if empty_history or not include_persisted or selector in ('" "', "no_such_command", "/no_such_regex/") + ], +) +@pytest.mark.parametrize("debug", [False, True]) +def test_history_empty_actions_have_no_side_effects( + base_app, hist, monkeypatch, tmp_path, action, selector, empty_history, include_persisted, debug +) -> None: base_app.history = hist + base_app.debug = debug + if empty_history: + hist.clear() hist.start_session() + initial_history = list(hist) output_file = tmp_path / "out.txt" output_file.write_text("old contents\n") + missing_file = tmp_path / "missing.txt" + mkstemp_mock = mock.MagicMock(name="mkstemp", wraps=tempfile.mkstemp) + monkeypatch.setattr("cmd2.cmd2.tempfile.mkstemp", mkstemp_mock) edit_mock = mock.MagicMock(name="run_editor") monkeypatch.setattr(base_app, "run_editor", edit_mock) run_script_mock = mock.MagicMock(name="do_run_script") monkeypatch.setattr(base_app, "do_run_script", run_script_mock) run_mock = mock.MagicMock(name="runcmds_plus_hooks") monkeypatch.setattr(base_app, "runcmds_plus_hooks", run_mock) + for target in (output_file, missing_file): + options = f'--output-file "{target}"' if action == "--output-file" else action + all_option = "--all" if include_persisted else "" + _out, err = run_cmd(base_app, f"history {all_option} {options} {selector}") + if action == "--run" and selector in ("", '""'): + assert err == [ + "Cowardly refusing to run all previously entered commands.", + "If this is what you want to do, specify '1:' as the range of history.", + ] + else: + assert err == ["No history items in range"] + assert base_app.last_result is False + + mkstemp_mock.assert_not_called() + edit_mock.assert_not_called() + run_script_mock.assert_not_called() + run_mock.assert_not_called() + assert output_file.read_text() == "old contents\n" + assert not missing_file.exists() + assert list(hist) == initial_history + + +@pytest.mark.parametrize("action", ["--run", "--edit", "--output-file"]) +@pytest.mark.parametrize("selector", [":", "..", '" : "', "first", "/first/"]) +@pytest.mark.parametrize("include_persisted", [False, True]) +def test_history_nonempty_actions_are_preserved( + base_app, hist, monkeypatch, tmp_path, action, selector, include_persisted +) -> None: + base_app.history = hist + if include_persisted: + hist.start_session() + expected = list(hist[:1] if selector in ("first", "/first/") else hist) + expected_text = "".join(f"{item.raw}\n" for item in expected) + output_file = tmp_path / "out.txt" + output_file.write_text("old contents\n") + run_mock = mock.Mock(return_value=False) + monkeypatch.setattr(base_app, "runcmds_plus_hooks", run_mock) + edited = [] + + def fake_editor(fname) -> None: + with open(fname) as script: + edited.append(script.read()) + + monkeypatch.setattr(base_app, "run_editor", fake_editor) + run_script_mock = mock.Mock() + monkeypatch.setattr(base_app, "do_run_script", run_script_mock) options = f'--output-file "{output_file}"' if action == "--output-file" else action + all_option = "--all" if include_persisted else "" - _out, err = run_cmd(base_app, f"history {options} {selector}") + _out, err = run_cmd(base_app, f"history {all_option} {options} {selector}") assert err == [] if action == "--run": - run_mock.assert_called_once_with([]) + run_mock.assert_called_once_with(expected) assert base_app.last_result is True elif action == "--edit": - edit_mock.assert_called_once() + assert edited == [expected_text] run_script_mock.assert_called_once() else: - assert output_file.read_text() == "" + assert output_file.read_text() == expected_text assert base_app.last_result is True +@pytest.mark.parametrize("action", ["--run", "--edit", "--output-file"]) +def test_history_unbounded_empty_action_after_reload(base_app, monkeypatch, tmp_path, action) -> None: + history_file = tmp_path / "history.dat" + base_app.persistent_history_file = str(history_file) + run_cmd(base_app, "help") + base_app._persist_history() + base_app._initialize_history(str(history_file)) + assert len(base_app.history) == base_app.history.session_start_index == 1 + output_file = tmp_path / "out.txt" + output_file.write_text("keep me\n") + mkstemp_mock = mock.Mock(wraps=tempfile.mkstemp) + monkeypatch.setattr("cmd2.cmd2.tempfile.mkstemp", mkstemp_mock) + editor = mock.Mock() + replay = mock.Mock() + monkeypatch.setattr(base_app, "run_editor", editor) + monkeypatch.setattr(base_app, "runcmds_plus_hooks", replay) + options = f'--output-file "{output_file}"' if action == "--output-file" else action + + out, err = run_cmd(base_app, f"history {options} :") + + assert out == [] + assert err == ["No history items in range"] + assert base_app.last_result is False + assert output_file.read_text() == "keep me\n" + mkstemp_mock.assert_not_called() + editor.assert_not_called() + replay.assert_not_called() + + @pytest.mark.parametrize("span", [":2", "..2", "..-3"]) def test_history_end_only_with_all_includes_persisted(base_app, hist, span) -> None: base_app.history = hist From 9ad0cf821d03d39e15e1ab58a4233040c5b9ab21 Mon Sep 17 00:00:00 2001 From: AryanRG Date: Wed, 21 Oct 2026 20:44:27 +0530 Subject: [PATCH 08/11] Make history selection errors and boundaries consistent --- CHANGELOG.md | 24 ++++--- cmd2/cmd2.py | 64 ++++++++--------- cmd2/history.py | 28 +++++--- docs/features/history.md | 23 ++++-- tests/test_history.py | 149 +++++++++++++++++++++++++++++++++++---- 5 files changed, 222 insertions(+), 66 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8463c9201..eab98958b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,17 +2,25 @@ - Breaking Changes - `History.get()` and `History.span()` now raise `HistoryIndexError`, an `IndexError` subclass, - for invalid indices and bounded ranges selecting no history items. This includes end-only - ranges. Unbounded ranges (`:` and `..`) can still return an empty dictionary, and negative - starts still clamp to the beginning of history. Invalid indices, including zero, now use the - message `No history items in range`. + for invalid indices and bounded ranges selecting no history items. Unbounded ranges (`:` and + `..`) can still return an empty dictionary, and negative starts still clamp to the beginning + of history. Malformed ranges, including zero endpoints, raise `HistoryFormatError`, a + `ValueError` subclass. The `history` command reports these selection errors without a + traceback hint, and index zero includes the reminder that the first command is command 1. + - End-only ranges now start at command 1 in the full history, including persisted commands, just + like explicit-start ranges and single indices. Bare `history` and unbounded ranges still + select only the current session unless `--all` is supplied. + - Running, editing, or saving an empty history selection now fails before replay, opening an + editor, or creating or overwriting a file. This includes `history -o FILE` and `history -e` + with no selector in an empty session, and actions on searches with no matches. Empty unbounded + selections and search misses report `No matching history items` for these actions; displaying + them still succeeds without output. The no-argument `history --run` safety refusal is + unchanged. - Bug Fixes - Fixed history ranges starting past the end selecting the last command. Invalid indices and bounded ranges selecting no items report `No history items in range` before running, editing, - or saving commands. Empty selections from unbounded ranges, omitted selectors, or searches - with no match also fail before replay, editing, or file output. Displaying an empty unbounded - selection or search still prints nothing without an error. The no-argument `history --run` - safety refusal is unchanged. + or saving commands. + - Negative single-index history selections now display and return the positive command number. ## 4.3.1 (TBD) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index 4f7cf5f76..d6b8e7adc 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -153,7 +153,9 @@ SkipPostcommandHooks, ) from .history import ( + NO_MATCHING_HISTORY_ITEMS, History, + HistoryFormatError, HistoryIndexError, HistoryItem, ) @@ -5505,14 +5507,10 @@ def do_history(self, args: argparse.Namespace) -> bool | None: return None # If an argument was supplied, then retrieve partial contents of the history, otherwise retrieve it all - history = self._get_history(args) - - if history is None: - return None - - # 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): - self.perror("No history items in range") + try: + history = self._get_history(args) + except (HistoryIndexError, HistoryFormatError) as ex: + self.perror(str(ex)) return None if args.run: @@ -5557,34 +5555,36 @@ def do_history(self, args: argparse.Namespace) -> bool | None: self.last_result = history return None - def _get_history(self, args: argparse.Namespace) -> dict[int, HistoryItem] | None: + def _get_history(self, args: argparse.Namespace) -> dict[int, HistoryItem]: """If an argument was supplied, then retrieve partial contents of the history; otherwise retrieve entire history. - This function returns a dictionary with history items keyed by their 1-based index in ascending order, - or None after reporting an index or range which selects no items. + This function returns a dictionary with history items keyed by their positive 1-based index in ascending order. + Selection errors, including empty selections for actions, propagate to the caller. """ - try: - if args.arg: - try: - int_arg = int(args.arg) - except ValueError: - pass - else: - return {int_arg: self.history.get(int_arg)} - - if ".." in args.arg or ":" in args.arg: - # Get a slice of history - history = self.history.span(args.arg, args.all) - elif args.arg.startswith(r"/") and args.arg.endswith(r"/"): - history = self.history.regex_search(args.arg, args.all) - else: - history = self.history.str_search(args.arg, args.all) + if args.arg: + try: + int_arg = int(args.arg) + except ValueError: + pass else: - # Get a copy of the history so it doesn't get mutated while we are using it - history = self.history.span(":", args.all) - except HistoryIndexError as ex: - self.perror(str(ex)) - return None + item = self.history.get(int_arg) + 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: + # Get a slice of history + history = self.history.span(args.arg, args.all) + elif args.arg.startswith(r"/") and args.arg.endswith(r"/"): + history = self.history.regex_search(args.arg, args.all) + else: + history = self.history.str_search(args.arg, args.all) + else: + # Get a copy of the history so it doesn't get mutated while we are using it + 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): + raise HistoryIndexError(NO_MATCHING_HISTORY_ITEMS) return history def _initialize_history(self, hist_file: str) -> None: diff --git a/cmd2/history.py b/cmd2/history.py index 9ae3fffbd..47602c641 100644 --- a/cmd2/history.py +++ b/cmd2/history.py @@ -18,6 +18,11 @@ shlex_split, ) +NO_HISTORY_ITEMS = "No history items in range" +NO_MATCHING_HISTORY_ITEMS = "No matching history items" +FIRST_HISTORY_INDEX = "The first command in history is command 1." +INVALID_HISTORY_RANGE = "History indices must be positive or negative integers, and may not be zero." + def single_line_format(statement: Statement) -> str: """Format a command line to display on a single line. @@ -135,6 +140,10 @@ class HistoryIndexError(IndexError): """Raised when a history index is invalid or a bounded range selects no items.""" +class HistoryFormatError(ValueError): + """Raised when a history range has invalid syntax or a zero endpoint.""" + + class History(list[HistoryItem]): """A list of [HistoryItem][cmd2.history.HistoryItem] objects with additional methods for searching and managing the list. @@ -196,8 +205,10 @@ def get(self, index: int) -> HistoryItem: :return: a single [cmd2.history.HistoryItem][] :raises HistoryIndexError: if index is zero or outside the history """ - if index == 0 or abs(index) > len(self): - raise HistoryIndexError("No history items in range") + if index == 0: + raise HistoryIndexError(f"{NO_HISTORY_ITEMS}. {FIRST_HISTORY_INDEX}") + if abs(index) > len(self): + raise HistoryIndexError(NO_HISTORY_ITEMS) if index < 0: return self[index] return self[index - 1] @@ -232,10 +243,11 @@ def span(self, span: str, include_persisted: bool = False) -> dict[int, "History """Return a slice of the History list. :param span: string containing an index or a slice - :param include_persisted: if True, then retrieve full results including from persisted history + :param include_persisted: if True, include persisted history for unbounded ranges. + Ranges with either endpoint always use the full history. :return: a dictionary of history items keyed by their 1-based index in ascending order, or an empty dictionary if an unbounded range (``:`` or ``..``) has no results - :raises ValueError: if the range format is invalid or an index is zero + :raises HistoryFormatError: if the range format is invalid or an index is zero :raises HistoryIndexError: if a range with an explicit start or end selects no history items This method can accommodate input in any of these forms: @@ -259,17 +271,17 @@ def span(self, span: str, include_persisted: bool = False) -> dict[int, "History results = self.spanpattern.search(span) 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}") start_token = results.group("start") + end_token = results.group("end") if start_token: start = self._zero_based_index(start_token) if start < 0: 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 - end_token = results.group("end") if end_token: end = min(int(end_token), len(self)) if end < 0: @@ -278,7 +290,7 @@ def span(self, span: str, include_persisted: bool = False) -> dict[int, "History end = len(self) if (start_token or end_token) and (start >= len(self) or end <= start): - raise HistoryIndexError("No history items in range") + raise HistoryIndexError(NO_HISTORY_ITEMS) return self._build_result_dictionary(start, end) diff --git a/docs/features/history.md b/docs/features/history.md index c9ece9765..6580d4329 100644 --- a/docs/features/history.md +++ b/docs/features/history.md @@ -87,8 +87,10 @@ separated by `..` or `:`, and you will see all commands between, and including, 2 alias create two !echo two 3 alias create three !echo three -If you omit the first number, it will start at the beginning. If you omit the last number, it will -continue to the end: +Command numbers refer to the full history, including commands loaded from a persistent history file. +A single number or a range with either endpoint can select those commands without `--all`. If you +omit the first number, the range starts at command 1. If you omit the last number, it continues to +the end of the full history: (Cmd) history :2 1 alias create one !echo one @@ -98,6 +100,11 @@ continue to the end: 3 alias create three !echo three 4 alias create four !echo four +With both endpoints omitted (`:` or `..`), only the current session is selected, as with plain +`history`. Use `--all` to include persisted commands in these unbounded selections or in searches. +For example, after loading five persisted commands into a fresh session, `history :5` and +`history 1:5` both display commands 1 through 5, while `history :` displays nothing. + If you want to display the last three commands entered: (Cmd) history -- -3: @@ -162,11 +169,19 @@ file are executed. To edit and then re-run commands 2-4 you would: If you want to save the commands to a text file, but not edit and re-run them, use the `-o` or `--output-file` option. This is a great way to create [Scripts](./scripting.md), which can be -executed using the `run_script` command. To save the first 5 commands entered in this session to a -text file: +executed using the `run_script` command. To save commands 1 through 5 from the full history, +including persisted commands, to a text file: (Cmd) history :5 -o history.txt +To save only commands entered in the current session, use `history -o history.txt`. Running, +editing, or saving an empty selection reports an error before executing commands, opening an editor, +or creating or overwriting a file. Empty searches report `No matching history items` for these +actions; displaying a search with no matches succeeds without output. The same display behavior +applies to empty unbounded selections. Invalid indices and bounded ranges selecting no commands +report `No history items in range`. Index zero also includes a reminder that the first command is +command 1. Malformed ranges report a format error. + The last action the history command can perform is to clear the command history using `-c` or `--clear`: diff --git a/tests/test_history.py b/tests/test_history.py index 1a296a38f..e34e397cb 100644 --- a/tests/test_history.py +++ b/tests/test_history.py @@ -2,6 +2,7 @@ import contextlib import os +import re import tempfile from unittest import ( mock, @@ -10,7 +11,7 @@ import pytest import cmd2 -from cmd2.history import HistoryIndexError +from cmd2.history import HistoryFormatError, HistoryIndexError from .conftest import ( normalize, @@ -265,7 +266,7 @@ def test_history_span_unbounded_without_matches(hist, span, empty_history) -> No assert hist.span(span) == {} -@pytest.mark.parametrize("span", [":2", "..2", "..-3", ":-999"]) +@pytest.mark.parametrize("span", [":-999", "..-999"]) @pytest.mark.parametrize("empty_history", [False, True]) def test_history_span_end_only_without_matches(hist, span, empty_history) -> None: if empty_history: @@ -316,8 +317,8 @@ def test_persisted_history_span(persisted_hist) -> None: assert span[5].statement.raw == "fifth" span = persisted_hist.span(":-2", include_persisted=False) - assert len(span) == 1 - assert span[5].statement.raw == "fifth" + assert list(span) == [1, 2, 3, 4, 5] + assert span == persisted_hist.span("1:-2") value_errors = ["fred", "fred:joe", "2", "-2", "a..b", "2 ..", "1 : 3", "1:0", "0:3"] expected_err = "History indices must be positive or negative integers, and may not be zero." @@ -711,11 +712,11 @@ def test_history_with_span_end(base_app) -> None: # Each of these selects nothing when history holds a single item -EMPTY_HISTORY_SPANS = ["0", "2", "5", "999", "-2", "2:", "2:3", "2..", "2..3", "2:1", ":-999"] +EMPTY_HISTORY_SPANS = ["2", "5", "999", "-2", "2:", "2:3", "2..", "2..3", "2:1", ":-999"] @pytest.mark.parametrize( - ("selector", "empty_selection"), [("", False), (":", False), ("..", False), (":2", True), ("..-3", True)] + ("selector", "empty_selection"), [("", False), (":", False), ("..", False), (":-999", True), ("..-999", True)] ) @pytest.mark.parametrize("empty_history", [False, True]) def test_history_selections_on_fresh_session(tmp_path, selector, empty_selection, empty_history) -> None: @@ -781,7 +782,7 @@ def test_history_empty_actions_have_no_side_effects( "If this is what you want to do, specify '1:' as the range of history.", ] else: - assert err == ["No history items in range"] + assert err == ["No matching history items"] assert base_app.last_result is False mkstemp_mock.assert_not_called() @@ -855,7 +856,7 @@ def test_history_unbounded_empty_action_after_reload(base_app, monkeypatch, tmp_ out, err = run_cmd(base_app, f"history {options} :") assert out == [] - assert err == ["No history items in range"] + assert err == ["No matching history items"] assert base_app.last_result is False assert output_file.read_text() == "keep me\n" mkstemp_mock.assert_not_called() @@ -877,7 +878,7 @@ def test_history_end_only_with_all_includes_persisted(base_app, hist, span) -> N @pytest.mark.parametrize( ("span", "session_start"), - [("3:1", 0), ("2:-4", 0), ("1:-999", 0), (":1", 2), ("..2", 2), ("..-3", 2), (":2", 4), ("..2", 4), ("..-3", 4)], + [(span, start) for span in ("3:1", "2:-4", "1:-999", ":-999", "..-999") for start in (0, 2, 4)], ) @pytest.mark.parametrize("action", ["", "--run", "--edit", "--output-file"]) @pytest.mark.parametrize("debug", [False, True]) @@ -1055,11 +1056,12 @@ def test_history_search_without_match_is_not_an_error(debug_history_app, search) @pytest.mark.parametrize("debug", [False, True]) -def test_history_index_zero_has_clean_error(debug_history_app, debug) -> None: +@pytest.mark.parametrize("selector", ["0", "-0"]) +def test_history_index_zero_has_clean_error(debug_history_app, debug, selector) -> None: debug_history_app.debug = debug - out, err = run_cmd(debug_history_app, "history 0") + out, err = run_cmd(debug_history_app, f"history {selector}") assert out == [] - assert err == ["No history items in range"] + assert err == ["No history items in range. The first command in history is command 1."] assert debug_history_app.last_result is False @@ -1084,8 +1086,127 @@ def test_history_with_span_index_error(base_app) -> None: run_cmd(base_app, "help history") run_cmd(base_app, "!ls -hal :") expected_err = "History indices must be positive or negative integers, and may not be zero." - with pytest.raises(ValueError, match=expected_err): - base_app.onecmd('history "hal :"') + out, err = run_cmd(base_app, 'history "hal :"') + assert out == [] + assert " ".join(err) == f"{expected_err} The first command in history is command 1." + assert base_app.last_result is False + + +@pytest.mark.parametrize("separator", [":", ".."]) +@pytest.mark.parametrize("session_start", [0, 2, 4]) +@pytest.mark.parametrize("include_persisted", [False, True]) +@pytest.mark.parametrize(("end", "expected"), [("2", [1, 2]), ("-3", [1, 2]), ("4", [1, 2, 3, 4]), ("999", [1, 2, 3, 4])]) +def test_history_bounded_ranges_use_absolute_indices(hist, separator, session_start, include_persisted, end, expected) -> None: + hist.session_start_index = session_start + assert list(hist.span(f"{separator}{end}", include_persisted)) == expected + assert list(hist.span(f"1{separator}{end}", include_persisted)) == expected + assert list(hist.span(f"-999{separator}{end}", include_persisted)) == expected + assert list(hist.span(separator, include_persisted)) == list(range(1 if include_persisted else session_start + 1, 5)) + + +@pytest.mark.parametrize("selector", [":2", "..2", "1:2", "1..2", ":-1", "..-1", "-2:", "-2.."]) +@pytest.mark.parametrize("action", ["", "--run", "--edit", "--output-file"]) +@pytest.mark.parametrize("include_persisted", [False, True]) +def test_history_bounded_actions_after_reload(base_app, monkeypatch, tmp_path, selector, action, include_persisted) -> None: + base_app.persistent_history_file = str(tmp_path / "history.dat") + run_cmd(base_app, "help") + run_cmd(base_app, "shortcuts") + base_app._persist_history() + base_app._initialize_history(base_app.persistent_history_file) + assert base_app.history.session_start_index == len(base_app.history) == 2 + expected = list(base_app.history) + expected_text = "help\nshortcuts\n" + output_file = tmp_path / "commands.txt" + replay = mock.Mock(return_value=False) + monkeypatch.setattr(base_app, "runcmds_plus_hooks", replay) + edited = [] + + def fake_editor(fname) -> None: + with open(fname) as script: + edited.append(script.read()) + + monkeypatch.setattr(base_app, "run_editor", fake_editor) + run_script = mock.Mock() + monkeypatch.setattr(base_app, "do_run_script", run_script) + options = f'--output-file "{output_file}"' if action == "--output-file" else action + all_option = "--all" if include_persisted else "" + out, err = run_cmd(base_app, f"history {all_option} {options} -- {selector}") + assert err == [] + if action == "--run": + replay.assert_called_once_with(expected) + assert base_app.last_result is True + elif action == "--edit": + assert edited == [expected_text] + run_script.assert_called_once() + elif action == "--output-file": + assert output_file.read_text() == expected_text + assert base_app.last_result is True + else: + assert out == [" 1 help", " 2 shortcuts"] + assert base_app.last_result == {1: expected[0], 2: expected[1]} + + +@pytest.mark.parametrize("selector", ["0", "-0", "0:2", "0:", "1:0", ":0", "-0..2", "..0", "1:2:3", "1..2:3", "a:b"]) +@pytest.mark.parametrize("action", ["", "--run", "--edit", "--output-file"]) +@pytest.mark.parametrize("debug", [False, True]) +def test_history_invalid_selection_has_no_side_effects(base_app, monkeypatch, tmp_path, selector, action, debug) -> None: + run_cmd(base_app, "help") + base_app.debug = debug + original_history = list(base_app.history) + effects = [mock.Mock() for _ in range(4)] + monkeypatch.setattr("cmd2.cmd2.tempfile.mkstemp", effects[0]) + monkeypatch.setattr(base_app, "run_editor", effects[1]) + monkeypatch.setattr(base_app, "do_run_script", effects[2]) + monkeypatch.setattr(base_app, "runcmds_plus_hooks", effects[3]) + existing = tmp_path / "existing.txt" + existing.write_text("keep me\n") + missing = tmp_path / "missing.txt" + for target in (existing, missing): + options = f'--output-file "{target}"' if action == "--output-file" else action + out, err = run_cmd(base_app, f"history {options} -- {selector}") + assert out == [] + message = ( + "No history items in range." + if selector in ("0", "-0") + else ("History indices must be positive or negative integers, and may not be zero.") + ) + assert " ".join(err) == f"{message} The first command in history is command 1." + assert base_app.last_result is False + for effect in effects: + effect.assert_not_called() + assert existing.read_text() == "keep me\n" + assert not missing.exists() + assert list(base_app.history) == original_history + + +@pytest.mark.parametrize("selector", ["0:2", "0:", "1:0", "1:2:3"]) +def test_history_invalid_range_preserves_value_error_api(hist, selector) -> None: + with pytest.raises(HistoryFormatError) as error: + hist.span(selector) + assert isinstance(error.value, ValueError) + + +@pytest.mark.parametrize(("index", "expected"), [(-1, 4), (-2, 3), (-4, 1)]) +@pytest.mark.parametrize("session_start", [0, 2, 4]) +def test_history_negative_index_uses_positive_key(base_app, hist, index, expected, session_start) -> None: + base_app.history = hist + hist.session_start_index = session_start + out, err = run_cmd(base_app, f"history {index}") + assert err == [] + assert out == [f" {expected:>4} {hist[expected - 1].raw}"] + assert base_app.last_result == {expected: hist[expected - 1]} + + +@pytest.mark.parametrize("selector", ["/[/", "no_such_command"]) +def test_history_does_not_hide_search_errors(base_app, monkeypatch, selector) -> None: + if selector == "no_such_command": + monkeypatch.setattr(base_app.history, "str_search", mock.Mock(side_effect=ValueError("unexpected search failure"))) + error = ValueError + else: + error = re.error + with pytest.raises(error): + base_app.onecmd(f"history -r {selector}") + assert base_app.last_result is False def test_history_output_file() -> None: From 28212f412778dd22748a47939703f8e13ee091d2 Mon Sep 17 00:00:00 2001 From: AryanRG Date: Thu, 22 Oct 2026 14:00:35 +0530 Subject: [PATCH 09/11] Fix history selection and require opt-in for persisted replay --- CHANGELOG.md | 21 ++++-- cmd2/cmd2.py | 64 ++++++++-------- cmd2/history.py | 40 +++------- docs/features/history.md | 27 +++++-- tests/test_history.py | 154 +++++++++++++++++++++++++++++++++++---- 5 files changed, 220 insertions(+), 86 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eab98958b..dbd0534a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,24 +3,31 @@ - Breaking Changes - `History.get()` and `History.span()` now raise `HistoryIndexError`, an `IndexError` subclass, for invalid indices and bounded ranges selecting no history items. Unbounded ranges (`:` and - `..`) can still return an empty dictionary, and negative starts still clamp to the beginning - of history. Malformed ranges, including zero endpoints, raise `HistoryFormatError`, a - `ValueError` subclass. The `history` command reports these selection errors without a - traceback hint, and index zero includes the reminder that the first command is command 1. + `..`) can still return an empty dictionary. Starts before the beginning of history are + rejected rather than clamped. Malformed ranges, including zero endpoints, raise + `HistoryFormatError`, a `ValueError` subclass. The `history` command reports these selection + errors without a traceback hint, and index zero includes the reminder that the first command + is command 1. - End-only ranges now start at command 1 in the full history, including persisted commands, just like explicit-start ranges and single indices. Bare `history` and unbounded ranges still select only the current session unless `--all` is supplied. + - Replaying or editing a selection containing persisted commands now requires `--all`. This + includes single indices and bounded ranges. Displaying or saving these selections does not + require `--all`, and command numbers are unchanged. - Running, editing, or saving an empty history selection now fails before replay, opening an editor, or creating or overwriting a file. This includes `history -o FILE` and `history -e` with no selector in an empty session, and actions on searches with no matches. Empty unbounded - selections and search misses report `No matching history items` for these actions; displaying - them still succeeds without output. The no-argument `history --run` safety refusal is - unchanged. + selections report `No history items in range`, while search misses report + `No matching history items`; displaying them still succeeds without output. The no-argument + `history --run` safety refusal is unchanged. - Bug Fixes - Fixed history ranges starting past the end selecting the last command. Invalid indices and bounded ranges selecting no items report `No history items in range` before running, editing, or saving commands. - Negative single-index history selections now display and return the positive command number. + - History regular expressions containing range separators are now handled as regex searches. + Other selectors are treated as numbers or ranges only when they match the supported syntax; + strings such as URLs, `a:b`, `+1`, and `1_0` remain literal searches. ## 4.3.1 (TBD) diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py index d6b8e7adc..de2e140a4 100644 --- a/cmd2/cmd2.py +++ b/cmd2/cmd2.py @@ -153,6 +153,7 @@ SkipPostcommandHooks, ) from .history import ( + NO_HISTORY_ITEMS, NO_MATCHING_HISTORY_ITEMS, History, HistoryFormatError, @@ -5449,13 +5450,14 @@ def _build_history_parser(cls) -> Cmd2ArgumentParser: "-a", "--all", action="store_true", - help="display all commands, including ones persisted from previous sessions", + help="include persisted commands; required to run or edit commands from previous sessions", ) history_arg_help = ( - "empty all history items\n" - "a one history item by number\n" - "a..b, a:b, a:, ..b items by indices (inclusive)\n" + "empty current session (--all includes persisted commands)\n" + "a one item by number in the full history\n" + "a..b, a:b, a:, ..b inclusive ranges in the full history\n" + ":, .. current session (--all includes persisted commands)\n" "string items containing string\n" "/regex/ items matching regular expression" ) @@ -5513,6 +5515,16 @@ def do_history(self, args: argparse.Namespace) -> bool | None: self.perror(str(ex)) return None + # Selection is independent of the requested action. Empty displays are harmless. + if not history and (args.run or args.edit or args.output_file): + is_search = args.arg and not self.history.spanpattern.fullmatch(args.arg) + self.perror(NO_MATCHING_HISTORY_ITEMS if is_search else NO_HISTORY_ITEMS) + return None + + if (args.run or args.edit) and not args.all and any(index <= self.history.session_start_index for index in history): + self.perror("Use --all to run or edit commands from previous sessions.") + return None + if args.run: stop = self.runcmds_plus_hooks(list(history.values())) self.last_result = True @@ -5559,33 +5571,27 @@ def _get_history(self, args: argparse.Namespace) -> dict[int, HistoryItem]: """If an argument was supplied, then retrieve partial contents of the history; otherwise retrieve entire history. This function returns a dictionary with history items keyed by their positive 1-based index in ascending order. - Selection errors, including empty selections for actions, propagate to the caller. + Index and range errors propagate to the caller. Empty selections return an empty dictionary. """ if args.arg: - try: - int_arg = int(args.arg) - except ValueError: - pass - else: - item = self.history.get(int_arg) - 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: - # Get a slice of history - history = self.history.span(args.arg, args.all) - elif args.arg.startswith(r"/") and args.arg.endswith(r"/"): - history = self.history.regex_search(args.arg, args.all) - else: - history = self.history.str_search(args.arg, args.all) - else: - # Get a copy of the history so it doesn't get mutated while we are using it - 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): - raise HistoryIndexError(NO_MATCHING_HISTORY_ITEMS) - return history + if args.arg.startswith(r"/") and args.arg.endswith(r"/"): + return self.history.regex_search(args.arg, args.all) + if re.fullmatch(r"-?[0-9]+", args.arg): + try: + index = int(args.arg) + except ValueError: + # Python may reject integer strings exceeding its conversion limit. + raise HistoryIndexError(NO_HISTORY_ITEMS) from None + if index == 0: + # Preserve the dedicated index-zero error and command-one reminder. + self.history.get(index) + return self.history.span(f"{index}:{index}") + if self.history.spanpattern.fullmatch(args.arg): + return self.history.span(args.arg, args.all) + return self.history.str_search(args.arg, args.all) + + # Get a copy of the history so it doesn't get mutated while we are using it. + return self.history.span(":", args.all) def _initialize_history(self, hist_file: str) -> None: """Initialize history using history related attributes. diff --git a/cmd2/history.py b/cmd2/history.py index 47602c641..abd168028 100644 --- a/cmd2/history.py +++ b/cmd2/history.py @@ -213,31 +213,9 @@ def get(self, index: int) -> HistoryItem: return self[index] return self[index - 1] - # This regular expression parses input for the span() method. There are five parts: - # - # ^\s* matches any whitespace at the beginning of the - # input. This is here so you don't have to trim the input - # - # (?P-?[1-9]{1}\d*)? create a capture group named 'start' which matches an - # optional minus sign, followed by exactly one non-zero - # digit, and as many other digits as you want. This group - # is optional so that we can match an input string like '..2'. - # This regex will match 1, -1, 10, -10, but not 0 or -0. - # - # (?P:|(\.{2,}))? create a capture group named 'separator' which matches either - # a colon or two periods. - # - # (?P-?[1-9]{1}\d*)? create a capture group named 'end' which matches an - # optional minus sign, followed by exactly one non-zero - # digit, and as many other digits as you want. This group is - # optional so that we can match an input string like ':' - # or '5:'. This regex will match 1, -1, 10, -10, but not - # 0 or -0. - # - # \s*$ match any whitespace at the end of the input. This is here so - # you don't have to trim the input - # - spanpattern = re.compile(r"^\s*(?P-?[1-9]\d*)?(?P:|(\.{2,}))(?P-?[1-9]\d*)?\s*$") + # Recognize ranges with optional signed ASCII integer endpoints. Zero is parsed so that + # span() can report its specific error; it is never a valid history index. + spanpattern = re.compile(r"\s*(?P-?[0-9]+)?(?P:|\.{2,})(?P-?[0-9]+)?\s*") def span(self, span: str, include_persisted: bool = False) -> dict[int, "HistoryItem"]: """Return a slice of the History list. @@ -248,7 +226,7 @@ def span(self, span: str, include_persisted: bool = False) -> dict[int, "History :return: a dictionary of history items keyed by their 1-based index in ascending order, or an empty dictionary if an unbounded range (``:`` or ``..``) has no results :raises HistoryFormatError: if the range format is invalid or an index is zero - :raises HistoryIndexError: if a range with an explicit start or end selects no history items + :raises HistoryIndexError: if the start is out of bounds or a bounded range selects no history items This method can accommodate input in any of these forms: @@ -268,17 +246,21 @@ def span(self, span: str, include_persisted: bool = False) -> dict[int, "History - off by one errors """ - results = self.spanpattern.search(span) + results = self.spanpattern.fullmatch(span) if not results: # our regex doesn't match the input, bail out - raise HistoryFormatError(f"{INVALID_HISTORY_RANGE} {FIRST_HISTORY_INDEX}") + raise HistoryFormatError(INVALID_HISTORY_RANGE) start_token = results.group("start") end_token = results.group("end") + if any(token is not None and int(token) == 0 for token in (start_token, end_token)): + raise HistoryFormatError(f"{INVALID_HISTORY_RANGE} {FIRST_HISTORY_INDEX}") if start_token: start = self._zero_based_index(start_token) if start < 0: - start = max(0, len(self) + start) + start += len(self) + if start < 0: + raise HistoryIndexError(NO_HISTORY_ITEMS) else: start = 0 if include_persisted or end_token else self.session_start_index diff --git a/docs/features/history.md b/docs/features/history.md index 6580d4329..59320f948 100644 --- a/docs/features/history.md +++ b/docs/features/history.md @@ -59,7 +59,8 @@ that saved input. The examples to follow all assume that you have entered the fo Alias 'four' created In its simplest form, the `history` command displays previously entered commands. With no additional -arguments, it displays all previously entered commands: +arguments, it displays commands entered in the current session. Use `history --all` to also display +commands loaded from a persistent history file: (Cmd) history 1 alias create one !echo one @@ -123,7 +124,9 @@ an argument: There is no zeroth command, so don't ask for it. If you are a Python programmer, you've probably noticed this looks a lot like the slice syntax for lists and arrays. It is, with the exception that -the first history command is 1, where the first element in a Python array is 0. +the first history command is 1, where the first element in a Python array is 0. A start before the +first command or after the last command is an error. An end after the last command stops at the end +of history. Besides selecting previous commands by number, you can also search for them. You can use a simple string search: @@ -139,6 +142,10 @@ Or a regular expression search by enclosing your regex in slashes: If your regular expression contains any characters that `argparse` finds interesting, like dash or plus, you also need to enclose your regular expression in quotation marks. +Regular expressions are recognized before ranges, so `/a..b/` and `/one:/` are regex searches. +Numbers use ASCII digits with an optional leading minus sign. Other arguments, such as `http://x`, +`a:b`, `+1`, and `1_0`, are literal searches rather than command numbers or ranges. + This all sounds great, but doesn't it seem like a bit of overkill to have all these ways to select commands if all we can do is display them? Turns out, displaying history commands is just the beginning. The history command can perform many other actions: @@ -167,6 +174,12 @@ file are executed. To edit and then re-run commands 2-4 you would: (Cmd) history --edit 2:4 +Running or editing a selection that includes commands from previous sessions requires `--all`, even +when selecting a single command or a bounded range. For example, use `history --all --run 1` to +replay a persisted command 1. Without `--all`, the entire action is refused if any selected command +is persisted; commands are not silently filtered out or renumbered. Current-session-only selections +do not require `--all`. + If you want to save the commands to a text file, but not edit and re-run them, use the `-o` or `--output-file` option. This is a great way to create [Scripts](./scripting.md), which can be executed using the `run_script` command. To save commands 1 through 5 from the full history, @@ -177,10 +190,12 @@ including persisted commands, to a text file: To save only commands entered in the current session, use `history -o history.txt`. Running, editing, or saving an empty selection reports an error before executing commands, opening an editor, or creating or overwriting a file. Empty searches report `No matching history items` for these -actions; displaying a search with no matches succeeds without output. The same display behavior -applies to empty unbounded selections. Invalid indices and bounded ranges selecting no commands -report `No history items in range`. Index zero also includes a reminder that the first command is -command 1. Malformed ranges report a format error. +actions, while empty selections with no selector or an unbounded range report +`No history items in range`. Displaying these empty selections succeeds without output. Invalid +indices and bounded ranges selecting no commands also report `No history items in range`. Index zero +and zero range endpoints include a reminder that the first command is command 1. The +`History.span()` API reports malformed ranges as format errors; the command treats arguments that do +not match range syntax as literal searches. The last action the history command can perform is to clear the command history using `-c` or `--clear`: diff --git a/tests/test_history.py b/tests/test_history.py index e34e397cb..1e1aa2220 100644 --- a/tests/test_history.py +++ b/tests/test_history.py @@ -1,5 +1,6 @@ """Test history functions of cmd2""" +import argparse import contextlib import os import re @@ -238,7 +239,7 @@ def test_history_span_past_end(hist, suffix) -> None: @pytest.mark.parametrize( ("span", "indices"), - [("4:", [4]), ("1:999", [1, 2, 3, 4]), ("-999:-1", [1, 2, 3, 4])], + [("4:", [4]), ("1:999", [1, 2, 3, 4]), ("-4:-1", [1, 2, 3, 4])], ) def test_history_span_boundaries(hist, span, indices) -> None: assert list(hist.span(span)) == indices @@ -782,7 +783,10 @@ def test_history_empty_actions_have_no_side_effects( "If this is what you want to do, specify '1:' as the range of history.", ] else: - assert err == ["No matching history items"] + message = ( + "No history items in range" if selector in ("", '""', ":", "..", '" : "') else "No matching history items" + ) + assert err == [message] assert base_app.last_result is False mkstemp_mock.assert_not_called() @@ -856,7 +860,7 @@ def test_history_unbounded_empty_action_after_reload(base_app, monkeypatch, tmp_ out, err = run_cmd(base_app, f"history {options} :") assert out == [] - assert err == ["No matching history items"] + assert err == ["No history items in range"] assert base_app.last_result is False assert output_file.read_text() == "keep me\n" mkstemp_mock.assert_not_called() @@ -878,7 +882,7 @@ def test_history_end_only_with_all_includes_persisted(base_app, hist, span) -> N @pytest.mark.parametrize( ("span", "session_start"), - [(span, start) for span in ("3:1", "2:-4", "1:-999", ":-999", "..-999") for start in (0, 2, 4)], + [(span, start) for span in ("3:1", "2:-4", "1:-999", ":-999", "..-999", "-5:", "-5..4", "-999:") for start in (0, 2, 4)], ) @pytest.mark.parametrize("action", ["", "--run", "--edit", "--output-file"]) @pytest.mark.parametrize("debug", [False, True]) @@ -902,7 +906,7 @@ def test_history_empty_bounded_span_has_no_side_effects( for output_file in (existing_file, missing_file): options = f'--output-file "{output_file}"' if action == "--output-file" else action - out, err = run_cmd(base_app, f"history {options} {span}") + out, err = run_cmd(base_app, f"history {options} -- {span}") assert out == [] assert err == ["No history items in range"] assert base_app.last_result is False @@ -1006,7 +1010,7 @@ def test_history_index_past_end_with_three_items(base_app, monkeypatch, tmp_path assert output_file.read_text() == "keep me\n" -@pytest.mark.parametrize("span", ["1", "-1", "1:", "-1:", "-999:"]) +@pytest.mark.parametrize("span", ["1", "-1", "1:", "-1:"]) def test_history_run_valid_span(debug_history_app, span) -> None: _out, err = run_cmd(debug_history_app, f"history --run -- {span}") @@ -1015,7 +1019,7 @@ def test_history_run_valid_span(debug_history_app, span) -> None: assert debug_history_app.debug is True -@pytest.mark.parametrize("span", ["1", "-1", "1:", "-1:", "-999:"]) +@pytest.mark.parametrize("span", ["1", "-1", "1:", "-1:"]) def test_history_output_file_valid_span(debug_history_app, tmp_path, span) -> None: output_file = tmp_path / "out.txt" output_file.write_text("old contents\n") @@ -1027,7 +1031,7 @@ def test_history_output_file_valid_span(debug_history_app, tmp_path, span) -> No assert output_file.read_text() == "set debug true\n" -@pytest.mark.parametrize("span", ["1", "-1", "1:", "-999:"]) +@pytest.mark.parametrize("span", ["1", "-1", "1:"]) def test_history_edit_valid_span(debug_history_app, monkeypatch, span) -> None: edited = [] @@ -1081,15 +1085,14 @@ def test_history_does_not_hide_unexpected_index_errors(base_app, hist, monkeypat assert any("unexpected lookup failure" in line for line in err) -def test_history_with_span_index_error(base_app) -> None: +def test_history_literal_search_containing_colon(base_app) -> None: run_cmd(base_app, "help") run_cmd(base_app, "help history") run_cmd(base_app, "!ls -hal :") - expected_err = "History indices must be positive or negative integers, and may not be zero." out, err = run_cmd(base_app, 'history "hal :"') - assert out == [] - assert " ".join(err) == f"{expected_err} The first command in history is command 1." - assert base_app.last_result is False + assert err == [] + assert out == [" 3 !ls -hal :"] + assert list(base_app.last_result) == [3] @pytest.mark.parametrize("separator", [":", ".."]) @@ -1100,7 +1103,7 @@ def test_history_bounded_ranges_use_absolute_indices(hist, separator, session_st hist.session_start_index = session_start assert list(hist.span(f"{separator}{end}", include_persisted)) == expected assert list(hist.span(f"1{separator}{end}", include_persisted)) == expected - assert list(hist.span(f"-999{separator}{end}", include_persisted)) == expected + assert list(hist.span(f"-4{separator}{end}", include_persisted)) == expected assert list(hist.span(separator, include_persisted)) == list(range(1 if include_persisted else session_start + 1, 5)) @@ -1131,6 +1134,15 @@ def fake_editor(fname) -> None: options = f'--output-file "{output_file}"' if action == "--output-file" else action all_option = "--all" if include_persisted else "" out, err = run_cmd(base_app, f"history {all_option} {options} -- {selector}") + if action in ("--run", "--edit") and not include_persisted: + assert out == [] + assert err == ["Use --all to run or edit commands from previous sessions."] + assert base_app.last_result is False + replay.assert_not_called() + run_script.assert_not_called() + assert edited == [] + assert not output_file.exists() + return assert err == [] if action == "--run": replay.assert_called_once_with(expected) @@ -1146,7 +1158,7 @@ def fake_editor(fname) -> None: assert base_app.last_result == {1: expected[0], 2: expected[1]} -@pytest.mark.parametrize("selector", ["0", "-0", "0:2", "0:", "1:0", ":0", "-0..2", "..0", "1:2:3", "1..2:3", "a:b"]) +@pytest.mark.parametrize("selector", ["0", "-0", "0:2", "0:", "1:0", ":0", "-0..2", "..0"]) @pytest.mark.parametrize("action", ["", "--run", "--edit", "--output-file"]) @pytest.mark.parametrize("debug", [False, True]) def test_history_invalid_selection_has_no_side_effects(base_app, monkeypatch, tmp_path, selector, action, debug) -> None: @@ -1209,6 +1221,118 @@ def test_history_does_not_hide_search_errors(base_app, monkeypatch, selector) -> assert base_app.last_result is False +@pytest.mark.parametrize("selector", ["-5:", "-5:-1", "-5:3", "-5..", "-5..-1", "-999:"]) +def test_history_span_rejects_overly_negative_starts(hist, selector) -> None: + with pytest.raises(HistoryIndexError, match="No history items in range"): + hist.span(selector) + + +@pytest.mark.parametrize( + ("selector", "needle"), + [ + ("/a..b/", "axxb"), + ("/one:/", "one:"), + ("http://x", "http://x"), + ("a:b", "a:b"), + ("1:2:3", "1:2:3"), + ("abc:", "abc:"), + ("1_0", "1_0"), + ("+1", "+1"), + ('" 1 "', " 1 "), + ("\u0661", "\u0661"), + ("1\u0661", "1\u0661"), + ("-\u0661", "-\u0661"), + ], +) +def test_history_search_dispatch_is_not_numeric_or_range(base_app, hist, selector, needle) -> None: + hist.clear() + for index in range(10): + text = f"help before{needle}after" if index == 2 else "help control" + hist.append(base_app.statement_parser.parse(text)) + base_app.history = hist + + out, err = run_cmd(base_app, f"history -- {selector}") + + assert err == [] + assert out == [f" 3 help before{needle}after"] + assert base_app.last_result == {3: hist[2]} + + +@pytest.mark.parametrize("selector", ["", ":", "no_such_command", "/no_such_regex/"]) +def test_get_history_returns_empty_dictionary_for_actions(base_app, selector) -> None: + args = argparse.Namespace(arg=selector, all=False, run=False, edit=True, output_file=None) + assert base_app._get_history(args) == {} + + +@pytest.mark.parametrize("selector", ["1:2:3", "abc:", "a:b", "\u0661:2", "1\u0661:2", "+1:2", "1_0:2"]) +def test_history_format_error_does_not_suggest_command_one(hist, selector) -> None: + with pytest.raises(HistoryFormatError) as error: + hist.span(selector) + assert "The first command" not in str(error.value) + + +@pytest.mark.parametrize("selector", ["0:2", "1:0", "-0..2", "..-0"]) +def test_history_zero_endpoint_has_command_one_hint(hist, selector) -> None: + with pytest.raises(HistoryFormatError) as error: + hist.span(selector) + assert "The first command in history is command 1." in str(error.value) + + +@pytest.mark.parametrize("selector", ["1", "-6", ":2", "1:5", "2:", "-6:-1"]) +@pytest.mark.parametrize("action", ["--run", "--edit"]) +def test_history_persisted_actions_require_all(base_app, persisted_hist, monkeypatch, selector, action) -> None: + base_app.history = persisted_hist + effects = [mock.Mock() for _ in range(4)] + monkeypatch.setattr("cmd2.cmd2.tempfile.mkstemp", effects[0]) + monkeypatch.setattr(base_app, "run_editor", effects[1]) + monkeypatch.setattr(base_app, "do_run_script", effects[2]) + monkeypatch.setattr(base_app, "runcmds_plus_hooks", effects[3]) + + out, err = run_cmd(base_app, f"history {action} -- {selector}") + + assert out == [] + assert err == ["Use --all to run or edit commands from previous sessions."] + assert base_app.last_result is False + for effect in effects: + effect.assert_not_called() + + +@pytest.mark.parametrize("selector", ["5", "-2", "5:", "-2:"]) +@pytest.mark.parametrize("action", ["--run", "--edit"]) +def test_history_current_session_actions_do_not_require_all(base_app, persisted_hist, monkeypatch, selector, action) -> None: + base_app.history = persisted_hist + expected = list(persisted_hist[4:5] if selector in ("5", "-2") else persisted_hist[4:]) + replay = mock.Mock(return_value=False) + monkeypatch.setattr(base_app, "runcmds_plus_hooks", replay) + edited = [] + + def fake_editor(fname) -> None: + with open(fname) as script: + edited.append(script.read()) + + monkeypatch.setattr(base_app, "run_editor", fake_editor) + run_script = mock.Mock() + monkeypatch.setattr(base_app, "do_run_script", run_script) + + _out, err = run_cmd(base_app, f"history {action} -- {selector}") + + assert err == [] + if action == "--run": + replay.assert_called_once_with(expected) + else: + assert edited == ["".join(f"{item.raw}\n" for item in expected)] + run_script.assert_called_once() + + +def test_history_help_explains_selection_and_persisted_actions(base_app) -> None: + out, err = run_cmd(base_app, "help history") + text = " ".join(out) + assert err == [] + assert "current session" in text + assert "full history" in text + assert "required to run or edit" in text + + def test_history_output_file() -> None: app = cmd2.Cmd(multiline_commands=["alias"]) run_cmd(app, "help") From 4cdc756efce863b30274db873d27722ad0f94f1c Mon Sep 17 00:00:00 2001 From: AryanRG Date: Thu, 22 Oct 2026 14:23:40 +0530 Subject: [PATCH 10/11] Cover the last persisted history command boundary --- tests/test_history.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_history.py b/tests/test_history.py index 1e1aa2220..763254c69 100644 --- a/tests/test_history.py +++ b/tests/test_history.py @@ -1278,7 +1278,7 @@ def test_history_zero_endpoint_has_command_one_hint(hist, selector) -> None: assert "The first command in history is command 1." in str(error.value) -@pytest.mark.parametrize("selector", ["1", "-6", ":2", "1:5", "2:", "-6:-1"]) +@pytest.mark.parametrize("selector", ["1", "-6", ":2", "1:5", "2:", "-6:-1", "4", "-3", "4:5"]) @pytest.mark.parametrize("action", ["--run", "--edit"]) def test_history_persisted_actions_require_all(base_app, persisted_hist, monkeypatch, selector, action) -> None: base_app.history = persisted_hist From 18506b6ffe8a99abb9254fb095a6d810d9c511cf Mon Sep 17 00:00:00 2001 From: AryanRG Date: Thu, 22 Oct 2026 14:52:46 +0530 Subject: [PATCH 11/11] Test clean handling of oversized history indices --- tests/test_history.py | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/test_history.py b/tests/test_history.py index 763254c69..7ff3fca27 100644 --- a/tests/test_history.py +++ b/tests/test_history.py @@ -4,6 +4,7 @@ import contextlib import os import re +import sys import tempfile from unittest import ( mock, @@ -1324,6 +1325,40 @@ def fake_editor(fname) -> None: run_script.assert_called_once() +@pytest.mark.parametrize("sign", ["", "-"]) +@pytest.mark.parametrize("action", ["", "--run", "--edit", "--output-file"]) +@pytest.mark.parametrize("debug", [False, True]) +def test_history_integer_conversion_limit_has_no_side_effects( + base_app, hist, monkeypatch, tmp_path, sign, action, debug +) -> None: + base_app.history = hist + base_app.debug = debug + effects = [mock.Mock() for _ in range(4)] + monkeypatch.setattr("cmd2.cmd2.tempfile.mkstemp", effects[0]) + monkeypatch.setattr(base_app, "run_editor", effects[1]) + monkeypatch.setattr(base_app, "do_run_script", effects[2]) + monkeypatch.setattr(base_app, "runcmds_plus_hooks", effects[3]) + output_file = tmp_path / "existing.txt" + output_file.write_text("keep me\n") + options = f'--output-file "{output_file}"' if action == "--output-file" else action + limit = sys.int_info.str_digits_check_threshold + selector = sign + "9" * (limit + 1) + original_limit = sys.get_int_max_str_digits() + try: + sys.set_int_max_str_digits(limit) + out, err = run_cmd(base_app, f"history {options} -- {selector}") + finally: + sys.set_int_max_str_digits(original_limit) + + assert out == [] + assert err == ["No history items in range"] + assert base_app.last_result is False + for effect in effects: + effect.assert_not_called() + assert output_file.read_text() == "keep me\n" + assert len(base_app.history) == 4 + + def test_history_help_explains_selection_and_persisted_actions(base_app) -> None: out, err = run_cmd(base_app, "help history") text = " ".join(out)