fix: show change preview before approving file-mutating tools in interactive CLI#3181
fix: show change preview before approving file-mutating tools in interactive CLI#3181praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
|
@coderabbitai review |
|
/review |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
MervinPraison
added
pipeline/blocked:ci
✅ Action performedReview finished.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughInteractive approval requests now generate previews for diffs, patches, and write content. Rich and Textual frontends display available previews before presenting approval choices. ChangesInteractive approval previews
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant InteractiveFrontend
participant ApprovalRequest
participant render_change_preview
User->>InteractiveFrontend: initiate approval
InteractiveFrontend->>ApprovalRequest: request.change_preview()
ApprovalRequest->>render_change_preview: pass tool and parameters
render_change_preview-->>ApprovalRequest: return concrete preview or None
ApprovalRequest-->>InteractiveFrontend: return preview
InteractiveFrontend-->>User: display preview and approval choices
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
MervinPraison
added
pipeline/awaiting-merge-gate
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #3181
Phase 1 — Architecture assessment ✅
Phase 2 — Fixes pushed (commit
|
| File | Fix |
|---|---|
frontends/rich_frontend.py |
P1 (crash, valid): Panel received the raw preview; any diff with [/] (ubiquitous in Python) raised rich.errors.MarkupError, uncaught → crashed the approval dialog. Now wrapped in rich.markup.escape(). |
frontends/textual_frontend.py |
P1 (same bug): Textual Static also interprets Rich markup by default — same bracket crash. Escaped the preview via rich.markup.escape(). Greptile only flagged the Rich frontend; I found and fixed the Textual one independently. |
interactive/events.py |
P2 (gap, valid): an edit call carrying only old_string/new_string (no precomputed diff) silently returned None → no preview. Now synthesises a unified diff via difflib.unified_diff; identical old/new → None (no noise). |
Verification
render_change_previewchecked for:edit+diff,editsynth (old/new),write, non-mutating (None), emptyedit(None), identical old/new (None) — all correct.- Confirmed
escape()renders a bracket-containing diff withoutMarkupError. py_compileclean; targeted tests6 passed, 5 skipped. The 12 collection errors are pre-existingpraisonaiagentsimport failures unrelated to this PR.
Skipped / notes
- Greptile's claim that the console backend "already" synthesises a diff from old/new is inaccurate —
approval_backend._render_previewalso returnsNonein that case. I still implemented the synthesis in the interactive layer since it clearly adds value; a follow-up could align the console backend, but that's out of scope for this PR.
Phase 3 — Verdict ✅
Both valid reviewer issues (P1 crash, P2 gap) resolved, plus the parallel Textual crash. Change stays lightweight, wrapper-only, and backward-compatible. Approve — ready to merge once CI is green.
· branch claude/issue-3179-20260719-0852
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/praisonai-code/praisonai_code/cli/interactive/frontends/rich_frontend.py`:
- Around line 182-187: Prevent Rich markup parsing of arbitrary preview content
in both frontend sites: in
src/praisonai-code/praisonai_code/cli/interactive/frontends/rich_frontend.py
lines 182-187, update the preview passed to Panel to use rich.syntax.Syntax for
literal syntax-highlighted rendering; in
src/praisonai-code/praisonai_code/cli/interactive/frontends/textual_frontend.py
lines 121-126, wrap preview with rich.text.Text before passing it to Static.
Preserve the existing panel and widget display behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d72e74df-e911-4632-bd3d-31047f7652c9
📒 Files selected for processing (3)
src/praisonai-code/praisonai_code/cli/interactive/events.pysrc/praisonai-code/praisonai_code/cli/interactive/frontends/rich_frontend.pysrc/praisonai-code/praisonai_code/cli/interactive/frontends/textual_frontend.py
| if preview: | ||
| console.print(Panel( | ||
| preview, | ||
| title="[cyan]Change Preview[/cyan]", | ||
| border_style="cyan", | ||
| )) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Prevent Rich markup injection crashes from arbitrary code.
Both frontends currently pass the raw preview string directly to Panel and Static. Because Rich parses markup tags by default, any file content or diff containing brackets (e.g., def foo(x: list[int]): or arr[i]) will be evaluated as Rich tags, crashing the interactive loop with a StyleSyntaxError or MarkupError.
src/praisonai-code/praisonai_code/cli/interactive/frontends/rich_frontend.py#L182-L187: Wrap the preview inrich.syntax.Syntaxto treat the string as literal code. This prevents the crash and fulfills the PR objective to display a "syntax-highlighted panel".src/praisonai-code/praisonai_code/cli/interactive/frontends/textual_frontend.py#L121-L126: Wrap the preview usingrich.text.Text(preview)to safely pass it as a literal renderable to Textual'sStaticwidget.
🛡️ Proposed fixes
For rich_frontend.py:
- if preview:
- console.print(Panel(
- preview,
- title="[cyan]Change Preview[/cyan]",
- border_style="cyan",
- ))
+ if preview:
+ from rich.syntax import Syntax
+ lexer = "diff" if request.tool_name in ("edit", "apply_patch") else "text"
+ console.print(Panel(
+ Syntax(preview, lexer, word_wrap=True),
+ title="[cyan]Change Preview[/cyan]",
+ border_style="cyan",
+ ))For textual_frontend.py:
- if preview:
- children.append(
- VerticalScroll(
- Static(preview, id="approval-preview"),
- id="approval-preview-scroll",
- )
- )
+ if preview:
+ from rich.text import Text
+ children.append(
+ VerticalScroll(
+ Static(Text(preview), id="approval-preview"),
+ id="approval-preview-scroll",
+ )
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if preview: | |
| console.print(Panel( | |
| preview, | |
| title="[cyan]Change Preview[/cyan]", | |
| border_style="cyan", | |
| )) | |
| if preview: | |
| from rich.syntax import Syntax | |
| lexer = "diff" if request.tool_name in ("edit", "apply_patch") else "text" | |
| console.print(Panel( | |
| Syntax(preview, lexer, word_wrap=True), | |
| title="[cyan]Change Preview[/cyan]", | |
| border_style="cyan", | |
| )) |
| if preview: | |
| console.print(Panel( | |
| preview, | |
| title="[cyan]Change Preview[/cyan]", | |
| border_style="cyan", | |
| )) | |
| if preview: | |
| from rich.text import Text | |
| children.append( | |
| VerticalScroll( | |
| Static(Text(preview), id="approval-preview"), | |
| id="approval-preview-scroll", | |
| ) | |
| ) |
📍 Affects 2 files
src/praisonai-code/praisonai_code/cli/interactive/frontends/rich_frontend.py#L182-L187(this comment)src/praisonai-code/praisonai_code/cli/interactive/frontends/textual_frontend.py#L121-L126
🤖 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/praisonai-code/praisonai_code/cli/interactive/frontends/rich_frontend.py`
around lines 182 - 187, Prevent Rich markup parsing of arbitrary preview content
in both frontend sites: in
src/praisonai-code/praisonai_code/cli/interactive/frontends/rich_frontend.py
lines 182-187, update the preview passed to Panel to use rich.syntax.Syntax for
literal syntax-highlighted rendering; in
src/praisonai-code/praisonai_code/cli/interactive/frontends/textual_frontend.py
lines 121-126, wrap preview with rich.text.Text before passing it to Static.
Preserve the existing panel and widget display behavior.
praisonai-triage-agent
Bot
removed
the
pipeline/blocked:cooldown
|
@claude this PR has merge conflicts with |
praisonai-triage-agent
Bot
added
pipeline/blocked:conflict
|
Claude finished @MervinPraison's task —— View job Merge conflicts resolved — PR #3181
Files resolved (2)Both conflicts were in the interactive approval
Validation
Notes
Branch is clean and up to date with |
…ractive CLI (fixes #3179) Render a unified diff (edit/apply_patch) or truncated new content (write) before prompting for approval in the interactive Rich and Textual frontends, so users approve the concrete change rather than just a tool label. Adds a shared render_change_preview helper and ApprovalRequest.change_preview() in the interactive events module, mirroring approval_backend._render_preview. Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
- rich_frontend/textual_frontend: escape preview via rich.markup.escape so diffs/content containing bracket chars no longer raise MarkupError and crash the approval dialog (Greptile P1) - events.render_change_preview: synthesise a unified diff from old_string/ new_string for edit calls lacking a precomputed diff (Greptile P2) Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
4d544d4 to
5f164d1
Compare
MervinPraison
removed
the
pipeline/blocked:conflict
praisonai-triage-agent
Bot
removed
the
pipeline/blocked:cooldown
1 participant
Fixes #3179
Summary
The interactive Rich and Textual frontends approved a file-mutating tool call from a bare summary (tool name, action type, description) with no preview of the actual change. This makes the interactive frontends display the concrete change — a unified diff when available, otherwise the truncated new content — before prompting, matching the behaviour the console
approval_backend._render_previewalready offers.Changes
interactive/events.py: added a sharedrender_change_preview(tool_name, parameters)helper andApprovalRequest.change_preview()that returns a unified diff foredit/apply_patch(fromdiff/patch) or truncated content forwrite;Nonefor non-mutating tools.frontends/rich_frontend.py: renders the preview in a syntax-highlightedPanel(and plain-text fallback) above the approval options.frontends/textual_frontend.py: renders the preview in a scrollableVerticalScroll(Static(...))in theApprovalDialog.Notes
previewis derived from existingparameters, so event payloads are unchanged.Validation
edit/apply_patch/write,Nonefor non-mutating tools, and truncation for large content.Generated with Claude Code
Summary by CodeRabbit