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 <noreply@anthropic.com>
This commit is contained in:
parent
759298abcb
commit
aaef4f06bd
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
||||
|
|
|
|||
|
|
@ -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"}))
|
||||
|
|
|
|||
Loading…
Reference in New Issue