Skip to content

Conversation

@streed
Copy link
Owner

@streed streed commented Sep 17, 2025

Summary

Changes in this pull request:

d99754f fix: improve MCP server error handling and resource validation
59b87ee feat: add dual binary distribution and enhance MCP server capabilities
26803b8 Merge pull request #9 from streed/lets-convert-to-wails
58bcd79 refactor: remove unused web assets and improve error logging
d0f60ec fix: improve error handling and code quality throughout codebase
91e059c Merge branch 'main' into lets-convert-to-wails
8b98139 feat: migrate CLI app to hybrid desktop/CLI using Wails framework
cc7a234 Merge pull request #8 from streed/copilot/fix-1391f1db-d7fa-4f9e-889d-1aaf714fb26e
eb4347d Rename import command to import-url and add documentation
181def2 Preserve original image URLs in imported website content
a25587c Implement secure Chrome configuration for website import
7104a1d Add tests and finalize website import feature
629fcb1 Implement website import functionality with headless browser
a8c4052 Initial plan
3d4cbce Merge pull request #7 from streed/copilot/fix-700df305-6a98-4aeb-bb43-47029478bb9b
0c6eb90 Use lil-rag repository's install.sh script for installation
4848e91 Fix install command syntax for lil-rag installation
9d60a84 Improve error handling for first-install dependencies to be non-fatal
88b911c Update install.sh
4f075be Add first-install detection and dependency installation to install script
d89875d Initial plan
13c5359 Merge pull request #6 from streed/copilot/fix-74f1c7aa-adf3-4f2d-a0b5-b55b3b99b121
0cbea41 Implement complete settings UI configuration support
3395839 Initial plan
2aaff96 Merge pull request #5 from streed/add-namespace-and-use-lil-rag
8ebe9b0 Lint fixes
6516825 A bunch of fixes and improvements
d4de49c Merge branch 'main' into add-namespace-and-use-lil-rag
5af45df Some refactors and changes
866d607 Merge pull request #4 from streed/streed-patch-1
db83b5c Update LICENSE
b0f19d9 Merge branch 'main' into streed-patch-1
101d051 Merge pull request #3 from streed/add-namespace-and-use-lil-rag
25a3919 Update LICENSE
4eeb730 Change license from MIT to GPLv3
53a63b0 Test and lint fixes
c14550c Only build binaries when merged into main
b16e1e6 Use lil-rag for indexing and semantic search also add the idea of a namespace to improve data seperation

🤖 Generated with Claude Code

This change modernizes the ML Notes project by introducing dual binary distribution and significantly enhancing the MCP (Model Context Protocol) server capabilities. The project now separates CLI functionality from desktop application builds, providing both a command-line interface () and a desktop application (ml-notes is a command-line interface for creating, managing, and searching notes using vector embeddings for semantic search.

First time users should run 'ml-notes init' to set up the configuration.

Usage:
  ml-notes [command]

Available Commands:
  add               Add a new note
  analyze           Analyze notes with AI
  auto-tag          Automatically generate tags for notes using AI
  completion        Generate the autocompletion script for the specified shell
  config            Manage ml-notes configuration
  delete            Delete one or more notes
  detect-dimensions Detect the actual embedding dimensions from the model
  edit              Edit an existing note
  get               Get a note by ID
  help              Help about any command
  init              Initialize ml-notes configuration
  list              List all notes
  mcp               Start MCP server for LLM integration
  reindex           Reindex all notes with current vector configuration
  search            Search notes
  serve             Start HTTP API server
  tags              Manage note tags
  update            Update ml-notes to the latest version

Flags:
      --debug     Enable debug logging
  -h, --help      help for ml-notes
  -v, --version   version for ml-notes

Use "ml-notes [command] --help" for more information about a command.) to serve different user workflows. The GitHub Actions workflows have been completely restructured to build both binary types in parallel, with the CLI binary targeting automation and terminal usage while the desktop application provides a modern GUI experience. The MCP server has been upgraded to version 1.1.0 with enhanced search capabilities including auto/vector/text/tag modes, flexible output formatting, comprehensive resource endpoints, and improved error handling and validation throughout.

🤖 Generated with Claude Code

Co-Authored-By: Claude <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 17, 2025 10:40
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request updates installer scripts to support dual binary distribution, introducing both CLI and desktop application variants. The changes enhance the MCP server capabilities and improve the build/release infrastructure to support both binaries.

Key changes:

  • Enhanced MCP server with improved search capabilities, new resources, and better error handling
  • Split build process to generate separate CLI (ml-notes-cli) and desktop (ml-notes) binaries
  • Updated documentation to reflect dual binary distribution and enhanced MCP features

Reviewed Changes

Copilot reviewed 8 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
internal/mcp/server.go Enhanced MCP server with new search modes, resources, and capabilities
internal/cli/commands.go Added MCP command to CLI with comprehensive documentation
go.mod Updated mcp-go dependency and reorganized imports
cmd/mcp.go Updated MCP command documentation for CLI binary usage
README.md Added comprehensive dual binary documentation and enhanced MCP section
.github/workflows/release.yml Split build process for CLI and desktop binaries with separate packaging
.github/workflows/manual-release.yml Updated manual release workflow for dual binary support
.github/workflows/ci.yml Removed desktop build from CI (moved to release workflows)

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 704 to 711
id, err := fmt.Sscanf(idStr, "%d", new(int))
if err != nil || id == 0 {
return nil, fmt.Errorf("invalid note ID: %s", idStr)
}

// Convert to actual int
var noteID int
fmt.Sscanf(idStr, "%d", &noteID)
Copy link

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

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

The ID parsing logic is flawed. Line 704 scans into a temporary new(int) and checks the return value, but then line 711 scans again into noteID without error handling. This could cause the function to proceed with an invalid ID if the second scan fails. Use a single scan operation: noteID, err := strconv.Atoi(idStr)

Copilot uses AI. Check for mistakes.
Comment on lines +823 to +824
s.getCurrentProjectNamespace(),
s.getCurrentProjectNamespace(),
Copy link

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

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

Duplicate call to getCurrentProjectNamespace() on consecutive lines. The second call should likely be removed or replaced with a different value since both lines appear to be setting the same field.

Copilot uses AI. Check for mistakes.
noteCount,
s.getCurrentProjectNamespace(),
s.getCurrentProjectNamespace(),
fmt.Sprintf("%v", context.Background().Value("timestamp")))
Copy link

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

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

Using context.Background().Value(\"timestamp\") will always return nil since a background context has no values. This will result in <nil> being displayed. Consider using time.Now().Format(time.RFC3339) or a similar approach to get the current timestamp.

Copilot uses AI. Check for mistakes.
-o dist/ml-notes-${{ matrix.name }}${{ matrix.ext }} .
- name: Upload artifact
-o dist/ml-notes-cli-${{ matrix.name }}${{ matrix.ext }} ./app/cli
Copy link

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

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

The build path ./app/cli doesn't match the typical Go project structure. Based on the repository context, this should likely be ./cmd/cli or just . (for the main package).

Suggested change
-o dist/ml-notes-cli-${{ matrix.name }}${{ matrix.ext }} ./app/cli
-o dist/ml-notes-cli-${{ matrix.name }}${{ matrix.ext }} ./cmd/cli

Copilot uses AI. Check for mistakes.
-o dist/ml-notes-${{ matrix.name }}${{ matrix.ext }} .
- name: Upload artifact
-o dist/ml-notes-cli-${{ matrix.name }}${{ matrix.ext }} ./app/cli
Copy link

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

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

Same issue as in release.yml - the build path ./app/cli appears incorrect for the Go project structure. This should match the actual CLI application entry point.

Suggested change
-o dist/ml-notes-cli-${{ matrix.name }}${{ matrix.ext }} ./app/cli
-o dist/ml-notes-cli-${{ matrix.name }}${{ matrix.ext }} ./cmd/cli

Copilot uses AI. Check for mistakes.
This change improves error handling and code quality in the MCP server's resource handling methods. The note ID parsing logic was refactored to properly validate the scanf operation by checking both the error and the number of parsed values, eliminating redundant parsing calls and ensuring robust input validation. Additionally, error handling was added to the database query in the health endpoint to gracefully handle potential SQL failures by defaulting to zero notes and logging the error rather than silently ignoring query failures. These modifications enhance the server's reliability and provide better debugging information when database operations encounter issues.

🤖 Generated with Claude Code

Co-Authored-By: Claude <noreply@anthropic.com>
@streed streed merged commit 5acd9cc into main Sep 17, 2025
3 checks passed
@streed streed deleted the update-installer-scripts branch September 17, 2025 10:48
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