From aaef4f06bdaab2e4795758fccc295e60e12ae384 Mon Sep 17 00:00:00 2001 From: Aakash Kattelu Date: Mon, 10 Aug 2026 17:47:41 -0400 Subject: [PATCH] fix(cli): read next-page hint scope from effective flag overrides Addresses the second CodeRabbit pass on #1006. `-w`/`-p` parse at group and top level as well as command level, all landing in `_global_overrides`, so reading the command-level params dropped the scope from `honcho session -w ws2 view ...`. The hint now reads the effective overrides via a new `get_flag_overrides()`, which deliberately excludes environment and config values since those resolve the same way on the next run. Also shell-quotes the hint's identifiers with `shlex.join`. Note this is hardening rather than a live injection fix: the API constrains IDs to `^[a-zA-Z0-9_-]+$`, so an ID carrying a space or metacharacter fails the fetch before any hint is printed. `validate_resource_id` is looser than the server though, so quoting is the cheaper invariant to hold locally. Co-Authored-By: Claude Opus 5 --- honcho-cli/src/honcho_cli/commands/session.py | 39 ++++++++++++++----- honcho-cli/src/honcho_cli/common.py | 9 +++++ honcho-cli/tests/test_commands.py | 39 +++++++++++++++++++ 3 files changed, 78 insertions(+), 9 deletions(-) diff --git a/honcho-cli/src/honcho_cli/commands/session.py b/honcho-cli/src/honcho_cli/commands/session.py index 22f4a371..08dd796c 100644 --- a/honcho-cli/src/honcho_cli/commands/session.py +++ b/honcho-cli/src/honcho_cli/commands/session.py @@ -3,6 +3,7 @@ from __future__ import annotations import json +import shlex from typing import List, Optional import typer @@ -14,7 +15,13 @@ from honcho_cli.output import print_error, print_result, print_transcript, statu from honcho_cli.validation import validate_resource_id from honcho_cli._help import HonchoTyperGroup -from honcho_cli.common import add_common_options, get_client, get_resolved_config, handle_cmd_flags +from honcho_cli.common import ( + add_common_options, + get_client, + get_flag_overrides, + get_resolved_config, + handle_cmd_flags, +) app = typer.Typer(cls=HonchoTyperGroup, help="List, inspect, view, create, delete, and manage conversation sessions and their peers.") add_common_options(app) @@ -174,19 +181,30 @@ def _next_page_command( ) -> str: """Continuation command for the next page, carrying this invocation's scope. - Scoping flags are echoed only when passed explicitly; anything resolved from - the environment or config file resolves the same way on the next run. + Scoping flags are echoed only when passed as flags; anything resolved from + the environment or config file resolves the same way on the next run. IDs + are shell-quoted — they may contain spaces and metacharacters, and this + string is meant to be pasted into a shell. """ - parts = [f"honcho session view {session_id}", f"--page {next_page}", f"--size {size}"] + parts = [ + "honcho", + "session", + "view", + session_id, + "--page", + str(next_page), + "--size", + str(size), + ] if reverse: parts.append("--reverse") if show_ids: parts.append("--ids") if workspace: - parts.append(f"-w {workspace}") + parts += ["-w", workspace] if peer: - parts.append(f"-p {peer}") - return " ".join(parts) + parts += ["-p", peer] + return shlex.join(parts) def _fetch_all_messages(sess, filters: dict | None) -> tuple[list, int | None]: @@ -333,14 +351,17 @@ def view( next_page_hint = None if page_meta is not None and pages_meta is not None and page_meta < pages_meta: + # Effective overrides, not the command-level params: -w/-p also parse at + # group and top level. + overrides = get_flag_overrides() next_page_hint = _next_page_command( sid, page_meta + 1, page_size, reverse=reverse, show_ids=show_ids, - workspace=workspace, - peer=peer, + workspace=overrides["workspace"], + peer=overrides["peer"], ) # Rendered outside the try: output failures aren't session API errors. diff --git a/honcho-cli/src/honcho_cli/common.py b/honcho-cli/src/honcho_cli/common.py index d87a4be7..ed1f54ae 100644 --- a/honcho-cli/src/honcho_cli/common.py +++ b/honcho-cli/src/honcho_cli/common.py @@ -52,6 +52,15 @@ def get_resolved_config(): return config +def get_flag_overrides() -> dict[str, str | None]: + """Workspace/peer/session as supplied by ``-w``/``-p``/``-s`` at any level. + + Unlike :func:`get_resolved_config`, this excludes values coming from the + environment or config file. + """ + return dict(_global_overrides) + + def maybe_refresh_token(config: CLIConfig) -> None: """Refresh an expired OAuth access token in place and persist it. diff --git a/honcho-cli/tests/test_commands.py b/honcho-cli/tests/test_commands.py index a24d6c77..2fb29256 100644 --- a/honcho-cli/tests/test_commands.py +++ b/honcho-cli/tests/test_commands.py @@ -387,6 +387,45 @@ class TestJsonContract: opts = {"reverse": False, "show_ids": False, "workspace": None, "peer": None, **kwargs} assert _next_page_command("s1", 2, 50, **opts) == expected + @pytest.mark.parametrize( + ("session_id", "workspace", "expected_fragment"), + [ + ("has space", None, "'has space'"), + ("a;rm -rf x", None, "'a;rm -rf x'"), + ("s1", "ws$(id)", "'ws$(id)'"), + ("s1", "ws|tee", "'ws|tee'"), + ], + ) + def test_next_page_command_shell_quotes_identifiers( + self, session_id, workspace, expected_fragment + ): + """IDs only reject ?#%/\\ and control chars, so spaces and metacharacters reach here.""" + hint = _next_page_command( + session_id, + 2, + 50, + reverse=False, + show_ids=False, + workspace=workspace, + peer=None, + ) + assert expected_fragment in hint + + def test_session_view_hint_carries_group_level_scope(self, cfg, runner): + """-w/-p also parse at group level, where the command-level params are None.""" + cfg.write_text(json.dumps({"apiKey": "k", "environmentUrl": "http://localhost:8000"})) + session = _fake_session(_fake_page([_view_msg(1)], total=10, page=1, pages=5)) + + with _patch_view(session), patch("honcho_cli.output.use_json", return_value=False): + result = runner.invoke( + app, + ["session", "-w", "ws2", "-p", "alice", "view", "sess1", "--page", "1"], + ) + + assert result.exit_code == 0, result.stderr + assert "-w ws2" in result.stderr + assert "-p alice" in result.stderr + def test_session_view_last_walks_pages_past_the_page_cap(self, cfg, runner): """`--last N` above the 100-item server cap keeps walking instead of truncating.""" cfg.write_text(json.dumps({"apiKey": "k", "environmentUrl": "http://localhost:8000"}))