fix: capture denial reason in interactive CLI approvals (reject-with-correction)#3180
fix: capture denial reason in interactive CLI approvals (reject-with-correction)#3180praisonai-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 responses now support optional denial reasons. Rich prompts for and trims rejection feedback, while Textual accepts an optional reason and normalizes blank values before returning the response. ChangesInteractive denial feedback
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant InteractiveFrontend
participant ApprovalResponse
User->>InteractiveFrontend: Reject approval request
InteractiveFrontend->>User: Prompt for optional denial reason
User-->>InteractiveFrontend: Enter reason or blank input
InteractiveFrontend->>ApprovalResponse: Create rejection with normalized reason
🚥 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 SummaryThis PR adds an optional denial reason to the interactive approval flow so users can steer the agent when rejecting a tool call. The Rich (REPL) frontend is fully wired — a new
Confidence Score: 4/5Safe to merge; the Rich/REPL frontend change is correct and complete, and the Textual TUI gap is intentionally deferred and documented. The Rich frontend delivers the full reject-with-correction loop correctly. The Textual TUI's handle_approval_key still posts denial without a reason, so users of the full-screen TUI get no steering benefit from this change — the same gap noted in the previous review thread remains open and unaddressed in code, even though it is documented in the PR. tool_panel.py (handle_approval_key) and textual_frontend.py (ApprovalDialog.on_button_pressed) — the reason collection scaffolding is present but the Textual Y/N key path never populates it. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant RichFrontend
participant ApprovalResponse
participant InteractiveCore
User->>RichFrontend: selects "5 - Reject"
RichFrontend->>User: prompt: "Reason for denial (optional):"
User->>RichFrontend: types reason (or blank / Ctrl+C)
RichFrontend->>RichFrontend: _prompt_deny_reason() returns str or None
RichFrontend->>ApprovalResponse: "ApprovalResponse(decision=REJECT, reason=...)"
ApprovalResponse->>InteractiveCore: response delivered
InteractiveCore->>InteractiveCore: tool_execution.py returns reason to model
Note over User,InteractiveCore: Textual TUI path (Y/N keys)
User->>ToolPanelWidget: presses N
ToolPanelWidget->>ToolPanelWidget: handle_approval_key - reason not collected
ToolPanelWidget->>InteractiveCore: "ApprovalResponse(approved=False, reason=None)"
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User
participant RichFrontend
participant ApprovalResponse
participant InteractiveCore
User->>RichFrontend: selects "5 - Reject"
RichFrontend->>User: prompt: "Reason for denial (optional):"
User->>RichFrontend: types reason (or blank / Ctrl+C)
RichFrontend->>RichFrontend: _prompt_deny_reason() returns str or None
RichFrontend->>ApprovalResponse: "ApprovalResponse(decision=REJECT, reason=...)"
ApprovalResponse->>InteractiveCore: response delivered
InteractiveCore->>InteractiveCore: tool_execution.py returns reason to model
Note over User,InteractiveCore: Textual TUI path (Y/N keys)
User->>ToolPanelWidget: presses N
ToolPanelWidget->>ToolPanelWidget: handle_approval_key - reason not collected
ToolPanelWidget->>InteractiveCore: "ApprovalResponse(approved=False, reason=None)"
Reviews (3): Last reviewed commit: "fix: make Textual denial-reason contract..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -155,5 +158,6 @@ def on_button_pressed(self, button_id: str) -> ApprovalResponse: | |||
| else: # reject | |||
| return ApprovalResponse( | |||
| request_id=self.request.request_id, | |||
| decision=ApprovalDecision.REJECT | |||
| decision=ApprovalDecision.REJECT, | |||
| reason=(reason.strip() or None) if reason else None, | |||
| ) | |||
There was a problem hiding this comment.
on_button_pressed now accepts reason, but no caller in the codebase passes a value for it — so it is always None. The real TUI approval flow goes through tool_panel.py's handle_approval_key (Y/N key press), not through ApprovalDialog.on_button_pressed at all. There is also no new UI element (e.g. an Input widget or a follow-up prompt) to collect the reason from the user before calling this method. As a result, the "steer agent on denial" feature is silently absent for the Textual frontend despite the PR description saying it threads the reason in.
|
@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 #3180 ✅ Approve
Phase 1 — Review per AGENTS.md
Phase 2 — Valid issue found & fixedGreptile P2 is accurate. The Minimal, honest fix pushed (commit
Deliberately skipped (scope creep): adding an Files modified this review
Testing
Phase 3 — Verdict: ✅ Approve / mergeThe Rich frontend deny-and-steer loop is correct, complete, and backward-compatible. The Textual contribution is now honest (documented as pending, with a consistent message contract) rather than misleadingly presented as active. No core changes, no scope creep, no regressions. --- • branch |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/praisonai-code/praisonai_code/cli/interactive/frontends/textual_frontend.py (1)
127-163: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a Textual prompt for the reject reason
src/praisonai-code/praisonai_code/cli/interactive/frontends/textual_frontend.py:84-163
ApprovalDialog.compose()only renders buttons, so the Textual path has no way to collect the optional denial reason thaton_button_pressed()already accepts. As a result, reject always falls back to a blank reason.🤖 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/textual_frontend.py` around lines 127 - 163, The Textual approval dialog must collect an optional rejection reason before invoking on_button_pressed. Update ApprovalDialog.compose() to render a suitable input prompt for the reject reason, and wire the reject interaction to pass the entered, optionally blank value into on_button_pressed while preserving existing behavior for once, always, and session decisions.
🤖 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.
Outside diff comments:
In
`@src/praisonai-code/praisonai_code/cli/interactive/frontends/textual_frontend.py`:
- Around line 127-163: The Textual approval dialog must collect an optional
rejection reason before invoking on_button_pressed. Update
ApprovalDialog.compose() to render a suitable input prompt for the reject
reason, and wire the reject interaction to pass the entered, optionally blank
value into on_button_pressed while preserving existing behavior for once,
always, and session decisions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd01a0f2-6a41-4332-ba4a-fdf6fa79b1fe
📒 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
- tool_panel.py ApprovalResponse message now carries optional reason (backward-compatible default None), matching the interactive events ApprovalResponse contract for the live y/n approval path. - Clarify ApprovalDialog.on_button_pressed docstring: the live Textual TUI approves/denies via the tool_panel y/n handler and does not yet collect a reason, so the param is not misleadingly presented as active. Addresses Greptile P2 on PR #3180. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
MervinPraison
added
pipeline/blocked:stale-final
praisonai-triage-agent
Bot
added
pipeline/blocked:conflict
|
@claude this PR has merge conflicts with |
|
Claude finished @MervinPraison's task —— View job Rebase onto main & resolve conflicts ✅
Conflict resolved
Resolution (minimal): kept main's new menu structure intact — The other three files ( SkippedNothing — no scope creep introduced. No Verification
Force-pushed |
fixes #3177) Add an optional reason field to the interactive ApprovalResponse and prompt for a one-line correction on Reject in the Rich and Textual frontends, mirroring the console approval backend. Empty input preserves plain denial. Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
- tool_panel.py ApprovalResponse message now carries optional reason (backward-compatible default None), matching the interactive events ApprovalResponse contract for the live y/n approval path. - Clarify ApprovalDialog.on_button_pressed docstring: the live Textual TUI approves/denies via the tool_panel y/n handler and does not yet collect a reason, so the param is not misleadingly presented as active. Addresses Greptile P2 on PR #3180. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
38195d9 to
b076806
Compare
MervinPraison
added
pipeline/blocked:cooldown
praisonai-triage-agent
Bot
removed
the
pipeline/blocked:cooldown
1 participant
Fixes #3177
Summary
Interactive Rich/Textual approval rejects were silent — the agent only learned a call was denied, with no way to say why, so it retried or stalled. The console backend already captures a denial reason; this brings the same "deny and steer" loop to the flagship interactive frontends.
Changes
interactive/events.py: add optionalreason: Optional[str] = NonetoApprovalResponse(and toto_dict()).frontends/rich_frontend.py: reject branch now prompts for an optional one-line reason via a new_prompt_deny_reason()helper (mirrorsapproval_backend._prompt_deny_reason). Blank input preserves today's plain-denial behaviour.frontends/textual_frontend.py:ApprovalDialog.on_button_pressedaccepts an optionalreasonand threads it into the rejectApprovalResponse.The reason maps to core's existing
ApprovalDecision.reason, whichtool_execution.py:1693already returns to the model as the denied tool's result — no core changes needed.Testing
python -m pytest tests/unit/test_interactive_edit_tools.py→ 11 passed.ApprovalResponsereason field round-trips (populated and defaultNone).Generated with Claude Code
Summary by CodeRabbit