fix(cache): Address Review #1580 findings - prevent data loss#65
Closed
fix(cache): Address Review #1580 findings - prevent data loss#65
Conversation
Optional Password Protection: - Add use_encryption parameter to CredentialManager.save_credentials() - Credentials can now be stored as plaintext JSON (credentials.json) or encrypted (credentials.enc) based on user preference - Add checkbox toggle in CredentialSetupScreen (default: no encryption) - Update CacheManager to support unencrypted caching when no encryption key - Auto-detect credential type on load and skip password prompt for plaintext - Plaintext files use restricted permissions (0600) for security MCP Server for Claude Desktop Integration: - Add moneyflow/mcp/ module with FastMCP-based server - Tools: search_transactions, get_transactions, get_spending_summary, get_categories, get_merchants, get_account_info, refresh_data - Resources: account info, categories, top merchants, monthly spending, recent transactions - CLI entry point: moneyflow-mcp (stdio or streamable-http transport) - Supports Monarch Money and YNAB backends - Lazy initialization on first tool/resource access 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add five new tools to help with transaction categorization: - get_uncategorized_transactions: Find transactions needing categories - update_transaction_category: Update single transaction category - batch_update_category: Bulk category updates for efficiency - get_amazon_order_details: Look up Amazon order items to aid categorization - get_transaction_details: Get full transaction information These tools enable Claude Desktop to assist with categorizing Amazon purchases by revealing what items were ordered, making it easy to assign appropriate categories. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Security improvements: - Add --read-only flag to disable all write operations - Add MAX_LIMIT (1000) to cap query results and prevent memory issues - Add MAX_BATCH_SIZE (100) to limit batch operations - Add dry_run parameter to update operations for safe previewing - Add security warning banner for HTTP transport The MCP server is designed to run on trusted networks (localhost or Tailscale). These safeguards help prevent accidental data modifications and resource exhaustion. Tests: - Add 36 unit tests covering security features, tool behavior, and error handling 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The MCP server cannot prompt for a password interactively when running via stdio transport. This change: - Detects encrypted credentials before attempting to load them - Checks for MONEYFLOW_PASSWORD environment variable - Provides a clear error message with options if password is needed - Documents the environment variable in CLI help Users with encrypted credentials can now either: 1. Set MONEYFLOW_PASSWORD env var before starting the server 2. Use an account with unencrypted credentials 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Remove invalid force_refresh parameter from fetch_all_data call - Allow cache initialization for plaintext credentials (CacheManager supports both modes) - Fix credential loading to prefer encrypted over plaintext when both exist - Add literal=True to Polars str.contains to prevent regex injection - Add tests for plaintext credentials, precedence, and tool signatures Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Change invalid 'border: dashed' to 'border: round' (valid Textual CSS) - Remove compound class selector '.encryption-fields.visible' (not supported) - Use direct display property toggle instead of class-based visibility Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add comprehensive test suite to catch UI issues like CSS parse errors: - CSS validation tests for all 15+ screen classes - Integration tests for account selection workflow - Integration tests for credential setup workflow - Demo mode and keyboard navigation tests - Error handling tests These tests would have caught the CSS parse error fixed in the previous commit (invalid 'border: dashed' and compound class selectors). Note: 2 credential setup integration tests are skipped due to Textual test runner limitations with CSS variable resolution when pushing screens. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The $text-muted CSS variable only works for the `color` property in Textual CSS, not for `border` or `background` properties. This was causing a CSS parse error when opening the credential setup screen. Also cleans up test file to remove unused imports and simplified test assertions. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add a "%" column to aggregate views showing each row's share of the total. Percentages are calculated separately for income and expenses: - Income rows show percentage of total income, displayed in green - Expense rows show percentage of total expenses, in standard color This helps users quickly see the relative weight of each category, merchant, or group within their income or spending. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Addresses Review #1034 and #1038 findings: - Replace hard-coded /tmp/test with tmp_path fixture for proper test isolation - Add timeout assertion to _wait_for_app_ready helper - returns bool and callers now assert the return value to catch initialization failures - Remove unused mock_backend fixture, rename test to test_demo_mode_populates_state_data to accurately reflect what it tests - Improve test_credential_setup_screen_mounts to verify key widgets are present (email input, password input, encryption checkbox) - Improve test_credential_setup_encryption_toggle to actually test the toggle behavior - verifies checkbox state and field visibility before/after toggle - Remove unused imports (Path, Input) flagged by ruff F401 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Medium severity - File permission race condition: - Add _secure_write_file helper that uses os.open with 0o600 mode to create files with restrictive permissions from the start - Eliminates race window where files briefly have default permissions - Applied to credentials.py (encrypted + plaintext creds, salt file) - Applied to cache_manager.py (_save_parquet, _save_categories, _save_metadata) Low severity - Category name disambiguation in MCP: - update_transaction_category now detects duplicate category names - When multiple categories share a name, returns error with IDs for disambiguation - Added optional category_id parameter as alternative to category_name - Use category_id when names are ambiguous across groups/backends Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Create shared file_utils.py module with secure_write_file and secure_atomic_write functions to eliminate code duplication - Fix fd leak in secure_write_file error handling path - Fix memory regression in _save_parquet: use streaming write with atomic rename for unencrypted mode instead of buffering - Add explicit fchmod to ensure 0o600 permissions on file overwrites - Add category_id parameter to MCP update_transaction_category to disambiguate duplicate category names - Add tests for file permissions and MCP category disambiguation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Replace shutil.move with os.replace for guaranteed atomic rename (shutil.move can fall back to copy+remove on some platforms) - Fix potential short-write issue in secure_atomic_write by using os.fdopen with .write() + flush/fsync instead of raw os.write - Remove unused shutil import from cache_manager.py - Add functional MCP tests that actually call tools via mcp.call_tool() instead of only inspecting source code strings The functional tests verify: - Missing both category_name and category_id returns error - Providing both params returns error - Duplicate category names returns disambiguation error with IDs - category_id bypasses name lookup for disambiguation - Invalid category_id returns error Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Restructure flake to use let block for custom package definitions - Add MCP (Model Context Protocol) package v1.25.0 and its dependencies: - httpx-sse v0.4.0 - pydantic-settings v2.7.1 - sse-starlette v2.2.1 - typing-inspection v0.4.0 - Update package version to 0.8.1 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The previous hashes were incorrect. Recalculated using actual wheel downloads and proper hex-to-base64 conversion. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Root cause: In handle_commit_result(), the `edits` parameter was the same list object as `self.data_manager.pending_edits`. When we called `pending_edits.clear()` BEFORE the cache update, it emptied the `edits` list (Python passes lists by reference), so the cache update applied zero edits. The fix moves `pending_edits.clear()` to AFTER the cache update completes. Also improved handling when cache tiers can't be loaded in filtered view: - Previously: silently skipped cache update entirely - Now: updates whichever tier(s) can be loaded, with detailed logging Added tests: - 5 new cache persistence tests in test_cache.py - 2 new partial cache update tests in test_app_controller.py Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Format tests/test_cache.py to pass ruff format check - Update typing-inspection wheel URL (old URL returned 404) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Review findings addressed: - medium: Remove dangerous save_cache fallback in filtered view when both cache tiers unavailable. In filtered view, data_manager.df only contains the filtered subset, so saving it would overwrite full cache with partial data and lose history. Now just logs error - edits are saved to backend. - low: Strengthen test assertion in test_multiple_sequential_saves to use a known hot cache ID and assert exact final value (EDIT_4) - low: Add test coverage for "both tiers unavailable" path to prevent future regressions Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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
1 participant
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
Review Findings Addressed
save_cachefallback when both cache tiers unavailable in filtered view. In filtered view,data_manager.dfonly contains the filtered subset, so saving it would overwrite full cache with partial data and lose historical transactions.test_multiple_sequential_saves_persist_correctlyto use a known hot cache ID and assert exact final value (EDIT_4)Test plan
🤖 Generated with Claude Code