fix: Make hook shell optional with OS-based defaults (#3613)

cli/azd/cmd/hooks.go

@@ -129,6 +129,11 @@ func (hra *hooksRunAction) Run(ctx context.Context) (*actions.ActionResult, erro
 		),
 	})
 
+	// Validate hooks and display warnings
+	if err := hra.validateAndWarnHooks(ctx); err != nil {
+		return nil, fmt.Errorf("failed validating hooks: %w", err)
+	}
+
 	// Validate service name
 	if hra.flags.service != "" {
 		if has, err := hra.importManager.HasService(ctx, hra.projectConfig, hra.flags.service); err != nil {
@@ -264,6 +269,44 @@ func (hra *hooksRunAction) execHook(
 	return nil
 }
 
+// Validates hooks and displays warnings for default shell usage and other issues
+func (hra *hooksRunAction) validateAndWarnHooks(ctx context.Context) error {
+	// Collect all hooks from project and services
+	allHooks := make(map[string][]*ext.HookConfig)
+
+	// Add project hooks
+	for hookName, hookConfigs := range hra.projectConfig.Hooks {
+		allHooks[hookName] = append(allHooks[hookName], hookConfigs...)
+	}
+
+	// Add service hooks
+	stableServices, err := hra.importManager.ServiceStable(ctx, hra.projectConfig)
+	if err == nil {
+		for _, service := range stableServices {
+			for hookName, hookConfigs := range service.Hooks {
+				allHooks[hookName] = append(allHooks[hookName], hookConfigs...)
+			}
+		}
+	}
+
+	// Create hooks manager and validate
+	hooksManager := ext.NewHooksManager(hra.projectConfig.Path, hra.commandRunner)
+	validationResult := hooksManager.ValidateHooks(ctx, allHooks)
+
+	// Display any warnings
+	for _, warning := range validationResult.Warnings {
+		hra.console.MessageUxItem(ctx, &ux.WarningMessage{
+			Description: warning.Message,
+		})
+		if warning.Suggestion != "" {
+			hra.console.Message(ctx, warning.Suggestion)
+		}
+		hra.console.Message(ctx, "")
+	}
+
+	return nil
+}
+
 // Overrides the configured hooks from command line flags
 func (hra *hooksRunAction) prepareHook(name string, hook *ext.HookConfig) error {
 	// Enable testing cross platform

cli/azd/pkg/ext/hooks_manager.go

@@ -10,6 +10,7 @@ import (
 	"log"
 	"os"
 	osexec "os/exec"
+	"path/filepath"
 	"runtime"
 	"strings"
 
@@ -142,17 +143,57 @@ func (h *HooksManager) ValidateHooks(ctx context.Context, allHooks map[string][]
 	}
 
 	hasPowerShellHooks := false
+	hasDefaultShellHooks := false
 
-	// Check all hooks
+	// Two-pass validation is required because:
+	// 1. First pass: Set shell defaults and detect inline scripts for each hook configuration
+	// 2. Second pass: Generate warnings only after all hooks have been processed and we know
+	//    the complete state (e.g., whether ANY hook uses PowerShell or default shell)
+	// We cannot merge these loops because warnings depend on global state across all hooks.
+
+	// First pass: perform lightweight validation to set flags like usingDefaultShell
+	// without creating temporary files (which full validation does)
+	for _, hookConfigs := range allHooks {
+		for _, hookConfig := range hookConfigs {
+			// Set the working directory for validation
+			if hookConfig.cwd == "" {
+				hookConfig.cwd = h.cwd
+			}
+
+			// Only perform shell detection for warning purposes, not full validation
+			if !hookConfig.validated && hookConfig.Run != "" {
+				// Check if it's an inline script (no file exists)
+				relativeCheckPath := strings.ReplaceAll(hookConfig.Run, "/", string(os.PathSeparator))
+				fullCheckPath := relativeCheckPath
+				if hookConfig.cwd != "" {
+					fullCheckPath = filepath.Join(hookConfig.cwd, hookConfig.Run)
+				}
+
+				_, err := os.Stat(fullCheckPath)
+				isInlineScript := err != nil // File doesn't exist, so it's inline
+
+				// If shell is not specified and it's an inline script, set default for warning
+				if hookConfig.Shell == ScriptTypeUnknown && isInlineScript {
+					if runtime.GOOS == "windows" {
+						hookConfig.Shell = ShellTypePowershell
+					} else {
+						hookConfig.Shell = ShellTypeBash
+					}
+					hookConfig.usingDefaultShell = true
+				}
+			}
+		}
+	}
+
+	// Second pass: check all hooks for warning conditions using the state set in first pass
 	for _, hookConfigs := range allHooks {
 		for _, hookConfig := range hookConfigs {
 			if hookConfig.IsPowerShellHook() {
 				hasPowerShellHooks = true
-				break
 			}
-		}
-		if hasPowerShellHooks {
-			break
+			if hookConfig.IsUsingDefaultShell() {
+				hasDefaultShellHooks = true
+			}
 		}
 	}
 
@@ -183,5 +224,33 @@ func (h *HooksManager) ValidateHooks(ctx context.Context, allHooks map[string][]
 		}
 	}
 
+	// If we found hooks using default shell, warn the user
+	if hasDefaultShellHooks {
+		var warningMessage string
+		var defaultShell string
+
+		if runtime.GOOS == "windows" {
+			defaultShell = "pwsh"
+		} else {
+			defaultShell = "sh"
+		}
+
+		warningMessage = fmt.Sprintf(
+			"Hook configurations found without explicit shell specification. Using OS default shell '%s'. "+
+				"For better reliability, consider specifying the shell explicitly in your hook configuration.\n"+
+				"More about using hooks: %s",
+			defaultShell,
+			output.WithHyperlink("aka.ms/azd-hooks", "aka.ms/azd-hooks"),
+		)
+
+		result.Warnings = append(result.Warnings, HookWarning{
+			Message: warningMessage,
+			Suggestion: fmt.Sprintf(
+				"Add 'shell: %s' to your hook configurations to remove this warning.",
+				defaultShell,
+			),
+		})
+	}
+
 	return result
 }

cli/azd/pkg/ext/hooks_manager_test.go

@@ -4,13 +4,15 @@
 package ext
 
 import (
+	"context"
 	"os"
 	"path/filepath"
 	"regexp"
 	"strings"
 	"testing"
 
 	"github.com/azure/azure-dev/cli/azd/pkg/osutil"
+	"github.com/azure/azure-dev/cli/azd/test/mocks"
 	"github.com/azure/azure-dev/cli/azd/test/mocks/mockexec"
 	"github.com/azure/azure-dev/cli/azd/test/ostest"
 	"github.com/stretchr/testify/require"
@@ -45,16 +47,18 @@ func Test_GetAllHookConfigs(t *testing.T) {
 	})
 
 	t.Run("With Invalid Configuration", func(t *testing.T) {
-		// All hooksMap are invalid because they are missing a script type
+		// All hooksMap are invalid because they are missing the run parameter
 		hooksMap := map[string][]*HookConfig{
 			"preinit": {
 				{
-					Run: "echo 'Hello'",
+					Shell: ShellTypeBash,
+					// Run is missing - this should cause an error
 				},
 			},
 			"postinit": {
 				{
-					Run: "echo 'Hello'",
+					Shell: ShellTypeBash,
+					// Run is missing - this should cause an error
 				},
 			},
 		}
@@ -115,16 +119,18 @@ func Test_GetByParams(t *testing.T) {
 	})
 
 	t.Run("With Invalid Configuration", func(t *testing.T) {
-		// All hooksMap are invalid because they are missing a script type
+		// All hooksMap are invalid because they are missing the run parameter
 		hooksMap := map[string][]*HookConfig{
 			"preinit": {
 				{
-					Run: "echo 'Hello'",
+					Shell: ShellTypeBash,
+					// Run is missing - this should cause an error
 				},
 			},
 			"postinit": {
 				{
-					Run: "echo 'Hello'",
+					Shell: ShellTypeBash,
+					// Run is missing - this should cause an error
 				},
 			},
 		}
@@ -140,6 +146,88 @@ func Test_GetByParams(t *testing.T) {
 	})
 }
 
+func Test_HookConfig_DefaultShell(t *testing.T) {
+	tests := []struct {
+		name             string
+		hookConfig       *HookConfig
+		expectedShell    ShellType
+		expectingDefault bool
+	}{
+		{
+			name: "No shell specified - should use OS default",
+			hookConfig: &HookConfig{
+				Name: "test",
+				Run:  "echo 'hello'",
+			},
+			expectedShell:    getDefaultShellForOS(),
+			expectingDefault: true,
+		},
+		{
+			name: "Shell explicitly specified - should not use default",
+			hookConfig: &HookConfig{
+				Name:  "test",
+				Shell: ShellTypeBash,
+				Run:   "echo 'hello'",
+			},
+			expectedShell:    ShellTypeBash,
+			expectingDefault: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Clone the config to avoid modifying the test case
+			config := *tt.hookConfig
+			config.cwd = t.TempDir()
+
+			err := config.validate()
+			require.NoError(t, err)
+
+			require.Equal(t, tt.expectedShell, config.Shell)
+			require.Equal(t, tt.expectingDefault, config.IsUsingDefaultShell())
+		})
+	}
+}
+
+func Test_HooksManager_ValidateDefaultShellWarning(t *testing.T) {
+	tempDir := t.TempDir()
+	mockContext := mocks.NewMockContext(context.Background())
+	hooksManager := NewHooksManager(tempDir, mockContext.CommandRunner)
+
+	// Create hooks without explicit shell configuration
+	// DON'T pre-validate - let ValidateHooks do the validation and warning detection
+	hooksWithoutShell := map[string][]*HookConfig{
+		"prebuild": {
+			{
+				Name: "prebuild",
+				Run:  "echo 'Building...'",
+				// No Shell specified - should trigger default shell warning
+				// No cwd specified - ValidateHooks should set it
+			},
+		},
+	}
+
+	// ValidateHooks should validate the hooks internally and detect default shell usage
+	result := hooksManager.ValidateHooks(context.Background(), hooksWithoutShell)
+
+	// Should have at least one warning about default shell usage
+	hasDefaultShellWarning := false
+	for _, warning := range result.Warnings {
+		if strings.Contains(warning.Message, "Hook configurations found without explicit shell specification") {
+			hasDefaultShellWarning = true
+			break
+		}
+	}
+
+	require.True(t, hasDefaultShellWarning, "Expected warning about default shell usage")
+
+	// Also verify that the hook was actually validated and has the default shell set
+	hook := hooksWithoutShell["prebuild"][0]
+	require.True(t, hook.IsUsingDefaultShell(), "Hook should be marked as using default shell")
+	expectedShell := getDefaultShellForOS()
+	require.Equal(t, expectedShell, hook.Shell, "Hook should have the OS default shell")
+}
+
 func ensureScriptsExist(t *testing.T, configs map[string][]*HookConfig) {
 	for _, hooks := range configs {
 		for _, hook := range hooks {

cli/azd/pkg/ext/hooks_runner_test.go

@@ -450,12 +450,12 @@ func Test_GetScript_Validation(t *testing.T) {
 
 	scriptValidations := []scriptValidationTest{
 		{
-			name: "Missing Script Type",
+			name: "Missing Script Type - Should Use Default Shell",
 			config: &HookConfig{
 				Name: "test1",
 				Run:  "echo 'Hello'",
 			},
-			expectedError: ErrScriptTypeUnknown,
+			expectedError: nil, // Should no longer error, should use default shell
 		},
 		{
 			name: "Missing Run param",

cli/azd/pkg/ext/models.go

@@ -59,6 +59,8 @@ type HookConfig struct {
 	cwd string
 	// When location is `inline` a script must be defined inline
 	script string
+	// Indicates if the shell was automatically detected based on OS (used for warnings)
+	usingDefaultShell bool
 
 	// Internal name of the hook running for a given command
 	Name string `yaml:",omitempty"`
@@ -104,8 +106,10 @@ func (hc *HookConfig) validate() error {
 		hc.script = hc.Run
 	}
 
+	// If shell is not specified and it's an inline script, use OS default
 	if hc.Shell == ScriptTypeUnknown && hc.path == "" {
-		return ErrScriptTypeUnknown
+		hc.Shell = getDefaultShellForOS()
+		hc.usingDefaultShell = true
 	}
 
 	if hc.location == ScriptLocationUnknown {
@@ -164,6 +168,12 @@ func (hc *HookConfig) IsPowerShellHook() bool {
 	return false
 }
 
+// IsUsingDefaultShell returns true if the hook is using the OS default shell
+// because no shell was explicitly configured
+func (hc *HookConfig) IsUsingDefaultShell() bool {
+	return hc.usingDefaultShell
+}
+
 func InferHookType(name string) (HookType, string) {
 	// Validate name length so go doesn't PANIC for string slicing below
 	if len(name) < 4 {
@@ -177,6 +187,14 @@ func InferHookType(name string) (HookType, string) {
 	return HookTypeNone, name
 }
 
+// getDefaultShellForOS returns the default shell type based on the operating system
+func getDefaultShellForOS() ShellType {
+	if runtime.GOOS == "windows" {
+		return ShellTypePowershell
+	}
+	return ShellTypeBash
+}
+
 func inferScriptTypeFromFilePath(path string) (ShellType, error) {
 	fileExtension := filepath.Ext(path)
 	switch fileExtension {