diff --git a/internal/config/config.go b/internal/config/config.go index cf1b604cd8..520019a2a2 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -493,14 +493,31 @@ func (c *orgConfig) DefaultRoles() []string { // Stored in .fullsend/config.yaml within the target repository. // Consumer packages should use the PerRepoConfigReader or ConfigWriter // interfaces rather than referencing this type directly. +// +// The parent field implements a fallback chain per ADR 0069 Decision 2: +// accessors check the local struct first, then fall through to parent +// when the local value is unset. The terminal parent is perRepoDefaults, +// which returns compiled-in code defaults. type perRepoConfig struct { - Version string `yaml:"version"` - KillSwitch bool `yaml:"kill_switch,omitempty"` - Runtime string `yaml:"runtime,omitempty"` - Roles []string `yaml:"roles,omitempty"` - Agents []AgentEntry `yaml:"agents,omitempty"` + // omitempty so unset version is not marshaled (unlike orgConfig, + // where version is always required). This allows the fallback + // chain to inherit version from the parent layer. + Version string `yaml:"version,omitempty"` + KillSwitch *bool `yaml:"kill_switch,omitempty"` + Runtime string `yaml:"runtime,omitempty"` + Roles []string `yaml:"roles,omitempty"` + Agents []AgentEntry `yaml:"agents,omitempty"` + // AllowedRemoteResources holds the locally-set allowed remote + // resource prefixes. MarshalYAML preserves the nil-vs-empty + // distinction: nil (unset) is omitted, empty (deny-all) is + // marshaled as `allowed_remote_resources: []`. AllowedRemoteResources []string `yaml:"allowed_remote_resources,omitempty"` CreateIssues *CreateIssuesConfig `yaml:"create_issues,omitempty"` + + // parent is the next layer in the fallback chain. Getters consult + // parent when the local field is unset. Excluded from YAML + // serialization so Marshal emits only locally-set values. + parent PerRepoConfigReader `yaml:"-"` } const perRepoConfigHeader = `# fullsend per-repo configuration @@ -522,6 +539,7 @@ func NewPerRepoConfig(roles []string, targetRepo string) PerRepoConfigWriter { Version: "1", Roles: roles, AllowedRemoteResources: DefaultAllowedRemoteResources(), + parent: &perRepoDefaults{}, } if targetRepo != "" { cfg.CreateIssues = &CreateIssuesConfig{ @@ -539,6 +557,7 @@ func ParsePerRepoConfig(data []byte) (PerRepoConfigReader, error) { if err := yaml.Unmarshal(data, &cfg); err != nil { return nil, fmt.Errorf("parsing per-repo config: %w", err) } + cfg.parent = &perRepoDefaults{} return &cfg, nil } @@ -549,6 +568,7 @@ func ParsePerRepoConfigWriter(data []byte) (ConfigWriter, error) { if err := yaml.Unmarshal(data, &cfg); err != nil { return nil, fmt.Errorf("parsing per-repo config: %w", err) } + cfg.parent = &perRepoDefaults{} return &cfg, nil } @@ -561,23 +581,69 @@ func (c *perRepoConfig) Marshal() ([]byte, error) { return []byte(perRepoConfigHeader + string(body)), nil } +// perRepoConfigMarshal is a shadow struct used by MarshalYAML to +// preserve the nil-vs-empty distinction for AllowedRemoteResources. +// With the plain []string + omitempty tag, yaml.v3 omits both nil +// and empty slices. Using *[]string here means a nil pointer (unset) +// is omitted while a non-nil pointer to an empty slice (deny-all) +// is marshaled as `allowed_remote_resources: []`. +type perRepoConfigMarshal struct { + Version string `yaml:"version,omitempty"` + KillSwitch *bool `yaml:"kill_switch,omitempty"` + Runtime string `yaml:"runtime,omitempty"` + Roles []string `yaml:"roles,omitempty"` + Agents []AgentEntry `yaml:"agents,omitempty"` + AllowedRemoteResources *[]string `yaml:"allowed_remote_resources,omitempty"` + CreateIssues *CreateIssuesConfig `yaml:"create_issues,omitempty"` +} + +// MarshalYAML implements yaml.Marshaler to preserve the nil-vs-empty +// distinction for AllowedRemoteResources through YAML roundtrips. +// nil (unset) is omitted; an explicit empty slice (deny-all) is +// marshaled as `allowed_remote_resources: []`. +func (c *perRepoConfig) MarshalYAML() (interface{}, error) { + h := perRepoConfigMarshal{ + Version: c.Version, + KillSwitch: c.KillSwitch, + Runtime: c.Runtime, + Roles: c.Roles, + Agents: c.Agents, + CreateIssues: c.CreateIssues, + } + if c.AllowedRemoteResources != nil { + h.AllowedRemoteResources = &c.AllowedRemoteResources + } + return &h, nil +} + // Validate checks the PerRepoConfig for structural correctness. +// Locally-set fields are validated; resolved values (e.g., +// AllowedResources) are used where validation requires the full +// effective config. func (c *perRepoConfig) Validate() error { - if c.Version != "1" { + // Version: empty means "inherit from parent"; non-empty must be "1". + if c.Version != "" && c.Version != "1" { return fmt.Errorf("unsupported version %q: must be \"1\"", c.Version) } - valid := ValidRoles() - seen := make(map[string]bool, len(c.Roles)) - for _, role := range c.Roles { - if !slices.Contains(valid, role) { - return fmt.Errorf("invalid role %q: must be one of %s", role, strings.Join(valid, ", ")) - } - if seen[role] { - return fmt.Errorf("duplicate role %q in roles", role) + // Roles: nil means "inherit from parent"; non-nil (including empty) + // is locally set and validated. + if c.Roles != nil { + valid := ValidRoles() + seen := make(map[string]bool, len(c.Roles)) + for _, role := range c.Roles { + if !slices.Contains(valid, role) { + return fmt.Errorf("invalid role %q: must be one of %s", role, strings.Join(valid, ", ")) + } + if seen[role] { + return fmt.Errorf("duplicate role %q in roles", role) + } + seen[role] = true } - seen[role] = true } - if err := ValidateAgentEntries(c.Agents, c.AllowedRemoteResources); err != nil { + // Agents are validated against the resolved allowlist (including + // parent resources) so that URL agents covered by a parent or + // default prefix pass validation. + if err := ValidateAgentEntries(c.Agents, c.AllowedResources()); err != nil { return err } if err := validateCreateIssues(c.CreateIssues); err != nil { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 4a31f05f7e..013246f34f 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1810,7 +1810,12 @@ allowed_remote_resources: require.Len(t, cfg.AgentEntries(), 2) assert.Contains(t, cfg.AgentEntries()[0].Source, "triage.yaml") assert.Equal(t, "lint", cfg.AgentEntries()[1].Name) - assert.Equal(t, []string{"https://raw.githubusercontent.com/fullsend-ai/agents/"}, cfg.AllowedResources()) + // AllowedResources now unions with parent defaults (code defaults). + resources := cfg.AllowedResources() + assert.Contains(t, resources, "https://raw.githubusercontent.com/fullsend-ai/agents/") + for _, d := range DefaultAllowedRemoteResources() { + assert.Contains(t, resources, d) + } } func TestPerRepoConfig_Validate_WithAgents(t *testing.T) { @@ -1894,7 +1899,12 @@ func TestPerRepoConfig_RoundTrip_WithAgents(t *testing.T) { require.Len(t, parsed.AgentEntries(), 2) assert.Equal(t, original.Agents[0].Source, parsed.AgentEntries()[0].Source) assert.Equal(t, original.Agents[1].Name, parsed.AgentEntries()[1].Name) - assert.Equal(t, original.AllowedRemoteResources, parsed.AllowedResources()) + // Parsed config has a parent so AllowedResources unions with + // code defaults. Verify local resource is present. + resources := parsed.AllowedResources() + assert.Contains(t, resources, "https://example.com/") + // Verify the raw struct field was preserved. + assert.Equal(t, original.AllowedRemoteResources, parsed.(*perRepoConfig).AllowedRemoteResources) } func TestOrgConfig_RoundTrip_WithAgents(t *testing.T) { diff --git a/internal/config/defaults.go b/internal/config/defaults.go new file mode 100644 index 0000000000..cc4d6c01a3 --- /dev/null +++ b/internal/config/defaults.go @@ -0,0 +1,36 @@ +package config + +// perRepoDefaults implements PerRepoConfigReader with compiled-in +// code defaults. It serves as the terminal node in the parent +// fallback chain (ADR 0069 Decision 2). +// +// Lookup order for all accessors: overlay -> base -> code defaults. +// perRepoDefaults is the "code defaults" layer. +type perRepoDefaults struct{} + +// Compile-time assertion that perRepoDefaults satisfies PerRepoConfigReader. +var _ PerRepoConfigReader = (*perRepoDefaults)(nil) + +// ConfigVersion returns the default schema version. +func (d *perRepoDefaults) ConfigVersion() string { return "1" } + +// ConfigRuntime returns the default runtime. +func (d *perRepoDefaults) ConfigRuntime() string { return "claude" } + +// IsKillSwitchActive returns the default kill switch state (off). +func (d *perRepoDefaults) IsKillSwitchActive() bool { return false } + +// ConfigRoles returns the default agent roles. +func (d *perRepoDefaults) ConfigRoles() []string { return PerRepoDefaultRoles() } + +// AgentEntries returns nil — no agents are configured by default. +func (d *perRepoDefaults) AgentEntries() []AgentEntry { return nil } + +// AllowedResources returns the default allowed remote resource prefixes. +func (d *perRepoDefaults) AllowedResources() []string { return DefaultAllowedRemoteResources() } + +// IssueCreationConfig returns nil — no issue creation config by default. +func (d *perRepoDefaults) IssueCreationConfig() *CreateIssuesConfig { return nil } + +// IsOrgMode returns false — per-repo configs are never org mode. +func (d *perRepoDefaults) IsOrgMode() bool { return false } diff --git a/internal/config/defaults_test.go b/internal/config/defaults_test.go new file mode 100644 index 0000000000..e86034fc52 --- /dev/null +++ b/internal/config/defaults_test.go @@ -0,0 +1,652 @@ +package config + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// --- perRepoDefaults interface satisfaction --- + +func TestPerRepoDefaults_SatisfiesPerRepoConfigReader(t *testing.T) { + var _ PerRepoConfigReader = (*perRepoDefaults)(nil) +} + +// --- perRepoDefaults returns compiled-in code defaults --- + +func TestPerRepoDefaults_CodeDefaults(t *testing.T) { + d := &perRepoDefaults{} + + assert.Equal(t, "1", d.ConfigVersion()) + assert.Equal(t, "claude", d.ConfigRuntime()) + assert.False(t, d.IsKillSwitchActive()) + assert.Equal(t, PerRepoDefaultRoles(), d.ConfigRoles()) + assert.Nil(t, d.AgentEntries()) + assert.Equal(t, DefaultAllowedRemoteResources(), d.AllowedResources()) + assert.Nil(t, d.IssueCreationConfig()) + assert.False(t, d.IsOrgMode()) +} + +// --- Unset fields resolve through parent to code defaults --- + +func TestPerRepoConfig_EmptyConfigResolvesDefaults(t *testing.T) { + cfg := &perRepoConfig{parent: &perRepoDefaults{}} + + assert.Equal(t, "1", cfg.ConfigVersion()) + assert.Equal(t, "claude", cfg.ConfigRuntime()) + assert.False(t, cfg.IsKillSwitchActive()) + assert.Equal(t, PerRepoDefaultRoles(), cfg.ConfigRoles()) + assert.Nil(t, cfg.AgentEntries()) + assert.Equal(t, DefaultAllowedRemoteResources(), cfg.AllowedResources()) + assert.Nil(t, cfg.IssueCreationConfig()) + assert.False(t, cfg.IsOrgMode()) +} + +// --- Local values override parent values --- + +func TestPerRepoConfig_LocalOverridesParent(t *testing.T) { + tr := true + cfg := &perRepoConfig{ + Version: "1", + KillSwitch: &tr, + Runtime: "dummy", + Roles: []string{"triage", "review"}, + Agents: []AgentEntry{ + {Source: "harness/code.yaml"}, + }, + AllowedRemoteResources: []string{"https://example.com/"}, + CreateIssues: &CreateIssuesConfig{ + AllowTargets: AllowTargets{Repos: []string{"org/repo"}}, + }, + parent: &perRepoDefaults{}, + } + + assert.Equal(t, "1", cfg.ConfigVersion()) + assert.Equal(t, "dummy", cfg.ConfigRuntime()) + assert.True(t, cfg.IsKillSwitchActive()) + assert.Equal(t, []string{"triage", "review"}, cfg.ConfigRoles()) + require.Len(t, cfg.AgentEntries(), 1) + assert.Equal(t, "harness/code.yaml", cfg.AgentEntries()[0].Source) + require.NotNil(t, cfg.IssueCreationConfig()) + assert.Equal(t, []string{"org/repo"}, cfg.IssueCreationConfig().AllowTargets.Repos) +} + +// --- Chained fallback: overlay -> base -> defaults --- + +func TestPerRepoConfig_ChainedFallback(t *testing.T) { + // defaults -> base -> overlay + // base sets runtime="dummy" and roles; overlay sets only version. + base := &perRepoConfig{ + Runtime: "dummy", + Roles: []string{"triage"}, + parent: &perRepoDefaults{}, + } + overlay := &perRepoConfig{ + Version: "1", + parent: base, + } + + // overlay has version locally. + assert.Equal(t, "1", overlay.ConfigVersion()) + // runtime falls through overlay (empty) -> base ("dummy"). + assert.Equal(t, "dummy", overlay.ConfigRuntime()) + // roles falls through overlay (nil) -> base. + assert.Equal(t, []string{"triage"}, overlay.ConfigRoles()) + // kill_switch falls through overlay (nil) -> base (nil) -> defaults (false). + assert.False(t, overlay.IsKillSwitchActive()) + // allowed_remote_resources: overlay nil -> base nil -> defaults. + assert.Equal(t, DefaultAllowedRemoteResources(), overlay.AllowedResources()) +} + +// --- KillSwitch *bool pointer semantics --- + +func TestPerRepoConfig_KillSwitch_PointerSemantics(t *testing.T) { + t.Run("nil falls through to parent default false", func(t *testing.T) { + cfg := &perRepoConfig{parent: &perRepoDefaults{}} + assert.False(t, cfg.IsKillSwitchActive()) + }) + + t.Run("explicit false does not fall through", func(t *testing.T) { + // Parent has kill_switch=true, overlay explicitly sets false. + tr := true + parentCfg := &perRepoConfig{ + KillSwitch: &tr, + parent: &perRepoDefaults{}, + } + f := false + overlay := &perRepoConfig{ + KillSwitch: &f, + parent: parentCfg, + } + assert.False(t, overlay.IsKillSwitchActive()) + }) + + t.Run("explicit true overrides parent false", func(t *testing.T) { + tr := true + cfg := &perRepoConfig{ + KillSwitch: &tr, + parent: &perRepoDefaults{}, + } + assert.True(t, cfg.IsKillSwitchActive()) + }) +} + +// --- Agents keyed merge by DerivedName --- + +func TestPerRepoConfig_AgentsMerge(t *testing.T) { + t.Run("nil overlay returns parent agents", func(t *testing.T) { + parent := &perRepoConfig{ + Agents: []AgentEntry{ + {Source: "harness/triage.yaml"}, + }, + parent: &perRepoDefaults{}, + } + overlay := &perRepoConfig{ + // Agents is nil (key omitted). + parent: parent, + } + agents := overlay.AgentEntries() + require.Len(t, agents, 1) + assert.Equal(t, "harness/triage.yaml", agents[0].Source) + }) + + t.Run("empty overlay preserves parent agents", func(t *testing.T) { + parent := &perRepoConfig{ + Agents: []AgentEntry{ + {Source: "harness/triage.yaml"}, + {Source: "harness/review.yaml"}, + }, + parent: &perRepoDefaults{}, + } + overlay := &perRepoConfig{ + Agents: []AgentEntry{}, // explicit empty + parent: parent, + } + agents := overlay.AgentEntries() + require.Len(t, agents, 2) + assert.Equal(t, "triage", agents[0].DerivedName()) + assert.Equal(t, "review", agents[1].DerivedName()) + }) + + t.Run("overlay disables parent agent", func(t *testing.T) { + parent := &perRepoConfig{ + Agents: []AgentEntry{ + {Source: "harness/triage.yaml"}, + {Source: "harness/review.yaml"}, + }, + parent: &perRepoDefaults{}, + } + f := false + overlay := &perRepoConfig{ + Agents: []AgentEntry{ + {Name: "triage", Enabled: &f}, + }, + parent: parent, + } + agents := overlay.AgentEntries() + require.Len(t, agents, 2) + // triage should be disabled. + assert.Equal(t, "triage", agents[0].DerivedName()) + assert.False(t, agents[0].IsEnabled()) + // triage source preserved from parent. + assert.Equal(t, "harness/triage.yaml", agents[0].Source) + // review unchanged. + assert.Equal(t, "review", agents[1].DerivedName()) + assert.True(t, agents[1].IsEnabled()) + }) + + t.Run("overlay replaces parent agent source", func(t *testing.T) { + parent := &perRepoConfig{ + Agents: []AgentEntry{ + {Source: "harness/triage.yaml"}, + }, + parent: &perRepoDefaults{}, + } + overlay := &perRepoConfig{ + Agents: []AgentEntry{ + {Name: "triage", Source: "harness/custom-triage.yaml"}, + }, + parent: parent, + } + agents := overlay.AgentEntries() + require.Len(t, agents, 1) + assert.Equal(t, "triage", agents[0].DerivedName()) + assert.Equal(t, "harness/custom-triage.yaml", agents[0].Source) + assert.True(t, agents[0].IsEnabled()) + }) + + t.Run("overlay adds new agent not in parent", func(t *testing.T) { + parent := &perRepoConfig{ + Agents: []AgentEntry{ + {Source: "harness/triage.yaml"}, + }, + parent: &perRepoDefaults{}, + } + overlay := &perRepoConfig{ + Agents: []AgentEntry{ + {Source: "harness/lint.yaml"}, + }, + parent: parent, + } + agents := overlay.AgentEntries() + require.Len(t, agents, 2) + assert.Equal(t, "triage", agents[0].DerivedName()) + assert.Equal(t, "lint", agents[1].DerivedName()) + }) + + t.Run("no parent returns local agents only", func(t *testing.T) { + cfg := &perRepoConfig{ + Agents: []AgentEntry{ + {Source: "harness/code.yaml"}, + }, + } + agents := cfg.AgentEntries() + require.Len(t, agents, 1) + assert.Equal(t, "code", agents[0].DerivedName()) + }) +} + +// --- AllowedResources union and deny-all --- + +func TestPerRepoConfig_AllowedResources_Fallback(t *testing.T) { + t.Run("nil falls through to parent defaults", func(t *testing.T) { + cfg := &perRepoConfig{parent: &perRepoDefaults{}} + assert.Equal(t, DefaultAllowedRemoteResources(), cfg.AllowedResources()) + }) + + t.Run("explicit empty is deny-all", func(t *testing.T) { + cfg := &perRepoConfig{ + AllowedRemoteResources: []string{}, + parent: &perRepoDefaults{}, + } + result := cfg.AllowedResources() + assert.NotNil(t, result) + assert.Empty(t, result) + }) + + t.Run("non-empty unions with parent", func(t *testing.T) { + cfg := &perRepoConfig{ + AllowedRemoteResources: []string{"https://example.com/custom/"}, + parent: &perRepoDefaults{}, + } + result := cfg.AllowedResources() + assert.Contains(t, result, "https://example.com/custom/") + // Defaults surface via the terminal perRepoDefaults parent. + for _, d := range DefaultAllowedRemoteResources() { + assert.Contains(t, result, d) + } + }) + + t.Run("non-empty with overlap produces no duplicates", func(t *testing.T) { + defaults := DefaultAllowedRemoteResources() + cfg := &perRepoConfig{ + AllowedRemoteResources: []string{defaults[0], "https://example.com/"}, + parent: &perRepoDefaults{}, + } + result := cfg.AllowedResources() + // Count occurrences of defaults[0]. + count := 0 + for _, r := range result { + if r == defaults[0] { + count++ + } + } + assert.Equal(t, 1, count, "should not duplicate existing entries") + assert.Contains(t, result, defaults[1], "missing default should be appended") + assert.Contains(t, result, "https://example.com/") + }) + + t.Run("deny-all in parent with non-empty overlay", func(t *testing.T) { + parent := &perRepoConfig{ + AllowedRemoteResources: []string{}, // deny-all + parent: &perRepoDefaults{}, + } + overlay := &perRepoConfig{ + AllowedRemoteResources: []string{"https://example.com/"}, + parent: parent, + } + result := overlay.AllowedResources() + assert.Contains(t, result, "https://example.com/") + // Parent returned deny-all (empty), so overlay union contains + // only the overlay entries — code defaults are not re-injected. + assert.Len(t, result, 1) + }) + + t.Run("custom parent without code defaults is honored", func(t *testing.T) { + // An intermediate parent that returns a custom allowlist without + // baked-in prefixes. The overlay getter honors the parent's + // effective list without re-injecting code defaults. + parent := &perRepoConfig{ + AllowedRemoteResources: []string{"https://custom.example.com/"}, + } + overlay := &perRepoConfig{ + AllowedRemoteResources: []string{"https://overlay.example.com/"}, + parent: parent, + } + result := overlay.AllowedResources() + assert.Contains(t, result, "https://overlay.example.com/") + assert.Contains(t, result, "https://custom.example.com/") + assert.Len(t, result, 2) + // Code defaults are NOT present — the parent did not include + // them, and the overlay getter honors that. + for _, d := range DefaultAllowedRemoteResources() { + assert.NotContains(t, result, d) + } + }) +} + +// --- CreateIssues fallback --- + +func TestPerRepoConfig_CreateIssues_Fallback(t *testing.T) { + t.Run("nil falls through to parent", func(t *testing.T) { + parent := &perRepoConfig{ + CreateIssues: &CreateIssuesConfig{ + AllowTargets: AllowTargets{Orgs: []string{"my-org"}}, + }, + parent: &perRepoDefaults{}, + } + overlay := &perRepoConfig{parent: parent} + require.NotNil(t, overlay.IssueCreationConfig()) + assert.Equal(t, []string{"my-org"}, overlay.IssueCreationConfig().AllowTargets.Orgs) + }) + + t.Run("local replaces parent entirely", func(t *testing.T) { + parent := &perRepoConfig{ + CreateIssues: &CreateIssuesConfig{ + AllowTargets: AllowTargets{Orgs: []string{"parent-org"}}, + }, + parent: &perRepoDefaults{}, + } + overlay := &perRepoConfig{ + CreateIssues: &CreateIssuesConfig{ + AllowTargets: AllowTargets{Repos: []string{"org/repo"}}, + }, + parent: parent, + } + require.NotNil(t, overlay.IssueCreationConfig()) + assert.Empty(t, overlay.IssueCreationConfig().AllowTargets.Orgs) + assert.Equal(t, []string{"org/repo"}, overlay.IssueCreationConfig().AllowTargets.Repos) + }) +} + +// --- Roles fallback --- + +func TestPerRepoConfig_Roles_Fallback(t *testing.T) { + t.Run("nil falls through to parent", func(t *testing.T) { + cfg := &perRepoConfig{parent: &perRepoDefaults{}} + assert.Equal(t, PerRepoDefaultRoles(), cfg.ConfigRoles()) + }) + + t.Run("empty slice replaces parent (no roles)", func(t *testing.T) { + cfg := &perRepoConfig{ + Roles: []string{}, + parent: &perRepoDefaults{}, + } + assert.NotNil(t, cfg.ConfigRoles()) + assert.Empty(t, cfg.ConfigRoles()) + }) + + t.Run("non-empty replaces parent", func(t *testing.T) { + cfg := &perRepoConfig{ + Roles: []string{"triage"}, + parent: &perRepoDefaults{}, + } + assert.Equal(t, []string{"triage"}, cfg.ConfigRoles()) + }) +} + +// --- Version fallback --- + +func TestPerRepoConfig_Version_Fallback(t *testing.T) { + t.Run("empty falls through to parent", func(t *testing.T) { + cfg := &perRepoConfig{parent: &perRepoDefaults{}} + assert.Equal(t, "1", cfg.ConfigVersion()) + }) + + t.Run("local overrides parent", func(t *testing.T) { + cfg := &perRepoConfig{ + Version: "1", + parent: &perRepoDefaults{}, + } + assert.Equal(t, "1", cfg.ConfigVersion()) + }) +} + +// --- Runtime fallback --- + +func TestPerRepoConfig_Runtime_Fallback(t *testing.T) { + t.Run("empty falls through to parent", func(t *testing.T) { + cfg := &perRepoConfig{parent: &perRepoDefaults{}} + assert.Equal(t, "claude", cfg.ConfigRuntime()) + }) + + t.Run("local overrides parent", func(t *testing.T) { + cfg := &perRepoConfig{ + Runtime: "dummy", + parent: &perRepoDefaults{}, + } + assert.Equal(t, "dummy", cfg.ConfigRuntime()) + }) +} + +// --- Marshal emits only locally-set fields --- + +func TestPerRepoConfig_MarshalOmitsInheritedValues(t *testing.T) { + // An empty config with parent should marshal to only the YAML + // header and an empty YAML body — no inherited values leak. + cfg := &perRepoConfig{parent: &perRepoDefaults{}} + data, err := cfg.Marshal() + require.NoError(t, err) + + output := string(data) + // Header is always present. + assert.Contains(t, output, "fullsend per-repo configuration") + // No inherited values should appear. + assert.NotContains(t, output, "version:") + assert.NotContains(t, output, "runtime:") + assert.NotContains(t, output, "kill_switch:") + assert.NotContains(t, output, "roles:") + assert.NotContains(t, output, "agents:") + assert.NotContains(t, output, "allowed_remote_resources:") + assert.NotContains(t, output, "create_issues:") + // But the config still resolves defaults via parent. + assert.Equal(t, "1", cfg.ConfigVersion()) + assert.Equal(t, "claude", cfg.ConfigRuntime()) +} + +func TestPerRepoConfig_MarshalEmitsLocalValues(t *testing.T) { + tr := true + cfg := &perRepoConfig{ + Version: "1", + KillSwitch: &tr, + Runtime: "dummy", + Roles: []string{"triage"}, + parent: &perRepoDefaults{}, + } + data, err := cfg.Marshal() + require.NoError(t, err) + + output := string(data) + assert.Contains(t, output, "version:") + assert.Contains(t, output, "kill_switch: true") + assert.Contains(t, output, "runtime: dummy") + assert.Contains(t, output, "- triage") +} + +func TestPerRepoConfig_MarshalExplicitFalseKillSwitch(t *testing.T) { + f := false + cfg := &perRepoConfig{ + Version: "1", + KillSwitch: &f, + parent: &perRepoDefaults{}, + } + data, err := cfg.Marshal() + require.NoError(t, err) + // Explicit false should appear in output (distinguishable from unset). + assert.Contains(t, string(data), "kill_switch: false") +} + +func TestPerRepoConfig_MarshalDenyAll(t *testing.T) { + cfg := &perRepoConfig{ + Version: "1", + AllowedRemoteResources: []string{}, // deny-all + parent: &perRepoDefaults{}, + } + data, err := cfg.Marshal() + require.NoError(t, err) + // Deny-all (empty slice) must appear in output so it survives + // a roundtrip. Without MarshalYAML the omitempty tag would drop it. + assert.Contains(t, string(data), "allowed_remote_resources: []") +} + +// --- Deny-all YAML roundtrip --- + +func TestPerRepoConfig_DenyAll_YAMLRoundTrip(t *testing.T) { + // A config with deny-all allowed_remote_resources must preserve + // the deny-all semantics through marshal -> parse -> getter. + cfg := &perRepoConfig{ + Version: "1", + Roles: []string{"triage"}, + AllowedRemoteResources: []string{}, // deny-all + parent: &perRepoDefaults{}, + } + + data, err := cfg.Marshal() + require.NoError(t, err) + + // The marshaled output must include the empty list. + assert.Contains(t, string(data), "allowed_remote_resources: []") + + // Strip the header so ParsePerRepoConfig can parse the YAML body. + headerEnd := strings.Index(string(data), "version:") + require.True(t, headerEnd >= 0, "marshaled output should contain version:") + + parsed, err := ParsePerRepoConfig(data[headerEnd:]) + require.NoError(t, err) + + // After roundtrip, AllowedResources must return deny-all (empty, + // non-nil) — NOT fall through to parent defaults. + result := parsed.AllowedResources() + assert.NotNil(t, result, "deny-all must not become nil after roundtrip") + assert.Empty(t, result, "deny-all must remain empty after roundtrip") +} + +// --- Validate with fallback --- + +func TestPerRepoConfig_ValidateEmptyVersion_FallsThrough(t *testing.T) { + // Empty version (inherits from parent) should pass validation. + cfg := &perRepoConfig{ + Roles: []string{"triage"}, + parent: &perRepoDefaults{}, + } + assert.NoError(t, cfg.Validate()) +} + +func TestPerRepoConfig_ValidateNilRoles_FallsThrough(t *testing.T) { + // Nil roles (inherits from parent) should pass validation. + cfg := &perRepoConfig{ + Version: "1", + parent: &perRepoDefaults{}, + } + assert.NoError(t, cfg.Validate()) +} + +// --- ParsePerRepoConfig sets parent --- + +func TestParsePerRepoConfig_SetsParent(t *testing.T) { + yamlData := `version: "1" +roles: + - triage +` + cfg, err := ParsePerRepoConfig([]byte(yamlData)) + require.NoError(t, err) + // ConfigRuntime should resolve through parent to "claude". + assert.Equal(t, "claude", cfg.ConfigRuntime()) +} + +func TestParsePerRepoConfigWriter_SetsParent(t *testing.T) { + yamlData := `version: "1" +roles: + - triage +` + cfg, err := ParsePerRepoConfigWriter([]byte(yamlData)) + require.NoError(t, err) + // Cast to PerRepoConfigReader to access ConfigRuntime. + pcr, ok := cfg.(PerRepoConfigReader) + require.True(t, ok) + assert.Equal(t, "claude", pcr.ConfigRuntime()) +} + +// --- Existing single-file behavior unchanged --- + +func TestPerRepoConfig_ExistingSingleFileBehavior(t *testing.T) { + // A fully populated config (like NewPerRepoConfig produces) should + // behave identically to pre-parent-chain behavior. + cfg := NewPerRepoConfig([]string{"triage", "coder", "review"}, "org/repo") + + assert.Equal(t, "1", cfg.ConfigVersion()) + assert.False(t, cfg.IsKillSwitchActive()) + assert.False(t, cfg.IsOrgMode()) + assert.Equal(t, []string{"triage", "coder", "review"}, cfg.ConfigRoles()) + assert.Empty(t, cfg.AgentEntries()) + assert.Equal(t, DefaultAllowedRemoteResources(), cfg.AllowedResources()) + require.NotNil(t, cfg.IssueCreationConfig()) + + // Marshal round-trip. + data, err := cfg.Marshal() + require.NoError(t, err) + + headerEnd := strings.Index(string(data), "version:") + require.True(t, headerEnd > 0) + + parsed, err := ParsePerRepoConfig(data[headerEnd:]) + require.NoError(t, err) + assert.Equal(t, cfg.ConfigVersion(), parsed.ConfigVersion()) + assert.Equal(t, cfg.IsKillSwitchActive(), parsed.IsKillSwitchActive()) + assert.Equal(t, cfg.ConfigRoles(), parsed.ConfigRoles()) +} + +// --- KillSwitch YAML round-trip --- + +func TestPerRepoConfig_KillSwitch_YAMLRoundTrip(t *testing.T) { + t.Run("kill_switch true round-trips", func(t *testing.T) { + yamlData := `version: "1" +kill_switch: true +roles: + - triage +` + cfg, err := ParsePerRepoConfig([]byte(yamlData)) + require.NoError(t, err) + assert.True(t, cfg.IsKillSwitchActive()) + }) + + t.Run("kill_switch false round-trips", func(t *testing.T) { + yamlData := `version: "1" +kill_switch: false +roles: + - triage +` + cfg, err := ParsePerRepoConfig([]byte(yamlData)) + require.NoError(t, err) + assert.False(t, cfg.IsKillSwitchActive()) + // Verify it was explicitly set (not inherited). + prc := cfg.(*perRepoConfig) + require.NotNil(t, prc.KillSwitch) + assert.False(t, *prc.KillSwitch) + }) + + t.Run("kill_switch omitted falls through to default", func(t *testing.T) { + yamlData := `version: "1" +roles: + - triage +` + cfg, err := ParsePerRepoConfig([]byte(yamlData)) + require.NoError(t, err) + assert.False(t, cfg.IsKillSwitchActive()) + // Verify it was not set. + prc := cfg.(*perRepoConfig) + assert.Nil(t, prc.KillSwitch) + }) +} diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 8a5eeaeb52..b9e34078a4 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "gopkg.in/yaml.v3" ) @@ -186,35 +187,185 @@ func (c *orgConfig) SetRepo(name string, rc RepoConfig) { } // --- perRepoConfig getter methods --- +// +// All getters implement fallback: local value -> parent -> zero value. +// The parent chain terminates at perRepoDefaults which returns +// compiled-in code defaults (ADR 0069 Decision 2). + +// AgentEntries returns the merged agent set. When the local Agents +// field is nil (key omitted from YAML), parent agents are returned +// unchanged. When local Agents is non-nil (including empty), a keyed +// merge by DerivedName is performed: overlay entries can toggle +// enable/disable or replace the source of a parent agent without +// replacing the entire agent list. +func (c *perRepoConfig) AgentEntries() []AgentEntry { + var parentAgents []AgentEntry + if c.parent != nil { + parentAgents = c.parent.AgentEntries() + } + if c.Agents == nil { + return parentAgents + } + if len(parentAgents) == 0 { + return c.Agents + } + // Build overlay index by lowercase DerivedName. Multiple entries + // with the same name may exist (disable-then-enable pairs). + type overlayInfo struct { + entry AgentEntry + used bool + } + overlayByName := make(map[string][]*overlayInfo, len(c.Agents)) + overlayOrder := make([]*overlayInfo, 0, len(c.Agents)) + for _, a := range c.Agents { + oi := &overlayInfo{entry: a} + key := strings.ToLower(a.DerivedName()) + overlayByName[key] = append(overlayByName[key], oi) + overlayOrder = append(overlayOrder, oi) + } -// AgentEntries returns the registered agent entries. -func (c *perRepoConfig) AgentEntries() []AgentEntry { return c.Agents } + // For each parent agent, apply matching overlay entries. + result := make([]AgentEntry, 0, len(parentAgents)+len(c.Agents)) + for _, pa := range parentAgents { + key := strings.ToLower(pa.DerivedName()) + overlays, hasOverlay := overlayByName[key] + if !hasOverlay { + result = append(result, pa) + continue + } + // Apply overlays in order (last wins per field). + merged := pa + for _, oi := range overlays { + oi.used = true + if oi.entry.Source != "" { + merged.Source = oi.entry.Source + } + if oi.entry.Enabled != nil { + merged.Enabled = oi.entry.Enabled + } + if oi.entry.Name != "" { + merged.Name = oi.entry.Name + } + } + result = append(result, merged) + } + + // Append overlay entries that did not match any parent agent. + for _, oi := range overlayOrder { + if !oi.used { + result = append(result, oi.entry) + } + } + + return result +} // IsKillSwitchActive reports whether the kill switch is engaged. -func (c *perRepoConfig) IsKillSwitchActive() bool { return c.KillSwitch } +// KillSwitch is a *bool: nil falls through to parent, non-nil +// (including explicit false) is the local decision. +func (c *perRepoConfig) IsKillSwitchActive() bool { + if c.KillSwitch != nil { + return *c.KillSwitch + } + if c.parent != nil { + return c.parent.IsKillSwitchActive() + } + return false +} -// AllowedResources returns the allowed remote resource prefixes. -func (c *perRepoConfig) AllowedResources() []string { return c.AllowedRemoteResources } +// AllowedResources returns the effective allowed remote resource +// prefixes. nil (key omitted) falls through to parent. An explicit +// empty slice signals deny-all with no fallthrough. A non-empty +// slice is unioned with parent resources when a parent is present; +// without a parent, the local list is returned as-is for backwards +// compatibility. Code defaults surface only through the terminal +// perRepoDefaults parent — intermediate parents may omit them. +func (c *perRepoConfig) AllowedResources() []string { + if c.AllowedRemoteResources == nil { + if c.parent != nil { + return c.parent.AllowedResources() + } + return nil + } + if len(c.AllowedRemoteResources) == 0 { + return c.AllowedRemoteResources + } + if c.parent == nil { + return c.AllowedRemoteResources + } + // Non-empty with parent: union overlay + parent. + seen := make(map[string]bool, len(c.AllowedRemoteResources)) + result := make([]string, len(c.AllowedRemoteResources)) + copy(result, c.AllowedRemoteResources) + for _, r := range c.AllowedRemoteResources { + seen[r] = true + } + for _, r := range c.parent.AllowedResources() { + if !seen[r] { + result = append(result, r) + seen[r] = true + } + } + return result +} // IssueCreationConfig returns the cross-repo issue creation config. -func (c *perRepoConfig) IssueCreationConfig() *CreateIssuesConfig { return c.CreateIssues } +// nil falls through to parent. +func (c *perRepoConfig) IssueCreationConfig() *CreateIssuesConfig { + if c.CreateIssues != nil { + return c.CreateIssues + } + if c.parent != nil { + return c.parent.IssueCreationConfig() + } + return nil +} -// ConfigVersion returns the config schema version. -func (c *perRepoConfig) ConfigVersion() string { return c.Version } +// ConfigVersion returns the config schema version. Empty falls +// through to parent (code default "1"). +func (c *perRepoConfig) ConfigVersion() string { + if c.Version != "" { + return c.Version + } + if c.parent != nil { + return c.parent.ConfigVersion() + } + return "" +} // IsOrgMode reports that this is a per-repo configuration. func (c *perRepoConfig) IsOrgMode() bool { return false } -// ConfigRoles returns the configured agent roles. -func (c *perRepoConfig) ConfigRoles() []string { return c.Roles } +// ConfigRoles returns the configured agent roles. nil (key omitted) +// falls through to parent. Non-nil (including empty) replaces the +// parent list entirely. +func (c *perRepoConfig) ConfigRoles() []string { + if c.Roles != nil { + return c.Roles + } + if c.parent != nil { + return c.parent.ConfigRoles() + } + return nil +} -// ConfigRuntime returns the configured agent runtime. -func (c *perRepoConfig) ConfigRuntime() string { return c.Runtime } +// ConfigRuntime returns the configured agent runtime. Empty falls +// through to parent (code default "claude"). +func (c *perRepoConfig) ConfigRuntime() string { + if c.Runtime != "" { + return c.Runtime + } + if c.parent != nil { + return c.parent.ConfigRuntime() + } + return "" +} // --- perRepoConfig setter methods --- -// SetKillSwitch sets the kill switch state. -func (c *perRepoConfig) SetKillSwitch(v bool) { c.KillSwitch = v } +// SetKillSwitch sets the kill switch state. Stores a *bool so that +// an explicit false is distinguishable from unset (nil) across layers. +func (c *perRepoConfig) SetKillSwitch(v bool) { c.KillSwitch = &v } // SetAgents replaces the registered agent entries. func (c *perRepoConfig) SetAgents(agents []AgentEntry) { c.Agents = agents } diff --git a/internal/config/interfaces_test.go b/internal/config/interfaces_test.go index 4268aa8636..0184bd8a1d 100644 --- a/internal/config/interfaces_test.go +++ b/internal/config/interfaces_test.go @@ -137,9 +137,11 @@ func TestPerRepoConfig_AgentEntries(t *testing.T) { } func TestPerRepoConfig_IsKillSwitchActive(t *testing.T) { - cfg := &perRepoConfig{KillSwitch: true} + tr := true + cfg := &perRepoConfig{KillSwitch: &tr} assert.True(t, cfg.IsKillSwitchActive()) - cfg.KillSwitch = false + f := false + cfg.KillSwitch = &f assert.False(t, cfg.IsKillSwitchActive()) } @@ -186,9 +188,11 @@ func TestPerRepoConfig_ConfigRuntime(t *testing.T) { func TestPerRepoConfig_SetKillSwitch(t *testing.T) { cfg := &perRepoConfig{} cfg.SetKillSwitch(true) - assert.True(t, cfg.KillSwitch) + require.NotNil(t, cfg.KillSwitch) + assert.True(t, *cfg.KillSwitch) cfg.SetKillSwitch(false) - assert.False(t, cfg.KillSwitch) + require.NotNil(t, cfg.KillSwitch) + assert.False(t, *cfg.KillSwitch) } func TestPerRepoConfig_SetAgents(t *testing.T) { diff --git a/internal/config/load_test.go b/internal/config/load_test.go index 6a89947f32..a671857ae1 100644 --- a/internal/config/load_test.go +++ b/internal/config/load_test.go @@ -43,7 +43,12 @@ allowed_remote_resources: assert.True(t, cfg.IsKillSwitchActive()) require.Len(t, cfg.AgentEntries(), 1) assert.Equal(t, "ping", cfg.AgentEntries()[0].Name) - assert.Equal(t, []string{"https://example.com/"}, cfg.AllowedResources()) + // AllowedResources unions with parent defaults. + resources := cfg.AllowedResources() + assert.Contains(t, resources, "https://example.com/") + for _, d := range DefaultAllowedRemoteResources() { + assert.Contains(t, resources, d) + } } func TestLoadConfig_Org(t *testing.T) {