Addressing PR comments#140
Conversation
Co-authored-by: sagikimhi <92639180+sagikimhi@users.noreply.github.com>
Co-authored-by: sagikimhi <92639180+sagikimhi@users.noreply.github.com>
Co-authored-by: sagikimhi <92639180+sagikimhi@users.noreply.github.com>
Co-authored-by: sagikimhi <92639180+sagikimhi@users.noreply.github.com>
- Fix test_update_plugin_success: Add exist_ok=True to .git mkdir - Fix test_load_plugin_config_missing: Use temp cache dir to isolate tests - Fix test_plugin_model_script: Update assertion to match script validator behavior All tests now use isolated temporary cache directories to prevent cross-test contamination. Co-authored-by: sagikimhi <92639180+sagikimhi@users.noreply.github.com>
- Support GitLab, Bitbucket, and local paths in addition to GitHub - Export PluginManager and PluginCache in main socx module - Update documentation strings for multi-provider support - Add comprehensive tests for URL normalization Co-authored-by: sagikimhi <92639180+sagikimhi@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds multi-provider support for remote plugins and exports plugin management classes in the public API. The plugin system previously only supported GitHub and did not expose PluginManager or PluginCache for programmatic access. This change enables plugins to be sourced from GitLab, Bitbucket, self-hosted Git providers, and local filesystem paths, while making the plugin infrastructure accessible to external code.
Changes:
- Enhanced URL normalization in
PluginCacheto support multiple Git providers and local paths - Exported
PluginManagerandPluginCachein the mainsocxmodule - Added plugin management CLI commands (add, remove, update, list)
- Extended
PluginModelschema withremoteandreffields - Added comprehensive tests for plugin cache, manager, and model functionality
- Provided detailed documentation with examples
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
src/socx/plugins/cache.py |
New plugin cache management with multi-provider URL normalization |
src/socx/plugins/manager.py |
New plugin manager for Git-based plugin operations |
src/socx/plugins/__init__.py |
Package-level exports for PluginManager and PluginCache |
src/socx/config/schema/plugin.py |
Added remote and ref fields to PluginModel schema |
src/socx/config/converters.py |
Auto-adds remote plugin paths to sys.path during loading |
src/socx/__init__.py |
Exported PluginManager and PluginCache in public API |
src/socx_plugins/plugin/__init__.py |
Added CLI commands for plugin management |
tests/test_plugin_cache.py |
Comprehensive tests for cache operations and URL normalization |
tests/test_plugin_manager.py |
Tests for plugin manager operations |
tests/test_plugin_model.py |
Tests for PluginModel schema extensions |
docs/remote-plugins.md |
Complete documentation for remote plugin feature |
docs/plugin-example.md |
Example plugin structure and usage guide |
Comments suppressed due to low confidence (2)
docs/plugin-example.md:137
- The documentation says "Publish to GitHub" but the PR adds support for multiple Git providers (GitLab, Bitbucket, self-hosted Git). Update this text to be provider-agnostic (e.g., "Publish to your Git provider").
5. **Publish** to GitHub and share with others:
```bash
socx plugin add your-plugin you/your-plugin
**docs/plugin-example.md:106**
* The documentation says "Create a new repository on GitHub" but the PR adds support for GitLab, Bitbucket, self-hosted Git, and local paths. Update this section to mention that plugins can be hosted on various Git providers, not just GitHub.
- Create a new repository on GitHub
</details>
---
💡 <a href="/sagikimhi/socx-cli/new/main?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.
| return str(Path(remote_url).expanduser().resolve()) | ||
|
|
||
| # If it looks like a Windows path (C:\ or similar) | ||
| if len(remote_url) > 2 and remote_url[1] == ":" and remote_url[2] in ("\\/"): |
There was a problem hiding this comment.
The check for Windows paths only handles backslash and forward slash, but the slice indexing could cause an IndexError if remote_url has fewer than 3 characters. Add a length check before accessing remote_url[2], or use a more robust condition like remote_url[2:3] in ("\\/") to safely handle edge cases.
| if len(remote_url) > 2 and remote_url[1] == ":" and remote_url[2] in ("\\/"): | |
| if remote_url[1:2] == ":" and remote_url[2:3] in "\\/": |
| """Tests for plugin manager.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import tempfile | ||
| from pathlib import Path | ||
| from unittest.mock import Mock, patch, MagicMock | ||
|
|
||
| import pytest | ||
| import yaml | ||
|
|
||
| from socx.plugins.manager import PluginManager | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def temp_project_dir(): | ||
| """Create a temporary project directory for testing.""" | ||
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| yield Path(tmpdir) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def temp_cache_dir(): | ||
| """Create a temporary cache directory for testing.""" | ||
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| yield Path(tmpdir) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def mock_manager(temp_project_dir, temp_cache_dir): | ||
| """Create a PluginManager with a temporary project root and cache.""" | ||
| from socx.plugins.cache import PluginCache | ||
| manager = PluginManager(project_root=temp_project_dir) | ||
| # Override the cache to use a temporary directory | ||
| manager.cache = PluginCache(cache_dir=temp_cache_dir) | ||
| return manager | ||
|
|
||
|
|
||
| def test_manager_initialization(temp_project_dir): | ||
| """Test PluginManager initialization.""" | ||
| manager = PluginManager(project_root=temp_project_dir) | ||
| assert manager.project_root == temp_project_dir | ||
| assert manager.config_file == temp_project_dir / ".socx.yaml" | ||
|
|
||
|
|
||
| def test_load_config_empty(mock_manager): | ||
| """Test loading config when file doesn't exist.""" | ||
| config = mock_manager._load_config() | ||
| assert config == {} | ||
|
|
||
|
|
||
| def test_save_and_load_config(mock_manager): | ||
| """Test saving and loading config.""" | ||
| test_config = {"plugins": {"test": {"name": "test", "enabled": True}}} | ||
|
|
||
| mock_manager._save_config(test_config) | ||
| loaded_config = mock_manager._load_config() | ||
|
|
||
| assert loaded_config == test_config | ||
|
|
||
|
|
||
| def test_list_plugins_empty(mock_manager): | ||
| """Test listing plugins when none are configured.""" | ||
| plugins = mock_manager.list_plugins() | ||
| assert plugins == {} | ||
|
|
||
|
|
||
| def test_list_plugins_with_data(mock_manager): | ||
| """Test listing plugins with data.""" | ||
| config = { | ||
| "plugins": { | ||
| "test-plugin": {"name": "test-plugin", "enabled": True}, | ||
| "another-plugin": {"name": "another-plugin", "enabled": False}, | ||
| } | ||
| } | ||
| mock_manager._save_config(config) | ||
|
|
||
| plugins = mock_manager.list_plugins() | ||
| assert len(plugins) == 2 | ||
| assert "test-plugin" in plugins | ||
| assert "another-plugin" in plugins | ||
|
|
||
|
|
||
| @patch("socx.plugins.manager.git.Repo") | ||
| def test_add_plugin_success(mock_git_repo, mock_manager, temp_project_dir): | ||
| """Test successfully adding a remote plugin.""" | ||
| # Mock the git clone operation | ||
| mock_repo_instance = MagicMock() | ||
| mock_git_repo.clone_from.return_value = mock_repo_instance | ||
|
|
||
| # Create a mock plugin config in the cache | ||
| cache_path = mock_manager.cache.get_plugin_path("owner/repo", "main") | ||
| cache_path.mkdir(parents=True, exist_ok=True) | ||
| (cache_path / ".git").mkdir() | ||
|
|
||
| plugin_config_path = cache_path / ".socx.yaml" | ||
| plugin_config = { | ||
| "plugins": { | ||
| "example": { | ||
| "command": "example:cli", | ||
| "short_help": "An example plugin", | ||
| "enabled": True, | ||
| } | ||
| } | ||
| } | ||
| with open(plugin_config_path, "w") as f: | ||
| yaml.safe_dump(plugin_config, f) | ||
|
|
||
| # Add the plugin | ||
| result = mock_manager.add_plugin("my-plugin", "owner/repo", "main") | ||
|
|
||
| assert result["name"] == "my-plugin" | ||
| assert result["remote"] == "owner/repo" | ||
| assert result["ref"] == "main" | ||
|
|
||
| # Verify it was added to config | ||
| plugins = mock_manager.list_plugins() | ||
| assert "my-plugin" in plugins | ||
|
|
||
|
|
||
| def test_add_plugin_already_exists(mock_manager): | ||
| """Test adding a plugin that already exists.""" | ||
| # Add a plugin first | ||
| config = {"plugins": {"test-plugin": {"name": "test-plugin"}}} | ||
| mock_manager._save_config(config) | ||
|
|
||
| # Try to add it again | ||
| with pytest.raises(ValueError, match="already exists"): | ||
| mock_manager.add_plugin("test-plugin", "owner/repo", "main") | ||
|
|
||
|
|
||
| @patch("socx.plugins.manager.git.Repo") | ||
| def test_remove_plugin_success(mock_git_repo, mock_manager): | ||
| """Test successfully removing a plugin.""" | ||
| # Setup: add a plugin first | ||
| config = { | ||
| "plugins": { | ||
| "test-plugin": { | ||
| "name": "test-plugin", | ||
| "remote": "owner/repo", | ||
| "ref": "main", | ||
| } | ||
| } | ||
| } | ||
| mock_manager._save_config(config) | ||
|
|
||
| # Remove the plugin | ||
| mock_manager.remove_plugin("test-plugin") | ||
|
|
||
| # Verify it was removed | ||
| plugins = mock_manager.list_plugins() | ||
| assert "test-plugin" not in plugins | ||
|
|
||
|
|
||
| def test_remove_plugin_not_found(mock_manager): | ||
| """Test removing a plugin that doesn't exist.""" | ||
| with pytest.raises(ValueError, match="not found"): | ||
| mock_manager.remove_plugin("nonexistent") | ||
|
|
||
|
|
||
| @patch("socx.plugins.manager.git.Repo") | ||
| def test_update_plugin_success(mock_git_repo, mock_manager): | ||
| """Test successfully updating a plugin.""" | ||
| # Setup: add a plugin first | ||
| cache_path = mock_manager.cache.get_plugin_path("owner/repo", "main") | ||
| cache_path.mkdir(parents=True, exist_ok=True) | ||
| (cache_path / ".git").mkdir(exist_ok=True) | ||
|
|
||
| plugin_config_path = cache_path / ".socx.yaml" | ||
| plugin_config = { | ||
| "plugins": { | ||
| "example": { | ||
| "command": "example:cli", | ||
| "short_help": "Updated plugin", | ||
| "enabled": True, | ||
| } | ||
| } | ||
| } | ||
| with open(plugin_config_path, "w") as f: | ||
| yaml.safe_dump(plugin_config, f) | ||
|
|
||
| config = { | ||
| "plugins": { | ||
| "test-plugin": { | ||
| "name": "test-plugin", | ||
| "remote": "owner/repo", | ||
| "ref": "main", | ||
| "command": "old:cli", | ||
| } | ||
| } | ||
| } | ||
| mock_manager._save_config(config) | ||
|
|
||
| # Mock the git operations | ||
| mock_repo_instance = MagicMock() | ||
| mock_git_repo.return_value = mock_repo_instance | ||
|
|
||
| # Update the plugin | ||
| result = mock_manager.update_plugin("test-plugin") | ||
|
|
||
| assert result["name"] == "test-plugin" | ||
| assert result["remote"] == "owner/repo" | ||
| assert result["ref"] == "main" | ||
|
|
||
|
|
||
| def test_update_plugin_not_found(mock_manager): | ||
| """Test updating a plugin that doesn't exist.""" | ||
| with pytest.raises(ValueError, match="not found"): | ||
| mock_manager.update_plugin("nonexistent") | ||
|
|
||
|
|
||
| def test_update_plugin_not_remote(mock_manager): | ||
| """Test updating a plugin that is not remote.""" | ||
| # Setup: add a local plugin | ||
| config = { | ||
| "plugins": { | ||
| "local-plugin": {"name": "local-plugin", "command": "local:cli"} | ||
| } | ||
| } | ||
| mock_manager._save_config(config) | ||
|
|
||
| with pytest.raises(ValueError, match="not a remote plugin"): | ||
| mock_manager.update_plugin("local-plugin") | ||
|
|
||
|
|
||
| def test_load_plugin_config_missing(mock_manager): | ||
| """Test loading plugin config when file doesn't exist.""" | ||
| with pytest.raises(ValueError, match="configuration not found"): | ||
| mock_manager._load_plugin_config("owner/repo", "main") | ||
|
|
||
|
|
||
| def test_load_plugin_config_no_plugins(mock_manager): | ||
| """Test loading plugin config with no plugins defined.""" | ||
| cache_path = mock_manager.cache.get_plugin_path("owner/repo", "main") | ||
| cache_path.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| plugin_config_path = cache_path / ".socx.yaml" | ||
| with open(plugin_config_path, "w") as f: | ||
| yaml.safe_dump({}, f) | ||
|
|
||
| with pytest.raises(ValueError, match="must define at least one plugin"): | ||
| mock_manager._load_plugin_config("owner/repo", "main") |
There was a problem hiding this comment.
The tests don't cover adding, updating, or managing plugins from local filesystem paths, despite the PR description and documentation stating this is supported. Add tests for local path scenarios to ensure the feature works correctly, including cases with absolute paths, relative paths, and home directory paths.
| # Normalize URL | ||
| normalized_url = self.cache._normalize_url(remote_url) | ||
|
|
||
| # Create parent directory | ||
| plugin_path.parent.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| # Clone the repository | ||
| repo = git.Repo.clone_from(normalized_url, plugin_path) | ||
|
|
||
| # Checkout the specified ref | ||
| repo.git.checkout(ref) |
There was a problem hiding this comment.
When cloning from a local filesystem path, git.Repo.clone_from will fail because local paths aren't valid Git repository URLs for cloning. The code should check if the normalized_url is a local path and handle it differently (e.g., by copying the directory or treating it as a local plugin without caching). Consider adding explicit handling for local paths or documenting that local paths should reference existing Git repositories.
| normalized = cache._normalize_url("/home/user/my-plugin") | ||
| assert normalized.startswith("/") | ||
| assert "my-plugin" in normalized | ||
|
|
There was a problem hiding this comment.
The test for Windows paths is commented as checking for Windows systems but the condition is inverted - it only runs on Unix-like systems (when os.name != 'nt'). This means Windows path normalization is never tested. Either fix the condition to test Windows paths on Windows systems, or add a separate test that mocks Windows path behavior.
| # Test local absolute path (Windows) | |
| if os.name == 'nt': # Windows systems | |
| normalized = cache._normalize_url(r"C:\Users\user\my-plugin") | |
| # Normalized path should contain the plugin directory name | |
| assert "my-plugin" in normalized |
There was a problem hiding this comment.
Fuck windows - this is a dev tool, not a microsoft simp tool, windows users should either use wsl or go find another tool to use cuz we don't give a damn - either use linux or go home bud
| if "/" in remote_url and not remote_url.startswith(("http://", "https://", "git@")): | ||
| # Check if it's a provider-specific shorthand (e.g., gitlab.com/owner/repo) | ||
| parts = remote_url.split("/") | ||
| if len(parts) >= 2 and "." in parts[0]: | ||
| # It's a domain-based shorthand like gitlab.com/owner/repo | ||
| return f"https://{remote_url}" | ||
| else: | ||
| # It's a GitHub shorthand like owner/repo | ||
| return f"https://github.com/{remote_url}" |
There was a problem hiding this comment.
The URL normalization logic handles git@ SSH URLs in the check on line 127 but doesn't actually normalize them. If a user provides an SSH URL like "git@github.com:owner/repo.git", it will pass through unchanged, which may cause issues later. Consider adding explicit handling for SSH URLs or document that only HTTPS URLs are supported.
| This document describes the remote plugin feature for socx-cli, which allows you to add, manage, and use plugins hosted on GitHub. | ||
|
|
||
| ## Overview | ||
|
|
||
| Remote plugins are GitHub repositories that contain socx plugin configurations and implementations. They are automatically cloned to your local cache and can be managed on a per-project basis, allowing different projects to use different versions of the same plugin. |
There was a problem hiding this comment.
The documentation states "Remote plugins are GitHub repositories" but the implementation supports multiple providers (GitLab, Bitbucket, self-hosted Git, and local paths). Update this text to accurately reflect the multi-provider support.
| This document describes the remote plugin feature for socx-cli, which allows you to add, manage, and use plugins hosted on GitHub. | |
| ## Overview | |
| Remote plugins are GitHub repositories that contain socx plugin configurations and implementations. They are automatically cloned to your local cache and can be managed on a per-project basis, allowing different projects to use different versions of the same plugin. | |
| This document describes the remote plugin feature for socx-cli, which allows you to add, manage, and use plugins hosted in Git repositories (for example, GitHub, GitLab, Bitbucket, self-hosted Git) or from local paths. | |
| ## Overview | |
| Remote plugins are Git repositories (from providers such as GitHub, GitLab, Bitbucket, or self-hosted instances) or local directories that contain socx plugin configurations and implementations. They are automatically cloned or copied to your local cache and can be managed on a per-project basis, allowing different projects to use different versions of the same plugin. |
|
|
||
| **Arguments:** | ||
| - `name`: Local name for the plugin (how you'll invoke it) | ||
| - `remote`: GitHub repository URL or shorthand (e.g., `owner/repo`) |
There was a problem hiding this comment.
The documentation says "remote: GitHub repository URL or shorthand" but the implementation supports multiple providers (GitLab, Bitbucket, self-hosted Git, and local paths). Add examples for other providers to match the PR description's stated functionality.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Sagi Kimhi <sagi.kim5@gmail.com>
sagikimhi
left a comment
There was a problem hiding this comment.
Please review this yourself and don't resubmit before reviewing it yourself.
I stopped looking right after I saw the RCE - a newborn baby could've seen this one easily, obviously, you did not review your own code, please don't waste my time reviewing shit like that again.
Its nice that you can write descriptive pretty comments but if your logic cant back it up, then it's completely useless. Talk less, think more.
There was a problem hiding this comment.
Implementation of _load_config and _add_to_config is incorrect, plugin manager should use the configuration mechanisms under socx.config to load the configuration file with Settings.load_file in order to ensure converters are applied to the plugin configuration.
In addition, configuration must be statically validated before it is actually loaded, are you trying to add a backdoor to my code? This looks more like a "remote code execution made easy tutorial" rather than an actual contribution.
There was a problem hiding this comment.
There are many other additional comments on specific lines that occur in many places in this file, please consider all comments in this file and rewrite this file with all comments in mind to provide a safer robust implementation that doesn't crash the entire app whenever a corrupt plugin is added and handles settings in a safe manner
Critical security fixes: - Add Pydantic validation using PluginModel before loading any plugin configuration - Validate all plugin configurations in _load_config() and _save_config() - Validate remote plugin configurations in _load_plugin_config() - Ensure Path objects are serialized to strings in _save_config() - Fix empty remote check in update_plugin() method Additional fixes: - Fix Windows path indexing issue in cache.py (prevent IndexError) - Use slicing [1:2] and [2:3] instead of direct indexing [1] and [2] All plugin tests pass successfully. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Sagi Kimhi <sagi.kim5@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Sagi Kimhi <sagi.kim5@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Sagi Kimhi <sagi.kim5@gmail.com>
| plugin_path.parent.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| # Clone the repository | ||
| repo = git.Repo.clone_from(normalized_url, plugin_path) |
There was a problem hiding this comment.
Please use pygit2 and definitions from socx.git for robust and performant handling of git related operations instead of using gitpython which is old and no longer being maintained rather than pygit2 which is even recommended in the git book itself
There was a problem hiding this comment.
Replaced gitpython with pygit2 in commit c5643c7. Now using pygit2.clone_repository() for cloning and pygit2.Repository() for repository operations.
| repo = git.Repo.clone_from(normalized_url, plugin_path) | ||
|
|
||
| # Checkout the specified ref | ||
| repo.git.checkout(ref) |
There was a problem hiding this comment.
Please use pygit2 and definitions from socx.git for robust and performant handling of git related operations instead of using gitpython which is old and no longer being maintained rather than pygit2 which is even recommended in the git book itself
There was a problem hiding this comment.
Updated to use pygit2 for all git operations including fetch and checkout in commit c5643c7.
|
|
||
| # Load and validate configuration | ||
| with open(self.config_file, "r") as f: | ||
| config = yaml.safe_load(f) or {} |
There was a problem hiding this comment.
Use either socx.config.settings or any other way that works with dynaconf to load plugin settings so that plugin settings may make use of dynaconf/socx converters in their settings definitions, as well as to support all common formats (json, yaml, toml, ini) rather than only supporting yaml settings
There was a problem hiding this comment.
Implemented dynaconf-based configuration loading in commit c5643c7. Plugin settings now use Dynaconf() which supports all common formats (YAML, JSON, TOML, INI) and enables use of dynaconf converters in plugin configurations.
| ) from e | ||
|
|
||
| # Convert config to YAML format | ||
| with open(self.config_file, "w") as f: |
There was a problem hiding this comment.
Use append for saving configurations and only write keys/values that are missing to the end of the file.
Since users may write these settings manually, add comments, etc. a whole rewrite may change the order of definitions, discard comments, use unicode points instead of emojis or colors making it non-readable to humans, etc.
Also, any values using dynaconf converters will be serialized as the converted value rather than as the definition, which will break most settings that rely on converters for reusability
In order to avoid that, we must append any missing updates or replace specific lines rather than rewriting the entire file by simply dumping the config as is
There was a problem hiding this comment.
Implemented append-based saving with ruamel.yaml in commit c5643c7. The system now:
- Preserves existing file structure, comments, quotes, and formatting
- Only updates changed/new plugin entries
- Removes deleted plugins while keeping others intact
- Avoids serializing converter definitions as converted values
This ensures manual edits, comments, and dynaconf converter syntax are preserved.
| """ | ||
| self.project_root = project_root or PROJECT_ROOT_DIR | ||
| self.config_file = self.project_root / LOCAL_CONFIG_FILENAME | ||
| self.cache = PluginCache() |
There was a problem hiding this comment.
Please use a .socx directory in the current project root directory (or create a new one if missing) for plugins cache rather than USER_CACHE_DIR by default, and only fallback to USER_CACHE_DIR if specifically requested by user (e.g. via some CLI flag) or if no project root was found
There was a problem hiding this comment.
Changed default cache location to .socx/plugins in project root (commit c5643c7). The PluginManager now accepts a use_user_cache parameter to fallback to USER_CACHE_DIR if needed. Default behavior uses project-local cache for better project isolation.
- Replace gitpython with pygit2 for git operations (clone, fetch, checkout) - Use dynaconf for loading plugin configurations to support converters and multiple formats (YAML, JSON, TOML, INI) - Implement append-based config saving with ruamel.yaml to preserve comments and formatting - Change cache directory to use .socx/plugins in project root instead of USER_CACHE_DIR (fallback to user cache if requested) - Convert Path objects to strings before YAML serialization - Filter dynaconf internal keys from loaded configuration - Update tests to mock pygit2 instead of gitpython - Handle plugin removal correctly in append-based saving All plugin tests pass successfully (21/21). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
| if len(parts) >= 2 and "." in parts[0]: | ||
| # It's a domain-based shorthand like gitlab.com/owner/repo | ||
| return f"https://{remote_url}" | ||
| else: |
There was a problem hiding this comment.
Add another elif to handle ssh url normalization
| assert cache.cache_dir.exists() | ||
|
|
||
|
|
||
| def test_normalize_url(): |
There was a problem hiding this comment.
Add additional test cases to test ssh url normalization
| normalized = cache._normalize_url("/home/user/my-plugin") | ||
| assert normalized.startswith("/") | ||
| assert "my-plugin" in normalized | ||
|
|
There was a problem hiding this comment.
Fuck windows - this is a dev tool, not a microsoft simp tool, windows users should either use wsl or go find another tool to use cuz we don't give a damn - either use linux or go home bud
|
|
||
| return config | ||
|
|
||
| def _save_config(self, config: dict[str, Any]) -> None: |
There was a problem hiding this comment.
Instead of merging remote plugin configurations with local project configurations, store plugin configurations separately in the cached plugin directory and load it dynamically on startup, this avoidsany modifications to the user's local configurations, and simplifies debug as bad configurations sources can be traced back to their plugin origin - unlike the current approach which makes it very hard to figure out the origin of a configuration value as it could originate from any remote plugin as well as from the original local settings written by the user, and theres no way to make a distinction to fix a corrupted key/value
| config = self._load_config() | ||
| return config.get("plugins", {}) | ||
|
|
||
| def _clone_plugin(self, remote_url: str, ref: str, plugin_path: Path) -> None: |
There was a problem hiding this comment.
Make sure to also initialize and update any git submodules so that users may utilize git submodules to craft custom plugin groups by creating a repo and adding related plugins as submodules so that when repo is added as remote plugin, all related submodule plugins are added as well
I would suggest moving most of the logic here to dedicated functions defined somewhere in socx.git and then use those to implement the desired logic
| settings_files=[str(config_path)], | ||
| root_path=str(plugin_path), | ||
| lowercase_read=True, # Keep lowercase keys | ||
| load_dotenv=False, # Don't load .env files |
There was a problem hiding this comment.
Do load dotenv files, please use the Settings wrapper class in socx.config._settings to initalize settings with correct options rather than using these definitions raw Dynaconf class from Dynaconf
| ValueError: If plugin already exists or repo cannot be cloned | ||
| """ | ||
| # Check if plugin already exists in local config | ||
| config = self._load_config() |
There was a problem hiding this comment.
Wrap config loading with try except and skip any plugins with invalid plugin settings instead of assuming they are valid.
In the current implementation, any invalid settings will cause the application to crash repeatedly with no way to remove the corrupted plugin via CLI.
Any invalid plugin that is added will require users to figure out where the bad config value is on their own and manually delete the plugin and any of its configurations which is simply terrible
There was a problem hiding this comment.
There are many other additional comments on specific lines that occur in many places in this file, please consider all comments in this file and rewrite this file with all comments in mind to provide a safer robust implementation that doesn't crash the entire app whenever a corrupt plugin is added and handles settings in a safe manner
No description provided.