From df6d8b6cef1219de90ee1f01a62c9fb7d8a3867c Mon Sep 17 00:00:00 2001 From: Greg Allen Date: Sat, 11 Jul 2026 20:51:47 -0400 Subject: [PATCH] feat(#4026): support disabling agents via config enabled field Add `Enabled *bool` to `AgentEntry` so operators can selectively disable agents (including scaffold defaults) without removing them from config. Nil defaults to enabled for backward compatibility. - Filter disabled agents in `MergedAgents()` and block fallback in `resolveAgentSource` - Gate dispatch on `agents[].enabled` in scaffold and reusable workflows with last-writer-wins semantics for enable-then-disable patterns - Add pre-dispatch config validation to catch malformed entries - Allow suppression-only entries (`enabled: false`, no source) to disable scaffold defaults by name - Allow disable-then-enable and enable-then-disable patterns for the same agent name in validation Closes #4026 Signed-off-by: Claude Signed-off-by: Greg Allen --- .github/workflows/reusable-dispatch.yml | 48 +++- docs/ADRs/0058-agent-registration.md | 6 +- docs/guides/user/customizing-agents.md | 22 ++ docs/plans/agent-extraction-to-agents-repo.md | 8 + docs/plans/agent-registration.md | 9 +- internal/cli/run.go | 7 + internal/cli/run_test.go | 92 ++++++++ internal/config/agents.go | 35 ++- internal/config/agents_test.go | 161 +++++++++++++ internal/config/config.go | 64 +++++- internal/config/config_test.go | 215 +++++++++++++++++- .../.github/workflows/dispatch.yml | 44 +++- 12 files changed, 693 insertions(+), 18 deletions(-) diff --git a/.github/workflows/reusable-dispatch.yml b/.github/workflows/reusable-dispatch.yml index cba6b359d..015cdf4e3 100644 --- a/.github/workflows/reusable-dispatch.yml +++ b/.github/workflows/reusable-dispatch.yml @@ -81,7 +81,7 @@ jobs: contents: read pull-requests: read outputs: - stage: ${{ steps.role-check.outputs.skipped != 'true' && steps.pr-check.outputs.skipped != 'true' && steps.route.outputs.stage || '' }} + stage: ${{ steps.role-check.outputs.skipped != 'true' && steps.agent-check.outputs.skipped != 'true' && steps.pr-check.outputs.skipped != 'true' && steps.route.outputs.stage || '' }} trigger_source: ${{ steps.route.outputs.trigger_source }} event_payload: ${{ steps.payload.outputs.event_payload }} steps: @@ -366,6 +366,23 @@ jobs: fi fi + - name: Validate agents config + if: steps.route.outputs.stage != '' + run: | + set -euo pipefail + if [[ ! -f .fullsend/config.yaml ]]; then + exit 0 + fi + AGENT_COUNT=$(yq '.agents | length // 0' .fullsend/config.yaml 2>/dev/null || echo "0") + if [[ "$AGENT_COUNT" == "0" ]]; then + exit 0 + fi + BAD=$(yq '.agents[] | select(.enabled == false and (.name == null or .name == "")) | line' .fullsend/config.yaml 2>/dev/null || echo "") + if [[ -n "$BAD" ]]; then + echo "::error::config.yaml: disabled agent entry without a name field — add 'name:' to identify which agent to disable" + exit 1 + fi + - name: Check role is enabled id: role-check if: steps.route.outputs.stage != '' @@ -392,8 +409,35 @@ jobs: fi fi + - name: Check agent is enabled + id: agent-check + if: steps.route.outputs.stage != '' && steps.role-check.outputs.skipped != 'true' && steps.pr-check.outputs.skipped != 'true' + env: + STAGE: ${{ steps.route.outputs.stage }} + run: | + set -euo pipefail + if [[ ! -f .fullsend/config.yaml ]]; then + exit 0 + fi + YQ_ERR=$(mktemp) + AGENT_ENABLED=$(yq " + .agents[] | + select((.name | downcase) == \"$STAGE\") | + .enabled + " .fullsend/config.yaml 2>"$YQ_ERR" | tail -1) || { + echo "::warning::yq failed checking agents[].enabled: $(cat "$YQ_ERR")" + rm -f "$YQ_ERR" + exit 0 + } + rm -f "$YQ_ERR" + if [[ "$AGENT_ENABLED" == "false" ]]; then + echo "::notice::Stage '$STAGE' skipped — agent '$STAGE' is disabled in config" + echo "skipped=true" >> "${GITHUB_OUTPUT}" + exit 0 + fi + - name: Block fork PRs for fix stage - if: steps.route.outputs.stage == 'fix' && steps.role-check.outputs.skipped != 'true' && github.event.issue.pull_request + if: steps.route.outputs.stage == 'fix' && steps.role-check.outputs.skipped != 'true' && steps.agent-check.outputs.skipped != 'true' && github.event.issue.pull_request env: GH_TOKEN: ${{ github.token }} SOURCE_REPO: ${{ github.repository }} diff --git a/docs/ADRs/0058-agent-registration.md b/docs/ADRs/0058-agent-registration.md index aef2d469c..9983d99ec 100644 --- a/docs/ADRs/0058-agent-registration.md +++ b/docs/ADRs/0058-agent-registration.md @@ -41,13 +41,17 @@ list to both `OrgConfig` and `PerRepoConfig`. (Note: ADR 0045 Phase 4 previously removed the `agents` block from `OrgConfig`; this re-adds a field with the same YAML key but different semantics — harness source URLs rather than role/name/slug identity tuples.) Each entry -is a URL or local path, with an optional name override: +is a URL or local path, with an optional name override. Entries may +also set `enabled: false` to disable an agent (including scaffold +defaults) without removing it from configuration: ```yaml agents: - https://raw.githubusercontent.com/fullsend-ai/agents//harness/triage.yaml#sha256= - name: lint source: harness/my-linter.yaml + - name: retro + enabled: false # suppression-only — disables scaffold default ``` `fullsend run ` resolves agents from config at runtime, loading diff --git a/docs/guides/user/customizing-agents.md b/docs/guides/user/customizing-agents.md index 8892d2807..07935896f 100644 --- a/docs/guides/user/customizing-agents.md +++ b/docs/guides/user/customizing-agents.md @@ -415,6 +415,28 @@ my-repo/ │ └── harness/code.yaml # Repo-specific harness config ``` +## Disabling Agents + +To disable an agent (including built-in scaffold agents) without removing +its role, add an entry with `enabled: false` in your config: + +```yaml +agents: + - name: retro + enabled: false +``` + +This prevents the agent from dispatching and from resolving via +`fullsend run`. The role can stay in `defaults.roles` — only the agent +is suppressed. Omitting `enabled` (or setting it to `true`) keeps the +agent active (backward compatible). + +**Important:** The `name` must match the **agent/harness name**, not the +role name. The built-in agent names are: `code`, `triage`, `review`, +`fix`, `retro`, `prioritize`. Note that the role `coder` maps to the +agent named `code` — writing `name: coder` passes validation but +disables nothing because no agent has that harness name. + ## See Also - [Default, derived, and custom agents](../../agents/topics/default-vs-custom.md) - When does configuration cross into derived or custom agent territory? diff --git a/docs/plans/agent-extraction-to-agents-repo.md b/docs/plans/agent-extraction-to-agents-repo.md index 609cc88cd..3dd8701e7 100644 --- a/docs/plans/agent-extraction-to-agents-repo.md +++ b/docs/plans/agent-extraction-to-agents-repo.md @@ -592,6 +592,14 @@ agents: - source: https://raw.githubusercontent.com/fullsend-ai/agents//harness/prioritize.yaml#sha256= ``` +To selectively disable an agent, add an `enabled: false` entry: + +```yaml +agents: + - name: retro + enabled: false +``` + #### 6b. Verify agent resolution Run `fullsend agent list` from the `.fullsend` checkout to verify all diff --git a/docs/plans/agent-registration.md b/docs/plans/agent-registration.md index 40fe4c77e..37c9ecbd1 100644 --- a/docs/plans/agent-registration.md +++ b/docs/plans/agent-registration.md @@ -17,11 +17,16 @@ object form via a custom YAML unmarshaler: ```go type AgentEntry struct { - Name string `yaml:"name,omitempty"` - Source string `yaml:"source"` + Name string `yaml:"name,omitempty"` + Source string `yaml:"source"` + Enabled *bool `yaml:"enabled,omitempty"` } ``` +When `Enabled` is nil (omitted), the entry defaults to enabled. +Setting `enabled: false` suppresses the agent from the merged set +and blocks resolution. Disabled entries must have an explicit `Name`. + `AgentEntry` implements `yaml.Unmarshaler`: if the YAML node is a scalar string, it populates `Source` and leaves `Name` empty (derived from the source filename at usage time). If the node is a mapping, it diff --git a/internal/cli/run.go b/internal/cli/run.go index 6a11d9cf6..059ebeb0a 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -2835,6 +2835,13 @@ func resolveAgentSource(ctx context.Context, fullsendDir, agentName string, forg agent := config.LookupMergedAgent(merged, agentName) if agent == nil || !agent.IsConfig { + // An explicitly disabled agent must not fall through to the + // agents-repo fallback or disk lookup — that would silently + // re-enable it. Return a clear error instead. + if config.IsAgentExplicitlyDisabled(orgCfg.Agents, agentName) { + printer.StepFail(fmt.Sprintf("Agent %s is disabled in config", agentName)) + return "", nil, fmt.Errorf("agent %q is explicitly disabled in config", agentName) + } if path, deps, ok := tryAgentsRepoFallback(ctx, agentName, forgeClient, composeOpts, printer); ok { return path, deps, nil } diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index 3eedf6382..51375f52c 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -1025,6 +1025,98 @@ func TestResolveAgentSource_AgentsRepoFallback_DiskFallback(t *testing.T) { assert.Empty(t, deps) } +func TestResolveAgentSource_DisabledAgentBlocksFallback(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, "harness"), 0o755)) + // Place a triage harness on disk to prove fallback is NOT used. + require.NoError(t, os.WriteFile( + filepath.Join(dir, "harness", "triage.yaml"), + []byte("agent: agents/triage.md\nrole: test\n"), + 0o644, + )) + + f := false + orgCfg := &config.OrgConfig{ + Agents: []config.AgentEntry{ + {Name: "triage", Enabled: &f}, + }, + } + + printer := ui.New(io.Discard) + _, _, err := resolveAgentSource(context.Background(), dir, "triage", nil, orgCfg, harness.ComposeOpts{}, printer) + require.Error(t, err) + assert.Contains(t, err.Error(), "explicitly disabled") +} + +func TestResolveAgentSource_DisabledFirstPartyAgentBlocksFallback(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, "harness"), 0o755)) + // Place a retro harness on disk so it would resolve if fallback were allowed. + require.NoError(t, os.WriteFile( + filepath.Join(dir, "harness", "retro.yaml"), + []byte("agent: agents/retro.md\nrole: test\n"), + 0o644, + )) + + f := false + orgCfg := &config.OrgConfig{ + Agents: []config.AgentEntry{ + {Name: "retro", Enabled: &f}, + }, + } + + fakeClient := forge.NewFakeClient() + printer := ui.New(io.Discard) + _, _, err := resolveAgentSource(context.Background(), dir, "retro", fakeClient, orgCfg, harness.ComposeOpts{}, printer) + require.Error(t, err) + assert.Contains(t, err.Error(), "explicitly disabled") +} + +func TestResolveAgentSource_SuppressionOnlyEntryBlocksFallback(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, "harness"), 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(dir, "harness", "retro.yaml"), + []byte("agent: agents/retro.md\nrole: test\n"), + 0o644, + )) + + // Suppression-only entry: enabled=false, no source. + f := false + orgCfg := &config.OrgConfig{ + Agents: []config.AgentEntry{ + {Name: "retro", Enabled: &f}, + }, + } + + printer := ui.New(io.Discard) + _, _, err := resolveAgentSource(context.Background(), dir, "retro", nil, orgCfg, harness.ComposeOpts{}, printer) + require.Error(t, err) + assert.Contains(t, err.Error(), "explicitly disabled") +} + +func TestResolveAgentSource_EnabledAgentStillResolves(t *testing.T) { + dir := canonTempDir(t) + require.NoError(t, os.MkdirAll(filepath.Join(dir, "harness"), 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(dir, "harness", "custom.yaml"), + []byte("agent: agents/custom.md\nrole: test\n"), + 0o644, + )) + + tr := true + orgCfg := &config.OrgConfig{ + Agents: []config.AgentEntry{ + {Name: "custom", Source: "harness/custom.yaml", Enabled: &tr}, + }, + } + + printer := ui.New(io.Discard) + path, _, err := resolveAgentSource(context.Background(), dir, "custom", nil, orgCfg, harness.ComposeOpts{}, printer) + require.NoError(t, err) + assert.Contains(t, path, "custom.yaml") +} + func TestTryAgentsRepoFallback_UnknownAgent(t *testing.T) { fakeClient := forge.NewFakeClient() printer := ui.New(io.Discard) diff --git a/internal/config/agents.go b/internal/config/agents.go index 6e70aeb7f..f1b6b16c0 100644 --- a/internal/config/agents.go +++ b/internal/config/agents.go @@ -42,11 +42,25 @@ func MergedAgents(scaffoldNames []string, commitSHA string, configAgents []Agent } } + inOrder := make(map[string]bool, len(order)) + for _, key := range order { + inOrder[key] = true + } + for _, entry := range configAgents { name := entry.DerivedName() lower := strings.ToLower(name) - if _, exists := byName[lower]; !exists { + + // A disabled entry suppresses the agent from the merged set. + // If it overrides a scaffold entry, that scaffold entry is removed. + if !entry.IsEnabled() { + delete(byName, lower) + continue + } + + if !inOrder[lower] { order = append(order, lower) + inOrder[lower] = true } byName[lower] = &MergedAgent{ Name: name, @@ -57,7 +71,9 @@ func MergedAgents(scaffoldNames []string, commitSHA string, configAgents []Agent result := make([]MergedAgent, 0, len(byName)) for _, key := range order { - result = append(result, *byName[key]) + if agent, ok := byName[key]; ok { + result = append(result, *agent) + } } sort.Slice(result, func(i, j int) bool { return strings.ToLower(result[i].Name) < strings.ToLower(result[j].Name) @@ -66,6 +82,21 @@ func MergedAgents(scaffoldNames []string, commitSHA string, configAgents []Agent return result, nil } +// IsAgentExplicitlyDisabled returns true if the agent name appears in the +// config entries with Enabled explicitly set to false. This allows callers +// to distinguish "agent is explicitly disabled" from "agent is not in the +// config at all" — a distinction that MergedAgents erases (both cases +// result in the agent being absent from the merged set). +func IsAgentExplicitlyDisabled(agents []AgentEntry, name string) bool { + lower := strings.ToLower(name) + for _, entry := range agents { + if strings.ToLower(entry.DerivedName()) == lower && !entry.IsEnabled() { + return true + } + } + return false +} + // LookupMergedAgent finds an agent by name (case-insensitive) in the merged set. // Returns nil if not found. func LookupMergedAgent(agents []MergedAgent, name string) *MergedAgent { diff --git a/internal/config/agents_test.go b/internal/config/agents_test.go index 074b5b55e..129cce004 100644 --- a/internal/config/agents_test.go +++ b/internal/config/agents_test.go @@ -134,6 +134,167 @@ func TestMergedAgents_BuilderError(t *testing.T) { assert.Contains(t, err.Error(), "build failed for code") } +func TestMergedAgents_DisabledScaffoldAgent(t *testing.T) { + f := false + agents := []AgentEntry{{Name: "triage", Enabled: &f}} + result, err := MergedAgents( + []string{"code", "triage", "retro"}, + "abc123def456abc123def456abc123def456abc1", + agents, fakeBuilder, + ) + require.NoError(t, err) + require.Len(t, result, 2) + assert.Equal(t, "code", result[0].Name) + assert.Equal(t, "retro", result[1].Name) +} + +func TestMergedAgents_EnabledDefaultTrue(t *testing.T) { + // Entry with nil Enabled (omitted) should be included — backward compatible. + agents := []AgentEntry{{Source: "harness/custom.yaml"}} + result, err := MergedAgents(nil, "", agents, nil) + require.NoError(t, err) + require.Len(t, result, 1) + assert.Equal(t, "custom", result[0].Name) + assert.True(t, result[0].IsConfig) +} + +func TestMergedAgents_EnabledExplicitTrue(t *testing.T) { + tr := true + agents := []AgentEntry{{Source: "harness/custom.yaml", Enabled: &tr}} + result, err := MergedAgents(nil, "", agents, nil) + require.NoError(t, err) + require.Len(t, result, 1) + assert.Equal(t, "custom", result[0].Name) + assert.True(t, result[0].IsConfig) +} + +func TestMergedAgents_DisabledCustomAgent(t *testing.T) { + f := false + agents := []AgentEntry{ + {Name: "my-linter", Source: "harness/lint.yaml", Enabled: &f}, + } + result, err := MergedAgents(nil, "", agents, nil) + require.NoError(t, err) + assert.Empty(t, result) +} + +func TestMergedAgents_SuppressionOnlyEntry(t *testing.T) { + // Entry with enabled=false and no source suppresses scaffold default. + f := false + agents := []AgentEntry{{Name: "retro", Enabled: &f}} + result, err := MergedAgents( + []string{"code", "retro", "triage"}, + "abc123def456abc123def456abc123def456abc1", + agents, fakeBuilder, + ) + require.NoError(t, err) + require.Len(t, result, 2) + assert.Equal(t, "code", result[0].Name) + assert.Equal(t, "triage", result[1].Name) +} + +func TestMergedAgents_DisabledDoesNotAffectOthers(t *testing.T) { + f := false + tr := true + agents := []AgentEntry{ + {Name: "retro", Enabled: &f}, + {Name: "custom", Source: "harness/custom.yaml", Enabled: &tr}, + } + result, err := MergedAgents( + []string{"code", "retro"}, + "abc123def456abc123def456abc123def456abc1", + agents, fakeBuilder, + ) + require.NoError(t, err) + require.Len(t, result, 2) + assert.Equal(t, "code", result[0].Name) + assert.False(t, result[0].IsConfig) + assert.Equal(t, "custom", result[1].Name) + assert.True(t, result[1].IsConfig) +} + +func TestMergedAgents_DisableThenEnableSameName(t *testing.T) { + f := false + tr := true + agents := []AgentEntry{ + {Name: "retro", Enabled: &f}, + {Name: "retro", Source: "harness/retro-custom.yaml", Enabled: &tr}, + } + result, err := MergedAgents( + []string{"code", "retro"}, + "abc123def456abc123def456abc123def456abc1", + agents, fakeBuilder, + ) + require.NoError(t, err) + require.Len(t, result, 2) + names := make(map[string]int) + for _, a := range result { + names[a.Name]++ + } + assert.Equal(t, 1, names["code"]) + assert.Equal(t, 1, names["retro"]) +} + +func TestMergedAgents_EnableThenDisableSameName(t *testing.T) { + f := false + tr := true + agents := []AgentEntry{ + {Name: "retro", Source: "harness/retro-custom.yaml", Enabled: &tr}, + {Name: "retro", Enabled: &f}, + } + result, err := MergedAgents( + []string{"code", "retro"}, + "abc123def456abc123def456abc123def456abc1", + agents, fakeBuilder, + ) + require.NoError(t, err) + require.Len(t, result, 1) + assert.Equal(t, "code", result[0].Name) +} + +func TestIsAgentExplicitlyDisabled_True(t *testing.T) { + f := false + agents := []AgentEntry{ + {Name: "retro", Enabled: &f}, + {Source: "harness/code.yaml"}, + } + assert.True(t, IsAgentExplicitlyDisabled(agents, "retro")) +} + +func TestIsAgentExplicitlyDisabled_CaseInsensitive(t *testing.T) { + f := false + agents := []AgentEntry{{Name: "Retro", Enabled: &f}} + assert.True(t, IsAgentExplicitlyDisabled(agents, "retro")) + assert.True(t, IsAgentExplicitlyDisabled(agents, "RETRO")) +} + +func TestIsAgentExplicitlyDisabled_EnabledAgent(t *testing.T) { + tr := true + agents := []AgentEntry{{Name: "retro", Source: "harness/retro.yaml", Enabled: &tr}} + assert.False(t, IsAgentExplicitlyDisabled(agents, "retro")) +} + +func TestIsAgentExplicitlyDisabled_NilEnabled(t *testing.T) { + agents := []AgentEntry{{Name: "retro", Source: "harness/retro.yaml"}} + assert.False(t, IsAgentExplicitlyDisabled(agents, "retro")) +} + +func TestIsAgentExplicitlyDisabled_NotInConfig(t *testing.T) { + f := false + agents := []AgentEntry{{Name: "retro", Enabled: &f}} + assert.False(t, IsAgentExplicitlyDisabled(agents, "triage")) +} + +func TestIsAgentExplicitlyDisabled_EmptyConfig(t *testing.T) { + assert.False(t, IsAgentExplicitlyDisabled(nil, "retro")) +} + +func TestIsAgentExplicitlyDisabled_DerivedName(t *testing.T) { + f := false + agents := []AgentEntry{{Source: "harness/retro.yaml", Enabled: &f}} + assert.True(t, IsAgentExplicitlyDisabled(agents, "retro")) +} + func TestLookupMergedAgent_Found(t *testing.T) { agents := []MergedAgent{ {Name: "code", Source: "url1"}, diff --git a/internal/config/config.go b/internal/config/config.go index b9b612721..dd142f66b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -20,9 +20,16 @@ var validConfigAgentName = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9_-]*$`) // AgentEntry represents a registered agent source in config. // It supports both string shorthand (just the source URL/path) and // object form (with an explicit name override). +// +// Enabled controls whether the agent participates in the merged agent +// set. When nil (omitted) the agent defaults to enabled. When +// explicitly set to false the agent is suppressed — this allows +// disabling built-in scaffold agents without removing their role. +// A suppression-only entry (Enabled=false, no Source) is valid. type AgentEntry struct { - Name string `yaml:"name,omitempty"` - Source string `yaml:"source"` + Name string `yaml:"name,omitempty"` + Source string `yaml:"source"` + Enabled *bool `yaml:"enabled,omitempty"` } // UnmarshalYAML implements yaml.Unmarshaler so that a plain string @@ -56,6 +63,12 @@ func (a *AgentEntry) UnmarshalYAML(value *yaml.Node) error { return fmt.Errorf("agents entry must be a string or mapping, got %v", value.Kind) } +// IsEnabled returns whether the agent entry is enabled. +// A nil Enabled pointer (field omitted) defaults to true. +func (a AgentEntry) IsEnabled() bool { + return a.Enabled == nil || *a.Enabled +} + // DerivedName returns the explicit Name if set, otherwise derives one // from the Source filename (e.g. "triage.yaml" → "triage"). func (a AgentEntry) DerivedName() string { @@ -354,10 +367,43 @@ func (c *OrgConfig) Validate() error { // resolution (case-insensitive scheme, percent-decoding, dot-segment // cleaning). func ValidateAgentEntries(agents []AgentEntry, allowlist []string) error { - seen := make(map[string]bool, len(agents)) + // seen tracks agent names for duplicate detection. The value records + // whether the previously seen entry was disabled so that + // disable-then-enable and enable-then-disable patterns are accepted + // (MergedAgents handles these with last-writer-wins semantics) while + // true duplicates (two entries with the same enabled state) are still + // rejected. + type seenState struct { + disabled bool + } + seen := make(map[string]seenState, len(agents)) for i, entry := range agents { + // A suppression-only entry (enabled: false, no source) is valid — + // it exists solely to disable a scaffold default by name. + if entry.Source == "" && !entry.IsEnabled() { + if entry.Name == "" { + return fmt.Errorf("agents[%d]: disabled entry with no source must have an explicit name", i) + } + if !validConfigAgentName.MatchString(entry.Name) { + return fmt.Errorf("agents[%d] (%s): name is invalid, must start with alphanumeric and contain only [a-zA-Z0-9_-]", i, entry.Name) + } + lowerName := strings.ToLower(entry.Name) + if prev, exists := seen[lowerName]; exists { + if prev.disabled { + return fmt.Errorf("agents[%d] (%s): duplicate agent name (case-insensitive)", i, entry.Name) + } + // Previous was enabled, this is disabled → enable-then-disable; allowed. + } + seen[lowerName] = seenState{disabled: true} + continue + } + // Disabled entries with a source must also have an explicit name so + // dispatch workflows can match by name via yq without deriving it. + if !entry.IsEnabled() && entry.Name == "" { + return fmt.Errorf("agents[%d]: disabled entry must have an explicit name", i) + } if entry.Source == "" { - return fmt.Errorf("agents[%d]: source must not be empty", i) + return fmt.Errorf("agents[%d]: enabled agent entry must have a source", i) } name := entry.DerivedName() @@ -365,10 +411,14 @@ func ValidateAgentEntries(agents []AgentEntry, allowlist []string) error { return fmt.Errorf("agents[%d] (%s): derived name is invalid, must start with alphanumeric and contain only [a-zA-Z0-9_-] (source: %q)", i, name, entry.Source) } lowerName := strings.ToLower(name) - if seen[lowerName] { - return fmt.Errorf("agents[%d] (%s): duplicate agent name (case-insensitive)", i, name) + currentDisabled := !entry.IsEnabled() + if prev, exists := seen[lowerName]; exists { + if prev.disabled == currentDisabled { + return fmt.Errorf("agents[%d] (%s): duplicate agent name (case-insensitive)", i, name) + } + // Different enabled states → disable-then-enable or enable-then-disable; allowed. } - seen[lowerName] = true + seen[lowerName] = seenState{disabled: currentDisabled} if urlutil.IsURL(entry.Source) { cleanURL, _, hasHash := urlutil.ParseIntegrityHash(entry.Source) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index d48ac2680..ade0b1119 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1327,7 +1327,7 @@ func TestValidateAgentEntries_EmptySource(t *testing.T) { } err := cfg.Validate() assert.Error(t, err) - assert.Contains(t, err.Error(), "source must not be empty") + assert.Contains(t, err.Error(), "enabled agent entry must have a source") } func TestValidateAgentEntries_LocalPathAcceptedWithoutHash(t *testing.T) { @@ -1467,6 +1467,219 @@ func TestValidateAgentEntries_DegenerateName_Rejected(t *testing.T) { assert.Contains(t, err.Error(), "is invalid") } +func TestValidateAgentEntries_SuppressionOnlyEntry_Valid(t *testing.T) { + f := false + agents := []AgentEntry{ + {Name: "retro", Enabled: &f}, + } + cfg := &OrgConfig{ + Version: "1", + Dispatch: DispatchConfig{Platform: "github-actions"}, + Defaults: RepoDefaults{Roles: []string{"fullsend"}, MaxImplementationRetries: 2}, + Agents: agents, + } + assert.NoError(t, cfg.Validate()) +} + +func TestValidateAgentEntries_SuppressionWithoutName_Invalid(t *testing.T) { + f := false + agents := []AgentEntry{ + {Enabled: &f}, + } + cfg := &OrgConfig{ + Version: "1", + Dispatch: DispatchConfig{Platform: "github-actions"}, + Defaults: RepoDefaults{Roles: []string{"fullsend"}, MaxImplementationRetries: 2}, + Agents: agents, + } + err := cfg.Validate() + assert.Error(t, err) + assert.Contains(t, err.Error(), "disabled entry with no source must have an explicit name") +} + +func TestValidateAgentEntries_SuppressionInvalidName_Rejected(t *testing.T) { + f := false + agents := []AgentEntry{ + {Name: "-bad", Enabled: &f}, + } + cfg := &OrgConfig{ + Version: "1", + Dispatch: DispatchConfig{Platform: "github-actions"}, + Defaults: RepoDefaults{Roles: []string{"fullsend"}, MaxImplementationRetries: 2}, + Agents: agents, + } + err := cfg.Validate() + assert.Error(t, err) + assert.Contains(t, err.Error(), "name is invalid") +} + +func TestValidateAgentEntries_DuplicateSuppression_Rejected(t *testing.T) { + f := false + agents := []AgentEntry{ + {Name: "retro", Enabled: &f}, + {Name: "retro", Enabled: &f}, + } + cfg := &OrgConfig{ + Version: "1", + Dispatch: DispatchConfig{Platform: "github-actions"}, + Defaults: RepoDefaults{Roles: []string{"fullsend"}, MaxImplementationRetries: 2}, + Agents: agents, + } + err := cfg.Validate() + assert.Error(t, err) + assert.Contains(t, err.Error(), "duplicate agent name") +} + +func TestValidateAgentEntries_DisableThenEnable_Accepted(t *testing.T) { + f := false + tr := true + agents := []AgentEntry{ + {Name: "retro", Enabled: &f}, + {Name: "retro", Source: "harness/retro-custom.yaml", Enabled: &tr}, + } + cfg := &OrgConfig{ + Version: "1", + Dispatch: DispatchConfig{Platform: "github-actions"}, + Defaults: RepoDefaults{Roles: []string{"fullsend"}, MaxImplementationRetries: 2}, + Agents: agents, + } + assert.NoError(t, cfg.Validate()) +} + +func TestValidateAgentEntries_EnableThenDisable_Accepted(t *testing.T) { + f := false + tr := true + agents := []AgentEntry{ + {Name: "retro", Source: "harness/retro-custom.yaml", Enabled: &tr}, + {Name: "retro", Enabled: &f}, + } + cfg := &OrgConfig{ + Version: "1", + Dispatch: DispatchConfig{Platform: "github-actions"}, + Defaults: RepoDefaults{Roles: []string{"fullsend"}, MaxImplementationRetries: 2}, + Agents: agents, + } + assert.NoError(t, cfg.Validate()) +} + +func TestValidateAgentEntries_DisabledWithSourceNoName_Rejected(t *testing.T) { + f := false + agents := []AgentEntry{ + {Source: "harness/retro.yaml", Enabled: &f}, + } + cfg := &OrgConfig{ + Version: "1", + Dispatch: DispatchConfig{Platform: "github-actions"}, + Defaults: RepoDefaults{Roles: []string{"fullsend"}, MaxImplementationRetries: 2}, + Agents: agents, + } + err := cfg.Validate() + assert.Error(t, err) + assert.Contains(t, err.Error(), "disabled entry must have an explicit name") +} + +func TestValidateAgentEntries_DisabledWithSourceAndName_Valid(t *testing.T) { + f := false + agents := []AgentEntry{ + {Name: "retro", Source: "harness/retro.yaml", Enabled: &f}, + } + cfg := &OrgConfig{ + Version: "1", + Dispatch: DispatchConfig{Platform: "github-actions"}, + Defaults: RepoDefaults{Roles: []string{"fullsend"}, MaxImplementationRetries: 2}, + Agents: agents, + } + assert.NoError(t, cfg.Validate()) +} + +func TestValidateAgentEntries_EnabledWithSource_Valid(t *testing.T) { + tr := true + agents := []AgentEntry{ + {Source: "harness/my-agent.yaml", Enabled: &tr}, + } + cfg := &OrgConfig{ + Version: "1", + Dispatch: DispatchConfig{Platform: "github-actions"}, + Defaults: RepoDefaults{Roles: []string{"fullsend"}, MaxImplementationRetries: 2}, + Agents: agents, + } + assert.NoError(t, cfg.Validate()) +} + +func TestValidateAgentEntries_EnabledOmittedWithSource_Valid(t *testing.T) { + agents := []AgentEntry{ + {Source: "harness/my-agent.yaml"}, + } + cfg := &OrgConfig{ + Version: "1", + Dispatch: DispatchConfig{Platform: "github-actions"}, + Defaults: RepoDefaults{Roles: []string{"fullsend"}, MaxImplementationRetries: 2}, + Agents: agents, + } + assert.NoError(t, cfg.Validate()) +} + +func TestAgentEntry_IsEnabled(t *testing.T) { + t.Run("nil defaults to true", func(t *testing.T) { + e := AgentEntry{Source: "harness/test.yaml"} + assert.True(t, e.IsEnabled()) + }) + t.Run("explicit true", func(t *testing.T) { + tr := true + e := AgentEntry{Source: "harness/test.yaml", Enabled: &tr} + assert.True(t, e.IsEnabled()) + }) + t.Run("explicit false", func(t *testing.T) { + f := false + e := AgentEntry{Source: "harness/test.yaml", Enabled: &f} + assert.False(t, e.IsEnabled()) + }) +} + +func TestOrgConfig_ParseYAML_WithDisabledAgent(t *testing.T) { + yamlData := ` +version: "1" +dispatch: + platform: github-actions +defaults: + roles: + - fullsend + max_implementation_retries: 2 +agents: + - name: retro + enabled: false + - name: lint + source: harness/my-linter.yaml +repos: {} +` + cfg, err := ParseOrgConfig([]byte(yamlData)) + require.NoError(t, err) + require.Len(t, cfg.Agents, 2) + assert.Equal(t, "retro", cfg.Agents[0].Name) + assert.NotNil(t, cfg.Agents[0].Enabled) + assert.False(t, *cfg.Agents[0].Enabled) + assert.Empty(t, cfg.Agents[0].Source) + assert.Nil(t, cfg.Agents[1].Enabled) + assert.NoError(t, cfg.Validate()) +} + +func TestPerRepoConfig_ParseYAML_WithDisabledAgent(t *testing.T) { + yamlData := ` +version: "1" +roles: + - fullsend +agents: + - name: retro + enabled: false +` + cfg, err := ParsePerRepoConfig([]byte(yamlData)) + require.NoError(t, err) + require.Len(t, cfg.Agents, 1) + assert.Equal(t, "retro", cfg.Agents[0].Name) + assert.False(t, *cfg.Agents[0].Enabled) + assert.NoError(t, cfg.Validate()) +} + // --- OrgConfig agents field tests --- func TestOrgConfig_ParseYAML_WithAgents(t *testing.T) { diff --git a/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml b/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml index 6a8b1ed2b..8ad0e6e22 100644 --- a/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml +++ b/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml @@ -1,5 +1,5 @@ --- -# lint-workflow-size: max-lines=475 +# lint-workflow-size: max-lines=515 # Dispatcher workflow that routes events to agent workflows based on stage. # Routing logic determines the stage from event context — the shim only # forwards the raw event. Adding a new stage requires only a case branch @@ -337,6 +337,20 @@ jobs: exit 1 fi + - name: Validate agents config + if: steps.route.outputs.stage != '' && steps.pr-check.outputs.skipped != 'true' + run: | + set -euo pipefail + AGENT_COUNT=$(yq '.agents | length // 0' config.yaml 2>/dev/null || echo "0") + if [[ "$AGENT_COUNT" == "0" ]]; then + exit 0 + fi + BAD=$(yq '.agents[] | select(.enabled == false and (.name == null or .name == "")) | line' config.yaml 2>/dev/null || echo "") + if [[ -n "$BAD" ]]; then + echo "::error::config.yaml: disabled agent entry without a name field — add 'name:' to identify which agent to disable" + exit 1 + fi + - name: Check role is enabled id: role-check if: steps.route.outputs.stage != '' && steps.pr-check.outputs.skipped != 'true' @@ -361,8 +375,32 @@ jobs: fi fi + - name: Check agent is enabled + id: agent-check + if: steps.route.outputs.stage != '' && steps.role-check.outputs.skipped != 'true' && steps.pr-check.outputs.skipped != 'true' + env: + STAGE: ${{ steps.route.outputs.stage }} + run: | + set -euo pipefail + YQ_ERR=$(mktemp) + AGENT_ENABLED=$(yq " + .agents[] | + select((.name | downcase) == \"$STAGE\") | + .enabled + " config.yaml 2>"$YQ_ERR" | tail -1) || { + echo "::warning::yq failed checking agents[].enabled: $(cat "$YQ_ERR")" + rm -f "$YQ_ERR" + exit 0 + } + rm -f "$YQ_ERR" + if [[ "$AGENT_ENABLED" == "false" ]]; then + echo "::notice::Stage '$STAGE' skipped — agent '$STAGE' is disabled in config" + echo "skipped=true" >> "${GITHUB_OUTPUT}" + exit 0 + fi + - name: Block fork PRs for fix stage - if: steps.route.outputs.stage == 'fix' && steps.role-check.outputs.skipped != 'true' && github.event.issue.pull_request + if: steps.route.outputs.stage == 'fix' && steps.role-check.outputs.skipped != 'true' && steps.agent-check.outputs.skipped != 'true' && github.event.issue.pull_request env: GH_TOKEN: ${{ github.token }} SOURCE_REPO: ${{ github.repository }} @@ -382,7 +420,7 @@ jobs: fi - name: Find and trigger agent workflows for stage - if: steps.route.outputs.stage != '' && steps.role-check.outputs.skipped != 'true' && steps.pr-check.outputs.skipped != 'true' + if: steps.route.outputs.stage != '' && steps.role-check.outputs.skipped != 'true' && steps.agent-check.outputs.skipped != 'true' && steps.pr-check.outputs.skipped != 'true' env: GH_TOKEN: ${{ steps.oidc-mint.outputs.token }} STAGE: ${{ steps.route.outputs.stage }}