-
Notifications
You must be signed in to change notification settings - Fork 106
fix: complete project management special character support (#272) #279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Fixes issue where default_project config setting would not persist due to mismatch between config name (e.g., 'InAMinute') and database permalink (e.g., 'in-a-minute'). Changes: - Use generate_permalink() for all database project lookups - Normalize default_project field during config synchronization - Ensure consistency between config and database project names Fixes #272 Co-authored-by: jope-bm <[email protected]>
Enhances project management to safely handle projects with special characters like spaces, forward slashes, and symbols. Changes: - CLI commands now use generate_permalink() for URL safety - Service layer uses robust get_project() lookup for database operations - ConfigManager.set_default_project() now uses canonical config keys - MCP tools continue using URL encoding for complex special characters - Add comprehensive unit tests for ConfigManager.set_default_project() This ensures project commands (remove, set-default, move, update) work correctly with project names containing special characters while maintaining clean, safe API URLs and robust database lookups. Fixes #272 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Claude <[email protected]> Signed-off-by: Joe P <[email protected]>
Minor cleanup of unused imports and test code formatting: - Remove unused timezone import from models/knowledge.py - Remove unused timezone import from schemas/base.py - Clean up test imports and formatting - Update test assertions and structure 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Claude <[email protected]> Signed-off-by: Joe P <[email protected]>
Signed-off-by: Joe P <[email protected]>
# Then update database | ||
project = await self.repository.get_by_name(name) | ||
# Then update database using the same lookup logic as get_project | ||
project = await self.get_project(name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is was the main fix why the new default project wasn't being set to active
for project in db_projects: | ||
if project.name not in config_projects: | ||
logger.info(f"Adding project '{project.name}' to configuration") | ||
self.config_manager.add_project(project.name, project.path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sync'd projects were having their name set to the permalink
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually this was some tangent claude went off on and I'm not actually convinced it's necessary to fix the issue that was reported. I can back it out and test again.
Cool. it looks like this will work for existing projects that may have Upper case and spaces and all that? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my main concern is making sure we don't break exisitng installs with wonky project names. If not, cool
I can back out the change to the sync code. I don't think it's outside the scope of the ticket |
nah. as long as it doesn't create duplicate projects, that's fine
|
…sues Fixes project synchronization issue where configuration and database had inconsistent project name formats (e.g., "Personal" vs "personal"). Now normalizes project names in config during synchronization to match the permalink format used by the database. - Updates project_service.py to normalize config names during sync - Adds proper test coverage for the normalization behavior - Ensures consistent project name format across config and database 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Joe P <[email protected]>
@phernandez I've just backed out sync changes. It was far beyond the scope of the issue. I was testing with duplicate projects and it does get confusing when using project names with mixed case, camel case, existing hyphens, etc. But i think that's something we want to address in another PR if at all. |
Signed-off-by: Joe P <[email protected]>
Updates test_project_move_command_uses_permalink to use os.path.join() for path construction instead of hardcoded Unix-style paths. This ensures the test works correctly on both Windows and Unix systems by using the appropriate path separator for each platform. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Joe P <[email protected]>
The test was failing on Windows because it expected backslash paths from os.path.abspath() while the actual function uses Path.as_posix() which always produces forward slash paths. Updated the test to use the same normalization as the implementation for consistency across all platforms. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Joe P <[email protected]>
Summary
Completes the fix for GitHub issue #272 by improving project command handling with special characters across CLI, MCP tools, and service layer.
Key Changes
generate_permalink()
for URL safety instead of raw project namesremove_project()
,move_project()
, andupdate_project()
to use robustget_project()
lookupset_default_project()
to use canonical config keys instead of user inputConfigManager.set_default_project()
Architecture Improvements
The fix implements a clean separation of concerns:
Issue Resolution
This fully resolves the original issue where:
Test Coverage
Test Plan
🤖 Generated with Claude Code