From c3c8731311d98ae326f6cd6c09683da616435ca7 Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Tue, 26 May 2026 22:59:27 -0400 Subject: [PATCH 01/13] feat(wire): add ToolExecutionStarted and ToolOutputPart events for live tool feedback Introduces two new wire protocol events so the UI can distinguish the approval/hooks phase from actual tool execution, and stream incremental shell output before the final ToolResult arrives. - ToolExecutionStarted: emitted once per tool call after approval completes, before the tool body runs; file/MCP/external tools emit from the approval path while Shell and RunAgents emit it themselves at the right moment - ToolOutputPart: streamed from the Shell tool as stdout/stderr lines arrive TUI blocks now hold the execution-started spinner until ToolExecutionStarted lands (showing a calm "preparing" row before that), render streamed output as a live tail preview, and the Bash card shows "running" status when partial output is present. The composing _ContentBlock also gains a live Markdown preview while the model writes. --- src/pythinker_code/soul/approval.py | 9 +- src/pythinker_code/soul/toolset.py | 49 +++++++++++ src/pythinker_code/tools/agent/__init__.py | 4 + src/pythinker_code/tools/file/replace.py | 3 + src/pythinker_code/tools/file/write.py | 3 + src/pythinker_code/tools/shell/__init__.py | 20 ++++- .../ui/shell/tool_renderers/bash.py | 8 +- .../ui/shell/visualize/_blocks.py | 84 +++++++++++++++++-- .../ui/shell/visualize/_live_view.py | 20 +++++ src/pythinker_code/wire/types.py | 22 +++++ .../test_streaming_content_block.py | 23 ++++- .../test_tui_blocks_integration.py | 32 +++++-- tests_e2e/test_wire_approvals_tools.py | 50 +++++++++++ tests_e2e/test_wire_prompt.py | 20 ++++- tests_e2e/test_wire_protocol.py | 5 ++ tests_e2e/test_wire_sessions.py | 12 ++- tests_e2e/test_wire_skills_mcp.py | 5 ++ tests_e2e/wire_helpers.py | 31 ++++++- web/src/hooks/wireTypes.ts | 18 ++++ 19 files changed, 391 insertions(+), 27 deletions(-) diff --git a/src/pythinker_code/soul/approval.py b/src/pythinker_code/soul/approval.py index b8f90779..55b56d8f 100644 --- a/src/pythinker_code/soul/approval.py +++ b/src/pythinker_code/soul/approval.py @@ -10,7 +10,10 @@ ApprovalSource, get_current_approval_source_or_none, ) -from pythinker_code.soul.toolset import get_current_tool_call_or_none +from pythinker_code.soul.toolset import ( + emit_current_tool_execution_started, + get_current_tool_call_or_none, +) from pythinker_code.tools.utils import ToolRejectedError from pythinker_code.utils.logging import logger from pythinker_code.wire.types import DisplayBlock @@ -210,6 +213,7 @@ async def request( tool_name=tool_call.function.name, approval_mode="auto" if self.is_auto() else "yolo", ) + emit_current_tool_execution_started() return ApprovalResult(approved=True) if action in self._state.auto_approve_actions: @@ -220,6 +224,7 @@ async def request( tool_name=tool_call.function.name, approval_mode="auto_session", ) + emit_current_tool_execution_started() return ApprovalResult(approved=True) request_id = str(uuid.uuid4()) @@ -258,6 +263,7 @@ async def request( tool_name=tool_call.function.name, approval_mode="manual", ) + emit_current_tool_execution_started() return ApprovalResult(approved=True) case "approve_for_session": track( @@ -270,6 +276,7 @@ async def request( for pending in self._runtime.list_pending(): if pending.action == action: self._runtime.resolve(pending.id, "approve") + emit_current_tool_execution_started() return ApprovalResult(approved=True) case "reject": track( diff --git a/src/pythinker_code/soul/toolset.py b/src/pythinker_code/soul/toolset.py index ba02fe4f..4b5251f2 100644 --- a/src/pythinker_code/soul/toolset.py +++ b/src/pythinker_code/soul/toolset.py @@ -43,6 +43,7 @@ TextPart, ToolCall, ToolCallRequest, + ToolExecutionStarted, ToolResult, ToolReturnValue, VideoURLPart, @@ -58,6 +59,9 @@ from pythinker_code.soul.agent import Runtime current_tool_call = ContextVar[ToolCall | None]("current_tool_call", default=None) +_current_tool_execution_started_ids: ContextVar[set[str] | None] = ContextVar( + "current_tool_execution_started_ids", default=None +) _current_session_id: ContextVar[str] = ContextVar("_current_session_id", default="") _MCP_LOG_NAME_RE = re.compile(r"[^A-Za-z0-9_.-]+") @@ -83,6 +87,41 @@ def get_current_tool_call_or_none() -> ToolCall | None: return current_tool_call.get() +def emit_current_tool_execution_started() -> None: + """Emit ToolExecutionStarted once for the current tool call, if wire is active.""" + tool_call = get_current_tool_call_or_none() + if tool_call is None: + return + + started_ids = _current_tool_execution_started_ids.get() + if started_ids is None: + started_ids = set[str]() + _current_tool_execution_started_ids.set(started_ids) + if tool_call.id in started_ids: + return + started_ids.add(tool_call.id) + + try: + from pythinker_code.soul import get_wire_or_none + + if wire := get_wire_or_none(): + wire.soul_side.send(ToolExecutionStarted(tool_call_id=tool_call.id)) + except Exception as exc: # noqa: BLE001 - lifecycle events must not break tool execution + logger.debug( + "Failed to emit tool execution start: {tool_name} (call_id={call_id}): {error}", + tool_name=tool_call.function.name, + call_id=tool_call.id, + error=exc, + ) + + +def _tool_defers_execution_started(tool: ToolType) -> bool: + return bool( + getattr(tool, "emits_tool_execution_started_after_approval", False) + or hasattr(tool, "_approval") + ) + + def _mcp_stderr_log_path(runtime: Runtime, server_name: str) -> Path: safe_name = _MCP_LOG_NAME_RE.sub("_", server_name).strip("._-") or "server" log_dir = runtime.session.dir / "mcp" @@ -184,6 +223,13 @@ def handle(self, tool_call: ToolCall) -> HandleResult: return ToolResult(tool_call_id=tool_call.id, return_value=ToolParseError(str(e))) async def _call(): + started_ids_token = _current_tool_execution_started_ids.set(set[str]()) + try: + return await _call_with_lifecycle() + finally: + _current_tool_execution_started_ids.reset(started_ids_token) + + async def _call_with_lifecycle(): tool_input_dict = arguments if isinstance(arguments, dict) else {} if self._runtime is not None: @@ -225,6 +271,9 @@ async def _call(): from pythinker_code.telemetry import metrics as _m from pythinker_code.telemetry import otel as _otel + if not _tool_defers_execution_started(tool): + emit_current_tool_execution_started() + t0 = time.monotonic() _tool_span_cm = _otel.start_span( "pythinker.tool", diff --git a/src/pythinker_code/tools/agent/__init__.py b/src/pythinker_code/tools/agent/__init__.py index a76cc9f4..02cf7127 100644 --- a/src/pythinker_code/tools/agent/__init__.py +++ b/src/pythinker_code/tools/agent/__init__.py @@ -462,6 +462,7 @@ def is_limited(self) -> bool: class RunAgentsTool(CallableTool2[RunAgentsParams]): name: str = "RunAgents" params: type[RunAgentsParams] = RunAgentsParams + emits_tool_execution_started_after_approval = True def __init__(self, runtime: Runtime): max_background = runtime.config.background.max_running_tasks @@ -546,6 +547,9 @@ async def __call__(self, params: RunAgentsParams) -> ToolReturnValue: fingerprint = _run_agents_fingerprint(params) if self._runtime.approval.is_orchestration_approved(fingerprint): + from pythinker_code.soul.toolset import emit_current_tool_execution_started + + emit_current_tool_execution_started() orchestration_approval = "reused" else: approved_count = capacity.launch_count if capacity is not None else len(params.agents) diff --git a/src/pythinker_code/tools/file/replace.py b/src/pythinker_code/tools/file/replace.py index 35e91cd9..2992d82e 100644 --- a/src/pythinker_code/tools/file/replace.py +++ b/src/pythinker_code/tools/file/replace.py @@ -173,6 +173,9 @@ async def __call__(self, params: Params) -> ToolReturnValue: if not result: return result.rejection_error() + from pythinker_code.soul.toolset import emit_current_tool_execution_started + + emit_current_tool_execution_started() create_file_restore_point(self._runtime.session, tool_name=self.name, path=str(p)) # Write the modified content back to the file diff --git a/src/pythinker_code/tools/file/write.py b/src/pythinker_code/tools/file/write.py index 60284673..b69afacc 100644 --- a/src/pythinker_code/tools/file/write.py +++ b/src/pythinker_code/tools/file/write.py @@ -159,6 +159,9 @@ async def __call__(self, params: Params) -> ToolReturnValue: if not result: return result.rejection_error() + from pythinker_code.soul.toolset import emit_current_tool_execution_started + + emit_current_tool_execution_started() create_file_restore_point(self._runtime.session, tool_name=self.name, path=str(p)) # Write content to file diff --git a/src/pythinker_code/tools/shell/__init__.py b/src/pythinker_code/tools/shell/__init__.py index 4c71cc8a..3d50cd0d 100644 --- a/src/pythinker_code/tools/shell/__init__.py +++ b/src/pythinker_code/tools/shell/__init__.py @@ -1,7 +1,7 @@ import asyncio from collections.abc import Callable from pathlib import Path -from typing import Self, override +from typing import Literal, Self, override import pythinker_host from pydantic import BaseModel, Field, model_validator @@ -124,13 +124,31 @@ async def __call__(self, params: Params) -> ToolReturnValue: if not result: return result.rejection_error() + tool_call = get_current_tool_call_or_none() + + def emit_output_part(stream: Literal["stdout", "stderr", "output"], text: str) -> None: + if tool_call is None or not text: + return + try: + from pythinker_code.soul import get_wire_or_none + from pythinker_code.wire.types import ToolOutputPart + + if wire := get_wire_or_none(): + wire.soul_side.send( + ToolOutputPart(tool_call_id=tool_call.id, stream=stream, text=text) + ) + except Exception as exc: # noqa: BLE001 - streaming must not break the tool + logger.debug("Failed to stream shell output: {error}", error=exc) + def stdout_cb(line: bytes): line_str = line.decode(encoding="utf-8", errors="replace") builder.write(line_str) + emit_output_part("stdout", line_str) def stderr_cb(line: bytes): line_str = line.decode(encoding="utf-8", errors="replace") builder.write(line_str) + emit_output_part("stderr", line_str) try: exitcode = await self._run_shell_command( diff --git a/src/pythinker_code/ui/shell/tool_renderers/bash.py b/src/pythinker_code/ui/shell/tool_renderers/bash.py index dde3db66..b885b62d 100644 --- a/src/pythinker_code/ui/shell/tool_renderers/bash.py +++ b/src/pythinker_code/ui/shell/tool_renderers/bash.py @@ -71,7 +71,9 @@ def _render_call(ctx: ToolRenderContext) -> RenderableType | None: description = as_str(args.get("description")) suffix = f" (background: {description})" if description else " (background)" summary.append_text(fg("muted", suffix)) - style_token = "error" if ctx.is_error else "success" if ctx.has_result else "muted" + style_token = ( + "error" if ctx.is_error else "success" if ctx.has_result and not ctx.is_partial else "muted" + ) line = tool_call_header("Bash", summary, style_token=style_token) return running_spinner(line, execution_started=ctx.execution_started, has_result=ctx.has_result) @@ -95,7 +97,9 @@ def _render_result(ctx: ToolRenderContext, result: ToolResultPayload) -> Rendera extras = cast("dict[str, object]", extras_raw) if isinstance(extras_raw, dict) else {} status_value = extras.get("status") status: BashStatus - if status_value == "cancelled": + if ctx.is_partial or status_value == "running": + status = "running" + elif status_value == "cancelled": status = "cancelled" else: status = "error" if result.is_error else "complete" diff --git a/src/pythinker_code/ui/shell/visualize/_blocks.py b/src/pythinker_code/ui/shell/visualize/_blocks.py index f5431c76..64246bd4 100644 --- a/src/pythinker_code/ui/shell/visualize/_blocks.py +++ b/src/pythinker_code/ui/shell/visualize/_blocks.py @@ -66,6 +66,7 @@ _ELLIPSIS = "..." _THINKING_PREVIEW_LINES = 6 +_COMPOSING_PREVIEW_LINES = 12 MAX_SUBAGENT_TOOL_CALLS_TO_SHOW = 4 # Background-agent statuses that mean "still running" — the tool call result @@ -203,7 +204,7 @@ def compose(self) -> RenderableType: if self._show_thinking_stream: return self._compose_thinking_stream() return self._compose_thinking() - return self._compose_spinner() + return self._compose_composing() def compose_final(self) -> RenderableType: """Render the remaining uncommitted content when the block ends.""" @@ -253,6 +254,15 @@ def _wrap_bullet(self, renderable: RenderableType) -> BulletColumns: bullet=Text(TRANSCRIPT_ASSISTANT_MARKER, style=tui_rich_style("success")), ) + def _wrap_preview_bullet(self, renderable: RenderableType) -> BulletColumns: + """Wrap transient live preview without mutating scrollback bullet state.""" + if self._has_printed_bullet: + return BulletColumns(renderable, bullet=Text(" ")) + return BulletColumns( + renderable, + bullet=Text(TRANSCRIPT_ASSISTANT_MARKER, style=tui_rich_style("success")), + ) + @property def has_emitted_to_scrollback(self) -> bool: """Whether any part of this block has been printed to scrollback yet.""" @@ -292,6 +302,14 @@ def _activity_snapshot( spinner="shape", ) + def _compose_composing(self) -> RenderableType: + spinner = self._compose_spinner() + pending = self._pending_text() + if not pending: + return spinner + preview = self._build_preview(pending, max_lines=_COMPOSING_PREVIEW_LINES) + return Group(spinner, self._wrap_preview_bullet(Markdown(preview))) + def _compose_spinner(self) -> Text: return activity_status_line( self._activity_snapshot("Composing", label_style=tui_rich_style("thinking_text")), @@ -304,7 +322,7 @@ def _compose_thinking_stream(self) -> RenderableType: pending = self._pending_text() if not pending: return spinner - preview = self._build_preview(pending) + preview = self._build_preview(pending, max_lines=_THINKING_PREVIEW_LINES) preview_style = tui_rich_style("thinking_text") + Style(italic=True) return Group(spinner, Text(preview, style=preview_style)) @@ -314,10 +332,10 @@ def _compose_thinking_spinner(self) -> Text: width=current_console_width(), ) - def _build_preview(self, text: str) -> str: - """Tail-trim *text* to the last ``_THINKING_PREVIEW_LINES`` and clamp width.""" + def _build_preview(self, text: str, *, max_lines: int) -> str: + """Tail-trim *text* to ``max_lines`` and clamp it to current terminal width.""" max_width = current_console_width() - 2 - tail_text = _tail_lines(text, _THINKING_PREVIEW_LINES) + tail_text = _tail_lines(text, max_lines) lines = tail_text.split("\n") return "\n".join(_truncate_to_display_width(line, max_width) for line in lines) @@ -358,6 +376,14 @@ def __init__(self, tool_call: ToolCall): # ``pythinker`` worklog path so that rendering is bit-for-bit # unchanged. self._tui_card: ToolExecutionComponent | None = None + # True once the runtime reports that approval/hooks are complete and + # the tool body is executing. Before this, streamed tool-call args render + # as a calm "preparing" row instead of an execution spinner. + self._execution_started: bool = False + # Incremental tool output (currently shell stdout/stderr) that should be + # visible before the final ToolResult arrives. + self._streamed_output_parts: list[str] = [] + self._streamed_output_had_stderr: bool = False # True while the Agent tool result indicates a still-running background # agent. The block stays in _tool_call_blocks (and in the Live area) # rather than being flushed to static scrollback, so the spinner keeps @@ -420,6 +446,23 @@ def append_args_part(self, args_part: str): self._argument = argument self._renderable = self._compose() + def mark_execution_started(self) -> None: + if self._execution_started: + return + self._execution_started = True + if self._tui_card is not None: + self._tui_card.mark_execution_started() + self._renderable = self._compose() + + def append_output_part(self, text: str, *, stream: str = "output") -> None: + if self.finished or not text: + return + self._streamed_output_parts.append(text) + if stream == "stderr": + self._streamed_output_had_stderr = True + self.mark_execution_started() + self._renderable = self._compose() + def finish(self, result: ToolReturnValue): self._result = result result_text = self._card_result_text(result) @@ -523,6 +566,11 @@ def _compose(self) -> RenderableType: children.append(render_activity_tree(rows, width=current_console_width())) if self._result is None: + streamed_output = self._streamed_output_text() + if streamed_output: + preview = _tail_lines(streamed_output.rstrip("\n"), 8) + output_style = "error" if self._streamed_output_had_stderr else "muted" + children.append(Text(preview, style=tui_rich_style(output_style))) return render_worklog_entry( label=style.label, target=self._argument, @@ -575,8 +623,8 @@ def _compose_card(self) -> RenderableType | None: self._tool_call_id, definition=definition, ) - # We see the tool call event, so the model has begun work. - self._tui_card.mark_execution_started() + if self._execution_started: + self._tui_card.mark_execution_started() raw_args = self._lexer.complete_json() or "{}" try: parsed = json.loads(raw_args, strict=False) @@ -584,10 +632,11 @@ def _compose_card(self) -> RenderableType | None: parsed = {} if isinstance(parsed, dict): self._tui_card.update_args(cast(dict[str, Any], parsed)) - # Args are complete once a result lands; before that we treat + # Args are complete once execution starts; before that we treat # complete_json output as best-effort. - if self._result is not None: + if self._execution_started or self._result is not None: self._tui_card.set_args_complete() + if self._result is not None: self._tui_card.set_result( ToolResultPayload( text=self._card_result_text(self._result), @@ -596,8 +645,25 @@ def _compose_card(self) -> RenderableType | None: ), is_partial=self._is_background_pending, ) + elif streamed_output := self._streamed_output_text(): + self._tui_card.set_result( + ToolResultPayload( + text=streamed_output, + is_error=False, + details={ + "output": streamed_output, + "message": "", + "display": [], + "extras": {"status": "running"}, + }, + ), + is_partial=True, + ) return self._tui_card.render() + def _streamed_output_text(self) -> str: + return "".join(self._streamed_output_parts) + @staticmethod def _card_result_details(result: ToolReturnValue) -> dict[str, Any]: """Preserve structured tool result data for Blackbox-style cards. diff --git a/src/pythinker_code/ui/shell/visualize/_live_view.py b/src/pythinker_code/ui/shell/visualize/_live_view.py index 5970e745..ce3f6040 100644 --- a/src/pythinker_code/ui/shell/visualize/_live_view.py +++ b/src/pythinker_code/ui/shell/visualize/_live_view.py @@ -93,6 +93,8 @@ ToolCall, ToolCallPart, ToolCallRequest, + ToolExecutionStarted, + ToolOutputPart, ToolResult, TurnBegin, TurnEnd, @@ -805,6 +807,10 @@ def dispatch_wire_message(self, msg: WireMessage) -> None: self.append_tool_call(msg) case ToolCallPart(): self.append_tool_call_part(msg) + case ToolExecutionStarted(): + self.mark_tool_execution_started(msg.tool_call_id) + case ToolOutputPart(): + self.append_tool_output_part(msg) case ToolResult(): self.append_tool_result(msg) case ApprovalResponse(): @@ -1092,6 +1098,16 @@ def append_tool_call_part(self, part: ToolCallPart) -> None: self._last_tool_call_block.append_args_part(part.arguments_part) self.refresh_soon() + def mark_tool_execution_started(self, tool_call_id: str) -> None: + if block := self._tool_call_blocks.get(tool_call_id): + block.mark_execution_started() + self.refresh_soon() + + def append_tool_output_part(self, part: ToolOutputPart) -> None: + if block := self._tool_call_blocks.get(part.tool_call_id): + block.append_output_part(part.text, stream=part.stream) + self.refresh_soon() + def append_tool_result(self, result: ToolResult) -> None: if block := self._tool_call_blocks.get(result.tool_call_id): self._record_todo_display(result.return_value) @@ -1244,6 +1260,10 @@ def handle_subagent_event(self, event: SubagentEvent) -> None: case ToolResult() as tool_result: block.finish_sub_tool_call(tool_result) self.refresh_soon() + case ToolExecutionStarted() | ToolOutputPart(): + # Nested subagent execution/output streaming is intentionally + # summarized at the parent Agent-card level for now. + self.refresh_soon() case _: # ignore other events for now # TODO: may need to handle multi-level nested subagents diff --git a/src/pythinker_code/wire/types.py b/src/pythinker_code/wire/types.py index 4d522840..b19926ab 100644 --- a/src/pythinker_code/wire/types.py +++ b/src/pythinker_code/wire/types.py @@ -96,6 +96,24 @@ class StepRetry(BaseModel): """HTTP status code when available.""" +class ToolExecutionStarted(BaseModel): + """Indicates that a tool call has passed approval/hooks and is executing.""" + + tool_call_id: str + """The ID of the tool call that started executing.""" + + +class ToolOutputPart(BaseModel): + """Incremental output produced by a currently executing tool.""" + + tool_call_id: str + """The ID of the tool call that produced this output.""" + stream: Literal["stdout", "stderr", "output"] = "output" + """The output stream, when known.""" + text: str + """The output chunk text.""" + + class CompactionBegin(BaseModel): """ Indicates that a compaction just began. @@ -542,6 +560,8 @@ def resolved(self) -> bool: | StepBegin | StepInterrupted | StepRetry + | ToolExecutionStarted + | ToolOutputPart | HookTriggered | HookResolved | CompactionBegin @@ -693,6 +713,8 @@ def to_wire_message(self) -> WireMessage: "StepBegin", "StepInterrupted", "StepRetry", + "ToolExecutionStarted", + "ToolOutputPart", "CompactionBegin", "CompactionEnd", "MCPLoadingBegin", diff --git a/tests/ui_and_conv/test_streaming_content_block.py b/tests/ui_and_conv/test_streaming_content_block.py index 1d981707..2c447680 100644 --- a/tests/ui_and_conv/test_streaming_content_block.py +++ b/tests/ui_and_conv/test_streaming_content_block.py @@ -222,6 +222,7 @@ def test_composing_live_label_uses_professional_activity_wording(): assert "Composing" in output assert "tokens" in output + assert "hello" in output def test_thinking_status_line_uses_compact_activity_metadata(): @@ -251,7 +252,7 @@ def test_composing_and_thinking_labels_are_neutral_grey(): composing = _ContentBlock(is_think=False) composing.append("hello") - composing_renderable = composing.compose() + composing_renderable = composing._compose_spinner() assert isinstance(composing_renderable, Text) assert _style_for(composing_renderable, "Composing").color == thinking_grey assert _style_for(composing_renderable, "Composing").color != muted @@ -288,6 +289,26 @@ def test_composing_no_commit_without_newline(self): block.append("just some text without newlines") assert block._committed_len == 0 + def test_composing_previews_pending_text_without_committing(self): + block = _ContentBlock(is_think=False) + block.append("live preview without newline") + console = Console(record=True, width=120, color_system=None) + console.print(block.compose()) + + assert "live preview without newline" in console.export_text() + assert block._committed_len == 0 + assert not block.has_emitted_to_scrollback + + def test_composing_preview_is_tail_limited(self): + block = _ContentBlock(is_think=False) + block.append("\n".join(f"line {i:02d}" for i in range(1, 21))) + console = Console(record=True, width=120, color_system=None) + console.print(block.compose()) + output = console.export_text() + + assert "line 20" in output + assert "line 01" not in output + def test_newline_split_across_chunks(self): """Block boundary \\n\\n split across two chunks should still commit.""" block = _ContentBlock(is_think=False) diff --git a/tests/ui_and_conv/test_tui_blocks_integration.py b/tests/ui_and_conv/test_tui_blocks_integration.py index 88cd3dc6..dda8d629 100644 --- a/tests/ui_and_conv/test_tui_blocks_integration.py +++ b/tests/ui_and_conv/test_tui_blocks_integration.py @@ -146,6 +146,21 @@ def test_card_style_streaming_args_then_result(_force_card_style): assert "5 lines" in rendered +def test_card_style_streamed_shell_output_renders_before_result(_force_card_style): + from pythinker_code.ui.shell.tool_renderers import register_builtin_renderers + + register_builtin_renderers() + block = _ToolCallBlock(_make_tool_call(name="Shell", args='{"command":"printf hi"}')) + + block.mark_execution_started() + block.append_output_part("hi", stream="stdout") + + rendered = render_plain(block.compose(), width=80) + assert "Bash" in rendered + assert "hi" in rendered + assert "esc to cancel" in rendered + + def test_card_style_error_result(_force_card_style): _register_read_renderer() block = _ToolCallBlock(_make_tool_call()) @@ -164,6 +179,7 @@ def test_card_style_running_subagent_uses_solid_circle(_force_card_style, monkey block = _ToolCallBlock( _make_tool_call(name="Agent", args='{"description":"Audit UI","prompt":"check"}') ) + block.mark_execution_started() rendered = render_plain(block.compose(), width=80) spinner_frames = set("⠋⠙⠹⠸⠼⠴⠦⠧⠇⠏") @@ -212,6 +228,7 @@ def test_card_style_running_task_output_uses_solid_circle(_force_card_style, mon args='{"task_id":"agent-123","block":true,"timeout":300}', ) ) + block.mark_execution_started() rendered = render_plain(block.compose(), width=80) spinner_frames = set("⠋⠙⠹⠸⠼⠴⠦⠧⠇⠏") @@ -231,6 +248,7 @@ def test_card_style_running_subagent_marker_pulses(_force_card_style, monkeypatc block = _ToolCallBlock( _make_tool_call(name="Agent", args='{"description":"Audit UI","prompt":"check"}') ) + block.mark_execution_started() first = render_plain(block.compose(), width=80) monkeypatch.setattr( @@ -314,8 +332,7 @@ def test_card_style_background_subagent_marker_pulses(_force_card_style, monkeyp def test_card_style_lifecycle_marks_execution_started(_force_card_style): - """_ToolCallBlock should call mark_execution_started on the card so - renderers see ctx.execution_started == True from the first compose.""" + """ToolExecutionStarted should mark the card running before the result arrives.""" seen = {"execution_started": False, "args_complete": False} def render_call(ctx: ToolRenderContext): @@ -331,11 +348,16 @@ def render_call(ctx: ToolRenderContext): ) ) block = _ToolCallBlock(_make_tool_call()) - # Initial compose runs from __init__ — execution_started should be set. render_plain(block.compose(), width=40) - assert seen["execution_started"] is True + assert seen["execution_started"] is False assert seen["args_complete"] is False - # After the result lands, args_complete should be set too. + + block.mark_execution_started() + render_plain(block.compose(), width=40) + assert seen["execution_started"] is True + assert seen["args_complete"] is True + + # After the result lands, args_complete should remain set. block.finish(_ok_result("done")) render_plain(block.compose(), width=40) assert seen["args_complete"] is True diff --git a/tests_e2e/test_wire_approvals_tools.py b/tests_e2e/test_wire_approvals_tools.py index 2cd5af63..3a87a936 100644 --- a/tests_e2e/test_wire_approvals_tools.py +++ b/tests_e2e/test_wire_approvals_tools.py @@ -144,6 +144,16 @@ def test_shell_approval_approve(tmp_path) -> None: "type": "ApprovalResponse", "payload": {"request_id": "", "response": "approve", "feedback": ""}, }, + { + "method": "event", + "type": "ToolExecutionStarted", + "payload": {"tool_call_id": "tc-1"}, + }, + { + "method": "event", + "type": "ToolOutputPart", + "payload": {"tool_call_id": "tc-1", "stream": "stdout", "text": "ok\n"}, + }, { "method": "event", "type": "ToolResult", @@ -410,6 +420,16 @@ def test_approve_for_session(tmp_path) -> None: "feedback": "", }, }, + { + "method": "event", + "type": "ToolExecutionStarted", + "payload": {"tool_call_id": "tc-1"}, + }, + { + "method": "event", + "type": "ToolOutputPart", + "payload": {"tool_call_id": "tc-1", "stream": "stdout", "text": "first\n"}, + }, { "method": "event", "type": "ToolResult", @@ -478,6 +498,16 @@ def test_approve_for_session(tmp_path) -> None: "mcp_status": None, }, }, + { + "method": "event", + "type": "ToolExecutionStarted", + "payload": {"tool_call_id": "tc-2"}, + }, + { + "method": "event", + "type": "ToolOutputPart", + "payload": {"tool_call_id": "tc-2", "stream": "stdout", "text": "second\n"}, + }, { "method": "event", "type": "ToolResult", @@ -584,6 +614,16 @@ def test_yolo_skips_approval(tmp_path) -> None: "mcp_status": None, }, }, + { + "method": "event", + "type": "ToolExecutionStarted", + "payload": {"tool_call_id": "tc-1"}, + }, + { + "method": "event", + "type": "ToolOutputPart", + "payload": {"tool_call_id": "tc-1", "stream": "stdout", "text": "ok\n"}, + }, { "method": "event", "type": "ToolResult", @@ -887,6 +927,11 @@ def test_display_block_todo(tmp_path) -> None: "mcp_status": None, }, }, + { + "method": "event", + "type": "ToolExecutionStarted", + "payload": {"tool_call_id": "tc-1"}, + }, { "method": "event", "type": "ToolResult", @@ -1005,6 +1050,11 @@ def test_tool_call_part_streaming(tmp_path) -> None: "mcp_status": None, }, }, + { + "method": "event", + "type": "ToolExecutionStarted", + "payload": {"tool_call_id": "tc-1"}, + }, { "method": "event", "type": "ToolResult", diff --git a/tests_e2e/test_wire_prompt.py b/tests_e2e/test_wire_prompt.py index 686de37a..cb8c8144 100644 --- a/tests_e2e/test_wire_prompt.py +++ b/tests_e2e/test_wire_prompt.py @@ -297,6 +297,11 @@ def test_max_steps_reached(tmp_path) -> None: "mcp_status": None, }, }, + { + "method": "event", + "type": "ToolExecutionStarted", + "payload": {"tool_call_id": "tc-1"}, + }, { "method": "event", "type": "ToolResult", @@ -307,10 +312,7 @@ def test_max_steps_reached(tmp_path) -> None: "output": "Todo list updated", "message": "Todo list updated", "display": [ - { - "type": "todo", - "items": [{"title": "x", "status": "pending"}], - } + {"type": "todo", "items": [{"title": "x", "status": "pending"}]} ], "extras": None, }, @@ -499,6 +501,16 @@ def test_concurrent_prompt_error(tmp_path) -> None: "type": "ApprovalResponse", "payload": {"request_id": "", "response": "approve", "feedback": ""}, }, + { + "method": "event", + "type": "ToolExecutionStarted", + "payload": {"tool_call_id": "tc-1"}, + }, + { + "method": "event", + "type": "ToolOutputPart", + "payload": {"tool_call_id": "tc-1", "stream": "stdout", "text": "hi\n"}, + }, { "method": "event", "type": "ToolResult", diff --git a/tests_e2e/test_wire_protocol.py b/tests_e2e/test_wire_protocol.py index ac33b2af..03651aa1 100644 --- a/tests_e2e/test_wire_protocol.py +++ b/tests_e2e/test_wire_protocol.py @@ -467,6 +467,11 @@ def handle_request(msg: dict[str, Any]) -> dict[str, Any]: "arguments": '{"path": "README.md"}', }, }, + { + "method": "event", + "type": "ToolExecutionStarted", + "payload": {"tool_call_id": "tc-1"}, + }, { "method": "event", "type": "ToolResult", diff --git a/tests_e2e/test_wire_sessions.py b/tests_e2e/test_wire_sessions.py index 5b03d84f..bd3a1e3d 100644 --- a/tests_e2e/test_wire_sessions.py +++ b/tests_e2e/test_wire_sessions.py @@ -425,7 +425,7 @@ def test_replay_streams_wire_history(tmp_path) -> None: assert resp.get("result") == snapshot( { "status": "finished", - "events": 10, + "events": 12, "requests": 0, } ) @@ -465,6 +465,16 @@ def test_replay_streams_wire_history(tmp_path) -> None: "mcp_status": None, }, }, + { + "method": "event", + "type": "ToolExecutionStarted", + "payload": {"tool_call_id": "tc-1"}, + }, + { + "method": "event", + "type": "ToolOutputPart", + "payload": {"tool_call_id": "tc-1", "stream": "stdout", "text": "ok\n"}, + }, { "method": "event", "type": "ToolResult", diff --git a/tests_e2e/test_wire_skills_mcp.py b/tests_e2e/test_wire_skills_mcp.py index 41b804b4..0a8e12df 100644 --- a/tests_e2e/test_wire_skills_mcp.py +++ b/tests_e2e/test_wire_skills_mcp.py @@ -387,6 +387,11 @@ def ping(text: str) -> str: "type": "ApprovalResponse", "payload": {"request_id": "", "response": "approve", "feedback": ""}, }, + { + "method": "event", + "type": "ToolExecutionStarted", + "payload": {"tool_call_id": "tc-1"}, + }, { "method": "event", "type": "ToolResult", diff --git a/tests_e2e/wire_helpers.py b/tests_e2e/wire_helpers.py index 09473d02..9ae091cd 100644 --- a/tests_e2e/wire_helpers.py +++ b/tests_e2e/wire_helpers.py @@ -552,6 +552,7 @@ def _normalize_step_block(block: list[dict[str, Any]]) -> list[dict[str, Any]]: status_updates: list[dict[str, Any]] = [] requests: list[dict[str, Any]] = [] approvals: list[dict[str, Any]] = [] + tool_progress: list[dict[str, Any]] = [] tool_results: list[dict[str, Any]] = [] other: list[dict[str, Any]] = [] tool_call_order: list[str] = [] @@ -571,22 +572,46 @@ def _normalize_step_block(block: list[dict[str, Any]]) -> list[dict[str, Any]]: requests.append(msg) elif msg_type == "ApprovalResponse": approvals.append(msg) + elif msg_type in {"ToolExecutionStarted", "ToolOutputPart"}: + tool_progress.append(msg) elif msg_type == "ToolResult": tool_results.append(msg) else: other.append(msg) + tool_progress = _order_tool_progress(tool_progress, tool_call_order) tool_results = _order_tool_results(tool_results, tool_call_order) - return head + stream_events + status_updates + requests + approvals + tool_results + other + return ( + head + + stream_events + + status_updates + + requests + + approvals + + tool_progress + + tool_results + + other + ) + + +def _order_tool_progress( + tool_progress: list[dict[str, Any]], tool_call_order: list[str] +) -> list[dict[str, Any]]: + return _order_by_tool_call_id(tool_progress, tool_call_order) def _order_tool_results( tool_results: list[dict[str, Any]], tool_call_order: list[str] +) -> list[dict[str, Any]]: + return _order_by_tool_call_id(tool_results, tool_call_order) + + +def _order_by_tool_call_id( + messages: list[dict[str, Any]], tool_call_order: list[str] ) -> list[dict[str, Any]]: if not tool_call_order: - return tool_results + return messages by_id: dict[str, list[dict[str, Any]]] = {} unknown: list[dict[str, Any]] = [] - for msg in tool_results: + for msg in messages: payload = msg.get("payload") tool_call_id = payload.get("tool_call_id") if isinstance(payload, dict) else None if isinstance(tool_call_id, str) and tool_call_id in tool_call_order: diff --git a/web/src/hooks/wireTypes.ts b/web/src/hooks/wireTypes.ts index 43eb3f17..9355472a 100644 --- a/web/src/hooks/wireTypes.ts +++ b/web/src/hooks/wireTypes.ts @@ -99,6 +99,22 @@ export type ToolCallPartEvent = { }; }; +export type ToolExecutionStartedEvent = { + type: "ToolExecutionStarted"; + payload: { + tool_call_id: string; + }; +}; + +export type ToolOutputPartEvent = { + type: "ToolOutputPart"; + payload: { + tool_call_id: string; + stream: "stdout" | "stderr" | "output"; + text: string; + }; +}; + /** * Tool result event from backend * @see pythinker_core.tooling.ToolReturnValue for the source type @@ -272,6 +288,8 @@ export type WireEvent = | ContentPartEvent | ToolCallEvent | ToolCallPartEvent + | ToolExecutionStartedEvent + | ToolOutputPartEvent | ToolResultEvent | StatusUpdateEvent | SessionNoticeEvent From 4748fc522a857778274bb635db3a9c8a86ebc0e0 Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Tue, 26 May 2026 23:15:24 -0400 Subject: [PATCH 02/13] fix(agents): guard against out-of-policy lint findings in explore/plan agents; pad markdown code blocks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit explore.yaml: require supplemental ruff checks (e.g. --select C901) to be labeled "outside project lint policy" so callers can distinguish enforced violations from advisory findings. plan.yaml: add context-gate rule to verify any lint/complexity finding is in the project's active select list before proposing a refactor — prevents plans driven by rules the project intentionally does not enforce. markdown.py: yield a blank_row() above and below each bordered code block so panels read as distinct sections rather than crowding surrounding prose. Test added to assert the blank-row framing is present. --- src/pythinker_code/agents/default/explore.yaml | 1 + src/pythinker_code/agents/default/plan.yaml | 1 + src/pythinker_code/ui/shell/components/markdown.py | 7 ++++++- tests/ui/test_shell_markdown.py | 14 ++++++++++++++ 4 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/pythinker_code/agents/default/explore.yaml b/src/pythinker_code/agents/default/explore.yaml index 9df5ba5c..a34ec95f 100644 --- a/src/pythinker_code/agents/default/explore.yaml +++ b/src/pythinker_code/agents/default/explore.yaml @@ -27,6 +27,7 @@ agent: - NEVER use Shell for any file creation or modification commands - Adapt your search depth based on the thoroughness level specified by the caller - Wherever possible, spawn multiple parallel tool calls for grepping and reading files to maximize speed + - When running lint or complexity checks (e.g. ruff, flake8), always run with the project's configured rule set first (no extra `--select` flags). If you run supplemental checks that add rules not in the project config (e.g. `--select C901` when C901 is absent from pyproject.toml), you MUST label those findings explicitly as "outside project lint policy — not an enforced violation" so the caller can distinguish real project violations from advisory findings. If the prompt includes a block, use it to orient yourself about the repository state before starting your investigation. diff --git a/src/pythinker_code/agents/default/plan.yaml b/src/pythinker_code/agents/default/plan.yaml index c7e10375..a643e6de 100644 --- a/src/pythinker_code/agents/default/plan.yaml +++ b/src/pythinker_code/agents/default/plan.yaml @@ -11,6 +11,7 @@ agent: - Before designing a plan, build a context packet from repository evidence, docs, tests, existing patterns, and the user's stated goal. - If the relevant codebase area is not understood, do not invent a plan. Recommend concrete `explore` questions for the parent agent to run first. - State assumptions explicitly and separate them from confirmed evidence. + - Before proposing a fix for any lint or complexity violation, verify the rule is in the project's active rule set (e.g. `select` in pyproject.toml or .ruff.toml). Findings that only appear via an explicit `--select ` flag not present in the project config are NOT project violations; do not include them in the plan unless the user explicitly asked to enforce that rule. Plan requirements: - Include a User Request Summary and the success criteria you optimized for. diff --git a/src/pythinker_code/ui/shell/components/markdown.py b/src/pythinker_code/ui/shell/components/markdown.py index 2bb9ed57..b7cb7f29 100644 --- a/src/pythinker_code/ui/shell/components/markdown.py +++ b/src/pythinker_code/ui/shell/components/markdown.py @@ -31,7 +31,7 @@ from rich.theme import Theme from pythinker_code.ui.shell.components.render_utils import sanitize_ansi -from pythinker_code.ui.shell.spacing import CODE_BLOCK_PADDING +from pythinker_code.ui.shell.spacing import CODE_BLOCK_PADDING, blank_row from pythinker_code.ui.theme import ThemeName, get_markdown_colors from pythinker_code.utils.rich.markdown import CodeBlock, Markdown from pythinker_code.utils.rich.syntax import PYTHINKER_ANSI_THEME_NAME @@ -99,6 +99,10 @@ def __rich_console__(self, console: Console, options: ConsoleOptions) -> RenderR highlighted.rstrip() lexer_name = self.lexer_name.strip() title = lexer_name if lexer_name and lexer_name != "text" else None + # Frame the code block with a blank row above and below so it reads as a + # distinct section instead of crowding the surrounding prose. Canonical + # ``blank_row()`` (an empty ``Text``) never picks up the panel's tint. + yield blank_row() yield Panel( highlighted, title=title, @@ -109,6 +113,7 @@ def __rich_console__(self, console: Console, options: ConsoleOptions) -> RenderR expand=True, style=panel_style, ) + yield blank_row() def _markdown_style_overrides(theme: ThemeName | None = None) -> dict[str, RichStyle]: diff --git a/tests/ui/test_shell_markdown.py b/tests/ui/test_shell_markdown.py index 8fdc7868..f5e5fdbd 100644 --- a/tests/ui/test_shell_markdown.py +++ b/tests/ui/test_shell_markdown.py @@ -37,6 +37,20 @@ def test_shell_markdown_uses_pythinker_code_block_frame() -> None: assert "╰" in output +def test_shell_markdown_pads_code_block_with_blank_rows() -> None: + # The code block should read as a distinct section, with a blank row framing + # the panel above and below so it never crowds the surrounding prose. + output = _render_text( + PythinkerMarkdown("Before text.\n\n```toml\nkey = 1\n```\n\nAfter text.") + ) + lines = output.splitlines() + top = next(i for i, line in enumerate(lines) if "╭" in line) + bottom = next(i for i, line in enumerate(lines) if "╰" in line) + + assert lines[top - 1].strip() == "" + assert lines[bottom + 1].strip() == "" + + def test_shell_markdown_simplifies_report_emoji_icons() -> None: output = _render_text( PythinkerMarkdown( From 1f4711908294f0fddc19eafc179df072b7d8c2b3 Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Tue, 26 May 2026 23:24:20 -0400 Subject: [PATCH 03/13] docs: add agent live tool stream design spec --- ...026-05-26-agent-live-tool-stream-design.md | 101 ++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 docs/superpowers/specs/2026-05-26-agent-live-tool-stream-design.md diff --git a/docs/superpowers/specs/2026-05-26-agent-live-tool-stream-design.md b/docs/superpowers/specs/2026-05-26-agent-live-tool-stream-design.md new file mode 100644 index 00000000..ec5aeaf2 --- /dev/null +++ b/docs/superpowers/specs/2026-05-26-agent-live-tool-stream-design.md @@ -0,0 +1,101 @@ +# Agent Live Tool Stream — Design Spec + +**Date:** 2026-05-26 +**Status:** Approved + +## Problem + +When a subagent is running inside an `Agent` tool call, the TUI shows only: +- The agent card header with a spinner +- Finished sub-tool calls in a static activity tree + +In-flight sub-tool calls — the `Read`, `Bash`, `Glob` calls the agent is actively making — are tracked internally (`_ongoing_subagent_tool_calls`) but never rendered. The user sees a frozen card with no indication of what the agent is doing moment-to-moment. + +## Goal + +Show the agent "skimming and checking and reading files" in real time: ongoing sub-tool calls appear as shimmering live rows, and shell output streams in below the active call. + +## Design + +### Section 1 — Event Flow + +`_live_view.py:handle_subagent_event()` currently ignores `ToolExecutionStarted` and `ToolOutputPart` subagent events with a "summarized at the parent Agent-card level for now" comment. + +**Change:** forward both events into the parent `_ToolCallBlock` via two new methods: + +``` +SubagentEvent(event=ToolCall) → block.append_sub_tool_call() [already wired] +SubagentEvent(event=ToolCallPart) → block.append_sub_tool_call_part() [already wired] +SubagentEvent(event=ToolExecutionStarted) → block.mark_sub_execution_started() [NEW] +SubagentEvent(event=ToolOutputPart) → block.append_sub_output_part() [NEW] +SubagentEvent(event=ToolResult) → block.finish_sub_tool_call() [already wired] +``` + +No changes needed to the wire protocol or event types — both event types already arrive via `SubagentEvent`. + +### Section 2 — State in `_ToolCallBlock` + +Two new fields added to `__init__`: + +```python +self._subagent_output_parts: dict[str, list[str]] = {} +# Keyed by sub-tool tool_call_id. Accumulates ToolOutputPart chunks. + +self._subagent_execution_started: set[str] = set() +# Sub-tool calls that have passed approval/hooks and are executing. +``` + +**`mark_sub_execution_started(tool_call_id)`**: adds to `_subagent_execution_started`, triggers recompose. + +**`append_sub_output_part(tool_call_id, text)`**: appends to `_subagent_output_parts[tool_call_id]`. Silently discards if `tool_call_id` not in `_ongoing_subagent_tool_calls`. Triggers recompose. + +**`finish_sub_tool_call()`** (existing, modified): also removes from `_subagent_output_parts` and `_subagent_execution_started` to free memory. + +### Section 3 — Rendering + +New layout inside the Agent card while running: + +``` +⠿ Agent(security-reviewer · Deep security code scan) ← existing spinner + ├─ agent Read src/pythinker_code/session.py ← NEW: shimmer (running) + │ def _reset_live_shape(self, live: Live) -> None: ← NEW: stdout preview + │ live._live_render._shape = None + └─ agent Bash grep -n "SubagentEvent" ... ← existing: completed +``` + +**Running rows**: `_ongoing_subagent_tool_calls` → `ActivityRow(state="running")` with shimmer, rendered *above* finished rows. + +**Output preview**: for the single most-recent ongoing call that has streamed output, show the last 4 lines below its activity row. Lines are: +- Indented with `│ ` prefix (or ` ` on the last line) +- Truncated to `current_console_width() - 6` chars per line +- Styled `muted` for stdout, `error` for stderr + +**Cap on running rows**: max 2 ongoing rows shown. If more in flight, prepend `… N more running` in muted style. + +**Cleanup on finish**: output buffer discarded when the sub-call completes. The finished `ActivityRow` shows tool name + key arg only — no trailing output. + +### Section 4 — Error Handling & Edge Cases + +| Scenario | Handling | +|---|---| +| Args not yet fully streamed | `extract_key_argument` returns `None`; row shows bare tool name (e.g. `Read`) until path arrives | +| `ToolOutputPart` arrives before `ToolCall` | Silently discarded — `tool_call_id` not in `_ongoing_subagent_tool_calls` | +| Output buffer growth | Capped at 200 chars total per sub-call (keep tail, discard head); discarded on finish | +| Background-pending agents | By the time `_is_background_pending` is set, `_ongoing_subagent_tool_calls` is empty — no change needed | +| Card style (`is_card_style()`) | New rendering only touches the worklog `_compose()` path; `_compose_card()` / `tool_renderers/agent.py` unchanged (follow-up if needed) | +| Stderr mixed with stdout | Track `_subagent_output_had_stderr: dict[str, bool]`; use `error` style if any stderr seen | + +## Files Changed + +| File | Change | +|---|---| +| `src/pythinker_code/ui/shell/visualize/_live_view.py` | Forward `ToolExecutionStarted` and `ToolOutputPart` subagent events to block | +| `src/pythinker_code/ui/shell/visualize/_blocks.py` | New fields + methods on `_ToolCallBlock`; update `_compose()` to render running rows + output preview | + +No changes to wire types, event bus, tool renderers, or activity tree. + +## Out of Scope + +- Card-style (`is_card_style()`) rendering — follow-up +- Multi-level nested subagents (agent spawning agent) — existing TODO in `_live_view.py` +- Showing subagent thinking/content blocks in the parent card From c1419b66259cd424a8ca8253344fb05eec0b8ca1 Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Tue, 26 May 2026 23:28:57 -0400 Subject: [PATCH 04/13] docs: add agent live tool stream implementation plan --- .../2026-05-26-agent-live-tool-stream.md | 616 ++++++++++++++++++ 1 file changed, 616 insertions(+) create mode 100644 docs/superpowers/plans/2026-05-26-agent-live-tool-stream.md diff --git a/docs/superpowers/plans/2026-05-26-agent-live-tool-stream.md b/docs/superpowers/plans/2026-05-26-agent-live-tool-stream.md new file mode 100644 index 00000000..ae9a455f --- /dev/null +++ b/docs/superpowers/plans/2026-05-26-agent-live-tool-stream.md @@ -0,0 +1,616 @@ +# Agent Live Tool Stream Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Show in-flight subagent tool calls as shimmering live rows with streamed output inside the parent Agent card while the agent is running. + +**Architecture:** Two surgical changes: (1) add three new fields and two new methods to `_ToolCallBlock` in `_blocks.py` so it can accumulate sub-tool output; (2) update `_compose()` to render ongoing calls as shimmer rows with output preview; (3) wire `ToolExecutionStarted` and `ToolOutputPart` subagent events through `_live_view.py` to the block. + +**Tech Stack:** Python, Rich (renderables, Text, Group), existing `ActivityRow`/`render_activity_tree` and `_tail_lines`/`_truncate_to_display_width` helpers already in `_blocks.py`. + +--- + +## File Map + +| File | Change | +|---|---| +| `src/pythinker_code/ui/shell/visualize/_blocks.py` | New constants, fields, methods on `_ToolCallBlock`; updated `_compose()` | +| `src/pythinker_code/ui/shell/visualize/_live_view.py` | Split `ToolExecutionStarted \| ToolOutputPart` case in `handle_subagent_event` | +| `tests/ui_and_conv/test_tool_call_block.py` | New unit tests for new methods and running-row rendering | +| `tests/ui_and_conv/test_subagent_live_stream.py` | New integration tests for end-to-end event dispatch | + +--- + +### Task 1: State fields and cleanup + +**Files:** +- Modify: `src/pythinker_code/ui/shell/visualize/_blocks.py` +- Test: `tests/ui_and_conv/test_tool_call_block.py` + +- [ ] **Step 1: Write failing tests for new fields and methods** + +Add to `tests/ui_and_conv/test_tool_call_block.py`: + +```python +def test_append_sub_output_part_accumulates_text(): + block = _ToolCallBlock(_tool_call("Agent", '{"description":"scan"}')) + call = _tool_call_with_id("sub-1", "Bash", '{"command":"ls"}') + block.append_sub_tool_call(call) + block.append_sub_output_part("sub-1", "file1.py\n") + block.append_sub_output_part("sub-1", "file2.py\n") + combined = "".join(block._subagent_output_parts["sub-1"]) + assert "file1.py" in combined + assert "file2.py" in combined + + +def test_append_sub_output_part_discards_unknown_call_id(): + block = _ToolCallBlock(_tool_call("Agent", '{"description":"scan"}')) + # no append_sub_tool_call — id is unknown + block.append_sub_output_part("ghost-id", "should be ignored\n") + assert "ghost-id" not in block._subagent_output_parts + + +def test_append_sub_output_part_caps_buffer_at_200_chars(): + block = _ToolCallBlock(_tool_call("Agent", '{"description":"scan"}')) + call = _tool_call_with_id("sub-1", "Bash", '{"command":"find ."}') + block.append_sub_tool_call(call) + # Fill with >200 chars in one shot + block.append_sub_output_part("sub-1", "x" * 300) + combined = "".join(block._subagent_output_parts["sub-1"]) + assert len(combined) <= 200 + + +def test_append_sub_output_part_tracks_stderr(): + block = _ToolCallBlock(_tool_call("Agent", '{"description":"scan"}')) + call = _tool_call_with_id("sub-1", "Bash", '{"command":"cat missing"}') + block.append_sub_tool_call(call) + block.append_sub_output_part("sub-1", "No such file\n", stream="stderr") + assert block._subagent_output_had_stderr.get("sub-1") is True + + +def test_mark_sub_execution_started_records_id(): + block = _ToolCallBlock(_tool_call("Agent", '{"description":"scan"}')) + call = _tool_call_with_id("sub-1", "Bash", '{"command":"ls"}') + block.append_sub_tool_call(call) + block.mark_sub_execution_started("sub-1") + assert "sub-1" in block._subagent_execution_started + + +def test_mark_sub_execution_started_discards_unknown_id(): + block = _ToolCallBlock(_tool_call("Agent", '{"description":"scan"}')) + block.mark_sub_execution_started("ghost-id") # should not raise + assert "ghost-id" not in block._subagent_execution_started + + +def test_finish_sub_tool_call_cleans_up_output_state(): + from pythinker_code.wire.types import ToolResult + from pythinker_core.tooling import ToolOk + + block = _ToolCallBlock(_tool_call("Agent", '{"description":"scan"}')) + call = _tool_call_with_id("sub-1", "Bash", '{"command":"ls"}') + block.append_sub_tool_call(call) + block.append_sub_output_part("sub-1", "output\n") + block.mark_sub_execution_started("sub-1") + block.finish_sub_tool_call(ToolResult(tool_call_id="sub-1", return_value=ToolOk(output=""))) + assert "sub-1" not in block._subagent_output_parts + assert "sub-1" not in block._subagent_output_had_stderr + assert "sub-1" not in block._subagent_execution_started +``` + +- [ ] **Step 2: Run tests to verify they fail** + +```bash +cd /home/ai/Projects/pythinker-code-main +python -m pytest tests/ui_and_conv/test_tool_call_block.py::test_append_sub_output_part_accumulates_text tests/ui_and_conv/test_tool_call_block.py::test_mark_sub_execution_started_records_id tests/ui_and_conv/test_tool_call_block.py::test_finish_sub_tool_call_cleans_up_output_state -v 2>&1 | tail -20 +``` + +Expected: `AttributeError` — `_ToolCallBlock` has no `_subagent_output_parts`. + +- [ ] **Step 3: Add constants and new fields to `_ToolCallBlock`** + +In `src/pythinker_code/ui/shell/visualize/_blocks.py`, add two constants near the top (after the existing `MAX_SUBAGENT_TOOL_CALLS_TO_SHOW = 4` line): + +```python +_MAX_RUNNING_ROWS = 2 +_MAX_SUB_OUTPUT_CHARS = 200 +``` + +In `_ToolCallBlock.__init__`, after the `self._is_background_pending: bool = False` line, add: + +```python +self._subagent_output_parts: dict[str, list[str]] = {} +self._subagent_output_had_stderr: dict[str, bool] = {} +self._subagent_execution_started: set[str] = set() +``` + +- [ ] **Step 4: Add `mark_sub_execution_started` method** + +Add after the existing `set_subagent_metadata` method (~line 506): + +```python +def mark_sub_execution_started(self, tool_call_id: str) -> None: + if tool_call_id not in self._ongoing_subagent_tool_calls: + return + self._subagent_execution_started.add(tool_call_id) + self._renderable = self._compose() +``` + +- [ ] **Step 5: Add `append_sub_output_part` method** + +Add directly after `mark_sub_execution_started`: + +```python +def append_sub_output_part( + self, tool_call_id: str, text: str, *, stream: str = "output" +) -> None: + if tool_call_id not in self._ongoing_subagent_tool_calls: + return + parts = self._subagent_output_parts.setdefault(tool_call_id, []) + parts.append(text) + if stream == "stderr": + self._subagent_output_had_stderr[tool_call_id] = True + combined = "".join(parts) + if len(combined) > _MAX_SUB_OUTPUT_CHARS: + self._subagent_output_parts[tool_call_id] = [combined[-_MAX_SUB_OUTPUT_CHARS:]] + self._renderable = self._compose() +``` + +- [ ] **Step 6: Update `finish_sub_tool_call` to clean up new state** + +In the existing `finish_sub_tool_call` method, add three cleanup lines right after `self._last_subagent_tool_call = None`: + +```python +def finish_sub_tool_call(self, tool_result: ToolResult): + self._last_subagent_tool_call = None + self._subagent_output_parts.pop(tool_result.tool_call_id, None) # NEW + self._subagent_output_had_stderr.pop(tool_result.tool_call_id, None) # NEW + self._subagent_execution_started.discard(tool_result.tool_call_id) # NEW + sub_tool_call = self._ongoing_subagent_tool_calls.pop(tool_result.tool_call_id, None) + if sub_tool_call is None: + return + self._finished_subagent_tool_calls.append( + _ToolCallBlock.FinishedSubCall( + call=sub_tool_call, + result=tool_result.return_value, + ) + ) + self._n_finished_subagent_tool_calls += 1 + self._renderable = self._compose() +``` + +- [ ] **Step 7: Run tests to verify they pass** + +```bash +cd /home/ai/Projects/pythinker-code-main +python -m pytest tests/ui_and_conv/test_tool_call_block.py -v 2>&1 | tail -25 +``` + +Expected: all tests pass including all 7 new ones. + +- [ ] **Step 8: Commit** + +```bash +git add src/pythinker_code/ui/shell/visualize/_blocks.py tests/ui_and_conv/test_tool_call_block.py +git commit -m "feat(blocks): add subagent output tracking state and methods to _ToolCallBlock" +``` + +--- + +### Task 2: Render running rows and output preview in `_compose()` + +**Files:** +- Modify: `src/pythinker_code/ui/shell/visualize/_blocks.py` +- Test: `tests/ui_and_conv/test_tool_call_block.py` + +- [ ] **Step 1: Write failing tests for running-row rendering** + +Add to `tests/ui_and_conv/test_tool_call_block.py`: + +```python +def test_running_agent_shows_ongoing_sub_tool_calls(): + block = _ToolCallBlock(_tool_call("Agent", '{"description":"scan"}')) + call = _tool_call_with_id("sub-1", "Read", '{"file_path":"src/app.py"}') + block.append_sub_tool_call(call) + output = _plain(block.compose()) + assert "Read" in output + assert "src/app.py" in output + + +def test_running_agent_shows_streamed_output_preview(): + block = _ToolCallBlock(_tool_call("Agent", '{"description":"scan"}')) + call = _tool_call_with_id("sub-1", "Bash", '{"command":"grep -r TODO ."}') + block.append_sub_tool_call(call) + block.append_sub_output_part("sub-1", "src/app.py:42: # TODO: fix\n") + output = _plain(block.compose()) + assert "src/app.py:42" in output + + +def test_running_agent_shows_only_last_4_output_lines(): + block = _ToolCallBlock(_tool_call("Agent", '{"description":"scan"}')) + call = _tool_call_with_id("sub-1", "Bash", '{"command":"find ."}') + block.append_sub_tool_call(call) + lines = [f"line{i}\n" for i in range(10)] + block.append_sub_output_part("sub-1", "".join(lines)) + output = _plain(block.compose()) + # Only last 4 lines should appear + assert "line9" in output + assert "line6" in output + assert "line5" not in output + assert "line0" not in output + + +def test_running_agent_caps_visible_running_rows_at_2(): + block = _ToolCallBlock(_tool_call("Agent", '{"description":"scan"}')) + for i in range(5): + call = _tool_call_with_id(f"sub-{i}", "Read", f'{{"file_path":"src/file{i}.py"}}') + block.append_sub_tool_call(call) + output = _plain(block.compose()) + # "… N more running" indicator must appear + assert "more running" in output + + +def test_finished_sub_tool_calls_not_shown_in_output_preview(): + from pythinker_code.wire.types import ToolResult + from pythinker_core.tooling import ToolOk + + block = _ToolCallBlock(_tool_call("Agent", '{"description":"scan"}')) + call = _tool_call_with_id("sub-1", "Bash", '{"command":"ls"}') + block.append_sub_tool_call(call) + block.append_sub_output_part("sub-1", "SHOULD_NOT_APPEAR\n") + block.finish_sub_tool_call(ToolResult(tool_call_id="sub-1", return_value=ToolOk(output=""))) + output = _plain(block.compose()) + assert "SHOULD_NOT_APPEAR" not in output +``` + +- [ ] **Step 2: Run tests to verify they fail** + +```bash +cd /home/ai/Projects/pythinker-code-main +python -m pytest tests/ui_and_conv/test_tool_call_block.py::test_running_agent_shows_ongoing_sub_tool_calls tests/ui_and_conv/test_tool_call_block.py::test_running_agent_shows_streamed_output_preview -v 2>&1 | tail -20 +``` + +Expected: FAIL — ongoing sub-tool calls are not yet rendered. + +- [ ] **Step 3: Update `_compose()` to render running rows and output preview** + +In `src/pythinker_code/ui/shell/visualize/_blocks.py`, find the block starting with: +```python +if not (style.label == "Subagent" and self._result is not None): + rows: list[ActivityRow] = [] + for sub_call, sub_result in self._finished_subagent_tool_calls: + ... + if rows: + children.append(render_activity_tree(rows, width=current_console_width())) +``` + +Replace the entire `if not (style.label == "Subagent" and self._result is not None):` block with: + +```python +if not (style.label == "Subagent" and self._result is not None): + # Finished sub-tool call rows + rows: list[ActivityRow] = [] + for sub_call, sub_result in self._finished_subagent_tool_calls: + argument = extract_key_argument( + sub_call.function.arguments or "", sub_call.function.name + ) + detail = sub_call.function.name + if argument: + detail = f"{detail} {argument}" + rows.append( + ActivityRow( + label="agent", + detail=detail, + state="failed" if sub_result.is_error else "completed", + ) + ) + + # Running sub-tool call rows (shown above finished rows) + ongoing = list(self._ongoing_subagent_tool_calls.values()) + n_hidden_running = max(0, len(ongoing) - _MAX_RUNNING_ROWS) + visible_running = ongoing[-_MAX_RUNNING_ROWS:] + running_rows: list[ActivityRow] = [] + for call in visible_running: + argument = extract_key_argument( + call.function.arguments or "", call.function.name + ) + detail = call.function.name + if argument: + detail = f"{detail} {argument}" + running_rows.append(ActivityRow(label="agent", detail=detail, state="running")) + + if n_hidden_running: + children.append(fg("muted", f"… {n_hidden_running} more running")) + + combined_rows = running_rows + rows + if combined_rows: + children.append(render_activity_tree(combined_rows, width=current_console_width())) + + # Output preview for the most-recent ongoing call that has streamed output + latest = self._last_subagent_tool_call + if latest is not None and latest.id in self._subagent_output_parts: + combined_output = "".join(self._subagent_output_parts[latest.id]).rstrip("\n") + if combined_output: + is_stderr = self._subagent_output_had_stderr.get(latest.id, False) + output_style = "error" if is_stderr else "muted" + preview = _tail_lines(combined_output, 4) + max_line_width = max(1, current_console_width() - 6) + for line in preview.splitlines(): + truncated = _truncate_to_display_width(line, max_line_width) + children.append(fg(output_style, f"│ {truncated}")) +``` + +- [ ] **Step 4: Run all tool_call_block tests** + +```bash +cd /home/ai/Projects/pythinker-code-main +python -m pytest tests/ui_and_conv/test_tool_call_block.py -v 2>&1 | tail -30 +``` + +Expected: all tests pass. + +- [ ] **Step 5: Run the full UI test suite to check for regressions** + +```bash +cd /home/ai/Projects/pythinker-code-main +python -m pytest tests/ui_and_conv/ -v 2>&1 | tail -40 +``` + +Expected: all pass. If any snapshot tests fail due to changed rendering, update them with `pytest --snapshot-update` — but review the diff first to make sure the new output is correct. + +- [ ] **Step 6: Commit** + +```bash +git add src/pythinker_code/ui/shell/visualize/_blocks.py tests/ui_and_conv/test_tool_call_block.py +git commit -m "feat(blocks): render running subagent tool calls and output preview in Agent card" +``` + +--- + +### Task 3: Wire events through `_live_view.py` + +**Files:** +- Modify: `src/pythinker_code/ui/shell/visualize/_live_view.py` +- Test: `tests/ui_and_conv/test_subagent_live_stream.py` (new file) + +- [ ] **Step 1: Write failing integration tests** + +Create `tests/ui_and_conv/test_subagent_live_stream.py`: + +```python +"""Integration tests for subagent ToolOutputPart and ToolExecutionStarted wiring.""" + +from __future__ import annotations + +from pythinker_core.message import ToolCall +from pythinker_core.tooling import ToolOk +from rich.console import Console + +from pythinker_code.ui.shell.visualize import _LiveView +from pythinker_code.wire.types import ( + StatusUpdate, + SubagentEvent, + ToolCall as WireToolCall, + ToolExecutionStarted, + ToolOutputPart, + ToolResult, + TurnBegin, +) + + +def _render(view: _LiveView, *, width: int = 100) -> str: + console = Console(width=width, record=True, highlight=False, color_system=None) + console.print(view.compose()) + return console.export_text() + + +def _agent_call(call_id: str = "agent-1") -> WireToolCall: + return WireToolCall( + id=call_id, + function=WireToolCall.FunctionBody( + name="Agent", + arguments='{"description":"security scan","subagent_type":"security-reviewer","prompt":"check it"}', + ), + ) + + +def _sub_tool_call(sub_id: str, name: str, args: str) -> ToolCall: + return ToolCall( + id=sub_id, + function=ToolCall.FunctionBody(name=name, arguments=args), + ) + + +def test_subagent_tool_output_part_appears_in_live_view(): + view = _LiveView(StatusUpdate(context_tokens=1000)) + view.dispatch_wire_message(TurnBegin(user_input="scan")) + view.dispatch_wire_message(_agent_call()) + + sub_call = _sub_tool_call("sub-1", "Bash", '{"command":"grep -r TODO ."}') + view.dispatch_wire_message( + SubagentEvent( + parent_tool_call_id="agent-1", + agent_id="a1", + subagent_type="security-reviewer", + event=sub_call, + ) + ) + view.dispatch_wire_message( + SubagentEvent( + parent_tool_call_id="agent-1", + agent_id="a1", + subagent_type="security-reviewer", + event=ToolOutputPart(tool_call_id="sub-1", text="src/app.py:42: # TODO\n"), + ) + ) + + output = _render(view) + assert "src/app.py:42" in output + + +def test_subagent_tool_execution_started_tracked(): + view = _LiveView(StatusUpdate(context_tokens=1000)) + view.dispatch_wire_message(TurnBegin(user_input="scan")) + view.dispatch_wire_message(_agent_call()) + + sub_call = _sub_tool_call("sub-1", "Read", '{"file_path":"src/app.py"}') + view.dispatch_wire_message( + SubagentEvent( + parent_tool_call_id="agent-1", + agent_id="a1", + subagent_type="security-reviewer", + event=sub_call, + ) + ) + view.dispatch_wire_message( + SubagentEvent( + parent_tool_call_id="agent-1", + agent_id="a1", + subagent_type="security-reviewer", + event=ToolExecutionStarted(tool_call_id="sub-1"), + ) + ) + + block = view._tool_call_blocks["agent-1"] + assert "sub-1" in block._subagent_execution_started + + +def test_output_part_for_unknown_parent_is_silently_ignored(): + view = _LiveView(StatusUpdate(context_tokens=1000)) + view.dispatch_wire_message(TurnBegin(user_input="scan")) + # No agent tool call dispatched — parent_tool_call_id won't resolve + view.dispatch_wire_message( + SubagentEvent( + parent_tool_call_id="nonexistent-agent", + agent_id="a1", + subagent_type="security-reviewer", + event=ToolOutputPart(tool_call_id="sub-1", text="should be ignored\n"), + ) + ) + # Must not raise; compose must still work + output = _render(view) + assert "should be ignored" not in output + + +def test_output_cleared_after_sub_tool_call_finishes(): + view = _LiveView(StatusUpdate(context_tokens=1000)) + view.dispatch_wire_message(TurnBegin(user_input="scan")) + view.dispatch_wire_message(_agent_call()) + + sub_call = _sub_tool_call("sub-1", "Bash", '{"command":"ls"}') + view.dispatch_wire_message( + SubagentEvent( + parent_tool_call_id="agent-1", + agent_id="a1", + subagent_type="security-reviewer", + event=sub_call, + ) + ) + view.dispatch_wire_message( + SubagentEvent( + parent_tool_call_id="agent-1", + agent_id="a1", + subagent_type="security-reviewer", + event=ToolOutputPart(tool_call_id="sub-1", text="SHOULD_DISAPPEAR\n"), + ) + ) + view.dispatch_wire_message( + SubagentEvent( + parent_tool_call_id="agent-1", + agent_id="a1", + subagent_type="security-reviewer", + event=ToolResult(tool_call_id="sub-1", return_value=ToolOk(output="")), + ) + ) + + output = _render(view) + assert "SHOULD_DISAPPEAR" not in output +``` + +- [ ] **Step 2: Run tests to verify they fail** + +```bash +cd /home/ai/Projects/pythinker-code-main +python -m pytest tests/ui_and_conv/test_subagent_live_stream.py -v 2>&1 | tail -20 +``` + +Expected: `test_subagent_tool_output_part_appears_in_live_view` FAILS because the output text is never forwarded. + +- [ ] **Step 3: Update `handle_subagent_event` in `_live_view.py`** + +In `src/pythinker_code/ui/shell/visualize/_live_view.py`, find the `handle_subagent_event` method. Locate the match arm: + +```python +case ToolExecutionStarted() | ToolOutputPart(): + # Nested subagent execution/output streaming is intentionally + # summarized at the parent Agent-card level for now. + self.refresh_soon() +``` + +Replace it with two separate arms: + +```python +case ToolExecutionStarted() as started: + block.mark_sub_execution_started(started.tool_call_id) + self.refresh_soon() +case ToolOutputPart() as output_part: + block.append_sub_output_part( + output_part.tool_call_id, + output_part.text, + stream=output_part.stream, + ) + self.refresh_soon() +``` + +- [ ] **Step 4: Run new integration tests** + +```bash +cd /home/ai/Projects/pythinker-code-main +python -m pytest tests/ui_and_conv/test_subagent_live_stream.py -v 2>&1 | tail -20 +``` + +Expected: all 4 tests pass. + +- [ ] **Step 5: Run the full test suite** + +```bash +cd /home/ai/Projects/pythinker-code-main +python -m pytest tests/ -x -q 2>&1 | tail -30 +``` + +Expected: all pass. If snapshot tests diverge, inspect the diffs and update snapshots only if the new output is correct. + +- [ ] **Step 6: Commit** + +```bash +git add src/pythinker_code/ui/shell/visualize/_live_view.py tests/ui_and_conv/test_subagent_live_stream.py +git commit -m "feat(live-view): stream subagent ToolOutputPart and ToolExecutionStarted into Agent card" +``` + +--- + +## Self-Review + +**Spec coverage check:** + +| Spec requirement | Task covering it | +|---|---| +| §1: Forward `ToolExecutionStarted` and `ToolOutputPart` via new block methods | Task 3 | +| §2: `_subagent_output_parts`, `_subagent_output_had_stderr`, `_subagent_execution_started` fields | Task 1 | +| §2: `mark_sub_execution_started`, `append_sub_output_part` methods | Task 1 | +| §2: `finish_sub_tool_call` cleans up new fields | Task 1 | +| §3: Running rows rendered above finished rows with shimmer | Task 2 | +| §3: Output preview — last 4 lines, `│ ` prefix, muted/error style | Task 2 | +| §3: Cap at 2 running rows, show `… N more running` | Task 2 | +| §3: Output buffer discarded on finish | Task 1 (method) + Task 2 (test) | +| §4: Partial args → show bare tool name | Covered by `extract_key_argument` returning `None` — Task 2 test indirectly | +| §4: `ToolOutputPart` for unknown call discarded | Task 3 test `test_output_part_for_unknown_parent_is_silently_ignored` | +| §4: Buffer capped at 200 chars | Task 1 test `test_append_sub_output_part_caps_buffer_at_200_chars` | +| §4: Card style unchanged | `_compose_card()` not touched — confirmed | + +**Placeholder scan:** No TBDs, TODOs, or "similar to" references found. + +**Type consistency:** +- `mark_sub_execution_started(tool_call_id: str)` — used by name in Task 3 wiring ✓ +- `append_sub_output_part(tool_call_id, text, *, stream)` — used by name in Task 3 wiring ✓ +- `_subagent_execution_started: set[str]` — checked in Task 3 test ✓ +- `_MAX_RUNNING_ROWS = 2`, `_MAX_SUB_OUTPUT_CHARS = 200` — defined in Task 1 §3, used in Task 2 §3 ✓ From f68f05037b1733a81fb0c77aad4a6433f68b4a39 Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Tue, 26 May 2026 23:45:02 -0400 Subject: [PATCH 05/13] feat(blocks): add subagent output tracking state and methods to _ToolCallBlock Add three new state fields (_subagent_output_parts, _subagent_output_had_stderr, _subagent_execution_started), two new public methods (mark_sub_execution_started, append_sub_output_part), and cleanup in finish_sub_tool_call for the new fields. Also update append_sub_tool_call and append_sub_tool_call_part to recompose after mutations, readying the block for live subagent tool streaming in Task 2. --- .../ui/shell/visualize/_blocks.py | 30 +++++++++ tests/ui_and_conv/test_tool_call_block.py | 65 +++++++++++++++++++ 2 files changed, 95 insertions(+) diff --git a/src/pythinker_code/ui/shell/visualize/_blocks.py b/src/pythinker_code/ui/shell/visualize/_blocks.py index 64246bd4..396e7f23 100644 --- a/src/pythinker_code/ui/shell/visualize/_blocks.py +++ b/src/pythinker_code/ui/shell/visualize/_blocks.py @@ -68,6 +68,8 @@ _THINKING_PREVIEW_LINES = 6 _COMPOSING_PREVIEW_LINES = 12 MAX_SUBAGENT_TOOL_CALLS_TO_SHOW = 4 +_MAX_RUNNING_ROWS = 2 +_MAX_SUB_OUTPUT_CHARS = 200 # Background-agent statuses that mean "still running" — the tool call result # has arrived but the spawned agent has not yet finished. Blocks with this @@ -389,6 +391,9 @@ def __init__(self, tool_call: ToolCall): # rather than being flushed to static scrollback, so the spinner keeps # animating at the Live refresh rate. self._is_background_pending: bool = False + self._subagent_output_parts: dict[str, list[str]] = {} + self._subagent_output_had_stderr: dict[str, bool] = {} + self._subagent_execution_started: set[str] = set() self._renderable: RenderableType = self._compose() @@ -472,6 +477,7 @@ def finish(self, result: ToolReturnValue): def append_sub_tool_call(self, tool_call: ToolCall): self._ongoing_subagent_tool_calls[tool_call.id] = tool_call self._last_subagent_tool_call = tool_call + self._renderable = self._compose() def append_sub_tool_call_part(self, tool_call_part: ToolCallPart): if self._last_subagent_tool_call is None: @@ -482,12 +488,16 @@ def append_sub_tool_call_part(self, tool_call_part: ToolCallPart): self._last_subagent_tool_call.function.arguments = tool_call_part.arguments_part else: self._last_subagent_tool_call.function.arguments += tool_call_part.arguments_part + self._renderable = self._compose() def finish_sub_tool_call(self, tool_result: ToolResult): self._last_subagent_tool_call = None sub_tool_call = self._ongoing_subagent_tool_calls.pop(tool_result.tool_call_id, None) if sub_tool_call is None: return + self._subagent_output_parts.pop(tool_result.tool_call_id, None) + self._subagent_output_had_stderr.pop(tool_result.tool_call_id, None) + self._subagent_execution_started.discard(tool_result.tool_call_id) self._finished_subagent_tool_calls.append( _ToolCallBlock.FinishedSubCall( @@ -505,6 +515,26 @@ def set_subagent_metadata(self, agent_id: str, subagent_type: str) -> None: if changed: self._renderable = self._compose() + def mark_sub_execution_started(self, tool_call_id: str) -> None: + if tool_call_id not in self._ongoing_subagent_tool_calls: + return + self._subagent_execution_started.add(tool_call_id) + self._renderable = self._compose() + + def append_sub_output_part( + self, tool_call_id: str, text: str, *, stream: str = "output" + ) -> None: + if tool_call_id not in self._ongoing_subagent_tool_calls: + return + parts = self._subagent_output_parts.setdefault(tool_call_id, []) + parts.append(text) + if stream == "stderr": + self._subagent_output_had_stderr[tool_call_id] = True + combined = "".join(parts) + if len(combined) > _MAX_SUB_OUTPUT_CHARS: + self._subagent_output_parts[tool_call_id] = [combined[-_MAX_SUB_OUTPUT_CHARS:]] + self._renderable = self._compose() + def _compose(self) -> RenderableType: if is_card_style(): card_rendered = self._compose_card() diff --git a/tests/ui_and_conv/test_tool_call_block.py b/tests/ui_and_conv/test_tool_call_block.py index cadae0b8..a363f10c 100644 --- a/tests/ui_and_conv/test_tool_call_block.py +++ b/tests/ui_and_conv/test_tool_call_block.py @@ -169,3 +169,68 @@ def test_completed_subagent_renders_compact_summary(): assert "completed" in output.lower() assert "7 tool calls" in output assert output.count("ReadFile") <= 4 + + +def test_append_sub_output_part_accumulates_text(): + block = _ToolCallBlock(_tool_call("Agent", '{"description":"scan"}')) + call = _tool_call_with_id("sub-1", "Bash", '{"command":"ls"}') + block.append_sub_tool_call(call) + block.append_sub_output_part("sub-1", "file1.py\n") + block.append_sub_output_part("sub-1", "file2.py\n") + combined = "".join(block._subagent_output_parts["sub-1"]) + assert "file1.py" in combined + assert "file2.py" in combined + + +def test_append_sub_output_part_discards_unknown_call_id(): + block = _ToolCallBlock(_tool_call("Agent", '{"description":"scan"}')) + # no append_sub_tool_call — id is unknown + block.append_sub_output_part("ghost-id", "should be ignored\n") + assert "ghost-id" not in block._subagent_output_parts + + +def test_append_sub_output_part_caps_buffer_at_200_chars(): + block = _ToolCallBlock(_tool_call("Agent", '{"description":"scan"}')) + call = _tool_call_with_id("sub-1", "Bash", '{"command":"find ."}') + block.append_sub_tool_call(call) + # Fill with >200 chars in one shot + block.append_sub_output_part("sub-1", "x" * 300) + combined = "".join(block._subagent_output_parts["sub-1"]) + assert len(combined) <= 200 + + +def test_append_sub_output_part_tracks_stderr(): + block = _ToolCallBlock(_tool_call("Agent", '{"description":"scan"}')) + call = _tool_call_with_id("sub-1", "Bash", '{"command":"cat missing"}') + block.append_sub_tool_call(call) + block.append_sub_output_part("sub-1", "No such file\n", stream="stderr") + assert block._subagent_output_had_stderr.get("sub-1") is True + + +def test_mark_sub_execution_started_records_id(): + block = _ToolCallBlock(_tool_call("Agent", '{"description":"scan"}')) + call = _tool_call_with_id("sub-1", "Bash", '{"command":"ls"}') + block.append_sub_tool_call(call) + block.mark_sub_execution_started("sub-1") + assert "sub-1" in block._subagent_execution_started + + +def test_mark_sub_execution_started_discards_unknown_id(): + block = _ToolCallBlock(_tool_call("Agent", '{"description":"scan"}')) + block.mark_sub_execution_started("ghost-id") # should not raise + assert "ghost-id" not in block._subagent_execution_started + + +def test_finish_sub_tool_call_cleans_up_output_state(): + from pythinker_code.wire.types import ToolResult + from pythinker_core.tooling import ToolOk + + block = _ToolCallBlock(_tool_call("Agent", '{"description":"scan"}')) + call = _tool_call_with_id("sub-1", "Bash", '{"command":"ls"}') + block.append_sub_tool_call(call) + block.append_sub_output_part("sub-1", "output\n") + block.mark_sub_execution_started("sub-1") + block.finish_sub_tool_call(ToolResult(tool_call_id="sub-1", return_value=ToolOk(output=""))) + assert "sub-1" not in block._subagent_output_parts + assert "sub-1" not in block._subagent_output_had_stderr + assert "sub-1" not in block._subagent_execution_started From fd13a8832e78cf8e0e5f1d8651db455d11f4353d Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Tue, 26 May 2026 23:48:14 -0400 Subject: [PATCH 06/13] test: sync default agent-spec snapshot with explore.yaml lint policy The committed explore.yaml carries lint/complexity-policy guidance (do not flag rules such as C901 that are absent from the project's configured rule set) that its inline snapshot in test_agent_spec.py never received, so the test failed on a clean checkout. Regenerate the snapshot to match HEAD. --- tests/core/test_agent_spec.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/core/test_agent_spec.py b/tests/core/test_agent_spec.py index 00718522..235e5d7d 100644 --- a/tests/core/test_agent_spec.py +++ b/tests/core/test_agent_spec.py @@ -211,6 +211,7 @@ def test_load_default_agent_spec(): - NEVER use Shell for any file creation or modification commands - Adapt your search depth based on the thoroughness level specified by the caller - Wherever possible, spawn multiple parallel tool calls for grepping and reading files to maximize speed +- When running lint or complexity checks (e.g. ruff, flake8), always run with the project's configured rule set first (no extra `--select` flags). If you run supplemental checks that add rules not in the project config (e.g. `--select C901` when C901 is absent from pyproject.toml), you MUST label those findings explicitly as "outside project lint policy — not an enforced violation" so the caller can distinguish real project violations from advisory findings. If the prompt includes a block, use it to orient yourself about the repository state before starting your investigation. @@ -305,6 +306,7 @@ def test_load_default_agent_spec(): - Before designing a plan, build a context packet from repository evidence, docs, tests, existing patterns, and the user's stated goal. - If the relevant codebase area is not understood, do not invent a plan. Recommend concrete `explore` questions for the parent agent to run first. - State assumptions explicitly and separate them from confirmed evidence. +- Before proposing a fix for any lint or complexity violation, verify the rule is in the project's active rule set (e.g. `select` in pyproject.toml or .ruff.toml). Findings that only appear via an explicit `--select ` flag not present in the project config are NOT project violations; do not include them in the plan unless the user explicitly asked to enforce that rule. Plan requirements: - Include a User Request Summary and the success criteria you optimized for. From 14e3c961e10da22c653bba18d6553bd06c0d342a Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Tue, 26 May 2026 23:48:14 -0400 Subject: [PATCH 07/13] fix(tui): refine markdown report rendering - Frame fenced code blocks with a blank row above and below so they read as a distinct section instead of crowding surrounding prose. - Add a conservative pre-parse normalizer that repairs malformed GFM tables the model sometimes emits (header glued to prose, a blank line before the |---| delimiter, data rows crammed onto the delimiter line) which markdown-it would otherwise render as raw text. Anchored on the delimiter row, it rebuilds only regions whose header and data cell counts validate, preserves column alignment, and leaves ambiguous or fenced content untouched. --- .../ui/shell/components/markdown.py | 166 +++++++++++++++++- tests/ui/test_shell_markdown.py | 33 ++++ 2 files changed, 198 insertions(+), 1 deletion(-) diff --git a/src/pythinker_code/ui/shell/components/markdown.py b/src/pythinker_code/ui/shell/components/markdown.py index b7cb7f29..3253a373 100644 --- a/src/pythinker_code/ui/shell/components/markdown.py +++ b/src/pythinker_code/ui/shell/components/markdown.py @@ -67,6 +67,11 @@ sorted(_MARKDOWN_ICON_REPLACEMENTS, key=len, reverse=True) ) _FENCE_RE = re.compile(r"^(?P {0,3})(?P`{3,}|~{3,})") +# A GFM table delimiter run, e.g. ``|---|:--:|---|``. Two or more dashes per +# cell keeps stray inline ``|-|`` out of the match. +_DELIM_RUN_RE = re.compile(r"\|(?:\s*:?-{2,}:?\s*\|)+") +# A header line: optional prose prefix, then a trailing run of pipe cells. +_HEADER_RE = re.compile(r"^(?P.*?)(?P(?:\|[^\n|]*)+\|)\s*$") __all__ = [ @@ -210,6 +215,165 @@ def _simplify_markdown_report_icons(markup: str) -> str: return "".join(lines) +def _split_pipe_cells(segment: str) -> list[str]: + """Split a ``| a | b |`` run into stripped inner cells (drops the frame).""" + parts = re.split(r"(? bool: + stripped = line.strip() + return stripped.startswith("|") and stripped.count("|") >= 2 + + +def _delimiter_markers(run: str) -> list[str]: + """Return per-column alignment markers (``---``, ``:---``, ``---:``, ``:---:``).""" + markers: list[str] = [] + for cell in _split_pipe_cells(run): + left = cell.startswith(":") + right = cell.endswith(":") + if left and right: + markers.append(":---:") + elif right: + markers.append("---:") + elif left: + markers.append(":---") + else: + markers.append("---") + return markers + + +def _normalize_table_block(text: str) -> str: + """Repair malformed GFM tables in a fence-free block of markdown. + + Models occasionally glue a table header onto preceding prose, drop the + newline between the header and the ``|---|`` delimiter, or cram data rows + onto the delimiter line — markdown-it then renders the whole thing as raw + text. Anchored on the delimiter run, this rebuilds each region it is + *confident* is a table (delimiter at line start, header and data cell counts + both equal to the delimiter's column count) and passes everything else + through untouched. Well-formed tables are rebuilt to identical-rendering + markdown, so the pass is safe to apply unconditionally. + """ + out = "" + while True: + match = _DELIM_RUN_RE.search(text) + if match is None: + return out + text + markers = _delimiter_markers(match.group(0)) + n_cols = len(markers) + head = text[: match.start()] + tail = text[match.end() :] + + # The delimiter must start its own line — guards against inline ``|-|``. + line_prefix = head[head.rfind("\n") + 1 :] + if n_cols < 2 or line_prefix.strip() != "": + out += text[: match.end()] + text = tail + continue + + head_lines = head.split("\n") + while head_lines and head_lines[-1] == "": + head_lines.pop() + header_match = _HEADER_RE.match(head_lines[-1]) if head_lines else None + header_cells = _split_pipe_cells(header_match.group("cells")) if header_match else [] + if header_match is None or len(header_cells) != n_cols: + out += text[: match.end()] + text = tail + continue + + # Data rows: the same-line remainder after the delimiter plus any + # following pipe rows, re-chunked into rows of ``n_cols`` cells. + tail_lines = tail.split("\n") + data_segments = [tail_lines[0]] if tail_lines[0].strip() else [] + consumed = 1 + for line in tail_lines[1:]: + if _is_pipe_row(line): + data_segments.append(line) + consumed += 1 + else: + break + data_rows: list[list[str]] = [] + bail = False + for segment in data_segments: + cells = _split_pipe_cells(segment) + if not cells: + continue + if len(cells) % n_cols != 0: + bail = True # ambiguous (e.g. glued rows with empty cells) — leave as-is + break + for i in range(0, len(cells), n_cols): + data_rows.append(cells[i : i + n_cols]) + if bail: + out += text[: match.end()] + text = tail + continue + + preamble = head_lines[:-1] + prose = header_match.group("prefix").rstrip() + if preamble: + out += "\n".join(preamble) + "\n" + if prose: + out += prose + "\n" + # A GFM table must be preceded by a blank line (it cannot interrupt a + # paragraph), so ensure one before emitting the header. + if out and not out.endswith("\n\n"): + out += "\n" if out.endswith("\n") else "\n\n" + out += "| " + " | ".join(header_cells) + " |\n" + out += "| " + " | ".join(markers) + " |\n" + for row in data_rows: + out += "| " + " | ".join(row) + " |\n" + + remainder = "\n".join(tail_lines[consumed:]) + if not remainder.strip(): + return out + text = remainder if remainder.startswith("\n") else "\n" + remainder + + +def _normalize_markdown_tables(markup: str) -> str: + """Apply :func:`_normalize_table_block` to every fence-free span of markup.""" + if "|" not in markup or "-" not in markup: + return markup + + out: list[str] = [] + buffer: list[str] = [] + in_fence = False + fence_char = "" + fence_len = 0 + + def flush() -> None: + if buffer: + out.append(_normalize_table_block("\n".join(buffer))) + buffer.clear() + + for line in markup.splitlines(): + match = _FENCE_RE.match(line) + if in_fence: + fence = match.group("fence") if match else "" + if fence and fence[0] == fence_char and len(fence) >= fence_len: + in_fence = False + out.append(line) + continue + if match: + flush() + in_fence = True + fence_char = match.group("fence")[0] + fence_len = len(match.group("fence")) + out.append(line) + continue + buffer.append(line) + flush() + + result = "\n".join(out) + if markup.endswith("\n") and not result.endswith("\n"): + result += "\n" + return result + + class PythinkerMarkdown(Markdown): """Drop-in replacement for ``rich.markdown.Markdown`` with the Pythinker palette. @@ -222,7 +386,7 @@ class PythinkerMarkdown(Markdown): elements = {**Markdown.elements, "fence": _BorderedCodeBlock, "code_block": _BorderedCodeBlock} def __init__(self, markup: str, *args: Any, **kwargs: Any) -> None: - safe_markup = sanitize_ansi(markup) + safe_markup = _normalize_markdown_tables(sanitize_ansi(markup)) super().__init__(_simplify_markdown_report_icons(safe_markup), *args, **kwargs) def __rich_console__(self, console: Console, options: ConsoleOptions) -> RenderResult: diff --git a/tests/ui/test_shell_markdown.py b/tests/ui/test_shell_markdown.py index f5e5fdbd..85705d0c 100644 --- a/tests/ui/test_shell_markdown.py +++ b/tests/ui/test_shell_markdown.py @@ -37,6 +37,39 @@ def test_shell_markdown_uses_pythinker_code_block_frame() -> None: assert "╰" in output +def test_shell_markdown_repairs_glued_table_header() -> None: + # The model sometimes glues the header onto preceding prose and drops the + # newline before the |---| delimiter; markdown-it then renders it as raw + # text. The normalizer should rebuild a real table. + output = _render_text( + PythinkerMarkdown( + "● LOW — Various| Category | Issue | Locations |\n\n" + "|----------|-------|-----------| | Error handling | Bare except | 12 files |\n" + ) + ) + # Header text on its own line, no raw delimiter pipes left in the output. + assert "Category" in output and "Locations" in output + assert "Error handling" in output and "12 files" in output + assert "---" not in output + assert "|----------|" not in output + + +def test_shell_markdown_leaves_inline_pipes_alone() -> None: + # A stray inline |-| in prose must not be mistaken for a table delimiter. + text = "Use the `a | b` operator. See |--| inline here." + output = _render_text(PythinkerMarkdown(text)) + assert "operator" in output and "inline here" in output + + +def test_shell_markdown_keeps_table_like_pipes_in_code_fence() -> None: + output = _render_text( + PythinkerMarkdown("```\n| not | a | table |\n|-----|---|-------|\n```\n") + ) + # Inside a fence the pipes and delimiter survive verbatim. + assert "| not | a | table |" in output + assert "|-----|---|-------|" in output + + def test_shell_markdown_pads_code_block_with_blank_rows() -> None: # The code block should read as a distinct section, with a blank row framing # the panel above and below so it never crowds the surrounding prose. From 0f7e0499791f1d83dba992c23b9a7aeaf063e025 Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Tue, 26 May 2026 23:48:14 -0400 Subject: [PATCH 08/13] feat(agent): guide output formatting and anchor date awareness - Add an Output Formatting section to the default system prompt: emit well-formed Markdown tables, reserve code fences for actual code (never wrap prose reports, finding lists, or ASCII boxes in a fence), and use status icons sparingly. This stops the model from producing the malformed tables and emoji-laden boxes that rendered poorly in the TUI. - Reword the Date and Time section so the injected ${PYTHINKER_NOW} is framed as the authoritative present, anchoring the agent's sense of 'now', recency, and 'latest' to it instead of a training-era year. - Refresh the prompt snapshot and add coverage for both. --- src/pythinker_code/agents/default/system.md | 11 +++++- tests/core/test_default_agent.py | 11 +++++- tests/core/test_load_agent.py | 39 +++++++++++++++++++++ 3 files changed, 59 insertions(+), 2 deletions(-) diff --git a/src/pythinker_code/agents/default/system.md b/src/pythinker_code/agents/default/system.md index a3638a1e..70552f41 100644 --- a/src/pythinker_code/agents/default/system.md +++ b/src/pythinker_code/agents/default/system.md @@ -135,7 +135,7 @@ The operating environment is not in a sandbox. Any actions you do will immediate ## Date and Time -The current date and time in ISO format is `${PYTHINKER_NOW}`. This is only a reference for you when searching the web, or checking file modification time, etc. If you need the exact time, use Shell tool with proper command. +The current date and time in ISO format is `${PYTHINKER_NOW}`. Treat this as the authoritative present — it reflects the real "now", which is later than your training data suggests. Anchor all reasoning about the current date, the year, recency, and what counts as the "latest" version or release to `${PYTHINKER_NOW}`; do not fall back on an earlier year you might assume from training. Use it as your reference when searching the web or checking file modification times. If you need the exact time, use the Shell tool with a proper command. ## Working Directory @@ -208,6 +208,15 @@ Identify the skills that are likely to be useful for the tasks you are currently Only read skill details when needed to conserve the context window. +# Output Formatting + +Your responses are rendered as Markdown in a terminal. Emit well-formed Markdown so it renders cleanly: + +- **Tables:** put the header row on its own line, the `|---|---|` delimiter row on the immediately following line (no blank line between them), and one row per line. Never glue a table onto adjacent prose (e.g. `Findings| Col |`) and never cram multiple rows onto one line. Leave a blank line before and after the table. +- Prefer a short bullet list over a table when there are only a few items or any cell is long; reserve tables for genuinely tabular data with short cells. +- **Code fences are for code only.** Use triple-backtick blocks (tagged with the language, e.g. ```python, ```toml) solely for source, config, or commands — one snippet per block. Never wrap a prose report, finding list, checklist, or ASCII box in a fence to align or frame it; write it as normal Markdown (headings, bullets, tables) so it renders cleanly. +- **Status icons sparingly.** A check/cross/dot can mark a single headline result, but do not prefix every line with one. Use plain words for severity and outcomes (e.g. `High`, `PASS`, `0 findings`). The terminal renders icons as calm monochrome glyphs only outside code fences — another reason not to box reports. + # Ultimate Reminders At any time, you should be HELPFUL, CONCISE, and ACCURATE. Be thorough in your actions — test what you build, verify what you change — not in your explanations. diff --git a/tests/core/test_default_agent.py b/tests/core/test_default_agent.py index 666f3f60..d59a8342 100644 --- a/tests/core/test_default_agent.py +++ b/tests/core/test_default_agent.py @@ -151,7 +151,7 @@ async def test_default_agent(runtime: Runtime): ## Date and Time -The current date and time in ISO format is `1970-01-01T00:00:00+00:00`. This is only a reference for you when searching the web, or checking file modification time, etc. If you need the exact time, use Shell tool with proper command. +The current date and time in ISO format is `1970-01-01T00:00:00+00:00`. Treat this as the authoritative present — it reflects the real "now", which is later than your training data suggests. Anchor all reasoning about the current date, the year, recency, and what counts as the "latest" version or release to `1970-01-01T00:00:00+00:00`; do not fall back on an earlier year you might assume from training. Use it as your reference when searching the web or checking file modification times. If you need the exact time, use the Shell tool with a proper command. ## Working Directory @@ -216,6 +216,15 @@ async def test_default_agent(runtime: Runtime): Only read skill details when needed to conserve the context window. +# Output Formatting + +Your responses are rendered as Markdown in a terminal. Emit well-formed Markdown so it renders cleanly: + +- **Tables:** put the header row on its own line, the `|---|---|` delimiter row on the immediately following line (no blank line between them), and one row per line. Never glue a table onto adjacent prose (e.g. `Findings| Col |`) and never cram multiple rows onto one line. Leave a blank line before and after the table. +- Prefer a short bullet list over a table when there are only a few items or any cell is long; reserve tables for genuinely tabular data with short cells. +- **Code fences are for code only.** Use triple-backtick blocks (tagged with the language, e.g. ```python, ```toml) solely for source, config, or commands — one snippet per block. Never wrap a prose report, finding list, checklist, or ASCII box in a fence to align or frame it; write it as normal Markdown (headings, bullets, tables) so it renders cleanly. +- **Status icons sparingly.** A check/cross/dot can mark a single headline result, but do not prefix every line with one. Use plain words for severity and outcomes (e.g. `High`, `PASS`, `0 findings`). The terminal renders icons as calm monochrome glyphs only outside code fences — another reason not to box reports. + # Ultimate Reminders At any time, you should be HELPFUL, CONCISE, and ACCURATE. Be thorough in your actions — test what you build, verify what you change — not in your explanations. diff --git a/tests/core/test_load_agent.py b/tests/core/test_load_agent.py index ccbf62bf..7e9ecd00 100644 --- a/tests/core/test_load_agent.py +++ b/tests/core/test_load_agent.py @@ -55,6 +55,24 @@ def test_system_prompt_contains_platform_info(builtin_args: BuiltinSystemPromptA assert builtin_args.PYTHINKER_SHELL in prompt +def test_system_prompt_treats_injected_date_as_authoritative( + builtin_args: BuiltinSystemPromptArgs, +): + """The injected date must be framed as authoritative so the model anchors + its sense of 'now' to it instead of a training-era year.""" + from pythinker_code.agentspec import DEFAULT_AGENT_FILE + + prompt = _load_system_prompt( + DEFAULT_AGENT_FILE.parent / "system.md", + {"ROLE_ADDITIONAL": ""}, + builtin_args, + ) + + assert builtin_args.PYTHINKER_NOW in prompt + assert "authoritative present" in prompt + assert "do not fall back on an earlier year" in prompt + + def test_system_prompt_enforces_context_first_orchestration( builtin_args: BuiltinSystemPromptArgs, ): @@ -74,6 +92,27 @@ def test_system_prompt_enforces_context_first_orchestration( assert "Treat subagent claims as leads, not proof" in prompt +def test_system_prompt_includes_markdown_table_formatting_guidance( + builtin_args: BuiltinSystemPromptArgs, +): + """Default prompt must reach the model with table-formatting rules so it + stops emitting headers glued to prose (which render as raw text).""" + from pythinker_code.agentspec import DEFAULT_AGENT_FILE + + prompt = _load_system_prompt( + DEFAULT_AGENT_FILE.parent / "system.md", + {"ROLE_ADDITIONAL": ""}, + builtin_args, + ) + + assert "# Output Formatting" in prompt + assert "glue a table onto adjacent prose" in prompt + # Reports must not be wrapped in code fences (that is what preserves raw + # emoji and breaks column alignment), and status icons should be sparing. + assert "Code fences are for code only" in prompt + assert "Status icons sparingly" in prompt + + def test_default_subagent_prompts_keep_robust_contracts(): """Specialist subagents should retain evidence, planning, and verification gates.""" from pythinker_code.agentspec import DEFAULT_AGENT_FILE, load_agent_spec From 999ec6337357c6c2db903740f6913163ba348de8 Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Tue, 26 May 2026 23:49:37 -0400 Subject: [PATCH 09/13] refactor(blocks): align subagent output methods with peer guards, add cap test --- src/pythinker_code/ui/shell/visualize/_blocks.py | 4 ++++ tests/ui_and_conv/test_tool_call_block.py | 13 ++++++++++--- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/pythinker_code/ui/shell/visualize/_blocks.py b/src/pythinker_code/ui/shell/visualize/_blocks.py index 396e7f23..ea69dece 100644 --- a/src/pythinker_code/ui/shell/visualize/_blocks.py +++ b/src/pythinker_code/ui/shell/visualize/_blocks.py @@ -518,6 +518,8 @@ def set_subagent_metadata(self, agent_id: str, subagent_type: str) -> None: def mark_sub_execution_started(self, tool_call_id: str) -> None: if tool_call_id not in self._ongoing_subagent_tool_calls: return + if tool_call_id in self._subagent_execution_started: + return self._subagent_execution_started.add(tool_call_id) self._renderable = self._compose() @@ -526,6 +528,8 @@ def append_sub_output_part( ) -> None: if tool_call_id not in self._ongoing_subagent_tool_calls: return + if not text: + return parts = self._subagent_output_parts.setdefault(tool_call_id, []) parts.append(text) if stream == "stderr": diff --git a/tests/ui_and_conv/test_tool_call_block.py b/tests/ui_and_conv/test_tool_call_block.py index a363f10c..4cdda396 100644 --- a/tests/ui_and_conv/test_tool_call_block.py +++ b/tests/ui_and_conv/test_tool_call_block.py @@ -199,6 +199,16 @@ def test_append_sub_output_part_caps_buffer_at_200_chars(): assert len(combined) <= 200 +def test_append_sub_output_part_caps_buffer_across_multiple_appends(): + block = _ToolCallBlock(_tool_call("Agent", '{"description":"scan"}')) + call = _tool_call_with_id("sub-1", "Bash", '{"command":"ls"}') + block.append_sub_tool_call(call) + for _ in range(30): + block.append_sub_output_part("sub-1", "x" * 10) # 300 chars total, 10 at a time + combined = "".join(block._subagent_output_parts["sub-1"]) + assert len(combined) <= 200 + + def test_append_sub_output_part_tracks_stderr(): block = _ToolCallBlock(_tool_call("Agent", '{"description":"scan"}')) call = _tool_call_with_id("sub-1", "Bash", '{"command":"cat missing"}') @@ -222,9 +232,6 @@ def test_mark_sub_execution_started_discards_unknown_id(): def test_finish_sub_tool_call_cleans_up_output_state(): - from pythinker_code.wire.types import ToolResult - from pythinker_core.tooling import ToolOk - block = _ToolCallBlock(_tool_call("Agent", '{"description":"scan"}')) call = _tool_call_with_id("sub-1", "Bash", '{"command":"ls"}') block.append_sub_tool_call(call) From 92018ad59ce4b4b3157ac41ea03d40fc992e7f9c Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Wed, 27 May 2026 09:03:16 -0400 Subject: [PATCH 10/13] feat(web): add fetch/search domain allowlist --- docs/en/configuration/config-files.md | 13 ++ src/pythinker_code/config.py | 32 +++++ src/pythinker_code/tools/web/_allowlist.py | 27 ++++ src/pythinker_code/tools/web/fetch.md | 2 +- src/pythinker_code/tools/web/fetch.py | 131 ++++++++++++------ src/pythinker_code/tools/web/search.md | 2 +- src/pythinker_code/tools/web/search.py | 20 +++ .../ui/shell/tool_renderers/web.py | 14 ++ tasks/todo.md | 26 ++++ tests/core/test_config.py | 1 + tests/tools/test_fetch_url.py | 76 +++++++++- tests/tools/test_web_allowlist.py | 60 ++++++++ tests/tools/test_web_allowlist_tools.py | 99 +++++++++++++ .../test_tui_card_tool_renderers.py | 23 ++- 14 files changed, 476 insertions(+), 50 deletions(-) create mode 100644 src/pythinker_code/tools/web/_allowlist.py create mode 100644 tests/tools/test_web_allowlist.py create mode 100644 tests/tools/test_web_allowlist_tools.py diff --git a/docs/en/configuration/config-files.md b/docs/en/configuration/config-files.md index f2e9cbde..f375d5c0 100644 --- a/docs/en/configuration/config-files.md +++ b/docs/en/configuration/config-files.md @@ -85,6 +85,9 @@ api_key = "sk-xxx" base_url = "https://api.pythinker.com/coding/v1/fetch" api_key = "sk-xxx" +[web] +allowed_domains = ["example.com", "docs.python.org"] + [mcp.client] tool_call_timeout_ms = 60000 ``` @@ -199,6 +202,16 @@ Configures web fetch service. When enabled, the `FetchURL` tool prioritizes usin When configuring the Pythinker platform using the `/login` command, search and fetch services are automatically configured. ::: +### `web` + +`web` configures policy shared by the `FetchURL` and `SearchWeb` tools. + +| Field | Type | Default | Description | +| --- | --- | --- | --- | +| `allowed_domains` | `array` | _unset_ | When set, web fetch and search may only touch these domains and their subdomains. `FetchURL` rejects URLs on other hosts before making any request, and `SearchWeb` drops results from other domains. Unset or empty means unrestricted. | + +This is a coarse governance control layered on top of the existing SSRF protections (which always block private, loopback, link-local, multicast, and reserved addresses); it does not replace them. Matching is label-aware: `example.com` matches `example.com` and `docs.example.com`, but not `notexample.com`. + ### `mcp` `mcp` configures MCP client behavior. diff --git a/src/pythinker_code/config.py b/src/pythinker_code/config.py index 27cff7fe..ed5b1748 100644 --- a/src/pythinker_code/config.py +++ b/src/pythinker_code/config.py @@ -14,6 +14,7 @@ SecretStr, ValidationError, field_serializer, + field_validator, model_validator, ) from tomlkit.exceptions import TOMLKitError @@ -178,6 +179,36 @@ class Services(BaseModel): """Pythinker AI Fetch configuration.""" +class WebConfig(BaseModel): + """Web fetch/search policy.""" + + allowed_domains: list[str] | None = Field( + default=None, + description=( + "If set, web fetch and search may only touch these domains and their " + "subdomains. None or empty means unrestricted (default)." + ), + ) + + @field_validator("allowed_domains") + @classmethod + def _validate_allowed_domains(cls, value: list[str] | None) -> list[str] | None: + for entry in value or []: + cleaned = entry.strip() + if not cleaned: + raise ValueError( + "Invalid allowed_domains entry: empty or whitespace-only hostname. " + "Remove it, or omit allowed_domains entirely to leave web access " + "unrestricted." + ) + if any(char in cleaned for char in "/: \t"): + raise ValueError( + f"Invalid allowed_domains entry {entry!r}: use a bare hostname " + "like 'example.com', not a URL, path, or host:port." + ) + return value + + class FeedbackConfig(BaseModel): """User-submitted feedback endpoint configuration.""" @@ -324,6 +355,7 @@ class Config(BaseModel): default_factory=NotificationConfig, description="Notification configuration" ) services: Services = Field(default_factory=Services, description="Services configuration") + web: WebConfig = Field(default_factory=WebConfig, description="Web fetch/search policy") feedback: FeedbackConfig = Field( default_factory=FeedbackConfig, description="User-submitted feedback endpoint configuration", diff --git a/src/pythinker_code/tools/web/_allowlist.py b/src/pythinker_code/tools/web/_allowlist.py new file mode 100644 index 00000000..81f4bf16 --- /dev/null +++ b/src/pythinker_code/tools/web/_allowlist.py @@ -0,0 +1,27 @@ +"""Domain allowlist matching shared by the web fetch and search tools.""" + +from __future__ import annotations + + +def _normalize(entry: str) -> str: + return entry.strip().lstrip(".").lower() + + +def host_in_allowlist(host: str | None, allowed: list[str] | None) -> bool: + """Return whether *host* is permitted by the *allowed* domain list. + + A ``None`` or empty allowlist imposes no restriction (returns ``True``), + preserving unconfigured behavior. Otherwise a host matches when it equals an + allowlist entry or is a subdomain of one. Matching is label-aware and + case-insensitive: ``example.com`` matches ``example.com`` and + ``docs.example.com`` but not ``notexample.com``. + """ + entries = [normalized for entry in (allowed or []) if (normalized := _normalize(entry))] + if not entries: + return True + + host = (host or "").strip().rstrip(".").lower() + if not host: + return False + + return any(host == entry or host.endswith(f".{entry}") for entry in entries) diff --git a/src/pythinker_code/tools/web/fetch.md b/src/pythinker_code/tools/web/fetch.md index 73ebcc80..e7b63dfd 100644 --- a/src/pythinker_code/tools/web/fetch.md +++ b/src/pythinker_code/tools/web/fetch.md @@ -1 +1 @@ -Fetch a web page from a URL and extract main text content from it. +Fetch a web page from a URL and extract main text content from it. Requests may be restricted to a configured set of allowed domains; fetching a disallowed host (including via a redirect) returns an error. diff --git a/src/pythinker_code/tools/web/fetch.py b/src/pythinker_code/tools/web/fetch.py index bdbe103e..28cd7552 100644 --- a/src/pythinker_code/tools/web/fetch.py +++ b/src/pythinker_code/tools/web/fetch.py @@ -2,7 +2,7 @@ import socket from pathlib import Path from typing import override -from urllib.parse import urlparse +from urllib.parse import urljoin, urlparse import aiohttp import trafilatura @@ -15,19 +15,25 @@ from pythinker_code.soul.agent import Runtime from pythinker_code.soul.toolset import get_current_tool_call_or_none from pythinker_code.tools.utils import ToolResultBuilder, load_desc +from pythinker_code.tools.web._allowlist import host_in_allowlist from pythinker_code.utils.aiohttp import new_client_session from pythinker_code.utils.logging import logger MAX_FETCH_BYTES = 5 * 1024 * 1024 +MAX_REDIRECTS = 5 +_REDIRECT_STATUSES = {301, 302, 303, 307, 308} -def _validate_fetch_url(url: str) -> str | None: +def _validate_fetch_url(url: str, allowed_domains: list[str] | None = None) -> str | None: parsed = urlparse(url) if parsed.scheme not in {"http", "https"}: return "Only http and https URLs are supported." if not parsed.hostname: return "URL must include a host." + if not host_in_allowlist(parsed.hostname, allowed_domains): + return "URL host is not in the configured web allowlist." + try: infos = socket.getaddrinfo(parsed.hostname, parsed.port, type=socket.SOCK_STREAM) except socket.gaierror: @@ -78,6 +84,7 @@ def __init__(self, config: Config, runtime: Runtime): super().__init__() self._runtime = runtime self._service_config = config.services.pythinker_ai_fetch + self._allowed_domains = config.web.allowed_domains @override async def __call__(self, params: Params) -> ToolReturnValue: @@ -96,65 +103,97 @@ async def __call__(self, params: Params) -> ToolReturnValue: return ret logger.warning("Failed to fetch URL via service: {error}", error=ret.message) # fallback to local fetch if service fetch fails - return await self.fetch_with_http_get(params) + return await self.fetch_with_http_get(params, self._allowed_domains) @staticmethod - async def fetch_with_http_get(params: Params) -> ToolReturnValue: + async def fetch_with_http_get( + params: Params, allowed_domains: list[str] | None = None + ) -> ToolReturnValue: builder = ToolResultBuilder(max_line_length=None) - if reason := _validate_fetch_url(params.url): + if reason := _validate_fetch_url(params.url, allowed_domains): return builder.error(f"Failed to fetch URL: {reason}", brief="URL blocked") + resp_text = "" + current_url = params.url try: # Fetching arbitrary web pages can take a while on large/slow sites. fetch_timeout = aiohttp.ClientTimeout(total=180, sock_read=60, sock_connect=15) - async with ( - new_client_session(timeout=fetch_timeout) as session, - session.get( - params.url, - headers={ - "User-Agent": ( - "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 " - "(KHTML, like Gecko) Chrome/91.0.4472.124 Safari/537.36" - ), - }, - ) as response, - ): - if response.status >= 400: - logger.warning( - "FetchURL HTTP error: status={status}, url={url}", - status=response.status, - url=params.url, - ) - return builder.error( - ( - f"Failed to fetch URL. Status: {response.status}. " - f"This may indicate the page is not accessible or the server is down." - ), - brief=f"HTTP {response.status} error", - ) + headers = { + "User-Agent": ( + "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 " + "(KHTML, like Gecko) Chrome/91.0.4472.124 Safari/537.36" + ), + } + # Follow redirects manually so each hop is re-validated against the + # allowlist and SSRF guard; aiohttp's automatic redirects would only + # honor the checks on the initial URL. + async with new_client_session(timeout=fetch_timeout) as session: + for _ in range(MAX_REDIRECTS + 1): + async with session.get( + current_url, headers=headers, allow_redirects=False + ) as response: + if response.status in _REDIRECT_STATUSES: + location = response.headers.get(aiohttp.hdrs.LOCATION) + if not location: + return builder.error( + "Failed to fetch URL: redirect response missing a " + "Location header.", + brief="Invalid redirect", + ) + current_url = urljoin(current_url, location) + if reason := _validate_fetch_url(current_url, allowed_domains): + return builder.error( + f"Failed to fetch URL: redirect to a disallowed " + f"location: {reason}", + brief="Redirect blocked", + ) + continue - try: - resp_text = (await _read_limited(response, MAX_FETCH_BYTES)).decode( - "utf-8", errors="replace" - ) - except ValueError: - max_mb = MAX_FETCH_BYTES // 1024 // 1024 + if response.status >= 400: + logger.warning( + "FetchURL HTTP error: status={status}, url={url}", + status=response.status, + url=current_url, + ) + return builder.error( + ( + f"Failed to fetch URL. Status: {response.status}. " + f"This may indicate the page is not accessible or the " + f"server is down." + ), + brief=f"HTTP {response.status} error", + ) + + try: + resp_text = (await _read_limited(response, MAX_FETCH_BYTES)).decode( + "utf-8", errors="replace" + ) + except ValueError: + max_mb = MAX_FETCH_BYTES // 1024 // 1024 + return builder.error( + f"Failed to fetch URL: response exceeds {max_mb}MB.", + brief="Response too large", + ) + + content_type = response.headers.get(aiohttp.hdrs.CONTENT_TYPE, "").lower() + if content_type.startswith(("text/plain", "text/markdown")): + builder.write(resp_text) + return builder.ok( + "The returned content is the full content of the page." + ) + break + else: return builder.error( - f"Failed to fetch URL: response exceeds {max_mb}MB.", - brief="Response too large", + "Failed to fetch URL: too many redirects.", + brief="Too many redirects", ) - - content_type = response.headers.get(aiohttp.hdrs.CONTENT_TYPE, "").lower() - if content_type.startswith(("text/plain", "text/markdown")): - builder.write(resp_text) - return builder.ok("The returned content is the full content of the page.") except TimeoutError: - logger.warning("FetchURL timed out: url={url}", url=params.url) + logger.warning("FetchURL timed out: url={url}", url=current_url) return builder.error( "Failed to fetch URL: request timed out. The server may be slow or unreachable.", brief="Request timed out", ) except aiohttp.ClientError as e: - logger.warning("FetchURL network error: {error}, url={url}", error=e, url=params.url) + logger.warning("FetchURL network error: {error}, url={url}", error=e, url=current_url) return builder.error( ( f"Failed to fetch URL due to network error: {e}. " @@ -206,7 +245,7 @@ async def _fetch_with_service(self, params: Params) -> ToolReturnValue: "Fetch service is not configured. You may want to try other methods to fetch.", brief="Fetch service not configured", ) - if reason := _validate_fetch_url(params.url): + if reason := _validate_fetch_url(params.url, self._allowed_domains): return builder.error(f"Failed to fetch URL: {reason}", brief="URL blocked") headers = { diff --git a/src/pythinker_code/tools/web/search.md b/src/pythinker_code/tools/web/search.md index 19e4cec7..1d6ea79b 100644 --- a/src/pythinker_code/tools/web/search.md +++ b/src/pythinker_code/tools/web/search.md @@ -1 +1 @@ -WebSearch tool allows you to search on the internet to get latest information, including news, documents, release notes, blog posts, papers, etc. +WebSearch tool allows you to search on the internet to get latest information, including news, documents, release notes, blog posts, papers, etc. Results may be limited to a configured set of allowed domains. diff --git a/src/pythinker_code/tools/web/search.py b/src/pythinker_code/tools/web/search.py index c7bb8a26..b96e030b 100644 --- a/src/pythinker_code/tools/web/search.py +++ b/src/pythinker_code/tools/web/search.py @@ -1,5 +1,6 @@ from pathlib import Path from typing import override +from urllib.parse import urlparse import aiohttp from pydantic import BaseModel, Field, ValidationError @@ -12,6 +13,7 @@ from pythinker_code.soul.toolset import get_current_tool_call_or_none from pythinker_code.tools import SkipThisTool from pythinker_code.tools.utils import ToolResultBuilder, load_desc +from pythinker_code.tools.web._allowlist import host_in_allowlist from pythinker_code.utils.aiohttp import new_client_session from pythinker_code.utils.logging import logger @@ -53,6 +55,7 @@ def __init__(self, config: Config, runtime: Runtime): self._api_key = config.services.pythinker_ai_search.api_key self._oauth_ref = config.services.pythinker_ai_search.oauth self._custom_headers = config.services.pythinker_ai_search.custom_headers or {} + self._allowed_domains = config.web.allowed_domains @override async def __call__(self, params: Params) -> ToolReturnValue: @@ -145,6 +148,23 @@ async def __call__(self, params: Params) -> ToolReturnValue: brief="Search request failed", ) + if self._allowed_domains: + kept = [ + result + for result in results + if host_in_allowlist(urlparse(result.url).hostname, self._allowed_domains) + ] + dropped = len(results) - len(kept) + results = kept + if dropped: + builder.extras(allowlist_filtered=dropped) + if not results: + return builder.ok( + f"All {dropped} search result(s) were outside the configured " + "web allowlist and have been omitted.", + brief="Filtered by allowlist", + ) + for i, result in enumerate(results): if i > 0: builder.write("---\n\n") diff --git a/src/pythinker_code/ui/shell/tool_renderers/web.py b/src/pythinker_code/ui/shell/tool_renderers/web.py index 1ccb7540..e8aee68f 100644 --- a/src/pythinker_code/ui/shell/tool_renderers/web.py +++ b/src/pythinker_code/ui/shell/tool_renderers/web.py @@ -2,6 +2,8 @@ from __future__ import annotations +from typing import cast + from rich.console import Group, RenderableType from rich.text import Text @@ -173,6 +175,16 @@ def _search_result_count(text: str) -> int: return len([line for line in text.splitlines() if line.strip()]) +def _allowlist_filtered_count(result: ToolResultPayload) -> int: + """Number of search results dropped by the domain allowlist, if any.""" + extras_raw = result.details.get("extras") + extras = cast("dict[str, object]", extras_raw) if isinstance(extras_raw, dict) else {} + value = extras.get("allowlist_filtered") + if isinstance(value, int) and value > 0: + return value + return 0 + + def _render_search_result( ctx: ToolRenderContext, result: ToolResultPayload ) -> RenderableType | None: @@ -196,6 +208,8 @@ def _render_search_result( summary.append("Found ", style=tui_rich_style("tool_output")) summary.append(str(count), style=tui_rich_style("tool_title")) summary.append(f" {_plural(count, 'result')}", style=tui_rich_style("tool_output")) + if filtered := _allowlist_filtered_count(result): + summary.append(f" · {filtered} filtered to allowlist", style=tui_rich_style("muted")) ctx.state["__suppress_generic_expand_hint__"] = True if count and not ctx.expanded: summary.append(" ") diff --git a/tasks/todo.md b/tasks/todo.md index c6005f91..0bfbc90c 100644 --- a/tasks/todo.md +++ b/tasks/todo.md @@ -467,3 +467,29 @@ PyPI 1.0.0 of pythinker-cli stays published forever. Anyone who installed it bef 4. **Migration text in README**: Do you want a "migrating from pythinker-cli" callout in 1.1.0's README, or just silently switch? 5. **CHANGELOG framing**: Is this a breaking change that warrants 2.0.0, or a layout change that's fine at 1.1.0? PyPI users perspective: install command changed, that's user-visible breakage. Could argue 2.0.0. + +--- + +# Web fetch/search domain allowlist (2026-05-27) + +Port of the one genuinely portable concept from pythinker-x's web search +(`allowed_domains`) onto our self-hosted FetchURL/SearchWeb tools. Design spec: +`docs/superpowers/specs/2026-05-27-web-allowed-domains-design.md`. + +- [x] `WebConfig.allowed_domains` config (+ field validator rejecting URLs/paths/host:port) +- [x] `host_in_allowlist` helper (label-aware subdomain match, unrestricted when empty) +- [x] FetchURL: reject out-of-allowlist hosts in `_validate_fetch_url` (no request made) +- [x] SearchWeb: post-filter results, surface dropped count via `extras` +- [x] TUI: muted "· N filtered to allowlist" indicator on the search result header +- [x] Tests: helper, config validation, fetch rejection, search filter, renderer indicator +- [x] Docs: `docs/en/configuration/config-files.md` `web` section + example + +## Review +- All affected suites green (tools/core/ui = 2517 passed earlier; affected subset 100 passed). +- ruff + ruff format clean; pyright clean on all changed files. The 8 pre-existing + pyright errors live in `cli/mcp.py` and `soul/toolset.py` (untouched, baseline). +- Dropped from scope (cosmetic/redundant in our architecture): action taxonomy relabel, + disabled/cached/live mode gating. See design doc "Out of scope". + +## Out of scope (observed, not changed) +- Pre-existing pyright errors in `cli/mcp.py`, `soul/toolset.py`. diff --git a/tests/core/test_config.py b/tests/core/test_config.py index d5194ec6..77f0cc64 100644 --- a/tests/core/test_config.py +++ b/tests/core/test_config.py @@ -58,6 +58,7 @@ def test_default_config_dump(): }, "services": {"pythinker_ai_search": None, "pythinker_ai_fetch": None}, "mcp": {"client": {"tool_call_timeout_ms": 60000}}, + "web": {"allowed_domains": None}, "feedback": { "endpoint_url": "", "api_key": None, diff --git a/tests/tools/test_fetch_url.py b/tests/tools/test_fetch_url.py index 16674c8a..6b727853 100644 --- a/tests/tools/test_fetch_url.py +++ b/tests/tools/test_fetch_url.py @@ -24,7 +24,7 @@ def _bypass_ssrf_validation(monkeypatch: pytest.MonkeyPatch) -> None: exercises malformed-URL handling that pre-dates the validator. Disable it for these unit tests; production callers still get the protection. """ - monkeypatch.setattr(fetch_module, "_validate_fetch_url", lambda _url: None) + monkeypatch.setattr(fetch_module, "_validate_fetch_url", lambda _url, _allowed=None: None) class MockServerFactory(Protocol): @@ -241,6 +241,80 @@ async def mocked_fetch(resp: str, *, content_type: str = "text/html") -> ToolRet assert result.message == "The returned content is the full content of the page." +async def _start_redirect_server(routes) -> tuple[str, web.AppRunner]: + """Start a server with the given (path, handler) routes; return base URL + runner.""" + app = web.Application() + for path, handler in routes: + app.router.add_get(path, handler) + runner = web.AppRunner(app) + await runner.setup() + site = web.TCPSite(runner, host="127.0.0.1", port=0) + await site.start() + port = site._server.sockets[0].getsockname()[1] # type: ignore[attr-defined] + return f"http://127.0.0.1:{port}", runner + + +async def test_fetch_url_follows_validated_redirect(fetch_url_tool: FetchURL) -> None: + """A redirect to an allowed location is followed (validation is bypassed here).""" + + async def start(request: web.Request) -> web.Response: # noqa: ARG001 + raise web.HTTPFound("/end") + + async def end(request: web.Request) -> web.Response: # noqa: ARG001 + return web.Response(text="redirected body content", content_type="text/plain") + + base, runner = await _start_redirect_server([("/start", start), ("/end", end)]) + try: + result = await fetch_url_tool(Params(url=f"{base}/start")) + finally: + await runner.cleanup() + + assert not result.is_error + assert "redirected body content" in result.output + + +async def test_fetch_url_blocks_redirect_to_disallowed_host( + fetch_url_tool: FetchURL, monkeypatch: pytest.MonkeyPatch +) -> None: + """A redirect whose target fails validation is blocked without being fetched.""" + + # Override the module-wide bypass: allow the initial localhost URL, block the + # redirect target. The blocked host is never actually contacted. + monkeypatch.setattr( + fetch_module, + "_validate_fetch_url", + lambda url, _allowed=None: ("host blocked" if "blocked.invalid" in url else None), + ) + + async def start(request: web.Request) -> web.Response: # noqa: ARG001 + raise web.HTTPFound("http://blocked.invalid/secret") + + base, runner = await _start_redirect_server([("/start", start)]) + try: + result = await fetch_url_tool(Params(url=f"{base}/start")) + finally: + await runner.cleanup() + + assert result.is_error + assert "redirect" in result.message.lower() + + +async def test_fetch_url_rejects_redirect_loop(fetch_url_tool: FetchURL) -> None: + """A redirect loop terminates with a 'too many redirects' error.""" + + async def loop(request: web.Request) -> web.Response: # noqa: ARG001 + raise web.HTTPFound("/loop") + + base, runner = await _start_redirect_server([("/loop", loop)]) + try: + result = await fetch_url_tool(Params(url=f"{base}/loop")) + finally: + await runner.cleanup() + + assert result.is_error + assert "too many redirects" in result.message.lower() + + async def test_fetch_url_with_service(runtime) -> None: """Test fetching using the pythinker_ai_fetch service.""" from pythinker_code.config import Config, PythinkerAIFetchConfig, Services diff --git a/tests/tools/test_web_allowlist.py b/tests/tools/test_web_allowlist.py new file mode 100644 index 00000000..6f8a456d --- /dev/null +++ b/tests/tools/test_web_allowlist.py @@ -0,0 +1,60 @@ +"""Tests for the web domain allowlist helper.""" + +from __future__ import annotations + +import pytest +from pydantic import ValidationError + +from pythinker_code.config import WebConfig +from pythinker_code.tools.web._allowlist import host_in_allowlist + + +@pytest.mark.parametrize( + ("host", "allowed", "expected"), + [ + # None / empty allowlist is unrestricted. + ("anything.com", None, True), + ("anything.com", [], True), + ("anything.com", [" ", "."], True), # entries normalize to empty + # Exact match. + ("example.com", ["example.com"], True), + # Subdomain match. + ("docs.example.com", ["example.com"], True), + ("a.b.example.com", ["example.com"], True), + # Lookalike must NOT match. + ("notexample.com", ["example.com"], False), + ("example.com.evil.com", ["example.com"], False), + # Different domain. + ("other.org", ["example.com"], False), + # Case-insensitivity and normalization (leading dot, whitespace). + ("DOCS.Example.COM", [" .Example.com "], True), + # Trailing-dot FQDN host. + ("docs.example.com.", ["example.com"], True), + # Multiple entries: match any. + ("foo.org", ["example.com", "foo.org"], True), + # Empty / None host with a non-empty allowlist is rejected. + ("", ["example.com"], False), + (None, ["example.com"], False), + ], +) +def test_host_in_allowlist(host: str | None, allowed: list[str] | None, expected: bool) -> None: + assert host_in_allowlist(host, allowed) is expected + + +def test_web_config_accepts_bare_hostnames() -> None: + cfg = WebConfig(allowed_domains=["example.com", "docs.python.org"]) + assert cfg.allowed_domains == ["example.com", "docs.python.org"] + + +@pytest.mark.parametrize( + "bad_entry", + [ + "https://example.com", # scheme + "example.com/path", # path + "example.com:8080", # port + "two words.com", # whitespace + ], +) +def test_web_config_rejects_malformed_entries(bad_entry: str) -> None: + with pytest.raises(ValidationError): + WebConfig(allowed_domains=[bad_entry]) diff --git a/tests/tools/test_web_allowlist_tools.py b/tests/tools/test_web_allowlist_tools.py new file mode 100644 index 00000000..d69a5270 --- /dev/null +++ b/tests/tools/test_web_allowlist_tools.py @@ -0,0 +1,99 @@ +# ruff: noqa + +"""Tool-level tests for the web domain allowlist on FetchURL and SearchWeb.""" + +from __future__ import annotations + +from aiohttp import web +from pydantic import SecretStr + +from pythinker_code.config import Config, PythinkerAISearchConfig +from pythinker_code.soul.toolset import current_tool_call +from pythinker_code.tools.web import fetch as fetch_module +from pythinker_code.tools.web.fetch import FetchURL, Params, _validate_fetch_url +from pythinker_code.tools.web.search import Params as SearchParams +from pythinker_code.tools.web.search import SearchWeb +from pythinker_code.wire.types import ToolCall + + +def test_validate_fetch_url_allows_in_allowlist_host() -> None: + # Allowed host (subdomain) passes validation; SSRF check is unrelated here. + assert _validate_fetch_url("https://docs.example.com/x", ["example.com"]) is None + + +def test_validate_fetch_url_rejects_out_of_allowlist_host() -> None: + reason = _validate_fetch_url("https://evil.org/x", ["example.com"]) + assert reason == "URL host is not in the configured web allowlist." + + +async def test_fetch_url_rejected_by_allowlist_makes_no_request( + config: Config, runtime, monkeypatch +) -> None: + config.web.allowed_domains = ["example.com"] + + def _no_network(*_args, **_kwargs): + raise AssertionError("network must not be opened for a disallowed host") + + monkeypatch.setattr(fetch_module, "new_client_session", _no_network) + + tool = FetchURL(config=config, runtime=runtime) + result = await tool(Params(url="https://evil.org/page")) + + assert result.is_error + assert "allowlist" in result.message.lower() + + +async def test_search_web_filters_results_by_allowlist(config: Config, runtime) -> None: + payload = { + "search_results": [ + { + "site_name": "Example", + "title": "Allowed", + "url": "https://docs.example.com/a", + "snippet": "kept", + }, + { + "site_name": "Evil", + "title": "Blocked", + "url": "https://evil.org/b", + "snippet": "dropped", + }, + ] + } + + async def handler(request: web.Request) -> web.Response: # noqa: ARG001 + return web.json_response(payload) + + app = web.Application() + app.router.add_post("/search", handler) + runner = web.AppRunner(app) + await runner.setup() + site = web.TCPSite(runner, host="127.0.0.1", port=0) + await site.start() + port = site._server.sockets[0].getsockname()[1] # type: ignore[index] + + try: + config.services.pythinker_ai_search = PythinkerAISearchConfig( + base_url=f"http://127.0.0.1:{port}/search", + api_key=SecretStr("test-key"), + ) + config.web.allowed_domains = ["example.com"] + tool = SearchWeb(config, runtime) + + token = current_tool_call.set( + ToolCall( + id="test-call-id", + function=ToolCall.FunctionBody(name="SearchWeb", arguments=None), + ) + ) + try: + result = await tool(SearchParams(query="x")) + finally: + current_tool_call.reset(token) + finally: + await runner.cleanup() + + assert not result.is_error + assert "docs.example.com" in result.output + assert "evil.org" not in result.output + assert (result.extras or {}).get("allowlist_filtered") == 1 diff --git a/tests/ui_and_conv/test_tui_card_tool_renderers.py b/tests/ui_and_conv/test_tui_card_tool_renderers.py index ae0aea30..37a5b651 100644 --- a/tests/ui_and_conv/test_tui_card_tool_renderers.py +++ b/tests/ui_and_conv/test_tui_card_tool_renderers.py @@ -46,6 +46,7 @@ def _render( is_error: bool = False, expanded: bool = False, width: int = 100, + details: dict | None = None, ) -> str: defn = get_tool_renderer(tool) assert defn is not None, f"renderer not registered for {tool!r}" @@ -53,7 +54,7 @@ def _render( comp.update_args(args) comp.set_args_complete() comp.mark_execution_started() - comp.set_result(ToolResultPayload(text=output, is_error=is_error)) + comp.set_result(ToolResultPayload(text=output, is_error=is_error, details=details or {})) comp.set_expanded(expanded) return render_plain(comp.render(), width=width) @@ -907,6 +908,26 @@ def test_search_counts_structured_result_blocks(): assert "Found 2 results" in rendered +def test_search_shows_allowlist_filtered_indicator(): + rendered = _render( + "SearchWeb", + {"query": "python"}, + output="Title: One\nDate: \nURL: https://example.com/1\nSummary: A\n\n", + details={"extras": {"allowlist_filtered": 2}}, + ) + assert "Found 1 result" in rendered + assert "2 filtered to allowlist" in rendered + + +def test_search_no_allowlist_indicator_when_not_filtered(): + rendered = _render( + "SearchWeb", + {"query": "python"}, + output="Title: One\nDate: \nURL: https://example.com/1\nSummary: A\n\n", + ) + assert "filtered to allowlist" not in rendered + + # --------------------------------------------------------------------------- # Background tasks # --------------------------------------------------------------------------- From c613490ffc34abbd40f027803701b5c33f6852a5 Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Wed, 27 May 2026 09:30:37 -0400 Subject: [PATCH 11/13] test(web): add domain-allowlist fetch tests and tool-description/config docs --- docs/en/configuration/config-files.md | 2 +- tasks/todo.md | 14 ++++++++++++++ tests/tools/test_fetch_url.py | 21 +++++++++++++++------ tests/tools/test_tool_descriptions.py | 4 ++-- 4 files changed, 32 insertions(+), 9 deletions(-) diff --git a/docs/en/configuration/config-files.md b/docs/en/configuration/config-files.md index f375d5c0..b6621036 100644 --- a/docs/en/configuration/config-files.md +++ b/docs/en/configuration/config-files.md @@ -208,7 +208,7 @@ When configuring the Pythinker platform using the `/login` command, search and f | Field | Type | Default | Description | | --- | --- | --- | --- | -| `allowed_domains` | `array` | _unset_ | When set, web fetch and search may only touch these domains and their subdomains. `FetchURL` rejects URLs on other hosts before making any request, and `SearchWeb` drops results from other domains. Unset or empty means unrestricted. | +| `allowed_domains` | `array` | _unset_ | When set, web fetch and search may only touch these domains and their subdomains. `FetchURL` rejects URLs on other hosts before making any request — including redirect targets, which are re-validated on every hop — and `SearchWeb` drops results from other domains. Unset or empty means unrestricted. Entries must be bare hostnames (e.g. `example.com`), not URLs, paths, or `host:port`. | This is a coarse governance control layered on top of the existing SSRF protections (which always block private, loopback, link-local, multicast, and reserved addresses); it does not replace them. Matching is label-aware: `example.com` matches `example.com` and `docs.example.com`, but not `notexample.com`. diff --git a/tasks/todo.md b/tasks/todo.md index 0bfbc90c..49435679 100644 --- a/tasks/todo.md +++ b/tasks/todo.md @@ -493,3 +493,17 @@ Port of the one genuinely portable concept from pythinker-x's web search ## Out of scope (observed, not changed) - Pre-existing pyright errors in `cli/mcp.py`, `soul/toolset.py`. + +## Review follow-up (2026-05-27) — context7-validated hardening +Reviewed the allowlist against context7 (aiohttp v3.13.2, pydantic v2) + 2026 agent-tool practice. +- [x] HIGH: redirect bypass — `fetch_with_http_get` now sets `allow_redirects=False` and follows + redirects manually (max 5), re-validating each hop via `_validate_fetch_url` (allowlist + + SSRF). Closes a pre-existing SSRF gap the allowlist had inherited. Tests: follows validated + redirect, blocks redirect to disallowed host (never contacted), rejects redirect loop. +- [x] LOW: `fetch.md` / `search.md` now state the allowlist constraint to the model. +- [x] LOW: `WebConfig` validator now rejects empty/whitespace-only entries (was silently unrestricted). +- Confirmed-good (context7): pydantic validator matches docs exactly; `extras` TUI channel; + fail-closed on unparseable hosts; allowlist-before-DNS ordering. +- Known/accepted limitation (pre-existing, not addressed): DNS-rebinding TOCTOU — `_validate_fetch_url` + resolves+checks IPs but aiohttp re-resolves at connect time. Out of scope; would need a pinning connector. +- Snapshots updated: `test_default_config_dump`, `test_fetch_url_description`, `test_search_web_description`. diff --git a/tests/tools/test_fetch_url.py b/tests/tools/test_fetch_url.py index 6b727853..091adab4 100644 --- a/tests/tools/test_fetch_url.py +++ b/tests/tools/test_fetch_url.py @@ -276,20 +276,27 @@ async def end(request: web.Request) -> web.Response: # noqa: ARG001 async def test_fetch_url_blocks_redirect_to_disallowed_host( fetch_url_tool: FetchURL, monkeypatch: pytest.MonkeyPatch ) -> None: - """A redirect whose target fails validation is blocked without being fetched.""" + """A redirect whose target fails validation is blocked *before* being fetched.""" - # Override the module-wide bypass: allow the initial localhost URL, block the - # redirect target. The blocked host is never actually contacted. + # Override the module-wide bypass: allow the initial URL, block the redirect + # target (any URL whose path is /blocked). monkeypatch.setattr( fetch_module, "_validate_fetch_url", - lambda url, _allowed=None: ("host blocked" if "blocked.invalid" in url else None), + lambda url, _allowed=None: ("host blocked" if "/blocked" in url else None), ) + blocked_hit = False + async def start(request: web.Request) -> web.Response: # noqa: ARG001 - raise web.HTTPFound("http://blocked.invalid/secret") + raise web.HTTPFound("/blocked") + + async def blocked(request: web.Request) -> web.Response: # noqa: ARG001 + nonlocal blocked_hit + blocked_hit = True + return web.Response(text="secret", content_type="text/plain") - base, runner = await _start_redirect_server([("/start", start)]) + base, runner = await _start_redirect_server([("/start", start), ("/blocked", blocked)]) try: result = await fetch_url_tool(Params(url=f"{base}/start")) finally: @@ -297,6 +304,8 @@ async def start(request: web.Request) -> web.Response: # noqa: ARG001 assert result.is_error assert "redirect" in result.message.lower() + # The security guarantee: the disallowed location was never contacted. + assert blocked_hit is False async def test_fetch_url_rejects_redirect_loop(fetch_url_tool: FetchURL) -> None: diff --git a/tests/tools/test_tool_descriptions.py b/tests/tools/test_tool_descriptions.py index 5c93e47f..eb6ee96b 100644 --- a/tests/tools/test_tool_descriptions.py +++ b/tests/tools/test_tool_descriptions.py @@ -374,12 +374,12 @@ def test_str_replace_file_description(str_replace_file_tool: StrReplaceFile): def test_search_web_description(search_web_tool: SearchWeb): """Test the description of PythinkerAISearch tool.""" assert search_web_tool.base.description == snapshot( - "WebSearch tool allows you to search on the internet to get latest information, including news, documents, release notes, blog posts, papers, etc.\n" + "WebSearch tool allows you to search on the internet to get latest information, including news, documents, release notes, blog posts, papers, etc. Results may be limited to a configured set of allowed domains.\n" ) def test_fetch_url_description(fetch_url_tool: FetchURL): """Test the description of FetchURL tool.""" assert fetch_url_tool.base.description == snapshot( - "Fetch a web page from a URL and extract main text content from it.\n" + "Fetch a web page from a URL and extract main text content from it. Requests may be restricted to a configured set of allowed domains; fetching a disallowed host (including via a redirect) returns an error.\n" ) From f348913ba7d4476dc8079f156427dee520ae455a Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Fri, 29 May 2026 14:00:50 -0400 Subject: [PATCH 12/13] fix(typos): unparseable -> unparsable in tasks/todo.md to satisfy spell-check gate --- tasks/todo.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tasks/todo.md b/tasks/todo.md index 67234cdd..e9ff38a6 100644 --- a/tasks/todo.md +++ b/tasks/todo.md @@ -503,7 +503,7 @@ Reviewed the allowlist against context7 (aiohttp v3.13.2, pydantic v2) + 2026 ag - [x] LOW: `fetch.md` / `search.md` now state the allowlist constraint to the model. - [x] LOW: `WebConfig` validator now rejects empty/whitespace-only entries (was silently unrestricted). - Confirmed-good (context7): pydantic validator matches docs exactly; `extras` TUI channel; - fail-closed on unparseable hosts; allowlist-before-DNS ordering. + fail-closed on unparsable hosts; allowlist-before-DNS ordering. - Known/accepted limitation (pre-existing, not addressed): DNS-rebinding TOCTOU — `_validate_fetch_url` resolves+checks IPs but aiohttp re-resolves at connect time. Out of scope; would need a pinning connector. - Snapshots updated: `test_default_config_dump`, `test_fetch_url_description`, `test_search_web_description`. From c6d3bc045b080166a39969a5c79a0bf0004dc8fe Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Fri, 29 May 2026 14:45:25 -0400 Subject: [PATCH 13/13] fix: address CodeRabbit review findings on web allowlist and markdown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - config.py: reject `allowed_domains` entries that are dots-only (would normalize to empty → silently unrestricted) and any entry containing whitespace (newlines previously slipped past the space/tab check). - tools/web/_allowlist.py: strip trailing dots when normalizing entries so `example.com.` matches `example.com` hosts. - tools/web/search.py + ui/shell/tool_renderers/web.py: emit a structured `returned_results=0` signal on the all-filtered path and have the search renderer prefer it, so an all-filtered result reports "0 results" instead of misreading the prose notice as one result. - ui/shell/components/markdown.py: prefix rebuilt table rows with the captured delimiter-line indent so normalization never promotes an indented table to top level (defensive; the guard already bails on non-empty indent today). - agents/default/system.md: reword the code-fence guidance to use inline code spans for language names (markdownlint MD038). - Tests: reject `.`/whitespace allowlist entries, trailing-dot entry matching, and an all-results-filtered renderer regression; refresh the default-agent system-prompt snapshot. --- src/pythinker_code/agents/default/system.md | 2 +- src/pythinker_code/config.py | 7 ++++++- src/pythinker_code/tools/web/_allowlist.py | 2 +- src/pythinker_code/tools/web/search.py | 3 +++ .../ui/shell/components/markdown.py | 13 ++++++++----- .../ui/shell/tool_renderers/web.py | 11 ++++++++++- tests/core/test_default_agent.py | 2 +- tests/tools/test_web_allowlist.py | 6 ++++++ .../ui_and_conv/test_tui_card_tool_renderers.py | 17 +++++++++++++++++ 9 files changed, 53 insertions(+), 10 deletions(-) diff --git a/src/pythinker_code/agents/default/system.md b/src/pythinker_code/agents/default/system.md index 8ccc0763..e69583a9 100644 --- a/src/pythinker_code/agents/default/system.md +++ b/src/pythinker_code/agents/default/system.md @@ -267,7 +267,7 @@ Your responses are rendered as Markdown in a terminal. Emit well-formed Markdown - **Tables:** put the header row on its own line, the `|---|---|` delimiter row on the immediately following line (no blank line between them), and one row per line. Never glue a table onto adjacent prose (e.g. `Findings| Col |`) and never cram multiple rows onto one line. Leave a blank line before and after the table. - Prefer a short bullet list over a table when there are only a few items or any cell is long; reserve tables for genuinely tabular data with short cells. -- **Code fences are for code only.** Use triple-backtick blocks (tagged with the language, e.g. ```python, ```toml) solely for source, config, or commands — one snippet per block. Never wrap a prose report, finding list, checklist, or ASCII box in a fence to align or frame it; write it as normal Markdown (headings, bullets, tables) so it renders cleanly. +- **Code fences are for code only.** Use triple-backtick blocks tagged with a language (for example, `python` or `toml`) solely for source, config, or commands — one snippet per block. Never wrap a prose report, finding list, checklist, or ASCII box in a fence to align or frame it; write it as normal Markdown (headings, bullets, tables) so it renders cleanly. - **Status icons sparingly.** A check/cross/dot can mark a single headline result, but do not prefix every line with one. Use plain words for severity and outcomes (e.g. `High`, `PASS`, `0 findings`). The terminal renders icons as calm monochrome glyphs only outside code fences — another reason not to box reports. # Ultimate Reminders diff --git a/src/pythinker_code/config.py b/src/pythinker_code/config.py index 44f48ce7..b90c754e 100644 --- a/src/pythinker_code/config.py +++ b/src/pythinker_code/config.py @@ -240,7 +240,12 @@ def _validate_allowed_domains(cls, value: list[str] | None) -> list[str] | None: "Remove it, or omit allowed_domains entirely to leave web access " "unrestricted." ) - if any(char in cleaned for char in "/: \t"): + if cleaned.strip(".") == "": + raise ValueError( + f"Invalid allowed_domains entry {entry!r}: hostname must contain " + "domain labels, not only dots." + ) + if any(char.isspace() for char in cleaned) or any(char in cleaned for char in "/:"): raise ValueError( f"Invalid allowed_domains entry {entry!r}: use a bare hostname " "like 'example.com', not a URL, path, or host:port." diff --git a/src/pythinker_code/tools/web/_allowlist.py b/src/pythinker_code/tools/web/_allowlist.py index 81f4bf16..6d91ebdb 100644 --- a/src/pythinker_code/tools/web/_allowlist.py +++ b/src/pythinker_code/tools/web/_allowlist.py @@ -4,7 +4,7 @@ def _normalize(entry: str) -> str: - return entry.strip().lstrip(".").lower() + return entry.strip().strip(".").lower() def host_in_allowlist(host: str | None, allowed: list[str] | None) -> bool: diff --git a/src/pythinker_code/tools/web/search.py b/src/pythinker_code/tools/web/search.py index b96e030b..ee80b8b7 100644 --- a/src/pythinker_code/tools/web/search.py +++ b/src/pythinker_code/tools/web/search.py @@ -159,6 +159,9 @@ async def __call__(self, params: Params) -> ToolReturnValue: if dropped: builder.extras(allowlist_filtered=dropped) if not results: + # Structured zero-result signal so the renderer reports "0 + # results" instead of misreading the prose below as one result. + builder.extras(returned_results=0) return builder.ok( f"All {dropped} search result(s) were outside the configured " "web allowlist and have been omitted.", diff --git a/src/pythinker_code/ui/shell/components/markdown.py b/src/pythinker_code/ui/shell/components/markdown.py index 81291b23..804cdd58 100644 --- a/src/pythinker_code/ui/shell/components/markdown.py +++ b/src/pythinker_code/ui/shell/components/markdown.py @@ -404,8 +404,11 @@ def _normalize_table_block(text: str) -> str: tail = text[match.end() :] # The delimiter must start its own line — guards against inline ``|-|``. - line_prefix = head[head.rfind("\n") + 1 :] - if n_cols < 2 or line_prefix.strip() != "": + # Any leading whitespace is the table's indentation (e.g. nested under a + # list item); preserve it when re-emitting so we never promote an + # indented table to top level. + indent = head[head.rfind("\n") + 1 :] + if n_cols < 2 or indent.strip() != "": out += text[: match.end()] text = tail continue @@ -457,10 +460,10 @@ def _normalize_table_block(text: str) -> str: # paragraph), so ensure one before emitting the header. if out and not out.endswith("\n\n"): out += "\n" if out.endswith("\n") else "\n\n" - out += "| " + " | ".join(header_cells) + " |\n" - out += "| " + " | ".join(markers) + " |\n" + out += f"{indent}| " + " | ".join(header_cells) + " |\n" + out += f"{indent}| " + " | ".join(markers) + " |\n" for row in data_rows: - out += "| " + " | ".join(row) + " |\n" + out += f"{indent}| " + " | ".join(row) + " |\n" remainder = "\n".join(tail_lines[consumed:]) if not remainder.strip(): diff --git a/src/pythinker_code/ui/shell/tool_renderers/web.py b/src/pythinker_code/ui/shell/tool_renderers/web.py index e8aee68f..f0f0e906 100644 --- a/src/pythinker_code/ui/shell/tool_renderers/web.py +++ b/src/pythinker_code/ui/shell/tool_renderers/web.py @@ -185,6 +185,14 @@ def _allowlist_filtered_count(result: ToolResultPayload) -> int: return 0 +def _explicit_result_count(result: ToolResultPayload) -> int | None: + """The tool's own result count, if it emitted one (preferred over text).""" + extras_raw = result.details.get("extras") + extras = cast("dict[str, object]", extras_raw) if isinstance(extras_raw, dict) else {} + value = extras.get("returned_results") + return value if isinstance(value, int) and value >= 0 else None + + def _render_search_result( ctx: ToolRenderContext, result: ToolResultPayload ) -> RenderableType | None: @@ -203,7 +211,8 @@ def _render_search_result( return Group(body, fg("muted", f"... ({remaining} more lines, ctrl+o to expand)")) return body - count = _search_result_count(result.text) + explicit = _explicit_result_count(result) + count = explicit if explicit is not None else _search_result_count(result.text) summary = Text() summary.append("Found ", style=tui_rich_style("tool_output")) summary.append(str(count), style=tui_rich_style("tool_title")) diff --git a/tests/core/test_default_agent.py b/tests/core/test_default_agent.py index 1e572611..472ec6f3 100644 --- a/tests/core/test_default_agent.py +++ b/tests/core/test_default_agent.py @@ -275,7 +275,7 @@ async def test_default_agent(runtime: Runtime): - **Tables:** put the header row on its own line, the `|---|---|` delimiter row on the immediately following line (no blank line between them), and one row per line. Never glue a table onto adjacent prose (e.g. `Findings| Col |`) and never cram multiple rows onto one line. Leave a blank line before and after the table. - Prefer a short bullet list over a table when there are only a few items or any cell is long; reserve tables for genuinely tabular data with short cells. -- **Code fences are for code only.** Use triple-backtick blocks (tagged with the language, e.g. ```python, ```toml) solely for source, config, or commands — one snippet per block. Never wrap a prose report, finding list, checklist, or ASCII box in a fence to align or frame it; write it as normal Markdown (headings, bullets, tables) so it renders cleanly. +- **Code fences are for code only.** Use triple-backtick blocks tagged with a language (for example, `python` or `toml`) solely for source, config, or commands — one snippet per block. Never wrap a prose report, finding list, checklist, or ASCII box in a fence to align or frame it; write it as normal Markdown (headings, bullets, tables) so it renders cleanly. - **Status icons sparingly.** A check/cross/dot can mark a single headline result, but do not prefix every line with one. Use plain words for severity and outcomes (e.g. `High`, `PASS`, `0 findings`). The terminal renders icons as calm monochrome glyphs only outside code fences — another reason not to box reports. # Ultimate Reminders diff --git a/tests/tools/test_web_allowlist.py b/tests/tools/test_web_allowlist.py index 6f8a456d..c61e51bf 100644 --- a/tests/tools/test_web_allowlist.py +++ b/tests/tools/test_web_allowlist.py @@ -30,6 +30,9 @@ ("DOCS.Example.COM", [" .Example.com "], True), # Trailing-dot FQDN host. ("docs.example.com.", ["example.com"], True), + # Trailing-dot allowlist *entry* matches a plain host. + ("docs.example.com", ["example.com."], True), + ("example.com", ["example.com."], True), # Multiple entries: match any. ("foo.org", ["example.com", "foo.org"], True), # Empty / None host with a non-empty allowlist is rejected. @@ -53,6 +56,9 @@ def test_web_config_accepts_bare_hostnames() -> None: "example.com/path", # path "example.com:8080", # port "two words.com", # whitespace + ".", # dots-only would normalize to empty (unrestricted) — reject + "..", # dots-only + "ex\nample.com", # newline whitespace must be rejected too ], ) def test_web_config_rejects_malformed_entries(bad_entry: str) -> None: diff --git a/tests/ui_and_conv/test_tui_card_tool_renderers.py b/tests/ui_and_conv/test_tui_card_tool_renderers.py index 37a5b651..7d098077 100644 --- a/tests/ui_and_conv/test_tui_card_tool_renderers.py +++ b/tests/ui_and_conv/test_tui_card_tool_renderers.py @@ -928,6 +928,23 @@ def test_search_no_allowlist_indicator_when_not_filtered(): assert "filtered to allowlist" not in rendered +def test_search_all_results_filtered_reports_zero(): + # When every result is dropped by the allowlist, SearchWeb emits prose plus a + # structured returned_results=0 signal; the renderer must prefer that count + # instead of misreading the one-line prose as a single result. + rendered = _render( + "SearchWeb", + {"query": "python"}, + output=( + "All 2 search result(s) were outside the configured web allowlist " + "and have been omitted." + ), + details={"extras": {"allowlist_filtered": 2, "returned_results": 0}}, + ) + assert "Found 0 results" in rendered + assert "2 filtered to allowlist" in rendered + + # --------------------------------------------------------------------------- # Background tasks # ---------------------------------------------------------------------------