Repository navigation
feat(wework): add git workspace project creation - #1297
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds Git-backed workspace project creation end-to-end: backend endpoint and service orchestrate device-side Git repository cloning with configurable timeout and directory handling; frontend APIs and workbench provider expose Git discovery and creation callbacks; new UI dialog mode handles repository/branch selection with async loading; and shared icon classification enables visual distinction for Git projects across the app. ChangesGit Workspace Project Creation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wework/src/components/layout/MobileWorkbenchLayout.tsx (1)
88-110:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForward the new Git callbacks to
MobileDrawer(currently dropped).The props were added to
MobileWorkbenchLayoutProps, but they are neither destructured nor passed down. This breaks the new Git project creation path on mobile.🔧 Suggested wiring fix
export function MobileWorkbenchLayout({ @@ onOpenTask, + onCreateGitWorkspaceProject, + onListGitRepositories, + onListGitBranches, onInputChange, @@ }: MobileWorkbenchLayoutProps) { @@ <MobileDrawer @@ onSelectProject={onSelectProject} onOpenTask={onOpenTask} + onCreateGitWorkspaceProject={onCreateGitWorkspaceProject} + onListGitRepositories={onListGitRepositories} + onListGitBranches={onListGitBranches} />Also applies to: 325-343
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/components/layout/MobileWorkbenchLayout.tsx` around lines 88 - 110, MobileWorkbenchLayout currently doesn't forward the new Git-related callbacks declared on MobileWorkbenchLayoutProps down into the child MobileDrawer; update the MobileWorkbenchLayout function to destructure the missing Git callbacks from its props and pass them through to the MobileDrawer component wherever MobileDrawer is rendered so the mobile Git project creation flow works; reference the prop names as declared on MobileWorkbenchLayoutProps and ensure MobileDrawer receives the same prop identifiers (i.e., forward each new callback prop from MobileWorkbenchLayout to MobileDrawer).
🧹 Nitpick comments (5)
wework/src/components/projects/ProjectFolderIcon.test.tsx (1)
6-24: ⚡ Quick winAdd a non-Git control test to prevent false positives.
This suite only validates the Git happy path. Add a local-path workspace case to assert the Git icon is not rendered.
🧪 Suggested test addition
describe('ProjectFolderIcon', () => { test('marks Git workspace projects with a Git folder icon', () => { @@ expect(screen.getByTestId('project-git-folder-icon-7')).toBeInTheDocument() }) + + test('does not render the Git icon for non-Git workspace projects', () => { + const project: ProjectWithTasks = { + id: 8, + name: 'LocalProject', + tasks: [], + config: { + mode: 'workspace', + workspace: { + source: 'local_path', + localPath: '/workspace/local', + }, + }, + } + + render(<ProjectFolderIcon project={project} className="h-4 w-4" />) + + expect(screen.queryByTestId('project-git-folder-icon-8')).not.toBeInTheDocument() + }) })Based on learnings, "Test edge cases and error conditions in test files".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/components/projects/ProjectFolderIcon.test.tsx` around lines 6 - 24, Add a negative test to ProjectFolderIcon.test.tsx that ensures a non-Git workspace does not render the Git icon: create a ProjectWithTasks object similar to the existing one but with config.mode === 'workspace' and workspace.source === 'local-path' (or another non-'git' value), render <ProjectFolderIcon project={...} className="h-4 w-4" /> and assert the Git icon test id used in the component (e.g., 'project-git-folder-icon-<id>') is not present using screen.queryByTestId(...)/toBeNull() or expect(...).not.toBeInTheDocument(); name the test something like "does not mark local-path workspace projects with a Git folder icon".wework/src/components/projects/ProjectCreateDialog.test.tsx (1)
268-368: ⚡ Quick winAdd one Git failure-path test for the new error normalization logic.
The new tests only cover success/loading. Add a rejection case (e.g., token missing or directory exists) to lock in the new error UX and prevent regressions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/components/projects/ProjectCreateDialog.test.tsx` around lines 268 - 368, Add a new test (e.g., "shows normalized error on Git workspace creation failure") that mounts ProjectCreateDialog (mode="git") and mocks onCreateGitWorkspaceProject to reject with an error matching the new normalization paths (for example reject(new Error('token missing')) or a custom rejection for "directory exists"); select the repository and branch using the existing test ids ('git-repository-select', 'git-branch-select'), click the 'create-project-button', then await and assert that the dialog does not call onClose, the submit spinner/progress stop and an error message element displays the normalized error text (use the same visible selectors used for progress/errors in other tests), and that the cancel/action buttons return to an enabled state to allow retry.wework/src/components/projects/ProjectCreateDialog.tsx (1)
880-958: ⚡ Quick winAlign dialog action buttons with the project button contract.
The primary dialog action is still a raw styled button. Please switch it to the standardized dialog primary button contract (
variant="primary") and keep touch-target sizing ath-11 min-w-[44px]for mobile reachability.As per coding guidelines, "Primary action buttons in dialogs MUST use
variant=\"primary\"" and "All interactive elements on mobile must be at least 44px × 44px - useh-11 min-w-[44px]for buttons".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wework/src/components/projects/ProjectCreateDialog.tsx` around lines 880 - 958, The primary action is a raw styled <button> (data-testid="create-project-button") inside ProjectCreateDialog; replace it with the standardized dialog primary button component by giving it variant="primary" (or using the shared DialogPrimaryButton) while preserving props (onClick handler, disabled, aria-busy, data-testid) and children (Loader2 spinner and labels), and update sizing classes to enforce mobile touch targets (use h-11 min-w-[44px] instead of the current h-10 and custom bg/hover classes); remove the hard-coded bg/hover styles that conflict with the shared variant so the unified primary styling is used.shared/utils/git_util.py (1)
16-30: ⚡ Quick winConsider adding edge-case tests for
get_safe_git_workspace_key.The current tests validate the happy path (explicit repo and URL-derived key), but several edge cases are untested:
- Empty or whitespace-only
repoparameter- Repository names containing only special characters (e.g.,
"...","___")- Very long repository names
- URLs with multiple consecutive slashes or special characters
- Repository names that become empty after sanitization
Adding these tests will improve confidence in the sanitization logic.
🧪 Example edge-case tests
def test_safe_git_workspace_key_sanitizes_special_characters(): key = get_safe_git_workspace_key("https://github.com/org/my@repo#name.git") assert key == "my_repo_name" def test_safe_git_workspace_key_handles_empty_after_sanitization(): key = get_safe_git_workspace_key("https://github.com/org/....git") assert key == "git_repo" # fallback def test_safe_git_workspace_key_strips_edge_special_chars(): key = get_safe_git_workspace_key("https://github.com/org/_myrepo_.git") assert key == "myrepo"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shared/utils/git_util.py` around lines 16 - 30, Add unit tests for get_safe_git_workspace_key to cover edge cases: pass empty and whitespace-only repo arguments and assert it falls back to URL-derived key or the default ("git_repo") when appropriate; test repo names made only of special characters (e.g., "....", "___", "...") and assert they sanitize to the fallback "git_repo"; test repo names with leading/trailing underscores or dots (e.g., "_myrepo_", "my@repo#name.git") and assert they are cleaned to alphanumeric/underscore form ("myrepo" or "my_repo_name"); test very long repository names to ensure truncation or length-limiting behavior is enforced consistently; and test URLs with multiple consecutive slashes or unusual characters to confirm the function extracts and sanitizes the repo segment correctly. Ensure each test references get_safe_git_workspace_key and asserts the exact expected sanitized key or fallback.backend/app/services/project_service.py (1)
197-197: ⚡ Quick winExtract inline timeout/output limits to named constants.
The new checkout flow introduces hard-coded numeric limits that should be promoted to constants for maintainability.
Suggested refactor
GIT_CLONE_TIMEOUT_SECONDS = 600 +PATH_EXISTS_TIMEOUT_SECONDS = 30 +GIT_CLONE_MAX_OUTPUT_BYTES = 5 * 1024 * 1024 ... - timeout_seconds=30, + timeout_seconds=PATH_EXISTS_TIMEOUT_SECONDS, ... - max_output_bytes=5 * 1024 * 1024, + max_output_bytes=GIT_CLONE_MAX_OUTPUT_BYTES,As per coding guidelines:
Extract magic numbers to named constants in Python code.Also applies to: 223-223
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/services/project_service.py` at line 197, Extract the hard-coded numeric limits into module-level named constants: replace the inline timeout_seconds=30 with a constant like DEFAULT_CHECKOUT_TIMEOUT_SECONDS = 30 and replace the other magic number at the second occurrence (reported at line 223) with a constant like DEFAULT_CHECKOUT_OUTPUT_LIMIT; declare these constants at the top of project_service.py, update the call sites (where timeout_seconds is passed and where the other numeric literal is used) to reference the new constants, and ensure any tests or callers that rely on the previous literals continue to work with the new constant names.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/api/endpoints/projects.py`:
- Around line 98-103: Add OpenTelemetry tracing to the new async endpoint by
decorating create_git_workspace_project_endpoint with `@trace_async` (importing
trace_async from the tracing utilities used in the repo) and instrumenting key
points inside the function with span metadata: call
add_span_event("create_git_workspace_project.started") at start,
set_span_attribute("user.id", current_user.id) and
set_span_attribute("project.type","git_workspace") after validation, and
add_span_event("git.clone.started", {"repo": project_create.repo_url}) before
cloning and add_span_event("git.clone.completed") (or an error event on failure)
after cloning; use the existing add_span_event/set_span_attribute helpers (or
their equivalents) to ensure request-level observability.
In `@backend/app/services/project_service.py`:
- Around line 92-97: The new async service entry point
create_git_workspace_project is missing OpenTelemetry instrumentation: add the
`@trace_async` decorator above the async def and inside the function set relevant
span attributes/events (e.g., call set_span_attribute for project_data fields
like repository/url/branch and add_span_event for key lifecycle steps such as
"clone_started" / "clone_succeeded" / "clone_failed"); apply the same pattern to
the other async functions referenced around lines 156-166 so each async entry
point uses `@trace_async` and emits span attributes/events to surface important
parameters and status.
- Around line 190-200: The current logic treats any non-zero result from
execute_configured_device_command(..., command_key="path_exists") as “not
exists” and proceeds, which masks execution failures; update the check around
exists_result returned by execute_configured_device_command (used with
command_key "path_exists") to distinguish an execution error from a genuine “not
found” result: inspect the result's execution status/exit code and error/message
fields from exists_result, if the command failed (non-standard error code or an
error message), raise or propagate an execution error (include the command error
details), and only call _target_path_exists_error when the result specifically
indicates the target path exists per _command_succeeded semantics. Ensure
references to execute_configured_device_command, _command_succeeded, and
_target_path_exists_error are used to locate and update the logic.
In `@executor/agents/base.py`:
- Around line 424-440: Add a clear docstring to the _resolve_workspace_path
method describing its behavior: explain that it expands ~ via
os.path.expanduser, returns absolute paths unchanged, normalizes relative paths
with os.path.normpath, and when workspace_source == "git" prefixes the path with
"projects/" if the first path segment isn't already "projects"; also note the
final join with config.get_workspace_root() and the function's return type and
parameters (project_path: str, workspace_source: Optional[str]) so callers
understand inputs, outputs, and side effects.
In `@shared/utils/git_util.py`:
- Around line 55-68: Add a clear docstring to get_safe_git_workspace_key
describing its purpose (produce a stable filesystem-safe key for a Git-backed
workspace), its parameters (git_url, domain—explain if domain is kept for API
compatibility and intentionally unused—or remove domain from the signature),
return value (sanitized key or "git_repo" fallback), and the sanitization rules
(strip trailing "/", remove ".git", use
get_project_path_from_url/get_repo_name_from_url to derive repo_name, apply
regex r"[^A-Za-z0-9._-]+" to replace invalid chars, then strip leading/trailing
"._-"). Also either remove the del domain line and the domain parameter if not
needed, or keep the parameter but replace del domain with a short inline comment
like "# domain kept for API compatibility, intentionally unused" to clarify
intent; reference the repo_name local variable, get_project_path_from_url, and
get_repo_name_from_url in the docstring so callers understand resolution order
and fallback behavior.
In `@wework/src/features/workbench/WorkbenchProvider.tsx`:
- Around line 993-1001: The current listGitRepositories and listGitBranches
callbacks swallow a missing gitApi by returning empty arrays; update them to
fail fast by returning a rejected Promise (or throwing) when
resolvedServices.gitApi is falsy. Specifically, in the functions
listGitRepositories and listGitBranches check resolvedServices.gitApi and if
it's missing return Promise.reject(new Error('Git API service not available'))
(or throw the same Error) instead of Promise.resolve([]), so callers see an
explicit failure rather than a silent empty list.
---
Outside diff comments:
In `@wework/src/components/layout/MobileWorkbenchLayout.tsx`:
- Around line 88-110: MobileWorkbenchLayout currently doesn't forward the new
Git-related callbacks declared on MobileWorkbenchLayoutProps down into the child
MobileDrawer; update the MobileWorkbenchLayout function to destructure the
missing Git callbacks from its props and pass them through to the MobileDrawer
component wherever MobileDrawer is rendered so the mobile Git project creation
flow works; reference the prop names as declared on MobileWorkbenchLayoutProps
and ensure MobileDrawer receives the same prop identifiers (i.e., forward each
new callback prop from MobileWorkbenchLayout to MobileDrawer).
---
Nitpick comments:
In `@backend/app/services/project_service.py`:
- Line 197: Extract the hard-coded numeric limits into module-level named
constants: replace the inline timeout_seconds=30 with a constant like
DEFAULT_CHECKOUT_TIMEOUT_SECONDS = 30 and replace the other magic number at the
second occurrence (reported at line 223) with a constant like
DEFAULT_CHECKOUT_OUTPUT_LIMIT; declare these constants at the top of
project_service.py, update the call sites (where timeout_seconds is passed and
where the other numeric literal is used) to reference the new constants, and
ensure any tests or callers that rely on the previous literals continue to work
with the new constant names.
In `@shared/utils/git_util.py`:
- Around line 16-30: Add unit tests for get_safe_git_workspace_key to cover edge
cases: pass empty and whitespace-only repo arguments and assert it falls back to
URL-derived key or the default ("git_repo") when appropriate; test repo names
made only of special characters (e.g., "....", "___", "...") and assert they
sanitize to the fallback "git_repo"; test repo names with leading/trailing
underscores or dots (e.g., "_myrepo_", "my@repo#name.git") and assert they are
cleaned to alphanumeric/underscore form ("myrepo" or "my_repo_name"); test very
long repository names to ensure truncation or length-limiting behavior is
enforced consistently; and test URLs with multiple consecutive slashes or
unusual characters to confirm the function extracts and sanitizes the repo
segment correctly. Ensure each test references get_safe_git_workspace_key and
asserts the exact expected sanitized key or fallback.
In `@wework/src/components/projects/ProjectCreateDialog.test.tsx`:
- Around line 268-368: Add a new test (e.g., "shows normalized error on Git
workspace creation failure") that mounts ProjectCreateDialog (mode="git") and
mocks onCreateGitWorkspaceProject to reject with an error matching the new
normalization paths (for example reject(new Error('token missing')) or a custom
rejection for "directory exists"); select the repository and branch using the
existing test ids ('git-repository-select', 'git-branch-select'), click the
'create-project-button', then await and assert that the dialog does not call
onClose, the submit spinner/progress stop and an error message element displays
the normalized error text (use the same visible selectors used for
progress/errors in other tests), and that the cancel/action buttons return to an
enabled state to allow retry.
In `@wework/src/components/projects/ProjectCreateDialog.tsx`:
- Around line 880-958: The primary action is a raw styled <button>
(data-testid="create-project-button") inside ProjectCreateDialog; replace it
with the standardized dialog primary button component by giving it
variant="primary" (or using the shared DialogPrimaryButton) while preserving
props (onClick handler, disabled, aria-busy, data-testid) and children (Loader2
spinner and labels), and update sizing classes to enforce mobile touch targets
(use h-11 min-w-[44px] instead of the current h-10 and custom bg/hover classes);
remove the hard-coded bg/hover styles that conflict with the shared variant so
the unified primary styling is used.
In `@wework/src/components/projects/ProjectFolderIcon.test.tsx`:
- Around line 6-24: Add a negative test to ProjectFolderIcon.test.tsx that
ensures a non-Git workspace does not render the Git icon: create a
ProjectWithTasks object similar to the existing one but with config.mode ===
'workspace' and workspace.source === 'local-path' (or another non-'git' value),
render <ProjectFolderIcon project={...} className="h-4 w-4" /> and assert the
Git icon test id used in the component (e.g., 'project-git-folder-icon-<id>') is
not present using screen.queryByTestId(...)/toBeNull() or
expect(...).not.toBeInTheDocument(); name the test something like "does not mark
local-path workspace projects with a Git folder icon".
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e62b6df3-d17e-4e35-90fd-ebcac589ccb7
📒 Files selected for processing (34)
backend/app/api/endpoints/projects.pybackend/app/schemas/project.pybackend/app/services/device/command_registry.pybackend/app/services/project_service.pybackend/tests/schemas/test_project_config_schema.pybackend/tests/services/test_local_device_command_service.pybackend/tests/services/test_project_git_workspace_service.pydocs/en/user-guide/coding/git-workspace-projects.mddocs/zh/user-guide/coding/git-workspace-projects.mdexecutor/agents/base.pyexecutor/tests/agents/test_project_workspace_paths.pyshared/tests/utils/test_git_util.pyshared/utils/git_util.pywework/src/api/adapters.test.tswework/src/api/git.tswework/src/api/projects.tswework/src/components/chat/composer/ProjectWorkBar.tsxwework/src/components/layout/DesktopSidebar.tsxwework/src/components/layout/DesktopWorkbenchLayout.test.tsxwework/src/components/layout/DesktopWorkbenchLayout.tsxwework/src/components/layout/MobileDrawer.tsxwework/src/components/layout/MobileWorkbenchLayout.tsxwework/src/components/projects/ProjectCreateDialog.test.tsxwework/src/components/projects/ProjectCreateDialog.tsxwework/src/components/projects/ProjectFolderIcon.test.tsxwework/src/components/projects/ProjectFolderIcon.tsxwework/src/features/workbench/WorkbenchProvider.tsxwework/src/i18n/locales/en/common.jsonwework/src/i18n/locales/zh-CN/common.jsonwework/src/lib/projectClassification.tswework/src/pages/PluginManagementPage.tsxwework/src/pages/PluginsPage.tsxwework/src/pages/WorkbenchPage.tsxwework/src/types/api.ts
Comment on lines
+98
to
+103
| async def create_git_workspace_project_endpoint( | ||
| project_create: GitWorkspaceProjectCreate, | ||
| current_user: User = Depends(get_current_user), | ||
| db: Session = Depends(get_db), | ||
| ): | ||
| """Create a Git-backed workspace project and clone it on the selected device.""" |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Instrument the new async endpoint with required tracing decorators/events.
create_git_workspace_project_endpoint is newly added and should be wrapped with @trace_async and span metadata for request-level observability.
As per coding guidelines: Use OpenTelemetry tracing decorators: @trace_asyncor@trace_sync for entire functions, and add_span_event() or set_span_attribute() for adding details to spans.
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 100-100: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
[warning] 101-101: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/api/endpoints/projects.py` around lines 98 - 103, Add
OpenTelemetry tracing to the new async endpoint by decorating
create_git_workspace_project_endpoint with `@trace_async` (importing trace_async
from the tracing utilities used in the repo) and instrumenting key points inside
the function with span metadata: call
add_span_event("create_git_workspace_project.started") at start,
set_span_attribute("user.id", current_user.id) and
set_span_attribute("project.type","git_workspace") after validation, and
add_span_event("git.clone.started", {"repo": project_create.repo_url}) before
cloning and add_span_event("git.clone.completed") (or an error event on failure)
after cloning; use the existing add_span_event/set_span_attribute helpers (or
their equivalents) to ensure request-level observability.
Comment on lines
+92
to
+97
| async def create_git_workspace_project( | ||
| db: Session, | ||
| project_data: GitWorkspaceProjectCreate, | ||
| user_id: int, | ||
| ) -> GitWorkspaceProjectResponse: | ||
| """Create a Git-backed workspace project and clone its checkout.""" |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add required tracing instrumentation to new Git workspace flow functions.
The newly added async service entry points are missing @trace_async and span annotations.
Example pattern
+from app.observability.tracing import trace_async, add_span_event, set_span_attribute
...
+@trace_async
async def create_git_workspace_project(...):
+ set_span_attribute('project.source', 'git')
+ add_span_event('project.create_git_workspace.start')
...
+@trace_async
async def _prepare_git_checkout(...):
+ set_span_attribute('device.id', device_id)
+ add_span_event('project.git_checkout.prepare')
...As per coding guidelines: Use OpenTelemetry tracing decorators: @trace_asyncor@trace_sync for entire functions, and add_span_event() or set_span_attribute() for adding details to spans.
Also applies to: 156-166
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/services/project_service.py` around lines 92 - 97, The new async
service entry point create_git_workspace_project is missing OpenTelemetry
instrumentation: add the `@trace_async` decorator above the async def and inside
the function set relevant span attributes/events (e.g., call set_span_attribute
for project_data fields like repository/url/branch and add_span_event for key
lifecycle steps such as "clone_started" / "clone_succeeded" / "clone_failed");
apply the same pattern to the other async functions referenced around lines
156-166 so each async entry point uses `@trace_async` and emits span
attributes/events to surface important parameters and status.
Comment on lines
+190
to
+200
| exists_result = await execute_configured_device_command( | ||
| db=db, | ||
| user_id=user_id, | ||
| device_id=device_id, | ||
| command_key="path_exists", | ||
| path=workspace_root, | ||
| args=[checkout_path], | ||
| timeout_seconds=30, | ||
| ) | ||
| if _command_succeeded(exists_result): | ||
| raise _target_path_exists_error(target_path) |
Contributor
There was a problem hiding this comment.
Handle path_exists execution errors separately from “path not found”.
On Line 199, any non-zero result is treated as “not exists”. If the check command itself fails, clone still proceeds and masks the root cause.
Suggested fix
exists_result = await execute_configured_device_command(
db=db,
user_id=user_id,
device_id=device_id,
command_key="path_exists",
path=workspace_root,
args=[checkout_path],
timeout_seconds=30,
)
+ exists_exit_code = exists_result.get("exit_code")
if _command_succeeded(exists_result):
raise _target_path_exists_error(target_path)
+ if exists_exit_code != 1:
+ _raise_for_failed_command(
+ exists_result,
+ "Failed to check whether target project directory exists",
+ )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/services/project_service.py` around lines 190 - 200, The current
logic treats any non-zero result from execute_configured_device_command(...,
command_key="path_exists") as “not exists” and proceeds, which masks execution
failures; update the check around exists_result returned by
execute_configured_device_command (used with command_key "path_exists") to
distinguish an execution error from a genuine “not found” result: inspect the
result's execution status/exit code and error/message fields from exists_result,
if the command failed (non-standard error code or an error message), raise or
propagate an execution error (include the command error details), and only call
_target_path_exists_error when the result specifically indicates the target path
exists per _command_succeeded semantics. Ensure references to
execute_configured_device_command, _command_succeeded, and
_target_path_exists_error are used to locate and update the logic.
Comment on lines
+424
to
+440
| def _resolve_workspace_path( | ||
| self, | ||
| project_path: str, | ||
| *, | ||
| workspace_source: Optional[str], | ||
| ) -> str: | ||
| resolved_path = os.path.expanduser(project_path) | ||
| if os.path.isabs(resolved_path): | ||
| return resolved_path | ||
|
|
||
| normalized_path = os.path.normpath(resolved_path) | ||
| if workspace_source == "git": | ||
| first_segment = normalized_path.split(os.sep, 1)[0] | ||
| if first_segment != "projects": | ||
| normalized_path = os.path.join("projects", normalized_path) | ||
|
|
||
| return os.path.join(config.get_workspace_root(), normalized_path) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add docstring to explain workspace path resolution behavior.
The _resolve_workspace_path helper lacks a docstring. This function implements important logic (expanduser, absolute/relative handling, and git-specific "projects/" auto-prefixing) that should be documented.
📝 Proposed docstring
def _resolve_workspace_path(
self,
project_path: str,
*,
workspace_source: Optional[str],
) -> str:
+ """
+ Resolve a project workspace path to an absolute path.
+
+ Handles:
+ - Tilde expansion for user home directories
+ - Absolute paths (returned as-is after expansion)
+ - Relative paths (joined under workspace root)
+ - Git source auto-prefixing: relative paths are placed under "projects/"
+ subdirectory unless already prefixed with "projects/"
+
+ Args:
+ project_path: The workspace path (may be relative, absolute, or use ~)
+ workspace_source: The workspace source type ("git", "local_path", etc.)
+
+ Returns:
+ Absolute path to the workspace directory.
+ """
resolved_path = os.path.expanduser(project_path)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _resolve_workspace_path( | |
| self, | |
| project_path: str, | |
| *, | |
| workspace_source: Optional[str], | |
| ) -> str: | |
| resolved_path = os.path.expanduser(project_path) | |
| if os.path.isabs(resolved_path): | |
| return resolved_path | |
| normalized_path = os.path.normpath(resolved_path) | |
| if workspace_source == "git": | |
| first_segment = normalized_path.split(os.sep, 1)[0] | |
| if first_segment != "projects": | |
| normalized_path = os.path.join("projects", normalized_path) | |
| return os.path.join(config.get_workspace_root(), normalized_path) | |
| def _resolve_workspace_path( | |
| self, | |
| project_path: str, | |
| *, | |
| workspace_source: Optional[str], | |
| ) -> str: | |
| """ | |
| Resolve a project workspace path to an absolute path. | |
| Handles: | |
| - Tilde expansion for user home directories | |
| - Absolute paths (returned as-is after expansion) | |
| - Relative paths (joined under workspace root) | |
| - Git source auto-prefixing: relative paths are placed under "projects/" | |
| subdirectory unless already prefixed with "projects/" | |
| Args: | |
| project_path: The workspace path (may be relative, absolute, or use ~) | |
| workspace_source: The workspace source type ("git", "local_path", etc.) | |
| Returns: | |
| Absolute path to the workspace directory. | |
| """ | |
| resolved_path = os.path.expanduser(project_path) | |
| if os.path.isabs(resolved_path): | |
| return resolved_path | |
| normalized_path = os.path.normpath(resolved_path) | |
| if workspace_source == "git": | |
| first_segment = normalized_path.split(os.sep, 1)[0] | |
| if first_segment != "projects": | |
| normalized_path = os.path.join("projects", normalized_path) | |
| return os.path.join(config.get_workspace_root(), normalized_path) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@executor/agents/base.py` around lines 424 - 440, Add a clear docstring to the
_resolve_workspace_path method describing its behavior: explain that it expands
~ via os.path.expanduser, returns absolute paths unchanged, normalizes relative
paths with os.path.normpath, and when workspace_source == "git" prefixes the
path with "projects/" if the first path segment isn't already "projects"; also
note the final join with config.get_workspace_root() and the function's return
type and parameters (project_path: str, workspace_source: Optional[str]) so
callers understand inputs, outputs, and side effects.
Comment on lines
+55
to
+68
| def get_safe_git_workspace_key( | ||
| git_url: str, | ||
| domain: str | None = None, | ||
| repo: str | None = None, | ||
| ) -> str: | ||
| """Return a stable directory key for a Git-backed project workspace.""" | ||
| del domain | ||
| repo_name = ( | ||
| repo or get_project_path_from_url(git_url) or get_repo_name_from_url(git_url) | ||
| ).strip() | ||
| repo_name = repo_name.rstrip("/").removesuffix(".git").split("/")[-1] | ||
| key = re.sub(r"[^A-Za-z0-9._-]+", "_", repo_name) | ||
| return key.strip("._-") or "git_repo" | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add docstring and clarify unused domain parameter.
The function lacks a docstring explaining its purpose, parameters, return value, and the sanitization rules it applies. Additionally, the domain parameter is accepted but immediately deleted (line 61), which is confusing. If domain is not needed, consider removing it from the signature. If it's kept for API compatibility, document why it's unused.
📝 Proposed docstring and signature cleanup
def get_safe_git_workspace_key(
git_url: str,
- domain: str | None = None,
repo: str | None = None,
) -> str:
- """Return a stable directory key for a Git-backed project workspace."""
- del domain
+ """
+ Generate a safe, sanitized directory key for a Git-backed project workspace.
+
+ Derives a stable directory name from the repository information by:
+ 1. Using `repo` if provided
+ 2. Otherwise extracting the project path from `git_url`
+ 3. Falling back to extracting the repo name from `git_url`
+ 4. Normalizing the result by removing .git suffix, taking the last path component,
+ replacing non-alphanumeric characters (except ._-) with underscores,
+ and trimming leading/trailing special characters.
+
+ Args:
+ git_url: Git repository URL (required)
+ repo: Optional repository path override (e.g., "org/repo")
+
+ Returns:
+ A sanitized directory key safe for filesystem use, or "git_repo" if derivation fails.
+ """
repo_name = (
repo or get_project_path_from_url(git_url) or get_repo_name_from_url(git_url)
).strip()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shared/utils/git_util.py` around lines 55 - 68, Add a clear docstring to
get_safe_git_workspace_key describing its purpose (produce a stable
filesystem-safe key for a Git-backed workspace), its parameters (git_url,
domain—explain if domain is kept for API compatibility and intentionally
unused—or remove domain from the signature), return value (sanitized key or
"git_repo" fallback), and the sanitization rules (strip trailing "/", remove
".git", use get_project_path_from_url/get_repo_name_from_url to derive
repo_name, apply regex r"[^A-Za-z0-9._-]+" to replace invalid chars, then strip
leading/trailing "._-"). Also either remove the del domain line and the domain
parameter if not needed, or keep the parameter but replace del domain with a
short inline comment like "# domain kept for API compatibility, intentionally
unused" to clarify intent; reference the repo_name local variable,
get_project_path_from_url, and get_repo_name_from_url in the docstring so
callers understand resolution order and fallback behavior.
Comment on lines
+993
to
+1001
| const listGitRepositories = useCallback( | ||
| () => resolvedServices.gitApi?.listRepositories() ?? Promise.resolve([]), | ||
| [resolvedServices] | ||
| ) | ||
|
|
||
| const listGitBranches = useCallback( | ||
| (repo: GitRepoInfo) => resolvedServices.gitApi?.listBranches(repo) ?? Promise.resolve([]), | ||
| [resolvedServices] | ||
| ) |
Contributor
There was a problem hiding this comment.
Fail fast when Git API service is missing instead of returning empty lists.
Returning [] here masks service wiring failures as “no repositories/branches.” Rejecting with an explicit error lets the dialog show a real failure state.
Suggested fix
- const listGitRepositories = useCallback(
- () => resolvedServices.gitApi?.listRepositories() ?? Promise.resolve([]),
- [resolvedServices]
- )
+ const listGitRepositories = useCallback(() => {
+ if (!resolvedServices.gitApi) {
+ return Promise.reject(new Error('Git repository service is unavailable'))
+ }
+ return resolvedServices.gitApi.listRepositories()
+ }, [resolvedServices])
- const listGitBranches = useCallback(
- (repo: GitRepoInfo) => resolvedServices.gitApi?.listBranches(repo) ?? Promise.resolve([]),
- [resolvedServices]
- )
+ const listGitBranches = useCallback((repo: GitRepoInfo) => {
+ if (!resolvedServices.gitApi) {
+ return Promise.reject(new Error('Git branch service is unavailable'))
+ }
+ return resolvedServices.gitApi.listBranches(repo)
+ }, [resolvedServices])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wework/src/features/workbench/WorkbenchProvider.tsx` around lines 993 - 1001,
The current listGitRepositories and listGitBranches callbacks swallow a missing
gitApi by returning empty arrays; update them to fail fast by returning a
rejected Promise (or throwing) when resolvedServices.gitApi is falsy.
Specifically, in the functions listGitRepositories and listGitBranches check
resolvedServices.gitApi and if it's missing return Promise.reject(new Error('Git
API service not available')) (or throw the same Error) instead of
Promise.resolve([]), so callers see an explicit failure rather than a silent
empty list.
FicoHu
force-pushed
the
feature/git-workspace-project
branch
from
June 5, 2026 11:19
dbb7e52 to
b033133
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/app/services/project_service.py (1)
138-140: 💤 Low valueException swallowed without logging before deactivation.
When
_prepare_git_checkoutfails, the project is deactivated and the exception re-raised. Consider adding a log statement before deactivation to capture the failure context for debugging, especially since the original exception may not include the project ID.♻️ Suggested improvement
+import logging + +logger = logging.getLogger(__name__) + ... try: reused_checkout = await _prepare_git_checkout(...) except Exception: + logger.warning( + "Git checkout failed for project %d, deactivating", + project.id, + exc_info=True, + ) _deactivate_project(db, project.id, user_id, project_data.client_origin) raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/services/project_service.py` around lines 138 - 140, Before calling _deactivate_project in the except block that catches failures from _prepare_git_checkout, capture and log the exception details (use logging.exception or logger.exception) including project.id, user_id and project_data.client_origin so the failure context is preserved; then call _deactivate_project(db, project.id, user_id, project_data.client_origin) and re-raise the exception as currently done. Ensure you bind the exception (except Exception as e) so the full stack/message is logged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/app/services/project_service.py`:
- Around line 138-140: Before calling _deactivate_project in the except block
that catches failures from _prepare_git_checkout, capture and log the exception
details (use logging.exception or logger.exception) including project.id,
user_id and project_data.client_origin so the failure context is preserved; then
call _deactivate_project(db, project.id, user_id, project_data.client_origin)
and re-raise the exception as currently done. Ensure you bind the exception
(except Exception as e) so the full stack/message is logged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c84f1369-19d6-4e81-9156-2340022c94f3
📒 Files selected for processing (32)
backend/app/api/endpoints/projects.pybackend/app/schemas/project.pybackend/app/services/device/command_registry.pybackend/app/services/project_service.pybackend/tests/schemas/test_project_config_schema.pybackend/tests/services/test_local_device_command_service.pybackend/tests/services/test_project_git_workspace_service.pydocs/en/user-guide/coding/git-workspace-projects.mddocs/zh/user-guide/coding/git-workspace-projects.mdshared/tests/utils/test_git_util.pyshared/utils/git_util.pywework/src/api/adapters.test.tswework/src/api/git.tswework/src/api/projects.tswework/src/components/chat/composer/ProjectWorkBar.tsxwework/src/components/layout/DesktopSidebar.tsxwework/src/components/layout/DesktopWorkbenchLayout.test.tsxwework/src/components/layout/DesktopWorkbenchLayout.tsxwework/src/components/layout/MobileDrawer.tsxwework/src/components/layout/MobileWorkbenchLayout.tsxwework/src/components/projects/ProjectCreateDialog.test.tsxwework/src/components/projects/ProjectCreateDialog.tsxwework/src/components/projects/ProjectFolderIcon.test.tsxwework/src/components/projects/ProjectFolderIcon.tsxwework/src/features/workbench/WorkbenchProvider.tsxwework/src/i18n/locales/en/common.jsonwework/src/i18n/locales/zh-CN/common.jsonwework/src/lib/projectClassification.tswework/src/pages/PluginManagementPage.tsxwework/src/pages/PluginsPage.tsxwework/src/pages/WorkbenchPage.tsxwework/src/types/api.ts
✅ Files skipped from review due to trivial changes (3)
- wework/src/components/layout/DesktopWorkbenchLayout.tsx
- wework/src/lib/projectClassification.ts
- wework/src/i18n/locales/en/common.json
🚧 Files skipped from review as they are similar to previous changes (21)
- backend/app/services/device/command_registry.py
- wework/src/components/projects/ProjectFolderIcon.test.tsx
- wework/src/api/projects.ts
- wework/src/pages/WorkbenchPage.tsx
- wework/src/api/adapters.test.ts
- wework/src/components/projects/ProjectFolderIcon.tsx
- backend/tests/schemas/test_project_config_schema.py
- wework/src/components/layout/MobileDrawer.tsx
- shared/tests/utils/test_git_util.py
- shared/utils/git_util.py
- wework/src/api/git.ts
- wework/src/components/projects/ProjectCreateDialog.test.tsx
- backend/tests/services/test_project_git_workspace_service.py
- wework/src/features/workbench/WorkbenchProvider.tsx
- wework/src/components/chat/composer/ProjectWorkBar.tsx
- backend/app/schemas/project.py
- wework/src/types/api.ts
- wework/src/components/layout/DesktopWorkbenchLayout.test.tsx
- wework/src/i18n/locales/zh-CN/common.json
- wework/src/components/layout/DesktopSidebar.tsx
- wework/src/components/projects/ProjectCreateDialog.tsx
qdaxb
approved these changes
Jun 5, 2026
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
2 participants
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.
Summary by CodeRabbit
Release Notes
New Features
Documentation