Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 24 additions & 4 deletions pkg/cli/devcontainer.go
Original file line number Diff line number Diff line change
Expand Up @@ -191,17 +191,37 @@ func ensureDevcontainerConfig(verbose bool, additionalRepos []string) error {
}
}

// Write config file with proper indentation
data, err := json.MarshalIndent(config, "", " ")
// Serialize the new config to JSON
newData, err := json.MarshalIndent(config, "", " ")
if err != nil {
return fmt.Errorf("failed to marshal devcontainer.json: %w", err)
}

// Add newline at end of file
data = append(data, '\n')
newData = append(newData, '\n')

// If file exists, check if content has changed (compare normalized JSON)
if existingConfig != nil {
// Serialize the existing config to compare
existingNormalized, err := json.MarshalIndent(existingConfig, "", " ")
if err != nil {
return fmt.Errorf("failed to marshal existing config for comparison: %w", err)
}
existingNormalized = append(existingNormalized, '\n')

// Compare normalized JSON - if they're the same, skip writing
if string(newData) == string(existingNormalized) {
devcontainerLog.Printf("No changes detected, skipping write: %s", devcontainerPath)
if verbose {
fmt.Fprintf(os.Stderr, "No changes to devcontainer.json\n")
}
return nil
}
devcontainerLog.Printf("Changes detected, will update file: %s", devcontainerPath)
}
Comment on lines +203 to +221
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

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

Comparison logic flaw: The comparison compares the newly computed config (after merge operations) against existingConfig. However, due to a shallow copy on line 112 (config = *existingConfig), the pointer fields (Customizations, Build) and map field (Features) are shared between config and existingConfig. When these shared structures are modified through config (lines 114-160), existingConfig is also mutated. This causes the comparison to always succeed because both variables point to the same mutated data, making the skip-write optimization ineffective for detecting actual changes.

A more robust approach would be to compare against the original file content before any mutations, or preserve the original existingConfig through deep copying before mutations.

Copilot uses AI. Check for mistakes.

// Use owner-only read/write permissions (0600) for security best practices
if err := os.WriteFile(devcontainerPath, data, 0600); err != nil {
if err := os.WriteFile(devcontainerPath, newData, 0600); err != nil {
return fmt.Errorf("failed to write devcontainer.json: %w", err)
}
devcontainerLog.Printf("Wrote file: %s", devcontainerPath)
Expand Down
93 changes: 93 additions & 0 deletions pkg/cli/devcontainer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,10 +119,27 @@ func TestEnsureDevcontainerConfig(t *testing.T) {
}

// Test that running again doesn't fail (idempotency)
// Get file stat before second run
statBefore, err := os.Stat(devcontainerPath)
if err != nil {
t.Fatalf("Failed to stat devcontainer.json before second run: %v", err)
}

err = ensureDevcontainerConfig(false, []string{})
if err != nil {
t.Fatalf("ensureDevcontainerConfig() should be idempotent, but failed: %v", err)
}

// Get file stat after second run
statAfter, err := os.Stat(devcontainerPath)
if err != nil {
t.Fatalf("Failed to stat devcontainer.json after second run: %v", err)
}

// File modification time should be the same (file should not have been rewritten)
if !statBefore.ModTime().Equal(statAfter.ModTime()) {
t.Error("Expected devcontainer.json to not be rewritten when no changes are needed")
}
}

func TestEnsureDevcontainerConfigWithAdditionalRepos(t *testing.T) {
Expand Down Expand Up @@ -768,3 +785,79 @@ func TestGetCurrentRepoName(t *testing.T) {
t.Error("Expected getCurrentRepoName() to return a non-empty string")
}
}

func TestEnsureDevcontainerConfigNoWriteWhenUnchanged(t *testing.T) {
tmpDir := testutil.TempDir(t, "test-*")

originalDir, err := os.Getwd()
if err != nil {
t.Fatalf("Failed to get current directory: %v", err)
}
defer func() {
_ = os.Chdir(originalDir)
}()

if err := os.Chdir(tmpDir); err != nil {
t.Fatalf("Failed to change to temp directory: %v", err)
}

// Initialize git repo
if err := exec.Command("git", "init").Run(); err != nil {
t.Skip("Git not available")
}

// Configure git and add remote
exec.Command("git", "config", "user.name", "Test User").Run()
exec.Command("git", "config", "user.email", "test@example.com").Run()
exec.Command("git", "remote", "add", "origin", "https://github.com/testorg/testrepo.git").Run()

// Create initial devcontainer.json
err = ensureDevcontainerConfig(false, []string{})
if err != nil {
t.Fatalf("Initial ensureDevcontainerConfig() failed: %v", err)
}

devcontainerPath := filepath.Join(".devcontainer", "devcontainer.json")

// Get file info after first write
firstStat, err := os.Stat(devcontainerPath)
if err != nil {
t.Fatalf("Failed to stat file after first write: %v", err)
}
firstModTime := firstStat.ModTime()

// Read the first content
firstContent, err := os.ReadFile(devcontainerPath)
if err != nil {
t.Fatalf("Failed to read file after first write: %v", err)
}

// Run again with same parameters - should not write
err = ensureDevcontainerConfig(false, []string{})
if err != nil {
t.Fatalf("Second ensureDevcontainerConfig() failed: %v", err)
}

// Get file info after second run
secondStat, err := os.Stat(devcontainerPath)
if err != nil {
t.Fatalf("Failed to stat file after second run: %v", err)
}
secondModTime := secondStat.ModTime()

// Read the second content
secondContent, err := os.ReadFile(devcontainerPath)
if err != nil {
t.Fatalf("Failed to read file after second run: %v", err)
}

// Modification times should be equal (file was not rewritten)
if !firstModTime.Equal(secondModTime) {
t.Errorf("File was rewritten when no changes were needed. First modtime: %v, Second modtime: %v", firstModTime, secondModTime)
}

// Content should be identical
if string(firstContent) != string(secondContent) {
t.Error("File content changed when it should not have")
}
}
Comment on lines +789 to +863
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

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

Missing test coverage for parameter changes: The tests verify that the file isn't rewritten when called with identical parameters, but they don't test whether the file IS correctly updated when parameters change (e.g., different additionalRepos). This is important because the comparison logic has a shallow copy issue that could cause updates to be skipped incorrectly.

Consider adding a test that:

  1. Creates a devcontainer.json with additionalRepos=[]
  2. Runs again with additionalRepos=["new/repo"]
  3. Verifies the file WAS updated and contains the new repo

Copilot uses AI. Check for mistakes.
Loading