Skip to content

Commit 198eb66

Browse files
dougborgclaude
andauthored
feat: add user confirmation for destructive operations (#80) (#81)
* docs: add ADR 001 documenting user confirmation pattern Add Architecture Decision Record documenting the choice to use FastMCP Elicitation for user confirmation on destructive operations. Documents: - Context and problem statement - 4 options considered (pre-flight, parameter, prompt, elicitation) - Decision rationale (MCP-native, industry best practice) - Implementation pattern with code examples - Tool categorization by risk level - Testing requirements - Consequences and validation criteria Decision: Use FastMCP Elicitation (MCP native protocol) Rationale: Standard protocol, strong safety guarantees, rich context, excellent developer experience * test: update purchase order and sales order deletion tests for elicitation - Replace old deletion tests with elicitation pattern tests - Add imports for AcceptedElicitation, DeclinedElicitation, CancelledElicitation - Test all elicitation response paths (not found, accepted, declined, cancelled) - Align with product and supplier test patterns - All 276 tests passing * test: implement autospec for service mocks to enforce interface compliance Use create_autospec() for all service mocks in conftest.py to prevent tests from passing while mocking non-existent methods. This ensures that test mocks always match the actual service interfaces. Benefits: - Tests will fail immediately if they mock non-existent methods - Prevents bugs where tests pass but production code fails - Provides better refactoring safety - Catches method name typos and signature mismatches This change was implemented after discovering that tests were mocking services.suppliers.list_suppliers() instead of list_all(), which allowed the bug to slip through to the resource implementation. All 276 tests pass with autospec enforcement. Addresses: #82 * fix: add nullable enum support to client regeneration + fix resource bug 1. Add nullable enum field support to regeneration script - Add `add_nullable_to_enum_fields()` function - Mark OrderPlanFilterCriteria.currentStatus as nullable - Fixes "None is not a valid CurrentStatusEnum" errors - Addresses: #83 2. Fix supplier directory resource method name - Change `list_suppliers()` to `list_all()` - This bug was caught by autospec implementation - Related: #82 The regeneration script now handles enum fields that can be null in API responses, using the allOf + nullable pattern for OpenAPI 3.0. * fix: regenerate client with nullable currentStatus enum field Regenerated Python client from StockTrim OpenAPI spec with the nullable enum field fix applied. The currentStatus field in OrderPlanFilterCriteria can now handle null values from the API. Changes: - OrderPlanFilterCriteria.currentStatus is now CurrentStatusEnum | None | Unset - from_dict() properly handles None values without throwing validation errors This fixes the "None is not a valid CurrentStatusEnum" error when querying order plan data. Fixes: #83 * fix: remove limit parameter from ProductService.list_all() calls The ProductService.list_all() method doesn't accept a limit parameter, but foundation.py was calling it with limit=50. This was caught when testing resources with MCP Inspector at runtime. Root cause: test_foundation.py was using mock_foundation_context which overrode the autospec'd services from conftest.py with plain AsyncMock, so tests couldn't catch the interface mismatch. Changes: - foundation.py: Remove limit=50 from list_all() call, use slicing instead - test_foundation.py: Remove mock_foundation_context fixture that was overriding autospec'd services with plain AsyncMock - test_foundation.py: Update all tests to use mock_context directly - test_foundation.py: Fix catalog limit test to verify slicing behavior This ensures autospec catches interface mismatches in resource tests. * fix: use bulk endpoint for listing all suppliers The Suppliers.get_all() method was incorrectly using /api/Suppliers endpoint without a code parameter, which returns 404. The StockTrim API has separate endpoints for different supplier operations: - /api/Suppliers?code=X - returns single supplier (requires code) - /api/SuppliersBulk - returns all suppliers (no parameters) This is different from other endpoints like Customers and Products which return arrays from their main endpoint. Changes: - Import get_api_suppliers_bulk from generated API - Use bulk endpoint when code is UNSET (listing all) - Use single endpoint when code is provided (get specific supplier) - Update docstring to clarify the conditional behavior This fixes the 404 error in the supplier directory MCP resource. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent f1209d8 commit 198eb66

21 files changed

Lines changed: 1572 additions & 237 deletions

File tree

Lines changed: 287 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,287 @@
1+
# ADR 001: User Confirmation Pattern for Destructive Operations
2+
3+
**Status**: Accepted
4+
5+
**Date**: 2025-11-07
6+
7+
**Deciders**: Development Team
8+
9+
## Context and Problem Statement
10+
11+
The StockTrim MCP Server provides tools that perform destructive operations (deletions,
12+
modifications) on production inventory data. Without user confirmation, AI agents could
13+
accidentally:
14+
15+
- Delete critical inventory items, suppliers, or orders
16+
- Modify forecast settings that impact business operations
17+
- Make irreversible changes based on misinterpreted user intent
18+
19+
We need a standardized pattern for obtaining explicit user confirmation before executing
20+
high-risk operations, while maintaining a good developer and user experience.
21+
22+
## Decision Drivers
23+
24+
- **Safety**: Prevent accidental data loss from AI agent errors or misinterpretation
25+
- **User Experience**: Provide clear context about what will be deleted/modified
26+
- **MCP Compliance**: Use native MCP protocol features rather than custom solutions
27+
- **Developer Experience**: Easy to implement consistently across tools
28+
- **Maintainability**: Pattern should be sustainable as we add more tools
29+
30+
## Considered Options
31+
32+
### Option 1: Pre-Flight Check (Tool Returns Preview)
33+
34+
Tools return a preview object requiring a second confirmation call.
35+
36+
**Pros**:
37+
38+
- Clear two-phase workflow
39+
- Tool schema remains simple
40+
41+
**Cons**:
42+
43+
- Requires managing state between calls
44+
- Two tool calls increases latency
45+
- Complex for developers to implement consistently
46+
47+
### Option 2: Confirmation Parameter
48+
49+
Add `confirm: bool` parameter to destructive tools.
50+
51+
**Pros**:
52+
53+
- Simple implementation
54+
- Single tool call
55+
56+
**Cons**:
57+
58+
- Defeats purpose - AI could set `confirm=true` automatically
59+
- No preview of what will be deleted
60+
- Not a real safety mechanism
61+
62+
### Option 3: Custom Prompt Pattern
63+
64+
Use tool description to instruct AI to confirm with user.
65+
66+
**Pros**:
67+
68+
- No code changes required
69+
- Leverages AI capabilities
70+
71+
**Cons**:
72+
73+
- Unreliable - depends on AI following instructions
74+
- No enforcement mechanism
75+
- Inconsistent across different AI models
76+
77+
### Option 4: FastMCP Elicitation (MCP Native)
78+
79+
Use MCP's built-in elicitation protocol via FastMCP.
80+
81+
**Pros**:
82+
83+
- Native MCP feature with standardized protocol
84+
- FastMCP provides clean Python API
85+
- Forces human-in-the-loop confirmation
86+
- Provides rich preview in confirmation message
87+
- Widely adopted pattern in MCP ecosystem
88+
89+
**Cons**:
90+
91+
- Requires FastMCP v2.10.0+ (we already use v2.11.0)
92+
- Slightly more complex than simple parameters
93+
94+
## Decision Outcome
95+
96+
**Chosen option**: **Option 4 - FastMCP Elicitation**
97+
98+
### Rationale
99+
100+
1. **MCP-Native Solution**: The MCP specification (2025-06-18) includes native
101+
elicitation support. Using the standard protocol ensures compatibility with all MCP
102+
clients.
103+
104+
1. **Industry Best Practice**: Analysis of production MCP servers (Git MCP, GitHub MCP,
105+
OpenAPI MCP) shows elicitation is the recommended pattern for destructive operations.
106+
107+
1. **Strong Safety Guarantees**: Unlike prompt-based or parameter-based approaches,
108+
elicitation provides a hard stop - the operation cannot proceed without explicit user
109+
approval.
110+
111+
1. **Rich Context**: Elicitation messages can include detailed previews (product names,
112+
costs, affected entities) helping users make informed decisions.
113+
114+
1. **Excellent DX**: FastMCP's `context.elicit()` API is simple and well-documented.
115+
Pattern matching on response types (`AcceptedElicitation`, `DeclinedElicitation`,
116+
`CancelledElicitation`) is clean and type-safe.
117+
118+
## Implementation Pattern
119+
120+
### Standard Elicitation Flow
121+
122+
```python
123+
from fastmcp.server.elicitation import (
124+
AcceptedElicitation,
125+
CancelledElicitation,
126+
DeclinedElicitation,
127+
)
128+
129+
async def delete_entity(request: DeleteRequest, context: Context) -> DeleteResponse:
130+
"""Delete an entity.
131+
132+
🔴 HIGH-RISK OPERATION: This action permanently deletes data
133+
and cannot be undone. User confirmation is required via elicitation.
134+
"""
135+
services = get_services(context)
136+
137+
# 1. Fetch entity for preview
138+
entity = await services.get_by_code(request.code)
139+
140+
if not entity:
141+
return DeleteResponse(success=False, message=f"Entity not found: {request.code}")
142+
143+
# 2. Build rich preview message
144+
result = await context.elicit(
145+
message=f"""⚠️ Delete entity {entity.code}?
146+
147+
**{entity.name}**
148+
Type: {entity.type}
149+
150+
This action will permanently delete the entity and cannot be undone.
151+
152+
Proceed with deletion?""",
153+
response_type=None, # Simple yes/no approval
154+
)
155+
156+
# 3. Handle elicitation response
157+
match result:
158+
case AcceptedElicitation():
159+
success, message = await services.delete(request.code)
160+
return DeleteResponse(
161+
success=success,
162+
message=f"{message}" if success else message,
163+
)
164+
165+
case DeclinedElicitation():
166+
return DeleteResponse(
167+
success=False,
168+
message=f"❌ Deletion of {entity.code} declined by user",
169+
)
170+
171+
case CancelledElicitation():
172+
return DeleteResponse(
173+
success=False,
174+
message=f"❌ Deletion of {entity.code} cancelled by user",
175+
)
176+
177+
case _:
178+
return DeleteResponse(
179+
success=False,
180+
message=f"Unexpected elicitation response for {entity.code}",
181+
)
182+
```
183+
184+
### Tool Categorization
185+
186+
**HIGH-RISK (Require Elicitation)**:
187+
188+
- Deletions: `delete_product`, `delete_supplier`, `delete_purchase_order`,
189+
`delete_sales_orders`
190+
- Irreversible modifications: TBD in Phase 2
191+
192+
**MEDIUM-RISK (Consider for Phase 2)**:
193+
194+
- Inventory modifications: `set_product_inventory`
195+
- Forecast configuration: `update_forecast_settings`, `configure_product`
196+
- Financial impact: `create_purchase_order` (large orders)
197+
198+
**LOW-RISK (No Elicitation Required)**:
199+
200+
- Read operations: All `get_*`, `list_*` tools
201+
- Reversible creates: `create_product`, `create_supplier`
202+
203+
### Preview Message Guidelines
204+
205+
1. **Start with warning emoji**: `⚠️ Delete...?`
206+
1. **Include entity details**: Name, code, status
207+
1. **Show impact**: Associated data, financial amounts, affected records
208+
1. **Use emoji indicators**: 🟢 Active, 🔴 Discontinued, 💰 Cost
209+
1. **Clear consequences**: "This cannot be undone"
210+
1. **Simple question**: "Proceed with deletion?"
211+
212+
### Response Message Guidelines
213+
214+
1. **Success**: Prefix with ✅ emoji
215+
1. **Declined**: Prefix with ❌, include "declined by user"
216+
1. **Cancelled**: Prefix with ❌, include "cancelled by user"
217+
1. **Error**: No emoji, clear error message
218+
219+
### Testing Requirements
220+
221+
Each elicitation-protected tool must have tests for:
222+
223+
- ✓ Entity not found (skip elicitation)
224+
- ✓ AcceptedElicitation (proceed with operation)
225+
- ✓ DeclinedElicitation (abort, no operation performed)
226+
- ✓ CancelledElicitation (abort, no operation performed)
227+
- ✓ Preview message content (verify entity details shown)
228+
229+
## Consequences
230+
231+
### Positive
232+
233+
- **Improved Safety**: Eliminates accidental destructive operations
234+
- **Better UX**: Users see exactly what will be deleted before confirming
235+
- **Consistent Pattern**: All high-risk operations follow same flow
236+
- **MCP Compliant**: Using standard protocol ensures broad client support
237+
- **Auditable**: Elicitation responses can be logged for compliance
238+
239+
### Negative
240+
241+
- **Increased Latency**: Each destructive operation requires extra round-trip for
242+
confirmation
243+
- **More Code**: Elicitation adds ~30 lines per tool compared to direct deletion
244+
- **Testing Complexity**: Each tool needs 4-5 additional tests for elicitation paths
245+
246+
### Neutral
247+
248+
- **No Impact on Read Operations**: Only affects destructive tools
249+
- **Backward Incompatible**: Old AI workflows expecting immediate deletion will break
250+
(this is intentional)
251+
252+
## Validation
253+
254+
### Success Criteria
255+
256+
- ✅ All 4 high-risk deletion tools implement elicitation (Phase 1)
257+
- ✅ Comprehensive test coverage for elicitation responses
258+
- ✅ Documentation updated with safety information
259+
- ⏳ No accidental deletions in production (ongoing monitoring)
260+
261+
### Implementation Status
262+
263+
**Phase 1 (Completed)**:
264+
265+
- `delete_product` - Products tool
266+
- `delete_supplier` - Suppliers tool
267+
- `delete_purchase_order` - Purchase Orders tool
268+
- `delete_sales_orders` - Sales Orders tool
269+
270+
**Phase 2 (Planned)**:
271+
272+
- Medium-risk modification tools (inventory, forecast)
273+
274+
**Phase 3 (Planned)**:
275+
276+
- Financial impact tools (large purchase orders)
277+
278+
## References
279+
280+
- [MCP Specification - Elicitation](https://spec.modelcontextprotocol.io/specification/2025-06-18/server/elicitation/)
281+
- [FastMCP Documentation - Elicitation](https://github.com/jlowin/fastmcp)
282+
- [Issue #80: Add user confirmation for destructive operations](https://github.com/dougborg/stocktrim-openapi-client/issues/80)
283+
- ADR Template: [MADR 3.0.0](https://adr.github.io/madr/)
284+
285+
## Changelog
286+
287+
- 2025-11-07: Initial ADR documenting FastMCP Elicitation pattern

0 commit comments

Comments
 (0)