Improve Windows test reliability

.github/skills/azd-code-reviewer/reviewers.md

@@ -199,6 +199,8 @@ Focus areas:
 - Test helpers: are setup/teardown patterns consistent? Are test utilities reused?
 - Integration vs unit: is the test level appropriate for what is being tested?
 - Flakiness risk: time-dependent tests, order-dependent tests, external service calls without mocks
+- State isolation: do functional tests that modify user-level state use a dedicated `AZD_CONFIG_DIR`, preserve existing env entries (`cli.Env = append(os.Environ(), cli.Env...)`), and disable telemetry (`AZURE_DEV_COLLECT_TELEMETRY=no`)?
+- Fixture safety: are type assertions on values read from maps, JSON, environment files, or recordings guarded with `require` (presence + type) so malformed data fails the test instead of panicking and aborting the package?
 
 Review tests statically — do not attempt to run them.
 
@@ -227,6 +229,7 @@ Focus areas:
 - Help text: is `--help` complete, with examples?
 - Interactive prompts: are they skippable with flags for CI/automation?
 - Progress feedback: for long operations, is there progress indication?
+- Writer propagation: do nested formatters, spinners, prompts, and cursors use the injected writer without leaking terminal output to global stdout?
 - Exit codes: are they meaningful and documented?
 - Consistency: does this match the patterns in existing azd commands?
 

cli/azd/AGENTS.md

@@ -359,7 +359,10 @@ This project uses Go 1.26. Use modern standard library features:
 ### Testing Best Practices
 
 - **Test the actual rules, not just the framework**: When adding YAML-based error suggestion rules, write tests that exercise the rules end-to-end through the pipeline, not just tests that validate the framework's generic matching behavior
-- **Extract shared test helpers**: Don't duplicate test utilities across files. Extract common helpers (e.g., shell wrappers, auth token fetchers, CLI runners) into shared `test_utils` packages. Duplication across 3+ files should always be refactored
+- **Search for prior art before adding helpers**: Search `test/`, `pkg/osutil`, and the package under test before adding cleanup, retry, process, or platform-specific helpers. Reuse or promote existing behavior and extract shared test utilities instead of creating parallel implementations
+- **Isolate persistent user state**: Functional tests that install extensions or mutate config, auth, cache, or other user-level state must set `AZD_CONFIG_DIR` to a dedicated `tempDirWithDiagnostics(t)` directory on the CLI env, preserving both the parent environment and existing CLI env (`append(os.Environ(), cli.Env...)`) and setting `AZURE_DEV_COLLECT_TELEMETRY=no` so the telemetry background writer doesn't hold files open in the temp dir. Tests that only use the existing login should leave `AZD_CONFIG_DIR` unset
+- **Make cleanup process-safe**: Wait for spawned commands to finish, register cleanup when state is created, and report failures. Use `osutil.RemoveAll` for directories containing executed binaries because Windows may retain transient locks; keep retries at the filesystem boundary instead of adding test sleeps. Since `t.Context()` is canceled before cleanup runs, use a separate bounded context when cleanup needs one
+- **Guard fixture type assertions**: Check presence and type with `require` before casting values loaded from maps, JSON, environment files, or recordings. A raw type assertion panic can abort the package and hide the initiating failure
 - **Use correct env vars for testing**:
   - Non-interactive mode: `AZD_FORCE_TTY=false` (not `AZD_DEBUG_FORCE_NO_TTY`, which doesn't exist)
   - No-prompt mode: use the `--no-prompt` flag for core azd commands; `AZD_NO_PROMPT=true` is only used for propagating no-prompt into extension subprocesses
@@ -370,7 +373,7 @@ This project uses Go 1.26. Use modern standard library features:
 - **Cross-platform paths**: When resolving binary paths in tests, handle `.exe` suffix on Windows (e.g., `azd` vs `azd.exe` via `process.platform === "win32"`)
 - **Test new JSON fields**: When adding fields to JSON command output (e.g., `expiresOn` in `azd auth status --output json`), add a test asserting the field's presence and format
 - **No unused dependencies**: Don't add npm/pip packages that aren't imported anywhere. Remove dead `devDependencies` before submitting
-- **Never write to `os.Stdout` in tests**: Tests that write directly to `os.Stdout` (via `fmt.Print*`, `os.Stdout`, or UX components like `ux.NewSpinner` without a `Writer` option) corrupt the `go test -json` event stream under parallel execution, causing phantom test failures in CI. Use `t.Log`/`t.Logf` for diagnostic output, `io.Discard` or `&bytes.Buffer{}` for UX component writers, and `SkipLoadingSpinner: true` for prompt tests that don't need spinner behavior. Enforced by the `forbidigo` linter in `.golangci.yaml`. See [#8385](https://github.com/Azure/azure-dev/issues/8385) for details.
+- **Keep terminal output on the injected writer**: Commands and UX components must pass the caller's `io.Writer` to every nested spinner, prompt, cursor, and formatter. In tests, use `t.Log`/`t.Logf` for diagnostics and `io.Discard` or `&bytes.Buffer{}` for UX writers. Raw stdout corrupts `go test -json` and creates phantom CI failures. `forbidigo` blocks direct writes in tests, but production writer propagation still needs buffer-based coverage. See [#8385](https://github.com/Azure/azure-dev/issues/8385)
 
 ## MCP Tools
 

cli/azd/cmd/extension.go

@@ -1825,7 +1825,7 @@ func (a *extensionUninstallAction) Run(ctx context.Context) (*actions.ActionResu
 		stepMessage += fmt.Sprintf(" (%s)", installed.Version)
 		a.console.ShowSpinner(ctx, stepMessage, input.Step)
 
-		if err := a.extensionManager.Uninstall(extensionId); err != nil {
+		if err := a.extensionManager.Uninstall(ctx, extensionId); err != nil {
 			a.console.StopSpinner(ctx, stepMessage, input.StepFailed)
 			return nil, fmt.Errorf("failed to uninstall extension: %w", err)
 		}

cli/azd/cmd/tool.go

@@ -183,15 +183,18 @@ func toolActions(root *actions.ActionDescriptor) *actions.ActionDescriptor {
 type toolAction struct {
 	manager *tool.Manager
 	console input.Console
+	writer  io.Writer
 }
 
 func newToolAction(
 	manager *tool.Manager,
 	console input.Console,
+	writer io.Writer,
 ) actions.Action {
 	return &toolAction{
 		manager: manager,
 		console: console,
+		writer:  writer,
 	}
 }
 
@@ -206,6 +209,7 @@ func (a *toolAction) Run(ctx context.Context) (*actions.ActionResult, error) {
 	spinner := uxlib.NewSpinner(&uxlib.SpinnerOptions{
 		Text:        "Detecting tools...",
 		ClearOnStop: true,
+		Writer:      a.writer,
 	})
 	if err := spinner.Run(ctx, func(ctx context.Context) error {
 		var detectErr error
@@ -374,6 +378,7 @@ func (a *toolListAction) Run(ctx context.Context) (*actions.ActionResult, error)
 		spinner := uxlib.NewSpinner(&uxlib.SpinnerOptions{
 			Text:        "Checking tool status...",
 			ClearOnStop: true,
+			Writer:      a.writer,
 		})
 		if err := spinner.Run(ctx, func(ctx context.Context) error {
 			var detectErr error
@@ -897,6 +902,7 @@ func detectAllTools(
 	ctx context.Context,
 	manager *tool.Manager,
 	formatter output.Formatter,
+	writer io.Writer,
 	spinnerText string,
 ) ([]*tool.ToolStatus, error) {
 	if formatter != nil && formatter.Kind() == output.JsonFormat {
@@ -907,6 +913,7 @@ func detectAllTools(
 	spinner := uxlib.NewSpinner(&uxlib.SpinnerOptions{
 		Text:        spinnerText,
 		ClearOnStop: true,
+		Writer:      writer,
 	})
 	if err := spinner.Run(ctx, func(ctx context.Context) error {
 		var detectErr error
@@ -1080,7 +1087,7 @@ func (a *toolInstallAction) resolveToolIds(ctx context.Context) ([]string, error
 
 	// --all: install all recommended tools that are not already installed.
 	if a.flags.all {
-		statuses, err := detectAllTools(ctx, a.manager, a.formatter, "Detecting tool status...")
+		statuses, err := detectAllTools(ctx, a.manager, a.formatter, a.writer, "Detecting tool status...")
 		if err != nil {
 			return nil, fmt.Errorf("detecting tools: %w", err)
 		}
@@ -1100,7 +1107,7 @@ func (a *toolInstallAction) resolveToolIds(ctx context.Context) ([]string, error
 	}
 
 	// Interactive: let the user pick from uninstalled tools.
-	statuses, err := detectAllTools(ctx, a.manager, a.formatter, "Detecting tool status...")
+	statuses, err := detectAllTools(ctx, a.manager, a.formatter, a.writer, "Detecting tool status...")
 	if err != nil {
 		return nil, fmt.Errorf("detecting tools: %w", err)
 	}
@@ -1439,7 +1446,7 @@ func (a *toolUpgradeAction) Run(ctx context.Context) (*actions.ActionResult, err
 // detectInstalledTools runs DetectAll behind a spinner and returns the full
 // set of tool statuses. Used by the --all and interactive upgrade paths.
 func (a *toolUpgradeAction) detectInstalledTools(ctx context.Context) ([]*tool.ToolStatus, error) {
-	statuses, err := detectAllTools(ctx, a.manager, a.formatter, "Detecting installed tools...")
+	statuses, err := detectAllTools(ctx, a.manager, a.formatter, a.writer, "Detecting installed tools...")
 	if err != nil {
 		return nil, fmt.Errorf("detecting installed tools: %w", err)
 	}
@@ -1757,7 +1764,7 @@ func (a *toolUninstallAction) resolveToolIds(ctx context.Context) ([]string, err
 
 	// --all, --dry-run, and the interactive picker all need the current
 	// installed set.
-	statuses, err := detectAllTools(ctx, a.manager, a.formatter, "Detecting installed tools...")
+	statuses, err := detectAllTools(ctx, a.manager, a.formatter, a.writer, "Detecting installed tools...")
 	if err != nil {
 		return nil, fmt.Errorf("detecting installed tools: %w", err)
 	}
@@ -1920,6 +1927,7 @@ func (a *toolCheckAction) Run(ctx context.Context) (*actions.ActionResult, error
 		spinner := uxlib.NewSpinner(&uxlib.SpinnerOptions{
 			Text:        "Checking for upgrades...",
 			ClearOnStop: true,
+			Writer:      a.writer,
 		})
 		if err := spinner.Run(ctx, func(ctx context.Context) error {
 			var detectErr error
@@ -2118,6 +2126,7 @@ func (a *toolShowAction) Run(ctx context.Context) (*actions.ActionResult, error)
 		spinner := uxlib.NewSpinner(&uxlib.SpinnerOptions{
 			Text:        fmt.Sprintf("Checking %s...", toolDef.Name),
 			ClearOnStop: true,
+			Writer:      a.writer,
 		})
 		if err := spinner.Run(ctx, func(ctx context.Context) error {
 			var detectErr error

cli/azd/cmd/tool_test.go

@@ -25,6 +25,13 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+// ANSI cursor-visibility control sequences a spinner emits to the writer:
+// it hides the cursor while running and shows it again when it stops.
+const (
+	ansiHideCursor = "\033[?25l"
+	ansiShowCursor = "\033[?25h"
+)
+
 func TestToolCommandGating(t *testing.T) {
 	// The "tool" command group is always registered, regardless of any
 	// alpha feature gating.
@@ -330,6 +337,106 @@ func TestToolShowAction_InvalidArgDoesNotEmitToolId(t *testing.T) {
 		"tool.id must not be emitted when FindTool fails on an unknown tool")
 }
 
+func TestToolActionSpinnersUseInjectedWriter(t *testing.T) {
+	detectErr := errors.New("detect failed")
+
+	tests := []struct {
+		name string
+		run  func(context.Context, *bytes.Buffer) error
+	}{
+		{
+			name: "bare tool action",
+			run: func(ctx context.Context, writer *bytes.Buffer) error {
+				detector := &cmdMockDetector{
+					detectAll: func(context.Context, []*tool.ToolDefinition) ([]*tool.ToolStatus, error) {
+						return nil, detectErr
+					},
+				}
+				action := newToolAction(
+					tool.NewManager(detector, &cmdMockInstaller{}, nil),
+					mockinput.NewMockConsole(),
+					writer,
+				)
+				_, err := action.Run(ctx)
+				return err
+			},
+		},
+		{
+			name: "tool list",
+			run: func(ctx context.Context, writer *bytes.Buffer) error {
+				detector := &cmdMockDetector{
+					detectAll: func(context.Context, []*tool.ToolDefinition) ([]*tool.ToolStatus, error) {
+						return nil, detectErr
+					},
+				}
+				action := newToolListAction(
+					tool.NewManager(detector, &cmdMockInstaller{}, nil),
+					mockinput.NewMockConsole(),
+					&output.TableFormatter{},
+					writer,
+				)
+				_, err := action.Run(ctx)
+				return err
+			},
+		},
+		{
+			name: "tool check",
+			run: func(ctx context.Context, writer *bytes.Buffer) error {
+				detector := &cmdMockDetector{
+					detectAll: func(context.Context, []*tool.ToolDefinition) ([]*tool.ToolStatus, error) {
+						return nil, detectErr
+					},
+				}
+				updateChecker := tool.NewUpdateChecker(
+					&memUserConfigManager{},
+					detector,
+					func() (string, error) { return t.TempDir(), nil },
+					nil,
+				)
+				action := newToolCheckAction(
+					tool.NewManager(detector, &cmdMockInstaller{}, updateChecker),
+					mockinput.NewMockConsole(),
+					&output.TableFormatter{},
+					writer,
+				)
+				_, err := action.Run(ctx)
+				return err
+			},
+		},
+		{
+			name: "tool show",
+			run: func(ctx context.Context, writer *bytes.Buffer) error {
+				detector := &cmdMockDetector{
+					detectTool: func(context.Context, *tool.ToolDefinition) (*tool.ToolStatus, error) {
+						return nil, detectErr
+					},
+				}
+				manager := tool.NewManager(detector, &cmdMockInstaller{}, nil)
+				action := newToolShowAction(
+					[]string{manager.GetAllTools()[0].Id},
+					manager,
+					mockinput.NewMockConsole(),
+					&output.NoneFormatter{},
+					writer,
+				)
+				_, err := action.Run(ctx)
+				return err
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			var writer bytes.Buffer
+			err := test.run(t.Context(), &writer)
+
+			require.ErrorIs(t, err, detectErr)
+			assert.Contains(t, writer.String(), ansiHideCursor)
+			assert.Contains(t, writer.String(), ansiShowCursor)
+		})
+	}
+}
+
 // lookupToolBoolUsage returns the most recent bool-valued usage attribute
 // for the given key (assumes the key was set via .Bool()).
 func lookupToolBoolUsage(key string) (bool, bool) {
@@ -1275,13 +1382,14 @@ func TestToolUpgradeAction_All_UpgradesInstalledTools(t *testing.T) {
 	}
 	manager := tool.NewManager(detector, installer, nil)
 
+	var writer bytes.Buffer
 	action := newToolUpgradeAction(
 		nil,
 		&toolUpgradeFlags{all: true},
 		manager,
 		mockinput.NewMockConsole(),
 		&output.NoneFormatter{},
-		io.Discard,
+		&writer,
 	)
 
 	_, err := action.Run(t.Context())
@@ -1290,6 +1398,8 @@ func TestToolUpgradeAction_All_UpgradesInstalledTools(t *testing.T) {
 	require.Len(t, installedIDs, 2)
 	assert.ElementsMatch(t, installedIDs, upgradedIDs,
 		"--all must upgrade exactly the installed tools")
+	assert.Contains(t, writer.String(), "\033[?25l")
+	assert.Contains(t, writer.String(), "\033[?25h")
 }
 
 // TestToolUpgradeAction_All_JsonFormat_EmitsCleanJson exercises the reviewer's

cli/azd/docs/recording-functional-tests-guide.md

@@ -718,15 +718,39 @@ if session == nil {
 }
 ```
 
-### 7. Add Debug Logging
+### 7. Isolate State and Use Shared Cleanup
+
+Use a private `AZD_CONFIG_DIR` when a test changes user-level state so parallel tests do not share `~/.azd`. Set it on the test CLI before the first `azd` invocation rather than using `t.Setenv`, which changes process-wide state:
+
+```go
+configDir := tempDirWithDiagnostics(t)
+cli.Env = append(os.Environ(), cli.Env...)
+cli.Env = append(cli.Env,
+    "AZD_CONFIG_DIR="+configDir,
+    "AZURE_DEV_COLLECT_TELEMETRY=no",
+)
+```
+
+A private config dir is not the default for functional tests because live tests inherit the pipeline's one-time `azd auth login` from the shared config dir: a new config directory is logged out, and there is currently no supported way to authenticate one in CI - leave `AZD_CONFIG_DIR` unset when a test needs live Azure access. Playback tests are unaffected: recording sessions authenticate through the session's credential server, not the config dir.
+
+Use `tempDirWithDiagnostics(t)` for generated or executed files. It retries transient Windows file locks and reports lock diagnostics if cleanup fails. Register other cleanup when state is created; don't duplicate retry loops or add fixed sleeps. Since `t.Context()` is canceled before cleanup runs, use a separate bounded context when needed.
+
+Guard type assertions when reading untyped fixture, JSON, environment, or recording data so malformed input fails the test instead of panicking:
+
+```go
+subscriptionID, ok := envValues["AZURE_SUBSCRIPTION_ID"].(string)
+require.True(t, ok, "AZURE_SUBSCRIPTION_ID should be a string")
+```
+
+### 8. Add Debug Logging
 
 ```go
 t.Logf("Recording mode: playback=%v", session != nil && session.Playback)
 t.Logf("Environment name: %s", envName)
 t.Logf("Subscription ID: %s", subscriptionId)
 ```
 
-### 8. Handle Skipped Tests Gracefully
+### 9. Handle Skipped Tests Gracefully
 
 **DO**:
 ```go
@@ -1022,10 +1046,12 @@ os.Setenv("AZD_TEST_FIXED_CLOCK_UNIX_TIME", "1744738873")
 - [ ] Start with `recording.Start(t)`
 - [ ] Use `randomOrStoredEnvName(session)`
 - [ ] Create CLI with `azdcli.WithSession(session)`
+- [ ] Set a private `AZD_CONFIG_DIR` (plus `AZURE_DEV_COLLECT_TELEMETRY=no`) on `cli.Env` for user-level state changes; leave it unset when the test needs live Azure access
 - [ ] Use `session.ProxyClient` for HTTP operations
 - [ ] Store dynamic values in `session.Variables`
-- [ ] Add cleanup with session checks
+- [ ] Use `tempDirWithDiagnostics(t)` and register other cleanup immediately
 - [ ] Use appropriate timeouts for recording/playback
+- [ ] Guard fixture type assertions so malformed data fails the test without panicking
 
 ### Recording a Test ✓
 - [ ] Set `AZURE_RECORD_MODE=record`
@@ -1056,8 +1082,3 @@ os.Setenv("AZD_TEST_FIXED_CLOCK_UNIX_TIME", "1744738873")
 - **Test Helpers**: `cli/azd/test/azdcli/`
 - **Example Tests**: `cli/azd/test/functional/up_test.go`
 - **go-vcr Documentation**: https://github.com/dnaeon/go-vcr
-
----
-
-**Last Updated**: December 2025  
-**Maintainers**: Azure Developer CLI Team

cli/azd/pkg/extensions/manager.go

@@ -781,8 +781,8 @@ func (m *Manager) installInternal(
 	return selectedVersion, nil
 }
 
-// Uninstall an extension by name
-func (m *Manager) Uninstall(id string) error {
+// Uninstall uninstalls an extension by name.
+func (m *Manager) Uninstall(ctx context.Context, id string) error {
 	// Get the installed extension
 	extension, err := m.GetInstalled(FilterOptions{Id: id})
 	if err != nil {
@@ -795,16 +795,8 @@ func (m *Manager) Uninstall(id string) error {
 	}
 
 	extensionDir := filepath.Join(userConfigDir, "extensions", extension.Id)
-	if err := os.MkdirAll(extensionDir, os.ModePerm); err != nil {
-		return fmt.Errorf("failed to create target directory: %w", err)
-	}
-
-	// Remove the extension artifacts when it exists
-	_, err = os.Stat(extensionDir)
-	if err == nil {
-		if err := os.RemoveAll(extensionDir); err != nil {
-			return fmt.Errorf("failed to remove extension: %w", err)
-		}
+	if err := osutil.RemoveAll(ctx, extensionDir); err != nil {
+		return fmt.Errorf("failed to remove extension: %w", err)
 	}
 
 	// Update the user config
@@ -901,7 +893,7 @@ func (m *Manager) upgradeInternal(
 	opts UpgradeOptions,
 	visited map[string]struct{},
 ) (*ExtensionVersion, []UpgradeResult, error) {
-	if err := m.Uninstall(extension.Id); err != nil {
+	if err := m.Uninstall(ctx, extension.Id); err != nil {
 		return nil, nil, fmt.Errorf("failed to uninstall extension: %w", err)
 	}