Add investment trade and holding support

[shrijayan]

Why

Sure's API already has full support for investment trades (/api/v1/trades) and holdings (/api/v1/holdings) — buying/selling stock, dividends, deposits/withdrawals, interest — but this MCP server only ever wrapped the generic cash /api/v1/transactions endpoint. There was no way to record a stock trade on an investment account through Claude/an MCP client; the only option (create_transaction) has no concept of ticker/quantity/price, so it can't represent a trade correctly.

What's added

7 new tools, following the exact same pattern already used by the transaction tools (get_client() / handle_response() / try-except-log-json.dumps):

Tool	Maps to
get_trades	GET /api/v1/trades (filters: account_id, account_ids, start_date, end_date, paginated)
get_trade	GET /api/v1/trades/:id
create_trade	POST /api/v1/trades — trade_type one of buy/sell/dividend/deposit/withdrawal/interest
update_trade	PATCH /api/v1/trades/:id
delete_trade	DELETE /api/v1/trades/:id
get_holdings	GET /api/v1/holdings (read-only — Sure computes these from trades + prices)
get_holding	GET /api/v1/holdings/:id

All endpoint paths, required params, and per-type validation rules were confirmed directly against Api::V1::TradesController / Api::V1::HoldingsController in we-promise/sure, not guessed.

Trade-type validation and per-type required-field rules (e.g. qty/price for buy/sell, amount for dividend/interest/deposit/withdrawal) are intentionally not duplicated client-side — same as the existing transaction tools, invalid input is left to the Sure API to reject with its own descriptive error message, so this stays in sync automatically if Sure's rules change.

Docs

README.md: added the new tools to the tools table, plus a short "Investment Trades" section documenting which fields are required per trade_type and the investment/crypto-exchange account requirement.

Testing

• get_trades / get_holdings (and their singular counterparts) were run live against a real self-hosted Sure instance with existing investment-account data and correctly returned real trades/holdings.
• Didn't run a live create/update/delete against real portfolio data to avoid touching real financial records during review; the write paths reuse the same request/response handling already proven by the existing create_transaction/update_transaction/delete_transaction tools, with parameters verified against the controller source (linked above).
• python -m py_compile and ruff check --select F,E9 pass on the new code (pre-existing lint/type debt elsewhere in the file — untyped @mcp.tool() decorators, missing mcp/httpx stubs, a few long log lines — is unrelated to this change and left as-is to keep the diff focused).

Note: this branch is also open as a PR against the upstream root repo, robcerda/sure-mcp-server (robcerda#4), since this fork's history forked from there. Happy to close that one if this is the canonical repo going forward.

Summary by CodeRabbit

• New Features
  • Added tools to view, create, update, and delete investment trades.
  • Added tools to retrieve stock holdings, including filtering and pagination.
  • Added support for viewing individual trades and holdings.
• Documentation
  • Documented investment trade fields and account eligibility requirements.
  • Clarified that holdings are read-only and derived from trade history.

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[coderabbitai]

Warning

Review limit reached

@shrijayan, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 53 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: fae34c0e-6a4c-4904-ac04-cee216736c49

📥 Commits

Reviewing files that changed from the base of the PR and between 86148e6 and 3cd3fee.

📒 Files selected for processing (1)

• src/sure_mcp_server/server.py

📝 Walkthrough

Walkthrough

The MCP server adds trade listing, retrieval, creation, updating, and deletion tools, plus read-only holding retrieval tools. The README documents these tools, trade type requirements, and holding limitations.

Changes

Investment data tools

Layer / File(s)	Summary
Trade retrieval tools src/sure_mcp_server/server.py, README.md	Adds filtered trade listing and single-trade retrieval using the trades API, with normalized JSON responses and updated tool documentation.
Trade mutation tools src/sure_mcp_server/server.py, README.md	Adds trade creation, conditional updates, and deletion, and documents required fields by trade type.
Holdings retrieval tools src/sure_mcp_server/server.py, README.md	Adds filtered holdings listing and single-holding retrieval, and documents holdings as read-only data derived from trade history.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant MCPTool
  participant HTTPClient
  participant SureAPI
  MCPTool->>HTTPClient: get_client()
  MCPTool->>SureAPI: Request trades or holdings endpoint
  SureAPI-->>MCPTool: API response
  MCPTool->>MCPTool: handle_response and serialize JSON

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change by describing the new investment trade and holding support.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 2

🤖 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.

Inline comments:
In `@src/sure_mcp_server/server.py`:
- Around line 389-394: Update the trade normalization around the response
handling at src/sure_mcp_server/server.py#L389-L394 and the holdings
normalization at src/sure_mcp_server/server.py#L632-L637 to branch on
isinstance(data, dict) before calling .get(); preserve direct list responses
as-is, while extracting the nested trades or holdings only from dictionaries.
- Line 577: Encode caller-controlled trade_id and holding_id values as single
URL path segments, rejecting or normalizing "." and ".." before encoding. Apply
this to src/sure_mcp_server/server.py lines 410, 554-557, 577, and 653, updating
each affected API request construction while preserving the existing endpoints
and request behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: af990990-e6a0-4ceb-b6f8-a40dbf214553

📥 Commits

Reviewing files that changed from the base of the PR and between d17b4f9 and 86148e6.

📒 Files selected for processing (2)

• README.md
• src/sure_mcp_server/server.py

[shrijayan]

Addressed both actionable CodeRabbit findings in 3cd3fee:

1. Direct list response handling — get_trades/get_holdings now branch on isinstance(data, dict) before calling .get(), so a non-dict response is passed through as-is instead of raising.
2. Path ID encoding — added an encode_path_id() helper (percent-encodes via urllib.parse.quote, rejects "."/".."/empty outright since those survive encoding unchanged) and applied it to trade_id/holding_id in get_trade, update_trade, delete_trade, and get_holding.

Verified with unit checks on encode_path_id (normal UUID passthrough, slash gets %2F-encoded, ./../empty rejected) and a live round-trip (get_trades → get_trade, get_holdings → get_holding) against a real Sure instance.