Skip to content

Conversation

@phernandez
Copy link
Member

Summary

  • stop forcing entity-only filter in recent_activity prompt and default timeframe to 7d when missing
  • add prompt tests to cover parameter passthrough and timeframe fallback
  • verified prompt test suite via uv run pytest tests/mcp/test_recent_activity_prompt_modes.py tests/mcp/test_prompts.py -q

Testing

  • uv run pytest tests/mcp/test_recent_activity_prompt_modes.py tests/mcp/test_prompts.py -q

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 44s —— View job


PR Review: Fix recent_activity prompt defaults

Summary

This PR successfully fixes two issues in the recent_activity prompt:

  1. ✅ Removes the forced type="entity" filter that prevented other content types from appearing
  2. ✅ Adds a fallback timeframe = timeframe or "7d" to handle None/falsy values
  3. ✅ Adds comprehensive tests to verify the fixes

Code Quality & Standards ✅

Follows Basic Memory conventions:

  • ✅ Python 3.12+ type annotations properly used
  • ✅ Async patterns maintained
  • ✅ Line length under 100 characters
  • ✅ Minimal diff - only changes what's needed
  • ✅ Clean, focused changes without unrelated refactoring
  • ✅ Import order correct (standard lib, third-party, local)

Code style:

  • ✅ Uses Pydantic Field annotations correctly
  • ✅ Proper logging with context
  • ✅ Clean function signature

Testing & Documentation ✅

Test coverage is excellent:

  • ✅ Unit test for discovery mode (test_recent_activity_prompt_discovery_mode)
  • ✅ Unit test for project-specific mode (test_recent_activity_prompt_project_mode)
  • New test for parameter passthrough verification (test_recent_activity_prompt_passes_correct_params)
  • New test for timeframe fallback behavior (test_recent_activity_prompt_defaults_timeframe)
  • ✅ Integration tests in test_prompts.py cover real-world scenarios
  • ✅ Tests verify both that type is NOT passed and that timeframe defaults correctly

Coverage considerations:

  • ✅ All new logic is covered (timeframe fallback + parameter removal)
  • ✅ Tests use monkeypatch to isolate prompt behavior
  • ✅ Should maintain 100% coverage requirement

Documentation:

  • ✅ Docstring remains accurate
  • ✅ Test docstrings clearly explain intent
  • ✅ PR description documents the changes well

Architecture Compliance ✅

MCP best practices:

  • ✅ Prompt correctly delegates to tool (recent_activity.fn())
  • ✅ Maintains separation: prompt adds guidance, tool returns data
  • ✅ Uses textwrap.dedent() for multi-line formatting
  • ✅ Follows atomic, composable design

Correctness of the fix:
The underlying recent_activity tool at src/basic_memory/mcp/tools/recent_activity.py:36 has signature:

async def recent_activity(
    type: Union[str, List[str]] = "",
    depth: int = 1,
    timeframe: TimeFrame = "7d",
    project: Optional[str] = None,
    ...
)
  • ✅ Tool defaults type="" (empty string = all types), so removing the forced type="entity" is correct
  • ✅ Tool defaults timeframe="7d", and prompt now matches this with timeframe or "7d"
  • ✅ Prompt properly passes through user-specified parameters

Security & Performance ✅

  • ✅ No security concerns
  • ✅ No input validation issues (parameters validated by tool layer)
  • ✅ No performance impact (removes unnecessary parameter)
  • ✅ No sensitive data exposure

Compatibility ✅

  • ✅ No file path operations (N/A for Windows compatibility)
  • ✅ No emojis added to code or console output
  • ✅ Changes are backward compatible (behavior is more correct now)

Minor Observations

Line 28 in recent_activity.py:

description="Specific project to get activity from (None for discovery across all projects)"

This line is exactly 100 characters (including indentation). While it meets the spec, consider if it could be slightly shorter for clarity, though this is not a blocker.

Line 40 in recent_activity.py (docstring):

project: Specific project to get activity from (None for discovery across all projects)

Same comment - at the limit but acceptable.

Test quality:
The new test test_recent_activity_prompt_defaults_timeframe is particularly good - it tests a subtle edge case (None vs missing parameter) that could easily be missed.

Overall Assessment

APPROVED ✅

This is a clean, well-tested fix that:

  • Corrects unintended filtering behavior
  • Adds proper fallback handling
  • Includes comprehensive tests
  • Follows all Basic Memory coding standards
  • Maintains 100% test coverage
  • Makes minimal, focused changes

The PR is ready to merge. Excellent work on the test coverage!


Checklist Results

Code Quality & Standards

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

Testing & Documentation

  • ✅ Unit tests for new functions/methods
  • N/A Integration tests for new MCP tools (prompt, not tool)
  • ✅ Test coverage for edge cases
  • 100% test coverage maintained
  • ✅ Documentation updated (docstrings accurate)
  • N/A CLAUDE.md updated (no convention changes)

Basic Memory Architecture

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

Security & Performance

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

Compatibility

  • N/A File path comparisons (no file operations)
  • ✅ Avoid using emojis and unicode in console/logs

@phernandez phernandez changed the title Fix recent_activity prompt defaults fix: recent_activity prompt defaults Feb 1, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2026

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


PR Review: recent_activity prompt defaults

Tasks

  • Read changed files
  • Review against code quality standards
  • Verify testing and documentation
  • Check architecture compliance
  • Validate security and performance
  • Post final review

Summary

This PR successfully fixes two issues in the recent_activity prompt:

  1. ✅ Removes the forced type="entity" filter that prevented other content types from appearing
  2. ✅ Adds a fallback timeframe = timeframe or "7d" to handle None/falsy values
  3. ✅ Adds comprehensive tests to verify both fixes

The changes are minimal, well-tested, and follow all Basic Memory conventions.


Checklist Results

Code Quality & Standards

  • Follows Basic Memory's coding conventions in CLAUDE.md

    • Minimal diff principle upheld (only 2 lines changed in source)
    • Python 3.12+ type annotations properly used
    • Line length under 100 characters
    • Clean, focused changes without unrelated refactoring
    • Proper import order maintained
  • Python 3.12+ type annotations and async patterns

    • Existing type annotations preserved and correct
    • Async pattern maintained (async def)
  • N/A SQLAlchemy 2.0 best practices (no database changes)

  • N/A FastAPI and Typer conventions (MCP prompt only)

  • 100-character line length limit maintained

    • Line 28 is exactly 100 chars (acceptable)
    • Line 45 is 28 chars
    • Line 49 is 79 chars
  • No commented-out code blocks

Testing & Documentation

  • Unit tests for new functions/methods

    • test_recent_activity_prompt_passes_correct_params verifies type is NOT passed
    • test_recent_activity_prompt_defaults_timeframe verifies 7d fallback
  • N/A Integration tests for new MCP tools (this is a prompt, not a tool)

  • Test coverage for edge cases

    • Tests cover timeframe=None case
    • Tests verify type parameter is excluded
    • Existing tests cover discovery and project modes
  • 100% test coverage maintained

    • All new logic is covered:
      • Line 45: timeframe = timeframe or "7d" → covered by test_recent_activity_prompt_defaults_timeframe
      • Line 49: removal of type="entity" → covered by test_recent_activity_prompt_passes_correct_params
  • Documentation updated

    • Docstring remains accurate (describes what the prompt does)
    • Test docstrings clearly explain intent
  • N/A CLAUDE.md updated (no convention changes)

Basic Memory Architecture

  • MCP tools follow atomic, composable design

    • Prompt correctly delegates to tool (recent_activity.fn())
    • Maintains separation: prompt adds guidance, tool returns data
    • Uses textwrap.dedent() for multi-line formatting
  • N/A Database changes include Alembic migrations (no DB changes)

  • N/A Preserves local-first architecture principles (no architecture changes)

  • N/A Knowledge graph operations maintain consistency (prompt layer only)

  • N/A Markdown file handling preserves integrity (no file handling changes)

  • AI-human collaboration patterns followed

    • Clear commit message
    • Signed-off commit
    • Good test coverage

Security & Performance

  • No hardcoded secrets or credentials

  • Input validation for MCP tools

    • Validation delegated to tool layer (correct pattern)
    • Tool at src/basic_memory/mcp/tools/recent_activity.py:36 has proper validation
  • Proper error handling and logging

    • Line 46: logger.info() provides context
  • Performance considerations addressed

    • Removing unnecessary type="entity" parameter actually improves flexibility
    • No performance impact
  • No sensitive data in logs or commits

Compatibility

  • N/A File path comparisons (no file operations)

  • Avoid using emojis and unicode in console and log output

    • No emojis added to console output
    • Emojis only in formatted user-facing text (acceptable per context)

Technical Analysis

Changes Overview

src/basic_memory/mcp/prompts/recent_activity.py

  • Line 45: Added timeframe = timeframe or "7d" fallback
  • Line 49: Removed forced type="entity" parameter

tests/mcp/test_recent_activity_prompt_modes.py

  • Line 91: Updated assertion to verify type is NOT in kwargs
  • Lines 95-109: New test test_recent_activity_prompt_defaults_timeframe

Correctness Verification

The underlying tool at src/basic_memory/mcp/tools/recent_activity.py:36 has:

async def recent_activity(
    type: Union[str, List[str]] = "",
    depth: int = 1,
    timeframe: TimeFrame = "7d",
    project: Optional[str] = None,
    ...
)
  • ✅ Tool defaults type="" (empty string = all types), so removing forced type="entity" is correct
  • ✅ Tool defaults timeframe="7d", and prompt now matches with timeframe or "7d"
  • ✅ Prompt properly passes through user-specified parameters

Test Quality

The new test test_recent_activity_prompt_defaults_timeframe is particularly thorough:

  • Tests the subtle edge case of None vs missing parameter
  • Uses monkeypatch to isolate prompt behavior
  • Captures kwargs to verify exact parameters passed
  • Clear docstring explains intent

Minor Observations

  1. Line 28 (description="Specific project...") is exactly 100 characters including indentation. While it meets the spec, this is at the limit. Not a blocker.

  2. Test naming: The new tests follow the convention test_recent_activity_prompt_* which is consistent with existing tests.

  3. Commit message: "fix: align recent activity prompt defaults" is clear and follows conventional commits format.


Overall Assessment

APPROVED ✅

This is an exemplary PR that:

  • ✅ Fixes real bugs (forced filter, missing fallback)
  • ✅ Makes minimal, surgical changes
  • ✅ Includes comprehensive test coverage
  • ✅ Maintains 100% coverage requirement
  • ✅ Follows all Basic Memory coding standards
  • ✅ Has clear commit message with sign-off
  • ✅ Well-documented in tests and PR description

The PR is ready to merge.


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