pkg/duration/duration_test.go (1)

55-61: Assert the sentinel error for invalid inputs.

These tests can be tighter by asserting ErrInvalidDuration via ErrorIs, not just any error. As per coding guidelines, use errors.Is() semantics in tests.

✅ Suggested test tightening

 import (
 	"testing"
 	"time"

+	errUtils "github.com/cloudposse/atmos/errors"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@
 		t.Run(tt.name, func(t *testing.T) {
 			result, err := Parse(tt.input)
 			if tt.wantErr {
 				assert.Error(t, err, "expected error for input %q", tt.input)
+				assert.ErrorIs(t, err, errUtils.ErrInvalidDuration)
 			} else {
 				require.NoError(t, err, "unexpected error for input %q", tt.input)
 				assert.Equal(t, tt.expected, result, "unexpected result for input %q", tt.input)
 			}
 		})
@@
 		t.Run(tt.name, func(t *testing.T) {
 			result, err := ParseDuration(tt.input)
 			if tt.wantErr {
 				assert.Error(t, err, "expected error for input %q", tt.input)
+				assert.ErrorIs(t, err, errUtils.ErrInvalidDuration)
 			} else {
 				require.NoError(t, err, "unexpected error for input %q", tt.input)
 				assert.Equal(t, tt.expected, result, "unexpected result for input %q", tt.input)
 			}
 		})

Also applies to: 86-91

pkg/provisioner/workdir/fs.go (1)

138-167: Verify empty directory cleanup.

After deleting files, empty directories in dst (except AtmosDir) may remain. This could accumulate over time if source directories are removed.

♻️ Optional: Clean up empty directories after file deletion

 func deleteRemovedFiles(dst string, srcFiles map[string]bool) (bool, error) {
 	anyDeleted := false
+	dirsToCheck := []string{}
 
 	err := filepath.WalkDir(dst, func(path string, d fs.DirEntry, err error) error {
 		// ... existing logic ...
 
 		if d.IsDir() || srcFiles[relPath] {
+			if d.IsDir() && relPath != "." && relPath != AtmosDir {
+				dirsToCheck = append(dirsToCheck, path)
+			}
 			return nil
 		}
 
 		anyDeleted = true
 		return os.Remove(path)
 	})
+
+	// Remove empty directories (in reverse order for deepest-first)
+	for i := len(dirsToCheck) - 1; i >= 0; i-- {
+		_ = os.Remove(dirsToCheck[i]) // Silently fails if not empty
+	}
 
 	return anyDeleted, err
 }

tests/cli_jit_source_workdir_test.go (1)

26-92: Test design is solid and effectively validates the source precedence fix.

The marker file approach clearly distinguishes local vs remote content, and the assertions properly verify provisioning succeeded. The fixture exists and the test setup is clean.

The error from cmd.Execute() is safe to ignore—if provisioning fails, the require.NoError(t, err, ...) on os.Stat(workdirPath) catches it. That said, logging the error would help with debugging when things go wrong:

♻️ Capture error from Execute for better debugging

 	// Execute - this may error due to terraform not being configured,
 	// but the provisioning should have been attempted.
-	_ = cmd.Execute()
+	err := cmd.Execute()
+	if err != nil {
+		t.Logf("Execute returned error (expected for incomplete terraform setup): %v", err)
+	}

pkg/provisioner/workdir/metadata_lock_windows.go (1)

31-47: Inconsistent error handling compared to Unix implementation.

The Unix version wraps errors with errUtils.Build(errUtils.ErrWorkdirMetadata) for both read and unmarshal failures, but this Windows version returns raw errors. Consider aligning for consistent error types across platforms.

♻️ Suggested alignment with Unix error handling

+import (
+	"encoding/json"
+	"os"
+	"time"
+
+	errUtils "github.com/cloudposse/atmos/errors"
+)

 func loadMetadataWithReadLockWindows(metadataFile string) (*WorkdirMetadata, error) {
 	// On Windows, skip read locks entirely to avoid timeout issues.
 	data, err := os.ReadFile(metadataFile)
 	if err != nil {
 		if os.IsNotExist(err) {
 			return nil, nil
 		}
-		return nil, err
+		return nil, errUtils.Build(errUtils.ErrWorkdirMetadata).
+			WithCause(err).
+			WithExplanation("Failed to read metadata file").
+			WithContext("path", metadataFile).
+			Err()
 	}

 	var metadata WorkdirMetadata
 	if err := json.Unmarshal(data, &metadata); err != nil {
-		return nil, err
+		return nil, errUtils.Build(errUtils.ErrWorkdirMetadata).
+			WithCause(err).
+			WithExplanation("Failed to unmarshal metadata").
+			WithContext("path", metadataFile).
+			Err()
 	}

 	return &metadata, nil
 }

pkg/provisioner/workdir/metadata.go (1)

24-41: Minor TOCTOU race in ReadMetadata.

The os.Stat check at line 30 and subsequent read via loadMetadataWithReadLock has a small race window where the file could be deleted between checks. Since loadMetadataWithReadLock already handles os.IsNotExist, consider simplifying:

♻️ Remove redundant stat checks

 func ReadMetadata(workdirPath string) (*WorkdirMetadata, error) {
 	defer perf.Track(nil, "workdir.ReadMetadata")()
 
 	metadataPath := MetadataPath(workdirPath)
 
-	// Check if metadata file exists.
-	if _, err := os.Stat(metadataPath); os.IsNotExist(err) {
-		// Try legacy location.
-		legacyPath := filepath.Join(workdirPath, WorkdirMetadataFile)
-		if _, err := os.Stat(legacyPath); os.IsNotExist(err) {
-			return nil, nil
-		}
-		metadataPath = legacyPath
-	}
-
-	// Use read lock for concurrent safety.
-	return loadMetadataWithReadLock(metadataPath)
+	// Try new location first.
+	metadata, err := loadMetadataWithReadLock(metadataPath)
+	if err != nil {
+		return nil, err
+	}
+	if metadata != nil {
+		return metadata, nil
+	}
+
+	// Try legacy location.
+	legacyPath := filepath.Join(workdirPath, WorkdirMetadataFile)
+	return loadMetadataWithReadLock(legacyPath)
 }

pkg/provisioner/workdir/metadata_lock_unix.go (1)

57-61: Variable shadowing in defer.

The err in the defer shadows the outer err variable. While it works here since the outer err isn't used after, it's a subtle gotcha.

♻️ Rename to avoid shadowing

 	defer func() {
-		if err := lock.Unlock(); err != nil {
-			log.Trace("Failed to unlock metadata file", "error", err, "path", lockPath)
+		if unlockErr := lock.Unlock(); unlockErr != nil {
+			log.Trace("Failed to unlock metadata file", "error", unlockErr, "path", lockPath)
 		}
 	}()

cmd/terraform/workdir/workdir_helpers.go (1)

151-160: Consider distinguishing metadata error vs not found.

When ReadMetadata returns (nil, nil), the current code wraps with WithCause(nil). This works but loses the distinction between "no metadata file" and "error reading metadata". A minor clarity improvement:

♻️ Handle nil metadata explicitly

 	metadata, err := provWorkdir.ReadMetadata(workdirPath)
-	if err != nil || metadata == nil {
+	if err != nil {
+		return nil, errUtils.Build(errUtils.ErrWorkdirMetadata).
+			WithCause(err).
+			WithExplanation(fmt.Sprintf("Failed to read workdir metadata for component '%s' in stack '%s'", component, stack)).
+			WithContext("component", component).
+			WithContext("stack", stack).
+			Err()
+	}
+	if metadata == nil {
 		return nil, errUtils.Build(errUtils.ErrWorkdirMetadata).
-			WithCause(err).
 			WithExplanation(fmt.Sprintf("Workdir not found for component '%s' in stack '%s'", component, stack)).
 			WithHint("Run 'atmos terraform init' to create the workdir").
 			WithContext("component", component).
 			WithContext("stack", stack).
 			Err()
 	}

pkg/provisioner/workdir/metadata_lock_windows.go (2)

22-25: Fixed delay adds latency regardless of need.

The 50ms sleep runs after every operation. For bulk workdir operations, this accumulates. Consider making this configurable or only applying the delay when the function actually performed writes.

♻️ Potential optimization

-func withMetadataFileLockWindows(metadataFile string, fn func() error) error {
-	// No file locking on Windows to avoid timeout issues.
-	// The metadata is non-critical functionality, so we can operate
-	// without strict locking on Windows.
-
-	// Add a small delay after operations to let Windows release file handles.
-	defer func() {
-		time.Sleep(50 * time.Millisecond)
-	}()
-
-	// Just execute the function without any locking.
-	return fn()
+func withMetadataFileLockWindows(_ string, fn func() error) error {
+	// No file locking on Windows to avoid timeout issues.
+	// The metadata is non-critical functionality, so we can operate
+	// without strict locking on Windows.
+	return fn()
 }

---

41-44: Consider wrapping unmarshal errors with context.

Per coding guidelines, errors should be wrapped with context using fmt.Errorf or the error builder. The raw unmarshal error lacks information about which file failed.

♻️ Add error context

 	var metadata WorkdirMetadata
 	if err := json.Unmarshal(data, &metadata); err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to unmarshal metadata from %s: %w", metadataFile, err)
 	}

pkg/duration/duration.go (1)

27-33: Consider adding 'w' for weeks suffix.

The keywordDurations map includes "weekly", but unitMultipliers doesn't support 'w'. Users might expect "1w" to work alongside "7d".

♻️ Add week suffix support

 var unitMultipliers = map[byte]int64{
 	's': 1,
 	'm': secondsPerMinute,
 	'h': secondsPerHour,
 	'd': secondsPerDay,
+	'w': secondsPerWeek,
 }

tests/cli_jit_source_workdir_test.go (2)

61-63: Consider capturing execution errors for debugging.

The return value is intentionally ignored since terraform may error without full configuration. However, logging or storing the error could help debug test failures.

♻️ Optional improvement

 	// Execute - this may error due to terraform not being configured,
 	// but the provisioning should have been attempted.
-	_ = cmd.Execute()
+	err := cmd.Execute()
+	t.Logf("cmd.Execute() returned: %v", err) // Helps debug test failures

---

113-125: Test coverage could be expanded.

This test verifies terraform source describe succeeds but doesn't assert on the output. Consider adding assertions on the describe output to verify source configuration is correctly recognized, or documenting why this is sufficient as a smoke test.

pkg/provisioner/source/provision_hook_test.go (1)

191-331: Add coverage for the non-workdir branch in needsProvisioning.

TestNeedsProvisioning always passes isWorkdir=true, so the new isWorkdir=false path isn’t exercised. Consider adding a small case to lock the “existing non-empty dir → no provisioning” behavior. As per coding guidelines, new features should include comprehensive unit tests.

♻️ Suggested test addition

@@
 func TestNeedsProvisioning(t *testing.T) {
@@
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			path := tt.setup(t)
 			// Test with isWorkdir=true since these tests verify metadata behavior.
 			result, reason := needsProvisioning(path, tt.sourceSpec, true)
 			assert.Equal(t, tt.expected, result)
 			assert.Equal(t, tt.expectedReason, reason)
 		})
 	}
+
+	t.Run("non-workdir with existing files", func(t *testing.T) {
+		tempDir := t.TempDir()
+		dirPath := filepath.Join(tempDir, "component")
+		err := os.MkdirAll(dirPath, 0o755)
+		require.NoError(t, err)
+		err = os.WriteFile(filepath.Join(dirPath, "main.tf"), []byte("# test"), 0o644)
+		require.NoError(t, err)
+
+		result, reason := needsProvisioning(dirPath, sourceSpec, false)
+		assert.False(t, result)
+		assert.Empty(t, reason)
+	})
 }

pkg/provisioner/workdir/clean_test.go (1)

286-413: Consider table-driven consolidation for expired-clean scenarios.

These cases share setup patterns and assertions; a table-driven structure would reduce duplication and ease expansion. As per coding guidelines, prefer table-driven tests when exercising multiple scenarios.

internal/exec/packer_output.go (1)

64-66: Wrap auto-provision error with sentinel (duplicate).

This was flagged in a previous review. Line 65 should join with errUtils.ErrProvisionSource or similar sentinel before adding context to support errors.Is checks.

cmd/terraform/workdir/workdir_helpers.go (1)

118-123: Don’t silently drop metadata read errors in ListWorkdirs.
Right now err != nil is treated the same as “no metadata,” which can mask corruption or permission problems. Consider returning a wrapped error on err != nil but still skipping when metadata == nil.

🛠️ Suggested tweak

-		metadata, err := provWorkdir.ReadMetadata(workdirPath)
-		if err != nil || metadata == nil {
-			// Skip directories with invalid or missing metadata.
-			// This allows the list operation to succeed even if some workdirs are corrupt.
-			continue
-		}
+		metadata, err := provWorkdir.ReadMetadata(workdirPath)
+		if err != nil {
+			return nil, errUtils.Build(errUtils.ErrWorkdirMetadata).
+				WithCause(err).
+				WithExplanation("Failed to read workdir metadata").
+				WithContext("path", workdirPath).
+				Err()
+		}
+		if metadata == nil {
+			// Skip directories without metadata.
+			continue
+		}

As per coding guidelines, errors should be wrapped with context.

examples/source-provisioning/README.md (1)

27-31: Clarify the version‑pinning claim.
The bullet says remote URIs are version‑pinned, but the example doesn’t show a version. Consider softening the wording or adding a version in the example.

Proposed tweak

-2. **Remote URIs** - GitHub URLs with version pinning
+2. **Remote URIs** - GitHub URLs (optionally version‑pinned)

pkg/provisioner/workdir/fs_test.go (1)

14-71: Consider table-driven cases for fileNeedsCopy.
These scenarios share setup and assertions; a table-driven test would trim repetition and scale better. As per coding guidelines, prefer table-driven tests for multi-scenario coverage.

pkg/provisioner/workdir/clean_test.go (1)

286-413: Consider table-driven tests for the TTL cleanup scenarios.
These cases are parallel and would be easier to extend as a single table. As per coding guidelines, prefer table-driven tests for multi-scenario coverage.

cmd/terraform/workdir/workdir_show_test.go (1)

470-484: Consider asserting SourceType defaults correctly.

This test verifies zero LastAccessed isn't displayed, but doesn't set SourceType. If printShowHuman has conditional logic based on SourceType, you may want to explicitly test local vs unset SourceType behavior separately. Not blocking, just a thought for completeness.

pkg/provisioner/workdir/metadata_lock_unix_test.go (1)

48-75: Potential test flakiness with exclusive locks.

The assertion at line 74 requires at least one goroutine to succeed. If the locking implementation uses non-blocking LOCK_NB and the timing is unfortunate, all 5 goroutines could theoretically fail. Consider either:

• Removing the assertion entirely (the test still verifies no crashes/deadlocks)
• Running goroutines sequentially with a smaller subset to guarantee success

Not blocking since the probability is low, but worth noting.

cmd/terraform/workdir/workdir_show.go (1)

75-86: Consider using SourceTypeRemote constant instead of magic string.

Line 90 compares sourceType == "remote" directly. The workdir package defines SourceTypeRemote constant which would be more maintainable.

♻️ Suggested improvement

+import provWorkdir "github.com/cloudposse/atmos/pkg/provisioner/workdir"
+
 // In printShowHuman:
-if sourceType == "remote" {
+if sourceType == string(provWorkdir.SourceTypeRemote) {

cmd/terraform/workdir/workdir_clean_cmd_test.go (1)

475-535: Consider a table‑driven structure for RunE validation cases.
The validation tests are very similar; a small table of flag/args permutations would cut repetition and make coverage easier to extend.

As per coding guidelines, prefer table-driven tests for multiple scenarios.

tests/cli_jit_source_workdir_test.go (2)

92-123: Placeholder test provides limited coverage.

Lines 121-122 explicitly call this a "placeholder." The test only confirms source describe doesn't error - it doesn't verify source precedence behavior.

Consider either:

• Adding meaningful assertions (e.g., workdir state, output verification)
• Removing this test since the first test already covers the behavior

---

294-334: Inconsistent assertions compared to other tests.

TestJITSource_PackerOutput and TestJITSource_HelmfileGenerateVarfile lack the workdir existence and content verification that TestJITSource_GenerateVarfile and TestJITSource_GenerateBackend have.

For consistency, add workdir assertions:

♻️ Suggested additions

For TestJITSource_PackerOutput (after line 315):

+	// Verify workdir was provisioned by JIT source.
+	workdirPath := filepath.Join(".workdir", "packer", "dev-ami-workdir")
+	info, statErr := os.Stat(workdirPath)
+	require.NoError(t, statErr, "Workdir should exist at %s (JIT provisioning should have run)", workdirPath)
+	require.True(t, info.IsDir(), "Workdir path should be a directory")

For TestJITSource_HelmfileGenerateVarfile (after line 333):

+	// Verify workdir was provisioned.
+	workdirPath := filepath.Join(".workdir", "helmfile", "dev-nginx-workdir")
+	info, err := os.Stat(workdirPath)
+	require.NoError(t, err, "Workdir should exist at %s", workdirPath)
+	require.True(t, info.IsDir(), "Workdir path should be a directory")

cmd/terraform/workdir/workdir_clean_cmd_test.go (1)

465-476: Use cmd.NewTestKit(t) to isolate RootCmd/Viper state.

The helper bypasses the required testkit reset and can leak global state across cmd tests. Consider restructuring tests (e.g., external test package or shared helper that avoids the circular import) so cmd.NewTestKit(t) runs before touching Viper. As per coding guidelines, ...

pkg/provisioner/workdir/fs_test.go (2)

3-10: Use runtime.GOOS for OS checks.

os.Getenv("GOOS") is non-idiomatic and may be unset; runtime.GOOS is the canonical, reliable check. Consider updating both Windows skips accordingly.

🔧 Suggested change

 import (
 	"os"
 	"path/filepath"
+	"runtime"
 	"testing"

 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@
-	if os.Getenv("GOOS") == "windows" || filepath.Separator == '\\' {
+	if runtime.GOOS == "windows" {
 		t.Skip("Skipping permission test on Windows - Unix file permissions not supported")
 	}
@@
-	if os.Getenv("GOOS") == "windows" || filepath.Separator == '\\' {
+	if runtime.GOOS == "windows" {
 		t.Skip("Skipping permission test on Windows - Unix file permissions not supported")
 	}

Also applies to: 49-52, 199-202

---

14-74: Consider table-driven tests to reduce repetition.

These scenarios are great, but the repeated setup makes the file longer than needed. A table-driven approach for fileNeedsCopy and copyFile would align with test guidelines and improve maintainability. As per coding guidelines, ...

Also applies to: 155-220

pkg/provisioner/workdir/workdir.go (1)

184-184: Silent discard of ReadMetadata error may hide real failures.

Ignoring the error is fine when metadata doesn't exist yet. However, this also silences permission errors or corrupt JSON. A warning log would help debugging without breaking the flow.

-	existingMetadata, _ := ReadMetadata(workdirPath)
+	existingMetadata, err := ReadMetadata(workdirPath)
+	if err != nil {
+		ui.Warning(fmt.Sprintf("Failed to read existing metadata (will recreate): %s", err))
+	}

cmd/terraform/workdir/workdir_clean_cmd_test.go (1)

478-538: Consolidate RunE validation tests into a table-driven test.

These cases are repetitive and easier to maintain as a single table-driven test. As per coding guidelines, ...

♻️ Suggested refactor

-func TestCleanCmd_RunE_ExpiredWithoutTTL(t *testing.T) {
-	resetViperForTest(t)
-	v := viper.GetViper()
-	v.Set("expired", true)
-	v.Set("ttl", "") // No TTL provided.
-
-	err := cleanCmd.RunE(cleanCmd, []string{})
-	assert.Error(t, err)
-	assert.ErrorIs(t, err, errUtils.ErrWorkdirClean)
-}
-
-func TestCleanCmd_RunE_ExpiredWithAll(t *testing.T) {
-	resetViperForTest(t)
-	v := viper.GetViper()
-	v.Set("expired", true)
-	v.Set("ttl", "7d")
-	v.Set("all", true) // Cannot use --all with --expired.
-
-	err := cleanCmd.RunE(cleanCmd, []string{})
-	assert.Error(t, err)
-	assert.ErrorIs(t, err, errUtils.ErrWorkdirClean)
-}
-
-func TestCleanCmd_RunE_ExpiredWithComponent(t *testing.T) {
-	resetViperForTest(t)
-	v := viper.GetViper()
-	v.Set("expired", true)
-	v.Set("ttl", "7d")
-
-	err := cleanCmd.RunE(cleanCmd, []string{"vpc"}) // Component arg with --expired.
-	assert.Error(t, err)
-	assert.ErrorIs(t, err, errUtils.ErrWorkdirClean)
-}
-
-func TestCleanCmd_RunE_ActualCall_AllWithComponent(t *testing.T) {
-	resetViperForTest(t)
-	v := viper.GetViper()
-	v.Set("all", true)
-
-	err := cleanCmd.RunE(cleanCmd, []string{"vpc"}) // --all with component arg.
-	assert.Error(t, err)
-	assert.ErrorIs(t, err, errUtils.ErrWorkdirClean)
-}
-
-func TestCleanCmd_RunE_ActualCall_NoFlagsNoArgs(t *testing.T) {
-	resetViperForTest(t)
-
-	err := cleanCmd.RunE(cleanCmd, []string{}) // No flags, no args.
-	assert.Error(t, err)
-	assert.ErrorIs(t, err, errUtils.ErrWorkdirClean)
-}
-
-func TestCleanCmd_RunE_ActualCall_ComponentWithoutStack(t *testing.T) {
-	resetViperForTest(t)
-	v := viper.GetViper()
-	v.Set("stack", "") // No stack provided.
-
-	err := cleanCmd.RunE(cleanCmd, []string{"vpc"}) // Component without --stack.
-	assert.Error(t, err)
-	assert.ErrorIs(t, err, errUtils.ErrWorkdirClean)
-}
+func TestCleanCmd_RunE_Validation(t *testing.T) {
+	cases := []struct {
+		name string
+		setup func(v *viper.Viper)
+		args  []string
+	}{
+		{
+			name: "expired without ttl",
+			setup: func(v *viper.Viper) {
+				v.Set("expired", true)
+				v.Set("ttl", "")
+			},
+			args: []string{},
+		},
+		{
+			name: "expired with all",
+			setup: func(v *viper.Viper) {
+				v.Set("expired", true)
+				v.Set("ttl", "7d")
+				v.Set("all", true)
+			},
+			args: []string{},
+		},
+		{
+			name: "expired with component",
+			setup: func(v *viper.Viper) {
+				v.Set("expired", true)
+				v.Set("ttl", "7d")
+			},
+			args: []string{"vpc"},
+		},
+		{
+			name: "all with component",
+			setup: func(v *viper.Viper) {
+				v.Set("all", true)
+			},
+			args: []string{"vpc"},
+		},
+		{
+			name: "no flags no args",
+			setup: func(v *viper.Viper) {},
+			args: []string{},
+		},
+		{
+			name: "component without stack",
+			setup: func(v *viper.Viper) {
+				v.Set("stack", "")
+			},
+			args: []string{"vpc"},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			resetViperForTest(t)
+			v := viper.GetViper()
+			tc.setup(v)
+
+			err := cleanCmd.RunE(cleanCmd, tc.args)
+			assert.Error(t, err)
+			assert.ErrorIs(t, err, errUtils.ErrWorkdirClean)
+		})
+	}
+}

pkg/provisioner/workdir/workdir_test.go (1)

1194-1278: Consider table-driven BuildLocalMetadata tests.

These three cases are structurally similar and could be consolidated to reduce duplication and make future additions easier.

As per coding guidelines, ...