feat: harden benchmark runs and stabilize TUI prompt bar#196
Conversation
… prompt After submitting a prompt the input card's top border (──────── ● off) + ❯ was fossilized above the streamed content as a second, ghostly prompt. The turn's first scrollback commit runs `run_in_terminal`, whose teardown erase-height drifts on the first transition into streaming and leaves the card behind; suppressing the card only *during* the handoff is too late (the erase runs before the repaint). - Hide the input card from turn-start until the turn's first commit, then repaint it below the live stream so the user can still see where to steer. Two windows are covered: a `_turn_starting` hint set the instant a turn is dispatched (the pre-attach gap where the resumed prompt can repaint before the running-prompt delegate exists), and, once attached, the delegate reporting `running_prompt_hide_input_card()` until it commits. - Wire the hint once, inside `run_soul_command` (the single funnel for all five dispatch paths), before the first await lets the resumed prompt repaint. - `clear_turn_starting()` is the public counterpart to `mark_turn_starting()`, used by the shell's error-path cleanup instead of writing the private `_turn_starting` attribute directly. - Tests: a pyte (VT100 emulator) e2e asserting the card never fossilizes above the stream and returns mid-turn once content commits (documented as a manual/local check — it is skipped on CI); a CI-runnable renderer-direct test that the chrome renderer actually consults the gate; unit tests for the gate + hint lifecycle, including the new public clear method. pyte is a new dev/test dependency.
# Conflicts: # CHANGELOG.md
…ning-prompt-input-card-collapse # Conflicts: # CHANGELOG.md
Adds an opt-in fullscreen Focus TUI (tui.focus_mode) that pins the composer, hides file activity by default behind a shelf, and renders live agent output without terminal scrollback jumps. Wires the new model/surface pair through shell and visualize so streamed turns can opt in alongside the existing diff-rendered scene mode. Covered with focused unit + integration tests for the model, the surface, the running-prompt scene regression, the config default, the shell PTY prompt layout, and the wire helper for Focus TUI init/close events.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds native benchmark commands and artifacts, a focus-mode TUI with sticky prompt/input handling and file activity tracking, and active-skill reminders that persist across turns. It also updates docs, config defaults, packaging inputs, and test coverage. ChangesNative Benchmark Subsystem
Estimated code review effort: 4 (Complex) | ~75 minutes Focus TUI, Sticky Prompt Input, and File Activity
Estimated code review effort: 4 (Complex) | ~70 minutes Active Skill Persistence
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SlashCommand
participant BenchmarkCommands
participant BenchmarkRunner
participant BenchmarkRecorder
participant PythinkerSoul
SlashCommand->>BenchmarkCommands: dispatch_benchmark("/benchmark start")
BenchmarkCommands->>BenchmarkRecorder: start_run()
BenchmarkCommands->>BenchmarkRunner: run_task(task, soul)
BenchmarkRunner->>PythinkerSoul: turn(prompt)
BenchmarkRunner->>BenchmarkRunner: run verification subprocess
BenchmarkRunner->>BenchmarkRecorder: record_event(), finish_run()
BenchmarkRecorder-->>BenchmarkCommands: summary.json, report.md
BenchmarkCommands-->>SlashCommand: formatted run summary
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pythinker_code/ui/shell/__init__.py (1)
1420-1525: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRe-arm
mark_turn_starting()for drained turns. The queued-turn loop dispatchesrun_soul()again without setting the pre-attach hint, so a repaint betweendetach_running_prompt()and the nextvisualize()attach can bring back the stale-card flicker on turn 2+. Add the sameprompt_session.mark_turn_starting()guard before each drained dispatch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pythinker_code/ui/shell/__init__.py` around lines 1420 - 1525, The drained-turn loop in the shell UI is starting additional `run_soul()` turns without re-arming the pre-attach hint, which can allow stale-card repaint flicker between detach and the next `visualize()` attach. Update the queued-message path around `captured_view.drain_queued_messages()` and the repeated `run_soul()` call to invoke `prompt_session.mark_turn_starting()` before each drained dispatch, matching the initial turn setup so turn 2+ behaves consistently.src/pythinker_code/ui/shell/visualize/_interactive.py (1)
439-480: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore the input card after the first post-commit flush
src/pythinker_code/ui/shell/visualize/_interactive.py:470-478, 951-975only clears_awaiting_input_card_restore_anchoron a later anchored batch, so a turn with one tool commit and trailing prose can keeprunning_prompt_hide_input_card()true until the turn ends. Clear the restore flag on the next successful flush, not only after a second anchored one.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pythinker_code/ui/shell/visualize/_interactive.py` around lines 439 - 480, The input card restore flag is only being cleared on a later anchored flush, which can leave the prompt hidden after the first successful post-commit flush. Update the scrollback flush flow in the pending scroller handling so the next successful flush after `_committed_scrollback_this_turn` sets `_awaiting_input_card_restore_anchor` back to false, rather than waiting for a second anchored batch; use the existing `_pending_scrollback`, `_committed_scrollback_this_turn`, and `_awaiting_input_card_restore_anchor` logic to place the reset in the successful flush path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 141-143: The SWE-style fixture example is missing the required
dataset argument, so update the `/benchmark:swe` usage text to include the
`--dataset` flag alongside `--trusted-dataset true`. Make sure the README
example matches the actual command contract used by `/benchmark:swe` and any
related documentation so users see a valid invocation rather than one that will
be rejected.
In `@src/pythinker_code/benchmark/discovery.py`:
- Around line 37-38: The DiscoveredBenchmarkTask.to_json_line helper is unused
and should be removed from discovery.py. Delete the to_json_line method from the
DiscoveredBenchmarkTask class, and make sure any imports that exist only to
support it, such as json or asdict if no longer needed elsewhere in the module,
are also cleaned up. Use the DiscoveredBenchmarkTask symbol in
src/pythinker_code/benchmark/discovery.py to locate the change.
- Around line 93-108: Re-check the final destination host in _fetch_text before
reading the response body, because urllib.request.urlopen can follow redirects
and bypass the _ALLOWED_HOSTS gate. Update the benchmark discovery fetch flow to
either use a no-redirect opener or inspect the final response URL/host after
urlopen and reject any non-allowlisted redirect target before calling
response.read().
In `@src/pythinker_code/benchmark/records.py`:
- Line 82: The finish_run method currently uses Any for result, which hides
strict type errors on attributes like status, verification, and activity. Update
BenchmarkRunRecords.finish_run to use the concrete BenchmarkResult type instead,
and avoid the circular import by importing BenchmarkResult only under
TYPE_CHECKING from the runner module. Keep the runtime behavior unchanged while
restoring pyright strict checking for this method.
- Around line 17-25: Reject path separators in benchmark run IDs by validating
or normalizing the CLI-provided run_id before it is used to build run_dir in
Records.__init__ and before load_run() reads artifacts. Ensure run_id cannot
contain path components such as ../ or absolute paths, either by accepting only
the expected generated ID format or by stripping any directory separators and
rejecting invalid values. Keep the fix localized to the run_id handling used by
Records and load_run so all benchmark artifact paths remain confined under root.
In `@src/pythinker_code/benchmark/redact.py`:
- Around line 8-16: Secret redaction in redact.py is missing several common
credential shapes, so expand the _SECRET_PATTERNS list to cover generic
password/passwd assignments, AWS access keys, PEM private key blocks, and
JWT-like tokens. Update the existing redact flow that uses these patterns so
benchmark artifacts written by this module keep these secrets out of trace.jsonl
and report.md, and ignore the unrelated ast-grep jsonify hint.
In `@src/pythinker_code/benchmark/report.py`:
- Around line 10-99: The reported PLR0912 branch-count warning is a false
positive for this project because Ruff does not enable PL/PLR rules, so no
functional change is required in render_run_report. If you want to improve
readability anyway, extract the activity-line construction block from
render_run_report into a small _activity_lines(activity) helper and keep the
existing behavior unchanged.
- Around line 67-69: The estimated cost formatting in render_run_report is doing
an unguarded float() cast on usage["estimated_cost_usd"], which can raise
ValueError during report generation. Update the estimated_cost handling in this
function to validate that usage is a dict and that estimated_cost_usd is numeric
before converting it, and fall back to "unavailable" for any missing or invalid
value. Keep the fix localized to the estimated_cost block so render_show
continues to degrade gracefully like the other summary fields.
In `@src/pythinker_code/benchmark/runner.py`:
- Around line 299-333: The wire-log JSONL parsing logic is duplicated across
_count_wire_tool_calls, _last_wire_usage, _json_object, and the similar parser
in activity.py, so schema handling can drift. Extract a shared wire message
iterator/helper (for example, a single _iter_wire_messages(wire_file, offset) or
equivalent) that owns the seek/decode/validation steps, then update these
consumers to build on it and keep message.type/payload extraction in one place.
- Around line 87-94: The timeout handling in runner.py is inconsistent:
`effective_timeout` is computed with a falsy fallback, so an explicit `0`
override is ignored, and `_run_verification` still derives its own timeout from
`task.limits.timeout_seconds` instead of the caller’s override. Update
`runner.py` so the timeout calculation in the main run path uses an explicit
None check, and pass `effective_timeout` through to `_run_verification` (and any
nested timeout logic there) so the verification subprocess, `asyncio.wait_for`,
and recorded environment metadata all use the same value.
- Around line 25-31: Exclude VCS metadata from benchmark snapshots by updating
runner._snapshot_files() to skip .git content, since workspace.files from swe
fixtures can create .git/** entries that would otherwise be captured in
before/after snapshots and pollute changed_files. Add .git to _GENERATED_DIRS or
apply an explicit .git filter in _snapshot_files(), keeping the existing
handling for cache directories and suffixes in runner.py.
In `@src/pythinker_code/benchmark/tasks.py`:
- Around line 105-121: The workspace-file validation logic is duplicated between
the file parsing loop here and the `_workspace_files` implementation in
`swe.py`, so update this module to expose a shared helper such as
`parse_workspace_files()` and have both call sites use it. Keep the existing
unsafe-path and text-content checks in one place, and make sure
`MalformedBenchmarkTaskError` remains the single error path used by both
`tasks.py` and `swe.py`.
- Around line 44-46: The _bundled_tasks_root() helper currently forces
resources.files("pythinker_code.benchmark.bundled.tasks") into a Path via str(),
which is not the documented-safe way to get a real filesystem path. Update this
code to use importlib.resources.as_file() (or otherwise materialize the
Traversable safely) before converting to Path, and keep the change localized
around _bundled_tasks_root() so it works for both unpacked and non-filesystem
package layouts.
In `@src/pythinker_code/soul/dynamic_injections/active_skills.py`:
- Around line 98-100: The `_asks_to_deactivate_skill` regex is using `\b` in a
way that can falsely match a shorter skill name inside a hyphenated longer one.
Update the matching logic so a skill name only matches when it is followed by a
true separator/end-of-name, not a hyphenated continuation, and keep the existing
verb pattern in place. Use `_asks_to_deactivate_skill` and the
`_DEACTIVATE_VERBS`/`skill_name` matching path to locate the fix, and add a
regression test covering two active skills where one name is a hyphen-prefix of
the other.
In `@src/pythinker_code/ui/shell/focus_surface.py`:
- Around line 70-85: create_application() is still hardcoding an 80-column
render width, which can clip or pad the UI in terminals of other sizes. Update
the FormattedTextControl callback to read the current application/terminal width
at render time and pass that width into renderer_text(), matching the width
handling used by render_running_prompt_body(). Keep the change localized to
create_application() and the renderer_text call site.
In `@src/pythinker_code/ui/shell/prompt.py`:
- Around line 4013-4022: clear_turn_starting() updates the prompt state but
never repaints, so the shell can keep showing a stale collapsed prompt on the
error-before-attach path. Update clear_turn_starting() in prompt.py to trigger
the same redraw path as mark_turn_starting() by calling invalidate() after
changing _turn_starting and _set_running_fullscreen(False), so the prompt bar is
refreshed immediately when run_soul_command clears the hint in its finally
block.
- Around line 3453-3478: The turn-start hidden-input branch in prompt rendering
drops the prompt marker for non-card style because it returns early from
`_input_card_hidden_pre_stream()` without appending `PROMPT_SYMBOL_AGENT_INPUT`.
Update the `prompt.py` logic around the `is_card_style()` check so the classic
style path still emits the same marker as the normal fallback, using the same
rendering flow in `mark_turn_starting()` / `_render_input_top_border()` /
`_thinking_prompt_prefix_style()` as needed. Keep the hidden-card chrome
behavior, but ensure both card and non-card styles show the prompt symbol during
the pre-stream window.
In `@src/pythinker_code/ui/shell/tui/scene.py`:
- Around line 15-23: The render method in scene.py is skipping blank body lines
because of the `if line:` check, which collapses multi-line content and breaks
line count alignment. Update `Scene.render` to preserve empty lines from
`self.body.splitlines()` by padding and appending them as well, while keeping
the existing border/prompt rendering intact. Use the `render` method and
`pad_line` flow as the place to fix this.
In `@src/pythinker_code/ui/shell/visualize/_interactive.py`:
- Around line 481-494: There is inconsistent guarding around
_pending_scrollback_anchors in the Interactive shell emit paths, which can cause
AttributeError when _emit_final_scrollback, _emit_action_block, or
_print_turn_recap run before the list exists. Update these emit sites in
_interactive.py to use a shared helper such as _append_pending_scrollback so
_pending_scrollback and _pending_scrollback_anchors are always updated together,
and keep the hasattr fallback in one place if initialization is still needed.
In `@src/pythinker_code/ui/shell/visualize/_live_view.py`:
- Around line 875-888: The tool-name filter in _tool_call_path duplicates the
mutating-tool list already maintained as _MUTATING_TOOL_NAMES, so replace the
inline lowercase set with that shared constant. Check that _MUTATING_TOOL_NAMES
in _blocks.py covers the same tool names, then import and reuse it in
_live_view.py so _tool_call_path and _changed_paths_from_sub_call stay aligned
when write-capable tools change.
In `@tests/core/test_benchmark_runner.py`:
- Around line 226-251: The current test only verifies that the timeout override
is passed into collect_benchmark_environment, so it misses the real bug in
_run_verification. Update test_run_task_records_timeout_override_for_environment
to also monkeypatch runner.subprocess.run and assert that
run_task/_run_verification passes the overridden effective timeout via
kwargs["timeout"], rather than task.limits.timeout_seconds. Use the existing
runner.subprocess.run pattern from
test_run_verification_does_not_use_login_shell so the test fails if verification
ignores the override.
In `@tests/ui_and_conv/test_tui_renderer.py`:
- Around line 62-78: Add a `RunningPromptScene` test that covers a body
containing an internal blank line, using the existing `RunningPromptScene` and
`render` assertions in `test_tui_renderer.py`. The new test should verify that a
body like “a\n\nb” preserves the blank line in the rendered output instead of
dropping it, matching the intended behavior for the scene’s body rendering
logic.
---
Outside diff comments:
In `@src/pythinker_code/ui/shell/__init__.py`:
- Around line 1420-1525: The drained-turn loop in the shell UI is starting
additional `run_soul()` turns without re-arming the pre-attach hint, which can
allow stale-card repaint flicker between detach and the next `visualize()`
attach. Update the queued-message path around
`captured_view.drain_queued_messages()` and the repeated `run_soul()` call to
invoke `prompt_session.mark_turn_starting()` before each drained dispatch,
matching the initial turn setup so turn 2+ behaves consistently.
In `@src/pythinker_code/ui/shell/visualize/_interactive.py`:
- Around line 439-480: The input card restore flag is only being cleared on a
later anchored flush, which can leave the prompt hidden after the first
successful post-commit flush. Update the scrollback flush flow in the pending
scroller handling so the next successful flush after
`_committed_scrollback_this_turn` sets `_awaiting_input_card_restore_anchor`
back to false, rather than waiting for a second anchored batch; use the existing
`_pending_scrollback`, `_committed_scrollback_this_turn`, and
`_awaiting_input_card_restore_anchor` logic to place the reset in the successful
flush path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 89fef507-eea2-459b-9433-7cb3405e9006
⛔ Files ignored due to path filters (11)
docs/.vitepress/config.tsis excluded by!docs/**docs/AGENTS.mdis excluded by!docs/**docs/en/customization/architecture.mdis excluded by!docs/**docs/en/reference/pythinker-benchmark.mdis excluded by!docs/**docs/en/reference/slash-commands.mdis excluded by!docs/**docs/en/release-notes/changelog.mdis excluded by!docs/**docs/superpowers/plans/2026-07-05-full-pi-tui-renderer-port.mdis excluded by!docs/**docs/superpowers/specs/2026-07-05-full-pi-tui-renderer-port-design.mdis excluded by!docs/**tasks/lessons.mdis excluded by!tasks/**tasks/todo.mdis excluded by!tasks/**uv.lockis excluded by!**/*.lock
📒 Files selected for processing (74)
.gitignoreCHANGELOG.mdREADME.mdpyproject.tomlsrc/pythinker_code/agents/default/system.mdsrc/pythinker_code/benchmark/__init__.pysrc/pythinker_code/benchmark/activity.pysrc/pythinker_code/benchmark/bundled/__init__.pysrc/pythinker_code/benchmark/bundled/suites/__init__.pysrc/pythinker_code/benchmark/bundled/suites/pythinker-core.jsonsrc/pythinker_code/benchmark/bundled/suites/pythinker-smoke.jsonsrc/pythinker_code/benchmark/bundled/tasks/__init__.pysrc/pythinker_code/benchmark/bundled/tasks/core-atomic-transfer.jsonsrc/pythinker_code/benchmark/bundled/tasks/core-dedup-order.jsonsrc/pythinker_code/benchmark/bundled/tasks/core-explicit-none-metadata.jsonsrc/pythinker_code/benchmark/bundled/tasks/core-safe-path-join.jsonsrc/pythinker_code/benchmark/bundled/tasks/smoke-add-small-function.jsonsrc/pythinker_code/benchmark/bundled/tasks/smoke-edit-readme.jsonsrc/pythinker_code/benchmark/bundled/tasks/smoke-fix-python-test.jsonsrc/pythinker_code/benchmark/commands.pysrc/pythinker_code/benchmark/compare.pysrc/pythinker_code/benchmark/discovery.pysrc/pythinker_code/benchmark/environment.pysrc/pythinker_code/benchmark/errors.pysrc/pythinker_code/benchmark/estimate.pysrc/pythinker_code/benchmark/export.pysrc/pythinker_code/benchmark/records.pysrc/pythinker_code/benchmark/redact.pysrc/pythinker_code/benchmark/report.pysrc/pythinker_code/benchmark/runner.pysrc/pythinker_code/benchmark/suites.pysrc/pythinker_code/benchmark/swe.pysrc/pythinker_code/benchmark/tasks.pysrc/pythinker_code/benchmark/types.pysrc/pythinker_code/config.pysrc/pythinker_code/soul/dynamic_injections/active_skills.pysrc/pythinker_code/soul/pythinkersoul.pysrc/pythinker_code/soul/slash.pysrc/pythinker_code/ui/shell/__init__.pysrc/pythinker_code/ui/shell/focus_model.pysrc/pythinker_code/ui/shell/focus_surface.pysrc/pythinker_code/ui/shell/prompt.pysrc/pythinker_code/ui/shell/tui/__init__.pysrc/pythinker_code/ui/shell/tui/components.pysrc/pythinker_code/ui/shell/tui/diff.pysrc/pythinker_code/ui/shell/tui/scene.pysrc/pythinker_code/ui/shell/tui/scheduler.pysrc/pythinker_code/ui/shell/tui/width.pysrc/pythinker_code/ui/shell/visualize/__init__.pysrc/pythinker_code/ui/shell/visualize/_blocks.pysrc/pythinker_code/ui/shell/visualize/_interactive.pysrc/pythinker_code/ui/shell/visualize/_live_view.pysrc/pythinker_code/utils/pyinstaller.pytests/core/test_active_skill_injection.pytests/core/test_benchmark_activity.pytests/core/test_benchmark_compare.pytests/core/test_benchmark_discovery.pytests/core/test_benchmark_records.pytests/core/test_benchmark_runner.pytests/core/test_benchmark_slash.pytests/core/test_benchmark_swe.pytests/core/test_benchmark_tasks.pytests/core/test_config.pytests/e2e/test_shell_pty_prompt_layout_e2e.pytests/ui_and_conv/test_focus_tui_integration.pytests/ui_and_conv/test_focus_tui_model.pytests/ui_and_conv/test_focus_tui_surface.pytests/ui_and_conv/test_prompt_tips.pytests/ui_and_conv/test_shell_run_placeholders.pytests/ui_and_conv/test_tui_renderer.pytests/ui_and_conv/test_visualize_running_prompt.pytests/utils/test_pyinstaller_utils.pytests_e2e/test_wire_protocol.pytests_e2e/wire_helpers.py
Summary
Related Issue
No linked issue.
Verification
make check-pythinker-codemake test-pythinker-codeuv run pytest tests/core/test_benchmark_discovery.py tests/core/test_benchmark_runner.py tests/core/test_benchmark_activity.py tests/core/test_benchmark_redact.py tests/core/test_active_skill_injection.py tests/core/test_benchmark_tasks.py tests/core/test_benchmark_swe.py tests/ui_and_conv/test_tui_renderer.py tests/ui_and_conv/test_focus_tui_surface.py tests/ui_and_conv/test_focus_tui_model.py tests/ui_and_conv/test_visualize_running_prompt.py -q --inline-snapshot=disableUV_PROJECT_ENVIRONMENT=.venv-312-smoke uv run --python 3.12.13 python - <<'PY' ...to confirm slash-command descriptions normalize on Python 3.12Checklist
Summary by CodeRabbit
/benchmarkslash commands (including:swe) with suites/tasks, compare/estimate/list/show/report, allowlisted discovery, and JSON/CSV result exports.