Skip to content

fix(soul): carry approval cancellation feedback to ApprovalResult - #1979

Merged
wbxl2000 merged 2 commits into
MoonshotAI:mainfrom
wbxl2000:fix/approval-timeout-feedback
Apr 22, 2026
Merged

fix(soul): carry approval cancellation feedback to ApprovalResult#1979
wbxl2000 merged 2 commits into
MoonshotAI:mainfrom
wbxl2000:fix/approval-timeout-feedback

Conversation

@wbxl2000

@wbxl2000 wbxl2000 commented Apr 21, 2026

Copy link
Copy Markdown
Collaborator

Related Issue

Related to #1823 — addresses only the UX/display aspect described in the issue (the Rejected by user misleading message on timeout). Does not resolve #1823; the broader configurable timeout feature request is already being tracked by #1837.

Description

Bug

When the 300 s safety timeout in ApprovalRuntime.wait_for_response fires (e.g. the user stepped away from their SSH/tmux session), _cancel_request writes request.feedback = "approval timed out" to the record before raising ApprovalCancelledError:

# src/kimi_cli/approval_runtime/runtime.py
self._cancel_request(request_id, feedback="approval timed out")
raise ApprovalCancelledError(request_id) from None

But the outer Approval.request was dropping that feedback:

# Before
except ApprovalCancelledError:
    return ApprovalResult(approved=False)

With empty feedback, ApprovalResult.rejection_error() fell back to ToolRejectedError()'s default Rejected by user brief — so users returning to their terminal saw a message implying they had manually rejected the tool call, when they had just walked away.

Fix

Read the feedback back from the record via the existing public get_request API:

except ApprovalCancelledError:
    record = self._runtime.get_request(request_id)
    return ApprovalResult(approved=False, feedback=record.feedback if record else "")

After the fix, the rejection surface reads:

  • Brief: Rejected: approval timed out
  • Message: The tool call is rejected by the user. User feedback: approval timed out

The if record else "" fallback preserves pre-fix behavior for the theoretical record is None case, and the read-from-record approach keeps all cancel paths uniform without inflating ApprovalCancelledError with state.

Tests

  • Extended test_approval_runtime_wait_for_response_times_out to assert record.feedback == "approval timed out" after timeout (guards the upstream invariant the fix depends on).
  • New regression test test_approval_request_timeout_carries_feedback_to_result exercising Approval.request through the cancellation path; asserts both ApprovalResult.feedback round-trips and rejection_error().brief == "Rejected: approval timed out". Verified locally that reverting the fix makes this test fail with AssertionError: assert '' == 'approval timed out'.

Checklist


Open in Devin Review

Copilot AI review requested due to automatic review settings April 21, 2026 14:03
When the 300s safety timeout in wait_for_response fires (e.g. the user
steps away from their SSH session), _cancel_request sets
request.feedback="approval timed out" on the record before raising
ApprovalCancelledError. The outer Approval.request was dropping that
feedback when constructing the ApprovalResult, so the downstream
ToolRejectedError fell back to the generic "Rejected by user" brief --
leaving users returning to their terminal unable to distinguish a
timeout from a manual rejection.

Read record.feedback back through the existing get_request API and pass
it into ApprovalResult. The rejection surface now reads
"Rejected: approval timed out" on the timeout path, and stays
backward-compatible for any cancel path that doesn't set feedback.

Adds:
- Assertion on the existing runtime timeout test that record.feedback is
  persisted as "approval timed out"
- New regression test driving Approval.request through the cancellation
  path and asserting both ApprovalResult.feedback and
  rejection_error().brief surface the reason

Related to MoonshotAI#1823 (only addresses the UX/display aspect; the broader
configurable-timeout feature request is tracked by MoonshotAI#1837).

This comment was marked as resolved.

@wbxl2000
wbxl2000 force-pushed the fix/approval-timeout-feedback branch from 6a5f126 to 214a609 Compare April 21, 2026 14:07

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 2 additional findings.

Open in Devin Review

@wbxl2000

Copy link
Copy Markdown
Collaborator Author

BEFORE
image

AFTER
image

@wbxl2000
wbxl2000 merged commit 4dc1764 into MoonshotAI:main Apr 22, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NOT ACTUALLY CLOSED: Feature Request: Configurable Approval Request Timeout (or Unlimited)

3 participants