Repository navigation
Feature/clipboard client - #4
Merged
Merged
Conversation
Add clipboard (memory) service with sync and async support: - ClipboardService (sync) and AsyncClipboardService - Named access: set, get (by name), delete - Indexed access: push, pop, get (by index) - List all entries, clear_all - Full Pydantic model validation Types: - ClipboardEntry, ClipboardSetRequest, ClipboardPushRequest - ClipboardGetFilters, ClipboardDeleteRequest - ClipboardResponse, ClipboardDeleteResponse, ClipboardListResponse - ClipboardEncoding and ClipboardVisibility enums
- Add ClipboardSource enum (ui/sdk/mcp) - Add source field to ClipboardEntry model - Hardcode source as 'sdk' in sync/async set/push methods
- Add DEFAULT_CLIPBOARD_SOURCE constant for backward compatibility - Make ClipboardEntry.source Optional to handle older API responses - Export constant from __init__.py Addresses code review feedback about breaking existing call sites.
- Add Clipboard Operations section to README - Document set, get, push, pop, list, and delete operations - Add ClipboardEntry structure with source field - Note that source is auto-set to ClipboardSource.SDK when using this SDK - Add Clipboard/Memory to Features list
- Use 'is not None' check for ttl_seconds instead of truthiness - Add explicit validation that ttl_seconds > 0 when provided - Fix clear_all to only increment counter when delete() returns True - Applied to both sync and async service classes (4 locations each)
- Import ClipboardSource from types - Replace hardcoded "sdk" string with ClipboardSource.SDK.value - Consistent with enum usage across the SDK
Feature/clipboard source tracking
Reviewer's GuideAdds a new clipboard domain to the SDK, including Pydantic models, sync/async service clients wired into the main client, and README documentation describing clipboard usage and entry structure. Sequence diagram for synchronous clipboard set operationsequenceDiagram
actor Developer
participant PluggedInClient
participant ClipboardService
participant ClipboardHTTPAPI
Developer->>PluggedInClient: create(api_key)
Developer->>PluggedInClient: clipboard.set(name,value,content_type,encoding,visibility,created_by_tool,created_by_model,ttl_seconds)
PluggedInClient->>ClipboardService: set(name,value,content_type,encoding,visibility,created_by_tool,created_by_model,ttl_seconds)
ClipboardService->>ClipboardService: _build_clipboard_payload(...)
ClipboardService->>ClipboardHTTPAPI: POST /api/clipboard
ClipboardHTTPAPI-->>ClipboardService: success, entry json
ClipboardService-->>PluggedInClient: ClipboardEntry
PluggedInClient-->>Developer: ClipboardEntry
Updated class diagram for clipboard domain models and servicesclassDiagram
class ClipboardEncoding {
<<enumeration>>
UTF8
BASE64
HEX
}
class ClipboardVisibility {
<<enumeration>>
PRIVATE
WORKSPACE
PUBLIC
}
class ClipboardSource {
<<enumeration>>
UI
SDK
MCP
}
class ClipboardEntry {
uuid: str
name: Optional[str]
idx: Optional[int]
value: str
content_type: str
encoding: ClipboardEncoding
size_bytes: int
visibility: ClipboardVisibility
created_by_tool: Optional[str]
created_by_model: Optional[str]
source: Optional[ClipboardSource]
created_at: datetime
updated_at: datetime
expires_at: Optional[datetime]
}
class ClipboardListResponse {
success: bool
entries: List[ClipboardEntry]
}
class ClipboardSetRequest {
name: str
value: str
content_type: Optional[str]
encoding: Optional[ClipboardEncoding]
visibility: Optional[ClipboardVisibility]
created_by_tool: Optional[str]
created_by_model: Optional[str]
ttl_seconds: Optional[int]
}
class ClipboardPushRequest {
value: str
content_type: Optional[str]
encoding: Optional[ClipboardEncoding]
visibility: Optional[ClipboardVisibility]
created_by_tool: Optional[str]
created_by_model: Optional[str]
ttl_seconds: Optional[int]
}
class ClipboardGetFilters {
name: Optional[str]
idx: Optional[int]
check_name_or_idx()
}
class ClipboardDeleteRequest {
name: Optional[str]
idx: Optional[int]
check_name_or_idx()
}
class ClipboardResponse {
success: bool
entry: Optional[ClipboardEntry]
error: Optional[str]
}
class ClipboardDeleteResponse {
success: bool
deleted: Optional[bool]
error: Optional[str]
}
class ClipboardService {
client: PluggedInClient
list() List[ClipboardEntry]
get(name: Optional[str], idx: Optional[int]) Optional[ClipboardEntry]
set(name: str, value: str, content_type: str, encoding: ClipboardEncoding, visibility: ClipboardVisibility, created_by_tool: Optional[str], created_by_model: Optional[str], ttl_seconds: Optional[int]) ClipboardEntry
push(value: str, content_type: str, encoding: ClipboardEncoding, visibility: ClipboardVisibility, created_by_tool: Optional[str], created_by_model: Optional[str], ttl_seconds: Optional[int]) ClipboardEntry
pop() Optional[ClipboardEntry]
delete(name: Optional[str], idx: Optional[int]) bool
clear_all() int
}
class AsyncClipboardService {
client: AsyncPluggedInClient
list() List[ClipboardEntry]
get(name: Optional[str], idx: Optional[int]) Optional[ClipboardEntry]
set(name: str, value: str, content_type: str, encoding: ClipboardEncoding, visibility: ClipboardVisibility, created_by_tool: Optional[str], created_by_model: Optional[str], ttl_seconds: Optional[int]) ClipboardEntry
push(value: str, content_type: str, encoding: ClipboardEncoding, visibility: ClipboardVisibility, created_by_tool: Optional[str], created_by_model: Optional[str], ttl_seconds: Optional[int]) ClipboardEntry
pop() Optional[ClipboardEntry]
delete(name: Optional[str], idx: Optional[int]) bool
clear_all() int
}
class PluggedInClient
class AsyncPluggedInClient
ClipboardEntry --> ClipboardEncoding
ClipboardEntry --> ClipboardVisibility
ClipboardEntry --> ClipboardSource
ClipboardListResponse --> ClipboardEntry
ClipboardResponse --> ClipboardEntry
ClipboardDeleteResponse --> ClipboardEntry
ClipboardService --> PluggedInClient
AsyncClipboardService --> AsyncPluggedInClient
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The sync and async clipboard services duplicate a lot of logic (payload building, error handling, clear_all implementation); consider extracting shared helpers to reduce maintenance overhead and keep behavior perfectly in sync.
- The
getanddeletemethods accept bothnameandidxsimultaneously; consider validating that exactly one is provided (or defining a clear precedence) to avoid ambiguous calls. - Service methods like
setandpushacceptencodingandvisibilityas plain strings while you have strongly-typed enums (ClipboardEncoding,ClipboardVisibility); consider using those enums in the public API for better type safety and to catch invalid values earlier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The sync and async clipboard services duplicate a lot of logic (payload building, error handling, clear_all implementation); consider extracting shared helpers to reduce maintenance overhead and keep behavior perfectly in sync.
- The `get` and `delete` methods accept both `name` and `idx` simultaneously; consider validating that exactly one is provided (or defining a clear precedence) to avoid ambiguous calls.
- Service methods like `set` and `push` accept `encoding` and `visibility` as plain strings while you have strongly-typed enums (`ClipboardEncoding`, `ClipboardVisibility`); consider using those enums in the public API for better type safety and to catch invalid values earlier.
## Individual Comments
### Comment 1
<location> `src/pluggedinkit/types.py:354-301` </location>
<code_context>
+ populate_by_name = True
+
+
+class ClipboardGetFilters(BaseModel):
+ """Filters for getting a clipboard entry"""
+ name: Optional[str] = None
+ idx: Optional[int] = None
+
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider validating that at least one of `name` or `idx` is provided in filter/delete models.
`ClipboardGetFilters` (and similarly `ClipboardDeleteRequest`) allow both `name` and `idx` to be `None`, but the service methods reject that case. This lets callers build models that will later fail at the service layer. Consider adding a `model_validator` (or similar) so the models enforce "at least one of `name` or `idx`" directly, keeping them consistent with the service behavior.
</issue_to_address>
### Comment 2
<location> `src/pluggedinkit/services/clipboard.py:39-33` </location>
<code_context>
+ list_response = ClipboardListResponse(**data)
+ return list_response.entries
+
+ def get(
+ self,
+ name: Optional[str] = None,
+ idx: Optional[int] = None,
+ ) -> Optional[ClipboardEntry]:
+ """Get a clipboard entry by name or index"""
+ if name is None and idx is None:
+ raise ValueError("Either 'name' or 'idx' must be provided")
+
+ params = {}
+ if name is not None:
+ params["name"] = name
+ if idx is not None:
+ params["idx"] = str(idx)
+
+ response = self.client.request("GET", "/api/clipboard", params=params)
+ data = response.json()
+
+ if not data.get("success"):
+ return None
+
</code_context>
<issue_to_address>
**question (bug_risk):** Error handling in `get` differs from the rest of the service API and may be surprising.
Here, non-success responses return `None`, while other methods (`list`, `set`, `push`) raise `PluggedInError` on failure. This makes it hard for callers to distinguish "not found" from other errors and forces them to interpret `success` themselves.
If the API can differentiate not-found from other errors, consider returning `None` only for not-found and raising an exception for other failures, to stay consistent with the rest of the service.
</issue_to_address>
### Comment 3
<location> `src/pluggedinkit/services/clipboard.py:64-70` </location>
<code_context>
+ return ClipboardEntry(**data["entry"])
+ return None
+
+ def set(
+ self,
+ name: str,
+ value: str,
+ content_type: str = "text/plain",
+ encoding: str = "utf-8",
+ visibility: str = "private",
+ created_by_tool: Optional[str] = None,
+ created_by_model: Optional[str] = None,
</code_context>
<issue_to_address>
**suggestion:** Leverage existing enums for `encoding`/`visibility` in the service interface for stronger typing.
The `set`/`push` parameters currently take `str` for `encoding` and `visibility`, even though `ClipboardEncoding` and `ClipboardVisibility` are already defined and used in the models. Accepting these enums (or `Union[str, ClipboardEncoding]` / `Union[str, ClipboardVisibility]`) would improve type safety and editor support while keeping compatibility with raw strings, and keep the service API consistent with the Pydantic model constraints.
</issue_to_address>
### Comment 4
<location> `src/pluggedinkit/services/clipboard.py:22` </location>
<code_context>
+ from ..client import AsyncPluggedInClient, PluggedInClient
+
+
+class ClipboardService:
+ """Synchronous clipboard service"""
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the clipboard services to share sync/async logic and rely on existing Pydantic request/response models instead of manual dict-building and validation.
The main complexity comes from duplicated sync/async logic and hand-rolled payload/validation that the existing Pydantic models could handle for you.
A few concrete refactors that reduce duplication while keeping behavior intact:
---
### 1. Share payload-building logic between sync/async
`set`/`push` are copy-pasted with the only difference being `await`. You can centralize payload construction and reuse it from both classes:
```python
# clipboard.py (module-level helpers)
def _build_set_payload(
name: str,
value: str,
content_type: str,
encoding: str,
visibility: str,
created_by_tool: Optional[str],
created_by_model: Optional[str],
ttl_seconds: Optional[int],
) -> dict:
req = ClipboardSetRequest(
name=name,
value=value,
content_type=content_type,
encoding=encoding,
visibility=visibility,
source=ClipboardSource.SDK, # or ClipboardSource.SDK.value if model expects string
created_by_tool=created_by_tool,
created_by_model=created_by_model,
ttl_seconds=ttl_seconds,
)
return req.model_dump(by_alias=True, exclude_none=True)
```
Then in both services:
```python
def set(...):
payload = _build_set_payload(
name=name,
value=value,
content_type=content_type,
encoding=encoding,
visibility=visibility,
created_by_tool=created_by_tool,
created_by_model=created_by_model,
ttl_seconds=ttl_seconds,
)
response = self.client.request("POST", "/api/clipboard", json=payload)
...
```
```python
async def set(...):
payload = _build_set_payload(...)
response = await self.client.request("POST", "/api/clipboard", json=payload)
...
```
Same idea for `push` via `_build_push_payload(...)`.
This removes repeated `ttl_seconds` checks and manual `"contentType"` / `"createdByTool"` keys; the Pydantic model already validates `ttl_seconds` and handles aliases.
---
### 2. Use request models for `get`/`delete` instead of ad-hoc dicts
You’re manually building `params`/`payload` and re-implementing validation like “either name or idx must be provided”. That’s exactly what `ClipboardGetFilters` and `ClipboardDeleteRequest` are for.
Example for `get`:
```python
# helper
def _build_get_params(
name: Optional[str],
idx: Optional[int],
) -> dict:
filters = ClipboardGetFilters(name=name, idx=idx)
return filters.model_dump(exclude_none=True)
```
Then:
```python
def get(self, name: Optional[str] = None, idx: Optional[int] = None) -> Optional[ClipboardEntry]:
params = _build_get_params(name, idx)
response = self.client.request("GET", "/api/clipboard", params=params)
data = response.json()
...
```
```python
async def get(...):
params = _build_get_params(name, idx)
response = await self.client.request("GET", "/api/clipboard", params=params)
...
```
Same pattern for `delete` using `ClipboardDeleteRequest`:
```python
def _build_delete_payload(name: Optional[str], idx: Optional[int]) -> dict:
req = ClipboardDeleteRequest(name=name, idx=idx)
return req.model_dump(exclude_none=True)
```
---
### 3. Use response models instead of manual `success`/`entry` handling
You already import `ClipboardResponse`, `ClipboardDeleteResponse`, `ClipboardListResponse`. They can replace the inline `data.get(...)` checks and `ClipboardEntry(**data["entry"])` calls.
Example for `set` (sync version):
```python
response = self.client.request("POST", "/api/clipboard", json=payload)
data = response.json()
resp = ClipboardResponse(**data)
if not resp.success:
raise PluggedInError(resp.error or "Failed to set clipboard entry")
return resp.entry
```
Same code in async (just `await` the request). Similar simplification is possible in `push`, `pop`, `delete`, and `list` with their respective response models.
---
### 4. Deduplicate `clear_all` with a shared helper
The `clear_all` loops are identical apart from `await`. You can centralize the core logic and pass in a delete function; this keeps behavior and error swallowing identical.
A small, focused helper that takes a list and a delete callable:
```python
def _clear_entries_sync(entries: List[ClipboardEntry], delete_fn) -> int:
deleted = 0
for entry in entries:
try:
success = False
if entry.name:
success = delete_fn(name=entry.name)
elif entry.idx is not None:
success = delete_fn(idx=entry.idx)
if success:
deleted += 1
except PluggedInError:
pass
return deleted
```
Then:
```python
def clear_all(self) -> int:
entries = self.list()
return _clear_entries_sync(entries, self.delete)
```
The async version can mirror this with a tiny async helper:
```python
async def _clear_entries_async(entries: List[ClipboardEntry], delete_fn) -> int:
deleted = 0
for entry in entries:
try:
success = False
if entry.name:
success = await delete_fn(name=entry.name)
elif entry.idx is not None:
success = await delete_fn(idx=entry.idx)
if success:
deleted += 1
except PluggedInError:
pass
return deleted
async def clear_all(self) -> int:
entries = await self.list()
return await _clear_entries_async(entries, self.delete)
```
---
Applying these patterns will:
- Remove duplicated parameter/validation logic across sync/async.
- Centralize schema and validation in the existing Pydantic models.
- Minimize manual dict-building and repeated `ttl_seconds` checks.
- Keep the public API and wire protocol behavior exactly as in your current implementation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| response = self.client.request("GET", "/api/clipboard") | ||
| data = response.json() | ||
|
|
||
| if not data.get("success"): |
There was a problem hiding this comment.
question (bug_risk): Error handling in get differs from the rest of the service API and may be surprising.
Here, non-success responses return None, while other methods (list, set, push) raise PluggedInError on failure. This makes it hard for callers to distinguish "not found" from other errors and forces them to interpret success themselves.
If the API can differentiate not-found from other errors, consider returning None only for not-found and raising an exception for other failures, to stay consistent with the rest of the service.
Pull Request Review: Feature/clipboard clientOverviewThis PR adds clipboard functionality to the SDK, providing persistent key-value storage for MCP tools and AI agents. The implementation follows good patterns and is generally well-structured. Code Quality and Best PracticesStrengths
Issues & Suggestions1. Inconsistent Return Type in list() method The list() method returns List[ClipboardEntry] but README example line 435 treats it as having .entries attribute - this would cause runtime errors. Fix: Update README to use 2. Missing Input Validation The set() method doesn't validate name parameter length (max 255 chars). Add validation before API call. 3. Silent Error Handling in clear_all() Silently catches PluggedInError exceptions, potentially masking auth failures or network issues. Potential Bugs1. Race Condition in clear_all() - Lists then deletes one-by-one. Concurrent operations could cause failures. 2. API Response Structure Mismatch - get() method needs better error handling for unexpected API responses. 3. Unused Type Definitions - ClipboardSetRequest, ClipboardPushRequest, ClipboardGetFilters, etc. are defined but never used. Either use them or remove. Performance Considerations1. Inefficient clear_all() - Makes N+1 API calls. Consider bulk delete endpoint or pagination. 2. Missing Pagination - list() fetches all entries at once. Could cause memory issues for large clipboards. Security Concerns1. No TTL Upper Bound - Add max validation (e.g., 1 year max) 2. No Content Size Validation - Large values could cause memory issues. Add size limit (e.g., 10MB max) 3. Source Field Override - Document hardcoded SDK source in method docstrings Test Coverage - CRITICALThe PR includes no tests for clipboard functionality. This is a blocker for production code. Essential test cases needed:
SummaryMust Fix Before Merge
Should Fix
Nice to Have
Recommendation: Request changes for test coverage and critical bugs before merging. Great work overall! With these improvements, this will be production-ready. |
- Add model_validator to ClipboardGetFilters for name/idx constraint - Add model_validator to ClipboardDeleteRequest for name/idx constraint - Use ClipboardEncoding enum for encoding parameter - Use ClipboardVisibility enum for visibility parameter - Extract _build_clipboard_payload helper to reduce sync/async duplication
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The enum handling in
_build_clipboard_payloadis a bit redundant: callers already normalizeencoding/visibilitytoClipboardEncoding/ClipboardVisibility, and then_build_clipboard_payloadchecks/normalizes them again—consider consolidating this so the conversion happens in only one place. - Error handling behavior is inconsistent across clipboard methods (
list/set/pushraise on failure whileget/pop/deletesilently returnNone/False); it may be worth aligning these semantics or at least distinguishing between a genuine error and a simple ‘not found’ case.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The enum handling in `_build_clipboard_payload` is a bit redundant: callers already normalize `encoding`/`visibility` to `ClipboardEncoding`/`ClipboardVisibility`, and then `_build_clipboard_payload` checks/normalizes them again—consider consolidating this so the conversion happens in only one place.
- Error handling behavior is inconsistent across clipboard methods (`list`/`set`/`push` raise on failure while `get`/`pop`/`delete` silently return `None`/`False`); it may be worth aligning these semantics or at least distinguishing between a genuine error and a simple ‘not found’ case.
## Individual Comments
### Comment 1
<location> `src/pluggedinkit/services/clipboard.py:189-198` </location>
<code_context>
+
+ return data.get("success", False) and data.get("deleted", False)
+
+ def clear_all(self) -> int:
+ """Clear all clipboard entries. Returns count of deleted entries."""
+ entries = self.list()
+ deleted = 0
+
+ for entry in entries:
+ try:
+ success = False
+ if entry.name:
+ success = self.delete(name=entry.name)
+ elif entry.idx is not None:
+ success = self.delete(idx=entry.idx)
+ if success:
+ deleted += 1
+ except PluggedInError:
+ pass
+
+ return deleted
+
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The clear_all implementation may be inefficient for large clipboards and silently swallows errors.
`clear_all` currently deletes entries one by one and ignores `PluggedInError` inside the loop. This can be slow with many entries and hides partial failures from callers.
If available, prefer a bulk delete API. Otherwise, consider at least logging/aggregating failures or returning structured results (e.g., total, succeeded, failed) so callers can detect and handle partial deletions.
</issue_to_address>
### Comment 2
<location> `src/pluggedinkit/services/clipboard.py:57` </location>
<code_context>
+ return payload
+
+
+class ClipboardService:
+ """Synchronous clipboard service"""
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring shared logic and input validation into small helper functions and existing models so the sync and async clipboard services stay thin and less duplicated while preserving behavior.
You can reduce complexity without changing behavior by centralizing the shared logic and normalizing inputs in one place.
### 1. Factor out shared sync/async request logic
Most methods differ only by `await`. You can extract common helpers that take a callable `request` (sync or async):
```python
def _build_params(name: Optional[str], idx: Optional[int]) -> dict:
if name is None and idx is None:
raise ValueError("Either 'name' or 'idx' must be provided")
params: dict = {}
if name is not None:
params["name"] = name
if idx is not None:
params["idx"] = str(idx)
return params
```
```python
def _parse_list_response(data: dict) -> List[ClipboardEntry]:
if not data.get("success"):
raise PluggedInError(data.get("error", "Failed to list clipboard entries"))
return ClipboardListResponse(**data).entries
```
Then in services:
```python
class ClipboardService:
def list(self) -> List[ClipboardEntry]:
response = self.client.request("GET", "/api/clipboard")
return _parse_list_response(response.json())
def get(self, name: Optional[str] = None, idx: Optional[int] = None) -> Optional[ClipboardEntry]:
params = _build_params(name, idx)
response = self.client.request("GET", "/api/clipboard", params=params)
data = response.json()
if not data.get("success") or not data.get("entry"):
return None
return ClipboardEntry(**data["entry"])
```
```python
class AsyncClipboardService:
async def list(self) -> List[ClipboardEntry]:
response = await self.client.request("GET", "/api/clipboard")
return _parse_list_response(response.json())
async def get(self, name: Optional[str] = None, idx: Optional[int] = None) -> Optional[ClipboardEntry]:
params = _build_params(name, idx)
response = await self.client.request("GET", "/api/clipboard", params=params)
data = response.json()
if not data.get("success") or not data.get("entry"):
return None
return ClipboardEntry(**data["entry"])
```
You can apply the same pattern to `set`, `push`, `delete`, and `pop` by extracting small helpers like `_parse_entry_response(data, default_none_ok: bool)` and `_parse_delete_response(data) -> bool`. That keeps sync/async classes thin but preserves the public API.
### 2. Centralize encoding/visibility normalization in `_build_clipboard_payload`
Right now you normalize enums at call sites and again inside `_build_clipboard_payload`. You can move that logic into `_build_clipboard_payload` and pass through whatever the caller received:
```python
from typing import Union
def _build_clipboard_payload(
value: str,
name: Optional[str] = None,
content_type: str = "text/plain",
encoding: Union[ClipboardEncoding, str] = ClipboardEncoding.UTF8,
visibility: Union[ClipboardVisibility, str] = ClipboardVisibility.PRIVATE,
created_by_tool: Optional[str] = None,
created_by_model: Optional[str] = None,
ttl_seconds: Optional[int] = None,
) -> dict:
if not isinstance(encoding, ClipboardEncoding):
encoding = ClipboardEncoding(encoding)
if not isinstance(visibility, ClipboardVisibility):
visibility = ClipboardVisibility(visibility)
payload = {
"value": value,
"contentType": content_type,
"encoding": encoding.value,
"visibility": visibility.value,
"source": ClipboardSource.SDK.value,
}
# ... rest unchanged ...
return payload
```
Then your service methods become simpler:
```python
def set(
self,
name: str,
value: str,
content_type: str = "text/plain",
encoding: Union[ClipboardEncoding, str] = ClipboardEncoding.UTF8,
visibility: Union[ClipboardVisibility, str] = ClipboardVisibility.PRIVATE,
created_by_tool: Optional[str] = None,
created_by_model: Optional[str] = None,
ttl_seconds: Optional[int] = None,
) -> ClipboardEntry:
payload = _build_clipboard_payload(
value=value,
name=name,
content_type=content_type,
encoding=encoding,
visibility=visibility,
created_by_tool=created_by_tool,
created_by_model=created_by_model,
ttl_seconds=ttl_seconds,
)
# request + parsing unchanged
```
Same change can be applied to `push` in both sync and async services.
### 3. Reuse existing Pydantic models for `get`/`delete` validation
Since you already have `ClipboardGetFilters` / `ClipboardDeleteRequest`, you can offload the “at least one of name or idx” validation there instead of duplicating it in methods:
```python
def delete(self, name: Optional[str] = None, idx: Optional[int] = None) -> bool:
req = ClipboardDeleteRequest(name=name, idx=idx)
response = self.client.request("DELETE", "/api/clipboard", json=req.model_dump())
data = response.json()
return data.get("success", False) and data.get("deleted", False)
```
```python
async def delete(self, name: Optional[str] = None, idx: Optional[int] = None) -> bool:
req = ClipboardDeleteRequest(name=name, idx=idx)
response = await self.client.request("DELETE", "/api/clipboard", json=req.model_dump())
data = response.json()
return data.get("success", False) and data.get("deleted", False)
```
You can do the same for `get` with `ClipboardGetFilters`, which will remove the manual `ValueError` checks and keep validation in one place.
### 4. Extract `_delete_entry` helper for `clear_all`
`clear_all` is fine functionally, but a tiny helper reduces branching and duplication and can be shared between sync and async:
```python
def _entry_delete_args(entry: ClipboardEntry) -> dict:
if entry.name:
return {"name": entry.name}
if entry.idx is not None:
return {"idx": entry.idx}
return {}
```
```python
def clear_all(self) -> int:
entries = self.list()
deleted = 0
for entry in entries:
args = _entry_delete_args(entry)
if not args:
continue
try:
if self.delete(**args):
deleted += 1
except PluggedInError:
# Optional: log here if desired
pass
return deleted
```
```python
async def clear_all(self) -> int:
entries = await self.list()
deleted = 0
for entry in entries:
args = _entry_delete_args(entry)
if not args:
continue
try:
if await self.delete(**args):
deleted += 1
except PluggedInError:
# Optional: log here if desired
pass
return deleted
```
These changes keep all functionality and public signatures intact while reducing duplication and making the code easier to evolve.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Consolidate enum normalization to _build_clipboard_payload only - Extract shared helper functions (_parse_list_response, _parse_entry_response, etc.) - Use Pydantic models (ClipboardGetFilters, ClipboardDeleteRequest) for validation - Fix clear_all to use bulk delete API with clearAll=true instead of one-by-one - Add ClearAllResult dataclass for structured feedback on bulk operations - Add explicit type annotations (Dict[str, Any]) to fix mypy errors - Change delete() return type from bool to int (deleted count) - Add consistent error handling with raise_on_failure parameter Key improvements: - Single place for enum normalization (no more redundant checks) - Shared helpers reduce duplication between sync/async services - Pydantic validators handle "at least one of name/idx" checks - Bulk delete is efficient for large clipboards - ClearAllResult provides deleted/failed counts and success property
Pull Request Review: Feature/clipboard clientSummaryThis PR introduces a new clipboard service feature to the SDK, adding persistent key-value and stack-based storage for MCP tools and AI agents. The implementation is well-structured and follows the existing codebase patterns. ✅ StrengthsCode Quality
Design Decisions
🔍 Issues & Recommendations1. Critical: Incomplete type validation in types.py (lines 359-363, 372-375)The validator uses Issue: Line 363 shows Recommendation: Ensure the validator returns @model_validator(mode='after')
def check_name_or_idx(self) -> 'ClipboardDeleteRequest':
if self.name is None and self.idx is None:
raise ValueError("Either 'name' or 'idx' must be provided")
return self # Not 'sel'2. Potential Bug: Inconsistent error response handling (clipboard.py:127-136)The Current behavior: def _parse_delete_response(data: Dict[str, Any]) -> int:
if not data.get("success"):
return 0 # Silently fails
deleted = data.get("deleted", 0)
return int(deleted) if deleted is not None else 0Recommendation: Consider raising an exception or logging when the API returns an error: def _parse_delete_response(data: Dict[str, Any]) -> int:
if not data.get("success"):
error_msg = data.get("error", "Failed to delete clipboard entry")
raise PluggedInError(error_msg)
deleted = data.get("deleted", 0)
return int(deleted) if deleted is not None else 0Or at minimum, distinguish between "not found" and "error" cases. 3. Code Smell: Unnecessary assertions (clipboard.py:289, 336, 499, 546)The code uses Issue: These assertions are redundant since the function will raise an exception if it fails. Assertions can also be disabled with Python's Recommendation: Remove the assertions or add a comment explaining they're for type checker satisfaction: result = _parse_entry_response(
response.json(),
error_msg="Failed to set clipboard entry",
raise_on_failure=True,
)
# Type narrowing: _parse_entry_response with raise_on_failure=True never returns None
assert result is not None
return resultOr better, adjust the function signature to reflect this: @overload
def _parse_entry_response(
data: Dict[str, Any], error_msg: str, raise_on_failure: Literal[True]
) -> ClipboardEntry: ...
@overload
def _parse_entry_response(
data: Dict[str, Any], error_msg: str, raise_on_failure: Literal[False]
) -> Optional[ClipboardEntry]: ...4. Performance: README examples show synchronous list iteration (README.md:435-438)The clipboard list example could mislead users about the return type: response = client.clipboard.list()
for entry in response.entries: # 'entries' attribute doesn't existIssue: Actual implementation (clipboard.py:201-211): def list(self) -> List[ClipboardEntry]:
response = self.client.request("GET", "/api/clipboard")
return _parse_list_response(response.json()) # Returns .entriesRecommendation: Fix the documentation: # List all entries
entries = client.clipboard.list()
for entry in entries:
label = entry.name or f"idx:{entry.idx}"
print(f"{label} - source: {entry.source}")5. Security: No validation on value sizeThe clipboard service doesn't validate the size of Recommendation: Add size validation in def _build_clipboard_payload(
value: str,
# ... other params ...
) -> Dict[str, Any]:
# Add reasonable size limit (e.g., 1MB for clipboard values)
MAX_VALUE_SIZE = 1024 * 1024 # 1MB
if len(value.encode('utf-8')) > MAX_VALUE_SIZE:
raise ValueError(f"Value size exceeds maximum of {MAX_VALUE_SIZE} bytes")
# ... rest of implementation6. Best Practice: Missing input validationThe Recommendation: Add basic MIME type validation or at least length limits: if content_type and len(content_type) > 256:
raise ValueError("content_type must be 256 characters or less")🧪 Test CoverageCritical Issue: No test files were added with this PR. Recommendation: Add comprehensive tests covering:
Example test structure: # tests/test_clipboard.py
import pytest
from pluggedinkit import PluggedInClient, ClipboardEncoding
def test_clipboard_set_get(client):
entry = client.clipboard.set("test_key", "test_value")
assert entry.name == "test_key"
retrieved = client.clipboard.get(name="test_key")
assert retrieved.value == "test_value"
def test_clipboard_ttl_validation():
with pytest.raises(ValueError, match="ttl_seconds must be greater than 0"):
client.clipboard.set("key", "value", ttl_seconds=0)🔒 Security ConsiderationsPositive
Concerns
📊 Performance Considerations
🎯 SummaryThis is a well-implemented feature that follows good practices and maintains consistency with the existing codebase. The main concerns are:
Recommended Actions Before Merge
Great work overall! The feature is production-ready once the above items are addressed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add synchronous and asynchronous clipboard services and types to provide persistent key-value storage for tools and agents, and document their usage in the SDK README.
New Features:
Enhancements:
Documentation: