fix: remove approval countdown timers and add system notifications

[DeJeune]

What this PR does

Before this PR:

• MCP tool approval had a 30-second countdown that auto-executed tools without user consent
• Agent tool approval had a 60-second countdown that auto-expired requests
• No system notification was sent when a tool awaited user approval
• A race condition between streaming status and requestToolConfirmation() could cause the approval card to never appear

After this PR:

• Both MCP and Agent tool approvals wait indefinitely for user action — no auto-execute, no auto-expire
• A system notification is sent when a tool requires approval, ensuring users notice even if the chat is not in focus
• The streaming race condition is fixed via a subscription mechanism (onToolPendingChange) that force-updates the component when the execution layer registers a pending approval

Why we need it and why it was done in this way

The following tradeoffs were made:

• Removed all countdown logic rather than making it configurable, since auto-executing tools without explicit consent is a security concern
• Used a useReducer force-update pattern + listener subscription to bridge the gap between the non-React resolver Map and React component state

The following alternatives were considered:

• Making countdown duration configurable — rejected because any auto-execution without consent is problematic
• Using React state to track pending resolvers — rejected because the resolver Map must live outside React to be accessible from the AI SDK execute callback

Breaking changes

None. Tools that were previously auto-approved via server config remain auto-approved. Only the countdown-based auto-execution/expiration is removed.

Special notes for your reviewer

• userConfirmation.ts: Added onToolPendingChange() subscription and isToolPending() / getPendingToolIds() query functions
• useMcpToolApproval.ts: Subscribes to pending changes, handles streaming status as potentially pending
• useAgentToolApproval.ts: Simplified — removed countdown, waits indefinitely
• mcp.ts: Sends notification via NotificationService when tool requires approval
• useAppInit.ts: Sends notification for agent tool permission requests
• Includes 9 unit tests for the userConfirmation module

Checklist

• PR: The PR description is expressive enough and will help future contributors
• Code: Write code that humans can understand and Keep it simple
• Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
• Upgrade: Impact of this change on upgrade flows was considered and addressed if required
• Documentation: A user-guide update was considered and is present (link) or not required. Check this only when the PR introduces or changes a user-facing feature or behavior.
• Self-review: I have reviewed my own code (e.g., via /gh-pr-review, gh pr diff, or GitHub UI) before requesting review from others

Release note

Remove auto-execution countdown timers from MCP and Agent tool approval. 
Tools now wait indefinitely for user confirmation. 
A system notification is sent when approval is required.

[EurFelux]

Review Summary

Overall this is a well-structured PR with a clear security motivation — removing auto-execution of tools without explicit user consent is the right call. The code changes are clean, the test coverage for userConfirmation is solid, and the onToolPendingChange subscription pattern elegantly bridges the non-React resolver Map with React component state.

Highlights

• Good security posture: Removing countdown-based auto-execution is the right approach
• Clean architecture: The subscription pattern (onToolPendingChange) + useReducer force-update is a well-known React pattern for external state synchronization
• Race condition fix: Detecting pending state during streaming status via isToolPending() is a smart solution
• Good test coverage: 9 tests covering the core userConfirmation module

Issues Found

1. isExpired dead code (useAgentToolApproval): The useMemo with Date.now() and [request] dependency will never re-evaluate over time. Since the main process handles the timeout independently via IPC, this code is effectively dead. Consider simplifying to const isExpired = false with a comment, matching the pattern in useMcpToolApproval.

2. PR description vs implementation (tool-permissions.ts): The description says "wait indefinitely" but the main process uses a 600s timeout. A clarifying comment would help.

3. Unrelated change (PluginDetailModal.tsx): CSS class reorder appears to be from auto-formatter.

None of these are blockers — the PR works correctly in practice. The main suggestion is to clean up the dead isExpired logic to avoid confusing future contributors.

[DeJeune]

Review Summary

Overall a well-motivated change — removing auto-execution countdown timers is the right security call, and the subscription mechanism to fix the streaming race condition is a clever solution. The PR is clean, well-documented, and includes good test coverage for the userConfirmation module.

Significant (2)

1. Empty id guard in useMcpToolApproval — When toolResponse?.id is undefined, id falls back to '', which means the isToolPending('') check and the onToolPendingChange subscription operate on an empty string. Should guard against this.

2. Notification spam for rapid tool calls — Multiple tools firing simultaneously will each send a separate system notification. Consider debouncing or batching.

Minor / Nit (4)

3. Duplicated notification construction — The NotificationService.send() call is nearly identical in mcp.ts and useAppInit.ts. Could be extracted to a shared helper.

4. NotificationService import rename — initialState is less self-documenting than the previous defaultNotificationSettings alias.

5. useReducer force-update — Well-documented pattern, just noting it's intentionally unbounded.

6. Test cleanup for toolIdToNameMap — The confirmSameNameTools test doesn't clean up name mappings, which could leak across suites.

Positives

• Thorough removal of all countdown/expiration plumbing across both MCP and agent paths
• The onToolPendingChange subscription pattern is a clean solution to the React/non-React state bridge
• Good test coverage (9 tests) for the userConfirmation module, covering confirm, cancel, abort signal, subscriptions, and confirmSameNameTools
• i18n keys properly added with [to be translated] markers for non-primary locales
• CI is green

[DeJeune]

Minor: The useReducer force-update pattern is a reasonable bridge between the non-React resolver Map and React state. Worth noting in a brief comment that this counter is intentionally unbounded (it wraps safely due to JS number precision being fine for this use case). The existing comments explain the "why" well — nicely documented.

[DeJeune]

Minor: The confirmSameNameTools test sets up mappings and requests but doesn't clean up toolIdToNameMap entries for id-3 after the test. Since the module-level Map persists across tests, this could leak into other test suites if more tests are added later. Consider adding a clearToolIdToNameMappings() utility or cleaning up in afterEach. Not blocking since the current test suite is self-contained.

[EurFelux]

Review Summary

Well-structured PR with a clear security motivation. The code is clean, well-tested, and the architecture decisions are sound.

Highlights

• Security improvement: Removing countdown-based auto-execution is the right call — no tool should execute without explicit user consent
• Clean race condition fix: The onToolPendingChange subscription + useReducer force-update pattern is an elegant bridge between the non-React resolver Map and React component state
• Good refactoring: cleanupTool() helper eliminates the duplicated cleanup logic across confirmToolAction, cancelToolAction, and the abort listener
• Solid test coverage: 9 tests for the userConfirmation module covering confirm, cancel, abort signal, subscriptions, and confirmSameNameTools
• Notification debouncing: Smart batching of rapid-fire tool approval requests into a single notification
• NotificationService bug fix: The initialState fallback was silently broken before this PR (see inline comment)

One Issue

1. i18n plural hardcoding (inline comment) — ${tools.length} tools breaks localization. Needs a separate i18n key for the plural form.

Minor Notes

• PluginDetailModal.tsx CSS class reorder looks like auto-formatter output — harmless but unrelated to the PR scope
• The self-review comments (useReducer unbounded counter, test cleanup) have been addressed in code — those threads can be resolved

Overall a clean PR. The i18n issue is the only thing worth fixing before merge.

[EurFelux]

i18n issue: The plural form ${tools.length} tools hardcodes English. This will render as Tool "3 tools" requires approval which reads awkwardly even in English, and breaks i18n for other locales.

Consider adding a dedicated plural key:

// en-us.json
"approvalRequiredMultiple": "{{count}} tools require approval"

// userConfirmation.ts
const message =
  tools.length === 1
    ? i18n.t('message.tools.approvalRequired', { tool: tools[0] })
    : i18n.t('message.tools.approvalRequiredMultiple', { count: tools.length })

Or use i18next's built-in pluralization with _one / _other suffix keys.

[EurFelux]

Good catch — the previous code used initialState (the entire settings object) as the fallback, aliased misleadingly as defaultNotificationSettings. So when store.getState().settings.notification was falsy, notificationSettings[notification.source] would look up e.g. initialState['assistant'] which is undefined, silently disabling all fallback notifications.

Now correctly falls back to initialState.notification. Nice fix 👍

[GeorgeDong32]

Review Summary

This is a well-structured PR with a clear security motivation — removing auto-execution of tools without explicit user consent is the right approach.

✅ Highlights

• Security improvement: Removing countdown-based auto-execution (30s MCP, 60s Agent) eliminates the risk of unauthorized tool execution
• Clean architecture: The onToolPendingChange subscription pattern elegantly bridges the non-React resolver Map with React component state
• Race condition fix: Detecting pending state during streaming status via isToolPending() is a smart solution
• Notification debouncing: 500ms debounce for sendToolApprovalNotification prevents notification spam
• Unified cleanup: cleanupTool() helper consolidates resource cleanup logic
• Good test coverage: 9 tests covering the core userConfirmation module

📝 Minor Suggestions (non-blocking)

1. Consider adding tests for sendToolApprovalNotification debounce behavior
2. toolIdToNameMap could have residual entries if tools are never confirmed/cancelled (low risk)

Metrics

Dimension	Rating
Security	⭐⭐⭐⭐⭐
Maintainability	⭐⭐⭐⭐
Test Coverage	⭐⭐⭐⭐
Performance	⭐⭐⭐⭐⭐

Recommendation: Ready to merge ✅