Skip to content

Remove sync_status_service#365

Closed
phernandez wants to merge 1 commit intomainfrom
claude/issue-363-20251016-1405
Closed

Remove sync_status_service#365
phernandez wants to merge 1 commit intomainfrom
claude/issue-363-20251016-1405

Conversation

@phernandez
Copy link
Copy Markdown
Member

Removes sync_status_service and all related functionality as requested in issue #363.

Changes

  • Removed 4 files (service, MCP tool, and tests)
  • Removed sync_status_tracker calls from 7 files
  • Total removal: 1,134 lines

Closes #363

Generated with Claude Code

- Removed sync_status_service.py tracking service
- Removed sync_status MCP tool
- Removed all sync_status_tracker calls from:
  - sync_service.py (8 calls)
  - initialization.py (4 calls)
  - mcp/tools/utils.py (2 utility functions)
- Removed migration status checks from MCP tools:
  - write_note.py
  - read_note.py
  - build_context.py
- Removed associated test files and test cases
- Total removal: 1,134 lines

Closes #363

Co-authored-by: Paul Hernandez <phernandez@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Oct 16, 2025

Claude finished @phernandez's task —— View job


Code Review: PR #365 - Remove sync_status_service

I've completed a thorough review of this PR against the Basic Memory team checklist. The removal is well-executed but has 3 issues that must be fixed before merging.


Critical Issues (Must Fix)

1. Broken Import in src/basic_memory/mcp/tools/__init__.py

Lines 21, 47 - The module still imports and exports the deleted sync_status tool:

from basic_memory.mcp.tools.sync_status import sync_status  # Line 21 - BROKEN
# ...
__all__ = [
    # ...
    "sync_status",  # Line 47 - SHOULD BE REMOVED
]

Impact: This will cause an ImportError when the basic_memory.mcp.tools module is imported, breaking the entire MCP server.

Fix Required: Remove both the import statement (line 21) and the export from __all__ (line 47).

2. Outdated Documentation in CLAUDE.md

Line 178 - Still lists sync_status() as an available MCP tool under "Project Management":

- `sync_status()` - Check file synchronization and background operation status

Impact: Misleads developers and AI assistants about available functionality.

Fix Required: Remove this line from the MCP Capabilities section.

3. Outdated Documentation in README.md

Line 398 - Still lists sync_status() under Project Management tools:

sync_status() - Check synchronization status

Impact: Misleads users about available MCP tools.

Fix Required: Remove this line from the Project Management tools section.


Team Checklist Results

✅ Code Quality & Standards

  • Follows Basic Memory's coding conventions - Deletions only, no new code
  • Python 3.12+ type annotations - N/A (deletions only)
  • 100-character line length - N/A (deletions only)
  • No commented-out code blocks - Clean removals

⚠️ Testing & Documentation

  • Test coverage - Properly removed test files (test_tool_sync_status.py, test_sync_status_service.py, and test cases in test_tool_utils.py)
  • [❌] Documentation updated - CLAUDE.md and README.md still reference the removed tool

✅ Basic Memory Architecture

  • MCP tools remain atomic and composable - Removed blocking status checks from tools
  • No database migrations needed - No schema changes
  • Preserves local-first architecture - Simplifies by removing status tracking overhead
  • Knowledge graph consistency maintained - No impact on graph operations

✅ Security & Performance

  • No security issues - Deletions only
  • Proper error handling - Removed complex status tracking reduces error surface
  • Performance improvements - Removes blocking status checks from critical paths (5-second timeouts removed from write_note, read_note, build_context)

Positive Aspects

  1. Comprehensive Removal - Successfully removed:

    • Core service (sync_status_service.py - 198 lines)
    • MCP tool (sync_status.py - 261 lines)
    • All test files (3 files, 513 lines)
    • All integration points (sync_service, initialization, MCP tool utilities)
  2. Clean Code Changes - All modifications are clean deletions of:

    • Import statements
    • Status tracking calls (8 in sync_service, 4 in initialization, 2 utility functions)
    • Migration status checks from 3 MCP tools (removing 5-second blocking timeouts)
  3. Performance Win - Eliminates blocking wait_for_migration_or_return_status() calls that could delay MCP operations by up to 5 seconds

  4. Reduced Complexity - Removes 1,134 lines of status tracking code, simplifying the codebase


Files Changed Analysis

Deleted Files (4):

  • src/basic_memory/services/sync_status_service.py (198 lines)
  • src/basic_memory/mcp/tools/sync_status.py (261 lines)
  • tests/services/test_sync_status_service.py (262 lines)
  • tests/mcp/test_tool_sync_status.py (170 lines)

Modified Files (7):

  • src/basic_memory/sync/sync_service.py - Removed 8 status tracking calls
  • src/basic_memory/services/initialization.py - Removed 4 status tracking calls
  • src/basic_memory/mcp/tools/utils.py - Removed 2 utility functions
  • src/basic_memory/mcp/tools/write_note.py - Removed blocking migration check
  • src/basic_memory/mcp/tools/read_note.py - Removed blocking migration check
  • src/basic_memory/mcp/tools/build_context.py - Removed blocking migration check
  • tests/mcp/test_tool_utils.py - Removed 81 lines of test cases

Missed Files (3):

  • src/basic_memory/mcp/tools/__init__.py - Still imports deleted tool
  • CLAUDE.md - Still documents deleted tool
  • README.md - Still documents deleted tool

Recommendations

Before Merging:

  1. Fix the broken import in __init__.py (lines 21, 47)
  2. Update CLAUDE.md (line 178)
  3. Update README.md (line 398)
  4. Run just test to verify no import errors
  5. Consider running just lint and just typecheck to catch any issues

Note on Cloud Commands: The grep results also showed references to show_bisync_status() and project_sync_status() - these are correctly preserved as they refer to different functionality (cloud bisync status and project scan endpoints), not the removed sync_status_service.


Summary

This is a well-executed removal that eliminates 1,134 lines of status tracking complexity and improves MCP tool performance by removing blocking timeout calls. The changes are clean and comprehensive.

However, the PR cannot be merged until the 3 documentation and import issues are resolved, as the broken import will cause runtime failures.

Recommendation: Request changes to fix the import and documentation issues, then approve.

--- | Branch: claude/issue-363-20251016-1405

@phernandez phernandez closed this Oct 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove sync_status_service

1 participant