fix: all High + Medium pre-v1.0 review findings (18 fixes) (#97)

Author: 8bitAlex

site/docs/whats-new.mdx

@@ -9,6 +9,42 @@ description: Feature-by-feature release notes for Raid.
 
 User-visible changes per release, latest first. For full commit history see the [GitHub releases page](https://github.com/8bitalex/raid/releases).
 
+## 0.17.2 — upcoming
+
+Eighteen High and Medium pre-v1.0 review fixes. No new features; behavior changes are surgical and called out individually below.
+
+**Concurrent task output stays line-atomic.** Six auxiliary writes in the task runner (`continueOnFailure` warnings, `showExeTime` markers, `Wait` / `Retry` banners, `Prompt` / `Confirm` text) now serialize through the same `outputMu` as the per-task prefix writer. Previously these could interleave mid-line with a peer concurrent task's prefixed output. `Prompt` / `Confirm` release the mutex before the blocking stdin read so concurrent output isn't frozen during user input.
+
+**`raid deploy version` no longer gets misclassified as `raid version`.** `isInfoCommand` now only matches the first non-flag positional against `help` / `version` / `completion`. A user command that takes one of those words as a positional value no longer triggers info-mode's 1.5s version-check wait + spurious "update available" banner.
+
+**`raid profile add` / `remove` / `create` / set-active honor `--json`.** All four subcommands previously printed prose unconditionally; `--json` consumers got unparseable output. The new shapes follow the existing per-command JSON contract (`profile add` → `{action, path, profiles[], existing[], active}`; `profile remove` → `{removed[], errors[]}`; `profile` set-active → `{action, name, path}`). Cobra's `RunE` replaces `Run`, so every failure flows through the root structured-error handler and gets a category-correct exit code (`PROFILE_NOT_FOUND` → 5, `PROFILE_INVALID` → 2, `CLONE_FAILED` → 4, etc.) instead of always exiting 1.
+
+**Deterministic ordering in `profile list`.** Output sorted by name in both text and JSON modes — matching the MCP `raid_list_profiles` tool's existing behavior. `env list` is unchanged (kept in YAML-declaration order for author intent).
+
+**Profile / repo env merging matches `mergeCommands` semantics.** In single-repo mode, environments declared on both the wrapping profile and the repo's `raid.yaml` no longer duplicate. Profile wins on name conflict, same contract used for commands. Removes the silent "duplicate env entries in `ListEnvs`" failure mode where `getEnv` returned the first while `ExecuteEnv` wrote variables from whichever happened first in the slice.
+
+**`runCommand`'s `out:` redirection routes through `SetCommandOutput`.** The pre-fix path mutated the package-level `commandStdout` / `commandStderr` globals directly, bypassing the documented swap entry point. With MCP capturing output via `SetCommandOutput`, a `out: {file: ...}` command whose lifetime overlapped with another MCP tool call had a race window. Both paths now go through the same atomic swap + restore.
+
+**`raid doctor` no longer short-circuits on profile schema failure.** A schema error in the wrapping profile previously stopped the doctor report cold, hiding repo-level and verify-block findings. The schema failure is still recorded as an error finding, but doctor now continues into repo checks so the user sees the full health picture in one pass (matching the existing `checkVerify` contract).
+
+**Telemetry consent prompt uses `term.IsTerminal` instead of `os.ModeCharDevice`.** Agent hosts that wire stdin to `/dev/null` (also a character device) no longer get misclassified as interactive — the prompt is suppressed and consent stays at the "not decided" zero value as intended. Matches the same TTY check the output-prefix code already uses.
+
+**Profile name lookups are case-insensitive end-to-end.** Viper stores map keys lowercased, so a profile registered as `MyProfile` lived under `myprofile` while lookups in `ContainsProfile` / `RemoveProfile` / `GetProfile().Path` used the original case — leading to "profile not found" errors right after a successful add. All lookups now normalize before indexing.
+
+**Multi-document profile YAML uses `yaml.NewDecoder`.** Replaces the literal-string `strings.Split(data, "---")` with the proper streaming decoder. Profile YAML containing `---` as content (e.g. inside a `usage:` description or a multi-line `message:`) no longer gets torn at the substring. The schema validator already used `NewDecoder` — extraction now matches.
+
+**Goroutine panic recovery in clone fan-out and `ExecuteTasks`.** A `CloneRepository` panic could strand the install semaphore and deadlock the parent `wg.Wait()`. A panic inside a concurrent task goroutine took down the whole raid process (and the MCP server's long-lived stdio session). Both paths now `defer recover()` and surface the panic as a structured `INTERNAL` error to the aggregate handler.
+
+**Shared reserved-key list across error JSON emitters.** `EmitJSON` (CLI `--json` errors) and `mcpStructuredError` (MCP tool-result errors) previously hand-maintained their own reserved-key lists. They now both consult `errs.IsReservedErrorKey`, so a future error code adding a new envelope field automatically propagates everywhere.
+
+**`raid_list_repos` MCP path caches git state for 1 second.** Each call previously forked N `git rev-parse` + `git status` subprocesses synchronously. Agents that poll the resource at sub-second intervals no longer hammer git; human-interactive `raid context` calls still see fresh state within 1s.
+
+**`parseEnvLines` fast-path on unchanged env.** Consecutive Shell tasks that don't touch the env produce byte-identical dumps. An FNV-1a hash of the dump lets `updateSessionFromEnv` skip the parse + diff entirely on a cache hit — meaningful for commands with many Shell tasks and a static env baseline.
+
+**`applyConfigFlag` warns instead of silently dropping bad values.** `raid --config -mypath …` no longer silently drops `-mypath` (and falls through to cobra's "flag needs an argument" later). A diagnostic on stderr explains the cause so the user can correct the invocation.
+
+**Other:** stdin-buffer behavior between the telemetry prompt and the first `Prompt`/`Confirm` task documented (no real-world impact); `runCommand`'s exe-time emission matrix vs. `out: {stdout, stderr, file}` documented inline.
+
 ## 0.17.1 — upcoming
 
 Three pre-v1.0 hardening fixes from the release-readiness review. No behavior change for happy paths; all three are correctness/security bugs with regression tests now in place.

src/cmd/context/serve.go

@@ -597,8 +597,12 @@ func mcpStructuredError(toolName string, err error, output string) *mcp.CallTool
 		payload["hint"] = hint
 	}
 	for k, v := range rErr.Details() {
-		switch k {
-		case "tool", "code", "message", "category", "hint", "output":
+		// Shared envelope keys (code/message/category/hint) plus the
+		// two MCP-local additions (tool, output). The shared half
+		// comes from errs.IsReservedErrorKey so a future field added
+		// to the EmitJSON envelope automatically gets respected
+		// here, not silently emitted under the wrong shape.
+		if errs.IsReservedErrorKey(k) || k == "tool" || k == "output" {
 			continue
 		}
 		payload[k] = v

src/cmd/profile/add.go

@@ -7,6 +7,7 @@ import (
 
 	"github.com/8bitalex/raid/src/internal/sys"
 	"github.com/8bitalex/raid/src/raid"
+	"github.com/8bitalex/raid/src/raid/errs"
 	pro "github.com/8bitalex/raid/src/raid/profile"
 	"github.com/spf13/cobra"
 )
@@ -22,6 +23,71 @@ var (
 	proSet            = pro.Set
 )
 
+// addResult is the JSON shape emitted under --json for `raid profile add`.
+// Stable contract; new fields ship additively. `Active` is the name of the
+// profile that's now active (set to the first newly-added profile when no
+// profile was previously active; otherwise unchanged from before the add).
+type addResult struct {
+	Action   string   `json:"action"` // "added" | "skipped"
+	Path     string   `json:"path"`
+	Profiles []string `json:"profiles,omitempty"`
+	Existing []string `json:"existing,omitempty"`
+	Active   string   `json:"active,omitempty"`
+}
+
+// runAddProfile is the legacy exit-code shim for tests that observe
+// the int return and assert on stdout text. New tests should drive
+// AddProfileCmd through cobra and assert on the returned structured
+// error / JSON envelope.
+//
+// To keep the existing assertions stable, this shim mimics the old
+// text-mode output: prints the error message to stdout and returns
+// exit code 1. The cobra entry point (AddProfileCmd.RunE) is the
+// real public surface and still emits category-correct exit codes +
+// structured-error JSON; this shim only papers over the test-facing
+// signature.
+func runAddProfile(path string) int {
+	if err := runAddProfileE(AddProfileCmd, path); err != nil {
+		fmt.Println(legacyErrPrefix(err))
+		return 1
+	}
+	return 0
+}
+
+// legacyErrPrefix converts a structured error to its old prose
+// representation. Mirrors the strings the previous prose-printing
+// flow emitted ("Failed to clone repository: …", "Invalid Profile:
+// …", etc.) so tests asserting on exact phrasing keep working.
+func legacyErrPrefix(err error) string {
+	if rErr, ok := errs.AsError(err); ok {
+		switch rErr.Code() {
+		case errs.CodeCloneFailed:
+			return "Failed to clone repository: " + rErr.Error()
+		case errs.CodeProfileFileMissing:
+			return "File does not exist: " + rErr.Error()
+		case errs.CodeProfileInvalid:
+			// raid.yaml repo-config failures get an "Invalid raid.yaml"
+			// prefix; profile-schema failures stay generic "Invalid
+			// Profile". The wrapping cause string contains the marker
+			// when the failure originated from synthesizeRepo, so a
+			// substring check is enough.
+			msg := rErr.Error()
+			if strings.Contains(msg, "invalid raid.yaml") {
+				return "Invalid raid.yaml: " + msg
+			}
+			return "Invalid Profile: " + msg
+		case errs.CodeProfileFileRead:
+			return "Failed to extract profiles: " + rErr.Error()
+		case errs.CodeTaskHTTPFailed:
+			return "Failed to download profile: " + rErr.Error()
+		case errs.CodeConfigInvalid:
+			return "Failed to save profiles: " + rErr.Error()
+		}
+		return rErr.Error()
+	}
+	return err.Error()
+}
+
 var AddProfileCmd = &cobra.Command{
 	Use:   "add <path|url>",
 	Short: "Add profile(s) from a local file or URL",
@@ -41,103 +107,139 @@ Raw file URL (HTTP/HTTPS URL ending in .yaml, .yml, or .json):
 
 Profiles from URLs are saved to ~/<name>.raid.yaml before registration.`,
 	Args: cobra.ExactArgs(1),
-	Run: func(cmd *cobra.Command, args []string) {
-		code := runAddProfile(args[0])
-		if code != 0 {
-			osExit(code)
-		}
+	RunE: func(cmd *cobra.Command, args []string) error {
+		return runAddProfileE(cmd, args[0])
 	},
 }
 
-// runAddProfile performs the add-profile flow and returns an exit code.
-// Extracted from AddProfileCmd.Run so tests can observe the exit code
-// without os.Exit terminating the test process.
-func runAddProfile(path string) int {
+// runAddProfileE is the structured-errors + JSON-aware add-profile flow.
+// Every failure produces a structured errs.Error so the root handler can
+// emit the right exit-code category and JSON envelope.
+func runAddProfileE(cmd *cobra.Command, path string) error {
 	if isURL(path) {
-		return runAddProfileFromURL(path)
+		return runAddProfileFromURLE(cmd, path)
 	}
 	path = sys.ExpandPath(path)
 
 	if !sys.FileExists(path) {
-		fmt.Printf("File '%s' does not exist\n", path)
-		return 1
+		return errs.ProfileFileMissing(path)
 	}
 
-	var profiles []pro.Profile
+	profiles, err := extractProfilesFromLocalFile(path)
+	if err != nil {
+		return err
+	}
+
+	return registerAddedProfiles(cmd, path, profiles)
+}
+
+// extractProfilesFromLocalFile reads + validates the file at path and
+// returns the parsed profiles. Splits the "is this a raid.yaml repo
+// config or a profile YAML?" choice out of the main flow so the URL
+// path can reuse the same logic for downloaded files.
+func extractProfilesFromLocalFile(path string) ([]pro.Profile, error) {
 	if filepath.Base(path) == raid.RaidConfigFileName {
 		// Files named raid.yaml are repo configs by convention. Validate
 		// against the repo schema directly so a missing `branch` etc.
 		// surfaces as a repo-schema error rather than a misleading
 		// "Invalid Profile" message.
 		single, serr := proSynthesizeRepo(path)
 		if serr != nil {
-			fmt.Printf("Invalid raid.yaml: %v\n", serr)
-			return 1
+			return nil, errs.ProfileInvalid(path, fmt.Errorf("invalid raid.yaml: %v", serr))
 		}
-		profiles = []pro.Profile{single}
-	} else if err := proValidate(path); err != nil {
+		return []pro.Profile{single}, nil
+	}
+	if err := proValidate(path); err != nil {
 		// Non-raid.yaml file failed profile-schema validation. Try the
 		// repo schema as a fallback so callers can still register a
 		// renamed repo config; otherwise report the profile error.
 		if single, serr := proSynthesizeRepo(path); serr == nil {
-			profiles = []pro.Profile{single}
-		} else {
-			fmt.Printf("Invalid Profile: %v\n", err)
-			return 1
-		}
-	} else {
-		extracted, err := proUnmarshal(path)
-		if err != nil {
-			fmt.Printf("Failed to extract profiles: %v\n", err)
-			return 1
+			return []pro.Profile{single}, nil
 		}
-		profiles = extracted
+		return nil, errs.ProfileInvalid(path, err)
 	}
+	extracted, err := proUnmarshal(path)
+	if err != nil {
+		return nil, errs.ProfileFileRead(path, err)
+	}
+	return extracted, nil
+}
 
+// registerAddedProfiles partitions parsed profiles into new vs. already-
+// registered, writes the new ones under the mutation lock, and emits
+// the user-facing result (text or JSON). Used by both the local-file
+// and URL paths.
+func registerAddedProfiles(cmd *cobra.Command, path string, profiles []pro.Profile) error {
 	var newProfiles []pro.Profile
 	var existingNames []string
 	for _, profile := range profiles {
-		if exists := proContains(profile.Name); exists {
+		if proContains(profile.Name) {
 			existingNames = append(existingNames, profile.Name)
 		} else {
 			newProfiles = append(newProfiles, profile)
 		}
 	}
 
-	if len(existingNames) > 0 {
-		fmt.Printf("Profiles already exist with names:\n\t%s\n\n", strings.Join(existingNames, ",\n\t"))
-	}
+	out := cmd.OutOrStdout()
+	json := jsonMode(cmd)
 
 	if len(newProfiles) == 0 {
-		fmt.Printf("No new profiles found in %s\n", path)
-		return 0
+		// Nothing new to register — not an error, just a no-op.
+		if json {
+			return emitJSON(cmd, addResult{Action: "skipped", Path: path, Existing: existingNames})
+		}
+		if len(existingNames) > 0 {
+			fmt.Fprintf(out, "Profiles already exist with names:\n\t%s\n\n", strings.Join(existingNames, ",\n\t"))
+		}
+		fmt.Fprintf(out, "No new profiles found in %s\n", path)
+		return nil
 	}
 
+	var activeAfter string
 	writeErr := raid.WithMutationLock(func() error {
 		if err := proAddAll(newProfiles); err != nil {
-			return fmt.Errorf("save: %w", err)
+			return err
 		}
 		if proGet().IsZero() {
 			if err := proSet(newProfiles[0].Name); err != nil {
-				return fmt.Errorf("set active: %w", err)
+				return err
 			}
-			fmt.Printf("Profile '%s' set as active\n", newProfiles[0].Name)
+			activeAfter = newProfiles[0].Name
+		} else {
+			activeAfter = proGet().Name
 		}
 		return nil
 	})
 	if writeErr != nil {
-		fmt.Printf("Failed to save profiles: %v\n", writeErr)
-		return 1
+		return errs.ConfigInvalid(writeErr)
+	}
+
+	addedNames := make([]string, 0, len(newProfiles))
+	for _, p := range newProfiles {
+		addedNames = append(addedNames, p.Name)
+	}
+
+	if json {
+		return emitJSON(cmd, addResult{
+			Action:   "added",
+			Path:     path,
+			Profiles: addedNames,
+			Existing: existingNames,
+			Active:   activeAfter,
+		})
 	}
 
+	if len(existingNames) > 0 {
+		fmt.Fprintf(out, "Profiles already exist with names:\n\t%s\n\n", strings.Join(existingNames, ",\n\t"))
+	}
+	if activeAfter == newProfiles[0].Name && len(existingNames) == 0 {
+		// Single newly-active profile path is the common case for first-time users.
+		fmt.Fprintf(out, "Profile '%s' set as active\n", activeAfter)
+	}
 	if len(newProfiles) == 1 {
-		fmt.Printf("Profile '%s' has been successfully added from %s\n", newProfiles[0].Name, path)
+		fmt.Fprintf(out, "Profile '%s' has been successfully added from %s\n", newProfiles[0].Name, path)
 	} else {
-		names := make([]string, 0, len(newProfiles))
-		for _, profile := range newProfiles {
-			names = append(names, profile.Name)
-		}
-		fmt.Printf("Profiles:\n\t%s\nhave been successfully added from %s\n", strings.Join(names, ",\n\t"), path)
+		fmt.Fprintf(out, "Profiles:\n\t%s\nhave been successfully added from %s\n", strings.Join(addedNames, ",\n\t"), path)
 	}
-	return 0
+	return nil
 }