|
| 1 | +# Improve Secrets API UX and Error Handling |
| 2 | + |
| 3 | +## Description |
| 4 | + |
| 5 | +Improve the secrets API user experience by addressing critical usability pitfalls and enhancing error handling to prevent common developer mistakes. The current API has several issues that lead to confusion and debugging difficulties: |
| 6 | + |
| 7 | +1. **Misleading parameter naming**: `filename` parameter in `load_secrets_from_file()` is actually treated as a path component |
| 8 | +2. **Silent failure**: Missing files return empty HashMap instead of errors |
| 9 | +3. **Poor error context**: Error messages don't explain path resolution logic |
| 10 | +4. **Inadequate documentation**: Examples don't clarify filename vs. path distinction |
| 11 | + |
| 12 | +This task focuses on improving developer experience through better API design, explicit error handling, comprehensive documentation, and helpful debugging tools. The improvements maintain full backward compatibility while adding new methods and enhancing existing ones. |
| 13 | + |
| 14 | +Based on real-world usage analysis from `api_huggingface` project where developers attempted `load_secrets_from_file("lib/llm_tools/.secret/-secrets.sh")` expecting it to work as a path, but the API treated it as a filename, resulting in silent failure and debugging confusion. |
| 15 | + |
| 16 | +## Requirements |
| 17 | + |
| 18 | +- All work must strictly adhere to the rules defined in the following rulebooks: |
| 19 | + - `$PRO/genai/code/rules/code_design.rulebook.md` |
| 20 | + - `$PRO/genai/code/rules/code_style.rulebook.md` |
| 21 | + |
| 22 | +## Acceptance Criteria |
| 23 | + |
| 24 | +### Phase 1: Enhanced Error Handling and Validation |
| 25 | + |
| 26 | +- [ ] **Explicit file existence errors**: Replace silent empty HashMap returns with explicit `WorkspaceError::ConfigurationError` when files don't exist |
| 27 | +- [ ] **Path validation warnings**: Detect when `filename` parameter contains path separators (`/` or `\`) and emit helpful warnings |
| 28 | +- [ ] **Enhanced error context**: Error messages must include both original parameter and resolved absolute path |
| 29 | +- [ ] **Available files suggestions**: When a file is not found, suggest available files in the secrets directory |
| 30 | + |
| 31 | +### Phase 2: API Method Improvements |
| 32 | + |
| 33 | +- [ ] **Parameter renaming**: Rename `filename` parameter to `secret_file_name` in `load_secrets_from_file()` with deprecation warning |
| 34 | +- [ ] **New path-aware methods**: Add `load_secrets_from_path()` for workspace-relative paths and `load_secrets_from_absolute_path()` for absolute paths |
| 35 | +- [ ] **Debug helper methods**: Add `secrets_file_exists()`, `resolve_secrets_path()`, and `list_secrets_files()` |
| 36 | +- [ ] **Validation method**: Add `load_secrets_with_debug()` that provides verbose path resolution and validation information |
| 37 | + |
| 38 | +### Phase 3: Documentation and Examples Enhancement |
| 39 | + |
| 40 | +- [ ] **Comprehensive API documentation**: Update all secrets-related method documentation with clear examples showing correct vs incorrect usage |
| 41 | +- [ ] **Path resolution explanation**: Document how each method resolves paths with explicit examples |
| 42 | +- [ ] **Migration guide**: Create guide for common mistakes and how to fix them |
| 43 | +- [ ] **Example updates**: Update existing examples to demonstrate best practices and common pitfalls |
| 44 | + |
| 45 | +### Phase 4: Testing and Validation |
| 46 | + |
| 47 | +- [ ] **Pitfall prevention tests**: Add tests that verify error cases (missing files, path-like filenames) produce helpful error messages |
| 48 | +- [ ] **API consistency tests**: Ensure new methods integrate seamlessly with existing functionality |
| 49 | +- [ ] **Documentation tests**: All code examples in documentation must compile and run successfully |
| 50 | +- [ ] **Backward compatibility tests**: Existing code using old API must continue working with deprecation warnings only |
| 51 | + |
| 52 | +## Implementation Plan |
| 53 | + |
| 54 | +### Step 1: Enhanced Error Handling |
| 55 | +```rust |
| 56 | +// Current (silent failure) |
| 57 | +if !secret_file.exists() { |
| 58 | + return Ok( HashMap::new() ); |
| 59 | +} |
| 60 | + |
| 61 | +// New (explicit error) |
| 62 | +if !secret_file.exists() { |
| 63 | + let available = self.list_secrets_files().unwrap_or_default(); |
| 64 | + let suggestion = if !available.is_empty() { |
| 65 | + format!("\nAvailable files: {}", available.join(", ")) |
| 66 | + } else { |
| 67 | + String::new() |
| 68 | + }; |
| 69 | + |
| 70 | + return Err( WorkspaceError::ConfigurationError( |
| 71 | + format!( |
| 72 | + "Secrets file '{}' not found at {}{}", |
| 73 | + secret_file_name, |
| 74 | + secret_file.display(), |
| 75 | + suggestion |
| 76 | + ) |
| 77 | + ) ); |
| 78 | +} |
| 79 | +``` |
| 80 | + |
| 81 | +### Step 2: Parameter Validation |
| 82 | +```rust |
| 83 | +pub fn load_secrets_from_file( &self, secret_file_name : &str ) -> Result< HashMap< String, String > > { |
| 84 | + // Validate parameter doesn't look like a path |
| 85 | + if secret_file_name.contains('/') || secret_file_name.contains('\\') { |
| 86 | + eprintln!( |
| 87 | + "⚠️ Warning: '{}' contains path separators. Use load_secrets_from_path() for paths.", |
| 88 | + secret_file_name |
| 89 | + ); |
| 90 | + } |
| 91 | + |
| 92 | + // Rest of implementation |
| 93 | +} |
| 94 | +``` |
| 95 | + |
| 96 | +### Step 3: New API Methods |
| 97 | +```rust |
| 98 | +/// Load secrets from workspace-relative path |
| 99 | +pub fn load_secrets_from_path( &self, relative_path : &str ) -> Result< HashMap< String, String > > |
| 100 | + |
| 101 | +/// Load secrets from absolute path |
| 102 | +pub fn load_secrets_from_absolute_path( &self, absolute_path : &Path ) -> Result< HashMap< String, String > > |
| 103 | + |
| 104 | +/// List available secrets files |
| 105 | +pub fn list_secrets_files( &self ) -> Result< Vec< String > > |
| 106 | + |
| 107 | +/// Check if secrets file exists |
| 108 | +pub fn secrets_file_exists( &self, secret_file_name : &str ) -> bool |
| 109 | + |
| 110 | +/// Get resolved path for debugging |
| 111 | +pub fn resolve_secrets_path( &self, secret_file_name : &str ) -> PathBuf |
| 112 | +``` |
| 113 | + |
| 114 | +### Step 4: Documentation Template |
| 115 | +```rust |
| 116 | +/// Load secrets from a file in the workspace secrets directory |
| 117 | +/// |
| 118 | +/// # Path Resolution |
| 119 | +/// |
| 120 | +/// Files are resolved as: `workspace_root/.secret/{secret_file_name}` |
| 121 | +/// |
| 122 | +/// # Examples |
| 123 | +/// |
| 124 | +/// ```rust |
| 125 | +/// // ✅ Correct usage - simple filenames |
| 126 | +/// let secrets = ws.load_secrets_from_file("-secrets.sh")?; // -> .secret/-secrets.sh |
| 127 | +/// let dev = ws.load_secrets_from_file("dev.env")?; // -> .secret/dev.env |
| 128 | +/// |
| 129 | +/// // ❌ Common mistake - using paths |
| 130 | +/// // let secrets = ws.load_secrets_from_file("config/secrets.sh")?; // DON'T DO THIS |
| 131 | +/// |
| 132 | +/// // ✅ For paths, use the path-specific method |
| 133 | +/// let secrets = ws.load_secrets_from_path("config/secrets.sh")?; // -> workspace/config/secrets.sh |
| 134 | +/// ``` |
| 135 | +``` |
| 136 | + |
| 137 | +## Success Metrics |
| 138 | + |
| 139 | +- **Zero silent failures**: All missing file cases produce explicit errors |
| 140 | +- **Clear error messages**: All errors include both input parameter and resolved path |
| 141 | +- **Intuitive API**: Developers can distinguish between filename and path parameters |
| 142 | +- **Comprehensive documentation**: Examples cover both correct usage and common mistakes |
| 143 | +- **Backward compatibility**: Existing code works with deprecation warnings only |
| 144 | + |
| 145 | +## Migration Strategy |
| 146 | + |
| 147 | +1. **Phase 1**: Add new methods alongside existing ones |
| 148 | +2. **Phase 2**: Add deprecation warnings to methods with confusing parameters |
| 149 | +3. **Phase 3**: Update all documentation and examples |
| 150 | +4. **Phase 4**: Plan future version with renamed parameters as defaults |
| 151 | + |
| 152 | +## Related Issues |
| 153 | + |
| 154 | +This task addresses developer experience issues discovered in: |
| 155 | +- `api_huggingface` project secret loading confusion |
| 156 | +- General workspace_tools API usability feedback |
| 157 | +- Need for better debugging tools for path resolution |
| 158 | + |
| 159 | +## Priority: High |
| 160 | + |
| 161 | +**Value**: 9/10 - Critical UX improvement preventing common developer mistakes |
| 162 | +**Easiness**: 7/10 - Mostly additive changes with clear implementation path |
| 163 | +**Safety**: 9/10 - Maintains backward compatibility while improving safety |
| 164 | +**Advisability**: 10/10 - Essential for developer productivity and API adoption |
0 commit comments