-
Notifications
You must be signed in to change notification settings - Fork 0
fix: Address Copilot review comments for connect.py #27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add connect.py with CapiscIO.connect() and CapiscIO.from_env() - Add events.py with EventEmitter for agent observability - AgentIdentity dataclass with emit(), get_badge(), status() methods - Auto-discover agent, generate keys via gRPC, register DID - Update quickstart docs with new one-liner setup - Regenerate proto bindings with Init RPC support
- Fix BadgeKeeper.get_badge() -> get_current_badge() (2 places) - Fix proto import paths: from capiscio.v1 -> from capiscio_sdk._rpc.gen.capiscio.v1 - Remove unused json imports from connect.py and events.py
- Add 32 tests for connect.py (AgentIdentity, CapiscIO.connect, from_env, _Connector) - Add 28 tests for events.py (EventEmitter, global functions) - Improve patch coverage for new Let's Encrypt-style setup feature
- Add tests for full connect() flow with/without badge - Add tests for _ensure_agent error paths - Add tests for _create_agent edge cases - Add tests for _init_identity with RPC and existing files - Add tests for _setup_badge success and failure - Coverage improved from 63% to 99% for connect.py
…leanup - connect.py: Add AgentIdentity.close() for resource cleanup - connect.py: Add finally block in connect() to close httpx/gRPC clients - connect.py: Sanitize error messages to not expose internal RPC details - connect.py: Fix badge_expires_at attribute access with hasattr - events.py: Add threading.Lock for thread-safe batch operations - events.py: Wrap emit() and flush() batch access with _batch_lock - test_connect.py: Remove unused PropertyMock import - test_events.py: Remove unused imports (time, datetime, timezone, _global_emitter)
Directly set _rpc_client on connector instead of patching, avoids naming conflict between capiscio_sdk.connect module and CapiscIO.connect method
- Wrap HTTP calls in try/except for httpx.RequestError - Remove resp.text from error messages to prevent API key exposure - Add tests for network error scenarios
|
✅ Documentation validation passed!
|
|
✅ All checks passed! Ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR applies follow-up fixes to the new “Let’s Encrypt-style” CapiscIO.connect() flow by improving network error handling/sanitization, adding event emission infrastructure with batching, and extending unit test coverage for connection + network failure paths.
Changes:
- Add/extend
connect.pywith httpx request error handling and anAgentIdentity.close()cleanup path. - Introduce
events.pywith a batchedEventEmitterand global convenience functions, plus unit tests. - Regenerate/adjust gRPC proto bindings and bump dependencies (notably
protobuf), plus update quickstart docs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| capiscio_sdk/connect.py | Implements connect flow updates, adds cleanup, and improves network error handling. |
| capiscio_sdk/events.py | Adds batched event emission with locking and global helper functions. |
| capiscio_sdk/_rpc/client.py | Adds SimpleGuard init() RPC wrapper for “one-call” identity init. |
| capiscio_sdk/_rpc/gen/** | Regenerated protobuf/grpc bindings reflecting new RPC + version bump. |
| capiscio_sdk/init.py | Exposes CapiscIO/connect/from_env/AgentIdentity and EventEmitter at package level. |
| tests/unit/test_connect.py | Adds unit tests for connect flow, error paths, and network exception handling. |
| tests/unit/test_events.py | Adds unit tests for EventEmitter behavior and global event helpers. |
| docs/getting-started/quickstart.md | Updates Quick Start to highlight CapiscIO.connect() / from_env() flow. |
| pyproject.toml | Updates protobuf version constraint and adds base58 to dev dependencies. |
Comments suppressed due to low confidence (9)
tests/unit/test_connect.py:736
- This test imports
ConfigurationErrorbut never uses it, which will be flagged by linters (e.g., ruff F401). Consider removing the unused import.
from capiscio_sdk.connect import ConfigurationError
capiscio_sdk/connect.py:405
_setup_badge()creates aSimpleGuardinstance which will connect to capiscio-core and (based onSimpleGuard’s implementation) load/generate keys underbase_dir/capiscio_keys. Sincebase_dirhere isself.keys_dir.parent, this can create/modify files outside the agent’skeys_dirused byCapiscIO.connect(), which is a surprising side effect for “badge setup”. Consider deferring guard creation, passing an explicitbadge_tokeninstead, or otherwise ensuring it doesn’t generate new keys/directories as part of connect.
# Set up SimpleGuard with correct parameters
guard = SimpleGuard(
base_dir=str(self.keys_dir.parent),
agent_id=self.agent_id,
dev_mode=self.dev_mode,
)
capiscio_sdk/connect.py:309
_ensure_agent()assumesresp.json()returns a dict and unconditionally callsdata.get(...). If the registry ever returns a bare list for/v1/agents, this will raiseAttributeErrorand break connect. Consider handlingdictvslistexplicitly (e.g., ifisinstance(data, list)treat it as the agents list; ifdict, readdata["data"]).
data = resp.json()
agents = data.get("data", data) if isinstance(data.get("data", data), list) else []
capiscio_sdk/connect.py:413
_setup_badge()constructs aSimpleGuard, but theBadgeKeeperis not configured with anon_renewcallback to update the guard’s badge token. This makes theguardinstance effectively unused and prevents consumers from using it for outbound headers that track badge renewal. Consider wiringon_renewtoguard.set_badge_token(and/or avoid constructingSimpleGuardhere if it isn’t needed).
try:
from .badge_keeper import BadgeKeeper
from .simple_guard import SimpleGuard
# Set up SimpleGuard with correct parameters
guard = SimpleGuard(
base_dir=str(self.keys_dir.parent),
agent_id=self.agent_id,
dev_mode=self.dev_mode,
)
# Set up BadgeKeeper with correct parameters
keeper = BadgeKeeper(
api_url=self.server_url,
api_key=self.api_key,
agent_id=self.agent_id,
mode="dev" if self.dev_mode else "ca",
output_file=str(self.keys_dir / "badge.jwt"),
)
capiscio_sdk/connect.py:96
AgentIdentity.close()cleans up the emitter and keeper, but it never closes_guard. SinceSimpleGuardopens a gRPC connection in its constructor, this will leak resources whenCapiscIO.connect()created a guard in_setup_badge(). Consider calling_guard.close()(and setting it toNone) duringclose().
def close(self) -> None:
"""Clean up resources."""
if self._emitter:
self._emitter.close()
self._emitter = None
if self._keeper:
try:
self._keeper.stop()
except Exception:
pass
self._keeper = None
tests/unit/test_connect.py:546
- Variable result is not used.
result = connector._create_agent()
tests/unit/test_connect.py:17
- Import of 'ConfigurationError' is not used.
from capiscio_sdk.connect import (
AgentIdentity,
CapiscIO,
_Connector,
ConfigurationError,
DEFAULT_CONFIG_DIR,
DEFAULT_KEYS_DIR,
PROD_REGISTRY,
)
capiscio_sdk/connect.py:93
- 'except' clause does nothing but pass and there is no explanatory comment.
except Exception:
capiscio_sdk/connect.py:282
- 'except' clause does nothing but pass and there is no explanatory comment.
except Exception:
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
✅ Integration tests passed! Server validation, BadgeKeeper, and gRPC tests all working. |
Follow-up fixes from Copilot review on PR #26:
_ensure_agent()and_create_agent()resp.text)threading.Lockin events.pyAgentIdentity.close()method for resource cleanup