Skip to content

fix(approvals): an approval card's reason is never the tool's instructions for the model - #404

Merged
itsskofficial merged 2 commits into
mainfrom
fix/398-approval-reason
Oct 16, 2026
Merged

itsskofficial merged 2 commits into
mainfrom
fix/398-approval-reason

Conversation

@itsskofficial

@itsskofficial itsskofficial commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Closes #398

What changes

approval_request.reason used to be f"{plugin}: {tool.description[:160]}": the tool's instructions for the model, cut off mid-word ("browser: Click the element with this ref (from browser_snapshot), e.g. "e12". Buttons that buy, pay, send, post, delete or confirm need the user's approval. Never type p").

It's now approval_reason(wording, risk) in sentient/tools/base.py:

  • When the tool's describe_fn gives a label that says more than the risk alone, the reason is that label. Examples: "Purchase", "Deletes", "Posts publicly" and "Signs up" from the browser, "Unlocks a door" from Home Assistant, "Takes a photo" from devices.
  • Otherwise the reason is "", and the card writes its own line ("... using browser_click").
  • It is never the tool's description.

That covers every tool that reaches this path. Built-in plugin tools, integrations and MCP tools all went through the same line, and now none of them show their model-facing description.

Side effect on the heading (desktop approvalRisk.ts, untouched here). The card guesses its heading from the tool name plus reason. The old description text held "buy, pay, send, post, delete", so a send-risk browser click always read Purchase. Now it reads "Sends", unless the browser's own label says Purchase, Deletes or Posts publicly. That label now arrives in reason, so the heading still matches it.

The one thing lost: an MCP tool whose purpose shows only in its description (not its name) gets the generic heading for its risk. Open PR #367 moves headings to risk_label, which is the long-term fix.

Changes are limited to the engine reason line, docs/API.md (the approval_request row) and CHANGELOG. ApprovalCard.tsx and approvalRisk.ts aren't touched, because #367 changes them. The doc comment in approvalRisk.ts still says reason is the tool's description. #367 or a follow-up can update it.

Tests

  • New tests/test_approval_reason.py:
    • approval_reason keeps only labels that say more than the risk.
    • A real run_turn with a plugin tool whose description is model text and whose describe_fn labels the call. The approval requests carry reason "" for a plain send and "Purchase" for "Place order", and the model text appears nowhere in the event.
  • tests/test_agent_loop.py, tests/browser, Home Assistant and terminal tests pass. Full suite: 2372 passed, 2 skipped. Ruff is clean, and desktop typecheck and build are clean.

Real test

Built app from this branch (npm run build), fresh SENTIENT_HOME, models claude-code/sonnet (fast claude-code/haiku). The app was driven over a DevTools port this run opened (9351), on its own window only. The page was local: http://127.0.0.1:8786/, "Maya's book club", with a "Send message" button.

  • I asked: "Open http://127.0.0.1:8786/ in the browser and click the Send message button to post my note to the book club." Sonnet called browser_open, then asked in words whether to send. I answered "Yes, click Send message now."

  • The approval card read, in full:

    Sentient wants to send something on your behalf [Sends]
    Click "Send message"
    Clicked using browser_click
    Sentient read content from Web Browser in this chat, so it checks with you before sending anything.
    Allow once · Deny

    There's no model text in it ("Click the element with this ref", "Never type ..." are gone). The badge says "Sends", not "Purchase".

  • I pressed Deny. The step shows "Browser step not done · You declined: Clicking", and the reply is "That didn't go through, the click was declined, so the message was not sent."

Screenshots, which I looked at: 398-approval-card (the card above) and 398-after-deny (the declined step and the reply).

Notes for the owner

Summary by CodeRabbit

  • Bug Fixes
    • Approval requests show a short, meaningful label such as “Purchase” or “Unlocks a door” when available. Otherwise, they show no reason rather than displaying tool descriptions or model instructions.
  • Documentation
    • Clarified that the approval reason is a plain label, not a tool description, and is empty when it adds no information beyond the risk level.

…tions for the model

The reason is the describe_fn label when it says more than the risk (Purchase,
Unlocks a door), else empty so the card writes its own line.
@github-actions github-actions Bot added documentation Improvements or additions to documentation area: engine Agent loop, models, approvals, storage, gateway labels Oct 16, 2026
@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: cb30382a-71d2-4f61-a2b6-99858dc83f68
📥 Commits

Reviewing files that changed from the base of the PR and between 7262a8e and 3508081.

📒 Files selected for processing: 2
  • CHANGELOG.md
  • sentient/agent/loop.py
🚧 Files skipped from review as they are similar to previous changes: 1
  • CHANGELOG.md

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

📝 Walkthrough

Approval requests now use a short, non-default risk label as their reason. If the label is missing or matches the effective risk’s default, the reason is empty. Tests check helper behavior and confirm serialized approval requests omit tool descriptions.

Changes: Approval reason formatting

Layer / File(s) Summary
Select a concise approval reason
sentient/tools/base.py, tests/test_approval_reason.py
Added approval_reason to return a trimmed risk label only when it differs from the effective risk’s default label. Tests cover default, missing, and custom labels.
Use the label in approval requests
sentient/agent/loop.py, tests/test_approval_reason.py, docs/API.md, CHANGELOG.md
Approval requests now derive reason from call wording and risk, rather than the tool name and description. Tests check request fields and confirm serialized requests omit description text. The API documentation and changelog describe the reason behavior.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 35080

Approval cards will use concise labels or their existing fallback instead of model-facing tool descriptions. Some desktop headings may be less specific when purpose appeared only in a description, but no material user-facing failure is established.

Security architecture risk: ⚪ Minimal · up to 35080

Approval explanations now exclude model-facing tool instructions. The approval scope and decision controls remain unchanged. Some tools may display a more generic heading, but the review found no material security risk introduced by this change.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The affected scope is approval presentation for tool calls passing through Agent._ask. The change does not expand which tools are reachable or the privileges represented by an approval.

Trust Boundaries and Controls

  • observed — Risk, purchase-sensitive choice selection, warnings, untrusted-content context, and approval identity remain separate from reason. Desktop rendering retains independent action details and server-provided decision choices even when reason becomes empty or a heading becomes generic.

Resilience and Maintainability Implications

  • observed — The change leaves the approval state sequence intact: create a request with offered choices, emit it, wait using the same request and session identity, record the decision, and populate a declined outcome for non-allowing decisions. No retry, recovery, cleanup, or concurrency behavior is changed by the reason-only substitution.
Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 3 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly describes the main change: approval card reasons no longer use model-facing tool instructions.
Linked Issues check Passed Issue #398 requires approval reasons that do not expose model-facing tool descriptions. The PR replaces the description-based reason with approval_reason(wording, risk). The function returns an empt…
Out of Scope Changes check Passed The changes remain within issue #398. They update approval reason generation, add focused approval-flow tests, and update the related API documentation and changelog. These changes directly support th…
Full details: Linked Issues check

Explanation

Issue #398 requires approval reasons that do not expose model-facing tool descriptions. The PR replaces the description-based reason with approval_reason(wording, risk). The function returns an empty string for the default risk label and a short describe_fn label when it adds information. The new approval-flow test checks model instructions in the tool description, expected reasons for send and purchase actions, and serialized approval requests. The reported real-app browser check shows no model instructions in the approval card.

Full details: Out of Scope Changes check

Explanation

The changes remain within issue #398. They update approval reason generation, add focused approval-flow tests, and update the related API documentation and changelog. These changes directly support the approval-reason objective. No unrelated product change is identified.

Full details: Docstring Coverage

Explanation

Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 3 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

A rabbit checks the label with care,
No model notes appear in there.
“Purchase” hops onto the card,
While empty reasons stay not hard.
The approval path now reads clear,
A pleased hare nibbles a carrot near.

Comment @coderabbitai help to get the list of available commands.

@itsskofficial
itsskofficial merged commit 4f789e3 into main Oct 16, 2026
8 checks passed
@itsskofficial
itsskofficial deleted the fix/398-approval-reason branch October 16, 2026 16:56
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 16, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area: engine Agent loop, models, approvals, storage, gateway documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Approval cards show the tool's instructions to the model as the reason

1 participant