fix: merge PRs gastownhall#3246+gastownhall#3243+gastownhall#3242 — hooks fixes (gt-91g)

Fix-merge three baumgold PRs with conflict resolution:

- PR gastownhall#3246: JSON-escape GT_BIN on Windows — use json.MarshalIndent in
  test reformatter to avoid corrupting Windows drive letters
- PR gastownhall#3243: doctor hooks-sync uses ResolveRoleAgentName + preset directly
  instead of ResolveAgentConfigByName, preventing false negatives when
  non-Claude binary not in PATH
- PR gastownhall#3242: hooks-sync uses same preset-based approach for installing
  hooks for configured agent even when binary not found
- Both gastownhall#3243/gastownhall#3242: remove t.Parallel() from tests that modify
  package-level globals, eliminating data races

Co-authored-by: baumgold <baumgold@users.noreply.github.com>

Author: 2 people

internal/cmd/hooks_sync.go

@@ -127,23 +127,33 @@ func runHooksSync(cmd *cobra.Command, args []string) error {
 			if loc.Rig != "" {
 				rigPath = filepath.Join(townRoot, loc.Rig)
 			}
-			// Use ResolveRoleAgentName + ResolveAgentConfigByName so hooks sync works
-			// even when the agent binary is not installed on this machine.
+
+			// Use ResolveRoleAgentName (not ResolveRoleAgentConfig) so that hooks are
+			// installed based on the *configured* agent, not the *resolved* one.
+			// ResolveRoleAgentConfig falls back to claude when the agent binary is not
+			// found in PATH (e.g., in CI or on a fresh machine), which would silently
+			// skip creating opencode/gemini/etc. plugin files.
 			agentName, _ := config.ResolveRoleAgentName(loc.Role, townRoot, rigPath)
-			if agentName == "" || agentName == "claude" {
+			if agentName == "" {
 				continue
 			}
-			rc := config.ResolveAgentConfigByName(agentName, townRoot, rigPath)
-			if rc == nil || rc.Hooks == nil || rc.Hooks.Provider == "" {
+
+			preset := config.GetAgentPresetByName(agentName)
+			if preset == nil || preset.HooksDir == "" || preset.HooksSettingsFile == "" {
 				continue
 			}
+
+			hooksProvider := preset.HooksProvider
+			if hooksProvider == "" {
+				hooksProvider = agentName
+			}
+
 			// Claude targets are already handled by DiscoverTargets + syncTarget above.
-			if rc.Hooks.Provider == "claude" {
+			if hooksProvider == "claude" {
 				continue
 			}
 
-			preset := config.GetAgentPresetByName(rc.Hooks.Provider)
-			useSettingsDir := preset != nil && preset.HooksUseSettingsDir
+			useSettingsDir := preset.HooksUseSettingsDir
 
 			// Determine sync targets.
 			// - Town-level roles (mayor, deacon): the role dir IS the working directory.
@@ -158,40 +168,40 @@ func runHooksSync(cmd *cobra.Command, args []string) error {
 			}
 
 			for _, dir := range syncDirs {
-				targetPath := filepath.Join(dir, rc.Hooks.Dir, rc.Hooks.SettingsFile)
+				targetPath := filepath.Join(dir, preset.HooksDir, preset.HooksSettingsFile)
 				relPath, pathErr := filepath.Rel(townRoot, targetPath)
 				if pathErr != nil {
 					relPath = targetPath
 				}
 
 				if hooksSyncDryRun {
 					if _, statErr := os.Stat(targetPath); statErr == nil {
-						fmt.Printf("  %s %s %s\n", style.Warning.Render("~"), relPath, style.Dim.Render("(would check "+rc.Hooks.Provider+")"))
+						fmt.Printf("  %s %s %s\n", style.Warning.Render("~"), relPath, style.Dim.Render("(would check "+hooksProvider+")"))
 					} else {
-						fmt.Printf("  %s %s %s\n", style.Warning.Render("~"), relPath, style.Dim.Render("(would create "+rc.Hooks.Provider+")"))
+						fmt.Printf("  %s %s %s\n", style.Warning.Render("~"), relPath, style.Dim.Render("(would create "+hooksProvider+")"))
 						created++
 					}
 					continue
 				}
 
-				result, syncErr := hooks.SyncForRole(rc.Hooks.Provider, dir, dir, loc.Role,
-					rc.Hooks.Dir, rc.Hooks.SettingsFile, useSettingsDir)
+				result, syncErr := hooks.SyncForRole(hooksProvider, dir, dir, loc.Role,
+					preset.HooksDir, preset.HooksSettingsFile, useSettingsDir)
 				if syncErr != nil {
-					fmt.Printf("  %s %s (%s): %v\n", style.Error.Render("✖"), relPath, rc.Hooks.Provider, syncErr)
+					fmt.Printf("  %s %s (%s): %v\n", style.Error.Render("✖"), relPath, hooksProvider, syncErr)
 					errors++
 					failedTargets = append(failedTargets, relPath)
 					continue
 				}
 
 				switch result {
 				case hooks.SyncCreated:
-					fmt.Printf("  %s %s %s\n", style.Success.Render("✓"), relPath, style.Dim.Render("(created "+rc.Hooks.Provider+")"))
+					fmt.Printf("  %s %s %s\n", style.Success.Render("✓"), relPath, style.Dim.Render("(created "+hooksProvider+")"))
 					created++
 				case hooks.SyncUpdated:
-					fmt.Printf("  %s %s %s\n", style.Success.Render("✓"), relPath, style.Dim.Render("(updated "+rc.Hooks.Provider+")"))
+					fmt.Printf("  %s %s %s\n", style.Success.Render("✓"), relPath, style.Dim.Render("(updated "+hooksProvider+")"))
 					updated++
 				case hooks.SyncUnchanged:
-					fmt.Printf("  %s %s %s\n", style.Dim.Render("·"), relPath, style.Dim.Render("(unchanged "+rc.Hooks.Provider+")"))
+					fmt.Printf("  %s %s %s\n", style.Dim.Render("·"), relPath, style.Dim.Render("(unchanged "+hooksProvider+")"))
 					unchanged++
 				}
 			}

internal/cmd/wl_stamp_loop_test.go

@@ -128,7 +128,6 @@ func TestStampLoop_EndToEnd(t *testing.T) {
 // TestStampLoop_SelfStampFails verifies the yearbook rule (author != subject).
 // Not parallel: mutates package-level wlStamp* globals.
 func TestStampLoop_SelfStampFails(t *testing.T) {
-
 	// Save/restore globals
 	origQ, origR, origC := wlStampQuality, wlStampReliability, wlStampCreativity
 	origSev, origType, origCtx := wlStampSeverity, wlStampType, wlStampContextType

internal/doctor/hooks_sync_check.go

@@ -99,23 +99,28 @@ func (c *HooksSyncCheck) Run(ctx *CheckContext) *CheckResult {
 			if loc.Rig != "" {
 				rigPath = filepath.Join(ctx.TownRoot, loc.Rig)
 			}
-			// Use ResolveRoleAgentName + ResolveAgentConfigByName so the check works
-			// even when the agent binary is not installed on this machine.
+			// Use ResolveRoleAgentName (not ResolveRoleAgentConfig) so that checks are
+			// based on the *configured* agent, not the *resolved* one.
+			// ResolveRoleAgentConfig falls back to claude when the agent binary is not
+			// found in PATH (e.g., in CI), which would silently skip non-Claude targets.
 			agentName, _ := config.ResolveRoleAgentName(loc.Role, ctx.TownRoot, rigPath)
-			if agentName == "" || agentName == "claude" {
+			if agentName == "" {
 				continue
 			}
-			rc := config.ResolveAgentConfigByName(agentName, ctx.TownRoot, rigPath)
-			if rc == nil || rc.Hooks == nil || rc.Hooks.Provider == "" {
+			preset := config.GetAgentPresetByName(agentName)
+			if preset == nil || preset.HooksDir == "" || preset.HooksSettingsFile == "" {
 				continue
 			}
+			hooksProvider := preset.HooksProvider
+			if hooksProvider == "" {
+				hooksProvider = agentName
+			}
 			// Claude targets are handled by Loop 1.
-			if rc.Hooks.Provider == "claude" {
+			if hooksProvider == "claude" {
 				continue
 			}
 
-			preset := config.GetAgentPresetByName(rc.Hooks.Provider)
-			useSettingsDir := preset != nil && preset.HooksUseSettingsDir
+			useSettingsDir := preset.HooksUseSettingsDir
 
 			var checkDirs []string
 			if loc.Rig == "" || useSettingsDir {
@@ -126,41 +131,41 @@ func (c *HooksSyncCheck) Run(ctx *CheckContext) *CheckResult {
 
 			for _, dir := range checkDirs {
 				totalTargets++
-				targetPath := filepath.Join(dir, rc.Hooks.Dir, rc.Hooks.SettingsFile)
+				targetPath := filepath.Join(dir, preset.HooksDir, preset.HooksSettingsFile)
 
-				expected, err := hooks.ComputeExpectedTemplate(rc.Hooks.Provider, rc.Hooks.SettingsFile, loc.Role)
+				expected, err := hooks.ComputeExpectedTemplate(hooksProvider, preset.HooksSettingsFile, loc.Role)
 				if err != nil {
-					details = append(details, fmt.Sprintf("%s (%s): error computing template: %v", targetPath, rc.Hooks.Provider, err))
+					details = append(details, fmt.Sprintf("%s (%s): error computing template: %v", targetPath, hooksProvider, err))
 					continue
 				}
 
 				actual, readErr := os.ReadFile(targetPath)
 				if readErr != nil {
 					// File missing
 					c.templateOutOfSync = append(c.templateOutOfSync, templateTarget{
-						path: targetPath, dir: dir, provider: rc.Hooks.Provider,
-						role: loc.Role, hooksDir: rc.Hooks.Dir,
-						settingsFile: rc.Hooks.SettingsFile, useSettingsDir: useSettingsDir,
+						path: targetPath, dir: dir, provider: hooksProvider,
+						role: loc.Role, hooksDir: preset.HooksDir,
+						settingsFile: preset.HooksSettingsFile, useSettingsDir: useSettingsDir,
 					})
-					details = append(details, fmt.Sprintf("%s (%s): missing", targetPath, rc.Hooks.Provider))
+					details = append(details, fmt.Sprintf("%s (%s): missing", targetPath, hooksProvider))
 					continue
 				}
 
 				// Compare: structural for JSON, byte-exact for other files.
 				inSync := false
-				if filepath.Ext(rc.Hooks.SettingsFile) == ".json" {
+				if filepath.Ext(preset.HooksSettingsFile) == ".json" {
 					inSync = hooks.TemplateContentEqual(expected, actual)
 				} else {
 					inSync = bytes.Equal(expected, actual)
 				}
 
 				if !inSync {
 					c.templateOutOfSync = append(c.templateOutOfSync, templateTarget{
-						path: targetPath, dir: dir, provider: rc.Hooks.Provider,
-						role: loc.Role, hooksDir: rc.Hooks.Dir,
-						settingsFile: rc.Hooks.SettingsFile, useSettingsDir: useSettingsDir,
+						path: targetPath, dir: dir, provider: hooksProvider,
+						role: loc.Role, hooksDir: preset.HooksDir,
+						settingsFile: preset.HooksSettingsFile, useSettingsDir: useSettingsDir,
 					})
-					details = append(details, fmt.Sprintf("%s (%s): out of sync", targetPath, rc.Hooks.Provider))
+					details = append(details, fmt.Sprintf("%s (%s): out of sync", targetPath, hooksProvider))
 				}
 			}
 		}

internal/hooks/installer_test.go

@@ -1,6 +1,7 @@
 package hooks
 
 import (
+	"encoding/json"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -230,14 +231,21 @@ func TestSyncForRole_JSONWhitespaceInsensitive(t *testing.T) {
 		t.Fatalf("reading created file: %v", err)
 	}
 
-	// Add extra whitespace — structurally identical JSON, different bytes.
-	// Use commas (not colons) since Windows paths contain "D:" which would
-	// corrupt the JSON value when colons are naively replaced.
-	reformatted := strings.ReplaceAll(string(original), ",", " , ")
-	if string(original) == reformatted {
+	// Reformat with different whitespace by round-tripping through json.MarshalIndent.
+	// This changes indentation structure without corrupting string values (safe on Windows
+	// where strings.ReplaceAll(":", " : ") would corrupt drive letters like C: → C :).
+	var parsed interface{}
+	if err := json.Unmarshal(original, &parsed); err != nil {
+		t.Fatalf("parsing original JSON: %v", err)
+	}
+	reformatted, err := json.MarshalIndent(parsed, "", "    ")
+	if err != nil {
+		t.Fatalf("reformatting JSON: %v", err)
+	}
+	if string(original) == string(reformatted) {
 		t.Fatal("reformatted content should differ from original bytes")
 	}
-	if err := os.WriteFile(targetPath, []byte(reformatted), 0600); err != nil {
+	if err := os.WriteFile(targetPath, reformatted, 0600); err != nil {
 		t.Fatalf("writing reformatted file: %v", err)
 	}
 
@@ -267,12 +275,10 @@ func TestSyncForRole_GeminiWithGTBinSubstitution(t *testing.T) {
 	if strings.Contains(string(got), "{{GT_BIN}}") {
 		t.Error("{{GT_BIN}} placeholder was not substituted")
 	}
-	// Verify the resolved binary path is present.
-	// JSON settings files have backslashes escaped (Windows paths), so check
-	// for the JSON-encoded form of the path rather than the raw path.
+	// Verify the resolved binary path is present (JSON-escaped for Windows compatibility).
 	gtBin := resolveGTBinary()
-	gtBinInFile := strings.ReplaceAll(gtBin, `\`, `\\`)
-	if !strings.Contains(string(got), gtBinInFile) {
+	gtBinJSON := strings.ReplaceAll(gtBin, `\`, `\\`)
+	if !strings.Contains(string(got), gtBinJSON) {
 		t.Errorf("expected resolved gt binary %q in output", gtBin)
 	}
 }
@@ -377,8 +383,7 @@ func TestInstallForRole_GeminiRoleAware(t *testing.T) {
 	got, _ := os.ReadFile(filepath.Join(dir, ".gemini", "settings.json"))
 	want, _ := templateFS.ReadFile("templates/gemini/settings-autonomous.json")
 	// Gemini templates contain {{GT_BIN}} which gets resolved at install time.
-	// JSON settings files have backslashes escaped (Windows paths), so apply
-	// the same JSON-escaping used by resolveAndSubstitute for comparison.
+	// Apply the same substitution (with JSON escaping) to the expected content for comparison.
 	gtBin := resolveGTBinary()
 	gtBinJSON := strings.ReplaceAll(gtBin, `\`, `\\`)
 	wantResolved := strings.ReplaceAll(string(want), "{{GT_BIN}}", gtBinJSON)