253 lines
9.2 KiB
Markdown
253 lines
9.2 KiB
Markdown
|
# Design Document
|
||
|
|
|
||
|
|
## Overview
|
||
|
|
|
||
|
|
This design addresses critical code quality issues in the Obsidian journal organizer project. The improvements focus on fixing syntax errors, adding comprehensive type hints, modernizing Python patterns, and establishing consistent error handling throughout the codebase. The design maintains backward compatibility while significantly improving maintainability and developer experience.
|
||
|
|
|
||
|
|
## Architecture
|
||
|
|
|
||
|
|
The existing Command + Skill architecture will be preserved, but enhanced with:
|
||
|
|
|
||
|
|
1. **Type Safety Layer**: Comprehensive type hints using Python's typing module
|
||
|
|
2. **Error Handling Framework**: Consistent error patterns across all components
|
||
|
|
3. **Configuration Validation**: Robust validation and sanitization of user inputs
|
||
|
|
4. **Modern Python Patterns**: Updated code to follow current best practices
|
||
|
|
|
||
|
|
## Components and Interfaces
|
||
|
|
|
||
|
|
### Enhanced Agent Core
|
||
|
|
|
||
|
|
The `agent_core.py` module will be updated with:
|
||
|
|
|
||
|
|
```python
|
||
|
|
from typing import Dict, Any, List, Optional, Union, Protocol, TypeVar
|
||
|
|
from dataclasses import dataclass, field
|
||
|
|
from pathlib import Path
|
||
|
|
from abc import ABC, abstractmethod
|
||
|
|
|
||
|
|
T = TypeVar('T')
|
||
|
|
|
||
|
|
class SkillProtocol(Protocol):
|
||
|
|
async def execute(self, context: 'CommandContext', **kwargs: Any) -> 'SkillResult':
|
||
|
|
...
|
||
|
|
|
||
|
|
@dataclass
|
||
|
|
class SkillResult:
|
||
|
|
success: bool
|
||
|
|
data: Optional[Dict[str, Any]] = None
|
||
|
|
error: Optional[str] = None
|
||
|
|
message: str = ""
|
||
|
|
timestamp: str = field(default_factory=lambda: datetime.now().isoformat())
|
||
|
|
|
||
|
|
def to_dict(self) -> Dict[str, Any]:
|
||
|
|
return asdict(self)
|
||
|
|
```
|
||
|
|
|
||
|
|
### Type-Safe Configuration Management
|
||
|
|
|
||
|
|
```python
|
||
|
|
from typing import TypedDict, Optional
|
||
|
|
from pathlib import Path
|
||
|
|
|
||
|
|
class ObsidianConfig(TypedDict):
|
||
|
|
vault_path: str
|
||
|
|
rest_api: Dict[str, Union[str, bool]]
|
||
|
|
|
||
|
|
class ClaudeConfig(TypedDict):
|
||
|
|
api_key: str
|
||
|
|
model: str
|
||
|
|
max_tokens: int
|
||
|
|
|
||
|
|
class SystemConfig(TypedDict):
|
||
|
|
obsidian: ObsidianConfig
|
||
|
|
claude: ClaudeConfig
|
||
|
|
output: Dict[str, str]
|
||
|
|
```
|
||
|
|
|
||
|
|
### Enhanced Error Handling
|
||
|
|
|
||
|
|
```python
|
||
|
|
class JournalOrganizerError(Exception):
|
||
|
|
"""Base exception for journal organizer errors"""
|
||
|
|
pass
|
||
|
|
|
||
|
|
class ConfigurationError(JournalOrganizerError):
|
||
|
|
"""Raised when configuration is invalid or missing"""
|
||
|
|
pass
|
||
|
|
|
||
|
|
class APIError(JournalOrganizerError):
|
||
|
|
"""Raised when external API calls fail"""
|
||
|
|
pass
|
||
|
|
|
||
|
|
class ValidationError(JournalOrganizerError):
|
||
|
|
"""Raised when input validation fails"""
|
||
|
|
pass
|
||
|
|
```
|
||
|
|
|
||
|
|
## Data Models
|
||
|
|
|
||
|
|
### Enhanced SkillResult with Validation
|
||
|
|
|
||
|
|
```python
|
||
|
|
@dataclass
|
||
|
|
class SkillResult:
|
||
|
|
success: bool
|
||
|
|
data: Optional[Dict[str, Any]] = None
|
||
|
|
error: Optional[str] = None
|
||
|
|
message: str = ""
|
||
|
|
timestamp: str = field(default_factory=lambda: datetime.now().isoformat())
|
||
|
|
|
||
|
|
def __post_init__(self) -> None:
|
||
|
|
if not self.success and not self.error:
|
||
|
|
raise ValueError("Failed results must include error message")
|
||
|
|
if self.success and self.error:
|
||
|
|
raise ValueError("Successful results should not include error message")
|
||
|
|
```
|
||
|
|
|
||
|
|
### Configuration Validation Models
|
||
|
|
|
||
|
|
```python
|
||
|
|
from pydantic import BaseModel, validator, Field
|
||
|
|
from typing import Optional
|
||
|
|
|
||
|
|
class ObsidianRestAPIConfig(BaseModel):
|
||
|
|
url: str = Field(..., regex=r'^https?://')
|
||
|
|
api_key: str = Field(..., min_length=1)
|
||
|
|
verify_ssl: bool = False
|
||
|
|
|
||
|
|
class ObsidianConfig(BaseModel):
|
||
|
|
vault_path: str = Field(..., min_length=1)
|
||
|
|
rest_api: ObsidianRestAPIConfig
|
||
|
|
|
||
|
|
@validator('vault_path')
|
||
|
|
def validate_vault_path(cls, v: str) -> str:
|
||
|
|
path = Path(v)
|
||
|
|
if not path.exists():
|
||
|
|
raise ValueError(f"Vault path does not exist: {v}")
|
||
|
|
return str(path.resolve())
|
||
|
|
```
|
||
|
|
|
||
|
|
## Error Handling
|
||
|
|
|
||
|
|
### Centralized Error Management
|
||
|
|
|
||
|
|
```python
|
||
|
|
class ErrorHandler:
|
||
|
|
"""Centralized error handling and logging"""
|
||
|
|
|
||
|
|
def __init__(self, logger: logging.Logger):
|
||
|
|
self.logger = logger
|
||
|
|
|
||
|
|
def handle_api_error(self, error: Exception, context: str) -> SkillResult:
|
||
|
|
"""Handle API-related errors consistently"""
|
||
|
|
self.logger.error(f"API error in {context}: {str(error)}")
|
||
|
|
return SkillResult(
|
||
|
|
success=False,
|
||
|
|
error=f"API call failed: {str(error)}",
|
||
|
|
message=f"Failed to complete {context}"
|
||
|
|
)
|
||
|
|
|
||
|
|
def handle_validation_error(self, error: Exception, field: str) -> SkillResult:
|
||
|
|
"""Handle validation errors with user-friendly messages"""
|
||
|
|
self.logger.warning(f"Validation error for {field}: {str(error)}")
|
||
|
|
return SkillResult(
|
||
|
|
success=False,
|
||
|
|
error=f"Invalid {field}: {str(error)}",
|
||
|
|
message="Please check your input and try again"
|
||
|
|
)
|
||
|
|
```
|
||
|
|
|
||
|
|
### Async Context Managers for Resources
|
||
|
|
|
||
|
|
```python
|
||
|
|
from contextlib import asynccontextmanager
|
||
|
|
from typing import AsyncGenerator
|
||
|
|
|
||
|
|
@asynccontextmanager
|
||
|
|
async def obsidian_client(config: ObsidianConfig) -> AsyncGenerator[aiohttp.ClientSession, None]:
|
||
|
|
"""Async context manager for Obsidian API client"""
|
||
|
|
ssl_context = ssl.create_default_context()
|
||
|
|
ssl_context.check_hostname = False
|
||
|
|
ssl_context.verify_mode = ssl.CERT_NONE
|
||
|
|
|
||
|
|
connector = aiohttp.TCPConnector(ssl=ssl_context)
|
||
|
|
headers = {
|
||
|
|
'Authorization': f'Bearer {config.rest_api.api_key}',
|
||
|
|
'Content-Type': 'application/json'
|
||
|
|
}
|
||
|
|
|
||
|
|
async with aiohttp.ClientSession(connector=connector, headers=headers) as session:
|
||
|
|
try:
|
||
|
|
yield session
|
||
|
|
except Exception as e:
|
||
|
|
logger.error(f"Error in Obsidian client: {str(e)}")
|
||
|
|
raise APIError(f"Obsidian API error: {str(e)}") from e
|
||
|
|
```
|
||
|
|
|
||
|
|
## Testing Strategy
|
||
|
|
|
||
|
|
### Dual Testing Approach
|
||
|
|
|
||
|
|
The testing strategy combines unit tests for specific functionality with property-based tests for comprehensive validation:
|
||
|
|
|
||
|
|
**Unit Tests:**
|
||
|
|
- Test specific error conditions and edge cases
|
||
|
|
- Validate configuration parsing and validation
|
||
|
|
- Test individual Skill and Command functionality
|
||
|
|
- Mock external API calls for isolated testing
|
||
|
|
|
||
|
|
**Property-Based Tests:**
|
||
|
|
- Validate that all SkillResult objects maintain consistency
|
||
|
|
- Test configuration validation across various input combinations
|
||
|
|
- Verify error handling patterns work for all error types
|
||
|
|
- Ensure type hints are correctly applied
|
||
|
|
|
||
|
|
**Testing Framework:**
|
||
|
|
- Use `pytest` for unit testing framework
|
||
|
|
- Use `hypothesis` for property-based testing
|
||
|
|
- Configure tests to run minimum 100 iterations per property test
|
||
|
|
- Tag each property test with: **Feature: code-quality-improvements, Property {number}: {property_text}**
|
||
|
|
|
||
|
|
## Correctness Properties
|
||
|
|
|
||
|
|
*A property is a characteristic or behavior that should hold true across all valid executions of a system-essentially, a formal statement about what the system should do. Properties serve as the bridge between human-readable specifications and machine-verifiable correctness guarantees.*
|
||
|
|
|
||
|
|
### Property 1: Syntax Validity Across All Modules
|
||
|
|
*For any* Python module in the project, parsing the module with Python's AST parser should succeed without raising SyntaxError exceptions
|
||
|
|
**Validates: Requirements 1.1, 1.2, 1.3**
|
||
|
|
|
||
|
|
### Property 2: Comprehensive Type Hint Coverage
|
||
|
|
*For any* public method or function in Agent_Core, Skills, Commands, or Chat_Interface, the method should have complete type annotations for parameters and return values
|
||
|
|
**Validates: Requirements 2.1, 2.2, 2.3, 2.4, 2.5**
|
||
|
|
|
||
|
|
### Property 3: Consistent Error Handling Structure
|
||
|
|
*For any* error condition in Skills or Commands, the returned SkillResult should have a consistent structure with appropriate error messages and no sensitive information exposure
|
||
|
|
**Validates: Requirements 3.1, 3.2, 3.3, 3.4, 3.5**
|
||
|
|
|
||
|
|
### Property 4: Modern Python Code Patterns
|
||
|
|
*For any* source file in the project, the code should use f-strings for formatting, pathlib.Path for file operations, proper dataclass definitions, and follow PEP 8 style guidelines
|
||
|
|
**Validates: Requirements 4.1, 4.2, 4.3, 4.4, 4.5**
|
||
|
|
|
||
|
|
### Property 5: Configuration Validation Completeness
|
||
|
|
*For any* configuration input (valid or invalid), the system should validate all required fields, provide specific error messages for invalid values, and handle missing API keys with clear guidance
|
||
|
|
**Validates: Requirements 5.1, 5.2, 5.4, 7.4**
|
||
|
|
|
||
|
|
### Property 6: Import Organization and Style
|
||
|
|
*For any* Python module in the project, imports should be organized according to PEP 8 standards, use absolute imports consistently, and handle missing dependencies gracefully
|
||
|
|
**Validates: Requirements 6.1, 6.2, 6.3, 6.4, 6.5**
|
||
|
|
|
||
|
|
### Property 7: Input Validation and Sanitization
|
||
|
|
*For any* user input (file paths, dates, configuration values), the system should validate format and constraints, sanitize potentially dangerous inputs, and reject malformed data
|
||
|
|
**Validates: Requirements 7.1, 7.2, 7.3, 7.5**
|
||
|
|
|
||
|
|
### Property 8: Environment Variable Expansion
|
||
|
|
*For any* configuration value containing environment variable references, the system should correctly expand the variables or provide clear error messages when variables are undefined
|
||
|
|
**Validates: Requirements 5.3**
|
||
|
|
|
||
|
|
### Property 9: String Formatting Consistency
|
||
|
|
*For any* string formatting operation in the codebase, the system should use consistent and appropriate formatting patterns (f-strings, proper escaping, etc.)
|
||
|
|
**Validates: Requirements 1.4**
|
||
|
|
|
||
|
|
### Property-Based Testing Configuration
|
||
|
|
|
||
|
|
Each property test will be tagged with comments referencing the design document property and run with sufficient iterations to catch edge cases through randomization.
|