Skip to content

Conversation

jamesbrink
Copy link
Member

@jamesbrink jamesbrink commented Aug 3, 2025

Summary

This PR significantly improves the discoverability and usability of the NixOS MCP tools by:

  • Renaming tools to remove redundant prefixes (e.g., nixos_searchsearch)
  • Updating descriptions to explicitly mention which shell commands they replace
  • Adding new helper tools: which and help for better user experience
  • Enhancing parameter descriptions with examples and clearer guidance
  • NEW: Implementing issue Feature: tools to search discussions #36 - Added community discussion search tools

Changes

Tool Renames (Breaking Change)

  • nixos_searchsearch
  • nixos_infoshow
  • nixos_channelschannels
  • nixos_statsstats
  • nixos_flakes_statsflakes
  • nixos_flakes_searchflake_search
  • nixhub_package_versionsversions
  • nixhub_find_versionfind_version
  • home_manager_searchhm_search
  • home_manager_infohm_show
  • home_manager_statshm_stats
  • home_manager_list_optionshm_options
  • home_manager_options_by_prefixhm_browse
  • darwin_infodarwin_show
  • darwin_list_optionsdarwin_options
  • darwin_options_by_prefixdarwin_browse

New Tools

  • which: Find which package provides a specific command (replaces command-not-found and nix-locate)
  • help: Quick reference guide showing all available tools and when to use them
  • discourse_search: Search NixOS Discourse forum for community discussions (fixes Feature: tools to search discussions #36)
  • github_search: Search GitHub issues/PRs in NixOS repositories (fixes Feature: tools to search discussions #36)

Community Discussion Search (Fixes #36)

Added two new tools to search for community discussions:

  • discourse_search: Searches the official NixOS Discourse forum for community solutions
  • github_search: Searches GitHub issues, PRs in NixOS repositories
  • Both tools provide formatted plain text output optimized for LLM consumption
  • Added aiohttp dependency for async HTTP requests
  • Comprehensive test coverage with 17 new async tests

Improved Descriptions

Each tool now:

  • Starts with "Replaces 'command'" to make it clear what shell command it replaces
  • Lists specific advantages (10x faster, no setup required, etc.)
  • Provides action-oriented guidance

Test Updates

  • Updated all 334 tests to reflect the renamed tools
  • Added 17 new tests for discussion search functionality
  • All tests passing ✅
  • Code quality checks: formatting ✅, linting ✅, type checking ✅

Breaking Changes

⚠️ This is a breaking change - existing integrations will need to update to use the new tool names.

Test Results

======================== 351 passed in 90.23s (0:01:30) ========================

All formatting, linting, and type checking pass.

Motivation

AI assistants currently default to using shell commands instead of MCP tools. With these changes:

  1. Tool names match the mental model (search instead of nixos_search)
  2. Descriptions explicitly state they replace shell commands
  3. Clear advantages are listed
  4. The help tool provides quick discovery
  5. Community discussion search provides access to real-world solutions

This makes MCP tools the natural first choice for any Nix-related queries.

@jamesbrink
Copy link
Member Author

🎉 All CI checks have passed!

Status Summary:

  • Build: Success
  • Tests (Python 3.11, 3.12, 3.13): All passing
  • Nix tests: Success
  • Claude review: Approved with positive feedback
  • Code quality: All formatting, linting, and type checking pass

Claude's Review Highlights:

  • "High-quality PR that significantly improves the user experience"
  • "Outstanding test maintenance" - all 334 tests updated
  • No security issues identified
  • No performance impact
  • "Well-managed breaking change"

Ready for Review

The PR is ready for maintainer review. The breaking change is well-documented with:

  • Complete mapping of old → new tool names
  • Clear migration path
  • Comprehensive test coverage ensuring no functionality lost

Please note: This will require a major version bump (2.0.0) due to the breaking nature of the tool renames.

@jamesbrink jamesbrink force-pushed the feature/improve-mcp-tool-calling branch from 2cbb274 to 6c4a7f1 Compare August 4, 2025 05:22
@claude
Copy link

claude bot commented Aug 4, 2025

Claude finished @jamesbrink's task —— View job


Code Review for PR #42: Improve MCP tool discoverability and usability

Review Checklist:

  • Analyze main server.py changes for code quality
  • Review new discussion search functionality (discourse_search, github_search)
  • Examine security implications of new HTTP requests
  • Assess test coverage for new features
  • Check performance implications
  • Evaluate breaking changes and migration strategy
  • Review error handling and edge cases
  • Final assessment and recommendations

📋 Code Review Summary

This is a high-quality PR that significantly improves the user experience of MCP-NixOS. The changes are well-implemented with excellent test coverage and thoughtful design decisions.

Strengths

1. Code Quality & Architecture

  • Clean Implementation: The tool renames are consistently applied across the codebase
  • Maintained Async Design: All new functionality properly uses async/await patterns
  • Good Separation of Concerns: Discussion search tools are cleanly isolated from existing functionality
  • Consistent Error Handling: New tools follow the established error() function pattern

2. New Discussion Search Features (mcp_nixos/server.py:2751-2852)

  • Well-Structured Implementation: Both discourse_search and github_search follow consistent patterns
  • Proper Async HTTP: Uses aiohttp.ClientSession correctly with timeouts (10s)
  • Good Parameter Validation: Input sanitization for queries, limits, and search types
  • Fallback Handling: GitHub Discussions gracefully returns helpful message about API limitations

3. Security Considerations

  • Input Sanitization: Query parameters are properly validated before API calls
  • Timeout Protection: 10-second timeouts prevent hanging requests
  • Rate Limit Handling: GitHub API rate limit errors are caught and reported
  • No Credential Exposure: API calls use public endpoints without sensitive data
  • Dependency Safety: aiohttp>=3.9.0 is a well-maintained, secure HTTP client

4. Test Coverage

  • Comprehensive Testing: 17 new tests in test_discussions.py covering all scenarios
  • Proper Mocking: Well-implemented aiohttp mocking for reliable unit tests
  • Edge Case Coverage: Empty queries, rate limits, network errors, and no results
  • Maintained Coverage: All existing 334 tests updated for renamed tools

⚠️ Areas for Improvement

1. Minor Code Issues

Error Message Consistency (mcp_nixos/server.py:2788):

# Current:
return error(f"Discourse API error: {response.status}")

# Suggestion: More descriptive
return error(f"Discourse API returned {response.status}. Try again later or check your query.")

GitHub API Headers (mcp_nixos/server.py:2820):
Consider adding a repository-specific User-Agent for better API compliance:

headers = {
    "Accept": "application/vnd.github.v3+json", 
    "User-Agent": "mcp-nixos/1.0.1 (+https://github.com/utensils/mcp-nixos)"
}

2. Performance Considerations

  • HTTP Client Reuse: Currently creates new aiohttp.ClientSession for each call. Consider connection pooling for high-frequency usage
  • Memory Usage: Acceptable increase (~400KB) due to new aiohttp dependency
  • API Rate Limits: GitHub API (60 req/hour unauthenticated) may be limiting for heavy usage

3. Feature Completeness

  • GitHub Discussions: Currently returns placeholder text. Consider future GraphQL implementation
  • Search Result Caching: Intentionally omitted per CLAUDE.md guidelines (✅ correct decision)

Breaking Changes Assessment

  • Well-Documented: Complete mapping provided in PR description
  • Clear Migration Path: Tool name changes are intuitive (nixos_searchsearch)
  • Version Strategy: Correctly indicates need for major version bump (2.0.0)
  • Backward Compatibility: No functional changes, only naming

Error Handling & Edge Cases

  • Network Failures: Properly caught and handled with user-friendly messages
  • Invalid Parameters: Input validation prevents API errors
  • Empty Results: Helpful suggestions provided when no results found
  • API Rate Limits: GitHub rate limiting properly handled

🎯 Recommendations

Before Merge:

  1. Version Bump: Update pyproject.toml to version 2.0.0 to reflect breaking changes
  2. User-Agent Enhancement: Consider more descriptive GitHub API User-Agent header

Future Enhancements (Not Blocking):

  1. Connection Pooling: Implement shared aiohttp.ClientSession for better performance
  2. GitHub Discussions: Add GraphQL support for complete discussion search
  3. Search Analytics: Consider adding usage metrics for popular search terms

📊 Final Assessment

Aspect Rating Notes
Code Quality ⭐⭐⭐⭐⭐ Clean, consistent, well-structured
Test Coverage ⭐⭐⭐⭐⭐ 17 new tests, all existing tests updated
Security ⭐⭐⭐⭐⭐ Proper input validation, secure HTTP practices
Performance ⭐⭐⭐⭐ Minimal impact, proper timeouts
Documentation ⭐⭐⭐⭐⭐ Excellent tool descriptions and examples
Breaking Changes ⭐⭐⭐⭐⭐ Well-managed with clear migration path

Approval Recommendation

APPROVE - This PR successfully achieves its goals of improving tool discoverability and adding valuable community search functionality. The implementation is solid, well-tested, and follows established patterns. The breaking changes are justified and well-documented.

The new discussion search tools address issue #36 effectively and will significantly improve user experience by providing access to community knowledge and real-world solutions.


@utensils utensils deleted a comment from claude bot Aug 4, 2025
@utensils utensils deleted a comment from claude bot Aug 4, 2025
@utensils utensils deleted a comment from claude bot Aug 4, 2025
@utensils utensils deleted a comment from claude bot Aug 4, 2025
@utensils utensils deleted a comment from claude bot Aug 4, 2025
@utensils utensils deleted a comment from claude bot Aug 4, 2025
@utensils utensils deleted a comment from claude bot Aug 4, 2025
@utensils utensils deleted a comment from claude bot Aug 4, 2025
- Convert all MCP tool names from kebab-case to snake_case for consistency
- Fix darwin_search to prioritize macOS dock settings over Docker results
- Enhance hm_show to display Type, Default, and Example values
- Add context awareness and intelligent suggestions across all tools
- Implement search result grouping for package versions
- Add concise mode parameter to reduce output verbosity
- Improve error messages with did-you-mean suggestions
- Add GitHub API integration for enhanced flake search
- Increase test coverage to 89% with 334 tests
- Rename test files for better clarity and organization
- Update documentation to reflect all changes

This represents a major enhancement to the MCP-NixOS server, improving
search relevance, output quality, and overall user experience.
@jamesbrink jamesbrink force-pushed the feature/improve-mcp-tool-calling branch from 6c4a7f1 to 4d8a1b4 Compare August 14, 2025 15:03
Copy link
Collaborator

@Malix-Labs Malix-Labs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to have a lot of diverse and sometimes unrelated change for a single commit which I have trouble understanding

A Python proficient might want to look into it before merging

I strongly advise against merging that PR as-is

@dhnaranjo
Copy link

Strong agree. I think it would help reviewers quite a lot if this were split into multiple PRs, at least one for the existing tool rename work and one for each of the new features (which, help, discord_ n github_). This will also, for your own benefit @jamesbrink, reduce the risk of a long delay in all elements being merged because of issues with a subsection. Nothings worse than a whole lot of code going stale because of one nagging issue.

@Malix-Labs
Copy link
Collaborator

Malix-Labs commented Oct 12, 2025

I have read more of the source code recently and I concluded that this repository and this PR has probably been almost 100% vibe-coded and is showing Claude 3 levels of overengineering, including flake.nix that the case?

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.

Feature: tools to search discussions

3 participants