From f4fe1599dacd83f73db05ab96f5c4b0daac95582 Mon Sep 17 00:00:00 2001 From: fullsend-code <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Mon, 31 Aug 2026 16:12:55 +0000 Subject: [PATCH] fix(#6678): split scaffold config into base layer and user overlay BuildScaffoldFiles now supports a ConfigLayout field that controls how per-repo config files are generated during admin install: - ConfigLayoutLayered (fresh install): generates config.base.yaml with scaffold defaults and config.yaml as a minimal user-owned overlay - ConfigLayoutUpgrade (re-onboarding): regenerates config.base.yaml with new defaults, leaves config.yaml untouched - ConfigLayoutDefault (empty, backward compatible): generates a single config.yaml with full defaults baked in The admin install path (runPerRepoInstall) now detects whether the repo is already installed and sets the appropriate layout. On re-onboarding, config.yaml is never overwritten, preserving user customizations (custom agents, roles, runtime overrides). New scaffold defaults from fullsend upgrades appear automatically via config.base.yaml fallthrough (ADR 0069 Decision 2). When PerRepoConfig is explicitly set (migration path), the layout is ignored and config.yaml is written from the provided config, preserving existing migration behavior. Closes #6678 --- internal/cli/admin.go | 11 +++ internal/cli/admin_test.go | 69 +++++++++++++++ internal/repos/install.go | 90 +++++++++++++++++-- internal/repos/install_test.go | 155 +++++++++++++++++++++++++++++++++ 4 files changed, 320 insertions(+), 5 deletions(-) diff --git a/internal/cli/admin.go b/internal/cli/admin.go index 8047e867fe..ef87a3415c 100644 --- a/internal/cli/admin.go +++ b/internal/cli/admin.go @@ -862,6 +862,10 @@ func runPerRepoInstall(ctx context.Context, c perRepoInstallConfig) error { // VendorBinary, UpstreamRef, UpstreamTag. Extra fields are included to stay aligned // with the non-dry-run installCfg; Skip* flags are omitted because // they control Install() flow, not scaffold file generation. + dryRunConfigLayout := repos.ConfigLayoutLayered + if alreadyInstalled { + dryRunConfigLayout = repos.ConfigLayoutUpgrade + } dryRunFiles, dryRunErr := repos.BuildScaffoldFiles(repos.InstallConfig{ Owner: owner, Repo: repo, @@ -876,6 +880,7 @@ func runPerRepoInstall(ctx context.Context, c perRepoInstallConfig) error { WIFProvider: inferenceWIFProvider, VendorBinary: vendor, Direct: c.Direct, + ConfigLayout: dryRunConfigLayout, }) if dryRunErr != nil { return fmt.Errorf("generating scaffold files for dry run: %w", dryRunErr) @@ -1084,6 +1089,11 @@ func runPerRepoInstall(ctx context.Context, c perRepoInstallConfig) error { // Resolve review app client ID for provenance validation. reviewAppClientID := resolveReviewAppClientID(ctx, client, c.AppSet) + configLayout := repos.ConfigLayoutLayered + if alreadyInstalled { + configLayout = repos.ConfigLayoutUpgrade + } + installCfg := repos.InstallConfig{ Owner: owner, Repo: repo, @@ -1101,6 +1111,7 @@ func runPerRepoInstall(ctx context.Context, c perRepoInstallConfig) error { VendorBinary: vendor, Direct: c.Direct, SkipScaffoldAndConfig: vendor, + ConfigLayout: configLayout, } progressFn := func(_ string, phase, msg string) { diff --git a/internal/cli/admin_test.go b/internal/cli/admin_test.go index 6af1a00d59..571393695b 100644 --- a/internal/cli/admin_test.go +++ b/internal/cli/admin_test.go @@ -2280,6 +2280,75 @@ func TestRunPerRepoInstall_AlreadyInstalledUpgrade(t *testing.T) { require.NoError(t, err) } +// TestRunPerRepoInstall_FreshInstall_LayeredConfig verifies that a fresh +// per-repo install generates both config.base.yaml (scaffold defaults) +// and config.yaml (minimal overlay) per the layered config model. +func TestRunPerRepoInstall_FreshInstall_LayeredConfig(t *testing.T) { + client := forge.NewFakeClient() + client.Repos = []forge.Repository{ + {Name: "widget", FullName: "acme/widget", DefaultBranch: "main"}, + } + + cfg := perRepoTestBase() + cfg.testClient = client + cfg.Direct = true + + err := runPerRepoInstall(context.Background(), cfg) + require.NoError(t, err) + + // Verify scaffold files include both config.base.yaml and config.yaml. + require.NotEmpty(t, client.CommittedFiles, "expected scaffold files to be committed") + paths := make(map[string]string) + for _, f := range client.CommittedFiles[0].Files { + paths[f.Path] = string(f.Content) + } + + assert.Contains(t, paths, ".fullsend/config.base.yaml", + "fresh install should generate config.base.yaml") + assert.Contains(t, paths, ".fullsend/config.yaml", + "fresh install should generate config.yaml overlay") + + // config.base.yaml should contain scaffold defaults (roles). + assert.Contains(t, paths[".fullsend/config.base.yaml"], "roles:", + "config.base.yaml should contain roles") + + // config.yaml should be a minimal overlay (no roles baked in). + assert.NotContains(t, paths[".fullsend/config.yaml"], "roles:", + "config.yaml overlay should not contain roles") +} + +// TestRunPerRepoInstall_ReOnboarding_PreservesUserConfig verifies that +// re-onboarding generates config.base.yaml with new defaults but does +// NOT overwrite config.yaml (the user-owned overlay). +func TestRunPerRepoInstall_ReOnboarding_PreservesUserConfig(t *testing.T) { + client := forge.NewFakeClient() + client.VariableValues = map[string]string{ + "acme/widget/FULLSEND_MINT_URL": "https://mint.example.com/v1/token", + } + client.Repos = []forge.Repository{ + {Name: "widget", FullName: "acme/widget", DefaultBranch: "main"}, + } + + cfg := perRepoTestBase() + cfg.testClient = client + cfg.Direct = true + + err := runPerRepoInstall(context.Background(), cfg) + require.NoError(t, err) + + // Verify scaffold files include config.base.yaml but NOT config.yaml. + require.NotEmpty(t, client.CommittedFiles, "expected scaffold files to be committed") + paths := make(map[string]bool) + for _, f := range client.CommittedFiles[0].Files { + paths[f.Path] = true + } + + assert.True(t, paths[".fullsend/config.base.yaml"], + "re-onboarding should generate config.base.yaml") + assert.False(t, paths[".fullsend/config.yaml"], + "re-onboarding should NOT overwrite config.yaml — it is user-owned") +} + func TestRunPerRepoInstall_DryRun(t *testing.T) { client := forge.NewFakeClient() diff --git a/internal/repos/install.go b/internal/repos/install.go index f94e6380f7..2977ee3c03 100644 --- a/internal/repos/install.go +++ b/internal/repos/install.go @@ -24,6 +24,44 @@ var WIFProviderPattern = regexp.MustCompile( `^projects/\d+/locations/global/workloadIdentityPools/[a-z][a-z0-9-]{2,30}[a-z0-9]/providers/[a-z][a-z0-9-]{2,30}[a-z0-9]$`, ) +// ConfigLayout controls how BuildScaffoldFiles generates per-repo config +// files. The default (empty string) preserves backward-compatible behavior. +type ConfigLayout string + +const ( + // ConfigLayoutDefault generates .fullsend/config.yaml with full + // scaffold defaults baked in. This is the backward-compatible default + // used by callers that have not opted into the layered config model + // (e.g. converge/drift detection). + ConfigLayoutDefault ConfigLayout = "" + + // ConfigLayoutLayered generates .fullsend/config.base.yaml with + // scaffold defaults and .fullsend/config.yaml as a minimal user-owned + // overlay. Used for fresh installs via admin install. + ConfigLayoutLayered ConfigLayout = "layered" + + // ConfigLayoutUpgrade generates only .fullsend/config.base.yaml with + // scaffold defaults. config.yaml is user-owned and left untouched. + // Used for re-onboarding via admin install. + ConfigLayoutUpgrade ConfigLayout = "upgrade" +) + +// minimalOverlayYAML is the stub config.yaml overlay committed on fresh +// installs when ConfigLayoutLayered is active. Scaffold defaults live in +// config.base.yaml; this file is user-owned for customization. +const minimalOverlayYAML = `# fullsend per-repo configuration (overlay) +# https://github.com/fullsend-ai/fullsend +# +# This file is the per-repo overlay for fullsend configuration. +# Scaffold defaults are provided by config.base.yaml. +# Values set here override the base layer. Omitted fields inherit +# from config.base.yaml, then from compiled-in code defaults. +# +# See https://fullsend.sh/docs/guides/infrastructure/layered-config-reference + +version: "1" +` + // InstallConfig is a pure data struct holding all inputs needed for a // per-repo installation. CLI flags, environment variables, and interactive // prompts are resolved by the caller before constructing this struct. @@ -83,6 +121,10 @@ type InstallConfig struct { // pipeline YAML so that agent jobs are routed to specific runners. RunnerTags []string + // ConfigLayout controls how BuildScaffoldFiles generates per-repo + // config files. See the ConfigLayout* constants. + ConfigLayout ConfigLayout + // Direct controls scaffold delivery: true pushes directly to the default // branch; false creates a PR. Direct bool @@ -402,11 +444,49 @@ func BuildScaffoldFiles(cfg InstallConfig) ([]forge.TreeFile, error) { Mode: f.Mode, }) } - files = append(files, forge.TreeFile{ - Path: ".fullsend/config.yaml", - Content: cfgYAML, - Mode: "100644", - }) + + // When PerRepoConfig is set, the caller provided an explicit config + // (e.g. migration path). Always write it as config.yaml regardless + // of ConfigLayout. When PerRepoConfig is nil, ConfigLayout controls + // the file strategy. + if cfg.PerRepoConfig != nil { + files = append(files, forge.TreeFile{ + Path: ".fullsend/config.yaml", + Content: cfgYAML, + Mode: "100644", + }) + } else { + switch cfg.ConfigLayout { + case ConfigLayoutLayered: + // Fresh install with layered config: write scaffold defaults + // to config.base.yaml and a minimal overlay to config.yaml. + files = append(files, forge.TreeFile{ + Path: ".fullsend/config.base.yaml", + Content: cfgYAML, + Mode: "100644", + }) + files = append(files, forge.TreeFile{ + Path: ".fullsend/config.yaml", + Content: []byte(minimalOverlayYAML), + Mode: "100644", + }) + case ConfigLayoutUpgrade: + // Re-onboarding: regenerate config.base.yaml with new + // defaults. config.yaml is user-owned and left untouched. + files = append(files, forge.TreeFile{ + Path: ".fullsend/config.base.yaml", + Content: cfgYAML, + Mode: "100644", + }) + default: + // Legacy/default: single config.yaml with full defaults. + files = append(files, forge.TreeFile{ + Path: ".fullsend/config.yaml", + Content: cfgYAML, + Mode: "100644", + }) + } + } return files, nil } diff --git a/internal/repos/install_test.go b/internal/repos/install_test.go index cb05794d6a..fce2c52c37 100644 --- a/internal/repos/install_test.go +++ b/internal/repos/install_test.go @@ -7,6 +7,7 @@ import ( "sync" "testing" + "github.com/fullsend-ai/fullsend/internal/config" "github.com/fullsend-ai/fullsend/internal/forge" "github.com/fullsend-ai/fullsend/internal/scaffold" ) @@ -898,6 +899,160 @@ func TestBuildScaffoldFiles_UnsupportedForge(t *testing.T) { } } +// TestBuildScaffoldFiles_LayeredFreshInstall verifies that a fresh install +// with ConfigLayoutLayered generates both config.base.yaml (scaffold +// defaults) and config.yaml (minimal overlay). +func TestBuildScaffoldFiles_LayeredFreshInstall(t *testing.T) { + cfg := baseCfg() + cfg.ConfigLayout = ConfigLayoutLayered + + files, err := BuildScaffoldFiles(cfg) + if err != nil { + t.Fatalf("BuildScaffoldFiles() returned error: %v", err) + } + + paths := make(map[string][]byte) + for _, f := range files { + paths[f.Path] = f.Content + } + + // config.base.yaml must exist with scaffold defaults. + baseContent, hasBase := paths[".fullsend/config.base.yaml"] + if !hasBase { + t.Fatal("expected .fullsend/config.base.yaml in scaffold files") + } + if !strings.Contains(string(baseContent), "roles:") { + t.Error("config.base.yaml should contain roles") + } + + // config.yaml must exist as a minimal overlay. + overlayContent, hasOverlay := paths[".fullsend/config.yaml"] + if !hasOverlay { + t.Fatal("expected .fullsend/config.yaml in scaffold files") + } + if !strings.Contains(string(overlayContent), "version:") { + t.Error("config.yaml overlay should contain version") + } + // The overlay must NOT contain scaffold defaults like roles. + if strings.Contains(string(overlayContent), "roles:") { + t.Error("config.yaml overlay should not contain roles (those belong in config.base.yaml)") + } +} + +// TestBuildScaffoldFiles_UpgradeSkipsConfigYAML verifies that re-onboarding +// with ConfigLayoutUpgrade generates config.base.yaml but does NOT generate +// config.yaml, leaving the user's existing overlay untouched. +func TestBuildScaffoldFiles_UpgradeSkipsConfigYAML(t *testing.T) { + cfg := baseCfg() + cfg.ConfigLayout = ConfigLayoutUpgrade + + files, err := BuildScaffoldFiles(cfg) + if err != nil { + t.Fatalf("BuildScaffoldFiles() returned error: %v", err) + } + + var hasBase, hasConfig bool + for _, f := range files { + switch f.Path { + case ".fullsend/config.base.yaml": + hasBase = true + if !strings.Contains(string(f.Content), "roles:") { + t.Error("config.base.yaml should contain roles") + } + case ".fullsend/config.yaml": + hasConfig = true + } + } + if !hasBase { + t.Fatal("expected .fullsend/config.base.yaml in scaffold files") + } + if hasConfig { + t.Error("config.yaml should NOT be generated on upgrade — it is user-owned") + } +} + +// TestBuildScaffoldFiles_UpgradeRuntime verifies that --runtime on +// re-onboarding writes the runtime to config.base.yaml. +func TestBuildScaffoldFiles_UpgradeRuntime(t *testing.T) { + cfg := baseCfg() + cfg.ConfigLayout = ConfigLayoutUpgrade + cfg.Runtime = "pi" + + files, err := BuildScaffoldFiles(cfg) + if err != nil { + t.Fatalf("BuildScaffoldFiles() returned error: %v", err) + } + + for _, f := range files { + if f.Path == ".fullsend/config.base.yaml" { + if !strings.Contains(string(f.Content), "runtime: pi") { + t.Errorf("config.base.yaml should contain runtime: pi:\n%s", f.Content) + } + return + } + } + t.Fatal("expected .fullsend/config.base.yaml in scaffold files") +} + +// TestBuildScaffoldFiles_DefaultLayoutBackwardCompat verifies that the +// default ConfigLayout (empty string) preserves backward-compatible +// behavior: a single config.yaml with full defaults, no config.base.yaml. +func TestBuildScaffoldFiles_DefaultLayoutBackwardCompat(t *testing.T) { + cfg := baseCfg() + // ConfigLayout is zero value (ConfigLayoutDefault). + + files, err := BuildScaffoldFiles(cfg) + if err != nil { + t.Fatalf("BuildScaffoldFiles() returned error: %v", err) + } + + var hasConfig, hasBase bool + for _, f := range files { + switch f.Path { + case ".fullsend/config.yaml": + hasConfig = true + case ".fullsend/config.base.yaml": + hasBase = true + } + } + if !hasConfig { + t.Error("expected .fullsend/config.yaml with default layout") + } + if hasBase { + t.Error("config.base.yaml should NOT be generated with default layout") + } +} + +// TestBuildScaffoldFiles_PerRepoConfigOverrideIgnoresLayout verifies that +// when PerRepoConfig is set (migration/override path), ConfigLayout is +// ignored and config.yaml is generated from the provided config. +func TestBuildScaffoldFiles_PerRepoConfigOverrideIgnoresLayout(t *testing.T) { + cfg := baseCfg() + cfg.ConfigLayout = ConfigLayoutUpgrade + cfg.PerRepoConfig = config.NewPerRepoConfig([]string{"triage"}, "acme/widgets") + + files, err := BuildScaffoldFiles(cfg) + if err != nil { + t.Fatalf("BuildScaffoldFiles() returned error: %v", err) + } + + var hasConfig, hasBase bool + for _, f := range files { + switch f.Path { + case ".fullsend/config.yaml": + hasConfig = true + case ".fullsend/config.base.yaml": + hasBase = true + } + } + if !hasConfig { + t.Error("expected .fullsend/config.yaml when PerRepoConfig is set") + } + if hasBase { + t.Error("config.base.yaml should NOT be generated when PerRepoConfig is set") + } +} + func TestInstall_FreshInstall_GitLab(t *testing.T) { fc := newFakeClientWithRepo() cfg := InstallConfig{