feat: tool-call dedup plus shell/terminal hardening and editor fixes - #116
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements cross-step and same-step tool-call deduplication with toolset and soul wiring and tests; extracts and appends command stderr tails to briefs and renders error briefs as plain text; makes terminal cursor probe non-blocking and restores VMIN/VTIME; refactors shell welcome UI (ASCII/motion-aware) and MCP timing; renames slash command ChangesDeduplication & runtime fixes
Terminal & cursor probe
Shell welcome, MCP, and UI
Command brief tail & worklog
Slash command rename and docs
App welcome facts and misc docs/comments
Sequence Diagram(s)(omitted — primary new control flow is internal dedup handling and local UI/TTY adjustments; a detailed multi-actor sequence diagram is unnecessary.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tasks/todo.md`:
- Line 144: Markdown has fenced code blocks missing surrounding blank lines at
the locations where the snippet shows "```bash" (notably around the block
starting at the snippet and similar blocks at lines 150 and 156); fix by adding
a blank line immediately before and after each fenced code block so there is an
empty line between the preceding paragraph/list item and the opening "```bash"
and another empty line after the closing "```", ensuring the code fences in the
tasks/todo.md (the blocks starting with "```bash") follow markdownlint
expectations.
- Around line 143-159: Update tasks/todo.md to replace the outdated guidance
about renaming pythinker_cli_session with current pythinker_code_session usage
(remove the suggested sed command that targets pythinker_cli_session and instead
reference pythinker_code_session where relevant), remove or adjust the proposed
telemetry regex change (do not add
r"^(.*?)(site-packages|pythinker_code|src/pythinker_code)/" since telemetry
paths are already handled in src/pythinker_code/telemetry/sentry.py), and re-run
Markdown lint to fix fencing/blank-line formatting around that section so the
fenced code blocks and checklist items conform to the repo markdown rules.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4bca9608-e3f9-4faf-8d0a-20fe9c5b490e
📒 Files selected for processing (1)
tasks/todo.md
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
tasks/todo.md (4)
100-100:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale mapping entry.
Line 100 maps
pythinker_cli_sessiontopythinker_code_sessionas if it's future work, but the code showspythinker_code_sessionis already the current attribute name. Remove this entry or update the document to reflect that this rename is complete.🤖 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 `@tasks/todo.md` at line 100, Remove the stale mapping line that maps pythinker_cli_session to pythinker_code_session in the tasks/todo.md table; since the code already uses pythinker_code_session, either delete the entire row or change its wording to indicate the rename is complete (e.g., mark as "renamed" or remove the mapping), ensuring no reference remains that suggests pythinker_cli_session is still the current attribute.
138-140: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueAdd blank line before fenced code block.
Markdownlint requires a blank line before the code block at line 139. Add a blank line after line 138.
🤖 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 `@tasks/todo.md` around lines 138 - 140, Add a blank line immediately before the fenced code block that begins with "```bash" (the grep command block) so the Markdown has an empty line separating the preceding paragraph and the code fence; update the tasks/todo.md content to insert that single blank line right above the "```bash" fence.
28-34:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPlanning document describes already-completed work.
Lines 28 and 34 present
pythinker_cli_sessionand the telemetry path regex as current problems requiring fixes, but the code shows both have already been renamed:
pythinker_code_sessionis the current attribute name (seesrc/pythinker_code/web/store/sessions.py:67andsrc/pythinker_code/web/runner/worker.py:32).- The telemetry regex already includes
pythinker_codeandsrc/pythinker_code(seesrc/pythinker_code/telemetry/sentry.py:38).Either update the document status to reflect that Phase 1–3 work is complete, or revise these sections to describe only the remaining work.
🤖 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 `@tasks/todo.md` around lines 28 - 34, Update the planning document to reflect that the renaming and telemetry regex changes are already done: replace references to the old attribute name pythinker_cli_session with the current pythinker_code_session (and note the locations using symbol pythinker_code_session such as sessions.py and worker.py), and mark item 4 as completed or revise its description to reference the updated telemetry regex that already includes pythinker_code and src/pythinker_code (symbol: the regex string used in sentry.py); alternatively, reword items 1 and 4 to describe any remaining follow-up work instead of presenting them as unresolved issues.
139-146:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVerification steps assume completed rename.
The grep command at line 139 expects "empty output" (line 142) and line 144-146 verifies
pythinker_code_sessionusage — both assume the rename is already done. This contradicts the document status "DO NOT execute yet" (line 3).If this is a planning document, these steps should describe the work to be done (e.g., "Run sed to replace pythinker_cli_session"). If the rename is complete, update the document status to reflect that Phases 1–3 are done.
🤖 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 `@tasks/todo.md` around lines 139 - 146, The verification steps incorrectly assume the rename is already complete; update the doc to either (A) change the status from "DO NOT execute yet" to "Phases 1–3 completed" if the rename from "pythinker_cli" to "pythinker_code" and attribute rename to "pythinker_code_session" is already finished, or (B) make the steps actionable by adding exact commands and intent (e.g., run a sed/grep replace for "pythinker_cli" -> "pythinker_code" and then grep to confirm no matches) and explicitly instruct to check references like joint_session.pythinker_code_session and session.pythinker_code_session in web API and worker files; ensure the document text clearly reflects which path (execute now vs already done) is correct.
🤖 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.
Duplicate comments:
In `@tasks/todo.md`:
- Line 100: Remove the stale mapping line that maps pythinker_cli_session to
pythinker_code_session in the tasks/todo.md table; since the code already uses
pythinker_code_session, either delete the entire row or change its wording to
indicate the rename is complete (e.g., mark as "renamed" or remove the mapping),
ensuring no reference remains that suggests pythinker_cli_session is still the
current attribute.
- Around line 138-140: Add a blank line immediately before the fenced code block
that begins with "```bash" (the grep command block) so the Markdown has an empty
line separating the preceding paragraph and the code fence; update the
tasks/todo.md content to insert that single blank line right above the "```bash"
fence.
- Around line 28-34: Update the planning document to reflect that the renaming
and telemetry regex changes are already done: replace references to the old
attribute name pythinker_cli_session with the current pythinker_code_session
(and note the locations using symbol pythinker_code_session such as sessions.py
and worker.py), and mark item 4 as completed or revise its description to
reference the updated telemetry regex that already includes pythinker_code and
src/pythinker_code (symbol: the regex string used in sentry.py); alternatively,
reword items 1 and 4 to describe any remaining follow-up work instead of
presenting them as unresolved issues.
- Around line 139-146: The verification steps incorrectly assume the rename is
already complete; update the doc to either (A) change the status from "DO NOT
execute yet" to "Phases 1–3 completed" if the rename from "pythinker_cli" to
"pythinker_code" and attribute rename to "pythinker_code_session" is already
finished, or (B) make the steps actionable by adding exact commands and intent
(e.g., run a sed/grep replace for "pythinker_cli" -> "pythinker_code" and then
grep to confirm no matches) and explicitly instruct to check references like
joint_session.pythinker_code_session and session.pythinker_code_session in web
API and worker files; ensure the document text clearly reflects which path
(execute now vs already done) is correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a4f184a6-d6c2-4b24-90af-98a78f34d190
📒 Files selected for processing (1)
tasks/todo.md
- Shell error briefs now surface the trailing output of a failed command (last non-empty lines rendered as plain text), so the collapsed worklog card explains why a command failed instead of only its exit code. - Subagents no longer receive plan-mode workflow reminders; the reminder is root-only, since subagent toolsets usually exclude the plan-mode tools and the injection only invited calls to tools they don't have. - The terminal cursor-position probe no longer risks hanging in raw mode on exit: reads are non-blocking during the probe and VMIN/VTIME are restored to canonical defaults so a cancelled probe can't wedge the tty. - Reorder the welcome info block so session/storage details render last, and log the auto-save path failure instead of swallowing it silently.
Identical tool calls in one step now share the original task's result instead of re-executing. Across steps, repeats are detected via canonical JSON arguments (key order no longer defeats matching) and nudged with sparse system-reminders at consecutive streaks of 3, 5, and 8, reducing loop-thrash without hard-blocking legitimate retries. Per-step state is armed inside the step-retry wrapper so a retried step never awaits tasks cancelled by the failed attempt, and a D-Mail revert clears the dedup seed since the reverted history no longer contains those calls. Adds tool_call_dedup_detected telemetry plus a dup_type property on tool_call events.
The pythinker-cli -> pythinker-code rename plan is fully realized (root pyproject is pythinker-code, module is src/pythinker_code/, no pythinker_cli references remain), so retire it from tasks/todo.md. Review nits: annotate /reports with NoReturn and assert the injection counter stays untouched in the subagent plan-mode suppression test.
soul/toolset: append the cross-step dedup reminder inline at the tool task's return instead of behind a second wrapper task. Behaviour is unchanged (toolset tests pass) but the returned task is now the tool task itself, with one fewer task and closure per reminder. Drop the now-unused default on _append_reminder_to_return_value and annotate begin_step/end_step with the existing ToolCallKey alias. tools/utils: tail() docstring no longer over-specifies the buffer as stderr (it holds mixed command output); rename stripped -> rstripped to reflect that it holds an rstrip() result.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/tools/shell/__init__.py (2)
194-207: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winGeneric exception handler should include tail for consistency.
For consistency with the timeout and exit-code paths (and with the ACP Terminal tool), include the tail in the exception brief when partial output was captured before the failure. The
if tail:guard makes this safe even when no output exists.Proposed fix
except Exception as e: from pythinker_code.telemetry.errors import report_handled_error report_handled_error(e, site="tool.shell.exec", tool="Shell") logger.error( "Shell command execution failed: {command}: {error}", command=params.command, error=e, ) + brief = "Execution failed" + tail = builder.tail() + if tail: + brief += f"\n{tail}" return builder.error( f"Command execution failed: {e}", - brief="Execution failed", + brief=brief, status=ToolResultStatus.error, )🤖 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/tools/shell/__init__.py` around lines 194 - 207, The generic exception handler in the shell execution path should append the captured output tail to the brief (like the timeout/exit-code branches) for consistency; update the except Exception as e block (the handler that calls report_handled_error and builder.error) to include the tail in the brief string when tail is non-empty (use an if tail guard) so the returned builder.error brief includes partial output before the failure while keeping the existing message and status (ToolResultStatus.error) and leaving the error log call using params.command and e unchanged.
188-193: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winTimeout handler should include tail for consistency.
The ACP Terminal tool includes
tailin its timeout brief (line 157 inacp/tools.py), and this file's own exit-code path includes it (lines 179-186). At timeout, the builder holds partial output from thestdout_cb/stderr_cbcallbacks, so the tail would surface the last captured lines before the kill — actionable context the user/model won't see otherwise.Proposed fix
except TimeoutError: + tail = builder.tail() + brief = f"Killed by timeout ({params.timeout}s)" + if tail: + brief += f"\n{tail}" return builder.error( f"Command killed by timeout ({params.timeout}s)", - brief=f"Killed by timeout ({params.timeout}s)", + brief=brief, status=ToolResultStatus.cancelled, )🤖 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/tools/shell/__init__.py` around lines 188 - 193, The TimeoutError handler currently returns builder.error(f"Command killed by timeout ({params.timeout}s)", ...) without the captured output tail; update the TimeoutError except block to include the last-captured lines (append builder.tail) in both the main message and the brief, mirroring how the exit-code path does it and how the ACP Terminal tool does at timeouts; keep references to builder.error, params.timeout, and include the builder.tail content gathered via the stdout_cb/stderr_cb callbacks in both message and brief.
🤖 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 `@src/pythinker_code/soul/toolset.py`:
- Around line 230-243: The _make_reminder_text_2 function currently inlines
canonical_args verbatim into the reminder; change it to include a bounded
preview instead (e.g., take the first N characters or first line(s) and append
an ellipsis) so large payloads (like WriteFile bodies) aren’t echoed fully; keep
the full canonical_args identity via the existing hash/telemetry fields already
emitted elsewhere. Update references in _make_reminder_text_2 to use the preview
variable when composing the f"- arguments: ..." line and ensure the reminder
still preserves the hash/telemetry for exact identification.
In `@tests/core/test_toolset.py`:
- Around line 452-468: Replace assertions that inspect private internals
(_previous_step_calls, _current_step_calls, _current_step_tasks) in
test_begin_end_step with observable-behavior checks: keep using begin_step(...)
and then call end_step() to assert the returned calls list is correct, assert
ts.dedup_triggered reflects the deduplication state after end_step(), and add an
explicit scenario that submits a duplicate call (e.g., repeat ("ToolA","{}")
across steps) to verify duplicate handling via end_step() return and
dedup_triggered toggling instead of reading private attributes.
---
Outside diff comments:
In `@src/pythinker_code/tools/shell/__init__.py`:
- Around line 194-207: The generic exception handler in the shell execution path
should append the captured output tail to the brief (like the timeout/exit-code
branches) for consistency; update the except Exception as e block (the handler
that calls report_handled_error and builder.error) to include the tail in the
brief string when tail is non-empty (use an if tail guard) so the returned
builder.error brief includes partial output before the failure while keeping the
existing message and status (ToolResultStatus.error) and leaving the error log
call using params.command and e unchanged.
- Around line 188-193: The TimeoutError handler currently returns
builder.error(f"Command killed by timeout ({params.timeout}s)", ...) without the
captured output tail; update the TimeoutError except block to include the
last-captured lines (append builder.tail) in both the main message and the
brief, mirroring how the exit-code path does it and how the ACP Terminal tool
does at timeouts; keep references to builder.error, params.timeout, and include
the builder.tail content gathered via the stdout_cb/stderr_cb callbacks in both
message and brief.
🪄 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 Plus
Run ID: 1ade6d66-d6f5-4e2c-a082-3fa3d67b4fb4
📒 Files selected for processing (40)
CHANGELOG.mddocs/en/reference/pythinker-vis.mddocs/en/reference/slash-commands.mdsrc/pythinker_code/acp/tools.pysrc/pythinker_code/app.pysrc/pythinker_code/auth/openai/models.pysrc/pythinker_code/auth/openai/oauth_client.pysrc/pythinker_code/auth/platforms.pysrc/pythinker_code/cli/__init__.pysrc/pythinker_code/hooks/runner.pysrc/pythinker_code/llm.pysrc/pythinker_code/session_cleanup.pysrc/pythinker_code/soul/dynamic_injections/plan_mode.pysrc/pythinker_code/soul/pythinkersoul.pysrc/pythinker_code/soul/toolset.pysrc/pythinker_code/tools/shell/__init__.pysrc/pythinker_code/tools/utils.pysrc/pythinker_code/ui/color_utils.pysrc/pythinker_code/ui/shell/__init__.pysrc/pythinker_code/ui/shell/components/markdown.pysrc/pythinker_code/ui/shell/mcp_status.pysrc/pythinker_code/ui/shell/motion.pysrc/pythinker_code/ui/shell/prompt.pysrc/pythinker_code/ui/shell/render_constants.pysrc/pythinker_code/ui/shell/slash.pysrc/pythinker_code/ui/shell/stats_pricing.pysrc/pythinker_code/ui/shell/usage_adapters/openai_chatgpt.pysrc/pythinker_code/ui/shell/visualize/_worklog.pysrc/pythinker_code/ui/terminal_background.pysrc/pythinker_code/ui/terminal_capabilities.pysrc/pythinker_code/ui/theme.pysrc/pythinker_code/utils/path.pysrc/pythinker_code/utils/term.pytasks/todo.mdtests/core/test_plan_mode_injection_provider.pytests/core/test_toolset.pytests/ui_and_conv/test_shell_switch_slash.pytests/ui_and_conv/test_shell_welcome_info.pytests/ui_and_conv/test_worklog_render.pytests/utils/test_result_builder.py
Bound the canonical arguments echoed in the strong dedup reminder to a 256-char preview so large-payload tools (WriteFile, MultiEdit) don't re-inject their whole body into context on every repeat; exact identity is still carried by the args_hash dedup telemetry. Rewrite test_begin_end_step to assert observable behaviour (handle()/end_step()/dedup_triggered) instead of poking private _current_step_* internals.
# Conflicts: # CHANGELOG.md # tasks/todo.md
elkaix
changed the title
handle() returns Task[ToolResult] | ToolResult; assert isinstance Task before awaiting, matching the other dedup tests, so pyright's check job passes.
7 tasks
Merged
5 tasks
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Brings
chore/clean-todoup to date withmainand bundles the work that accumulated on it:soul/toolset.py,soul/pythinkersoul.py): identical calls within a step share one result; cross-step repeats are detected via canonical-JSON args and nudged with sparse reminders at consecutive streaks of 3/5/8. Echoed reminder args are bounded to a 256-char preview (identity is carried by the dedup telemetry hash).tasks/todo.md; CodeRabbit review nits (telemetry eventreports_opened, capability comment,tail()doc/naming, observable-behavior test).main(#117) was merged in to resolve drift; onlyCHANGELOG.mdandtasks/todo.mdconflicted (docs/tracking — resolved keeping both changelog sets and the cleaned todo).Verification
Dedup +
/goalsuites green locally on the merged tree (62 tests); lint/format clean. Full CI runs on the branch.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests