fix: address code review findings for script provisioning provider

- Fix ContinueOnError bug: platform override no longer unconditionally
  resets the field when the override doesn't set it
- Fix getAzdEnv: propagate context cancellation, warn on other errors
  instead of silently swallowing all failures
- Add 10MB size limit on outputs.json to prevent OOM
- Remove dead ScriptResult fields (Stdout/Stderr never populated)
- Remove unused exported functions (OutputsToEnvMap, OutputsToProvisioning)
- Extract toProtoOutputs helper to reduce duplication
- Improve shBinary() portability: use LookPath with /bin/sh fallback
- Add platform override tests (ContinueOnError preservation, env merge)
- Add executor unit tests (buildShellCommand, mapToEnvSlice)
- Fix hardcoded /tmp path in test to use t.TempDir()
- Update README: clarify secrets are plain values in alpha

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: wbreza

cli/azd/extensions/microsoft.azd.scripts/README.md

@@ -46,7 +46,7 @@ infra:
 | `kind` | string | No | Script type: `sh` or `pwsh` (auto-detected from extension) |
 | `name` | string | No | Display name for progress reporting |
 | `env` | map | No | Environment variables with `${EXPRESSION}` substitution |
-| `secrets` | map | No | Secret references (`akvs://vault/secret`) |
+| `secrets` | map | No | Secret key-value pairs (plain values in alpha; Key Vault resolution planned) |
 | `continueOnError` | bool | No | Continue execution on script failure |
 | `windows` | object | No | Windows-specific overrides |
 | `posix` | object | No | Linux/macOS-specific overrides |

cli/azd/extensions/microsoft.azd.scripts/internal/provisioning/config.go

@@ -178,7 +178,9 @@ func applyPlatformOverride(sc *ScriptConfig) {
 		}
 		maps.Copy(sc.Secrets, override.Secrets)
 	}
-	sc.ContinueOnError = override.ContinueOnError
+	if override.ContinueOnError {
+		sc.ContinueOnError = override.ContinueOnError
+	}
 }
 
 // normalizeKind resolves the script kind from the Kind, Shell, or file extension.

cli/azd/extensions/microsoft.azd.scripts/internal/provisioning/config_test.go

@@ -51,7 +51,7 @@ func TestParseProviderConfig_EmptyConfig(t *testing.T) {
 	cfg, err := ParseProviderConfig(nil)
 	require.NoError(t, err)
 
-	err = cfg.Validate("/tmp")
+	err = cfg.Validate(t.TempDir())
 	require.ErrorContains(t, err, "at least one")
 }
 
@@ -177,6 +177,46 @@ func TestValidateScriptConfig_ContinueOnError(t *testing.T) {
 	require.True(t, cfg.Provision[0].ContinueOnError)
 }
 
+func TestApplyPlatformOverride_DoesNotResetContinueOnError(t *testing.T) {
+	sc := &ScriptConfig{
+		Kind:            "sh",
+		Run:             "fallback.sh",
+		ContinueOnError: true,
+		Posix: &ScriptConfig{
+			Run: "linux-specific.sh",
+		},
+		Windows: &ScriptConfig{
+			Run: "windows-specific.sh",
+		},
+	}
+
+	applyPlatformOverride(sc)
+
+	// ContinueOnError should NOT be reset to false by a platform override
+	// that doesn't explicitly set it.
+	require.True(t, sc.ContinueOnError, "ContinueOnError should not be reset by platform override")
+}
+
+func TestApplyPlatformOverride_MergesEnvMaps(t *testing.T) {
+	sc := &ScriptConfig{
+		Kind: "sh",
+		Run:  "base.sh",
+		Env:  map[string]string{"A": "base", "B": "base"},
+		Posix: &ScriptConfig{
+			Env: map[string]string{"B": "override", "C": "new"},
+		},
+		Windows: &ScriptConfig{
+			Env: map[string]string{"B": "win-override", "D": "win-new"},
+		},
+	}
+
+	applyPlatformOverride(sc)
+
+	require.Equal(t, "base", sc.Env["A"], "base env preserved")
+	// Depending on platform, B should be overridden
+	require.NotEmpty(t, sc.Env["B"])
+}
+
 func createFile(t *testing.T, base, relPath, content string) {
 	t.Helper()
 	fullPath := filepath.Join(base, relPath)

cli/azd/extensions/microsoft.azd.scripts/internal/provisioning/executor.go

@@ -16,8 +16,6 @@ import (
 // ScriptResult holds the outcome of a single script execution.
 type ScriptResult struct {
 	ExitCode int
-	Stdout   string
-	Stderr   string
 }
 
 // ScriptExecutor runs shell scripts with a prepared environment.
@@ -79,13 +77,15 @@ func pwshBinary() string {
 
 func shBinary() string {
 	if runtime.GOOS == "windows" {
-		// Prefer Git Bash on Windows
 		if gitBash, err := exec.LookPath("bash.exe"); err == nil {
 			return gitBash
 		}
 		return "bash.exe"
 	}
-	return "/bin/bash"
+	if bash, err := exec.LookPath("bash"); err == nil {
+		return bash
+	}
+	return "/bin/sh"
 }
 
 // mapToEnvSlice converts a map to the KEY=VALUE slice format expected by exec.Cmd.Env.

cli/azd/extensions/microsoft.azd.scripts/internal/provisioning/executor_test.go

@@ -0,0 +1,42 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+package provisioning
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestBuildShellCommand_Sh(t *testing.T) {
+	shell, args := buildShellCommand("sh", "/path/to/script.sh")
+	require.NotEmpty(t, shell)
+	require.Contains(t, args, "/path/to/script.sh")
+}
+
+func TestBuildShellCommand_Pwsh(t *testing.T) {
+	shell, args := buildShellCommand("pwsh", "/path/to/script.ps1")
+	require.Contains(t, shell, "pwsh")
+	require.Contains(t, args, "-NoProfile")
+	require.Contains(t, args, "-NonInteractive")
+	require.Contains(t, args, "-File")
+	require.Contains(t, args, "/path/to/script.ps1")
+}
+
+func TestMapToEnvSlice(t *testing.T) {
+	env := map[string]string{
+		"FOO": "bar",
+		"BAZ": "qux",
+	}
+
+	slice := mapToEnvSlice(env)
+	require.Len(t, slice, 2)
+	require.Contains(t, slice, "FOO=bar")
+	require.Contains(t, slice, "BAZ=qux")
+}
+
+func TestMapToEnvSlice_Empty(t *testing.T) {
+	slice := mapToEnvSlice(map[string]string{})
+	require.Empty(t, slice)
+}

cli/azd/extensions/microsoft.azd.scripts/internal/provisioning/output_collector.go

@@ -9,7 +9,6 @@ import (
 	"maps"
 	"os"
 	"path/filepath"
-	"strings"
 )
 
 // OutputParameter represents a single output value from a script.
@@ -28,17 +27,32 @@ func NewOutputCollector(projectPath string) *OutputCollector {
 	return &OutputCollector{projectPath: projectPath}
 }
 
+// maxOutputFileSize is the maximum allowed size for an outputs.json file (10 MB).
+const maxOutputFileSize = 10 * 1024 * 1024
+
 // Collect looks for an outputs.json file in the same directory as the script
 // and parses it into a map of output parameters. If no file is found, returns an empty map.
 func (c *OutputCollector) Collect(sc *ScriptConfig) (map[string]OutputParameter, error) {
 	scriptDir := filepath.Dir(filepath.Join(c.projectPath, sc.Run))
 	outputsPath := filepath.Join(scriptDir, "outputs.json")
 
-	data, err := os.ReadFile(outputsPath)
+	fi, err := os.Stat(outputsPath)
 	if err != nil {
 		if os.IsNotExist(err) {
 			return nil, nil
 		}
+		return nil, fmt.Errorf("checking outputs file %q: %w", outputsPath, err)
+	}
+
+	if fi.Size() > maxOutputFileSize {
+		return nil, fmt.Errorf(
+			"outputs file %q exceeds maximum allowed size of %d bytes (actual: %d bytes)",
+			outputsPath, maxOutputFileSize, fi.Size(),
+		)
+	}
+
+	data, err := os.ReadFile(outputsPath)
+	if err != nil {
 		return nil, fmt.Errorf("reading outputs file %q: %w", outputsPath, err)
 	}
 
@@ -59,23 +73,3 @@ func MergeOutputs(outputs ...map[string]OutputParameter) map[string]OutputParame
 	}
 	return merged
 }
-
-// OutputsToEnvMap converts output parameters to a flat key=value map
-// suitable for storing in the azd environment.
-func OutputsToEnvMap(outputs map[string]OutputParameter) map[string]string {
-	result := make(map[string]string, len(outputs))
-	for k, v := range outputs {
-		result[k] = v.Value
-	}
-	return result
-}
-
-// OutputsToProvisioning converts output parameters to the provisioning output format
-// with uppercase keys following azd conventions.
-func OutputsToProvisioning(outputs map[string]OutputParameter) map[string]OutputParameter {
-	result := make(map[string]OutputParameter, len(outputs))
-	for k, v := range outputs {
-		result[strings.ToUpper(k)] = v
-	}
-	return result
-}

cli/azd/extensions/microsoft.azd.scripts/internal/provisioning/output_collector_test.go

@@ -60,14 +60,3 @@ func TestMergeOutputs(t *testing.T) {
 	require.Equal(t, "b", merged["X"].Value, "later overrides earlier")
 	require.Equal(t, "c", merged["Y"].Value)
 }
-
-func TestOutputsToEnvMap(t *testing.T) {
-	outputs := map[string]OutputParameter{
-		"KEY1": {Type: "string", Value: "val1"},
-		"KEY2": {Type: "string", Value: "val2"},
-	}
-
-	env := OutputsToEnvMap(outputs)
-	require.Equal(t, "val1", env["KEY1"])
-	require.Equal(t, "val2", env["KEY2"])
-}

cli/azd/extensions/microsoft.azd.scripts/internal/provisioning/provider.go

@@ -6,6 +6,7 @@ package provisioning
 import (
 	"context"
 	"fmt"
+	"os"
 
 	"github.com/azure/azure-dev/cli/azd/pkg/azdext"
 	"github.com/azure/azure-dev/cli/azd/pkg/grpcbroker"
@@ -58,17 +59,9 @@ func (p *ScriptProvisioningProvider) State(
 	ctx context.Context,
 	options *azdext.ProvisioningStateOptions,
 ) (*azdext.ProvisioningStateResult, error) {
-	protoOutputs := make(map[string]*azdext.ProvisioningOutputParameter, len(p.outputs))
-	for k, v := range p.outputs {
-		protoOutputs[k] = &azdext.ProvisioningOutputParameter{
-			Type:  v.Type,
-			Value: v.Value,
-		}
-	}
-
 	return &azdext.ProvisioningStateResult{
 		State: &azdext.ProvisioningState{
-			Outputs: protoOutputs,
+			Outputs: toProtoOutputs(p.outputs),
 		},
 	}, nil
 }
@@ -89,17 +82,9 @@ func (p *ScriptProvisioningProvider) Deploy(
 
 	p.outputs = MergeOutputs(p.outputs, outputs)
 
-	protoOutputs := make(map[string]*azdext.ProvisioningOutputParameter, len(p.outputs))
-	for k, v := range p.outputs {
-		protoOutputs[k] = &azdext.ProvisioningOutputParameter{
-			Type:  v.Type,
-			Value: v.Value,
-		}
-	}
-
 	return &azdext.ProvisioningDeployResult{
 		Deployment: &azdext.ProvisioningDeployment{
-			Outputs: protoOutputs,
+			Outputs: toProtoOutputs(p.outputs),
 		},
 	}, nil
 }
@@ -206,11 +191,26 @@ func (p *ScriptProvisioningProvider) runScripts(
 	return allOutputs, nil
 }
 
+// toProtoOutputs converts internal OutputParameter map to the protobuf format.
+func toProtoOutputs(outputs map[string]OutputParameter) map[string]*azdext.ProvisioningOutputParameter {
+	result := make(map[string]*azdext.ProvisioningOutputParameter, len(outputs))
+	for k, v := range outputs {
+		result[k] = &azdext.ProvisioningOutputParameter{Type: v.Type, Value: v.Value}
+	}
+	return result
+}
+
 // getAzdEnv retrieves current azd environment values via the gRPC client.
 func (p *ScriptProvisioningProvider) getAzdEnv(ctx context.Context) (map[string]string, error) {
 	resp, err := p.azdClient.Environment().GetValues(ctx, &azdext.GetEnvironmentRequest{})
 	if err != nil {
-		// If environment is not available, return empty map
+		// Context cancellation should propagate
+		if ctx.Err() != nil {
+			return nil, ctx.Err()
+		}
+
+		// Log the error but continue with empty env — environment may not be initialized yet
+		fmt.Fprintf(os.Stderr, "Warning: could not load azd environment: %v\n", err)
 		return make(map[string]string), nil
 	}