Skip to content

Conversation

@pilat
Copy link
Owner

@pilat pilat commented Dec 26, 2025

Summary

  • Refactored manager package from package-level functions to a struct-based Manager type for better testability and dependency injection
  • Added comprehensive unit tests for the manager package (1596+ lines of test code)
  • Added Service interface to git package and FileSystem abstraction in internal/pkg/fs for improved testability
  • Fixed hosts file update to only show message when changes actually occur
  • Fixed certificate sync to correctly handle multiple PEM blocks when checking keychain

Summary by CodeRabbit

  • Refactoring

    • Reworked project management to a centralized manager for more reliable project autodetection, init/list/load/destroy flows.
    • Improved source detection for mount/unmount and restart operations, reducing false positives and handling edge cases.
    • Propagates context through CLI operations for more consistent behavior.
  • Tests

    • Expanded autogenerated mocks and filesystem abstractions to increase test coverage and reliability.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 26, 2025

Warning

Rate limit exceeded

@pilat has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 8 minutes and 59 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between afd019a and 00705c9.

⛔ Files ignored due to path filters (1)
  • internal/manager/manager_test.go is excluded by !**/*_test.go
📒 Files selected for processing (3)
  • cmd/devbox/devbox.go
  • cmd/devbox/list.go
  • internal/manager/manager.go

Walkthrough

This PR centralizes project logic into a new Manager type, adds filesystem and git service interfaces (with OS implementations and mocks), and updates CLI commands to use an instantiated manager with context-aware APIs and structured autodetection/source-result types.

Changes

Cohort / File(s) Summary
Configuration
\.mockery.yaml
Added mock targets: github.com/pilat/devbox/internal/git: Service and github.com/pilat/devbox/internal/pkg/fs: FileSystem, FileInfo, DirEntry.
Filesystem abstraction
internal/pkg/fs/fs.go
New interfaces FileSystem, FileInfo, DirEntry and OSFileSystem implementation wrapping stdlib calls; compile-time assertion added.
Filesystem mocks
internal/pkg/fs/mock_FileSystem.go, internal/pkg/fs/mock_FileInfo.go, internal/pkg/fs/mock_DirEntry.go
Auto-generated testify mocks for the new fs interfaces.
Git service interface
internal/git/git.go
Added exported Service interface (Clone, Sync, Pull, GetInfo, GetRemote, GetTopLevel) and changed New to return Service; compile-time assertion added.
Git mocks
internal/git/mock_Service.go, internal/git/mock_CommandRunner.go
Added autogenerated MockService and updated mock header.
Manager core
internal/manager/manager.go
Introduced Manager type with DI hooks (gitFactory, fs), SourceDetectionResult (or similar), and many methods: New, AutodetectProject(ctx,...), AutodetectSource(ctx,...), Init, Destroy, List, Load, GetLocalMountCandidates, GetLocalMounts, plus detection helpers (detectByLocalMount, detectBySourceRemote, detectByProjectRepo, etc.). Significant logic moved into Manager.
Manager API consumers (CLI)
cmd/devbox/*.go (e.g. devbox.go, init.go, list.go, mount.go, umount.go, run.go, up.go, down.go, ps.go, logs.go, restart.go, env.go, info.go, install_ca.go, update.go, update_hosts.go, destroy.go, shell.go)
Replaced package-level manager calls with an instantiated mgr (var mgr *manager.Manager) initialized in initDocker. Updated call signatures to pass context.Context (e.g., mgr.AutodetectProject(ctx, name)) and adjusted uses of AutodetectSource to accept/return structured result (Sources, LocalPath, AffectedServices). list.go now uses mgr.List / mgr.Load. Many repetitive but API-significant changes.

Sequence Diagram(s)

sequenceDiagram
  participant CLI as CLI Command
  participant Manager as Manager
  participant FS as FileSystem
  participant Git as Git Service
  participant Project as Project Repo

  CLI->>Manager: AutodetectProject(ctx, name)
  Manager->>FS: ReadDir / Stat (cwd, mounts)
  alt matched local mount
    FS-->>Manager: local mount path
    Manager->>Project: Load(project by mount)
    Project-->>Manager: Project
  else check git remotes
    Manager->>Git: New(path).GetRemote(ctx)
    Git-->>Manager: remote URL
    Manager->>Project: match remote → map to project
    Project-->>Manager: Project
  else check manifest repos
    Manager->>Project: enumerate manifests
    Project-->>Manager: Project
  end
  Manager-->>CLI: *Project (autodetected) or error
Loading

Notes:

  • New interactions include FS and Git via injected interfaces; Manager coordinates detection and returns structured results to CLI.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • fix: improve git sync #17 — Changes related to internal/git and internal/manager detection logic; likely overlaps in git normalization and manager detection behavior.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 19.44% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: service path detection' directly summarizes the main change—fixing autodetection logic for service paths. It aligns with the core refactoring work in manager.go and reflects the PR's primary objective.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@pilat pilat self-assigned this Dec 26, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
internal/git/git.go (1)

28-33: Consider returning the interface type for better abstraction.

New() returns *svc (concrete type). For consistency with dependency injection patterns, consider returning Service instead. This makes it explicit that callers should program to the interface.

-func New(targetFolder string) *svc {
+func New(targetFolder string) Service {

This is optional since callers can still assign to a Service variable.

cmd/devbox/ps.go (1)

56-63: Missing return after sending to error channel causes goroutine leak.

After sending to errCh on lines 58 and 62, the goroutine continues its infinite loop. This causes:

  1. Potential deadlock on second send to unbuffered channel
  2. Goroutine leak since loop never terminates on error/empty case
🔎 Proposed fix
 			containers, err := apiService.Ps(ctx, p.Name, opts)
 			if err != nil {
 				errCh <- fmt.Errorf("failed to list services: %w", err)
+				return
 			}

 			if len(containers) == 0 {
 				errCh <- nil
+				return
 			}
cmd/devbox/devbox.go (1)

69-83: Minor: Project loaded but discarded in wrapper.

The AutodetectProject result is discarded on line 73. The wrapper validates detectability while commands reload the project in RunE. This is acceptable but causes duplicate work.

If this becomes a performance concern, consider passing the detected project through context or a closure.

cmd/devbox/umount.go (1)

79-84: Same copy-paste error in runUmount.

 func runUmount(ctx context.Context, p *project.Project, sources []string) error {
 	err := p.Umount(ctx, sources)
 	if err != nil {
-		return fmt.Errorf("failed to mount source code: %w", err)
+		return fmt.Errorf("failed to unmount source code: %w", err)
 	}
 
 	return nil
 }
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bae7621 and 1c47353.

⛔ Files ignored due to path filters (2)
  • internal/git/git_test.go is excluded by !**/*_test.go
  • internal/manager/manager_test.go is excluded by !**/*_test.go
📒 Files selected for processing (27)
  • .mockery.yaml
  • cmd/devbox/destroy.go
  • cmd/devbox/devbox.go
  • cmd/devbox/down.go
  • cmd/devbox/env.go
  • cmd/devbox/info.go
  • cmd/devbox/init.go
  • cmd/devbox/install_ca.go
  • cmd/devbox/list.go
  • cmd/devbox/logs.go
  • cmd/devbox/mount.go
  • cmd/devbox/ps.go
  • cmd/devbox/restart.go
  • cmd/devbox/run.go
  • cmd/devbox/shell.go
  • cmd/devbox/umount.go
  • cmd/devbox/up.go
  • cmd/devbox/update.go
  • cmd/devbox/update_hosts.go
  • internal/git/git.go
  • internal/git/mock_CommandRunner.go
  • internal/git/mock_Service.go
  • internal/manager/manager.go
  • internal/pkg/fs/fs.go
  • internal/pkg/fs/mock_DirEntry.go
  • internal/pkg/fs/mock_FileInfo.go
  • internal/pkg/fs/mock_FileSystem.go
🧰 Additional context used
📓 Path-based instructions (3)
internal/**/*.go

⚙️ CodeRabbit configuration file

internal/**/*.go: Core packages providing project functionality:

  • project/: Project configuration, Docker Compose extensions (x-devbox-*)
  • manager/: Project/service autodetection from current directory
  • git/: Git operations (clone, sparse checkout, sync)
  • cert/: SSL certificate generation
  • hosts/: /etc/hosts management with project-scoped markers
  • table/: CLI table output formatting

Review for:

  • Clean interfaces and proper encapsulation
  • Error wrapping with context
  • No interface{} - use 'any' instead (enforced by linter)
  • YAGNI principle - no speculative features or premature abstractions
  • Security considerations for host file and certificate operations

Files:

  • internal/pkg/fs/mock_FileInfo.go
  • internal/git/mock_CommandRunner.go
  • internal/git/git.go
  • internal/git/mock_Service.go
  • internal/pkg/fs/mock_DirEntry.go
  • internal/manager/manager.go
  • internal/pkg/fs/fs.go
  • internal/pkg/fs/mock_FileSystem.go
cmd/devbox/**/*.go

⚙️ CodeRabbit configuration file

cmd/devbox/**/*.go: This directory contains CLI commands using Cobra framework.
Each command is in a separate file with an init() function that registers via root.AddCommand().

Review for:

  • Proper error handling with context wrapping (fmt.Errorf with %w)
  • Consistent command structure following existing patterns
  • Appropriate use of runWrapper for context handling
  • Clear, concise command descriptions
  • No over-engineering - only implement what's needed

Files:

  • cmd/devbox/update_hosts.go
  • cmd/devbox/destroy.go
  • cmd/devbox/down.go
  • cmd/devbox/restart.go
  • cmd/devbox/list.go
  • cmd/devbox/up.go
  • cmd/devbox/info.go
  • cmd/devbox/init.go
  • cmd/devbox/umount.go
  • cmd/devbox/env.go
  • cmd/devbox/install_ca.go
  • cmd/devbox/logs.go
  • cmd/devbox/update.go
  • cmd/devbox/devbox.go
  • cmd/devbox/run.go
  • cmd/devbox/ps.go
  • cmd/devbox/mount.go
  • cmd/devbox/shell.go
internal/manager/**/*.go

⚙️ CodeRabbit configuration file

internal/manager/**/*.go: Project autodetection with three-step process:

  1. Check if directory is a local mount of any project
  2. Match Git remote URL + path against project source definitions
  3. Check if directory is the project's manifest repository

Review for edge cases in path matching and Git remote detection.

Files:

  • internal/manager/manager.go
🧬 Code graph analysis (10)
cmd/devbox/destroy.go (1)
internal/project/project.go (2)
  • p (108-118)
  • Project (24-35)
cmd/devbox/list.go (3)
internal/git/git.go (1)
  • New (28-33)
internal/manager/manager.go (1)
  • New (43-53)
internal/project/project.go (4)
  • New (50-107)
  • p (108-118)
  • Project (24-35)
  • p (139-148)
internal/git/git.go (2)
internal/git/structs.go (1)
  • CommitInfo (3-8)
internal/project/config.go (1)
  • Service (12-21)
internal/git/mock_Service.go (1)
internal/git/structs.go (1)
  • CommitInfo (3-8)
cmd/devbox/devbox.go (1)
internal/manager/manager.go (2)
  • Manager (35-40)
  • New (43-53)
internal/pkg/fs/mock_DirEntry.go (1)
internal/pkg/fs/fs.go (1)
  • FileInfo (18-25)
internal/manager/manager.go (5)
internal/git/git.go (2)
  • Service (11-19)
  • New (28-33)
internal/pkg/fs/fs.go (2)
  • FileSystem (9-15)
  • New (51-53)
internal/project/project.go (2)
  • Project (24-35)
  • New (50-107)
internal/git/normalizer.go (1)
  • NormalizeURL (21-60)
internal/app/const.go (4)
  • SourcesDir (13-13)
  • AppDir (9-9)
  • EnvFile (15-15)
  • StateFile (14-14)
internal/pkg/fs/fs.go (1)
internal/manager/manager.go (1)
  • New (43-53)
cmd/devbox/mount.go (1)
internal/manager/manager.go (1)
  • AutodetectSourceForMount (22-22)
internal/pkg/fs/mock_FileSystem.go (1)
internal/pkg/fs/fs.go (2)
  • DirEntry (28-33)
  • FileInfo (18-25)
🔇 Additional comments (34)
internal/git/mock_CommandRunner.go (1)

1-1: Auto-generated mock file updated with new mockery version.

No issues here—this is a standard regeneration after updating the mockery tool to v2.53.5.

cmd/devbox/run.go (2)

21-24: Context-aware autodetection in ValidArgsFunction.

Good—context propagation now covers the shell completion path as well, allowing cancellation during tab completion scenarios.


34-37: Consistent context propagation in RunE.

Same pattern applied correctly to the command execution path.

cmd/devbox/info.go (1)

26-29: LGTM.

Clean migration to the context-aware AutodetectProject API.

cmd/devbox/env.go (1)

29-32: LGTM.

Context is properly passed. The error wrapping with "failed to detect project" provides good diagnostic context.

.mockery.yaml (1)

14-19: Mock generation config expanded for new abstractions.

Good additions for testability. The Service interface in git and the filesystem abstractions (FileSystem, FileInfo, DirEntry) enable proper unit testing with dependency injection.

cmd/devbox/update_hosts.go (1)

21-24: LGTM.

Context-aware autodetection applied consistently to this hidden command.

cmd/devbox/down.go (1)

20-23: LGTM.

Context propagation enables proper cancellation through the down operation chain.

cmd/devbox/logs.go (1)

21-24: LGTM!

Context propagation to AutodetectProject is correctly implemented. The error handling remains proper with early returns.

Also applies to: 34-37

internal/git/git.go (1)

11-21: Well-structured interface for testability.

The Service interface correctly abstracts git operations. The compile-time assertion (var _ Service = (*svc)(nil)) is idiomatic Go for ensuring interface compliance.

cmd/devbox/restart.go (1)

23-26: LGTM!

Context propagation is correctly applied. Using context.Background() in the flag completion callback (line 59) is appropriate since there's no parent context available in that callback signature.

Also applies to: 59-62, 71-74

cmd/devbox/ps.go (1)

22-25: LGTM!

Context propagation is correct.

cmd/devbox/destroy.go (1)

20-23: LGTM!

Context propagation to both AutodetectProject and Destroy is correctly implemented. Error wrapping follows proper Go patterns.

Also applies to: 44-46

cmd/devbox/devbox.go (1)

23-23: LGTM!

The mgr package-level variable is properly initialized in initDocker() before initCobra() calls root.Execute(), ensuring it's available for flag completions and command execution.

Also applies to: 64-65

cmd/devbox/shell.go (1)

32-35: LGTM!

Context propagation is correctly implemented.

cmd/devbox/up.go (1)

25-28: LGTM!

Context propagation is correctly applied across all AutodetectProject call sites. Using context.Background() in the flag completion callback (line 80) is appropriate given the callback signature limitations.

Also applies to: 38-41, 80-83

cmd/devbox/init.go (1)

47-51: Context propagation looks good.

The signature change to accept context.Context and its propagation to mgr.Init aligns with the broader refactor for context-aware operations.

cmd/devbox/list.go (1)

36-52: Clean migration to mgr-based API.

The refactor to use mgr.List() and mgr.Load() is consistent with the new Manager abstraction. Using git.New() directly for read-only git info retrieval is acceptable here.

cmd/devbox/install_ca.go (1)

18-22: Context propagation is correct.

The migration to mgr.AutodetectProject(ctx, projectName) is consistent with the refactored API.

internal/pkg/fs/fs.go (2)

9-15: Clean FileSystem abstraction.

The interface is minimal and covers the essential operations needed for the manager package. The compile-time assertion on line 47 is a good practice.


63-73: ReadDir wrapper correctly adapts os.DirEntry to the custom interface.

The conversion loop properly wraps each os.DirEntry into osDirEntry to satisfy the []DirEntry return type.

internal/pkg/fs/mock_FileInfo.go (1)

1-310: Autogenerated mock - no manual changes needed.

This is mockery-generated code following standard testify/mock patterns. Ensure this file is regenerated if the FileInfo interface changes.

cmd/devbox/umount.go (1)

21-30: Context-aware API migration looks correct.

The switch to mgr.AutodetectProject(ctx, ...) and mgr.AutodetectSource(ctx, ...) with result struct unpacking is consistent. Using context.Background() in the flag completion callback (line 68) is reasonable since completions don't benefit from cancellation.

Also applies to: 39-47, 68-73

cmd/devbox/mount.go (2)

50-55: Clear mount path resolution logic.

The fallback from explicit targetPath to result.LocalPath is straightforward and the intent is clear.


22-31: Context-aware API migration is consistent.

The migration to mgr.AutodetectProject and mgr.AutodetectSource with result struct unpacking follows the established pattern. Shell completion uses context.Background() appropriately.

Also applies to: 40-48, 75-80

internal/git/mock_Service.go (1)

1-395: Autogenerated mock - no manual changes needed.

This is mockery-generated code for the Service interface. Ensure regeneration if the interface changes.

internal/pkg/fs/mock_FileSystem.go (1)

1-301: Autogenerated mock — looks good.

Standard mockery-generated mock implementing the FileSystem interface. The generated code correctly handles all interface methods with proper type assertions and panic guards for missing return values.

internal/pkg/fs/mock_DirEntry.go (1)

1-228: Autogenerated mock — looks good.

Standard mockery-generated mock for DirEntry interface. Correctly uses iofs alias for io/fs to avoid package name collision with the local fs package.

internal/manager/manager.go (6)

35-53: Clean dependency injection pattern.

The Manager struct with injectable gitFactory, fs, listFn, and loadFn fields enables unit testing without mocking global state. The self-referential function fields (m.listFn = m.list) is a reasonable pattern for swapping implementations in tests.


482-525: Good defensive coding in Init.

Proper input validation with validNameRegex prevents path traversal. The cleanup function ensures partial state is removed on failure. The _ = m.fs.RemoveAll() pattern is appropriate for best-effort cleanup where failure shouldn't mask the original error.


428-445: Correct path prefix matching.

Using component-based comparison (strings.Split on separator) rather than string prefix is the right approach — prevents false positives like ./sources/foo matching ./sources/foobar. Edge cases handled properly.


55-112: Well-structured three-step autodetection.

The detection follows the documented process (local mount → git remote → manifest repo) with proper ambiguity handling. Error messages are descriptive and help users understand why detection failed.


270-301: Potential issue: localPath takes last match value.

When multiple services use the same source, localPath is reassigned in each iteration (lines 286, 296). The final value depends on map iteration order, which is non-deterministic. If all matching services should yield the same localPath, this is fine. Otherwise, consider verifying consistency or documenting the expected behavior.


598-605: Clean utility function with sorted output.

mapKeys correctly provides deterministic ordering. Consider reusing this in GetLocalMountCandidates and GetLocalMounts for consistency.

- Fix misleading error messages in init.go and umount.go
- Add missing returns after error channel send in ps.go (goroutine leak)
- Wrap error with context in update.go for consistency
- Use app.SourcesDir constant instead of hardcoded string
- Add sorting for deterministic output in GetLocalMountCandidates/GetLocalMounts
- Return interface type from git.New() for better abstraction
- Add comment about structural typing in fs.go

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
internal/git/git.go (2)

128-132: Error chain lost - missing %w verb.

GetInfo returns an error without wrapping the original err. This breaks error unwrapping for callers.

Suggested fix
 func (s *svc) GetInfo(ctx context.Context) (*CommitInfo, error) {
 	out, err := s.runner.Run(ctx, "git", "-C", s.targetPath, "log", "-1", "--pretty=format:%H%n%aN%n%ad%n%s")
 	if err != nil {
-		return nil, fmt.Errorf("failed to get commit info: %s", out)
+		return nil, fmt.Errorf("failed to get commit info: %s: %w", out, err)
 	}

91-109: Inconsistent error format string style.

These errors use %s %w (space before %w) while others like line 43, 80, 86 use %s\n%w or proper colon separation. Pick one style for consistency. Typically ": %w" is idiomatic.

Example normalization
-		return fmt.Errorf("failed to init sparse-checkout: %s %w", out, err)
+		return fmt.Errorf("failed to init sparse-checkout: %s: %w", out, err)
♻️ Duplicate comments (1)
internal/manager/manager.go (1)

455-459: Silent error swallowing persists - consider propagating or logging.

ReadDir errors are still silently ignored, returning an empty slice. This hides potential issues (permission denied, missing directory). If changing the signature is difficult, at minimum log the error for debugging.

Suggested fix - return error
-func (m *Manager) list(filter string) []string {
+func (m *Manager) list(filter string) ([]string, error) {
 	folders, err := m.fs.ReadDir(app.AppDir)
 	if err != nil {
-		return []string{}
+		return nil, fmt.Errorf("failed to read app directory: %w", err)
 	}

This would require updating listFn type and callers, but provides better error visibility.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1c47353 and afd019a.

📒 Files selected for processing (7)
  • cmd/devbox/init.go
  • cmd/devbox/ps.go
  • cmd/devbox/umount.go
  • cmd/devbox/update.go
  • internal/git/git.go
  • internal/manager/manager.go
  • internal/pkg/fs/fs.go
🧰 Additional context used
📓 Path-based instructions (3)
internal/**/*.go

⚙️ CodeRabbit configuration file

internal/**/*.go: Core packages providing project functionality:

  • project/: Project configuration, Docker Compose extensions (x-devbox-*)
  • manager/: Project/service autodetection from current directory
  • git/: Git operations (clone, sparse checkout, sync)
  • cert/: SSL certificate generation
  • hosts/: /etc/hosts management with project-scoped markers
  • table/: CLI table output formatting

Review for:

  • Clean interfaces and proper encapsulation
  • Error wrapping with context
  • No interface{} - use 'any' instead (enforced by linter)
  • YAGNI principle - no speculative features or premature abstractions
  • Security considerations for host file and certificate operations

Files:

  • internal/git/git.go
  • internal/pkg/fs/fs.go
  • internal/manager/manager.go
cmd/devbox/**/*.go

⚙️ CodeRabbit configuration file

cmd/devbox/**/*.go: This directory contains CLI commands using Cobra framework.
Each command is in a separate file with an init() function that registers via root.AddCommand().

Review for:

  • Proper error handling with context wrapping (fmt.Errorf with %w)
  • Consistent command structure following existing patterns
  • Appropriate use of runWrapper for context handling
  • Clear, concise command descriptions
  • No over-engineering - only implement what's needed

Files:

  • cmd/devbox/init.go
  • cmd/devbox/ps.go
  • cmd/devbox/umount.go
  • cmd/devbox/update.go
internal/manager/**/*.go

⚙️ CodeRabbit configuration file

internal/manager/**/*.go: Project autodetection with three-step process:

  1. Check if directory is a local mount of any project
  2. Match Git remote URL + path against project source definitions
  3. Check if directory is the project's manifest repository

Review for edge cases in path matching and Git remote detection.

Files:

  • internal/manager/manager.go
🧬 Code graph analysis (4)
internal/git/git.go (3)
internal/git/structs.go (1)
  • CommitInfo (3-8)
internal/git/exec.go (1)
  • CommandRunner (10-13)
internal/manager/manager.go (1)
  • New (43-53)
internal/pkg/fs/fs.go (2)
internal/git/git.go (1)
  • New (28-33)
internal/manager/manager.go (1)
  • New (43-53)
cmd/devbox/umount.go (1)
internal/manager/manager.go (1)
  • AutodetectSourceForUmount (23-23)
internal/manager/manager.go (5)
internal/git/git.go (2)
  • Service (11-19)
  • New (28-33)
internal/pkg/fs/fs.go (2)
  • FileSystem (9-15)
  • New (52-54)
internal/project/project.go (2)
  • Project (24-35)
  • New (50-107)
internal/git/normalizer.go (1)
  • NormalizeURL (21-60)
internal/app/const.go (4)
  • SourcesDir (13-13)
  • AppDir (9-9)
  • EnvFile (15-15)
  • StateFile (14-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: E2E Tests
🔇 Additional comments (21)
internal/git/git.go (2)

11-22: Interface and compile-time check look good.

Clean interface definition with appropriate method signatures. The var _ Service = (*svc)(nil) compile-time assertion is idiomatic Go for ensuring interface compliance.


28-33: Returning interface from constructor is correct for this use case.

Returning Service from New() enables the dependency injection pattern used by Manager. This aligns with the gitFactory field in internal/manager/manager.go (lines 42-52).

cmd/devbox/ps.go (2)

22-25: Context propagation to AutodetectProject is correct.

The call now properly passes ctx to enable cancellation and timeout propagation through the autodetection flow.


56-65: Good fix - early returns prevent goroutine leaks.

The added return statements after sending to errCh ensure the goroutine exits instead of continuing the infinite loop after an error or empty result. This prevents resource leaks.

cmd/devbox/umount.go (3)

21-30: Context-aware autodetection in completion handler looks good.

Properly propagates context to AutodetectProject and uses the new AutodetectSource returning *SourceDetectionResult.


44-55: Error message correctly updated and result struct usage is proper.

The error message now says "unmount" (previously flagged). Using result.Sources and result.AffectedServices from the new SourceDetectionResult struct is correct.


68-73: Completion function uses context.Background() - acceptable for shell completions.

Shell completion functions often can't access a request-scoped context, so context.Background() is a reasonable fallback here.

cmd/devbox/update.go (1)

33-36: Error wrapping now consistent with the rest of the function.

The autodetect error is properly wrapped with context, matching the pattern used elsewhere in this handler (lines 39-54).

cmd/devbox/init.go (2)

33-35: Error message corrected.

Changed from "failed to list projects" to "failed to initialize project" - now accurately describes the operation.


47-51: Context propagation through runInit is correct.

The function signature properly accepts context.Context and passes it to mgr.Init. Error wrapping is appropriate.

internal/pkg/fs/fs.go (3)

9-15: Clean FileSystem interface for dependency injection.

The interface surface is minimal and appropriate for the manager's needs. Good adherence to interface segregation.


43-46: Structural typing comment added as previously suggested.

The comment clarifies the implicit interface satisfaction between os.FileInfo and the custom FileInfo interface.


48-54: Compile-time check and constructor are idiomatic.

var _ FileSystem = (*OSFileSystem)(nil) ensures interface compliance at compile time. Constructor returns concrete type, allowing flexibility for callers.

internal/manager/manager.go (8)

26-31: SourceDetectionResult struct is well-documented.

Clear field names with comments explaining each field's purpose. Good practice for exported types.


42-53: Manager constructor properly initializes dependencies.

Default implementations are injected for gitFactory and fs, with internal function pointers (listFn, loadFn) enabling test overrides. This is a clean pattern for testability.


55-112: AutodetectProject implements a clear three-step detection strategy.

The detection order (local mount → source remote → project repo) is logical. Each step checks for ambiguity and returns appropriate errors. Context is properly propagated.


206-209: Hardcoded path replaced with app.SourcesDir constant.

Now uses "./"+app.SourcesDir+"/" instead of hardcoded "./sources/", addressing the previous review feedback.


582-583: Sorting added for deterministic output - good fix.

Both GetLocalMountCandidates and GetLocalMounts now call sort.Strings() before returning, ensuring consistent CLI output and stable test results.

Also applies to: 596-597


600-607: mapKeys helper includes sorting - deterministic output.

The helper properly sorts results before returning, which ensures all callers get consistent ordering.


420-425: cwdMatchesServiceSubpath handles edge cases correctly.

Properly handles empty serviceSubpath (root case) by checking for "" or "." in cwdRelPath. The pathStartsWith delegation for non-empty subpaths is appropriate.


428-445: pathStartsWith uses proper path component matching.

Uses filepath.Clean and splits by separator to avoid false positives from string prefix matching (e.g., foo/bar wouldn't incorrectly match foo/barbaz).

Changed list() to return ([]string, error) to properly propagate
ReadDir errors instead of silently returning an empty slice.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@pilat pilat force-pushed the fix/service-path-detection branch from 9e146af to 00705c9 Compare December 26, 2025 05:04
@pilat pilat merged commit aa374ce into main Dec 26, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants