From 0b89ec56e37d34f1fd88c03351cadcb14f6c89ac Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Fri, 2 Oct 2026 20:53:32 -0400 Subject: [PATCH 1/6] fix(configurations): a workspace override of a composed module's group overlays per key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A consuming workspace that declared a group one of its composed modules also provides replaced that group WHOLE. Three things were lost with it: the module's values for every key the workspace never mentioned, the ${profile} declarations among them — so nothing reported the omission, the load returned nil, and an empty override discharged a value that must differ per environment — and the group's provenance, which reclassified it as the composition root's own and widened its delivery from the services that declared it to every service of the composition. A partial override narrowed a group's contents and widened its delivery at the same time. The workspace now wins per key. composeModuleWorkspaceConfigurations no longer drops a module's offer on a name the workspace declares: the offer is resolved like any other, and the workspace's values are overlaid onto the resolved group by overlayWorkspaceConfigurationOverride, which carries the rules that already hold between profile derivation layers across this boundary — the module's group is the declared set of the group's keys (ErrUndeclaredProfileKey), a marker the override does not discharge is still owed, and an override that empties one is refused naming the key (ErrEmptyProfileValue, new). The overlaid group stays composed, so its delivery stays scoped to the services that declared it. The per-key primitives are now shared with profileOverlay rather than written twice (findConfigurationValue, setConfigurationValue). Fixes #693 Co-Authored-By: Claude Opus 5 (1M context) --- configurations/doc.go | 11 +- configurations/local_reader.go | 221 +++++++++++-- configurations/local_reader_test.go | 9 +- configurations/manager_test.go | 23 +- configurations/profile.go | 73 +++-- configurations/workspace_override_test.go | 379 ++++++++++++++++++++++ docs/workspace-composition.md | 64 ++++ 7 files changed, 726 insertions(+), 54 deletions(-) create mode 100644 configurations/workspace_override_test.go diff --git a/configurations/doc.go b/configurations/doc.go index d9934ad66..f0dfe1d57 100644 --- a/configurations/doc.go +++ b/configurations/doc.go @@ -115,9 +115,14 @@ // GetCompositionRootWorkspaceConfigurations returns exactly the root-provided set // for that run-wide injection, resolved and endpoint-interpolated like // GetWorkspaceConfigurations. The root-provided and per-dependency composed-module -// sets are disjoint by name (a name the root also declares is resolved to the -// root at load), so the root fills what a composed module leaves unset without -// shadowing a name only the module provides. An invocation-scoped override is the +// sets are disjoint by name: a name a composed module provides stays in the +// composed set even when the root declares it too, because the root's values are +// overlaid onto the module's group PER KEY (composeModuleWorkspaceConfigurations) +// — the group is still the module's, so it is still delivered to the services +// that declared it, and a run-wide injection of it would carry the module's own +// keys into every service of the composition. So the root fills what a composed +// module leaves unset without shadowing a name only the module provides, and +// without widening that name's delivery. An invocation-scoped override is the // run supplying a value, so it is composition-root even when it lands on a name a // composed module also provides. // diff --git a/configurations/local_reader.go b/configurations/local_reader.go index c8bdefd28..adbef6606 100644 --- a/configurations/local_reader.go +++ b/configurations/local_reader.go @@ -89,11 +89,17 @@ func NewConfigurationLocalReader(_ context.Context, workspace *resources.Workspa // composed modules ship. See ReadWorkspaceConfigurations. type WorkspaceConfigurations struct { // Infos holds every provided configuration in provisioning order: the - // workspace's own first, then the composed modules' contributions. + // workspace's own first, then the composed modules' contributions. A group + // the workspace overrides keeps the position the workspace declared it in, + // carrying the module's group with the workspace's values overlaid on it. Infos []*basev0.ConfigurationInformation // ComposedBy names, for each configuration a composed module contributes, - // the module that provides it. A name absent from it is the workspace's - // own — which wins over any module offering the same name. + // the module that provides it. A name the consuming workspace also declares + // stays in it: the workspace's values are overlaid onto the module's group + // per key, so the group is still the module's and its delivery stays scoped + // to the services that declared it. A name absent from it is the + // workspace's own alone — no composed module offers it, or the modules that + // do disagree and the workspace's declaration resolves the ambiguity. ComposedBy map[string]string // Ambiguous holds the names two composed modules define differently, mapped // to the diagnostic naming both providers. They are absent from Infos: a @@ -132,11 +138,11 @@ func ReadWorkspaceConfigurations(ctx context.Context, workspace *resources.Works if err != nil { return nil, w.Wrapf(err, "cannot select workspace configuration directory") } - workspaceInfos, unsupplied, err := readOwnedWorkspaceConfigurations(ctx, workspace, profiles) + workspaceLevel, err := readOwnedWorkspaceConfigurations(ctx, workspace, profiles) if err != nil { return nil, w.Wrapf(err, "cannot inherit workspace configurations") } - workspaceInfos, composedBy, ambiguous, composedUnsupplied, err := composeModuleWorkspaceConfigurations(ctx, workspace, workspaceInfos, configurationDir, profiles) + workspaceInfos, composedBy, ambiguous, composedUnsupplied, err := composeModuleWorkspaceConfigurations(ctx, workspace, workspaceLevel, configurationDir, profiles) if err != nil { return nil, w.Wrapf(err, "cannot compose module workspace configurations") } @@ -144,38 +150,71 @@ func ReadWorkspaceConfigurations(ctx context.Context, workspace *resources.Works Infos: workspaceInfos, ComposedBy: composedBy, Ambiguous: ambiguous, - Unsupplied: append(unsupplied, composedUnsupplied...), + Unsupplied: append(workspaceLevel.Unsupplied, composedUnsupplied...), }, nil } +// ownedWorkspaceConfigurations is what the workspace level of one composition +// provides: a workspace's own configuration groups and the ones it inherited +// from the workspaces it composes, with the two kinds of provenance kept apart. +type ownedWorkspaceConfigurations struct { + // Infos are the groups in provisioning order, the workspace's own first. + Infos []*basev0.ConfigurationInformation + // Own names the groups this workspace declares ITSELF. + Own map[string]bool + // FromComposedWorkspace names every group a composed workspace contributed, + // including the ones this workspace's own declaration suppressed — so a + // consumer can tell a name that is this workspace's alone from one that two + // workspace-level declarations both carry. + FromComposedWorkspace map[string]bool + // Unsupplied are the per-profile values the provisioned groups still owe. + Unsupplied []ProfileRequirement +} + // A product inherits its selected workspace's wiring. Its own groups override // the inherited groups; sibling workspaces must agree unless the product chooses. // Module defaults are composed separately, retaining their existing precedence. -func readOwnedWorkspaceConfigurations(ctx context.Context, workspace *resources.Workspace, profiles []string) ([]*basev0.ConfigurationInformation, []ProfileRequirement, error) { +// +// Provenance is reported, not just the result, because the two kinds are not +// interchangeable against a composed module's group of the same name. A group +// the consuming workspace declares itself, and no composed workspace also +// carries, is overlaid onto the module's group per key +// (composeModuleWorkspaceConfigurations). Any other workspace-level declaration +// of that name still replaces the module's group whole: an inherited declaration +// is not the consuming workspace's to amend, so a per-key overlay there would +// refuse a product over two files it cannot edit, and where a name is declared by +// a composed workspace AND the product, overlaying the product's onto the +// MODULE's group would rank a module default above the inherited declaration it +// is supposed to sit below. Both want the precedence model of workspace +// composition decided per key, which this composition rule does not do. +func readOwnedWorkspaceConfigurations(ctx context.Context, workspace *resources.Workspace, profiles []string) (*ownedWorkspaceConfigurations, error) { provided, err := LoadProfileConfigurations(ctx, workspace.Dir(), "configurations", profiles) if err != nil { - return nil, nil, err + return nil, err } infos := provided.Infos unsupplied := provided.Unsupplied owned := make(map[string]bool) + fromComposed := make(map[string]bool) inherited := make(map[string]*basev0.ConfigurationInformation) owners := make(map[string]string) for _, info := range infos { owned[info.Name] = true } for _, child := range workspace.ComposedWorkspaces() { - contributions, childUnsupplied, err := readOwnedWorkspaceConfigurations(ctx, child, profiles) + contributed, err := readOwnedWorkspaceConfigurations(ctx, child, profiles) if err != nil { - return nil, nil, err + return nil, err } - for _, info := range contributions { + childUnsupplied := contributed.Unsupplied + for _, info := range contributed.Infos { + fromComposed[info.Name] = true if owned[info.Name] { continue } if previous, ok := inherited[info.Name]; ok { if !proto.Equal(previous, info) { - return nil, nil, fmt.Errorf("workspace configuration %q differs between workspaces %q and %q: %w", info.Name, owners[info.Name], child.Name, ErrConfigurationConflict) + return nil, fmt.Errorf("workspace configuration %q differs between workspaces %q and %q: %w", info.Name, owners[info.Name], child.Name, ErrConfigurationConflict) } continue } @@ -192,7 +231,12 @@ func readOwnedWorkspaceConfigurations(ctx context.Context, workspace *resources. unsupplied = append(unsupplied, requirement) } } - return infos, unsupplied, nil + return &ownedWorkspaceConfigurations{ + Infos: infos, + Own: owned, + FromComposedWorkspace: fromComposed, + Unsupplied: unsupplied, + }, nil } func (local *ConfigurationInformationLocalReader) Load(ctx context.Context, env *resources.Environment) error { @@ -367,7 +411,31 @@ type composedConfiguration struct { // // The consuming workspace wins on a name conflict, so a solution can override a // composed configuration by declaring one of the same name, but is never -// required to redeclare every configuration the host brings. Two composed +// required to redeclare every configuration the host brings. It wins PER KEY: +// the workspace's values are overlaid onto the module's group, which keeps +// supplying every key the workspace does not mention. Replacing the group whole +// instead lost three things at once — the module's defaults for the keys the +// workspace never mentioned, the ProfileValueMarker declarations among them (so +// nothing reported the omission and an empty override discharged a value that +// must differ per environment), and the group's provenance, which reclassified +// it as the composition root's own and widened its delivery from the services +// that declared it to every service of the composition. A partial override +// narrowed a group's contents and widened its delivery at the same time. So the +// overlaid group stays composed, the module remains its provider, and the rules +// that hold between profile derivation layers hold across this boundary too: +// the module's group is the declared set of the group's keys (a key only the +// workspace carries is refused, ErrUndeclaredProfileKey), a marker the override +// does not discharge is still owed, and an override that empties one is refused +// naming it (ErrEmptyProfileValue). A workspace that needs a key of its own +// declares a group of its own name, which reaches every service of the +// composition. +// +// That is the consuming workspace's own declaration of a name no composed +// workspace also carries. Every other workspace-level declaration still replaces +// a module's group of that name whole — see readOwnedWorkspaceConfigurations for +// why the product model's precedence is not decided here. +// +// Two composed // modules defining the same name identically is not a conflict — there is one // value to provision and nothing to arbitrate, which is what two submodules // vendoring the same file amount to. Two that define it *differently* are @@ -387,7 +455,7 @@ type composedConfiguration struct { func composeModuleWorkspaceConfigurations( ctx context.Context, workspace *resources.Workspace, - workspaceInfos []*basev0.ConfigurationInformation, + workspaceLevel *ownedWorkspaceConfigurations, workspaceConfigurationDir string, profiles []string, ) ([]*basev0.ConfigurationInformation, map[string]string, map[string]error, []ProfileRequirement, error) { @@ -397,9 +465,20 @@ func composeModuleWorkspaceConfigurations( if err != nil { return nil, nil, nil, nil, w.Wrapf(err, "cannot load modules") } - fromWorkspace := make(map[string]bool, len(workspaceInfos)) - for _, info := range workspaceInfos { - fromWorkspace[info.Name] = true + workspaceInfos := workspaceLevel.Infos + // overriding locates the consuming workspace's own declaration of a name, by + // position: a module offering that name does not lose to it, it is overlaid + // onto it in place, where the workspace declared it. replacing holds every + // other workspace-level declaration — inherited from a composed workspace, or + // shadowing one — which still wins whole. + overriding := make(map[string]int, len(workspaceInfos)) + replacing := make(map[string]bool, len(workspaceInfos)) + for index, info := range workspaceInfos { + if workspaceLevel.Own[info.Name] && !workspaceLevel.FromComposedWorkspace[info.Name] { + overriding[info.Name] = index + continue + } + replacing[info.Name] = true } sourceReferenced := make(map[string]bool) for _, ref := range workspace.Modules { @@ -412,11 +491,17 @@ func composeModuleWorkspaceConfigurations( var order []string offer := func(module string, repo string, fromRepo bool, infos []*basev0.ConfigurationInformation) { for _, info := range infos { - if fromWorkspace[info.Name] { - w.Debug("workspace configuration overrides composed module configuration", + if replacing[info.Name] { + w.Debug("workspace-level configuration a composed workspace also declares replaces composed module configuration", wool.Field("configuration", info.Name), wool.Field("module", module)) continue } + // A name the consuming workspace declares ITSELF is not dropped here: + // its values are overlaid onto this group once every offer is in, so + // the group the workspace overrides must first be resolved like any + // other — a submodule's declaration over its repository's, and an + // ambiguity between two modules that the workspace's own declaration + // then resolves. existing, ok := composed[info.Name] if !ok { composed[info.Name] = &composedConfiguration{info: info, module: module, repo: repo, fromRepo: fromRepo} @@ -461,9 +546,11 @@ func composeModuleWorkspaceConfigurations( loaded := make(map[string]bool) // unsuppliedByGroup holds, per group name a composed module offers, the // per-profile values that module left unsupplied. It is keyed by group so only - // the requirements of the offers that actually win are reported: a group the - // consuming workspace overrides, or that loses an ambiguity, is not - // provisioned and owes nothing. + // the requirements of the offers that actually win are reported: a group that + // loses an ambiguity is not provisioned and owes nothing. A group the + // consuming workspace overrides still owes them — the override is per key, so + // a marker it does not discharge is still in the provisioned group, and + // StillUnsupplied drops exactly the ones it did discharge. unsuppliedByGroup := make(map[string][]ProfileRequirement) readOnce := func(dir string) ([]*basev0.ConfigurationInformation, error) { key := dir @@ -536,19 +623,105 @@ func composeModuleWorkspaceConfigurations( var unsupplied []ProfileRequirement for _, name := range order { configuration := composed[name] + index, overridden := overriding[name] if configuration.conflict != nil { + // The ambiguity's own remedy is a declaration in the consuming + // workspace, so one that is already there resolves it rather than + // being reported it. There is no single group to overlay onto — + // that is what the two modules disagree about — so the workspace's + // declaration stands whole, as the composition root's own. + if overridden { + w.Debug("the consuming workspace's own configuration resolves an ambiguity between composed modules", + wool.Field("configuration", name)) + continue + } w.Warn("composed modules disagree on a workspace configuration; it is unavailable until the solution declares it", wool.Field("configuration", name)) ambiguous[name] = configuration.conflict continue } + info := configuration.info + if overridden { + overlaid, err := overlayWorkspaceConfigurationOverride(configuration.info, workspaceInfos[index], configuration.module) + if err != nil { + return nil, nil, nil, nil, w.Wrap(err) + } + w.Debug("workspace configuration overrides composed module configuration, per key", + wool.Field("configuration", name), wool.Field("module", configuration.module)) + info = overlaid + } + // The group is the module's whether or not the workspace overrode keys of + // it, so it is reported as composed either way and reaches the services + // that declared it rather than every service of the composition. composedBy[name] = configuration.module - workspaceInfos = append(workspaceInfos, configuration.info) unsupplied = append(unsupplied, unsuppliedByGroup[name]...) + if overridden { + workspaceInfos[index] = info + continue + } + workspaceInfos = append(workspaceInfos, info) } return workspaceInfos, composedBy, ambiguous, unsupplied, nil } +// overlayWorkspaceConfigurationOverride overlays the consuming workspace's +// declaration of a group onto the group a composed module provides, PER KEY: the +// workspace supplies the values it declares, and the module's group supplies +// every other key it declares. It is the same overlay a derived profile gets over +// the profile it derives from (profileOverlay), applied across the +// workspace/module boundary, and it carries the same two rules with it. +// +// The module's group is the declared SET of the group's keys. A key only the +// workspace carries is refused naming it: the group is the module's, delivered to +// the module's services, and a key its declaration never mentions is a value no +// reader of that group knows to read — while the workspace has a way to say what +// it means, a group of its own name, which reaches every service of the +// composition. A ProfileValueMarker the override does not discharge survives it +// and is still owed, and an override that empties one is refused naming it: the +// marker is the module author's statement that this key must carry a real value +// in each environment, and a consumer of that group cannot discharge it with +// nothing. +// +// A structured document has no keys to overlay, so the workspace's document +// replaces the module's whole — as a derived profile's does — and a boundary +// that would turn a document into key/value pairs, or the reverse, is a conflict +// rather than a silent choice between them. +func overlayWorkspaceConfigurationOverride( + base *basev0.ConfigurationInformation, + override *basev0.ConfigurationInformation, + module string, +) (*basev0.ConfigurationInformation, error) { + overlaid, ok := proto.Clone(base).(*basev0.ConfigurationInformation) + if !ok { + return nil, fmt.Errorf("cannot clone composed module configuration %q", base.GetName()) + } + if (override.GetData() != nil) != (overlaid.GetData() != nil) { + return nil, fmt.Errorf("workspace configuration %q is a structured document on one side of the workspace/module boundary and key/value pairs on the other, between the consuming workspace and composed module %q: %w", + override.GetName(), module, ErrConfigurationConflict) + } + if override.GetData() != nil { + if DataDeclaredPerProfile(overlaid.GetData()) && len(bytes.TrimSpace(override.GetData().GetContent())) == 0 { + return nil, fmt.Errorf("workspace configuration %q (the whole document) is supplied per profile by composed module %q, and the consuming workspace supplies an empty document: %w", + override.GetName(), module, ErrEmptyProfileValue) + } + overlaid.Data = override.GetData() + return overlaid, nil + } + for _, value := range override.GetConfigurationValues() { + existing := findConfigurationValue(overlaid, value.GetKey()) + if existing == nil { + return nil, fmt.Errorf("the consuming workspace overrides %s/%s, a key composed module %q does not declare in the group it provides; declare it there (as %s when each profile supplies its own), or declare a group of the workspace's own name: %w", + override.GetName(), value.GetKey(), module, ProfileValueMarker, ErrUndeclaredProfileKey) + } + if ValueDeclaredPerProfile(existing) && configurationValueSuppliesNothing(value) { + return nil, fmt.Errorf("%s/%s is supplied per profile by composed module %q, and the consuming workspace supplies an empty value: %w", + override.GetName(), value.GetKey(), module, ErrEmptyProfileValue) + } + setConfigurationValue(overlaid, value) + } + return overlaid, nil +} + // ProfileDirectory selects the profile directory one configuration location is // read from: // for the first profile in the chain that // exists there, and whether one does. Each location resolves on its own, so a diff --git a/configurations/local_reader_test.go b/configurations/local_reader_test.go index 86cc2c6cf..e3c4ca28e 100644 --- a/configurations/local_reader_test.go +++ b/configurations/local_reader_test.go @@ -1017,12 +1017,15 @@ modules: require.Equal(t, "host-a-legal", url) require.Empty(t, loader.CompositionRootWorkspaceConfigurationNames()) - // The workspace's own file wins over a module's: the name is no longer - // composed, and the module's value is not what is read. + // The workspace's own file wins over a module's, per key: the module's value + // is not what is read, and the group is still the module's — the workspace + // overrode a key of it, which does not make the group its own. See + // TestAnOverriddenComposedGroupStaysComposedAndKeepsItsDeliveryScoped for + // why that is the delivery this must report. writeConfigurationFile(t, root, "solution/configurations/local/legal.env", "LEGAL_URL=solution-legal\n") provided, err = configurations.ReadWorkspaceConfigurations(ctx, workspace, resources.LocalEnvironment()) require.NoError(t, err) - require.Equal(t, map[string]string{"analytics": "host-b"}, provided.ComposedBy) + require.Equal(t, map[string]string{"legal": "host-a", "analytics": "host-b"}, provided.ComposedBy) for _, info := range provided.Infos { if info.Name != "legal" { continue diff --git a/configurations/manager_test.go b/configurations/manager_test.go index 22ae0b0e9..77bdda328 100644 --- a/configurations/manager_test.go +++ b/configurations/manager_test.go @@ -559,9 +559,18 @@ layout: modules } // When the composition root and a composed module both declare a configuration -// of the same name, the root wins at load (mirror of #374) and the name is -// composition-root origin — the run-wide value is the root's, never the -// module's suppressed one. +// of the same name, the root's VALUE wins at load (mirror of #374): what a +// consumer reads is the root's, never the module's suppressed one. +// +// The group itself stays the module's, so it is not composition-root origin and +// is not injected run-wide (#693). It used to be, back when the root's +// declaration replaced the module's group whole; once the root's values are +// overlaid onto that group per key, a run-wide injection would carry the +// MODULE's keys — every key the root did not mention — into every service of the +// composition, which is not what declaring one key of someone else's group says. +// A root group under a name no module provides is run-wide as it always was, and +// so is an operator's --set +// (TestManagerCompositionRootConfigurationsIncludeOverrideOfComposedName). func TestManagerCompositionRootWinsOnNameConflict(t *testing.T) { ctx := context.Background() root := t.TempDir() @@ -603,8 +612,12 @@ agent: rootConfs, err := manager.GetCompositionRootWorkspaceConfigurations(ctx) require.NoError(t, err) - require.Len(t, rootConfs, 1) - url, err := resources.GetConfigurationValue(ctx, rootConfs[0], "work-context", "authority-jwks-url") + require.Empty(t, rootConfs, "an overridden composed group is not injected run-wide") + + depConfs, err := manager.GetWorkspaceDependenciesConfigurations(ctx, "work-context") + require.NoError(t, err) + require.Len(t, depConfs, 1) + url, err := resources.GetConfigurationValue(ctx, depConfs[0], "work-context", "authority-jwks-url") require.NoError(t, err) require.Equal(t, "https://root/jwks.json", url) } diff --git a/configurations/profile.go b/configurations/profile.go index 1792db2fe..de7eae643 100644 --- a/configurations/profile.go +++ b/configurations/profile.go @@ -60,15 +60,29 @@ type ProfileDerivation struct { // deriving from one that is not there, a cycle, or a chain past the depth bound. var ErrProfileDerivation = errors.New("configuration profile derivation cannot be resolved") -// ErrUndeclaredProfileKey marks a key a derived profile introduces into a group -// the profile it derives from already declares. The base profile is the declared -// set of a group's keys, and a key only one profile carries is a difference -// between environments that the declared set never mentions: absent from the -// profiles that do not carry it, it resolves to nothing at all — the empty string -// every reader of a missing key gets — which is the silent, until-runtime failure -// the declared set exists to prevent. Declaring it in the base, with -// ProfileValueMarker when each profile must supply its own, is the remedy. -var ErrUndeclaredProfileKey = errors.New("configuration key is not declared by the profile it is derived from") +// ErrUndeclaredProfileKey marks a key a layer introduces into a group the layer +// below it already declares: a derived profile into the profile it derives from, +// or a consuming workspace into the group a composed module provides. The layer +// below is the declared set of a group's keys, and a key only the layer above +// carries is a difference the declared set never mentions: absent wherever that +// layer is not read, it resolves to nothing at all — the empty string every +// reader of a missing key gets — which is the silent, until-runtime failure the +// declared set exists to prevent. Declaring it in the layer below, with +// ProfileValueMarker when each profile must supply its own, is the remedy; a +// consuming workspace that cannot amend the module's group declares a group of +// its own name instead, which reaches every service of the composition. +var ErrUndeclaredProfileKey = errors.New("configuration key is not declared by the layer it overrides") + +// ErrEmptyProfileValue marks an override that empties a value its declarer +// marked as supplied per profile (ProfileValueMarker). An empty value is not a +// value: accepted, it discharges the requirement and the group is delivered with +// the key reading as the empty string every reader of a missing key gets — +// exactly the silent, until-runtime failure the marker exists to refuse, with +// the declaration that was supposed to prevent it still on the page. A consumer +// with no value for the key leaves it to whoever has one; a key for which +// nothing is a legitimate value is declared with a default by the group's +// author, not with the marker. +var ErrEmptyProfileValue = errors.New("configuration value declared as supplied per profile is overridden with an empty value") // ProfileRequirement is one configuration value a group declares as supplied per // profile (ProfileValueMarker) that the selected profile did not supply. @@ -369,12 +383,7 @@ func (overlay *profileOverlay) add(layer string, infos []*basev0.ConfigurationIn // is about a key that hides inside a group the base already declares, where // absence is indistinguishable from an empty value. func (overlay *profileOverlay) declares(info *basev0.ConfigurationInformation, key string) bool { - for _, existing := range info.GetConfigurationValues() { - if resources.Match(existing.GetKey(), key) { - return true - } - } - return false + return findConfigurationValue(info, key) != nil } func (overlay *profileOverlay) find(name string) *basev0.ConfigurationInformation { @@ -388,12 +397,31 @@ func (overlay *profileOverlay) find(name string) *basev0.ConfigurationInformatio func (overlay *profileOverlay) set(layer string, info *basev0.ConfigurationInformation, value *basev0.ConfigurationValue) { overlay.writtenIn[profileValueKey(info.GetName(), value.GetKey())] = layer + setConfigurationValue(info, value) +} + +// findConfigurationValue returns the value a group carries for key, matched the +// way every other configuration lookup matches (resources.Match: case +// insensitively, with "-" and "_" equivalent), or nil when the group does not +// declare it. It is what makes "the layer below declares this key" one question +// with one answer, wherever a layer is overlaid on another. +func findConfigurationValue(info *basev0.ConfigurationInformation, key string) *basev0.ConfigurationValue { + for _, existing := range info.GetConfigurationValues() { + if resources.Match(existing.GetKey(), key) { + return existing + } + } + return nil +} + +// setConfigurationValue overlays one value onto a group, per key: the +// declaration replaces the WHOLE value of a key the group already carries, not +// only its text — a layer that turns a plaintext default into a secret, or into +// an assembled template, says so here and a merge of the two would deliver half +// of each — and is appended when the group does not carry it. +func setConfigurationValue(info *basev0.ConfigurationInformation, value *basev0.ConfigurationValue) { for index, existing := range info.ConfigurationValues { if resources.Match(existing.GetKey(), value.GetKey()) { - // The derived layer's declaration replaces the whole value, not only - // its text: a profile that turns a plaintext default into a secret, - // or into an assembled template, says so here and a merge of the two - // would deliver half of each. info.ConfigurationValues[index] = value return } @@ -401,6 +429,13 @@ func (overlay *profileOverlay) set(layer string, info *basev0.ConfigurationInfor info.ConfigurationValues = append(info.ConfigurationValues, value) } +// configurationValueSuppliesNothing reports whether a value supplies nothing at +// all: no text, and no template to assemble one. A value that supplies nothing +// does not discharge a ProfileValueMarker — see ErrEmptyProfileValue. +func configurationValueSuppliesNothing(value *basev0.ConfigurationValue) bool { + return value.GetValue() == "" && value.GetTemplate() == nil +} + // unsupplied lists what still carries the marker once every layer is in. The // marker is looked for anywhere in a value, not only as the whole of it, so a // partially written declaration ("https://${profile}/token") is owed too rather diff --git a/configurations/workspace_override_test.go b/configurations/workspace_override_test.go new file mode 100644 index 000000000..e4e26f307 --- /dev/null +++ b/configurations/workspace_override_test.go @@ -0,0 +1,379 @@ +package configurations_test + +import ( + "context" + "path/filepath" + "testing" + + "github.com/codefly-dev/core/configurations" + basev0 "github.com/codefly-dev/core/generated/go/codefly/base/v0" + "github.com/codefly-dev/core/resources" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// composedGroupOverriddenBy writes the two-workspace fixture every test in this +// file reads: a composed host module shipping one group — a default, and two +// values it declares each profile must supply — and a consuming solution whose +// own file for that group holds whatever the test is about. It returns the +// solution directory. +// +// The module declares the group, so the module is where the group's keys and its +// per-profile requirements are declared; the solution's file is an override of +// that declaration, never a replacement of it. +func composedGroupOverriddenBy(t *testing.T, override string) string { + t.Helper() + root := t.TempDir() + + writeConfigurationFile(t, root, "solution/workspace.codefly.yaml", `name: solution +layout: modules +modules: + - name: host + path: ../host +`) + if override != "" { + writeConfigurationFile(t, root, "solution/configurations/local/app-config.env", override) + } + + writeConfigurationFile(t, root, "host/module.codefly.yaml", `kind: module +name: host +services: + - name: telemetry +`) + writeConfigurationFile(t, root, "host/configurations/local/app-config.env", + "CONFIG_DIR=/etc/app\nCONFIG_FILE=${profile}\nCONFIG_MODE=${profile}\n") + writeConfigurationFile(t, root, "host/services/telemetry/service.codefly.yaml", `kind: service +name: telemetry +version: 0.0.0 +agent: + kind: runtime::service + name: go-grpc + version: 0.0.1 + publisher: codefly.ai +`) + return filepath.Join(root, "solution") +} + +func loadWorkspace(t *testing.T, ctx context.Context, dir string) *resources.Workspace { + t.Helper() + workspace, err := resources.LoadWorkspaceFromDir(ctx, dir) + require.NoError(t, err) + return workspace +} + +func groupValues(t *testing.T, infos []*basev0.ConfigurationInformation, group string) map[string]string { + t.Helper() + values := make(map[string]string) + for _, info := range infos { + if info.GetName() != group { + continue + } + for _, value := range info.GetConfigurationValues() { + values[value.GetKey()] = value.GetValue() + } + } + return values +} + +// A solution that overrides one key of a composed module's group overrides THAT +// KEY. The module keeps supplying the keys the solution never mentioned — both +// its default and, decisively, the declaration that a value is supplied per +// profile, which is reported as owed and fails the load. Replacing the group +// whole discarded the default, discarded the requirement with it, and so +// returned a clean load for a composition that is missing a value no profile +// ever supplies. +func TestAWorkspaceOverrideOfAComposedGroupOverlaysPerKey(t *testing.T) { + ctx := context.Background() + solution := composedGroupOverriddenBy(t, "CONFIG_FILE=/etc/app/solution.yaml\n") + workspace := loadWorkspace(t, ctx, solution) + + provided, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, resources.LocalEnvironment()) + require.NoError(t, err) + + assert.Equal(t, map[string]string{ + "CONFIG_DIR": "/etc/app", + "CONFIG_FILE": "/etc/app/solution.yaml", + "CONFIG_MODE": configurations.ProfileValueMarker, + }, groupValues(t, provided.Infos, "app-config"), + "the overridden key is the solution's, every other key is still the module's") + + owed := configurations.StillUnsupplied(provided.Unsupplied, func(string) []*basev0.ConfigurationInformation { + return provided.Infos + }) + require.Len(t, owed, 1, "the override discharged one of the module's two requirements, and only one") + assert.Equal(t, "app-config", owed[0].Group) + assert.Equal(t, "CONFIG_MODE", owed[0].Key) + + loader, err := configurations.NewConfigurationLocalReader(ctx, workspace) + require.NoError(t, err) + err = loader.Load(ctx, resources.LocalEnvironment()) + require.Error(t, err, "a value no profile supplies fails the load, override or no override") + assert.ErrorContains(t, err, "app-config/CONFIG_MODE") +} + +// The group is the module's, and overriding keys of it does not make it the +// solution's. It stays composed, so it reaches the services that declared it as +// a dependency rather than every service of the composition. +// +// This is the half of the defect that widened delivery while the other half +// narrowed contents. It matters more now than it did before the overlay: a +// run-wide injection of an overridden group would inject the MODULE's keys — +// CONFIG_DIR here, which the solution never wrote — into every service of the +// composition. A solution that wants a group of its own in every service +// declares one under its own name, where no module's keys ride along; an +// operator who wants one value everywhere uses --set, which is attributed to the +// run and stays composition-root. +func TestAnOverriddenComposedGroupStaysComposedAndKeepsItsDeliveryScoped(t *testing.T) { + ctx := context.Background() + solution := composedGroupOverriddenBy(t, + "CONFIG_FILE=/etc/app/solution.yaml\nCONFIG_MODE=solution\n") + workspace := loadWorkspace(t, ctx, solution) + + provided, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, resources.LocalEnvironment()) + require.NoError(t, err) + assert.Equal(t, map[string]string{"app-config": "host"}, provided.ComposedBy, + "the module that declared the group is still the group's provider") + assert.Empty(t, provided.Ambiguous) + + loader, err := configurations.NewConfigurationLocalReader(ctx, workspace) + require.NoError(t, err) + require.NoError(t, loader.Load(ctx, resources.LocalEnvironment()), + "both per-profile values are supplied now, one by the module's own file and one by the override") + assert.Empty(t, loader.CompositionRootWorkspaceConfigurationNames(), + "an overridden composed group is not the composition root's own") + + manager, err := configurations.NewManager(ctx, workspace) + require.NoError(t, err) + manager.WithLoader(loader) + require.NoError(t, manager.Load(ctx, resources.LocalEnvironment())) + + rootConfs, err := manager.GetCompositionRootWorkspaceConfigurations(ctx) + require.NoError(t, err) + assert.Empty(t, rootConfs, "nothing is injected run-wide") + + confs, err := manager.GetWorkspaceDependenciesConfigurations(ctx, "app-config") + require.NoError(t, err) + require.Len(t, confs, 1, "and a consumer that declares the group still receives it") + for key, expected := range map[string]string{ + "CONFIG_DIR": "/etc/app", + "CONFIG_FILE": "/etc/app/solution.yaml", + "CONFIG_MODE": "solution", + } { + value, err := resources.GetConfigurationValue(ctx, confs[0], "app-config", key) + require.NoError(t, err) + assert.Equal(t, expected, value, key) + } +} + +// An empty value is not a value. A solution that overrides a key the module +// declared as supplied per profile with nothing at all is refused NAMING the +// key: accepted, it would discharge the requirement and deliver the group with +// the key reading as the empty string, which is the until-runtime failure the +// declaration exists to refuse — and it would do it with the declaration still +// on the page. +func TestAWorkspaceOverrideCannotEmptyAValueTheComposedModuleDeclaresPerProfile(t *testing.T) { + ctx := context.Background() + solution := composedGroupOverriddenBy(t, "CONFIG_FILE=\n") + workspace := loadWorkspace(t, ctx, solution) + + _, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, resources.LocalEnvironment()) + require.Error(t, err) + require.ErrorIs(t, err, configurations.ErrEmptyProfileValue) + assert.ErrorContains(t, err, "app-config/CONFIG_FILE") + assert.ErrorContains(t, err, `"host"`, "the diagnostic names the module whose declaration is being emptied") + + loader, err := configurations.NewConfigurationLocalReader(ctx, workspace) + require.NoError(t, err) + require.ErrorIs(t, loader.Load(ctx, resources.LocalEnvironment()), configurations.ErrEmptyProfileValue, + "the load fails too, so no render can reach a workload from this composition") +} + +// The module's group is the declared SET of its keys, as a base profile is for +// the profiles derived from it. A key only the solution carries is refused where +// it is written: the group is delivered to the module's services, which read the +// keys the module declared, so a key the declaration never mentions is a value +// nothing reads — and the remedy is named, since a solution that wants a key of +// its own has a group of its own to put it in. +func TestAWorkspaceOverrideCannotIntroduceAKeyTheComposedGroupDoesNotDeclare(t *testing.T) { + ctx := context.Background() + solution := composedGroupOverriddenBy(t, "CONFIG_FILE=/etc/app/solution.yaml\nCONFIG_EXTRA=solution\n") + workspace := loadWorkspace(t, ctx, solution) + + _, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, resources.LocalEnvironment()) + require.Error(t, err) + require.ErrorIs(t, err, configurations.ErrUndeclaredProfileKey) + assert.ErrorContains(t, err, "app-config/CONFIG_EXTRA") + assert.ErrorContains(t, err, configurations.ProfileValueMarker, "the diagnostic names the remedy") +} + +// Keys are matched the way every other configuration lookup matches them — case +// insensitively, with "-" and "_" equivalent — so an override is an override of +// the module's key however the solution's file spells it, rather than a second +// key beside it that the undeclared-key rule then refuses. The declaration +// replaces the whole value, its spelling included, exactly as a derived +// profile's does; nothing downstream reads the difference, since every lookup +// matches either spelling and the delivered environment variable is normalized +// (resources.NameToKey). +func TestAWorkspaceOverrideMatchesTheComposedGroupsKeysTheWayEveryLookupDoes(t *testing.T) { + ctx := context.Background() + solution := composedGroupOverriddenBy(t, "config-file=/etc/app/solution.yaml\nconfig-mode=solution\n") + workspace := loadWorkspace(t, ctx, solution) + + provided, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, resources.LocalEnvironment()) + require.NoError(t, err) + assert.Equal(t, map[string]string{ + "CONFIG_DIR": "/etc/app", + "config-file": "/etc/app/solution.yaml", + "config-mode": "solution", + }, groupValues(t, provided.Infos, "app-config"), + "one key per declaration of the module's group, carrying the solution's value") + assert.Empty(t, configurations.StillUnsupplied(provided.Unsupplied, func(string) []*basev0.ConfigurationInformation { + return provided.Infos + }), "the marker is discharged through the match, not left standing beside a second key") + + loader, err := configurations.NewConfigurationLocalReader(ctx, workspace) + require.NoError(t, err) + require.NoError(t, loader.Load(ctx, resources.LocalEnvironment())) + conf, err := resources.FindWorkspaceConfiguration(ctx, loader.Configurations(), "app-config") + require.NoError(t, err) + value, err := resources.GetConfigurationValue(ctx, conf, "app-config", "CONFIG_FILE") + require.NoError(t, err) + assert.Equal(t, "/etc/app/solution.yaml", value, "a reader spelling the key the module's way still finds it") + variables, err := resources.ConfigurationAsEnvironmentVariables(conf, "local", false) + require.NoError(t, err) + var delivered []string + for _, variable := range variables { + delivered = append(delivered, variable.Key) + } + assert.ElementsMatch(t, + []string{"CODEFLY__WORKSPACE_CONFIGURATION__APP_CONFIG__CONFIG_DIR", + "CODEFLY__WORKSPACE_CONFIGURATION__APP_CONFIG__CONFIG_FILE", + "CODEFLY__WORKSPACE_CONFIGURATION__APP_CONFIG__CONFIG_MODE"}, + delivered, "and the workload receives the keys under one normalized name either way") +} + +// Two composed modules that disagree on a group are ambiguous, and the +// diagnostic's own remedy is a declaration in the consuming workspace. So a +// workspace that has one resolves the ambiguity rather than being reported it: +// there is no single group to overlay onto — that is precisely what the two +// modules disagree about — and the workspace's declaration stands whole, as the +// composition root's own, reaching every service of the composition. +func TestAWorkspaceDeclarationResolvesAnAmbiguityBetweenComposedModules(t *testing.T) { + ctx := context.Background() + root := t.TempDir() + + writeConfigurationFile(t, root, "solution/workspace.codefly.yaml", `name: solution +layout: modules +modules: + - name: host-a + path: ../host-a + - name: host-b + path: ../host-b +`) + writeConfigurationFile(t, root, "solution/configurations/local/legal.env", "LEGAL_URL=solution-legal\n") + for _, host := range []string{"host-a", "host-b"} { + writeConfigurationFile(t, root, host+"/module.codefly.yaml", "kind: module\nname: "+host+"\nservices: []\n") + writeConfigurationFile(t, root, host+"/configurations/local/legal.env", "LEGAL_URL="+host+"-legal\n") + } + + workspace := loadWorkspace(t, ctx, filepath.Join(root, "solution")) + provided, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, resources.LocalEnvironment()) + require.NoError(t, err) + assert.Empty(t, provided.Ambiguous, "the workspace's declaration is the remedy the diagnostic names") + assert.Empty(t, provided.ComposedBy, "with no single module definition to overlay, the declaration stands whole") + assert.Equal(t, map[string]string{"LEGAL_URL": "solution-legal"}, groupValues(t, provided.Infos, "legal")) + + loader, err := configurations.NewConfigurationLocalReader(ctx, workspace) + require.NoError(t, err) + require.NoError(t, loader.Load(ctx, resources.LocalEnvironment())) + assert.Equal(t, []string{"legal"}, loader.CompositionRootWorkspaceConfigurationNames(), + "it is the composition root's own group, injected run-wide") +} + +// A structured document has no keys to overlay, so a solution's document +// replaces the module's whole — as a derived profile's does. The declaration +// that each profile supplies the document is still a declaration: an empty +// document supplies nothing and is refused naming the group, and a boundary that +// turns a document into key/value pairs is a conflict rather than a silent +// choice between the two shapes. +func TestAWorkspaceOverrideOfAComposedDocumentReplacesItWhole(t *testing.T) { + ctx := context.Background() + + fixture := func(t *testing.T, override string, overrideName string) *resources.Workspace { + t.Helper() + root := t.TempDir() + writeConfigurationFile(t, root, "solution/workspace.codefly.yaml", `name: solution +layout: modules +modules: + - name: host + path: ../host +`) + writeConfigurationFile(t, root, "solution/configurations/local/"+overrideName, override) + writeConfigurationFile(t, root, "host/module.codefly.yaml", "kind: module\nname: host\nservices: []\n") + writeConfigurationFile(t, root, "host/configurations/local/policy.yaml", "mode: ${profile}\n") + return loadWorkspace(t, ctx, filepath.Join(root, "solution")) + } + + t.Run("supplied", func(t *testing.T) { + provided, err := configurations.ReadWorkspaceConfigurations(ctx, fixture(t, "mode: solution\n", "policy.yaml"), resources.LocalEnvironment()) + require.NoError(t, err) + assert.Equal(t, map[string]string{"policy": "host"}, provided.ComposedBy) + var content string + for _, info := range provided.Infos { + if info.GetName() == "policy" { + content = string(info.GetData().GetContent()) + } + } + assert.Equal(t, "mode: solution\n", content) + }) + + t.Run("emptied", func(t *testing.T) { + _, err := configurations.ReadWorkspaceConfigurations(ctx, fixture(t, "", "policy.yaml"), resources.LocalEnvironment()) + require.ErrorIs(t, err, configurations.ErrEmptyProfileValue) + assert.ErrorContains(t, err, "policy") + }) + + t.Run("shape conflict", func(t *testing.T) { + _, err := configurations.ReadWorkspaceConfigurations(ctx, fixture(t, "MODE=solution\n", "policy.env"), resources.LocalEnvironment()) + require.ErrorIs(t, err, configurations.ErrConfigurationConflict) + assert.ErrorContains(t, err, "policy") + }) +} + +// The overlay is the consuming workspace's own declaration over a composed +// module's group. A workspace-level declaration a COMPOSED WORKSPACE also +// carries — the product model — still replaces a module's group of that name +// whole, deliberately: an inherited declaration is not the product's to amend, +// and overlaying the product's own onto the module's group where a composed +// workspace declares the name too would rank a module default above the +// inherited declaration it is supposed to sit below. Deciding the product +// model's precedence per key is a separate change; this test is here so the +// boundary of this one is visible rather than discovered. +func TestAnInheritedGroupStillReplacesAComposedModuleGroupWhole(t *testing.T) { + ctx := context.Background() + root := t.TempDir() + + writeConfigurationFile(t, root, "product/workspace.codefly.yaml", `name: product +layout: modules +workspaces: + - name: platform-core + path: ../core +modules: + - name: host + path: ../host +`) + writeConfigurationFile(t, root, "core/workspace.codefly.yaml", "name: platform-core\nlayout: modules\n") + writeConfigurationFile(t, root, "core/configurations/local/app-config.env", "CONFIG_DIR=/platform\n") + writeConfigurationFile(t, root, "host/module.codefly.yaml", "kind: module\nname: host\nservices: []\n") + writeConfigurationFile(t, root, "host/configurations/local/app-config.env", + "CONFIG_DIR=/etc/app\nCONFIG_FILE=/etc/app/host.yaml\n") + + workspace := loadWorkspace(t, ctx, filepath.Join(root, "product")) + provided, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, resources.LocalEnvironment()) + require.NoError(t, err) + assert.Equal(t, map[string]string{"CONFIG_DIR": "/platform"}, + groupValues(t, provided.Infos, "app-config"), + "the inherited declaration stands whole; the module's CONFIG_FILE is not overlaid under it") + assert.Empty(t, provided.ComposedBy, "and the group is the workspace level's, not the module's") +} diff --git a/docs/workspace-composition.md b/docs/workspace-composition.md index cf45d1d9f..8f020251c 100644 --- a/docs/workspace-composition.md +++ b/docs/workspace-composition.md @@ -39,6 +39,70 @@ workspace groups fail unless the product supplies that group. Existing module configuration fallback remains below workspace-owned configuration. Imported environments never override the product's deployment target. +### Overriding a composed module's group + +A composed module brings its `configurations//*` into the consuming +workspace's configuration space, so the module's services satisfy their +workspace-configuration-dependencies from the solution alone. A solution +overrides one of those groups by declaring a group of the same name — and it +overrides it **per key**: + +``` +host/configurations/local/app-config.env solution/configurations/local/app-config.env + CONFIG_DIR=/etc/app CONFIG_FILE=/etc/app/solution.yaml + CONFIG_FILE=${profile} + CONFIG_MODE=${profile} +``` + +`CONFIG_FILE` is the solution's. `CONFIG_DIR` is still the module's default, and +`CONFIG_MODE` is still a value each profile must supply — so this composition is +refused, naming `app-config/CONFIG_MODE`, instead of running without it. A +solution overriding one key is never required to restate the group. + +**The module's group is the declared set of the group's keys**, exactly as a base +profile is for the profiles derived from it, and the same two rules follow: + +- a key only the solution carries is refused, naming it. The group is delivered + to the module's services, which read the keys the module declared; a key its + declaration never mentions is a value nothing reads. A solution that needs a + key of its own declares a **group** of its own name, which reaches every + service of the composition. +- a `${profile}` the override does not discharge is still owed, and an override + that discharges one with an **empty value** is refused naming the key. An empty + value is not a value: accepted, it would deliver the key as the empty string + every reader of a missing key gets, which is what the declaration exists to + refuse. A key for which nothing is a legitimate value is declared with a + default by the group's author, not with the marker. + +Keys are matched the way every other configuration lookup matches them — case +insensitively, with `-` and `_` equivalent. A structured document (a `.yaml` +group) has no keys to overlay, so the solution's document replaces the module's +whole; an empty one does not discharge a document declared per profile, and a +boundary that turns a document into key/value pairs is a conflict. + +**An overridden group stays composed.** Its provider is still the module, so it +reaches the services that declared it as a dependency rather than every service +of the composition. Overriding a key of someone else's group is not a statement +about every service in the run — and a run-wide injection of an overridden group +would carry the module's own keys, the ones the solution never wrote, into every +service of it. Run-wide delivery has two ways to be asked for, and both say so: +a group under a name no composed module provides is the composition root's own, +and an operator's `--set` is attributed to the run, which stays composition-root +even on a name a composed module provides. + +When two composed modules define one name differently, the name is ambiguous and +unavailable until the solution declares it; a solution that does declare it +resolves the ambiguity rather than being reported it — there is no single group +to overlay onto, so its declaration stands whole as the composition root's own. + +This per-key overlay is the consuming workspace's **own** declaration over a +composed module's group. A workspace-level group an imported workspace also +carries — the product model above — still replaces a module's group of that name +whole: an imported declaration is not the product's to amend, and overlaying the +product's own onto the module's group where an imported workspace declares the +name too would rank a module default above the imported declaration it sits +below. Deciding the product model's precedence per key is a separate change. + ### Configuration profile chains An environment reads `configurations//` (and each service's From 2d7fe60237dda74c2f616b7dc0449bb76de36780 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Sun, 4 Oct 2026 11:38:34 -0400 Subject: [PATCH 2/6] chore: release 0.9.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The manifest is the release: version-tag.yml cuts the tag from it once the suite is green on main, and declines to move a tag that already exists. The head declared 0.7.1 — the version its own parent published — so merging this fix would have published nothing and left the consumer waiting on it (codefly-dev/cli#883) gated on a version that does not exist. 0.9.1 is the next unused version above main's v0.9.0. check_version_tag.sh passes: the manifest may run ahead of the published tags, and only behind is a bug. Co-Authored-By: Claude Opus 5 (1M context) --- version/info.codefly.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version/info.codefly.yaml b/version/info.codefly.yaml index 71fb34006..65876c1f7 100644 --- a/version/info.codefly.yaml +++ b/version/info.codefly.yaml @@ -1 +1 @@ -version: 0.9.0 +version: 0.9.1 From f001d9db4c5bc1329f12cdd72283b02ab09aeee6 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Sun, 4 Oct 2026 11:38:47 -0400 Subject: [PATCH 3/6] fix(configurations): one key declared twice cannot discharge a per-profile value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The empty-value refusal this PR added read requiredness from the group it was building, not from the declaration it was overlaying. A group may declare one key twice — the parsers append every declaration they read, from a repeated line or from two files consolidated into one group name — so an override of CONFIG_FILE=${profile} reading CONFIG_FILE=/etc/app/solution.yaml CONFIG_FILE= discharged the marker with the first entry, found no marker left to protect on the second, and delivered CONFIG_FILE as the empty string with nothing owed and a clean Load. Executed against the parent of this commit: delivered CONFIG_FILE="", still owed 0, Load error nil — the exact failure the marker exists to refuse, from a file that names the key twice on the page. Reported by Codex (gpt-6-astra) round one on #694, finding 2. Both halves are closed. A duplicate canonical key — matched the way every lookup matches, so CONFIG_FILE and config-file are one key — is refused on whichever side of the boundary declares it, before any rule reads a value: there is no single value to overlay or to overlay onto, and which declaration would win is a fact about line order rather than about what either author said. Refusing it on the module's side too names the only author who can fix it, instead of telling a solution that supplied the key that the key is still owed. And every rule now reads the module's declaration as it stands before the overlay, so no guard can be answered by an earlier entry's own replacement. This narrows one case that loaded before: a module declaring one key twice in a group a solution overrides. Replacing the group whole made that duplicate invisible; overlaying onto it has to choose a declaration, so it is named rather than chosen. Also qualifies three claims the same review found overstated (finding 6, minor): the empty-value refusal is this boundary's own and profile derivation does not carry it, so the docs no longer claim the two layers share both rules; and the ComposedBy and package-doc statements that an overridden name stays composed now name the inherited-workspace exception they always had. Co-Authored-By: Claude Opus 5 (1M context) --- configurations/doc.go | 17 ++-- configurations/local_reader.go | 47 ++++++++-- configurations/profile.go | 23 +++++ configurations/workspace_override_test.go | 101 +++++++++++++++++++++- docs/workspace-composition.md | 27 ++++-- 5 files changed, 189 insertions(+), 26 deletions(-) diff --git a/configurations/doc.go b/configurations/doc.go index f0dfe1d57..ffe18ca9c 100644 --- a/configurations/doc.go +++ b/configurations/doc.go @@ -116,13 +116,16 @@ // for that run-wide injection, resolved and endpoint-interpolated like // GetWorkspaceConfigurations. The root-provided and per-dependency composed-module // sets are disjoint by name: a name a composed module provides stays in the -// composed set even when the root declares it too, because the root's values are -// overlaid onto the module's group PER KEY (composeModuleWorkspaceConfigurations) -// — the group is still the module's, so it is still delivered to the services -// that declared it, and a run-wide injection of it would carry the module's own -// keys into every service of the composition. So the root fills what a composed -// module leaves unset without shadowing a name only the module provides, and -// without widening that name's delivery. An invocation-scoped override is the +// composed set even when the root declares it as its OWN, because the root's +// values are overlaid onto the module's group PER KEY +// (composeModuleWorkspaceConfigurations) — the group is still the module's, so it +// is still delivered to the services that declared it, and a run-wide injection +// of it would carry the module's own keys into every service of the composition. +// So the root fills what a composed module leaves unset without shadowing a name +// only the module provides, and without widening that name's delivery. A name an +// imported workspace also declares is the exception the overlay does not cover: +// an imported declaration is not the product's to amend, so it still replaces a +// composed module's group of that name whole and is classified as the root's. An invocation-scoped override is the // run supplying a value, so it is composition-root even when it lands on a name a // composed module also provides. // diff --git a/configurations/local_reader.go b/configurations/local_reader.go index adbef6606..629af2dfb 100644 --- a/configurations/local_reader.go +++ b/configurations/local_reader.go @@ -94,12 +94,16 @@ type WorkspaceConfigurations struct { // carrying the module's group with the workspace's values overlaid on it. Infos []*basev0.ConfigurationInformation // ComposedBy names, for each configuration a composed module contributes, - // the module that provides it. A name the consuming workspace also declares - // stays in it: the workspace's values are overlaid onto the module's group - // per key, so the group is still the module's and its delivery stays scoped - // to the services that declared it. A name absent from it is the - // workspace's own alone — no composed module offers it, or the modules that - // do disagree and the workspace's declaration resolves the ambiguity. + // the module that provides it. A name the consuming workspace declares as + // its OWN stays in it: the workspace's values are overlaid onto the module's + // group per key, so the group is still the module's and its delivery stays + // scoped to the services that declared it. A name absent from it is the + // workspace's own alone, which happens three ways: no composed module offers + // it; the modules that do disagree and the workspace's declaration resolves + // the ambiguity; or an imported workspace declares it too, which still + // replaces a module's group of that name whole rather than overlaying onto + // it (readOwnedWorkspaceConfigurations, and the closing paragraph of + // docs/workspace-composition.md's "Overriding a composed module's group"). ComposedBy map[string]string // Ambiguous holds the names two composed modules define differently, mapped // to the diagnostic naming both providers. They are absent from Infos: a @@ -686,6 +690,23 @@ func composeModuleWorkspaceConfigurations( // replaces the module's whole — as a derived profile's does — and a boundary // that would turn a document into key/value pairs, or the reverse, is a conflict // rather than a silent choice between them. +// +// Both rules are read from the module's declaration as it stands BEFORE the +// overlay, never from the group being built: a group may declare one key twice +// (the parsers append every declaration they read), and reading requiredness +// from the group under construction would let a second declaration of a key be +// checked against the first declaration's own replacement. An override of +// CONFIG_FILE=${profile} reading +// +// CONFIG_FILE=/etc/app/solution.yaml +// CONFIG_FILE= +// +// would then discharge the marker with the first entry, find no marker left for +// the second, and deliver the key as the empty string the marker exists to +// refuse. Either side declaring one key twice is refused outright for the same +// reason the rest of this boundary refuses ambiguity: there is no single value +// to overlay or to overlay onto, and which declaration would win is a fact +// about line order rather than about what either author said. func overlayWorkspaceConfigurationOverride( base *basev0.ConfigurationInformation, override *basev0.ConfigurationInformation, @@ -707,13 +728,21 @@ func overlayWorkspaceConfigurationOverride( overlaid.Data = override.GetData() return overlaid, nil } + if first, second, duplicated := duplicateConfigurationKey(base); duplicated { + return nil, fmt.Errorf("composed module %q declares %s/%s and %s/%s, one key under two spellings, in the group the consuming workspace overrides; declare it once: %w", + module, base.GetName(), first, base.GetName(), second, ErrConfigurationConflict) + } + if first, second, duplicated := duplicateConfigurationKey(override); duplicated { + return nil, fmt.Errorf("the consuming workspace declares %s/%s and %s/%s, one key under two spellings, overriding the group composed module %q provides; declare it once: %w", + override.GetName(), first, override.GetName(), second, module, ErrConfigurationConflict) + } for _, value := range override.GetConfigurationValues() { - existing := findConfigurationValue(overlaid, value.GetKey()) - if existing == nil { + declared := findConfigurationValue(base, value.GetKey()) + if declared == nil { return nil, fmt.Errorf("the consuming workspace overrides %s/%s, a key composed module %q does not declare in the group it provides; declare it there (as %s when each profile supplies its own), or declare a group of the workspace's own name: %w", override.GetName(), value.GetKey(), module, ProfileValueMarker, ErrUndeclaredProfileKey) } - if ValueDeclaredPerProfile(existing) && configurationValueSuppliesNothing(value) { + if ValueDeclaredPerProfile(declared) && configurationValueSuppliesNothing(value) { return nil, fmt.Errorf("%s/%s is supplied per profile by composed module %q, and the consuming workspace supplies an empty value: %w", override.GetName(), value.GetKey(), module, ErrEmptyProfileValue) } diff --git a/configurations/profile.go b/configurations/profile.go index de7eae643..600ba2db7 100644 --- a/configurations/profile.go +++ b/configurations/profile.go @@ -429,6 +429,29 @@ func setConfigurationValue(info *basev0.ConfigurationInformation, value *basev0. info.ConfigurationValues = append(info.ConfigurationValues, value) } +// duplicateConfigurationKey returns the two spellings under which a group +// declares one key twice — matched the way every lookup matches, so CONFIG_FILE +// and config-file are one key — and whether it does. +// +// A group that carries one key twice has no single value for it: the parsers +// append every declaration they read (a repeated line in one .env file, or two +// files contributing to one group name), findConfigurationValue answers with the +// first and setConfigurationValue overlays the first, so which declaration a +// reader gets depends on where it is read from. Where that group is overlaid +// onto someone else's, the duplicate is refused rather than resolved by +// position — see overlayWorkspaceConfigurationOverride. +func duplicateConfigurationKey(info *basev0.ConfigurationInformation) (string, string, bool) { + values := info.GetConfigurationValues() + for index, value := range values { + for _, candidate := range values[index+1:] { + if resources.Match(value.GetKey(), candidate.GetKey()) { + return value.GetKey(), candidate.GetKey(), true + } + } + } + return "", "", false +} + // configurationValueSuppliesNothing reports whether a value supplies nothing at // all: no text, and no template to assemble one. A value that supplies nothing // does not discharge a ProfileValueMarker — see ErrEmptyProfileValue. diff --git a/configurations/workspace_override_test.go b/configurations/workspace_override_test.go index e4e26f307..78fee74df 100644 --- a/configurations/workspace_override_test.go +++ b/configurations/workspace_override_test.go @@ -23,6 +23,17 @@ import ( // per-profile requirements are declared; the solution's file is an override of // that declaration, never a replacement of it. func composedGroupOverriddenBy(t *testing.T, override string) string { + t.Helper() + return composedGroupDeclaredAsOverriddenBy(t, + "CONFIG_DIR=/etc/app\nCONFIG_FILE=${profile}\nCONFIG_MODE=${profile}\n", override, "") +} + +// composedGroupDeclaredAsOverriddenBy is composedGroupOverriddenBy with the +// module's own declaration of the group under the test's control, and with a +// second file of the solution's contributing to the same group name — both of +// which a test about one key declared twice needs, since a duplicate arrives +// either from a repeated line or from two files consolidated into one group. +func composedGroupDeclaredAsOverriddenBy(t *testing.T, declaration, override, overrideSecret string) string { t.Helper() root := t.TempDir() @@ -35,14 +46,16 @@ modules: if override != "" { writeConfigurationFile(t, root, "solution/configurations/local/app-config.env", override) } + if overrideSecret != "" { + writeConfigurationFile(t, root, "solution/configurations/local/app-config.secret.env", overrideSecret) + } writeConfigurationFile(t, root, "host/module.codefly.yaml", `kind: module name: host services: - name: telemetry `) - writeConfigurationFile(t, root, "host/configurations/local/app-config.env", - "CONFIG_DIR=/etc/app\nCONFIG_FILE=${profile}\nCONFIG_MODE=${profile}\n") + writeConfigurationFile(t, root, "host/configurations/local/app-config.env", declaration) writeConfigurationFile(t, root, "host/services/telemetry/service.codefly.yaml", `kind: service name: telemetry version: 0.0.0 @@ -189,6 +202,90 @@ func TestAWorkspaceOverrideCannotEmptyAValueTheComposedModuleDeclaresPerProfile( "the load fails too, so no render can reach a workload from this composition") } +// One key declared twice has no single value, and at this boundary it is refused +// rather than resolved by position. It is how the empty-value refusal above was +// bypassed: an override reading +// +// CONFIG_FILE=/etc/app/solution.yaml +// CONFIG_FILE= +// +// discharged the marker with the first entry, and the second then found no +// marker left to protect — because requiredness was read from the group being +// built instead of from the module's declaration — so the group was delivered +// with the key reading as the empty string, the exact failure the marker +// refuses, from a file that names the key twice on the page. +// +// Both halves of that are closed here: a duplicate is refused before any rule +// reads a value, and every rule reads the module's declaration as it stands +// before the overlay. A duplicate reaches this function from a repeated line or +// from two of the solution's files consolidated into one group — the parsers +// append every declaration they read — and neither spelling of the key has to +// match the other's, since one key is one key however it is written. +func TestAWorkspaceOverrideDeclaringOneKeyTwiceIsRefusedRatherThanResolvedByOrder(t *testing.T) { + ctx := context.Background() + for _, test := range []struct { + name string + override string + secret string + }{ + { + name: "the same spelling twice, the second emptying the first", + override: "CONFIG_FILE=/etc/app/solution.yaml\nCONFIG_FILE=\nCONFIG_MODE=solution\n", + }, + { + name: "two spellings of one key, matched the way every lookup matches", + override: "CONFIG_FILE=/etc/app/solution.yaml\nconfig-file=\nCONFIG_MODE=solution\n", + }, + { + name: "two spellings carrying two real values, with nothing to choose between them", + override: "CONFIG_FILE=/etc/app/solution.yaml\nconfig_file=/etc/app/other.yaml\nCONFIG_MODE=solution\n", + }, + { + name: "two of the solution's files consolidated into one group", + override: "CONFIG_FILE=/etc/app/solution.yaml\nCONFIG_MODE=solution\n", + secret: "CONFIG_FILE=\n", + }, + } { + t.Run(test.name, func(t *testing.T) { + solution := composedGroupDeclaredAsOverriddenBy(t, + "CONFIG_DIR=/etc/app\nCONFIG_FILE=${profile}\nCONFIG_MODE=${profile}\n", test.override, test.secret) + workspace := loadWorkspace(t, ctx, solution) + + _, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, resources.LocalEnvironment()) + require.Error(t, err, "a key declared twice is never resolved by which line came last") + require.ErrorIs(t, err, configurations.ErrConfigurationConflict) + assert.ErrorContains(t, err, "app-config/CONFIG_FILE", "the diagnostic names the key, under the spelling the file used") + + loader, err := configurations.NewConfigurationLocalReader(ctx, workspace) + require.NoError(t, err) + require.Error(t, loader.Load(ctx, resources.LocalEnvironment()), + "the load fails rather than discharging the module's requirement with an empty value") + }) + } +} + +// The same rule on the module's side of the boundary, where the duplicate is not +// the solution's to fix. Replacing the group whole made it invisible: the +// module's group was discarded, duplicate and all. Overlaying onto it has to +// pick a declaration to overlay — and a solution that supplies the key would +// otherwise be told the key is still owed, because the second declaration still +// carries the marker. Naming the module's duplicate says what is actually +// wrong, to the only author who can fix it. +func TestAComposedModuleDeclaringOneKeyTwiceIsRefusedWhereItIsOverridden(t *testing.T) { + ctx := context.Background() + solution := composedGroupDeclaredAsOverriddenBy(t, + "CONFIG_DIR=/etc/app\nCONFIG_FILE=${profile}\nconfig-file=${profile}\n", + "CONFIG_FILE=/etc/app/solution.yaml\n", "") + workspace := loadWorkspace(t, ctx, solution) + + _, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, resources.LocalEnvironment()) + require.Error(t, err) + require.ErrorIs(t, err, configurations.ErrConfigurationConflict) + assert.ErrorContains(t, err, "app-config/CONFIG_FILE") + assert.ErrorContains(t, err, "app-config/config-file", "the diagnostic names both declarations") + assert.ErrorContains(t, err, `"host"`, "and the module that carries them") +} + // The module's group is the declared SET of its keys, as a base profile is for // the profiles derived from it. A key only the solution carries is refused where // it is written: the group is delivered to the module's services, which read the diff --git a/docs/workspace-composition.md b/docs/workspace-composition.md index 8f020251c..9bbf570c8 100644 --- a/docs/workspace-composition.md +++ b/docs/workspace-composition.md @@ -60,10 +60,11 @@ refused, naming `app-config/CONFIG_MODE`, instead of running without it. A solution overriding one key is never required to restate the group. **The module's group is the declared set of the group's keys**, exactly as a base -profile is for the profiles derived from it, and the same two rules follow: +profile is for the profiles derived from it, and two rules follow: -- a key only the solution carries is refused, naming it. The group is delivered - to the module's services, which read the keys the module declared; a key its +- a key only the solution carries is refused, naming it — the same rule a derived + profile gets over the profile it derives from. The group is delivered to the + module's services, which read the keys the module declared; a key its declaration never mentions is a value nothing reads. A solution that needs a key of its own declares a **group** of its own name, which reaches every service of the composition. @@ -74,11 +75,21 @@ profile is for the profiles derived from it, and the same two rules follow: refuse. A key for which nothing is a legitimate value is declared with a default by the group's author, not with the marker. -Keys are matched the way every other configuration lookup matches them — case -insensitively, with `-` and `_` equivalent. A structured document (a `.yaml` -group) has no keys to overlay, so the solution's document replaces the module's -whole; an empty one does not discharge a document declared per profile, and a -boundary that turns a document into key/value pairs is a conflict. + This second refusal is **this boundary's own**: profile derivation does not + carry it today, so a derived profile that empties a `${profile}` its base + declares is still accepted. The two layers are not the same situation — the + profiles of one workspace are written by the author who wrote the declaration, + while across this boundary the author discharging the marker is not the author + who declared it, and cannot be refused later by a reviewer of the module. + +One key declared twice — a repeated line, or two files consolidated into one +group, under either spelling — is refused on whichever side of the boundary +declares it, rather than resolved by which line came last. Keys are matched the +way every other configuration lookup matches them: case insensitively, with `-` +and `_` equivalent. A structured document (a `.yaml` group) has no keys to +overlay, so the solution's document replaces the module's whole; an empty one +does not discharge a document declared per profile, and a boundary that turns a +document into key/value pairs is a conflict. **An overridden group stays composed.** Its provider is still the module, so it reaches the services that declared it as a dependency rather than every service From 86fde0218e7dcfc89d01cce61d3e306ebf7b0f11 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Sun, 4 Oct 2026 11:42:53 -0400 Subject: [PATCH 4/6] chore(scout): the configurations package passes the full linter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lint gate in .github/workflows/go.yml is commented out, so nothing has run golangci-lint over this package in CI. Run in full at this head (--max-issues-per-linter=0 --max-same-issues=0, no --new-from-rev) it reported 15 findings here. They are all pre-existing — `go vet ./...` is already clean, and the one finding this PR's own diff moved is the composer's cyclomatic complexity, which stood at 31 before it and 37 after. Behaviour is unchanged. What the findings asked for: - goconst: the four format strings that decide parsing — env, yaml, yml, json — are named once in configuration.go. The same four decided it in three files, where a typo reads as "unsupported kind" at the far end of a load. - gocyclo: composeModuleWorkspaceConfigurations was three phases in one function, past the threshold before this PR touched it. The two it already had — accumulate every composed module's offers, then resolve what survives against the consuming workspace's own declarations — are now a composedModuleOffers value with gather and resolve on it, and the closures that carried the shared state are its methods. Complexity 37 to under the gate, with the same control flow and comments in each phase. - govet shadow: five errors in a loop body and two in an if took the name of the function's own err. Each is now named for the call that produced it. - gosec G304/G204: three reads and one subprocess, annotated per site with what the variable actually is — a configuration file of the workspace being read, a dns declaration of it, a profile derivation declaration, and a resolver's own CLI named by this package. These are the package's purpose, not unchecked input, and the annotation says so where a reader meets it rather than as a config-wide exclusion. - revive: the secret-file narrative above OnePasswordScheme was being read as that constant's doc. The constant has its own now. Module-wide the linter reports 1312 findings across 60 packages, which is a campaign of its own and not this PR's to carry. See the review comment on #694 for the per-linter breakdown. Co-Authored-By: Claude Opus 5 (1M context) --- configurations/configuration.go | 18 +- configurations/local_reader.go | 304 +++++++++++++++++++------------- configurations/profile.go | 6 +- configurations/secrets.go | 12 +- 4 files changed, 205 insertions(+), 135 deletions(-) diff --git a/configurations/configuration.go b/configurations/configuration.go index be4534bcc..adc967515 100644 --- a/configurations/configuration.go +++ b/configurations/configuration.go @@ -13,9 +13,21 @@ import ( "github.com/codefly-dev/core/wool" ) +// The format a configuration file's content is in, which is its extension: it +// is what ConfigurationData.Kind carries for a structured group, and what the +// local reader reads a file as. They are named because the same four strings +// decide parsing in three files of this package, and a typo in one of them +// reads as "an unsupported kind" at the far end of a load. +const ( + kindEnv = "env" + kindYaml = "yaml" + kindYml = "yml" + kindJSON = "json" +) + func ConfigurationInformationDataFromFile(ctx context.Context, name string, p string, isSecret bool) (*basev0.ConfigurationInformation, error) { w := wool.Get(ctx).In("provider.ConfigurationInformationDataFromFile") - content, err := os.ReadFile(p) + content, err := os.ReadFile(p) //nolint:gosec // p is a configuration file of the workspace being read if err != nil { return nil, w.Wrapf(err, "cannot read yaml env file") } @@ -35,10 +47,10 @@ func InformationUnmarshal(info *basev0.ConfigurationInformation, v interface{}) if info.Data == nil { return nil } - if info.Data.Kind == "yaml" { + if info.Data.Kind == kindYaml { return yaml.Unmarshal(info.Data.Content, v) } - if info.Data.Kind == "json" { + if info.Data.Kind == kindJSON { return json.Unmarshal(info.Data.Content, v) } return fmt.Errorf("unsupported kind %s", info.Data.Kind) diff --git a/configurations/local_reader.go b/configurations/local_reader.go index 629af2dfb..e807f854e 100644 --- a/configurations/local_reader.go +++ b/configurations/local_reader.go @@ -294,14 +294,14 @@ func (local *ConfigurationInformationLocalReader) Load(ctx context.Context, env serviceConfs := make(map[string]*basev0.Configuration) serviceOrigins := make([]string, 0, len(services)) for _, svc := range services { - identity, err := svc.Identity() - if err != nil { - return w.Wrapf(err, "cannot get service identity") + identity, identityErr := svc.Identity() + if identityErr != nil { + return w.Wrapf(identityErr, "cannot get service identity") } serviceOrigins = append(serviceOrigins, identity.Unique()) - serviceProfile, err := LoadProfileConfigurations(ctx, svc.Dir(), "configurations", profiles) - if err != nil { - return w.Wrapf(err, "cannot load service configuration profile") + serviceProfile, profileErr := LoadProfileConfigurations(ctx, svc.Dir(), "configurations", profiles) + if profileErr != nil { + return w.Wrapf(profileErr, "cannot load service configuration profile") } if serviceProfile.Exists { serviceInfos := serviceProfile.Infos @@ -322,14 +322,14 @@ func (local *ConfigurationInformationLocalReader) Load(ctx context.Context, env } } // Load DNS - dnsFile, exists, err := ProfileFile(ctx, svc.Dir(), "dns", profiles, "dns.codefly.yaml") - if err != nil { - return w.Wrapf(err, "cannot select service dns declaration") + dnsFile, exists, dnsSelectErr := ProfileFile(ctx, svc.Dir(), "dns", profiles, "dns.codefly.yaml") + if dnsSelectErr != nil { + return w.Wrapf(dnsSelectErr, "cannot select service dns declaration") } if exists { - dns, err := loadDNS(ctx, dnsFile) - if err != nil { - return w.Wrapf(err, "cannot load dns") + dns, dnsErr := loadDNS(ctx, dnsFile) + if dnsErr != nil { + return w.Wrapf(dnsErr, "cannot load dns") } for _, d := range dns { d.Service = identity.Name @@ -465,89 +465,41 @@ func composeModuleWorkspaceConfigurations( ) ([]*basev0.ConfigurationInformation, map[string]string, map[string]error, []ProfileRequirement, error) { w := wool.Get(ctx).In("configurations.composeModuleWorkspaceConfigurations") - modules, err := workspace.LoadModules(ctx) - if err != nil { - return nil, nil, nil, nil, w.Wrapf(err, "cannot load modules") - } - workspaceInfos := workspaceLevel.Infos // overriding locates the consuming workspace's own declaration of a name, by // position: a module offering that name does not lose to it, it is overlaid // onto it in place, where the workspace declared it. replacing holds every // other workspace-level declaration — inherited from a composed workspace, or // shadowing one — which still wins whole. + workspaceInfos := workspaceLevel.Infos overriding := make(map[string]int, len(workspaceInfos)) - replacing := make(map[string]bool, len(workspaceInfos)) + offers := newComposedModuleOffers(len(workspaceInfos), profiles) for index, info := range workspaceInfos { if workspaceLevel.Own[info.Name] && !workspaceLevel.FromComposedWorkspace[info.Name] { overriding[info.Name] = index continue } - replacing[info.Name] = true - } - sourceReferenced := make(map[string]bool) - for _, ref := range workspace.Modules { - if ref.Source != "" { - sourceReferenced[ref.Name] = true - } + offers.replacing[info.Name] = true } - composed := make(map[string]*composedConfiguration) - var order []string - offer := func(module string, repo string, fromRepo bool, infos []*basev0.ConfigurationInformation) { - for _, info := range infos { - if replacing[info.Name] { - w.Debug("workspace-level configuration a composed workspace also declares replaces composed module configuration", - wool.Field("configuration", info.Name), wool.Field("module", module)) - continue - } - // A name the consuming workspace declares ITSELF is not dropped here: - // its values are overlaid onto this group once every offer is in, so - // the group the workspace overrides must first be resolved like any - // other — a submodule's declaration over its repository's, and an - // ambiguity between two modules that the workspace's own declaration - // then resolves. - existing, ok := composed[info.Name] - if !ok { - composed[info.Name] = &composedConfiguration{info: info, module: module, repo: repo, fromRepo: fromRepo} - order = append(order, info.Name) - continue - } - if existing.conflict != nil { - continue - } - // A module directory and the root of the repository holding it are - // not two competing providers: they are one artifact, where the - // module's own directory is the more specific declaration. - if existing.repo == repo && existing.fromRepo != fromRepo { - overrides := !proto.Equal(existing.info, info) - submodule := existing.module - if !fromRepo { - submodule = module - existing.info, existing.module, existing.fromRepo = info, module, false - } - if overrides { - w.Warn("composed module configuration overrides the one its repository provides, for every service in the run", - wool.Field("configuration", info.Name), wool.Field("module", submodule)) - } - continue - } - if proto.Equal(existing.info, info) { - continue - } - existing.conflict = w.NewError( - "workspace configuration %q is provided by composed modules %q and %q; declare it in the solution workspace to resolve the ambiguity: %w", - info.Name, existing.module, module, ErrConfigurationConflict) - } + if err := offers.gather(ctx, workspace, workspaceConfigurationDir); err != nil { + return nil, nil, nil, nil, w.Wrap(err) } + return offers.resolve(ctx, workspaceInfos, overriding) +} - // Several module references can resolve to the same directory: to one - // composed workspace root (e.g. two non-flat submodules composed from one - // host repo, one reached through a symlinked path), or — since a foreign - // reference names its module by coordinate rather than by name — to one - // submodule composed twice under different names. Resolve symlinks so those - // collapse to one key and read the directory once; reading it twice would - // offer every configuration in it against itself. - loaded := make(map[string]bool) +// composedModuleOffers accumulates what the composed modules offer for the +// selected profile, before any of it is resolved against the consuming +// workspace's own declarations. One name can be offered more than once — by a +// module and by the repository holding it, or by two modules that disagree about +// it — so an offer is recorded against what is already there rather than +// appended, and resolve then turns what survives into the provisioned set. +type composedModuleOffers struct { + // replacing are the workspace-level names that win whole, so a module's + // offer of one is dropped where it is made rather than resolved. + replacing map[string]bool + profiles []string + composed map[string]*composedConfiguration + order []string // unsuppliedByGroup holds, per group name a composed module offers, the // per-profile values that module left unsupplied. It is keyed by group so only // the requirements of the offers that actually win are reported: a group that @@ -555,27 +507,115 @@ func composeModuleWorkspaceConfigurations( // consuming workspace overrides still owes them — the override is per key, so // a marker it does not discharge is still in the provisioned group, and // StillUnsupplied drops exactly the ones it did discharge. - unsuppliedByGroup := make(map[string][]ProfileRequirement) - readOnce := func(dir string) ([]*basev0.ConfigurationInformation, error) { - key := dir - if resolved, err := filepath.EvalSymlinks(dir); err == nil { - key = resolved + unsuppliedByGroup map[string][]ProfileRequirement + // loaded are the configuration directories already read, keyed by resolved + // path. Several module references can resolve to the same directory: to one + // composed workspace root (e.g. two non-flat submodules composed from one + // host repo, one reached through a symlinked path), or — since a foreign + // reference names its module by coordinate rather than by name — to one + // submodule composed twice under different names. Symlinks are resolved so + // those collapse to one key and the directory is read once; reading it twice + // would offer every configuration in it against itself. + loaded map[string]bool +} + +func newComposedModuleOffers(workspaceNames int, profiles []string) *composedModuleOffers { + return &composedModuleOffers{ + replacing: make(map[string]bool, workspaceNames), + profiles: profiles, + composed: make(map[string]*composedConfiguration), + unsuppliedByGroup: make(map[string][]ProfileRequirement), + loaded: make(map[string]bool), + } +} + +// offer records what one composed module provides, from one of its directories. +func (offers *composedModuleOffers) offer(ctx context.Context, module string, repo string, fromRepo bool, infos []*basev0.ConfigurationInformation) { + w := wool.Get(ctx).In("configurations.composedModuleOffers.offer") + for _, info := range infos { + if offers.replacing[info.Name] { + w.Debug("workspace-level configuration a composed workspace also declares replaces composed module configuration", + wool.Field("configuration", info.Name), wool.Field("module", module)) + continue } - if loaded[key] { - return nil, nil + // A name the consuming workspace declares ITSELF is not dropped here: + // its values are overlaid onto this group once every offer is in, so + // the group the workspace overrides must first be resolved like any + // other — a submodule's declaration over its repository's, and an + // ambiguity between two modules that the workspace's own declaration + // then resolves. + existing, ok := offers.composed[info.Name] + if !ok { + offers.composed[info.Name] = &composedConfiguration{info: info, module: module, repo: repo, fromRepo: fromRepo} + offers.order = append(offers.order, info.Name) + continue } - loaded[key] = true - provided, err := LoadProfileConfigurations(ctx, dir, "configurations", profiles) - if err != nil { - return nil, w.Wrapf(err, "cannot load module configuration profile") + if existing.conflict != nil { + continue } - if !provided.Exists { - return nil, nil + // A module directory and the root of the repository holding it are + // not two competing providers: they are one artifact, where the + // module's own directory is the more specific declaration. + if existing.repo == repo && existing.fromRepo != fromRepo { + overrides := !proto.Equal(existing.info, info) + submodule := existing.module + if !fromRepo { + submodule = module + existing.info, existing.module, existing.fromRepo = info, module, false + } + if overrides { + w.Warn("composed module configuration overrides the one its repository provides, for every service in the run", + wool.Field("configuration", info.Name), wool.Field("module", submodule)) + } + continue } - for _, requirement := range provided.Unsupplied { - unsuppliedByGroup[requirement.Group] = append(unsuppliedByGroup[requirement.Group], requirement) + if proto.Equal(existing.info, info) { + continue + } + existing.conflict = w.NewError( + "workspace configuration %q is provided by composed modules %q and %q; declare it in the solution workspace to resolve the ambiguity: %w", + info.Name, existing.module, module, ErrConfigurationConflict) + } +} + +// readOnce reads one configuration directory, the first time it is asked for it, +// and collects what the profile left unsupplied there. +func (offers *composedModuleOffers) readOnce(ctx context.Context, dir string) ([]*basev0.ConfigurationInformation, error) { + w := wool.Get(ctx).In("configurations.composedModuleOffers.readOnce") + key := dir + if resolved, err := filepath.EvalSymlinks(dir); err == nil { + key = resolved + } + if offers.loaded[key] { + return nil, nil + } + offers.loaded[key] = true + provided, err := LoadProfileConfigurations(ctx, dir, "configurations", offers.profiles) + if err != nil { + return nil, w.Wrapf(err, "cannot load module configuration profile") + } + if !provided.Exists { + return nil, nil + } + for _, requirement := range provided.Unsupplied { + offers.unsuppliedByGroup[requirement.Group] = append(offers.unsuppliedByGroup[requirement.Group], requirement) + } + return provided.Infos, nil +} + +// gather reads every composed module's configurations and offers them. +func (offers *composedModuleOffers) gather(ctx context.Context, workspace *resources.Workspace, workspaceConfigurationDir string) error { + w := wool.Get(ctx).In("configurations.composedModuleOffers.gather") + + modules, err := workspace.LoadModules(ctx) + if err != nil { + return w.Wrapf(err, "cannot load modules") + } + sourceReferenced := make(map[string]bool) + for _, ref := range workspace.Modules { + if ref.Source != "" { + sourceReferenced[ref.Name] = true } - return provided.Infos, nil } for _, mod := range modules { @@ -595,38 +635,52 @@ func composeModuleWorkspaceConfigurations( } repoDir := composedModuleWorkspaceDir(mod.Dir(), consumingBoundary) repo := repoDir - if resolved, err := filepath.EvalSymlinks(repoDir); err == nil { + if resolved, resolveErr := filepath.EvalSymlinks(repoDir); resolveErr == nil { repo = resolved } // The consuming workspace's own configurations are already loaded; a flat // root module's workspace root coincides with the consuming workspace's, // as does an in-repo module whose nearest workspace root is the consuming // one, so skip either rather than load the same directory twice. - repoConfigurationDir, _, err := ProfileDirectory(ctx, repoDir, "configurations", profiles) - if err != nil { - return nil, nil, nil, nil, w.Wrapf(err, "cannot select configuration directory for composed module %s", mod.Name) + repoConfigurationDir, _, dirErr := ProfileDirectory(ctx, repoDir, "configurations", offers.profiles) + if dirErr != nil { + return w.Wrapf(dirErr, "cannot select configuration directory for composed module %s", mod.Name) } if !resources.SameDir(repoConfigurationDir, workspaceConfigurationDir) { - infos, err := readOnce(repoDir) - if err != nil { - return nil, nil, nil, nil, w.Wrapf(err, "cannot load configurations for composed module %s", mod.Name) + infos, readErr := offers.readOnce(ctx, repoDir) + if readErr != nil { + return w.Wrapf(readErr, "cannot load configurations for composed module %s", mod.Name) } - offer(mod.Name, repo, true, infos) + offers.offer(ctx, mod.Name, repo, true, infos) } if foreign && !resources.SameDir(mod.Dir(), repoDir) { - infos, err := readOnce(mod.Dir()) - if err != nil { - return nil, nil, nil, nil, w.Wrapf(err, "cannot load configurations for composed module %s", mod.Name) + infos, readErr := offers.readOnce(ctx, mod.Dir()) + if readErr != nil { + return w.Wrapf(readErr, "cannot load configurations for composed module %s", mod.Name) } - offer(mod.Name, repo, false, infos) + offers.offer(ctx, mod.Name, repo, false, infos) } } + return nil +} + +// resolve turns the offers that survived into the provisioned set, overlaying +// the consuming workspace's own declaration of a name onto the group offered +// under it — in the position the workspace declared it — and reporting what each +// provisioned name is composed by, which names stayed ambiguous, and what the +// provisioned groups still owe. +func (offers *composedModuleOffers) resolve( + ctx context.Context, + workspaceInfos []*basev0.ConfigurationInformation, + overriding map[string]int, +) ([]*basev0.ConfigurationInformation, map[string]string, map[string]error, []ProfileRequirement, error) { + w := wool.Get(ctx).In("configurations.composedModuleOffers.resolve") - composedBy := make(map[string]string, len(order)) + composedBy := make(map[string]string, len(offers.order)) ambiguous := make(map[string]error) var unsupplied []ProfileRequirement - for _, name := range order { - configuration := composed[name] + for _, name := range offers.order { + configuration := offers.composed[name] index, overridden := overriding[name] if configuration.conflict != nil { // The ambiguity's own remedy is a declaration in the consuming @@ -658,7 +712,7 @@ func composeModuleWorkspaceConfigurations( // it, so it is reported as composed either way and reaches the services // that declared it rather than every service of the composition. composedBy[name] = configuration.module - unsupplied = append(unsupplied, unsuppliedByGroup[name]...) + unsupplied = append(unsupplied, offers.unsuppliedByGroup[name]...) if overridden { workspaceInfos[index] = info continue @@ -944,7 +998,7 @@ func applyConfigurationValueOverride( func loadDNS(_ context.Context, file string) ([]*basev0.DNS, error) { var dns []*basev0.DNS - f, err := os.ReadFile(file) + f, err := os.ReadFile(file) //nolint:gosec // file is a dns declaration of the workspace being read if err != nil { return nil, err } @@ -1034,17 +1088,17 @@ func LoadConfigurationInformationsFromFiles(ctx context.Context, dir string) ([] sort.Slice(files, func(i, j int) bool { return files[i].relative < files[j].relative }) - if err := validateConfigurationDefinitions(files); err != nil { - return nil, w.Wrapf(err, "cannot load secret configurations") + if definitionErr := validateConfigurationDefinitions(files); definitionErr != nil { + return nil, w.Wrapf(definitionErr, "cannot load secret configurations") } infos := make([]*basev0.ConfigurationInformation, 0, len(files)) for _, file := range files { var confInfo *basev0.ConfigurationInformation switch file.kind { - case "env": + case kindEnv: confInfo, err = loadFromEnvFile(ctx, file) - case "yaml": + case kindYaml: confInfo, err = loadFromYamlFile(ctx, file) } if err != nil { @@ -1071,28 +1125,28 @@ func classifyConfigurationFile(dir, p string) (*configurationFile, bool, error) switch { case strings.HasSuffix(relative, ".secret.ref.env"): file.name = strings.TrimSuffix(relative, ".secret.ref.env") - file.kind = "env" + file.kind = kindEnv file.secret = true file.referenceOnly = true case strings.HasSuffix(relative, ".secret.ref.yaml"): file.name = strings.TrimSuffix(relative, ".secret.ref.yaml") - file.kind = "yaml" + file.kind = kindYaml file.secret = true file.referenceOnly = true case strings.HasSuffix(relative, ".secret.env"): file.name = strings.TrimSuffix(relative, ".secret.env") - file.kind = "env" + file.kind = kindEnv file.secret = true case strings.HasSuffix(relative, ".secret.yaml"): file.name = strings.TrimSuffix(relative, ".secret.yaml") - file.kind = "yaml" + file.kind = kindYaml file.secret = true case strings.HasSuffix(relative, ".env"): file.name = strings.TrimSuffix(relative, ".env") - file.kind = "env" + file.kind = kindEnv case strings.HasSuffix(relative, ".yaml"): file.name = strings.TrimSuffix(relative, ".yaml") - file.kind = "yaml" + file.kind = kindYaml default: return nil, false, nil } @@ -1125,7 +1179,7 @@ func validateConfigurationDefinitions(files []*configurationFile) error { definition.legacy = file.relative } } - if file.kind == "yaml" && definition.first != "" { + if file.kind == kindYaml && definition.first != "" { return fmt.Errorf("configuration %q has incompatible data definitions in %q and %q: %w", file.name, definition.first, file.relative, ErrConfigurationConflict) } if definition.data != "" { @@ -1134,7 +1188,7 @@ func validateConfigurationDefinitions(files []*configurationFile) error { if definition.first == "" { definition.first = file.relative } - if file.kind == "yaml" { + if file.kind == kindYaml { definition.data = file.relative } seen[file.name] = definition diff --git a/configurations/profile.go b/configurations/profile.go index 600ba2db7..663ad8544 100644 --- a/configurations/profile.go +++ b/configurations/profile.go @@ -177,8 +177,8 @@ func ProfileLayers(ctx context.Context, base, kind string, profiles []string) ([ return nil, false, fmt.Errorf("%s declares no derives-from; remove the file or name the profile this one derives from: %w", path.Join(current, ProfileDerivationFile), ErrProfileDerivation) } - if err := resources.ValidateConfigurationProfileName(name); err != nil { - return nil, false, fmt.Errorf("%s: %w: %w", path.Join(current, ProfileDerivationFile), err, ErrProfileDerivation) + if nameErr := resources.ValidateConfigurationProfileName(name); nameErr != nil { + return nil, false, fmt.Errorf("%s: %w: %w", path.Join(current, ProfileDerivationFile), nameErr, ErrProfileDerivation) } if seen[name] { return nil, false, fmt.Errorf("configuration profile %q at %s derives from %q, which is already in the chain: %w", @@ -219,7 +219,7 @@ func readProfileDerivation(ctx context.Context, dir string) (*ProfileDerivation, if !exists { return nil, nil } - content, err := os.ReadFile(file) + content, err := os.ReadFile(file) //nolint:gosec // file is the profile derivation declaration of the directory being read if err != nil { return nil, err } diff --git a/configurations/secrets.go b/configurations/secrets.go index e5935e5b3..ff1702e0a 100644 --- a/configurations/secrets.go +++ b/configurations/secrets.go @@ -29,6 +29,9 @@ import ( // Resolved values never touch disk. In legacy files only known schemes are // references, so a postgres:// plaintext value still passes through. The // SecretResolver seam allows additional backends without changing loaders. + +// OnePasswordScheme is the URI scheme that names the 1Password backend in a +// reference value. const OnePasswordScheme = "op" // ProviderOnePassword is the `secrets.kind` that selects the 1Password backend. @@ -149,6 +152,7 @@ func (r *OnePasswordResolver) Resolve(ctx context.Context, ref *SecretReference) // intact. Provider output is suppressed on failure because neither stream is // guaranteed to be free of secret material. func runCommand(ctx context.Context, name string, args ...string) (string, error) { + //nolint:gosec // name is a resolver's own CLI, named by this package, never by configuration content cmd := exec.CommandContext(ctx, name, args...) var stdout bytes.Buffer var stderr bytes.Buffer @@ -270,11 +274,11 @@ func (e *secretResolution) resolveConfiguration(ctx context.Context, conf *basev func (e *secretResolution) resolveData(ctx context.Context, data *basev0.ConfigurationData, env *resources.Environment, origin string) error { var node any switch data.Kind { - case "yaml", "yml": + case kindYaml, kindYml: if err := yaml.Unmarshal(data.Content, &node); err != nil { return err } - case "json": + case kindJSON: dec := json.NewDecoder(bytes.NewReader(data.Content)) dec.UseNumber() if err := dec.Decode(&node); err != nil { @@ -293,9 +297,9 @@ func (e *secretResolution) resolveData(ctx context.Context, data *basev0.Configu } var out []byte switch data.Kind { - case "yaml", "yml": + case kindYaml, kindYml: out, err = yaml.Marshal(resolved) - case "json": + case kindJSON: out, err = json.Marshal(resolved) } if err != nil { From 3cc18fed3ea7edceb26756a55d0e814769ba30c2 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Sun, 4 Oct 2026 12:22:57 -0400 Subject: [PATCH 5/6] fix(configurations): a duplicate key is refused per profile, before flattening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The duplicate refusal landed at the workspace/module boundary, and the inputs reach that boundary with their profile layers already flattened. So a duplicate written in a DERIVED profile was erased before the refusal could see it: profileOverlay.add overlaid the second entry onto the first and handed one value across. Reported by Codex (gpt-6-astra) round two on #694, finding 1. Executed against this commit's parent, with the module declaring CONFIG_FILE=${profile}, the solution's shared profile declaring CONFIG_FILE=seed, and the profile deriving from it declaring both spellings: CONFIG_FILE=one / config-file=two -> delivered config-file=two config-file=two / CONFIG_FILE=one -> delivered CONFIG_FILE=one Two lines, swapped, and the workload gets a different value — with no error either way. That is exactly what the refusal exists to prevent, surviving one layer up, so the promised rule still depended on where the duplicate was written. It is now refused per LAYER, in profileOverlay.add, before anything is flattened: a layer is one profile's declaration of a group, so two entries for one key in it have no single value and nothing to arbitrate between them. Per layer is also what keeps the legitimate case distinguishable — one declaration in a base profile and one in a profile derived from it is the derivation this type performs, and is untouched. The diagnostic names the profile and both spellings, so it points at the file to open rather than at the boundary that noticed. The boundary keeps its own check. No profile-loaded input can now reach it carrying a duplicate, which makes it the precondition rather than a reachable path, so it is tested directly in local_reader_internal_test.go: a guard whose only justification is "nothing can get here" is the one that silently stops holding when a second caller appears. Also finishes round two's finding 5 (minor), which this change decides anyway: the helper and composer comments still promised "the same two rules" as profile derivation. They now say which rules are shared — the lookup and set mechanics, the undeclared-key refusal, and as of this commit the duplicate refusal — and that the empty-value refusal is this boundary's own, with the reason. The docs gain the per-profile duplicate rule in both the composition and the derivation sections. Co-Authored-By: Claude Opus 5 (1M context) --- configurations/local_reader.go | 29 ++++-- configurations/local_reader_internal_test.go | 49 +++++++++ configurations/profile.go | 18 ++++ configurations/workspace_override_test.go | 103 ++++++++++++++++++- docs/workspace-composition.md | 20 ++-- 5 files changed, 203 insertions(+), 16 deletions(-) diff --git a/configurations/local_reader.go b/configurations/local_reader.go index e807f854e..ce1d023e2 100644 --- a/configurations/local_reader.go +++ b/configurations/local_reader.go @@ -425,12 +425,16 @@ type composedConfiguration struct { // it as the composition root's own and widened its delivery from the services // that declared it to every service of the composition. A partial override // narrowed a group's contents and widened its delivery at the same time. So the -// overlaid group stays composed, the module remains its provider, and the rules -// that hold between profile derivation layers hold across this boundary too: -// the module's group is the declared set of the group's keys (a key only the -// workspace carries is refused, ErrUndeclaredProfileKey), a marker the override -// does not discharge is still owed, and an override that empties one is refused -// naming it (ErrEmptyProfileValue). A workspace that needs a key of its own +// overlaid group stays composed, the module remains its provider, and the +// module's group is the declared set of the group's keys across this boundary as +// a base profile is for the profiles derived from it: a key only the workspace +// carries is refused (ErrUndeclaredProfileKey, the same rule profile derivation +// applies), a marker the override does not discharge is still owed, and one key +// declared twice is refused rather than resolved by position +// (ErrConfigurationConflict, refused per layer in profileOverlay.add). An +// override that empties a marker is refused naming it (ErrEmptyProfileValue) — +// that one is THIS boundary's own, and profile derivation does not carry it; see +// overlayWorkspaceConfigurationOverride for why. A workspace that needs a key of its own // declares a group of its own name, which reaches every service of the // composition. // @@ -727,7 +731,18 @@ func (offers *composedModuleOffers) resolve( // workspace supplies the values it declares, and the module's group supplies // every other key it declares. It is the same overlay a derived profile gets over // the profile it derives from (profileOverlay), applied across the -// workspace/module boundary, and it carries the same two rules with it. +// workspace/module boundary, with the same lookup and set mechanics +// (findConfigurationValue, setConfigurationValue) and the same refusal of a key +// the layer below never declared. +// +// One rule is this boundary's OWN: an override that discharges a +// ProfileValueMarker with an empty value is refused here, and profile derivation +// accepts it. The asymmetry is deliberate. A workspace's profiles are written by +// the author who wrote the declaration they override, in a file beside it, so +// emptying one there is a local statement about one environment. Across this +// boundary the author discharging the marker is not the author who declared it, +// is not reviewed with it, and the module's own reviewers never see the value +// that satisfied their requirement. // // The module's group is the declared SET of the group's keys. A key only the // workspace carries is refused naming it: the group is the module's, delivered to diff --git a/configurations/local_reader_internal_test.go b/configurations/local_reader_internal_test.go index e8d0ca284..8d0320f45 100644 --- a/configurations/local_reader_internal_test.go +++ b/configurations/local_reader_internal_test.go @@ -1,6 +1,7 @@ package configurations import ( + "errors" "strings" "testing" @@ -41,3 +42,51 @@ func TestApplyServiceConfigurationOverridesRejectsOriginCollisionWhenTargeted(t t.Fatalf("a targeted override with colliding origins must be rejected, got %v", err) } } + +// The overlay boundary refuses a duplicate canonical key on either side. With +// the per-layer refusal in profileOverlay.add, no profile-loaded input can +// reach here carrying one, so this is the boundary stating its own precondition +// rather than a reachable path — and it is tested directly, because a guard +// whose only justification is "nothing can get here" is the guard that silently +// stops holding when a second caller appears. +func TestOverlayWorkspaceConfigurationOverrideRefusesADuplicateKeyOnEitherSide(t *testing.T) { + group := func(values ...*basev0.ConfigurationValue) *basev0.ConfigurationInformation { + return &basev0.ConfigurationInformation{Name: "app-config", ConfigurationValues: values} + } + value := func(key, v string) *basev0.ConfigurationValue { + return &basev0.ConfigurationValue{Key: key, Value: v} + } + + for _, test := range []struct { + name string + base *basev0.ConfigurationInformation + override *basev0.ConfigurationInformation + expect string + }{ + { + name: "the module's group declares one key twice", + base: group(value("CONFIG_FILE", ProfileValueMarker), value("config-file", ProfileValueMarker)), + override: group(value("CONFIG_FILE", "/etc/app/solution.yaml")), + expect: "composed module", + }, + { + name: "the override declares one key twice", + base: group(value("CONFIG_FILE", ProfileValueMarker)), + override: group(value("CONFIG_FILE", "/etc/app/solution.yaml"), value("config_file", "")), + expect: "the consuming workspace declares", + }, + } { + t.Run(test.name, func(t *testing.T) { + _, err := overlayWorkspaceConfigurationOverride(test.base, test.override, "host") + if err == nil { + t.Fatalf("a duplicate canonical key must be refused, not resolved by position") + } + if !errors.Is(err, ErrConfigurationConflict) { + t.Fatalf("want ErrConfigurationConflict, got %v", err) + } + if !strings.Contains(err.Error(), test.expect) { + t.Fatalf("the diagnostic must name which side declared it, got %v", err) + } + }) + } +} diff --git a/configurations/profile.go b/configurations/profile.go index 663ad8544..bcbb26680 100644 --- a/configurations/profile.go +++ b/configurations/profile.go @@ -343,6 +343,24 @@ func (overlay *profileOverlay) add(layer string, infos []*basev0.ConfigurationIn if info == nil { continue } + // One key declared twice IN ONE LAYER is refused here, before the layer is + // overlaid onto anything. A layer is one profile's declaration of a group, + // so two entries for one key in it have no single value and nothing to + // arbitrate between them: set below would simply overlay the second onto + // the first and the group would carry whichever the file happened to name + // last. That silent collapse also erased the evidence — a duplicate + // written in a derived profile reached the workspace/module boundary as + // one value, so the refusal there could not see it, and which declaration + // was delivered stayed a fact about line order. + // + // This is per LAYER, which is what keeps a legitimate override + // distinguishable from a duplicate: the same key declared once in a base + // profile and once in a profile derived from it is the derivation this + // type exists to perform, and is not touched here. + if first, second, duplicated := duplicateConfigurationKey(info); duplicated { + return fmt.Errorf("profile %q declares %s/%s and %s/%s, one key under two spellings; declare it once: %w", + path.Base(layer), info.GetName(), first, info.GetName(), second, ErrConfigurationConflict) + } existing := overlay.find(info.GetName()) if existing == nil { overlay.infos = append(overlay.infos, info) diff --git a/configurations/workspace_override_test.go b/configurations/workspace_override_test.go index 78fee74df..ec7069dc5 100644 --- a/configurations/workspace_override_test.go +++ b/configurations/workspace_override_test.go @@ -269,8 +269,11 @@ func TestAWorkspaceOverrideDeclaringOneKeyTwiceIsRefusedRatherThanResolvedByOrde // module's group was discarded, duplicate and all. Overlaying onto it has to // pick a declaration to overlay — and a solution that supplies the key would // otherwise be told the key is still owed, because the second declaration still -// carries the marker. Naming the module's duplicate says what is actually -// wrong, to the only author who can fix it. +// carries the marker. +// +// It is refused where it is written: the diagnostic names the profile holding +// the two declarations, and the read that failed names the module, so the only +// author who can fix it is told which file to open. func TestAComposedModuleDeclaringOneKeyTwiceIsRefusedWhereItIsOverridden(t *testing.T) { ctx := context.Background() solution := composedGroupDeclaredAsOverriddenBy(t, @@ -283,7 +286,101 @@ func TestAComposedModuleDeclaringOneKeyTwiceIsRefusedWhereItIsOverridden(t *test require.ErrorIs(t, err, configurations.ErrConfigurationConflict) assert.ErrorContains(t, err, "app-config/CONFIG_FILE") assert.ErrorContains(t, err, "app-config/config-file", "the diagnostic names both declarations") - assert.ErrorContains(t, err, `"host"`, "and the module that carries them") + assert.ErrorContains(t, err, `profile "local"`, "and the profile they are written in") + assert.ErrorContains(t, err, "composed module host", "and the module that carries them") +} + +// A duplicate written in a DERIVED profile used to be erased before any refusal +// could see it. The solution's shared profile declares the key once; the profile +// deriving from it declares it twice, under two spellings. profileOverlay.add +// overlaid the second onto the first and handed one value to the boundary, so +// the refusal there had nothing to refuse — and the value delivered was whichever +// spelling the file named last. Reversing the two lines changed what the workload +// got, which is the thing the refusal exists to prevent, surviving one layer up. +// +// The refusal is therefore per layer, before anything is flattened. A key +// declared once in the base and once in the profile derived from it is the +// derivation itself and stays legitimate — asserted here so the two cases cannot +// collapse into one. +func TestADerivedProfileDeclaringOneKeyTwiceIsRefusedBeforeItIsFlattened(t *testing.T) { + ctx := context.Background() + for _, test := range []struct { + name string + local string + shared string + refuse bool + }{ + { + name: "two spellings in the derived profile, the order of which decided the value", + local: "CONFIG_FILE=one\nconfig-file=two\n", + shared: "CONFIG_FILE=seed\nCONFIG_MODE=seed\n", + refuse: true, + }, + { + name: "the same two, written the other way round", + local: "config-file=two\nCONFIG_FILE=one\n", + shared: "CONFIG_FILE=seed\nCONFIG_MODE=seed\n", + refuse: true, + }, + { + name: "two spellings in the base profile this one derives from", + local: "CONFIG_FILE=one\n", + shared: "CONFIG_FILE=seed\nconfig-file=seed\nCONFIG_MODE=seed\n", + refuse: true, + }, + { + name: "one declaration per layer, which is the derivation and not a duplicate", + local: "CONFIG_FILE=one\n", + shared: "CONFIG_FILE=seed\nCONFIG_MODE=seed\n", + refuse: false, + }, + } { + t.Run(test.name, func(t *testing.T) { + root := t.TempDir() + writeConfigurationFile(t, root, "solution/workspace.codefly.yaml", `name: solution +layout: modules +modules: + - name: host + path: ../host +`) + writeConfigurationFile(t, root, "solution/configurations/shared/app-config.env", test.shared) + writeConfigurationFile(t, root, "solution/configurations/local/profile.codefly.yaml", "derives-from: shared\n") + writeConfigurationFile(t, root, "solution/configurations/local/app-config.env", test.local) + writeConfigurationFile(t, root, "host/module.codefly.yaml", `kind: module +name: host +services: + - name: telemetry +`) + writeConfigurationFile(t, root, "host/configurations/local/app-config.env", + "CONFIG_DIR=/etc/app\nCONFIG_FILE=${profile}\nCONFIG_MODE=${profile}\n") + writeConfigurationFile(t, root, "host/services/telemetry/service.codefly.yaml", `kind: service +name: telemetry +version: 0.0.0 +agent: + kind: runtime::service + name: go-grpc + version: 0.0.1 + publisher: codefly.ai +`) + workspace := loadWorkspace(t, ctx, filepath.Join(root, "solution")) + + provided, err := configurations.ReadWorkspaceConfigurations(ctx, workspace, resources.LocalEnvironment()) + if test.refuse { + require.Error(t, err, "the duplicate is refused rather than flattened into one value") + require.ErrorIs(t, err, configurations.ErrConfigurationConflict) + assert.ErrorContains(t, err, "app-config/CONFIG_FILE") + assert.ErrorContains(t, err, "app-config/config-file") + return + } + require.NoError(t, err, "a key declared once per layer is the derivation, not a duplicate") + assert.Equal(t, map[string]string{ + "CONFIG_DIR": "/etc/app", + "CONFIG_FILE": "one", + "CONFIG_MODE": "seed", + }, groupValues(t, provided.Infos, "app-config"), + "the derived profile's value wins per key, over the base's and over the module's") + }) + } } // The module's group is the declared SET of its keys, as a base profile is for diff --git a/docs/workspace-composition.md b/docs/workspace-composition.md index 9bbf570c8..86c843095 100644 --- a/docs/workspace-composition.md +++ b/docs/workspace-composition.md @@ -82,11 +82,16 @@ profile is for the profiles derived from it, and two rules follow: while across this boundary the author discharging the marker is not the author who declared it, and cannot be refused later by a reviewer of the module. -One key declared twice — a repeated line, or two files consolidated into one -group, under either spelling — is refused on whichever side of the boundary -declares it, rather than resolved by which line came last. Keys are matched the -way every other configuration lookup matches them: case insensitively, with `-` -and `_` equivalent. A structured document (a `.yaml` group) has no keys to +One key declared twice **in one profile** — a repeated line, or two files +consolidated into one group, under either spelling — is refused where it is +written, naming the profile and both spellings, rather than resolved by which +line came last. It is refused per profile rather than at this boundary alone, +because a derived profile's duplicate would otherwise be flattened into one +value before anything could see it, and *which* value reached the workload would +be decided by the order of two lines. A key declared once in a base profile and +once in a profile derived from it is the derivation itself, not a duplicate. +Keys are matched the way every other configuration lookup matches them: case +insensitively, with `-` and `_` equivalent. A structured document (a `.yaml` group) has no keys to overlay, so the solution's document replaces the module's whole; an empty one does not discharge a document declared per profile, and a boundary that turns a document into key/value pairs is a conflict. @@ -185,7 +190,10 @@ declares a group nothing provides is told so by name. A derived declaration replaces the whole value rather than merging into it, so a profile that makes a shared default secret, or replaces it with an assembled -template, says so in one place. A structured document (a `.yaml` group) has no +template, says so in one place. One profile declaring the same key **twice** is +refused instead, naming the profile and both spellings: overlaying the second +onto the first would deliver whichever the file named last, which is not a +statement either declaration makes. A structured document (a `.yaml` group) has no keys to overlay, so a derived profile's document replaces the one it derives from — overlaying one *field* of a document per profile is not supported, and such a group is declared per profile whole or stays shared; turning a document into From 620e412d9089081415431ac44d07fcd00249dbd3 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Sun, 4 Oct 2026 12:28:03 -0400 Subject: [PATCH 6/6] chore(configurations): rewrap two comment lines this PR overran Comment text only, no code. Both are lines this PR wrote: the composer's rule summary ran to 89 columns and doc.go's origin-classification paragraph to 115, against the ~80 the surrounding comments keep. gofmt does not reflow comments, so neither showed up in a format check. Co-Authored-By: Claude Opus 5 (1M context) --- configurations/doc.go | 7 ++++--- configurations/local_reader.go | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/configurations/doc.go b/configurations/doc.go index ffe18ca9c..2f760e509 100644 --- a/configurations/doc.go +++ b/configurations/doc.go @@ -125,9 +125,10 @@ // only the module provides, and without widening that name's delivery. A name an // imported workspace also declares is the exception the overlay does not cover: // an imported declaration is not the product's to amend, so it still replaces a -// composed module's group of that name whole and is classified as the root's. An invocation-scoped override is the -// run supplying a value, so it is composition-root even when it lands on a name a -// composed module also provides. +// composed module's group of that name whole and is classified as the root's. +// +// An invocation-scoped override is the run supplying a value, so it is +// composition-root even when it lands on a name a composed module also provides. // // Git owns the non-secret manifest and workspace configuration, Core validates // and resolves only provider references, the CLI selects the declared diff --git a/configurations/local_reader.go b/configurations/local_reader.go index ce1d023e2..fd44f7e8e 100644 --- a/configurations/local_reader.go +++ b/configurations/local_reader.go @@ -434,8 +434,8 @@ type composedConfiguration struct { // (ErrConfigurationConflict, refused per layer in profileOverlay.add). An // override that empties a marker is refused naming it (ErrEmptyProfileValue) — // that one is THIS boundary's own, and profile derivation does not carry it; see -// overlayWorkspaceConfigurationOverride for why. A workspace that needs a key of its own -// declares a group of its own name, which reaches every service of the +// overlayWorkspaceConfigurationOverride for why. A workspace that needs a key of +// its own declares a group of its own name, which reaches every service of the // composition. // // That is the consuming workspace's own declaration of a name no composed