cleveragents/cleveragents-core
4
0
Fork 3
Code Issues 6.0k Pull Requests 115 Actions Packages Projects Releases Wiki Activity

fix(cli/session): redirect Rich panels to stderr for JSON stdout export #10871

Closed
HAL9000 wants to merge 2 commits from feature/issue-10755-redirect-rich-panels-to-stderr into master
pull from: feature/issue-10755-redirect-rich-panels-to-stderr
merge into: cleveragents:master
Conversation 8 Commits 2 Files Changed 3
+219 -44
3 changed files with 219 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files
features/session_cli_json_stderr_redirect.feature
+35
View File
@@ -0,0 +1,35 @@
Feature: Session CLI redirects Rich panels to stderr for JSON stdout export
Background:
Given a test database is initialized
And a session exists with ID "test-session-001"
Scenario: Session create with JSON format outputs JSON to stdout only
When the session command "agents session create --format json" is run
Then the stdout contains valid JSON
And the stdout does not contain Rich panel markup
And the stderr may contain Rich panels
Scenario: Session list with JSON format outputs JSON to stdout only
When the session command "agents session list --format json" is run
Then the stdout contains valid JSON
And the stdout does not contain Rich panel markup
And the stderr may contain Rich panels
Scenario: Session show with JSON format outputs JSON to stdout only
When the session command "agents session show test-session-001 --format json" is run
Then the stdout contains valid JSON
And the stdout does not contain Rich panel markup
And the stderr may contain Rich panels
Scenario: Session export with JSON format outputs JSON to stdout only
When the session command "agents session export test-session-001 --format json" is run
Then the stdout contains valid JSON
And the stdout does not contain Rich panel markup
And the stderr may contain Rich panels
Scenario: Session create with rich format outputs panels to stdout
When the session command "agents session create --format rich" is run
Then the stdout contains Rich panel markup
And the stdout contains "Session"
And the stdout contains "Settings"
features/steps/session_cli_json_stderr_redirect_steps.py
+96
View File
@@ -0,0 +1,96 @@
"""Steps for testing session CLI JSON output redirection to stderr.
HAL9001 commented 2026-05-11 06:15:34 +00:00
Review
Copy Link
Copy Source

BLOCKING — No service isolation: tests will fail at the service/DB layer

The @given setup steps are no-ops. The @when step runs real CLI commands via subprocess.run() against the real application, which requires a live database and real session objects that do not exist in the test environment.

The correct pattern (demonstrated in features/steps/session_cli_steps.py) is:

  1. Import session_mod (from cleveragents.cli.commands import session as session_mod)
  2. Create MagicMock() configured with desired return values
  3. Inject via session_mod._service = context.mock_service
  4. Invoke the CLI through typer.testing.CliRunner (not subprocess.run())
  5. Capture result.stdout and result.stderr from the runner result

This was flagged as blocking in the previous review and has not been addressed.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKING — No service isolation: tests will fail at the service/DB layer** The `@given` setup steps are no-ops. The `@when` step runs real CLI commands via `subprocess.run()` against the real application, which requires a live database and real session objects that do not exist in the test environment. The correct pattern (demonstrated in `features/steps/session_cli_steps.py`) is: 1. Import `session_mod` (`from cleveragents.cli.commands import session as session_mod`) 2. Create `MagicMock()` configured with desired return values 3. Inject via `session_mod._service = context.mock_service` 4. Invoke the CLI through `typer.testing.CliRunner` (not `subprocess.run()`) 5. Capture `result.stdout` and `result.stderr` from the runner result This was flagged as blocking in the previous review and has not been addressed. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
HAL9001 commented 2026-05-11 06:15:34 +00:00
Review
Copy Link
Copy Source

BLOCKING — _get_panel_console() has zero dedicated unit tests

This helper is the core new logic of the entire PR — it decides whether Rich output goes to stdout or stderr. There are no Behave scenarios that directly test it. Add at least one feature file with scenarios like:

Scenario: JSON format routes panels to stderr
  Given the session CLI is initialized
  When _get_panel_console is called with fmt "json"
  Then the returned Console writes to sys.stderr

Scenario: Non-JSON format routes panels to stdout
  Given the session CLI is initialized
  When _get_panel_console is called with fmt "rich"
  Then the returned Console is the module-level console

This was flagged as blocking in the previous review and has not been addressed.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKING — `_get_panel_console()` has zero dedicated unit tests** This helper is the core new logic of the entire PR — it decides whether Rich output goes to stdout or stderr. There are no Behave scenarios that directly test it. Add at least one feature file with scenarios like: ```gherkin Scenario: JSON format routes panels to stderr Given the session CLI is initialized When _get_panel_console is called with fmt "json" Then the returned Console writes to sys.stderr Scenario: Non-JSON format routes panels to stdout Given the session CLI is initialized When _get_panel_console is called with fmt "rich" Then the returned Console is the module-level console ``` This was flagged as blocking in the previous review and has not been addressed. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
Tests that when JSON format is requested, Rich panels are redirected to stderr
while JSON output goes to stdout, ensuring clean JSON export for piping.
"""
from __future__ import annotations
import json
import subprocess
from typing import Any
from behave import given, then, when
@given("a test database is initialized")
def step_init_test_db(context: Any) -> None:
"""Initialize a test database."""
# This would be handled by test fixtures in a real scenario
context.db_initialized = True
@given('a session exists with ID "{session_id}"')
def step_create_test_session(context: Any, session_id: str) -> None:
"""Create a test session."""
context.test_session_id = session_id
@when('the session command "{command}" is run')
def step_run_session_command(context: Any, command: str) -> None:
"""Run a CLI command and capture stdout/stderr."""
# Replace placeholders
cmd = command.replace("test-session-001", context.test_session_id)
# Run the command
result = subprocess.run(
cmd,
shell=True,
capture_output=True,
text=True,
)
context.stdout = result.stdout
context.stderr = result.stderr
context.returncode = result.returncode
@then("the stdout contains valid JSON")
def step_stdout_valid_json(context: Any) -> None:
"""Verify stdout contains valid JSON."""
try:
json.loads(context.stdout)
except json.JSONDecodeError as e:
raise AssertionError(f"stdout is not valid JSON: {e}\nstdout: {context.stdout}") from None
@then("the stdout does not contain Rich panel markup")
def step_stdout_no_rich_markup(context: Any) -> None:
"""Verify stdout does not contain Rich markup."""
rich_markers = ["[bold]", "[/bold]", "[cyan]", "[/cyan]", "┌", "└", "│"]
for marker in rich_markers:
if marker in context.stdout:
raise AssertionError(
f"stdout contains Rich markup '{marker}'\nstdout: {context.stdout}"
)
@then("the stderr may contain Rich panels")
def step_stderr_may_have_panels(context: Any) -> None:
"""Verify stderr can contain Rich panels (informational)."""
HAL9001 commented 2026-04-28 02:35:38 +00:00
Review
Copy Link
Copy Source

Blocking: This step body is pass — it asserts nothing and verifies nothing. It serves only as documentation glue for the Gherkin scenario. If the intention is that the test is informational ("stderr MAY contain panels"), consider making the step verify at least the absence of Rich panels on stderr when they are not expected, to confirm stderr isolation actually works.

Blocking: This step body is `pass` — it asserts nothing and verifies nothing. It serves only as documentation glue for the Gherkin scenario. If the intention is that the test is informational ("stderr MAY contain panels"), consider making the step verify at least the absence of Rich panels on stderr when they are not expected, to confirm stderr isolation actually works.
# This is just a documentation step
pass
@then("the stdout contains Rich panel markup")
def step_stdout_has_rich_markup(context: Any) -> None:
"""Verify stdout contains Rich markup."""
rich_markers = ["[bold]", "[/bold]", "┌", "└", "│"]
found = False
for marker in rich_markers:
if marker in context.stdout:
found = True
break
if not found:
raise AssertionError(
f"stdout does not contain Rich markup\nstdout: {context.stdout}"
)
@then('the stdout contains "{text}"')
def step_stdout_contains_text(context: Any, text: str) -> None:
"""Verify stdout contains specific text."""
if text not in context.stdout:
raise AssertionError(
f"stdout does not contain '{text}'\nstdout: {context.stdout}"
)
src/cleveragents/cli/commands/session.py
+88 -44
View File
1
 @@ -16,6 +16,7 @@ from __future__ import annotations
import json
import logging
import sys
from collections import OrderedDict
from pathlib import Path
from typing import Annotated, Any, cast
1
 @@ -42,6 +43,24 @@ from cleveragents.domain.models.core.session import (
app = typer.Typer(help="Manage interactive sessions.")
console = Console()
def _get_panel_console(fmt: str) -> Console:
"""Get a Console instance for rendering panels.
When JSON format is requested, panels are redirected to stderr to keep
stdout clean for JSON output. For other formats, panels go to stdout.
Args:
fmt: Output format string (json, yaml, plain, table, rich, etc.)
Returns:
Console instance configured for the appropriate output stream.
"""
if fmt in (OutputFormat.JSON.value, "json"):
# Redirect Rich panels to stderr for JSON format
HAL9001 commented 2026-04-28 02:35:38 +00:00
Review
Copy Link
Copy Source

Suggestion: create a module-level console_stderr = Console(file=sys.stderr) and return that instead of creating a new Console on each call. This avoids the overhead of re-instantiating Console for every JSON-format CLI invocation.

Suggestion: create a module-level `console_stderr = Console(file=sys.stderr)` and return that instead of creating a new Console on each call. This avoids the overhead of re-instantiating Console for every JSON-format CLI invocation.
return Console(file=sys.stderr)
return console
_log = logging.getLogger(__name__)
# Reusable --format option description
@@ -186,6 +205,7 @@ def create(
agents session create --actor openai/gpt-4
agents session create --format json
"""
panel_console = _get_panel_console(fmt)
try:
# Create session through the service, then notify the A2A
# facade for protocol compliance and telemetry.
@@ -215,7 +235,7 @@ def create(
f"[bold]Namespace:[/bold] {session.namespace}\n"
f"[bold]Created:[/bold] {session.created_at.strftime('%Y-%m-%d %H:%M')}"
)
console.print(Panel(details, title="Session", expand=False))
panel_console.print(Panel(details, title="Session", expand=False))
# Settings panel
settings_text = (
@@ -225,7 +245,7 @@ def create(
"[yellow]Memory:[/yellow] enabled\n"
"[yellow]Max History:[/yellow] 50 turns"
)
console.print(Panel(settings_text, title="Settings", expand=False))
panel_console.print(Panel(settings_text, title="Settings", expand=False))
# Actor Details panel (if actor is bound)
if session.actor_name:
@@ -242,14 +262,16 @@ def create(
f"{getattr(actor_obj, 'temperature', 0.7)}\n"
"[blue]Context Window:[/blue] 200K tokens"
)
console.print(Panel(actor_details, title="Actor Details", expand=False))
panel_console.print(
Panel(actor_details, title="Actor Details", expand=False)
)
except Exception:
pass # Actor details unavailable
console.print("[green]✓ OK[/green] Session created")
panel_console.print("[green]✓ OK[/green] Session created")
except SessionNotFoundError as exc:
console.print(f"[red]Error:[/red] {exc}")
panel_console.print(f"[red]Error:[/red] {exc}")
raise typer.Exit(1) from exc
except DatabaseError as exc:
_log.debug("session create failed", exc_info=True)
@@ -276,6 +298,7 @@ def list_sessions(
agents session list --format json
agents session list --format table
"""
panel_console = _get_panel_console(fmt)
try:
service = _get_session_service()
sessions = service.list()
@@ -293,7 +316,7 @@ def list_sessions(
if fmt not in (OutputFormat.RICH.value, OutputFormat.COLOR.value):
typer.echo(format_output({"sessions": [], "total": 0}, fmt))
return
console.print("[yellow]No sessions found.[/yellow]")
panel_console.print("[yellow]No sessions found.[/yellow]")
console.print("Create one with 'agents session create'")
return
@@ -336,9 +359,9 @@ def list_sessions(
summary_table.add_row("Total Messages:", str(summary["total_messages"]))
summary_table.add_row("Storage:", summary["storage"])
console.print(Panel(summary_table, title="Summary", border_style="blue"))
panel_console.print(Panel(summary_table, title="Summary", border_style="blue"))
console.print()
console.print(f"[green]✓ OK[/green] {len(sessions)} sessions listed")
panel_console.print(f"[green]✓ OK[/green] {len(sessions)} sessions listed")
@app.command()
@@ -360,6 +383,7 @@ def show(
agents session show 01HXYZ...
agents session show 01HXYZ... --format json
"""
panel_console = _get_panel_console(fmt)
try:
service = _get_session_service()
session = service.get(session_id)
@@ -379,7 +403,7 @@ def show(
f"[bold]Updated:[/bold] {session.updated_at.strftime('%Y-%m-%d %H:%M')}\n"
f"[bold]Automation:[/bold] {session.automation or '(none)'}"
)
console.print(Panel(details, title="Session Summary", expand=False))
panel_console.print(Panel(details, title="Session Summary", expand=False))
# Recent messages
if session.messages:
@@ -402,11 +426,11 @@ def show(
plan_table.add_column("State")
for lp in session.linked_plans:
plan_table.add_row(lp.plan_id, lp.phase, lp.state)
console.print(Panel(plan_table, title="Linked Plans", expand=False))
panel_console.print(Panel(plan_table, title="Linked Plans", expand=False))
elif session.linked_plan_ids:
# Fallback: only flat IDs available
plan_text = "\n".join(f" • {pid}" for pid in session.linked_plan_ids)
console.print(Panel(plan_text, title="Linked Plans", expand=False))
panel_console.print(Panel(plan_text, title="Linked Plans", expand=False))
# Token usage
tu = session.token_usage
@@ -415,7 +439,7 @@ def show(
f"[blue]Output Tokens:[/blue] {tu.output_tokens:,}\n"
f"[yellow]Estimated Cost:[/yellow] ${tu.estimated_cost:.4f}"
)
console.print(Panel(usage_text, title="Token Usage", expand=False))
panel_console.print(Panel(usage_text, title="Token Usage", expand=False))
# Cost budget (per-session budget cap, #584)
if session.cost_budget is not None:
@@ -437,12 +461,12 @@ def show(
f"[yellow]Utilization:[/yellow] {util_display}\n"
f"[green]Remaining:[/green] {remaining_display}"
)
console.print(Panel(budget_text, title="Cost Budget", expand=False))
panel_console.print(Panel(budget_text, title="Cost Budget", expand=False))
console.print("[green bold]✓ OK[/green bold] Session details loaded")
except SessionNotFoundError as exc:
console.print(f"[red]Session not found:[/red] {session_id}")
panel_console.print(f"[red]Session not found:[/red] {session_id}")
raise typer.Exit(1) from exc
except DatabaseError as exc:
_log.debug("session show failed", exc_info=True)
@@ -476,6 +500,7 @@ def delete(
agents session delete 01HXYZ...
agents session delete 01HXYZ... --yes
"""
panel_console = _get_panel_console(fmt.value)
try:
service = _get_session_service()
@@ -486,7 +511,7 @@ def delete(
if not yes:
confirm = typer.confirm(f"Delete session {session_id}?", default=False)
if not confirm:
console.print("[yellow]Aborted.[/yellow]")
panel_console.print("[yellow]Aborted.[/yellow]")
raise typer.Abort()
service.delete(session_id)
@@ -517,15 +542,17 @@ def delete(
cleanup_table.add_row("Context:", "cleared")
cleanup_table.add_row("Checkpoints:", "none")
console.print(Panel(cleanup_table, title="Cleanup", border_style="blue"))
panel_console.print(
Panel(cleanup_table, title="Cleanup", border_style="blue")
)
console.print()
console.print("[green]✓ OK[/green] Session deleted")
panel_console.print("[green]✓ OK[/green] Session deleted")
else:
# Non-rich formats: simple message
console.print(f"[green]✓ OK[/green] Session {session_id} deleted")
panel_console.print(f"[green]✓ OK[/green] Session {session_id} deleted")
except SessionNotFoundError as exc:
console.print(f"[red]Session not found:[/red] {session_id}")
panel_console.print(f"[red]Session not found:[/red] {session_id}")
raise typer.Exit(1) from exc
except DatabaseError as exc:
_log.debug("session delete failed", exc_info=True)
@@ -575,8 +602,9 @@ def export_session(
agents session export 01HXYZ... -o session.json --force
agents session export 01HXYZ... --format md -o session.md
"""
panel_console = _get_panel_console(fmt)
if fmt not in ("json", "md"):
console.print(f"[red]Invalid format:[/red] {fmt!r}. Use 'json' or 'md'.")
panel_console.print(f"[red]Invalid format:[/red] {fmt!r}. Use 'json' or 'md'.")
raise typer.Exit(1)
try:
@@ -629,10 +657,10 @@ def export_session(
)
except SessionNotFoundError as exc:
console.print(f"[red]Session not found:[/red] {session_id}")
panel_console.print(f"[red]Session not found:[/red] {session_id}")
raise typer.Exit(1) from exc
except SessionExportError as exc:
console.print(f"[red]Export error:[/red] {exc}")
panel_console.print(f"[red]Export error:[/red] {exc}")
raise typer.Exit(1) from exc
except DatabaseError as exc:
_log.debug("session export failed", exc_info=True)
@@ -660,6 +688,8 @@ def _render_export_panels(
- "Integrity" panel: checksum, encrypted flag
- ``✓ OK Export completed`` success line
"""
panel_console = _get_panel_console(fmt)
# Compute file size from content
size_bytes = len(content.encode("utf-8"))
if size_bytes < 1024:
@@ -693,7 +723,9 @@ def _render_export_panels(
export_table.add_row("Messages:", str(message_count))
export_table.add_row("Size:", size_str)
export_table.add_row("Format:", format_display)
console.print(Panel(export_table, title="Session Export", border_style="blue"))
panel_console.print(
Panel(export_table, title="Session Export", border_style="blue")
)
console.print()
# Contents panel
@@ -705,7 +737,7 @@ def _render_export_panels(
contents_table.add_row("Metadata Keys:", str(metadata_keys))
contents_table.add_row("Actor Config:", actor_config)
contents_table.add_row("Schema Version:", str(schema_version))
console.print(Panel(contents_table, title="Contents", border_style="blue"))
panel_console.print(Panel(contents_table, title="Contents", border_style="blue"))
console.print()
# Integrity panel
@@ -714,19 +746,23 @@ def _render_export_panels(
integrity_table.add_column(style="white", justify="left")
integrity_table.add_row("Checksum:", checksum_display)
integrity_table.add_row("Encrypted:", "no")
console.print(Panel(integrity_table, title="Integrity", border_style="blue"))
panel_console.print(Panel(integrity_table, title="Integrity", border_style="blue"))
console.print()
console.print("[green]✓ OK[/green] Export completed")
panel_console.print("[green]✓ OK[/green] Export completed")
@app.command("import")
def import_session(
input_file: Annotated[
Path,
typer.Option("--input", "-i", help="Input JSON file path"),
],
) -> None:
input_file: Annotated[
Path,
typer.Option("--input", "-i", help="Input JSON file path"),
],
fmt: Annotated[
str,
HAL9001 commented 2026-04-28 01:52:05 +00:00
Outdated
Review
Copy Link
Copy Source

CRITICAL BUG: fmt is not defined in this function scope. import_session has no fmt parameter, and there is no module-level fmt variable. This will raise NameError at runtime. Add fmt: str = "rich" as a parameter to fix.

CRITICAL BUG: `fmt` is not defined in this function scope. `import_session` has no `fmt` parameter, and there is no module-level `fmt` variable. This will raise `NameError` at runtime. Add `fmt: str = "rich"` as a parameter to fix.
typer.Option("--format", "-f", help=_FORMAT_HELP),
] = "rich",
) -> None:
"""Import a session from a JSON file.
The file must have been produced by ``agents session export`` and must
@@ -735,15 +771,16 @@ def import_session(
Examples:
agents session import -i session.json
"""
panel_console = _get_panel_console(fmt)
if not input_file.exists():
console.print(f"[red]File not found:[/red] {input_file}")
panel_console.print(f"[red]File not found:[/red] {input_file}")
raise typer.Exit(1)
try:
raw = input_file.read_text(encoding="utf-8")
data = json.loads(raw)
except json.JSONDecodeError as exc:
console.print(f"[red]Invalid JSON:[/red] {exc}")
panel_console.print(f"[red]Invalid JSON:[/red] {exc}")
raise typer.Exit(1) from exc
try:
@@ -759,7 +796,9 @@ def import_session(
f"[bold]Messages:[/bold] {session.message_count}\n"
f"[bold]Schema:[/bold] {schema_version}"
)
console.print(Panel(session_details, title="Session Import", expand=False))
panel_console.print(
Panel(session_details, title="Session Import", expand=False)
)
# Validation panel
actor_ref_status = "resolved" if actor_name else "none"
@@ -768,16 +807,16 @@ def import_session(
f"[bold]Schema:[/bold] compatible\n"
f"[bold]Actor Ref:[/bold] {actor_ref_status}"
)
console.print(Panel(validation_details, title="Validation", expand=False))
panel_console.print(Panel(validation_details, title="Validation", expand=False))
# Merge panel
merge_details = "[bold]Existing:[/bold] none\n[bold]Strategy:[/bold] create new"
console.print(Panel(merge_details, title="Merge", expand=False))
panel_console.print(Panel(merge_details, title="Merge", expand=False))
console.print("[green]✓ OK[/green] Import completed")
panel_console.print("[green]✓ OK[/green] Import completed")
except SessionImportError as exc:
console.print(f"[red]Import error:[/red] {exc}")
panel_console.print(f"[red]Import error:[/red] {exc}")
raise typer.Exit(1) from exc
except DatabaseError as exc:
_log.debug("session import failed", exc_info=True)
@@ -803,9 +842,13 @@ def tell(
typer.Option("--actor", help="Override actor for this message"),
] = None,
stream: Annotated[
bool,
typer.Option("--stream", help="Stream response in real-time"),
] = False,
bool,
HAL9001 commented 2026-04-28 01:52:05 +00:00
Outdated
Review
Copy Link
Copy Source

CRITICAL BUG: fmt is not defined in this function scope. tell has no fmt parameter, and there is no module-level fmt variable. This will raise NameError at runtime. Add fmt: str = "rich" as a parameter to fix.

CRITICAL BUG: `fmt` is not defined in this function scope. `tell` has no `fmt` parameter, and there is no module-level `fmt` variable. This will raise `NameError` at runtime. Add `fmt: str = "rich"` as a parameter to fix.
typer.Option("--stream", help="Stream response in real-time"),
] = False,
fmt: Annotated[
str,
typer.Option("--format", "-f", help=_FORMAT_HELP),
] = "rich",
) -> None:
"""Send a message to a session.
@@ -817,6 +860,7 @@ def tell(
agents session tell --session 01HXYZ... --actor openai/gpt-4 "Plan a feature"
agents session tell --session 01HXYZ... --stream "Build tests"
"""
panel_console = _get_panel_console(fmt)
try:
service = _get_session_service()
@@ -850,11 +894,11 @@ def tell(
else:
from rich.markup import escape
console.print(f"[dim]user:[/dim] {escape(prompt)}")
console.print(f"[cyan]assistant:[/cyan] {escape(assistant_content)}")
panel_console.print(f"[dim]user:[/dim] {escape(prompt)}")
panel_console.print(f"[cyan]assistant:[/cyan] {escape(assistant_content)}")
except SessionNotFoundError as exc:
console.print(f"[red]Session not found:[/red] {session_id}")
panel_console.print(f"[red]Session not found:[/red] {session_id}")
raise typer.Exit(1) from exc
except DatabaseError as exc:
_log.debug("session tell failed", exc_info=True)