From 3ea0e2b224db6b727c4755d055653701d4daca2f Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Fri, 6 Mar 2026 14:11:18 +0100 Subject: [PATCH 1/5] feat: init command Signed-off-by: Philippe Martin Co-Authored-By: Claude Code (Claude Sonnet 4.5) --- AGENTS.md | 124 +++++++++ pkg/cmd/init.go | 119 +++++++++ pkg/cmd/init_test.go | 606 +++++++++++++++++++++++++++++++++++++++++++ pkg/cmd/root.go | 1 + 4 files changed, 850 insertions(+) create mode 100644 pkg/cmd/init.go create mode 100644 pkg/cmd/init_test.go diff --git a/AGENTS.md b/AGENTS.md index 6cd147b..5f9d09b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -170,6 +170,130 @@ func NewExampleCmd() *cobra.Command { rootCmd.AddCommand(NewExampleCmd()) ``` +### Command Implementation Pattern + +Commands should follow a consistent structure for maintainability and testability: + +1. **Command Struct** - Contains all command state: + - Input values from flags/args + - Computed/validated values + - Dependencies (e.g., manager instances) + +2. **preRun Method** - Validates parameters and prepares: + - Parse and validate arguments/flags + - Access global flags (e.g., `--storage`) + - Create dependencies (managers, etc.) + - Convert paths to absolute using `filepath.Abs()` + - Store validated values in struct fields + +3. **run Method** - Executes the command logic: + - Use validated values from struct fields + - Perform the actual operation + - Output results to user + +**Reference:** See `pkg/cmd/init.go` for a complete implementation of this pattern. + +### Testing Pattern for Commands + +Commands should have two types of tests following the pattern in `pkg/instances/instance_test.go`: + +1. **Unit Tests** - Test the `preRun` method directly: + - Use `t.Run()` for subtests within a parent test function + - Test with different argument/flag combinations + - Verify struct fields are set correctly + - Use `t.TempDir()` for temporary directories (automatic cleanup) + +2. **E2E Tests** - Test the full command execution: + - Execute via `rootCmd.Execute()` + - Use real temp directories with `t.TempDir()` + - Verify output messages + - Verify persistence (check storage/database) + - Verify all field values from `manager.List()` or similar + - Test multiple scenarios (default args, custom args, edge cases) + +**Reference:** See `pkg/cmd/init_test.go` for complete examples of both unit and E2E tests. + +### Working with the Instances Manager + +When commands need to interact with workspaces: + +```go +// In preRun - create manager from storage flag +storageDir, _ := cmd.Flags().GetString("storage") +manager, err := instances.NewManager(storageDir) +if err != nil { + return fmt.Errorf("failed to create manager: %w", err) +} + +// In run - use manager to add instances +instance, err := instances.NewInstance(sourceDir, configDir) +if err != nil { + return fmt.Errorf("failed to create instance: %w", err) +} + +addedInstance, err := manager.Add(instance) +if err != nil { + return fmt.Errorf("failed to add instance: %w", err) +} + +// List instances +instancesList, err := manager.List() +if err != nil { + return fmt.Errorf("failed to list instances: %w", err) +} + +// Get specific instance +instance, err := manager.Get(id) +if err != nil { + return fmt.Errorf("instance not found: %w", err) +} + +// Delete instance +err := manager.Delete(id) +if err != nil { + return fmt.Errorf("failed to delete instance: %w", err) +} +``` + +### Cross-Platform Path Handling + +**IMPORTANT**: All path operations must be cross-platform compatible (Linux, macOS, Windows). + +**Rules:** +- Always use `filepath.Join()` for path construction (never hardcode "/" or "\\") +- Convert relative paths to absolute with `filepath.Abs()` +- Never hardcode paths with `~` - use `os.UserHomeDir()` instead +- In tests, use `filepath.Join()` for all path assertions +- Use `t.TempDir()` for temporary directories in tests + +**Examples:** + +```go +// GOOD: Cross-platform path construction +configDir := filepath.Join(sourceDir, ".kortex") +absPath, err := filepath.Abs(relativePath) + +// BAD: Hardcoded separator +configDir := sourceDir + "/.kortex" // Don't do this! + +// GOOD: User home directory +homeDir, err := os.UserHomeDir() +defaultPath := filepath.Join(homeDir, ".kortex-cli") + +// BAD: Hardcoded tilde +defaultPath := "~/.kortex-cli" // Don't do this! + +// GOOD: Test assertions +expectedPath := filepath.Join(".", "relative", "path") +if result != expectedPath { + t.Errorf("Expected %s, got %s", expectedPath, result) +} + +// GOOD: Temporary directories in tests +tempDir := t.TempDir() // Automatic cleanup +sourcesDir := t.TempDir() +``` + ## Copyright Headers All source files must include Apache License 2.0 copyright headers with Red Hat copyright. Use the `/copyright-headers` skill to add or update headers automatically. The current year is 2026. diff --git a/pkg/cmd/init.go b/pkg/cmd/init.go new file mode 100644 index 0000000..66ee63e --- /dev/null +++ b/pkg/cmd/init.go @@ -0,0 +1,119 @@ +/********************************************************************** + * Copyright (C) 2026 Red Hat, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * SPDX-License-Identifier: Apache-2.0 + **********************************************************************/ + +package cmd + +import ( + "fmt" + "path/filepath" + + "github.com/kortex-hub/kortex-cli/pkg/instances" + "github.com/spf13/cobra" +) + +// InitCmd contains the configuration for the init command +type InitCmd struct { + sourcesDir string + workspaceConfigDir string + absSourcesDir string + absConfigDir string + manager instances.Manager +} + +// preRun validates the parameters and flags +func (i *InitCmd) preRun(cmd *cobra.Command, args []string) error { + // Get storage directory from global flag + storageDir, _ := cmd.Flags().GetString("storage") + + // Create manager + manager, err := instances.NewManager(storageDir) + if err != nil { + return fmt.Errorf("failed to create manager: %w", err) + } + i.manager = manager + + // Get sources directory (default to current directory) + i.sourcesDir = "." + if len(args) > 0 { + i.sourcesDir = args[0] + } + + // Convert to absolute path for clarity + absSourcesDir, err := filepath.Abs(i.sourcesDir) + if err != nil { + return fmt.Errorf("failed to resolve sources directory path: %w", err) + } + i.absSourcesDir = absSourcesDir + + // If workspace-configuration flag was not explicitly set, default to .kortex/ inside sources directory + if !cmd.Flags().Changed("workspace-configuration") { + i.workspaceConfigDir = filepath.Join(i.sourcesDir, ".kortex") + } + + // Convert workspace config to absolute path + absConfigDir, err := filepath.Abs(i.workspaceConfigDir) + if err != nil { + return fmt.Errorf("failed to resolve workspace configuration directory path: %w", err) + } + i.absConfigDir = absConfigDir + + return nil +} + +// run executes the init command logic +func (i *InitCmd) run(cmd *cobra.Command, args []string) error { + // Create a new instance + instance, err := instances.NewInstance(i.absSourcesDir, i.absConfigDir) + if err != nil { + return fmt.Errorf("failed to create instance: %w", err) + } + + // Add the instance to the manager + addedInstance, err := i.manager.Add(instance) + if err != nil { + return fmt.Errorf("failed to add instance: %w", err) + } + + cmd.Printf("Registered workspace:\n") + cmd.Printf(" ID: %s\n", addedInstance.GetID()) + cmd.Printf(" Sources directory: %s\n", addedInstance.GetSourceDir()) + cmd.Printf(" Configuration directory: %s\n", addedInstance.GetConfigDir()) + + return nil +} + +func NewInitCmd() *cobra.Command { + initCmd := &InitCmd{} + + cmd := &cobra.Command{ + Use: "init [sources-directory]", + Short: "Register a new workspace", + Long: `Register a new workspace with the specified sources directory and configuration location. + +The sources directory defaults to the current directory (.) if not specified. +The workspace configuration directory defaults to .kortex/ inside the sources directory if not specified.`, + Args: cobra.MaximumNArgs(1), + PreRunE: initCmd.preRun, + RunE: initCmd.run, + } + + // Add workspace-configuration flag + cmd.Flags().StringVar(&initCmd.workspaceConfigDir, "workspace-configuration", "", "Directory for workspace configuration (default: /.kortex)") + + return cmd +} diff --git a/pkg/cmd/init_test.go b/pkg/cmd/init_test.go new file mode 100644 index 0000000..2b6d40b --- /dev/null +++ b/pkg/cmd/init_test.go @@ -0,0 +1,606 @@ +/********************************************************************** + * Copyright (C) 2026 Red Hat, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * SPDX-License-Identifier: Apache-2.0 + **********************************************************************/ + +package cmd + +import ( + "bytes" + "path/filepath" + "strings" + "testing" + + "github.com/kortex-hub/kortex-cli/pkg/instances" + "github.com/spf13/cobra" +) + +func TestInitCmd_PreRun(t *testing.T) { + t.Parallel() + + t.Run("default arguments", func(t *testing.T) { + t.Parallel() + + tempDir := t.TempDir() + + initCmd := &InitCmd{} + cmd := &cobra.Command{} + cmd.Flags().String("workspace-configuration", "", "test flag") + cmd.Flags().String("storage", tempDir, "test storage flag") + + args := []string{} + + err := initCmd.preRun(cmd, args) + if err != nil { + t.Fatalf("preRun() failed: %v", err) + } + + if initCmd.manager == nil { + t.Error("Expected manager to be created") + } + + if initCmd.sourcesDir != "." { + t.Errorf("Expected sourcesDir to be '.', got %s", initCmd.sourcesDir) + } + + expectedAbsSourcesDir, _ := filepath.Abs(".") + if initCmd.absSourcesDir != expectedAbsSourcesDir { + t.Errorf("Expected absSourcesDir to be %s, got %s", expectedAbsSourcesDir, initCmd.absSourcesDir) + } + + expectedConfigDir := filepath.Join(".", ".kortex") + if initCmd.workspaceConfigDir != expectedConfigDir { + t.Errorf("Expected workspaceConfigDir to be %s, got %s", expectedConfigDir, initCmd.workspaceConfigDir) + } + + expectedAbsConfigDir, _ := filepath.Abs(expectedConfigDir) + if initCmd.absConfigDir != expectedAbsConfigDir { + t.Errorf("Expected absConfigDir to be %s, got %s", expectedAbsConfigDir, initCmd.absConfigDir) + } + }) + + t.Run("with sources directory", func(t *testing.T) { + t.Parallel() + + tempDir := t.TempDir() + sourcesDir := t.TempDir() + + initCmd := &InitCmd{} + cmd := &cobra.Command{} + cmd.Flags().String("workspace-configuration", "", "test flag") + cmd.Flags().String("storage", tempDir, "test storage flag") + + args := []string{sourcesDir} + + err := initCmd.preRun(cmd, args) + if err != nil { + t.Fatalf("preRun() failed: %v", err) + } + + if initCmd.manager == nil { + t.Error("Expected manager to be created") + } + + if initCmd.sourcesDir != sourcesDir { + t.Errorf("Expected sourcesDir to be %s, got %s", sourcesDir, initCmd.sourcesDir) + } + + expectedAbsSourcesDir, _ := filepath.Abs(sourcesDir) + if initCmd.absSourcesDir != expectedAbsSourcesDir { + t.Errorf("Expected absSourcesDir to be %s, got %s", expectedAbsSourcesDir, initCmd.absSourcesDir) + } + + expectedConfigDir := filepath.Join(sourcesDir, ".kortex") + if initCmd.workspaceConfigDir != expectedConfigDir { + t.Errorf("Expected workspaceConfigDir to be %s, got %s", expectedConfigDir, initCmd.workspaceConfigDir) + } + + expectedAbsConfigDir, _ := filepath.Abs(expectedConfigDir) + if initCmd.absConfigDir != expectedAbsConfigDir { + t.Errorf("Expected absConfigDir to be %s, got %s", expectedAbsConfigDir, initCmd.absConfigDir) + } + }) + + t.Run("with workspace configuration flag", func(t *testing.T) { + t.Parallel() + + tempDir := t.TempDir() + configDir := t.TempDir() + + initCmd := &InitCmd{ + workspaceConfigDir: configDir, + } + cmd := &cobra.Command{} + cmd.Flags().String("workspace-configuration", "", "test flag") + cmd.Flags().Set("workspace-configuration", configDir) + cmd.Flags().String("storage", tempDir, "test storage flag") + + args := []string{} + + err := initCmd.preRun(cmd, args) + if err != nil { + t.Fatalf("preRun() failed: %v", err) + } + + if initCmd.manager == nil { + t.Error("Expected manager to be created") + } + + if initCmd.sourcesDir != "." { + t.Errorf("Expected sourcesDir to be '.', got %s", initCmd.sourcesDir) + } + + if initCmd.workspaceConfigDir != configDir { + t.Errorf("Expected workspaceConfigDir to be %s, got %s", configDir, initCmd.workspaceConfigDir) + } + + expectedAbsConfigDir, _ := filepath.Abs(configDir) + if initCmd.absConfigDir != expectedAbsConfigDir { + t.Errorf("Expected absConfigDir to be %s, got %s", expectedAbsConfigDir, initCmd.absConfigDir) + } + }) + + t.Run("with both arguments", func(t *testing.T) { + t.Parallel() + + tempDir := t.TempDir() + sourcesDir := t.TempDir() + configDir := t.TempDir() + + initCmd := &InitCmd{ + workspaceConfigDir: configDir, + } + cmd := &cobra.Command{} + cmd.Flags().String("workspace-configuration", "", "test flag") + cmd.Flags().Set("workspace-configuration", configDir) + cmd.Flags().String("storage", tempDir, "test storage flag") + + args := []string{sourcesDir} + + err := initCmd.preRun(cmd, args) + if err != nil { + t.Fatalf("preRun() failed: %v", err) + } + + if initCmd.manager == nil { + t.Error("Expected manager to be created") + } + + if initCmd.sourcesDir != sourcesDir { + t.Errorf("Expected sourcesDir to be %s, got %s", sourcesDir, initCmd.sourcesDir) + } + + expectedAbsSourcesDir, _ := filepath.Abs(sourcesDir) + if initCmd.absSourcesDir != expectedAbsSourcesDir { + t.Errorf("Expected absSourcesDir to be %s, got %s", expectedAbsSourcesDir, initCmd.absSourcesDir) + } + + if initCmd.workspaceConfigDir != configDir { + t.Errorf("Expected workspaceConfigDir to be %s, got %s", configDir, initCmd.workspaceConfigDir) + } + + expectedAbsConfigDir, _ := filepath.Abs(configDir) + if initCmd.absConfigDir != expectedAbsConfigDir { + t.Errorf("Expected absConfigDir to be %s, got %s", expectedAbsConfigDir, initCmd.absConfigDir) + } + }) + + t.Run("relative sources directory", func(t *testing.T) { + t.Parallel() + + tempDir := t.TempDir() + relativePath := filepath.Join(".", "relative", "path") + + initCmd := &InitCmd{} + cmd := &cobra.Command{} + cmd.Flags().String("workspace-configuration", "", "test flag") + cmd.Flags().String("storage", tempDir, "test storage flag") + + args := []string{relativePath} + + err := initCmd.preRun(cmd, args) + if err != nil { + t.Fatalf("preRun() failed: %v", err) + } + + if initCmd.manager == nil { + t.Error("Expected manager to be created") + } + + if initCmd.sourcesDir != relativePath { + t.Errorf("Expected sourcesDir to be %s, got %s", relativePath, initCmd.sourcesDir) + } + + expectedAbsSourcesDir, _ := filepath.Abs(relativePath) + if initCmd.absSourcesDir != expectedAbsSourcesDir { + t.Errorf("Expected absSourcesDir to be %s, got %s", expectedAbsSourcesDir, initCmd.absSourcesDir) + } + + expectedConfigDir := filepath.Join(relativePath, ".kortex") + if initCmd.workspaceConfigDir != expectedConfigDir { + t.Errorf("Expected workspaceConfigDir to be %s, got %s", expectedConfigDir, initCmd.workspaceConfigDir) + } + }) +} + +func TestInitCmd_E2E(t *testing.T) { + t.Parallel() + + t.Run("registers workspace with default arguments", func(t *testing.T) { + t.Parallel() + + storageDir := t.TempDir() + + rootCmd := NewRootCmd() + buf := new(bytes.Buffer) + rootCmd.SetOut(buf) + rootCmd.SetArgs([]string{"--storage", storageDir, "init"}) + + err := rootCmd.Execute() + if err != nil { + t.Fatalf("Execute() failed: %v", err) + } + + output := buf.String() + if !strings.Contains(output, "Registered workspace:") { + t.Errorf("Expected output to contain 'Registered workspace:', got: %s", output) + } + if !strings.Contains(output, "ID:") { + t.Errorf("Expected output to contain 'ID:', got: %s", output) + } + + // Verify instance was created + manager, err := instances.NewManager(storageDir) + if err != nil { + t.Fatalf("Failed to create manager: %v", err) + } + + instancesList, err := manager.List() + if err != nil { + t.Fatalf("Failed to list instances: %v", err) + } + + if len(instancesList) != 1 { + t.Errorf("Expected 1 instance, got %d", len(instancesList)) + } + + inst := instancesList[0] + + // Verify instance has a non-empty ID + if inst.GetID() == "" { + t.Error("Expected instance to have a non-empty ID") + } + + // Verify sources directory is current directory (absolute) + expectedAbsSourcesDir, _ := filepath.Abs(".") + if inst.GetSourceDir() != expectedAbsSourcesDir { + t.Errorf("Expected source dir %s, got %s", expectedAbsSourcesDir, inst.GetSourceDir()) + } + + // Verify config directory defaults to .kortex in current directory + expectedConfigDir := filepath.Join(expectedAbsSourcesDir, ".kortex") + if inst.GetConfigDir() != expectedConfigDir { + t.Errorf("Expected config dir %s, got %s", expectedConfigDir, inst.GetConfigDir()) + } + + // Verify paths are absolute + if !filepath.IsAbs(inst.GetSourceDir()) { + t.Errorf("Expected source dir to be absolute, got %s", inst.GetSourceDir()) + } + if !filepath.IsAbs(inst.GetConfigDir()) { + t.Errorf("Expected config dir to be absolute, got %s", inst.GetConfigDir()) + } + }) + + t.Run("registers workspace with custom sources directory", func(t *testing.T) { + t.Parallel() + + storageDir := t.TempDir() + sourcesDir := t.TempDir() + + rootCmd := NewRootCmd() + buf := new(bytes.Buffer) + rootCmd.SetOut(buf) + rootCmd.SetArgs([]string{"--storage", storageDir, "init", sourcesDir}) + + err := rootCmd.Execute() + if err != nil { + t.Fatalf("Execute() failed: %v", err) + } + + output := buf.String() + if !strings.Contains(output, sourcesDir) { + t.Errorf("Expected output to contain sources directory %s, got: %s", sourcesDir, output) + } + + // Verify instance was created with correct paths + manager, err := instances.NewManager(storageDir) + if err != nil { + t.Fatalf("Failed to create manager: %v", err) + } + + instancesList, err := manager.List() + if err != nil { + t.Fatalf("Failed to list instances: %v", err) + } + + if len(instancesList) != 1 { + t.Fatalf("Expected 1 instance, got %d", len(instancesList)) + } + + inst := instancesList[0] + + // Verify instance has a non-empty ID + if inst.GetID() == "" { + t.Error("Expected instance to have a non-empty ID") + } + + expectedAbsSourcesDir, _ := filepath.Abs(sourcesDir) + if inst.GetSourceDir() != expectedAbsSourcesDir { + t.Errorf("Expected source dir %s, got %s", expectedAbsSourcesDir, inst.GetSourceDir()) + } + + expectedConfigDir := filepath.Join(expectedAbsSourcesDir, ".kortex") + if inst.GetConfigDir() != expectedConfigDir { + t.Errorf("Expected config dir %s, got %s", expectedConfigDir, inst.GetConfigDir()) + } + + // Verify paths are absolute + if !filepath.IsAbs(inst.GetSourceDir()) { + t.Errorf("Expected source dir to be absolute, got %s", inst.GetSourceDir()) + } + if !filepath.IsAbs(inst.GetConfigDir()) { + t.Errorf("Expected config dir to be absolute, got %s", inst.GetConfigDir()) + } + + // Verify output contains the instance ID + if !strings.Contains(output, inst.GetID()) { + t.Errorf("Expected output to contain instance ID %s, got: %s", inst.GetID(), output) + } + }) + + t.Run("registers workspace with custom configuration directory", func(t *testing.T) { + t.Parallel() + + storageDir := t.TempDir() + configDir := t.TempDir() + + rootCmd := NewRootCmd() + buf := new(bytes.Buffer) + rootCmd.SetOut(buf) + rootCmd.SetArgs([]string{"--storage", storageDir, "init", "--workspace-configuration", configDir}) + + err := rootCmd.Execute() + if err != nil { + t.Fatalf("Execute() failed: %v", err) + } + + output := buf.String() + if !strings.Contains(output, configDir) { + t.Errorf("Expected output to contain config directory %s, got: %s", configDir, output) + } + + // Verify instance was created with correct paths + manager, err := instances.NewManager(storageDir) + if err != nil { + t.Fatalf("Failed to create manager: %v", err) + } + + instancesList, err := manager.List() + if err != nil { + t.Fatalf("Failed to list instances: %v", err) + } + + if len(instancesList) != 1 { + t.Fatalf("Expected 1 instance, got %d", len(instancesList)) + } + + inst := instancesList[0] + + // Verify instance has a non-empty ID + if inst.GetID() == "" { + t.Error("Expected instance to have a non-empty ID") + } + + // Verify sources directory defaults to current directory + expectedAbsSourcesDir, _ := filepath.Abs(".") + if inst.GetSourceDir() != expectedAbsSourcesDir { + t.Errorf("Expected source dir %s, got %s", expectedAbsSourcesDir, inst.GetSourceDir()) + } + + expectedAbsConfigDir, _ := filepath.Abs(configDir) + if inst.GetConfigDir() != expectedAbsConfigDir { + t.Errorf("Expected config dir %s, got %s", expectedAbsConfigDir, inst.GetConfigDir()) + } + + // Verify paths are absolute + if !filepath.IsAbs(inst.GetSourceDir()) { + t.Errorf("Expected source dir to be absolute, got %s", inst.GetSourceDir()) + } + if !filepath.IsAbs(inst.GetConfigDir()) { + t.Errorf("Expected config dir to be absolute, got %s", inst.GetConfigDir()) + } + + // Verify output contains the instance ID + if !strings.Contains(output, inst.GetID()) { + t.Errorf("Expected output to contain instance ID %s, got: %s", inst.GetID(), output) + } + }) + + t.Run("registers workspace with both custom directories", func(t *testing.T) { + t.Parallel() + + storageDir := t.TempDir() + sourcesDir := t.TempDir() + configDir := t.TempDir() + + rootCmd := NewRootCmd() + buf := new(bytes.Buffer) + rootCmd.SetOut(buf) + rootCmd.SetArgs([]string{"--storage", storageDir, "init", sourcesDir, "--workspace-configuration", configDir}) + + err := rootCmd.Execute() + if err != nil { + t.Fatalf("Execute() failed: %v", err) + } + + output := buf.String() + if !strings.Contains(output, sourcesDir) { + t.Errorf("Expected output to contain sources directory %s, got: %s", sourcesDir, output) + } + if !strings.Contains(output, configDir) { + t.Errorf("Expected output to contain config directory %s, got: %s", configDir, output) + } + + // Verify instance was created with correct paths + manager, err := instances.NewManager(storageDir) + if err != nil { + t.Fatalf("Failed to create manager: %v", err) + } + + instancesList, err := manager.List() + if err != nil { + t.Fatalf("Failed to list instances: %v", err) + } + + if len(instancesList) != 1 { + t.Fatalf("Expected 1 instance, got %d", len(instancesList)) + } + + inst := instancesList[0] + + // Verify instance has a non-empty ID + if inst.GetID() == "" { + t.Error("Expected instance to have a non-empty ID") + } + + expectedAbsSourcesDir, _ := filepath.Abs(sourcesDir) + if inst.GetSourceDir() != expectedAbsSourcesDir { + t.Errorf("Expected source dir %s, got %s", expectedAbsSourcesDir, inst.GetSourceDir()) + } + + expectedAbsConfigDir, _ := filepath.Abs(configDir) + if inst.GetConfigDir() != expectedAbsConfigDir { + t.Errorf("Expected config dir %s, got %s", expectedAbsConfigDir, inst.GetConfigDir()) + } + + // Verify paths are absolute + if !filepath.IsAbs(inst.GetSourceDir()) { + t.Errorf("Expected source dir to be absolute, got %s", inst.GetSourceDir()) + } + if !filepath.IsAbs(inst.GetConfigDir()) { + t.Errorf("Expected config dir to be absolute, got %s", inst.GetConfigDir()) + } + + // Verify output contains the instance ID + if !strings.Contains(output, inst.GetID()) { + t.Errorf("Expected output to contain instance ID %s, got: %s", inst.GetID(), output) + } + }) + + t.Run("registers multiple workspaces", func(t *testing.T) { + t.Parallel() + + storageDir := t.TempDir() + sourcesDir1 := t.TempDir() + sourcesDir2 := t.TempDir() + + // Register first workspace + rootCmd1 := NewRootCmd() + buf1 := new(bytes.Buffer) + rootCmd1.SetOut(buf1) + rootCmd1.SetArgs([]string{"--storage", storageDir, "init", sourcesDir1}) + + err := rootCmd1.Execute() + if err != nil { + t.Fatalf("Execute() failed for first workspace: %v", err) + } + + // Register second workspace + rootCmd2 := NewRootCmd() + buf2 := new(bytes.Buffer) + rootCmd2.SetOut(buf2) + rootCmd2.SetArgs([]string{"--storage", storageDir, "init", sourcesDir2}) + + err = rootCmd2.Execute() + if err != nil { + t.Fatalf("Execute() failed for second workspace: %v", err) + } + + // Verify both instances exist + manager, err := instances.NewManager(storageDir) + if err != nil { + t.Fatalf("Failed to create manager: %v", err) + } + + instancesList, err := manager.List() + if err != nil { + t.Fatalf("Failed to list instances: %v", err) + } + + if len(instancesList) != 2 { + t.Errorf("Expected 2 instances, got %d", len(instancesList)) + } + + // Verify both instances have unique IDs + if instancesList[0].GetID() == "" || instancesList[1].GetID() == "" { + t.Error("Expected both instances to have non-empty IDs") + } + if instancesList[0].GetID() == instancesList[1].GetID() { + t.Error("Expected instances to have unique IDs") + } + + // Verify both instances have correct source directories + expectedAbsSourcesDir1, _ := filepath.Abs(sourcesDir1) + expectedAbsSourcesDir2, _ := filepath.Abs(sourcesDir2) + + foundDir1 := false + foundDir2 := false + for _, inst := range instancesList { + if inst.GetSourceDir() == expectedAbsSourcesDir1 { + foundDir1 = true + // Verify config dir for first workspace + expectedConfigDir1 := filepath.Join(expectedAbsSourcesDir1, ".kortex") + if inst.GetConfigDir() != expectedConfigDir1 { + t.Errorf("Expected config dir %s for first workspace, got %s", expectedConfigDir1, inst.GetConfigDir()) + } + } + if inst.GetSourceDir() == expectedAbsSourcesDir2 { + foundDir2 = true + // Verify config dir for second workspace + expectedConfigDir2 := filepath.Join(expectedAbsSourcesDir2, ".kortex") + if inst.GetConfigDir() != expectedConfigDir2 { + t.Errorf("Expected config dir %s for second workspace, got %s", expectedConfigDir2, inst.GetConfigDir()) + } + } + + // Verify paths are absolute + if !filepath.IsAbs(inst.GetSourceDir()) { + t.Errorf("Expected source dir to be absolute, got %s", inst.GetSourceDir()) + } + if !filepath.IsAbs(inst.GetConfigDir()) { + t.Errorf("Expected config dir to be absolute, got %s", inst.GetConfigDir()) + } + } + + if !foundDir1 { + t.Errorf("Expected to find instance with source dir %s", expectedAbsSourcesDir1) + } + if !foundDir2 { + t.Errorf("Expected to find instance with source dir %s", expectedAbsSourcesDir2) + } + }) +} diff --git a/pkg/cmd/root.go b/pkg/cmd/root.go index 9017a9e..8a5b8d1 100644 --- a/pkg/cmd/root.go +++ b/pkg/cmd/root.go @@ -45,6 +45,7 @@ func NewRootCmd() *cobra.Command { // Add subcommands rootCmd.AddCommand(NewVersionCmd()) + rootCmd.AddCommand(NewInitCmd()) // Global flags rootCmd.PersistentFlags().String("storage", defaultStoragePath, "Directory where kortex-cli will store all its files") From 17c508b38956e7772b3219ddc4aedcb33e8af778 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Fri, 6 Mar 2026 14:23:25 +0100 Subject: [PATCH 2/5] fix: review Signed-off-by: Philippe Martin Co-Authored-By: Claude Code (Claude Sonnet 4.5) --- AGENTS.md | 4 ++-- pkg/cmd/init.go | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5f9d09b..d7e6a46 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -195,7 +195,7 @@ Commands should follow a consistent structure for maintainability and testabilit ### Testing Pattern for Commands -Commands should have two types of tests following the pattern in `pkg/instances/instance_test.go`: +Commands should have two types of tests following the pattern in `pkg/cmd/init_test.go`: 1. **Unit Tests** - Test the `preRun` method directly: - Use `t.Run()` for subtests within a parent test function @@ -211,7 +211,7 @@ Commands should have two types of tests following the pattern in `pkg/instances/ - Verify all field values from `manager.List()` or similar - Test multiple scenarios (default args, custom args, edge cases) -**Reference:** See `pkg/cmd/init_test.go` for complete examples of both unit and E2E tests. +**Reference:** See `pkg/cmd/init_test.go` for complete examples of both `preRun` unit tests (in `TestInitCmd_PreRun`) and E2E tests (in `TestInitCmd_E2E`). ### Working with the Instances Manager diff --git a/pkg/cmd/init.go b/pkg/cmd/init.go index 66ee63e..79e2d03 100644 --- a/pkg/cmd/init.go +++ b/pkg/cmd/init.go @@ -38,7 +38,10 @@ type InitCmd struct { // preRun validates the parameters and flags func (i *InitCmd) preRun(cmd *cobra.Command, args []string) error { // Get storage directory from global flag - storageDir, _ := cmd.Flags().GetString("storage") + storageDir, err := cmd.Flags().GetString("storage") + if err != nil { + return fmt.Errorf("failed to read --storage flag: %w", err) + } // Create manager manager, err := instances.NewManager(storageDir) From a7f9624c79a972211f5e753bb3477c3afc72c486 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Fri, 6 Mar 2026 14:31:15 +0100 Subject: [PATCH 3/5] fix: make InitCmd struct private Signed-off-by: Philippe Martin Co-Authored-By: Claude Code (Claude Sonnet 4.5) --- pkg/cmd/init.go | 16 +++---- pkg/cmd/init_test.go | 106 +++++++++++++++++++++---------------------- 2 files changed, 61 insertions(+), 61 deletions(-) diff --git a/pkg/cmd/init.go b/pkg/cmd/init.go index 79e2d03..f560416 100644 --- a/pkg/cmd/init.go +++ b/pkg/cmd/init.go @@ -26,8 +26,8 @@ import ( "github.com/spf13/cobra" ) -// InitCmd contains the configuration for the init command -type InitCmd struct { +// initCmd contains the configuration for the init command +type initCmd struct { sourcesDir string workspaceConfigDir string absSourcesDir string @@ -36,7 +36,7 @@ type InitCmd struct { } // preRun validates the parameters and flags -func (i *InitCmd) preRun(cmd *cobra.Command, args []string) error { +func (i *initCmd) preRun(cmd *cobra.Command, args []string) error { // Get storage directory from global flag storageDir, err := cmd.Flags().GetString("storage") if err != nil { @@ -79,7 +79,7 @@ func (i *InitCmd) preRun(cmd *cobra.Command, args []string) error { } // run executes the init command logic -func (i *InitCmd) run(cmd *cobra.Command, args []string) error { +func (i *initCmd) run(cmd *cobra.Command, args []string) error { // Create a new instance instance, err := instances.NewInstance(i.absSourcesDir, i.absConfigDir) if err != nil { @@ -101,7 +101,7 @@ func (i *InitCmd) run(cmd *cobra.Command, args []string) error { } func NewInitCmd() *cobra.Command { - initCmd := &InitCmd{} + c := &initCmd{} cmd := &cobra.Command{ Use: "init [sources-directory]", @@ -111,12 +111,12 @@ func NewInitCmd() *cobra.Command { The sources directory defaults to the current directory (.) if not specified. The workspace configuration directory defaults to .kortex/ inside the sources directory if not specified.`, Args: cobra.MaximumNArgs(1), - PreRunE: initCmd.preRun, - RunE: initCmd.run, + PreRunE: c.preRun, + RunE: c.run, } // Add workspace-configuration flag - cmd.Flags().StringVar(&initCmd.workspaceConfigDir, "workspace-configuration", "", "Directory for workspace configuration (default: /.kortex)") + cmd.Flags().StringVar(&c.workspaceConfigDir, "workspace-configuration", "", "Directory for workspace configuration (default: /.kortex)") return cmd } diff --git a/pkg/cmd/init_test.go b/pkg/cmd/init_test.go index 2b6d40b..8ca7c16 100644 --- a/pkg/cmd/init_test.go +++ b/pkg/cmd/init_test.go @@ -36,39 +36,39 @@ func TestInitCmd_PreRun(t *testing.T) { tempDir := t.TempDir() - initCmd := &InitCmd{} + c := &initCmd{} cmd := &cobra.Command{} cmd.Flags().String("workspace-configuration", "", "test flag") cmd.Flags().String("storage", tempDir, "test storage flag") args := []string{} - err := initCmd.preRun(cmd, args) + err := c.preRun(cmd, args) if err != nil { t.Fatalf("preRun() failed: %v", err) } - if initCmd.manager == nil { + if c.manager == nil { t.Error("Expected manager to be created") } - if initCmd.sourcesDir != "." { - t.Errorf("Expected sourcesDir to be '.', got %s", initCmd.sourcesDir) + if c.sourcesDir != "." { + t.Errorf("Expected sourcesDir to be '.', got %s", c.sourcesDir) } expectedAbsSourcesDir, _ := filepath.Abs(".") - if initCmd.absSourcesDir != expectedAbsSourcesDir { - t.Errorf("Expected absSourcesDir to be %s, got %s", expectedAbsSourcesDir, initCmd.absSourcesDir) + if c.absSourcesDir != expectedAbsSourcesDir { + t.Errorf("Expected absSourcesDir to be %s, got %s", expectedAbsSourcesDir, c.absSourcesDir) } expectedConfigDir := filepath.Join(".", ".kortex") - if initCmd.workspaceConfigDir != expectedConfigDir { - t.Errorf("Expected workspaceConfigDir to be %s, got %s", expectedConfigDir, initCmd.workspaceConfigDir) + if c.workspaceConfigDir != expectedConfigDir { + t.Errorf("Expected workspaceConfigDir to be %s, got %s", expectedConfigDir, c.workspaceConfigDir) } expectedAbsConfigDir, _ := filepath.Abs(expectedConfigDir) - if initCmd.absConfigDir != expectedAbsConfigDir { - t.Errorf("Expected absConfigDir to be %s, got %s", expectedAbsConfigDir, initCmd.absConfigDir) + if c.absConfigDir != expectedAbsConfigDir { + t.Errorf("Expected absConfigDir to be %s, got %s", expectedAbsConfigDir, c.absConfigDir) } }) @@ -78,39 +78,39 @@ func TestInitCmd_PreRun(t *testing.T) { tempDir := t.TempDir() sourcesDir := t.TempDir() - initCmd := &InitCmd{} + c := &initCmd{} cmd := &cobra.Command{} cmd.Flags().String("workspace-configuration", "", "test flag") cmd.Flags().String("storage", tempDir, "test storage flag") args := []string{sourcesDir} - err := initCmd.preRun(cmd, args) + err := c.preRun(cmd, args) if err != nil { t.Fatalf("preRun() failed: %v", err) } - if initCmd.manager == nil { + if c.manager == nil { t.Error("Expected manager to be created") } - if initCmd.sourcesDir != sourcesDir { - t.Errorf("Expected sourcesDir to be %s, got %s", sourcesDir, initCmd.sourcesDir) + if c.sourcesDir != sourcesDir { + t.Errorf("Expected sourcesDir to be %s, got %s", sourcesDir, c.sourcesDir) } expectedAbsSourcesDir, _ := filepath.Abs(sourcesDir) - if initCmd.absSourcesDir != expectedAbsSourcesDir { - t.Errorf("Expected absSourcesDir to be %s, got %s", expectedAbsSourcesDir, initCmd.absSourcesDir) + if c.absSourcesDir != expectedAbsSourcesDir { + t.Errorf("Expected absSourcesDir to be %s, got %s", expectedAbsSourcesDir, c.absSourcesDir) } expectedConfigDir := filepath.Join(sourcesDir, ".kortex") - if initCmd.workspaceConfigDir != expectedConfigDir { - t.Errorf("Expected workspaceConfigDir to be %s, got %s", expectedConfigDir, initCmd.workspaceConfigDir) + if c.workspaceConfigDir != expectedConfigDir { + t.Errorf("Expected workspaceConfigDir to be %s, got %s", expectedConfigDir, c.workspaceConfigDir) } expectedAbsConfigDir, _ := filepath.Abs(expectedConfigDir) - if initCmd.absConfigDir != expectedAbsConfigDir { - t.Errorf("Expected absConfigDir to be %s, got %s", expectedAbsConfigDir, initCmd.absConfigDir) + if c.absConfigDir != expectedAbsConfigDir { + t.Errorf("Expected absConfigDir to be %s, got %s", expectedAbsConfigDir, c.absConfigDir) } }) @@ -120,7 +120,7 @@ func TestInitCmd_PreRun(t *testing.T) { tempDir := t.TempDir() configDir := t.TempDir() - initCmd := &InitCmd{ + c := &initCmd{ workspaceConfigDir: configDir, } cmd := &cobra.Command{} @@ -130,26 +130,26 @@ func TestInitCmd_PreRun(t *testing.T) { args := []string{} - err := initCmd.preRun(cmd, args) + err := c.preRun(cmd, args) if err != nil { t.Fatalf("preRun() failed: %v", err) } - if initCmd.manager == nil { + if c.manager == nil { t.Error("Expected manager to be created") } - if initCmd.sourcesDir != "." { - t.Errorf("Expected sourcesDir to be '.', got %s", initCmd.sourcesDir) + if c.sourcesDir != "." { + t.Errorf("Expected sourcesDir to be '.', got %s", c.sourcesDir) } - if initCmd.workspaceConfigDir != configDir { - t.Errorf("Expected workspaceConfigDir to be %s, got %s", configDir, initCmd.workspaceConfigDir) + if c.workspaceConfigDir != configDir { + t.Errorf("Expected workspaceConfigDir to be %s, got %s", configDir, c.workspaceConfigDir) } expectedAbsConfigDir, _ := filepath.Abs(configDir) - if initCmd.absConfigDir != expectedAbsConfigDir { - t.Errorf("Expected absConfigDir to be %s, got %s", expectedAbsConfigDir, initCmd.absConfigDir) + if c.absConfigDir != expectedAbsConfigDir { + t.Errorf("Expected absConfigDir to be %s, got %s", expectedAbsConfigDir, c.absConfigDir) } }) @@ -160,7 +160,7 @@ func TestInitCmd_PreRun(t *testing.T) { sourcesDir := t.TempDir() configDir := t.TempDir() - initCmd := &InitCmd{ + c := &initCmd{ workspaceConfigDir: configDir, } cmd := &cobra.Command{} @@ -170,31 +170,31 @@ func TestInitCmd_PreRun(t *testing.T) { args := []string{sourcesDir} - err := initCmd.preRun(cmd, args) + err := c.preRun(cmd, args) if err != nil { t.Fatalf("preRun() failed: %v", err) } - if initCmd.manager == nil { + if c.manager == nil { t.Error("Expected manager to be created") } - if initCmd.sourcesDir != sourcesDir { - t.Errorf("Expected sourcesDir to be %s, got %s", sourcesDir, initCmd.sourcesDir) + if c.sourcesDir != sourcesDir { + t.Errorf("Expected sourcesDir to be %s, got %s", sourcesDir, c.sourcesDir) } expectedAbsSourcesDir, _ := filepath.Abs(sourcesDir) - if initCmd.absSourcesDir != expectedAbsSourcesDir { - t.Errorf("Expected absSourcesDir to be %s, got %s", expectedAbsSourcesDir, initCmd.absSourcesDir) + if c.absSourcesDir != expectedAbsSourcesDir { + t.Errorf("Expected absSourcesDir to be %s, got %s", expectedAbsSourcesDir, c.absSourcesDir) } - if initCmd.workspaceConfigDir != configDir { - t.Errorf("Expected workspaceConfigDir to be %s, got %s", configDir, initCmd.workspaceConfigDir) + if c.workspaceConfigDir != configDir { + t.Errorf("Expected workspaceConfigDir to be %s, got %s", configDir, c.workspaceConfigDir) } expectedAbsConfigDir, _ := filepath.Abs(configDir) - if initCmd.absConfigDir != expectedAbsConfigDir { - t.Errorf("Expected absConfigDir to be %s, got %s", expectedAbsConfigDir, initCmd.absConfigDir) + if c.absConfigDir != expectedAbsConfigDir { + t.Errorf("Expected absConfigDir to be %s, got %s", expectedAbsConfigDir, c.absConfigDir) } }) @@ -204,34 +204,34 @@ func TestInitCmd_PreRun(t *testing.T) { tempDir := t.TempDir() relativePath := filepath.Join(".", "relative", "path") - initCmd := &InitCmd{} + c := &initCmd{} cmd := &cobra.Command{} cmd.Flags().String("workspace-configuration", "", "test flag") cmd.Flags().String("storage", tempDir, "test storage flag") args := []string{relativePath} - err := initCmd.preRun(cmd, args) + err := c.preRun(cmd, args) if err != nil { t.Fatalf("preRun() failed: %v", err) } - if initCmd.manager == nil { + if c.manager == nil { t.Error("Expected manager to be created") } - if initCmd.sourcesDir != relativePath { - t.Errorf("Expected sourcesDir to be %s, got %s", relativePath, initCmd.sourcesDir) + if c.sourcesDir != relativePath { + t.Errorf("Expected sourcesDir to be %s, got %s", relativePath, c.sourcesDir) } expectedAbsSourcesDir, _ := filepath.Abs(relativePath) - if initCmd.absSourcesDir != expectedAbsSourcesDir { - t.Errorf("Expected absSourcesDir to be %s, got %s", expectedAbsSourcesDir, initCmd.absSourcesDir) + if c.absSourcesDir != expectedAbsSourcesDir { + t.Errorf("Expected absSourcesDir to be %s, got %s", expectedAbsSourcesDir, c.absSourcesDir) } expectedConfigDir := filepath.Join(relativePath, ".kortex") - if initCmd.workspaceConfigDir != expectedConfigDir { - t.Errorf("Expected workspaceConfigDir to be %s, got %s", expectedConfigDir, initCmd.workspaceConfigDir) + if c.workspaceConfigDir != expectedConfigDir { + t.Errorf("Expected workspaceConfigDir to be %s, got %s", expectedConfigDir, c.workspaceConfigDir) } }) } @@ -274,7 +274,7 @@ func TestInitCmd_E2E(t *testing.T) { } if len(instancesList) != 1 { - t.Errorf("Expected 1 instance, got %d", len(instancesList)) + t.Fatalf("Expected 1 instance, got %d", len(instancesList)) } inst := instancesList[0] @@ -552,7 +552,7 @@ func TestInitCmd_E2E(t *testing.T) { } if len(instancesList) != 2 { - t.Errorf("Expected 2 instances, got %d", len(instancesList)) + t.Fatalf("Expected 2 instances, got %d", len(instancesList)) } // Verify both instances have unique IDs From 83a9d817a36e1d30c87cfaed3189cca8f8ebef1f Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Fri, 6 Mar 2026 14:42:12 +0100 Subject: [PATCH 4/5] feat: add verbose flag and display ID only by default Signed-off-by: Philippe Martin Co-Authored-By: Claude Code (Claude Sonnet 4.5) --- pkg/cmd/init.go | 16 ++++-- pkg/cmd/init_test.go | 130 +++++++++++++++++++++++++++++-------------- 2 files changed, 101 insertions(+), 45 deletions(-) diff --git a/pkg/cmd/init.go b/pkg/cmd/init.go index f560416..0e70a9a 100644 --- a/pkg/cmd/init.go +++ b/pkg/cmd/init.go @@ -33,6 +33,7 @@ type initCmd struct { absSourcesDir string absConfigDir string manager instances.Manager + verbose bool } // preRun validates the parameters and flags @@ -92,10 +93,14 @@ func (i *initCmd) run(cmd *cobra.Command, args []string) error { return fmt.Errorf("failed to add instance: %w", err) } - cmd.Printf("Registered workspace:\n") - cmd.Printf(" ID: %s\n", addedInstance.GetID()) - cmd.Printf(" Sources directory: %s\n", addedInstance.GetSourceDir()) - cmd.Printf(" Configuration directory: %s\n", addedInstance.GetConfigDir()) + if i.verbose { + cmd.Printf("Registered workspace:\n") + cmd.Printf(" ID: %s\n", addedInstance.GetID()) + cmd.Printf(" Sources directory: %s\n", addedInstance.GetSourceDir()) + cmd.Printf(" Configuration directory: %s\n", addedInstance.GetConfigDir()) + } else { + cmd.Println(addedInstance.GetID()) + } return nil } @@ -118,5 +123,8 @@ The workspace configuration directory defaults to .kortex/ inside the sources di // Add workspace-configuration flag cmd.Flags().StringVar(&c.workspaceConfigDir, "workspace-configuration", "", "Directory for workspace configuration (default: /.kortex)") + // Add verbose flag + cmd.Flags().BoolVarP(&c.verbose, "verbose", "v", false, "Show detailed output") + return cmd } diff --git a/pkg/cmd/init_test.go b/pkg/cmd/init_test.go index 8ca7c16..ef2b645 100644 --- a/pkg/cmd/init_test.go +++ b/pkg/cmd/init_test.go @@ -254,14 +254,6 @@ func TestInitCmd_E2E(t *testing.T) { t.Fatalf("Execute() failed: %v", err) } - output := buf.String() - if !strings.Contains(output, "Registered workspace:") { - t.Errorf("Expected output to contain 'Registered workspace:', got: %s", output) - } - if !strings.Contains(output, "ID:") { - t.Errorf("Expected output to contain 'ID:', got: %s", output) - } - // Verify instance was created manager, err := instances.NewManager(storageDir) if err != nil { @@ -284,6 +276,12 @@ func TestInitCmd_E2E(t *testing.T) { t.Error("Expected instance to have a non-empty ID") } + // Verify output contains only the ID (default non-verbose output) + output := strings.TrimSpace(buf.String()) + if output != inst.GetID() { + t.Errorf("Expected output to be just the ID %s, got: %s", inst.GetID(), output) + } + // Verify sources directory is current directory (absolute) expectedAbsSourcesDir, _ := filepath.Abs(".") if inst.GetSourceDir() != expectedAbsSourcesDir { @@ -321,11 +319,6 @@ func TestInitCmd_E2E(t *testing.T) { t.Fatalf("Execute() failed: %v", err) } - output := buf.String() - if !strings.Contains(output, sourcesDir) { - t.Errorf("Expected output to contain sources directory %s, got: %s", sourcesDir, output) - } - // Verify instance was created with correct paths manager, err := instances.NewManager(storageDir) if err != nil { @@ -348,6 +341,12 @@ func TestInitCmd_E2E(t *testing.T) { t.Error("Expected instance to have a non-empty ID") } + // Verify output contains only the ID (default non-verbose output) + output := strings.TrimSpace(buf.String()) + if output != inst.GetID() { + t.Errorf("Expected output to be just the ID %s, got: %s", inst.GetID(), output) + } + expectedAbsSourcesDir, _ := filepath.Abs(sourcesDir) if inst.GetSourceDir() != expectedAbsSourcesDir { t.Errorf("Expected source dir %s, got %s", expectedAbsSourcesDir, inst.GetSourceDir()) @@ -365,11 +364,6 @@ func TestInitCmd_E2E(t *testing.T) { if !filepath.IsAbs(inst.GetConfigDir()) { t.Errorf("Expected config dir to be absolute, got %s", inst.GetConfigDir()) } - - // Verify output contains the instance ID - if !strings.Contains(output, inst.GetID()) { - t.Errorf("Expected output to contain instance ID %s, got: %s", inst.GetID(), output) - } }) t.Run("registers workspace with custom configuration directory", func(t *testing.T) { @@ -388,11 +382,6 @@ func TestInitCmd_E2E(t *testing.T) { t.Fatalf("Execute() failed: %v", err) } - output := buf.String() - if !strings.Contains(output, configDir) { - t.Errorf("Expected output to contain config directory %s, got: %s", configDir, output) - } - // Verify instance was created with correct paths manager, err := instances.NewManager(storageDir) if err != nil { @@ -415,6 +404,12 @@ func TestInitCmd_E2E(t *testing.T) { t.Error("Expected instance to have a non-empty ID") } + // Verify output contains only the ID (default non-verbose output) + output := strings.TrimSpace(buf.String()) + if output != inst.GetID() { + t.Errorf("Expected output to be just the ID %s, got: %s", inst.GetID(), output) + } + // Verify sources directory defaults to current directory expectedAbsSourcesDir, _ := filepath.Abs(".") if inst.GetSourceDir() != expectedAbsSourcesDir { @@ -433,11 +428,6 @@ func TestInitCmd_E2E(t *testing.T) { if !filepath.IsAbs(inst.GetConfigDir()) { t.Errorf("Expected config dir to be absolute, got %s", inst.GetConfigDir()) } - - // Verify output contains the instance ID - if !strings.Contains(output, inst.GetID()) { - t.Errorf("Expected output to contain instance ID %s, got: %s", inst.GetID(), output) - } }) t.Run("registers workspace with both custom directories", func(t *testing.T) { @@ -457,14 +447,6 @@ func TestInitCmd_E2E(t *testing.T) { t.Fatalf("Execute() failed: %v", err) } - output := buf.String() - if !strings.Contains(output, sourcesDir) { - t.Errorf("Expected output to contain sources directory %s, got: %s", sourcesDir, output) - } - if !strings.Contains(output, configDir) { - t.Errorf("Expected output to contain config directory %s, got: %s", configDir, output) - } - // Verify instance was created with correct paths manager, err := instances.NewManager(storageDir) if err != nil { @@ -487,6 +469,12 @@ func TestInitCmd_E2E(t *testing.T) { t.Error("Expected instance to have a non-empty ID") } + // Verify output contains only the ID (default non-verbose output) + output := strings.TrimSpace(buf.String()) + if output != inst.GetID() { + t.Errorf("Expected output to be just the ID %s, got: %s", inst.GetID(), output) + } + expectedAbsSourcesDir, _ := filepath.Abs(sourcesDir) if inst.GetSourceDir() != expectedAbsSourcesDir { t.Errorf("Expected source dir %s, got %s", expectedAbsSourcesDir, inst.GetSourceDir()) @@ -504,11 +492,6 @@ func TestInitCmd_E2E(t *testing.T) { if !filepath.IsAbs(inst.GetConfigDir()) { t.Errorf("Expected config dir to be absolute, got %s", inst.GetConfigDir()) } - - // Verify output contains the instance ID - if !strings.Contains(output, inst.GetID()) { - t.Errorf("Expected output to contain instance ID %s, got: %s", inst.GetID(), output) - } }) t.Run("registers multiple workspaces", func(t *testing.T) { @@ -603,4 +586,69 @@ func TestInitCmd_E2E(t *testing.T) { t.Errorf("Expected to find instance with source dir %s", expectedAbsSourcesDir2) } }) + + t.Run("registers workspace with verbose flag", func(t *testing.T) { + t.Parallel() + + storageDir := t.TempDir() + sourcesDir := t.TempDir() + + rootCmd := NewRootCmd() + buf := new(bytes.Buffer) + rootCmd.SetOut(buf) + rootCmd.SetArgs([]string{"--storage", storageDir, "init", sourcesDir, "--verbose"}) + + err := rootCmd.Execute() + if err != nil { + t.Fatalf("Execute() failed: %v", err) + } + + output := buf.String() + + // Verify verbose output contains expected strings + if !strings.Contains(output, "Registered workspace:") { + t.Errorf("Expected verbose output to contain 'Registered workspace:', got: %s", output) + } + if !strings.Contains(output, "ID:") { + t.Errorf("Expected verbose output to contain 'ID:', got: %s", output) + } + if !strings.Contains(output, "Sources directory:") { + t.Errorf("Expected verbose output to contain 'Sources directory:', got: %s", output) + } + if !strings.Contains(output, "Configuration directory:") { + t.Errorf("Expected verbose output to contain 'Configuration directory:', got: %s", output) + } + + // Verify instance was created with correct paths + manager, err := instances.NewManager(storageDir) + if err != nil { + t.Fatalf("Failed to create manager: %v", err) + } + + instancesList, err := manager.List() + if err != nil { + t.Fatalf("Failed to list instances: %v", err) + } + + if len(instancesList) != 1 { + t.Fatalf("Expected 1 instance, got %d", len(instancesList)) + } + + inst := instancesList[0] + + // Verify verbose output contains the actual values + expectedAbsSourcesDir, _ := filepath.Abs(sourcesDir) + if !strings.Contains(output, expectedAbsSourcesDir) { + t.Errorf("Expected verbose output to contain sources directory %s, got: %s", expectedAbsSourcesDir, output) + } + + expectedConfigDir := filepath.Join(expectedAbsSourcesDir, ".kortex") + if !strings.Contains(output, expectedConfigDir) { + t.Errorf("Expected verbose output to contain config directory %s, got: %s", expectedConfigDir, output) + } + + if !strings.Contains(output, inst.GetID()) { + t.Errorf("Expected verbose output to contain instance ID %s, got: %s", inst.GetID(), output) + } + }) } From 9dc470e22c0716175045554b0f9224572c216ab9 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Fri, 6 Mar 2026 15:09:56 +0100 Subject: [PATCH 5/5] feat: use nested structure for storage Signed-off-by: Philippe Martin Co-Authored-By: Claude Code (Claude Sonnet 4.5) --- AGENTS.md | 57 +++++++++++++++++++++++++++++++++++ pkg/instances/instance.go | 30 +++++++++++------- pkg/instances/manager_test.go | 50 +++++++++++++++--------------- 3 files changed, 102 insertions(+), 35 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index d7e6a46..83f90fe 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -129,6 +129,63 @@ All modules (packages outside of `cmd/`) MUST follow the interface-based design **This pattern is MANDATORY for all new modules in `pkg/`.** +### JSON Storage Structure + +When designing JSON storage structures for persistent data, use **nested objects with subfields** instead of flat structures with naming conventions. + +**Preferred Pattern (nested structure):** +```json +{ + "id": "dc610bffa75f21b5b043f98aff12b157fb16fae6c0ac3139c28f85d6defbe017", + "paths": { + "source": "/Users/user/project", + "configuration": "/Users/user/project/.kortex" + } +} +``` + +**Avoid (flat structure with snake_case or camelCase):** +```json +{ + "id": "...", + "source_dir": "/Users/user/project", // Don't use snake_case + "config_dir": "/Users/user/project/.kortex" +} +``` + +```json +{ + "id": "...", + "sourceDir": "/Users/user/project", // Don't use camelCase + "configDir": "/Users/user/project/.kortex" +} +``` + +**Benefits:** +- **Better organization** - Related fields are grouped together +- **Clarity** - Field relationships are explicit through nesting +- **Extensibility** - Easy to add new subfields without polluting the top level +- **No naming conflicts** - Avoids debates about snake_case vs camelCase +- **Self-documenting** - Structure communicates intent + +**Implementation:** +- Create nested structs with `json` tags +- Use lowercase field names in JSON (Go convention for exported fields + json tags) +- Group related fields under descriptive parent keys + +**Example:** +```go +type InstancePaths struct { + Source string `json:"source"` + Configuration string `json:"configuration"` +} + +type InstanceData struct { + ID string `json:"id"` + Paths InstancePaths `json:"paths"` +} +``` + ### Skills System Skills are reusable capabilities that can be discovered and executed by AI agents: - **Location**: `skills//SKILL.md` diff --git a/pkg/instances/instance.go b/pkg/instances/instance.go index 5e635a1..f8639b8 100644 --- a/pkg/instances/instance.go +++ b/pkg/instances/instance.go @@ -29,14 +29,20 @@ var ( ErrInvalidPath = errors.New("invalid path") ) +// InstancePaths represents the paths in an instance +type InstancePaths struct { + // Source is the directory containing source files (absolute path) + Source string `json:"source"` + // Configuration is the directory containing workspace configuration (absolute path) + Configuration string `json:"configuration"` +} + // InstanceData represents the serializable data of an instance type InstanceData struct { // ID is the unique identifier for the instance ID string `json:"id"` - // SourceDir is the directory containing source files (absolute path) - SourceDir string `json:"source_dir"` - // ConfigDir is the directory containing workspace configuration (absolute path) - ConfigDir string `json:"config_dir"` + // Paths contains the source and configuration directories + Paths InstancePaths `json:"paths"` } // Instance represents a workspace instance with source and configuration directories. @@ -101,9 +107,11 @@ func (i *instance) IsAccessible() bool { // Dump returns the serializable data of the instance func (i *instance) Dump() InstanceData { return InstanceData{ - ID: i.ID, - SourceDir: i.SourceDir, - ConfigDir: i.ConfigDir, + ID: i.ID, + Paths: InstancePaths{ + Source: i.SourceDir, + Configuration: i.ConfigDir, + }, } } @@ -142,17 +150,17 @@ func NewInstanceFromData(data InstanceData) (Instance, error) { if data.ID == "" { return nil, errors.New("instance ID cannot be empty") } - if data.SourceDir == "" { + if data.Paths.Source == "" { return nil, ErrInvalidPath } - if data.ConfigDir == "" { + if data.Paths.Configuration == "" { return nil, ErrInvalidPath } return &instance{ ID: data.ID, - SourceDir: data.SourceDir, - ConfigDir: data.ConfigDir, + SourceDir: data.Paths.Source, + ConfigDir: data.Paths.Configuration, }, nil } diff --git a/pkg/instances/manager_test.go b/pkg/instances/manager_test.go index 9a6d9cd..e755321 100644 --- a/pkg/instances/manager_test.go +++ b/pkg/instances/manager_test.go @@ -53,9 +53,11 @@ func (f *fakeInstance) IsAccessible() bool { func (f *fakeInstance) Dump() InstanceData { return InstanceData{ - ID: f.id, - SourceDir: f.sourceDir, - ConfigDir: f.configDir, + ID: f.id, + Paths: InstancePaths{ + Source: f.sourceDir, + Configuration: f.configDir, + }, } } @@ -74,18 +76,18 @@ func fakeInstanceFactory(data InstanceData) (Instance, error) { if data.ID == "" { return nil, errors.New("instance ID cannot be empty") } - if data.SourceDir == "" { + if data.Paths.Source == "" { return nil, ErrInvalidPath } - if data.ConfigDir == "" { + if data.Paths.Configuration == "" { return nil, ErrInvalidPath } // For testing, we assume instances are accessible by default // Tests can verify accessibility behavior separately return &fakeInstance{ id: data.ID, - sourceDir: data.SourceDir, - configDir: data.ConfigDir, + sourceDir: data.Paths.Source, + configDir: data.Paths.Configuration, accessible: true, }, nil } @@ -557,13 +559,13 @@ func TestManager_Reconcile(t *testing.T) { if data.ID == "" { return nil, errors.New("instance ID cannot be empty") } - if data.SourceDir == "" || data.ConfigDir == "" { + if data.Paths.Source == "" || data.Paths.Configuration == "" { return nil, ErrInvalidPath } return &fakeInstance{ id: data.ID, - sourceDir: data.SourceDir, - configDir: data.ConfigDir, + sourceDir: data.Paths.Source, + configDir: data.Paths.Configuration, accessible: false, // Always inaccessible for this test }, nil } @@ -593,13 +595,13 @@ func TestManager_Reconcile(t *testing.T) { if data.ID == "" { return nil, errors.New("instance ID cannot be empty") } - if data.SourceDir == "" || data.ConfigDir == "" { + if data.Paths.Source == "" || data.Paths.Configuration == "" { return nil, ErrInvalidPath } return &fakeInstance{ id: data.ID, - sourceDir: data.SourceDir, - configDir: data.ConfigDir, + sourceDir: data.Paths.Source, + configDir: data.Paths.Configuration, accessible: false, // Always inaccessible for this test }, nil } @@ -629,13 +631,13 @@ func TestManager_Reconcile(t *testing.T) { if data.ID == "" { return nil, errors.New("instance ID cannot be empty") } - if data.SourceDir == "" || data.ConfigDir == "" { + if data.Paths.Source == "" || data.Paths.Configuration == "" { return nil, ErrInvalidPath } return &fakeInstance{ id: data.ID, - sourceDir: data.SourceDir, - configDir: data.ConfigDir, + sourceDir: data.Paths.Source, + configDir: data.Paths.Configuration, accessible: false, // Always inaccessible for this test }, nil } @@ -672,14 +674,14 @@ func TestManager_Reconcile(t *testing.T) { if data.ID == "" { return nil, errors.New("instance ID cannot be empty") } - if data.SourceDir == "" || data.ConfigDir == "" { + if data.Paths.Source == "" || data.Paths.Configuration == "" { return nil, ErrInvalidPath } - accessible := data.SourceDir == accessibleSource + accessible := data.Paths.Source == accessibleSource return &fakeInstance{ id: data.ID, - sourceDir: data.SourceDir, - configDir: data.ConfigDir, + sourceDir: data.Paths.Source, + configDir: data.Paths.Configuration, accessible: accessible, }, nil } @@ -823,11 +825,11 @@ func TestManager_Persistence(t *testing.T) { if instances[0].ID != generatedID { t.Errorf("JSON ID = %v, want %v", instances[0].ID, generatedID) } - if instances[0].SourceDir != expectedSource { - t.Errorf("JSON SourceDir = %v, want %v", instances[0].SourceDir, expectedSource) + if instances[0].Paths.Source != expectedSource { + t.Errorf("JSON Paths.Source = %v, want %v", instances[0].Paths.Source, expectedSource) } - if instances[0].ConfigDir != expectedConfig { - t.Errorf("JSON ConfigDir = %v, want %v", instances[0].ConfigDir, expectedConfig) + if instances[0].Paths.Configuration != expectedConfig { + t.Errorf("JSON Paths.Configuration = %v, want %v", instances[0].Paths.Configuration, expectedConfig) } }) }