Skip to content

Conversation

@groksrc
Copy link
Member

@groksrc groksrc commented Jan 24, 2026

Summary

  • Fix MCP prompt "Error rendering prompt recent_activity" error
  • Change TimeFrame to str in prompt type annotations for FastMCP JSON schema compatibility
  • Fix recent_activity_prompt architecture - it was checking isinstance(result, ProjectActivitySummary) on a string

Problem

The recent_activity MCP prompt was failing with "Error rendering prompt recent_activity" because:

  1. Type annotation issue: TimeFrame = Annotated[str, BeforeValidator(...)] could cause issues with FastMCP's JSON schema generation for prompt parameters

  2. Architecture bug: The prompt called recent_activity.fn() which returns a formatted string, but then checked isinstance(recent, ProjectActivitySummary) which was always False

Solution

  1. Changed all prompt timeframe parameters from TimeFrame to str
  2. Rewrote recent_activity_prompt to use the tool's string output directly and add helpful guidance
  3. Updated PromptContext dataclass to use str instead of TimeFrame
  4. Pass type="entity" as string instead of enum [SearchItemType.ENTITY]

Test plan

  • All existing prompt tests pass
  • New test verifies correct parameters are passed to tool
  • Manual test: Call /basic-memory-cloud:recent_activity (MCP) in Claude Code

🤖 Generated with Claude Code

- Change TimeFrame to str in prompt signatures to ensure FastMCP can
  properly generate JSON schemas (affects continue_conversation, search,
  and recent_activity prompts)

- Fix recent_activity_prompt architecture: the prompt was calling
  recent_activity.fn() which returns a formatted string, but then
  checking isinstance(result, ProjectActivitySummary) which always
  failed. Now the prompt uses the tool's string output directly and
  adds helpful guidance for next steps.

- Update PromptContext dataclass to use str instead of TimeFrame

- Pass type parameter as string "entity" instead of enum to match
  the tool's expected input format

- Update tests to match new prompt output format

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@github-actions
Copy link
Contributor

github-actions bot commented Jan 24, 2026

Claude finished @groksrc's task in 2m 12s —— View job


PR Review: MCP Prompt Type Annotations Fix

I've completed a comprehensive review of this PR against the Basic Memory team checklist. Here's my detailed analysis:


✅ Code Quality & Standards

Follows Basic Memory coding conventions:

  • Python 3.12+ type annotations properly used
  • 100-character line length maintained
  • Proper import order (stdlib, third-party, local)
  • Async patterns followed
  • Uses textwrap.dedent() for multi-line strings (recent_activity.py:58)

Type Annotations:

  • Correct change from TimeFrame (Annotated type with validator) to str for FastMCP compatibility
  • The TimeFrame type uses BeforeValidator(validate_timeframe) which can cause issues with JSON schema generation in FastMCP
  • All function signatures properly annotated

No commented-out code:


✅ Testing & Documentation

Test Coverage: ✅ EXCELLENT

  • All existing tests updated to match new output format
  • New test added: test_recent_activity_prompt_passes_correct_params verifies correct parameter passing
  • Tests use monkeypatching appropriately to isolate prompt logic from tool implementation
  • Both discovery mode and project-specific mode tested
  • Edge cases covered (custom timeframe, parameter validation)

Test Quality:

  • Tests verify both tool output inclusion AND prompt guidance addition
  • Clear test names and docstrings
  • Proper use of pytest fixtures and marks

Coverage Maintenance:

  • No # pragma: no cover added (good - none needed)
  • All code paths appear testable and tested

Documentation:

  • Docstrings updated in recent_activity.py
  • PR description clearly explains the problem and solution
  • Code comments explain the architectural change (lines 47-48 in recent_activity.py)

✅ Basic Memory Architecture

MCP Prompt Design: ✅ SIGNIFICANT IMPROVEMENT

  • Key Fix: The prompt was incorrectly treating the tool's string output as a ProjectActivitySummary object
  • Old code tried to check isinstance(result, ProjectActivitySummary) on what was already a formatted string
  • New architecture correctly recognizes that recent_activity.fn() returns a formatted string, not a data object
  • Prompt now acts as a proper wrapper: takes tool output + adds guidance for LLM

Atomic, Composable Operations:

  • Prompt properly delegates to the recent_activity tool
  • Clean separation: tool formats data, prompt adds context/guidance
  • Follows the pattern: MCP Prompt → MCP Tool → API → Service → Repository

Type Parameter Handling:

  • Correctly passes type="entity" as string instead of [SearchItemType.ENTITY] enum
  • Matches the tool's signature: Union[str, List[str]]

✅ Security & Performance

No Security Issues:

  • No hardcoded credentials or secrets
  • Input validation handled by Pydantic and tool layer
  • No unsafe operations

Performance:

  • Reduced code complexity from 168 lines → 101 lines (40% reduction)
  • Eliminated unnecessary data processing (was extracting/limiting results already formatted by tool)
  • Simpler code = faster execution

✅ Compatibility

Windows Compatibility: ✅ N/A (no file path operations in changes)

Console Output:

  • No emojis added in the prompt code itself
  • The tool may return emojis (that's fine for LLM consumption)

📋 Review Checklist Results

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 - N/A
  • FastAPI and Typer conventions followed - N/A
  • 100-character line length limit maintained
  • No commented-out code blocks

Testing & Documentation

  • Unit tests for new functions/methods
  • Integration tests for new MCP tools - N/A (prompt change)
  • Test coverage for edge cases
  • 100% test coverage maintained
  • Documentation updated (README, docstrings)
  • CLAUDE.md updated if conventions change - N/A

Basic Memory Architecture

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

Security & Performance

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

Compatibility

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

🎯 Summary

This is a high-quality fix that addresses two important issues:

  1. Type Annotation Issue: The TimeFrame annotated type with BeforeValidator was incompatible with FastMCP's JSON schema generation. Changing to plain str resolves this.

  2. Architecture Bug: The prompt was treating a formatted string as a data object. The fix correctly recognizes the tool returns a string and the prompt's job is to wrap it with guidance.

Key Improvements:

  • ✅ 40% code reduction (228 deletions, 119 additions)
  • ✅ Simpler, more maintainable architecture
  • ✅ Better separation of concerns (tool formats, prompt adds guidance)
  • ✅ All tests passing with proper coverage
  • ✅ Consistent with other prompts (continue_conversation, search)

Outstanding Item:

  • Manual test: Call /basic-memory-cloud:recent_activity (MCP) in Claude Code (noted in PR description)

Recommendation:APPROVE - This PR is ready to merge pending the manual test validation.


@groksrc groksrc requested a review from phernandez January 24, 2026 23:51
@groksrc groksrc merged commit e14ba92 into main Jan 26, 2026
38 of 39 checks passed
@groksrc groksrc deleted the fix/mcp-prompt-type-annotations branch January 26, 2026 01:14
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.

3 participants