Add MCP tool annotations#81
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughA new helper module is introduced to centralize MCP tool annotation creation with two functions: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #81 +/- ##
=======================================
Coverage 97.66% 97.66%
=======================================
Files 19 20 +1
Lines 3121 3130 +9
=======================================
+ Hits 3048 3057 +9
Misses 73 73
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/test_tool_annotations.py (1)
32-46: Consider parametrizingidempotentin the helper.
_assert_mutating_toolhard-codesidempotentHint is False, which couples the helper to the current set of callsites. If any mutating tool is later markedidempotent=True(e.g., updates — see comment ontransactions.py), this helper will need a kwarg. A small forward-looking change keeps things symmetric withdestructive:♻️ Proposed change
def _assert_mutating_tool( tool: Tool, title: str, *, destructive: bool = False, + idempotent: bool = False, ) -> None: """Assert standard annotations for mutating tools.""" annotations = tool.annotations assert annotations is not None assert annotations.title == title assert annotations.readOnlyHint is False assert annotations.destructiveHint is destructive - assert annotations.idempotentHint is False + assert annotations.idempotentHint is idempotent assert annotations.openWorldHint is False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_tool_annotations.py` around lines 32 - 46, The helper _assert_mutating_tool currently hard-codes annotations.idempotentHint as False; add an idempotent: bool = False parameter to _assert_mutating_tool, update its docstring to mention idempotency, and replace the hard-coded assertion with assert annotations.idempotentHint is idempotent so callers can opt-in when a mutating tool is idempotent.src/lampyrid/tools/transactions.py (1)
142-163: Consider marking updates as idempotent.Per the MCP
idempotentHintsemantics ("calling repeatedly with the same arguments has no additional effect on the environment"),update_transactionandbulk_update_transactionsare idempotent — applying the same payload twice produces the same end state. Themutating_annotationshelper already exposes this flag but no callsite sets it. This will require updating the test expectations (_assert_mutating_toolcurrently hard-assertsidempotentHint is Falsefor all mutating tools).♻️ Proposed change
`@transactions_mcp.tool`( tags={'transactions', 'manage'}, - annotations=mutating_annotations('Update Transaction'), + annotations=mutating_annotations('Update Transaction', idempotent=True), ) async def update_transaction(req: UpdateTransactionRequest) -> Transaction: @@ `@transactions_mcp.tool`( tags={'transactions', 'manage', 'bulk'}, - annotations=mutating_annotations('Bulk Update Transactions'), + annotations=mutating_annotations('Bulk Update Transactions', idempotent=True), ) async def bulk_update_transactions(req: BulkUpdateTransactionsRequest) -> BulkUpdateResult:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lampyrid/tools/transactions.py` around lines 142 - 163, The mutating tools update_transaction and bulk_update_transactions should be marked idempotent via the existing mutating_annotations helper: change the calls to mutating_annotations('Update Transaction') and mutating_annotations('Bulk Update Transactions') to pass the idempotent flag (e.g., mutating_annotations('Update Transaction', idempotent=True) and mutating_annotations('Bulk Update Transactions', idempotent=True)); also update the test helper _assert_mutating_tool to stop hard-asserting idempotentHint is False (make it accept a configurable expectation or assert True for these tools) so the tests reflect the idempotent semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lampyrid/tools/transactions.py`:
- Around line 142-163: The mutating tools update_transaction and
bulk_update_transactions should be marked idempotent via the existing
mutating_annotations helper: change the calls to mutating_annotations('Update
Transaction') and mutating_annotations('Bulk Update Transactions') to pass the
idempotent flag (e.g., mutating_annotations('Update Transaction',
idempotent=True) and mutating_annotations('Bulk Update Transactions',
idempotent=True)); also update the test helper _assert_mutating_tool to stop
hard-asserting idempotentHint is False (make it accept a configurable
expectation or assert True for these tools) so the tests reflect the idempotent
semantics.
In `@tests/unit/test_tool_annotations.py`:
- Around line 32-46: The helper _assert_mutating_tool currently hard-codes
annotations.idempotentHint as False; add an idempotent: bool = False parameter
to _assert_mutating_tool, update its docstring to mention idempotency, and
replace the hard-coded assertion with assert annotations.idempotentHint is
idempotent so callers can opt-in when a mutating tool is idempotent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91292d0e-00fb-4fd7-bd03-060ec4bdf98d
📒 Files selected for processing (6)
src/lampyrid/tools/_annotations.pysrc/lampyrid/tools/accounts.pysrc/lampyrid/tools/budgets.pysrc/lampyrid/tools/insights.pysrc/lampyrid/tools/transactions.pytests/unit/test_tool_annotations.py
2 participants
$subject
Resolves #80