fix(config): implement role_agents support in BuildStartupCommand (#19) (#456)

* fix(config): implement role_agents support in BuildStartupCommand The role_agents field in TownSettings and RigSettings existed but was not being used by the startup command builders. All services fell back to the default agent instead of using role-specific agent assignments. Changes: - BuildStartupCommand now extracts GT_ROLE from envVars and uses ResolveRoleAgentConfig() for role-based agent selection - BuildStartupCommandWithAgentOverride follows the same pattern when no explicit override is provided - refinery/manager.go uses ResolveRoleAgentConfig with constants - cmd/start.go uses ResolveRoleAgentConfig with constants - Updated comments from hardcoded agent name to generic "agent" - Added ValidateAgentConfig() to check agent exists and binary is in PATH - Added lookupAgentConfigIfExists() helper for validation - ResolveRoleAgentConfig now warns to stderr and falls back to default if configured agent is invalid or binary is missing Resolution priority (now working): 1. Explicit --agent override 2. Rig's role_agents[role] (validated) 3. Town's role_agents[role] (validated) 4. Rig's agent setting 5. Town's default_agent 6. Hardcoded default fallback Adds tests for: - TestBuildStartupCommand_UsesRoleAgentsFromTownSettings - TestBuildStartupCommand_RigRoleAgentsOverridesTownRoleAgents - TestBuildAgentStartupCommand_UsesRoleAgents - TestValidateAgentConfig - TestResolveRoleAgentConfig_FallsBackOnInvalidAgent Fixes: role_agents configuration not being applied to services * fix(config): add GT_ROOT to BuildStartupCommandWithAgentOverride - Fixes missing GT_ROOT and GT_SESSION_ID_ENV exports in BuildStartupCommandWithAgentOverride, matching BuildStartupCommand behavior - Adds test for override priority over role_agents - Adds test verifying GT_ROOT is included in command This addresses the Greptile review comment about agents started with an override not having access to town-level resources. Co-authored-by: Steve Yegge <steve.yegge@gmail.com>
2026-01-14 03:04:22 +05:30
parent fa99e615f0
commit c61b67eb03
4 changed files with 393 additions and 30 deletions
--- a/internal/config/loader_test.go
+++ b/internal/config/loader_test.go
@@ -6,6 +6,8 @@ import (
 	"strings"
 	"testing"
 	"time"
+
+	"github.com/steveyegge/gastown/internal/constants"
 )

 func TestTownConfigRoundTrip(t *testing.T) {
@@ -1193,6 +1195,189 @@ func TestBuildStartupCommand_UsesRigAgentWhenRigPathProvided(t *testing.T) {
 	}
 }

+func TestBuildStartupCommand_UsesRoleAgentsFromTownSettings(t *testing.T) {
+	t.Parallel()
+	townRoot := t.TempDir()
+	rigPath := filepath.Join(townRoot, "testrig")
+
+	// Configure town settings with role_agents
+	townSettings := NewTownSettings()
+	townSettings.DefaultAgent = "claude"
+	townSettings.RoleAgents = map[string]string{
+		constants.RoleRefinery: "gemini",
+		constants.RoleWitness:  "codex",
+	}
+	if err := SaveTownSettings(TownSettingsPath(townRoot), townSettings); err != nil {
+		t.Fatalf("SaveTownSettings: %v", err)
+	}
+
+	// Create empty rig settings (no agent override)
+	rigSettings := NewRigSettings()
+	if err := SaveRigSettings(RigSettingsPath(rigPath), rigSettings); err != nil {
+		t.Fatalf("SaveRigSettings: %v", err)
+	}
+
+	t.Run("refinery role gets gemini from role_agents", func(t *testing.T) {
+		cmd := BuildStartupCommand(map[string]string{"GT_ROLE": constants.RoleRefinery}, rigPath, "")
+		if !strings.Contains(cmd, "gemini") {
+			t.Fatalf("expected gemini for refinery role, got: %q", cmd)
+		}
+	})
+
+	t.Run("witness role gets codex from role_agents", func(t *testing.T) {
+		cmd := BuildStartupCommand(map[string]string{"GT_ROLE": constants.RoleWitness}, rigPath, "")
+		if !strings.Contains(cmd, "codex") {
+			t.Fatalf("expected codex for witness role, got: %q", cmd)
+		}
+	})
+
+	t.Run("crew role falls back to default_agent (not in role_agents)", func(t *testing.T) {
+		cmd := BuildStartupCommand(map[string]string{"GT_ROLE": constants.RoleCrew}, rigPath, "")
+		if !strings.Contains(cmd, "claude") {
+			t.Fatalf("expected claude fallback for crew role, got: %q", cmd)
+		}
+	})
+
+	t.Run("no role falls back to default resolution", func(t *testing.T) {
+		cmd := BuildStartupCommand(map[string]string{}, rigPath, "")
+		if !strings.Contains(cmd, "claude") {
+			t.Fatalf("expected claude for no role, got: %q", cmd)
+		}
+	})
+}
+
+func TestBuildStartupCommand_RigRoleAgentsOverridesTownRoleAgents(t *testing.T) {
+	t.Parallel()
+	townRoot := t.TempDir()
+	rigPath := filepath.Join(townRoot, "testrig")
+
+	// Town settings has witness = gemini
+	townSettings := NewTownSettings()
+	townSettings.DefaultAgent = "claude"
+	townSettings.RoleAgents = map[string]string{
+		constants.RoleWitness: "gemini",
+	}
+	if err := SaveTownSettings(TownSettingsPath(townRoot), townSettings); err != nil {
+		t.Fatalf("SaveTownSettings: %v", err)
+	}
+
+	// Rig settings overrides witness to codex
+	rigSettings := NewRigSettings()
+	rigSettings.RoleAgents = map[string]string{
+		constants.RoleWitness: "codex",
+	}
+	if err := SaveRigSettings(RigSettingsPath(rigPath), rigSettings); err != nil {
+		t.Fatalf("SaveRigSettings: %v", err)
+	}
+
+	cmd := BuildStartupCommand(map[string]string{"GT_ROLE": constants.RoleWitness}, rigPath, "")
+	if !strings.Contains(cmd, "codex") {
+		t.Fatalf("expected codex from rig role_agents override, got: %q", cmd)
+	}
+	if strings.Contains(cmd, "gemini") {
+		t.Fatalf("did not expect town role_agents (gemini) in command: %q", cmd)
+	}
+}
+
+func TestBuildAgentStartupCommand_UsesRoleAgents(t *testing.T) {
+	t.Parallel()
+	townRoot := t.TempDir()
+	rigPath := filepath.Join(townRoot, "testrig")
+
+	// Configure town settings with role_agents
+	townSettings := NewTownSettings()
+	townSettings.DefaultAgent = "claude"
+	townSettings.RoleAgents = map[string]string{
+		constants.RoleRefinery: "codex",
+	}
+	if err := SaveTownSettings(TownSettingsPath(townRoot), townSettings); err != nil {
+		t.Fatalf("SaveTownSettings: %v", err)
+	}
+
+	// Create empty rig settings
+	rigSettings := NewRigSettings()
+	if err := SaveRigSettings(RigSettingsPath(rigPath), rigSettings); err != nil {
+		t.Fatalf("SaveRigSettings: %v", err)
+	}
+
+	// BuildAgentStartupCommand passes role via GT_ROLE env var
+	cmd := BuildAgentStartupCommand(constants.RoleRefinery, "testrig/refinery", rigPath, "")
+	if !strings.Contains(cmd, "codex") {
+		t.Fatalf("expected codex for refinery role, got: %q", cmd)
+	}
+	if !strings.Contains(cmd, "GT_ROLE="+constants.RoleRefinery) {
+		t.Fatalf("expected GT_ROLE=%s in command: %q", constants.RoleRefinery, cmd)
+	}
+}
+
+func TestValidateAgentConfig(t *testing.T) {
+	t.Parallel()
+
+	t.Run("valid built-in agent", func(t *testing.T) {
+		// claude is a built-in preset and binary should exist
+		err := ValidateAgentConfig("claude", nil, nil)
+		// Note: This may fail if claude binary is not installed, which is expected
+		if err != nil && !strings.Contains(err.Error(), "not found in PATH") {
+			t.Errorf("unexpected error for claude: %v", err)
+		}
+	})
+
+	t.Run("invalid agent name", func(t *testing.T) {
+		err := ValidateAgentConfig("nonexistent-agent-xyz", nil, nil)
+		if err == nil {
+			t.Error("expected error for nonexistent agent")
+		}
+		if !strings.Contains(err.Error(), "not found in config or built-in presets") {
+			t.Errorf("unexpected error message: %v", err)
+		}
+	})
+
+	t.Run("custom agent with missing binary", func(t *testing.T) {
+		townSettings := NewTownSettings()
+		townSettings.Agents = map[string]*RuntimeConfig{
+			"my-custom-agent": {
+				Command: "nonexistent-binary-xyz123",
+				Args:    []string{"--some-flag"},
+			},
+		}
+		err := ValidateAgentConfig("my-custom-agent", townSettings, nil)
+		if err == nil {
+			t.Error("expected error for missing binary")
+		}
+		if !strings.Contains(err.Error(), "not found in PATH") {
+			t.Errorf("unexpected error message: %v", err)
+		}
+	})
+}
+
+func TestResolveRoleAgentConfig_FallsBackOnInvalidAgent(t *testing.T) {
+	t.Parallel()
+	townRoot := t.TempDir()
+	rigPath := filepath.Join(townRoot, "testrig")
+
+	// Configure town settings with an invalid agent for refinery
+	townSettings := NewTownSettings()
+	townSettings.DefaultAgent = "claude"
+	townSettings.RoleAgents = map[string]string{
+		constants.RoleRefinery: "nonexistent-agent-xyz", // Invalid agent
+	}
+	if err := SaveTownSettings(TownSettingsPath(townRoot), townSettings); err != nil {
+		t.Fatalf("SaveTownSettings: %v", err)
+	}
+
+	// Create empty rig settings
+	rigSettings := NewRigSettings()
+	if err := SaveRigSettings(RigSettingsPath(rigPath), rigSettings); err != nil {
+		t.Fatalf("SaveRigSettings: %v", err)
+	}
+
+	// Should fall back to default (claude) when agent is invalid
+	rc := ResolveRoleAgentConfig(constants.RoleRefinery, townRoot, rigPath)
+	if rc.Command != "claude" {
+		t.Errorf("expected fallback to claude, got: %s", rc.Command)
+	}
+}
+
 func TestGetRuntimeCommand_UsesRigAgentWhenRigPathProvided(t *testing.T) {
 	t.Parallel()
 	townRoot := t.TempDir()
@@ -2204,8 +2389,8 @@ func TestEscalationConfigGetRouteForSeverity(t *testing.T) {
 	}{
 		{SeverityLow, []string{"bead"}},
 		{SeverityMedium, []string{"bead", "mail:mayor"}},
-		{SeverityHigh, []string{"bead", "mail:mayor"}},       // fallback for missing
-		{SeverityCritical, []string{"bead", "mail:mayor"}},   // fallback for missing
+		{SeverityHigh, []string{"bead", "mail:mayor"}},     // fallback for missing
+		{SeverityCritical, []string{"bead", "mail:mayor"}}, // fallback for missing
 	}

 	for _, tt := range tests {
@@ -2321,3 +2506,73 @@ func TestEscalationConfigPath(t *testing.T) {
 		t.Errorf("EscalationConfigPath = %q, want %q", path, expected)
 	}
 }
+
+func TestBuildStartupCommandWithAgentOverride_PriorityOverRoleAgents(t *testing.T) {
+	t.Parallel()
+	townRoot := t.TempDir()
+	rigPath := filepath.Join(townRoot, "testrig")
+
+	// Configure town settings with role_agents: refinery = codex
+	townSettings := NewTownSettings()
+	townSettings.DefaultAgent = "claude"
+	townSettings.RoleAgents = map[string]string{
+		constants.RoleRefinery: "codex",
+	}
+	if err := SaveTownSettings(TownSettingsPath(townRoot), townSettings); err != nil {
+		t.Fatalf("SaveTownSettings: %v", err)
+	}
+
+	// Create empty rig settings
+	rigSettings := NewRigSettings()
+	if err := SaveRigSettings(RigSettingsPath(rigPath), rigSettings); err != nil {
+		t.Fatalf("SaveRigSettings: %v", err)
+	}
+
+	// agentOverride = "gemini" should take priority over role_agents[refinery] = "codex"
+	cmd, err := BuildStartupCommandWithAgentOverride(
+		map[string]string{"GT_ROLE": constants.RoleRefinery},
+		rigPath,
+		"",
+		"gemini", // explicit override
+	)
+	if err != nil {
+		t.Fatalf("BuildStartupCommandWithAgentOverride: %v", err)
+	}
+
+	if !strings.Contains(cmd, "gemini") {
+		t.Errorf("expected gemini (override) in command, got: %q", cmd)
+	}
+	if strings.Contains(cmd, "codex") {
+		t.Errorf("did not expect codex (role_agents) when override is set: %q", cmd)
+	}
+}
+
+func TestBuildStartupCommandWithAgentOverride_IncludesGTRoot(t *testing.T) {
+	t.Parallel()
+	townRoot := t.TempDir()
+	rigPath := filepath.Join(townRoot, "testrig")
+
+	// Create necessary config files
+	townSettings := NewTownSettings()
+	if err := SaveTownSettings(TownSettingsPath(townRoot), townSettings); err != nil {
+		t.Fatalf("SaveTownSettings: %v", err)
+	}
+	if err := SaveRigSettings(RigSettingsPath(rigPath), NewRigSettings()); err != nil {
+		t.Fatalf("SaveRigSettings: %v", err)
+	}
+
+	cmd, err := BuildStartupCommandWithAgentOverride(
+		map[string]string{"GT_ROLE": constants.RoleWitness},
+		rigPath,
+		"",
+		"gemini",
+	)
+	if err != nil {
+		t.Fatalf("BuildStartupCommandWithAgentOverride: %v", err)
+	}
+
+	// Should include GT_ROOT in export
+	if !strings.Contains(cmd, "GT_ROOT="+townRoot) {
+		t.Errorf("expected GT_ROOT=%s in command, got: %q", townRoot, cmd)
+	}
+}