Skip to content

Address review findings on Sage Agents over MCP - #1015

Closed
Rigidity wants to merge 9 commits into
mainfrom
rigidity/review-loop-pr-1005
Closed

Rigidity wants to merge 9 commits into
mainfrom
rigidity/review-loop-pr-1005

Conversation

@Rigidity

Copy link
Copy Markdown
Collaborator

Follow-up to #1005 (Sage Agents over MCP, stage 1). That PR is from a fork, so this branch continues from its head (111d90b) and adds review fixes on top. It includes #1005's commits, so merging this one also lands #1005.

What changed

Apps database reset

  • Take the data directory lock before reporting an apps database failure. A second Sage instance now says "already running" instead of offering to delete the database the first instance is using. Deleting it would lose un-checkpointed WAL data.
  • reset_apps_database takes the lock again before it deletes anything.
  • A database migrated by a newer Sage (sqlx VersionMissing) is shown as a plain startup error, with no delete offered.
  • The delete is gated with cfg(not(mobile)), since sage-apps isn't a dependency on Android and iOS. Without the gate, the mobile builds didn't compile.

Agent call path

  • Grants and wallet scope are checked in one function, authorize_agent_call. It runs at the MCP boundary in call_tool, so the server tests cover the real check, and again in process_agent. tools.sage() checks the scope a third time while it holds the Sage lock.
  • process_agent takes the catalog tool directly and returns a Value. This removes the JSON string round trip, the second lookup by method name and the allowlist of error codes, which turned unlisted codes into internal_error.
  • The watch-only check is now one helper (ensure_can_sign) that apps and agents share.

Agents database

  • Pruning deleted wallets from agent scopes now updates only the scope, and only if the scope hasn't changed since it was read. Before, it wrote back the whole agent as it read it, so an edit saved in between could be lost, including a capability the user had just removed.
  • apps_list_agents reads the table once per poll instead of twice.

UI

  • The agent settings reuse SettingsSection, the warning Alert and a ServerStatusIndicator shared with the RPC settings.
  • The port check and the client snippets moved to src/lib/agentPermissions.ts, with tests.
  • Removed the unused showMoreLabel prop from PermissionEditor.

Cleanup

  • Dropped the unused http dependency.
  • Removed the step-by-step implementation plans in docs/superpowers/plans/.

New tests

  • The authorization of agent calls, including switching to a wallet outside the scope, logging out, a method missing from the registry, and grants for secret or signing capabilities.
  • The exact Host header responses, including local hosts with a port.
  • The wallet scope of the app caller.
  • The agents migration on a database that already has settings.
  • The newer-version detection and which failures offer the reset.
  • Sage::lock_data_dir.
  • The two-step reset confirmation in StartupError.

Decisions to confirm

  • Database from a newer Sage: no delete is offered, because installing that version again recovers the apps and agents. The alternative is to offer the delete behind the same warning: drop the newer_version branch in AppsDbFailure.
  • docs/superpowers/: the plans were removed and the two design specs kept. The alternatives are to drop the specs too, or to move the MCP design spec to something like docs/agents-design.md.
  • Scope: the apps database reset from 111d90b stays in this PR rather than being split out.

Known gaps

  • wallet.getXchUsdPrice holds the Sage lock for the whole CoinGecko request, up to 10s. Main already does this for webview apps and for the Sage UI's own command, so the fix belongs in the shared endpoint layer.
  • The mobile builds weren't compiled locally. The mobile CI job checks them.

Checks

These all pass:

  • pnpm prettier:check, pnpm lint, pnpm exec tsc -b and pnpm test:unit.
  • cargo clippy --workspace --all-features --all-targets -- -D warnings, cargo fmt --check, cargo test --workspace --all-features and cargo machete.
  • pnpm build.

pnpm generate:bindings and pnpm generate:app-docs leave the tree unchanged.

The next release needs pnpm extract, since this adds new Lingui strings.

dkackman and others added 9 commits November 14, 2026 15:59
- Introduced a new document outlining the design for Sage Agents interacting with wallets via the Model Context Protocol (MCP).
- Implemented the AgentSettings component to manage agent server settings, including adding, editing, and revoking agents.
- Integrated capabilities management for agents, allowing users to specify wallet scopes and capabilities.
- Added UI elements for displaying agent status, capabilities, and token management.
- Established a polling mechanism to keep agent server status updated in real-time.
- Added `AgentPermissionsPanel` component to manage agent capabilities and wallet scopes using `PermissionEditor` and `WalletScopeEditor`.
- Updated `tailwind.config.js` to include paths for `sage-app-ui` components.
- Created a new schema file for OpenAPI definitions in Rust.
- Developed tests for the new `AgentPermissionsPanel` and related functionalities.
- Defined utility functions for handling agent permissions and wallet scopes in `agentPermissions.ts`.
- Documented the implementation plan and design specifications for the agent permissions UI.
- Introduced a mechanism to drop deleted wallets from agents' scopes, ensuring that agents only retain access to existing wallets.
- Enhanced error reporting for agent server startup failures, providing clearer feedback on why the server could not start.
- Updated the agent settings UI to display error messages related to agent server startup issues.
- Refactored bridge methods to utilize a new `sage` method for accessing wallet state, improving code clarity and reducing redundancy.
- Added tests to verify the correct behavior of wallet retention logic and agent capabilities.
- Improved pluralization in UI messages for better user experience.
…base

- Take the data directory lock before reporting an apps database failure, so a second
  instance reports "already running" instead of offering to delete the live database
- Take the lock again in reset_apps_database before deleting any files
- Don't offer the reset when a newer version of Sage migrated the database, since
  installing that version again recovers the apps and agents
- Test that the agents migration keeps settings in an existing database
- Run agent tool calls straight from the catalog tool, without a JSON round trip, a
  second lookup by method name, or an allowlist of error codes
- Test the authorization of agent calls, including a method missing from the registry
- Prune deleted wallets from agent scopes without overwriting a newer edit
- Test that secret and signing grants expose no tools, and the exact Host header checks
- Test the app caller's wallet scope in the bridge context
- Move the port check and client snippets into agentPermissions and test them
- Test that the apps database reset needs a second confirmation
- Reuse SettingsSection and the warning Alert in the agent settings
- Drop the unused http dependency and showMoreLabel prop, and the implementation plans
- Keep the apps database reset out of mobile builds, which have no sage-apps crate
- Test which apps database failures offer the reset
- Authorize agent calls in one place, and share the watch-only check with apps
- Return the pruned agents from retain_agent_wallets instead of listing them twice
- Share the server status indicator between the RPC and agent settings
- Keep the agent dialog's wallets as WalletInfo, null until loaded
…eal check

- Check grants and wallet scope in call_tool, with the current wallet from the executor,
  before the executor runs; the bridge still checks again when it runs the method
- Test that switching to a wallet outside an agent's scope, or logging out, refuses calls
- Compute the dialog's loaded wallets once
@Rigidity

Copy link
Copy Markdown
Collaborator Author

These commits were pushed onto #1005's branch instead, so the review fixes land in that PR. The description above still lists what they change and the decisions to confirm.

@Rigidity Rigidity closed this Nov 15, 2026
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.

2 participants