Fix ProviderError and git worktree path conflict failures#552
Open
studiozeroseven wants to merge 2 commits intospacedriveapp:mainfrom
Open
Fix ProviderError and git worktree path conflict failures#552studiozeroseven wants to merge 2 commits intospacedriveapp:mainfrom
studiozeroseven wants to merge 2 commits intospacedriveapp:mainfrom
Conversation
Contributor
WalkthroughFormatting and UTF‑8-safe truncation fixes across messaging and agent code, JSON stream deserialization robustness and model-name remapping updates, git worktree removal force flag, and multi-repo worktree naming to include repository name. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/projects/git.rs (1)
251-260:⚠️ Potential issue | 🟡 MinorUpdate the function doc to reflect forced removal.
Line 253 says
git worktree remove <worktree_path>, but Line 260 now runs with--force. Please document that behavior explicitly to avoid accidental misuse.📝 Suggested doc update
-/// Runs `git worktree remove <worktree_path>`. +/// Runs `git worktree remove --force <worktree_path>`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/projects/git.rs` around lines 251 - 260, Update the docstring for the function remove_worktree to explicitly state that it performs a forced removal (it runs `git worktree remove --force <worktree_path>`), so callers know this can discard local changes; adjust the first line and description to mention the use of the --force flag and its implication rather than the current non-forced command text.src/tools/project_manage.rs (1)
643-658:⚠️ Potential issue | 🟠 MajorMulti-repo worktree path can still collide at project root.
Line 657 places worktrees at
root/<repo>-<worktree>. That can still conflict with existing top-level repo directories, and it also contradicts the.worktrees/...behavior described on Line 644.🛠️ Suggested fix (dedicated namespace)
- // For multi-repo projects, place them inside the project root, - // prefixed with `.worktrees/repo_name-worktree_name` to avoid conflicts. + // For multi-repo projects, place them under a dedicated `.worktrees` + // namespace using `repo_name-worktree_name` to avoid root-level collisions. let (worktree_abs_path, worktree_db_path) = if is_single_repo { let parent = root.parent().ok_or_else(|| { ProjectManageError("single-repo project root has no parent directory".into()) })?; ( parent.join(&worktree_dir_name), format!("../{worktree_dir_name}"), ) } else { - // Include repo name in the worktree directory name to avoid conflicts - // with other repos or their worktrees in a multi-repo project. + // Include repo name and store under `.worktrees/` for strong namespacing. let dir_name = format!("{}-{}", repo.name, worktree_dir_name); - (root.join(&dir_name), dir_name) + let db_path = format!(".worktrees/{dir_name}"); + (root.join(&db_path), db_path) };// Before git::create_worktree(...) call: if !is_single_repo { tokio::fs::create_dir_all(root.join(".worktrees")) .await .map_err(|error| { ProjectManageError(format!("failed to create .worktrees directory: {error}")) })?; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/project_manage.rs` around lines 643 - 658, The current multi-repo branch still places worktrees directly under root as root/<repo>-<worktree>, which can collide with top-level repo dirs and contradicts the documented `.worktrees/...` namespace; change the multi-repo branch so it creates and uses a dedicated `.worktrees` directory under root (e.g., root.join(".worktrees").join(format!("{}-{}", repo.name, worktree_dir_name))) and ensure you create the `.worktrees` directory before use (propagate errors as ProjectManageError), updating the variables used later (worktree_abs_path and worktree_db_path) so they reference the `.worktrees` namespaced path instead of root/<repo>-<worktree>; keep single-repo behavior unchanged and ensure the created path is passed to git::create_worktree call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/projects/git.rs`:
- Around line 251-260: Update the docstring for the function remove_worktree to
explicitly state that it performs a forced removal (it runs `git worktree remove
--force <worktree_path>`), so callers know this can discard local changes;
adjust the first line and description to mention the use of the --force flag and
its implication rather than the current non-forced command text.
In `@src/tools/project_manage.rs`:
- Around line 643-658: The current multi-repo branch still places worktrees
directly under root as root/<repo>-<worktree>, which can collide with top-level
repo dirs and contradicts the documented `.worktrees/...` namespace; change the
multi-repo branch so it creates and uses a dedicated `.worktrees` directory
under root (e.g., root.join(".worktrees").join(format!("{}-{}", repo.name,
worktree_dir_name))) and ensure you create the `.worktrees` directory before use
(propagate errors as ProjectManageError), updating the variables used later
(worktree_abs_path and worktree_db_path) so they reference the `.worktrees`
namespaced path instead of root/<repo>-<worktree>; keep single-repo behavior
unchanged and ensure the created path is passed to git::create_worktree call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d8460a3-4ef2-434e-a60c-775b57c84085
📒 Files selected for processing (5)
src/agent/channel.rssrc/config/load.rssrc/llm/model.rssrc/projects/git.rssrc/tools/project_manage.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/messaging/target.rs`:
- Around line 669-672: The validation now rejects Slack-style IDs starting with
'T', 'C', or 'E' in the code path that checks name length and leading character
(see the check in src/messaging/target.rs that uses
name.starts_with('T'|'C'|'E')), so update the API validation message in
src/api/messaging.rs to mention T/C/E (not just T/C) and add a new assertion in
the is_valid_instance_name_invalid_slack_workspace test in
src/messaging/target.rs to cover an 'E...' ID case (i.e., add an invalid
instance name beginning with 'E' that matches the same length/char pattern) so
tests and user-facing guidance stay consistent with the implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a5f259fa-17d2-41a5-bdee-c5258c781d37
📒 Files selected for processing (4)
src/agent/channel.rssrc/llm/model.rssrc/messaging/signal.rssrc/messaging/target.rs
✅ Files skipped from review due to trivial changes (1)
- src/messaging/signal.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/llm/model.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes multiple stability issues within the Spacebot orchestration layer:
TrailingCharactersif LLMs hallucinated extra noise outside of the JSON tool call block. The fix leverages a JSON stream chunk.next()extractor which naturally isolates the first valid JSON blob and purposefully ignores arbitrary trailing text.project_managetool previously caused multi-repo environments to conflict when branches shared names with tracked repositories. We now safely place multirepo worktrees underneath a merged-namespace. As an extra layer of robust environment tear-downs,--forceis added to git worktree removal.&conclusion[..200]with&conclusion[..conclusion.floor_char_boundary(200)]inchannel.rsandsignal.rs. This prevents panics caused by multi-byte emojis or non-ASCII characters on boundaries.is_valid_instance_name(target.rs) to includeEprefixes, protecting large Enterprise setups from misrouting errors by preventing them from being misclassified as a user-configured named instance.litellm/from forwarded completion requests inremap_model_name_for_api(model.rs) uniformly.Tested locally, resulting in clean passing tests on CI. Let me know if there are any questions.