feat(workspace): add project detection for workspace grouping#91
Merged
feloy merged 1 commit intokortex-hub:mainfrom Mar 23, 2026
Merged
feat(workspace): add project detection for workspace grouping#91feloy merged 1 commit intokortex-hub:mainfrom
feloy merged 1 commit intokortex-hub:mainfrom
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughAdds project detection and grouping: auto-detects a workspace project from git (preferring Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as "CLI (init)"
participant Manager
participant Detector as "Git Detector"
participant Exec as "Git Executor"
User->>CLI: run kortex-cli init [--project custom]
CLI->>Manager: Add(opts{Project: "custom"/"" , sourceDir})
alt custom project provided
Manager->>Manager: set instance.Project = opts.Project
else auto-detect
Manager->>Detector: DetectRepository(ctx, sourceDir)
Detector->>Exec: git rev-parse --show-toplevel
Exec-->>Detector: root dir or exit 128
alt repo detected
Detector->>Exec: git remote get-url upstream
Exec-->>Detector: remote URL / error
Detector->>Exec: git remote get-url origin
Exec-->>Detector: remote URL / error
Detector-->>Manager: RepositoryInfo{RootDir, RemoteURL, RelativePath}
Manager->>Manager: build Project = normalize(RemoteURL) + "/" + RelativePath
else not a git repo
Detector-->>Manager: ErrNotGitRepository
Manager->>Manager: Project = sourceDir
end
end
Manager->>Manager: create instance with Project
Manager-->>CLI: instance created
CLI-->>User: workspace registered
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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. 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/instances/manager.go (2)
273-284:⚠️ Potential issue | 🟠 MajorProject field not preserved when starting instance.
The
updatedInstanceis missing theProjectfield. AfterStart()is called, the persisted instance will lose its project association.🐛 Proposed fix
// Update the instance with new runtime state updatedInstance := &instance{ ID: instanceToStart.GetID(), Name: instanceToStart.GetName(), SourceDir: instanceToStart.GetSourceDir(), ConfigDir: instanceToStart.GetConfigDir(), Runtime: RuntimeData{ Type: runtimeData.Type, InstanceID: runtimeData.InstanceID, State: runtimeInfo.State, Info: runtimeInfo.Info, }, + Project: instanceToStart.GetProject(), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/instances/manager.go` around lines 273 - 284, The new updatedInstance constructed in Start() omits the Project field so the persisted instance loses its project association; update the construction of updatedInstance (created from instanceToStart in Start/manager.go) to include Project: instanceToStart.GetProject() (or the equivalent Project value from instanceToStart) so the instance struct's Project is preserved when calling Start() and saving the updated instance.
341-352:⚠️ Potential issue | 🟠 MajorProject field not preserved when stopping instance.
Same issue as
Start()— theProjectfield is omitted fromupdatedInstance, causing data loss on save.🐛 Proposed fix
// Update the instance with new runtime state updatedInstance := &instance{ ID: instanceToStop.GetID(), Name: instanceToStop.GetName(), SourceDir: instanceToStop.GetSourceDir(), ConfigDir: instanceToStop.GetConfigDir(), Runtime: RuntimeData{ Type: runtimeData.Type, InstanceID: runtimeData.InstanceID, State: runtimeInfo.State, Info: runtimeInfo.Info, }, + Project: instanceToStop.GetProject(), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/instances/manager.go` around lines 341 - 352, The Stop() flow constructs updatedInstance without copying the Project field, causing data loss on save; update the construction of updatedInstance (the instance literal created in Stop(), same pattern as Start()) to include Project: instanceToStop.GetProject() (or the appropriate accessor) so the Project value is preserved in the Runtime/instance struct before saving.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/instances/manager.go`:
- Around line 524-528: Normalize repoInfo.RelativePath to use forward slashes
before concatenating with baseURL to avoid backslash URL bugs on Windows:
replace the direct concatenation return baseURL + repoInfo.RelativePath with
normalization (e.g., use filepath.ToSlash(repoInfo.RelativePath) or
strings.ReplaceAll with string(os.PathSeparator) -> "/") and then append that
normalized value to baseURL; ensure you import the needed package (path/filepath
or strings/os) and keep the comment about avoiding path.Join.
---
Outside diff comments:
In `@pkg/instances/manager.go`:
- Around line 273-284: The new updatedInstance constructed in Start() omits the
Project field so the persisted instance loses its project association; update
the construction of updatedInstance (created from instanceToStart in
Start/manager.go) to include Project: instanceToStart.GetProject() (or the
equivalent Project value from instanceToStart) so the instance struct's Project
is preserved when calling Start() and saving the updated instance.
- Around line 341-352: The Stop() flow constructs updatedInstance without
copying the Project field, causing data loss on save; update the construction of
updatedInstance (the instance literal created in Stop(), same pattern as
Start()) to include Project: instanceToStop.GetProject() (or the appropriate
accessor) so the Project value is preserved in the Runtime/instance struct
before saving.
🪄 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: b80fb989-992d-4cc2-a89e-f2bb58eaf083
📒 Files selected for processing (11)
AGENTS.mdREADME.mdpkg/cmd/init.gopkg/cmd/init_test.gopkg/git/exec.gopkg/git/git.gopkg/git/git_test.gopkg/instances/instance.gopkg/instances/manager.gopkg/instances/manager_project_test.gopkg/instances/manager_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/git/git_test.go (1)
239-246: Consider using a simpler interface forexitError128.The
exitError128type embedsexec.ExitErrorwhich has unexported fields that will be zero-valued. This works because the production code only callsExitCode(), but embedding the full struct is unnecessary and could cause confusion.♻️ Proposed simplification
// exitError128 is a test helper that simulates git exit code 128 -type exitError128 struct { - exec.ExitError -} +type exitError128 struct{} func (e *exitError128) ExitCode() int { return 128 } + +func (e *exitError128) Error() string { + return "exit status 128" +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/git/git_test.go` around lines 239 - 246, The test helper exitError128 currently embeds exec.ExitError unnecessarily; replace the embedding with a minimal type (e.g., type exitError128 struct{}) that implements the methods used by production code: implement ExitCode() int returning 128 and also Error() string to satisfy the error interface, removing the exec.ExitError embedding and any dependence on its unexported fields; update any test construction or assertions to use the new minimal exitError128 type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/git/git_test.go`:
- Around line 239-246: The test helper exitError128 currently embeds
exec.ExitError unnecessarily; replace the embedding with a minimal type (e.g.,
type exitError128 struct{}) that implements the methods used by production code:
implement ExitCode() int returning 128 and also Error() string to satisfy the
error interface, removing the exec.ExitError embedding and any dependence on its
unexported fields; update any test construction or assertions to use the new
minimal exitError128 type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 27bf71a5-a03c-4241-a385-2fe49bfc982b
📒 Files selected for processing (11)
AGENTS.mdREADME.mdpkg/cmd/init.gopkg/cmd/init_test.gopkg/git/exec.gopkg/git/git.gopkg/git/git_test.gopkg/instances/instance.gopkg/instances/manager.gopkg/instances/manager_project_test.gopkg/instances/manager_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/git/exec.go
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/cmd/init.go
- pkg/instances/manager_test.go
- pkg/git/git.go
- pkg/cmd/init_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pkg/git/git.go (2)
84-90: Consider usingerrors.As()for more robust error unwrapping.The direct type assertion
err.(exitCoder)won't match if the error is wrapped. While this currently works withos/execerrors, usingerrors.As()is the idiomatic Go pattern and more resilient to future changes.♻️ Suggested improvement
output, err := d.executor.Output(ctx, absDir, "rev-parse", "--show-toplevel") if err != nil { // Exit status 128 means not a git repository - if exitErr, ok := err.(exitCoder); ok && exitErr.ExitCode() == 128 { + var exitErr exitCoder + if errors.As(err, &exitErr) && exitErr.ExitCode() == 128 { return nil, ErrNotGitRepository } return nil, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/git/git.go` around lines 84 - 90, Replace the direct type assertion on err with errors.As to correctly unwrap wrapped errors: when checking for the exit code 128, declare a variable of type exitCoder (e.g., var exitErr exitCoder) and use errors.As(err, &exitErr) to detect the underlying exitCoder, then check exitErr.ExitCode() == 128 and return ErrNotGitRepository; update the error handling branch in the code that currently does if err != nil { ... } so it uses errors.As instead of err.(exitCoder).
107-123: Minor duplication in remote URL processing.The trim and suffix removal logic is duplicated for both upstream and origin remotes. Consider extracting to reduce repetition, though this is a minor nitpick.
♻️ Optional refactor to reduce duplication
// Get the remote URL (try upstream first, then origin) - remoteURL := "" - output, err = d.executor.Output(ctx, rootDir, "remote", "get-url", "upstream") - if err == nil { - remoteURL = strings.TrimSpace(string(output)) - // Remove .git suffix if present - remoteURL = strings.TrimSuffix(remoteURL, ".git") - } else { - // upstream not found, try origin - output, err = d.executor.Output(ctx, rootDir, "remote", "get-url", "origin") - if err == nil { - remoteURL = strings.TrimSpace(string(output)) - // Remove .git suffix if present - remoteURL = strings.TrimSuffix(remoteURL, ".git") + remoteURL := d.getRemoteURL(ctx, rootDir, "upstream") + if remoteURL == "" { + remoteURL = d.getRemoteURL(ctx, rootDir, "origin") + }Add a helper method:
// getRemoteURL returns the URL for the specified remote, or empty string if not found func (d *detector) getRemoteURL(ctx context.Context, dir, remote string) string { + output, err := d.executor.Output(ctx, dir, "remote", "get-url", remote) + if err != nil { + return "" + } + url := strings.TrimSpace(string(output)) + return strings.TrimSuffix(url, ".git") +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/git/git.go` around lines 107 - 123, The remote URL trimming/suffix removal is duplicated; add a helper method named getRemoteURL on the detector that calls d.executor.Output(ctx, dir, "remote", "get-url", remote), returns empty string on error, otherwise returns strings.TrimSuffix(strings.TrimSpace(string(output)), ".git"); then replace the inline logic in the existing function to set remoteURL = d.getRemoteURL(ctx, rootDir, "upstream") and if that returns empty call d.getRemoteURL(ctx, rootDir, "origin") so all trimming/cleanup is centralized in getRemoteURL.pkg/instances/instance.go (2)
165-168: Persistence and accessor wiring forProjectis consistent.
GetProject(),Dump(), andNewInstanceFromData()align, which gives a solid round-trip path for stored project identifiers.For reliability, please add a focused unit test for
Projectround-trip (instance -> Dump -> NewInstanceFromData -> GetProject) to guard regressions.Also applies to: 180-180, 246-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/instances/instance.go` around lines 165 - 168, Add a focused unit test that verifies the Project field round-trips through the instance serialization cycle: create an instance (use the constructor or struct literal), set Project, call instance.Dump(), reconstruct with NewInstanceFromData(dumpedData), and assert reconstructed.GetProject() equals the original Project; reference GetProject, Dump, and NewInstanceFromData in the test to catch regressions and include edge cases like empty and non-empty project strings.
110-111: Consider whetherProjectshould be passed toNewInstancefor more explicit initialization.Currently,
Projectis set duringManager.Add()when a new instance struct is created with the project identifier. While this works correctly (no out-of-band mutation occurs), passingProjectthroughNewInstanceParamswould make the factory's purpose clearer and reduce the multi-step initialization pattern.This is an optional API improvement; the current design where
Manager.Add()handles full instance initialization is sound.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/instances/instance.go` around lines 110 - 111, The Project field is currently assigned in Manager.Add() after creating the instance; change NewInstance to accept Project via NewInstanceParams so instances are fully initialized by the factory. Update the NewInstance signature and NewInstanceParams struct to include Project, propagate that new param through calls from Manager.Add, and remove the out-of-factory assignment of instance.Project so the factory (NewInstance) sets Project directly on the instance struct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/instances/instance.go`:
- Around line 63-67: The struct field comment for Project on the Instance type
is outdated and should be updated to accurately describe the normalized project
identifier produced by the detection logic; update the comment for the Project
string `json:"project"` to describe the current formats: a normalized remote
repository identifier (remote URL normalized/stripped of `.git` and credentials,
not raw remote URL), a repository root path for local-only git repos (absolute
path), and a workspace source directory for non-git directories, and also
mention that a relative-path suffix may be appended when detection includes a
subpath; ensure the comment references the Project field and uses the same
terminology as the project-detection behavior in this PR.
---
Nitpick comments:
In `@pkg/git/git.go`:
- Around line 84-90: Replace the direct type assertion on err with errors.As to
correctly unwrap wrapped errors: when checking for the exit code 128, declare a
variable of type exitCoder (e.g., var exitErr exitCoder) and use errors.As(err,
&exitErr) to detect the underlying exitCoder, then check exitErr.ExitCode() ==
128 and return ErrNotGitRepository; update the error handling branch in the code
that currently does if err != nil { ... } so it uses errors.As instead of
err.(exitCoder).
- Around line 107-123: The remote URL trimming/suffix removal is duplicated; add
a helper method named getRemoteURL on the detector that calls
d.executor.Output(ctx, dir, "remote", "get-url", remote), returns empty string
on error, otherwise returns
strings.TrimSuffix(strings.TrimSpace(string(output)), ".git"); then replace the
inline logic in the existing function to set remoteURL = d.getRemoteURL(ctx,
rootDir, "upstream") and if that returns empty call d.getRemoteURL(ctx, rootDir,
"origin") so all trimming/cleanup is centralized in getRemoteURL.
In `@pkg/instances/instance.go`:
- Around line 165-168: Add a focused unit test that verifies the Project field
round-trips through the instance serialization cycle: create an instance (use
the constructor or struct literal), set Project, call instance.Dump(),
reconstruct with NewInstanceFromData(dumpedData), and assert
reconstructed.GetProject() equals the original Project; reference GetProject,
Dump, and NewInstanceFromData in the test to catch regressions and include edge
cases like empty and non-empty project strings.
- Around line 110-111: The Project field is currently assigned in Manager.Add()
after creating the instance; change NewInstance to accept Project via
NewInstanceParams so instances are fully initialized by the factory. Update the
NewInstance signature and NewInstanceParams struct to include Project, propagate
that new param through calls from Manager.Add, and remove the out-of-factory
assignment of instance.Project so the factory (NewInstance) sets Project
directly on the instance struct.
🪄 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: 159aa613-5ab6-4195-bedd-b982768c62d1
📒 Files selected for processing (11)
AGENTS.mdREADME.mdpkg/cmd/init.gopkg/cmd/init_test.gopkg/git/exec.gopkg/git/git.gopkg/git/git_test.gopkg/instances/instance.gopkg/instances/manager.gopkg/instances/manager_project_test.gopkg/instances/manager_test.go
✅ Files skipped from review due to trivial changes (3)
- README.md
- pkg/git/exec.go
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/cmd/init.go
- pkg/cmd/init_test.go
- pkg/git/git_test.go
- pkg/instances/manager_project_test.go
- pkg/instances/manager_test.go
- pkg/instances/manager.go
benoitf
approved these changes
Mar 23, 2026
Implements project abstraction that allows grouping workspaces belonging to the same project across different branches, forks, or subdirectories. The project identifier is automatically detected from: - Git repository with remote: uses upstream (or origin) URL + relative path - Git repository without remote: uses repository root + relative path - Non-git directory: uses source directory path Users can override auto-detection with the --project flag to specify custom identifiers for organizational needs (e.g., team names, clients). Closes kortex-hub#89, kortex-hub#41 Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
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
3 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.
Implements project abstraction that allows grouping workspaces belonging to the same project across different branches, forks, or subdirectories.
The project identifier is automatically detected from:
Users can override auto-detection with the --project flag to specify custom identifiers for organizational needs (e.g., team names, clients).
Closes #89, #41
This will be needed for giving specific user configuration per project (see #88)