Skip to content

Conversation

@phernandez
Copy link
Member

Summary

  • add fast feedback loop targets (testmon, test-smoke, fast-check, doctor) in justfile
  • add basic-memory doctor command for a clean, self-checking loop
  • add MCP smoke + tool-contract tests to validate tool signatures
  • wire up pytest-testmon, smoke marker, and ignore .testmondata*
  • refresh docs for the code/test/verify loop and keep CLAUDE.md as a symlink to AGENTS.md
  • include minor formatting fixes in existing tests

Why

  • shorten the local verification loop for agent-driven changes
  • catch tool-contract drift and basic integration issues earlier
  • make it easy to run a clean, reproducible “does this work on my machine?” check

How

  • added a doctor CLI entry point that runs with a temporary HOME/config
  • introduced focused MCP smoke/contract tests to validate tool names and args
  • standardized the fast path in justfile and documented the loop

Testing

  • just fast-check
  • just test-smoke
  • uv run pytest tests/mcp/test_tool_contracts.py
  • just doctor

Signed-off-by: phernandez <paul@basicmachines.co>
@github-actions
Copy link
Contributor

github-actions bot commented Feb 1, 2026

Claude finished @phernandez's task in 1m 17s —— View job


Reviewing PR: Fast Feedback Loop Tooling

Todo List

  • Read and understand changed files
  • Review code quality & standards
  • Review testing & documentation
  • Review Basic Memory architecture compliance
  • Check security & performance
  • Check compatibility
  • Post comprehensive review feedback

Review Summary

This PR successfully adds fast feedback loop tooling to improve the development workflow. The implementation is solid with good test coverage and documentation. I've identified a few items that need attention:

✅ Code Quality & Standards

  • Follows Basic Memory's coding conventions in CLAUDE.md
  • Python 3.12+ type annotations and async patterns
  • SQLAlchemy 2.0 best practices
  • FastAPI and Typer conventions followed
  • 100-character line length limit maintained
  • No commented-out code blocks

Observations:

  • Code follows project conventions well
  • Type hints are properly used throughout
  • Async patterns correctly implemented

⚠️ Testing & Documentation

  • Unit tests for new functions/methods (test_tool_contracts.py)
  • Integration tests for new MCP tools (test_smoke_integration.py)
  • Test coverage for edge cases
  • 100% test coverage maintained - doctor.py is missing test coverage
  • Documentation updated (README, AGENTS.md)
  • CLAUDE.md updated (now a symlink to AGENTS.md)

Issue: The new doctor.py command (src/basic_memory/cli/commands/doctor.py:1-154) lacks dedicated unit tests. While it's exercised via the just doctor integration test, CLAUDE.md states: "Coverage must stay at 100%: Write tests for new code." The CLI omit pattern in pyproject.toml:143 may cover this, but please verify coverage doesn't drop.

Fix test coverage →

✅ Basic Memory Architecture

  • MCP tools follow atomic, composable design
  • Database changes include Alembic migrations (N/A for this PR)
  • Preserves local-first architecture principles
  • Knowledge graph operations maintain consistency
  • Markdown file handling preserves integrity
  • AI-human collaboration patterns followed

Observations:

  • The doctor command properly uses temporary directories and isolated config
  • MCP smoke tests validate end-to-end tool contracts
  • Follows the async client pattern correctly (get_client() context manager)

✅ Security & Performance

  • No hardcoded secrets or credentials
  • Input validation for MCP tools
  • Proper error handling and logging
  • Performance considerations addressed
  • No sensitive data in logs or commits

Observations:

  • doctor.py properly handles cleanup in finally block (lines 128-130)
  • Temporary directories are used securely
  • Error handling covers ToolError, ValueError, and general exceptions

⚠️ Compatibility

  • File path comparisons must be windows compatible
  • Avoid using emojis and unicode characters in console and log output

Issue: The justfile:8 contains an emoji: @echo "💡 Remember to activate...". While this is in the justfile (not code), CLAUDE.md explicitly states to avoid emojis in console output unless the user explicitly requests it.

Fix emoji in justfile →

📋 Additional Observations

Positive Highlights:

  1. Excellent documentation updates - The fast path workflow is clearly documented in AGENTS.md (lines 47-54)
  2. Smart symlink approach - Converting CLAUDE.md to a symlink pointing to AGENTS.md (lines 1-408) reduces duplication
  3. Comprehensive test contracts - test_tool_contracts.py will catch MCP tool signature drift early
  4. testmon integration - Using pytest-testmon for impacted test detection is a smart optimization
  5. Proper .gitignore entry - .testmondata* added at line 4

Minor Formatting Fixes:

  • test_entity_parser_error_handling.py has proper spacing improvements (no issues)
  • test_search_repository.py has cleaned up imports (removed unused from datetime import timezone, datetime)

🎯 Verdict

This PR adds valuable tooling for the development workflow. The implementation is solid, but needs attention on:

  1. Test coverage for doctor.py (high priority if not covered by CLI omit pattern)
  2. Emoji removal from justfile (low priority, style compliance)

The core functionality is excellent:

  • just fast-check - Smart fast path for local development
  • just testmon - Runs only impacted tests
  • just test-smoke - Quick MCP end-to-end validation
  • just doctor - Self-checking file ↔ DB loop
  • Tool contract tests prevent signature drift

Recommend addressing test coverage verification before merging. The emoji fix is optional but recommended for consistency.


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.

2 participants