diff --git a/nix/lib/aspects/fx/aspect/children.nix b/nix/lib/aspects/fx/aspect/children.nix index b1f55aa2..f64126cd 100644 --- a/nix/lib/aspects/fx/aspect/children.nix +++ b/nix/lib/aspects/fx/aspect/children.nix @@ -55,7 +55,7 @@ let registerPolicy = p: fx.send "register-aspect-policy" { - inherit (p) name fn; + inherit (p) fn; ownerIdentity = identity.key p; }; diff --git a/nix/lib/aspects/fx/aspect/provide.nix b/nix/lib/aspects/fx/aspect/provide.nix index 0c87ad12..4949cf53 100644 --- a/nix/lib/aspects/fx/aspect/provide.nix +++ b/nix/lib/aspects/fx/aspect/provide.nix @@ -14,7 +14,7 @@ let inherit (den.lib.schemaUtil) schemaEntityKinds schemaEntityKindsSet; mkCrossPolicy = - aspectName: nodeIdentity: provides: key: + nodeIdentity: provides: key: let value = provides.${key}; policyFn = @@ -52,9 +52,9 @@ let ); in fx.send "register-aspect-policy" { - name = "${aspectName}/${key}"; fn = policyFn; ownerIdentity = nodeIdentity; + label = key; }; # Extract the inner function and args from a provider value. @@ -148,8 +148,13 @@ let provides = aspect.provides or { }; crossKeys = builtins.filter (k: k != aspectName) (builtins.attrNames provides); - compatKeys = builtins.filter (k: !(schemaEntityKindsSet ? ${k})) crossKeys; - allRegistrations = map (mkCrossPolicy aspectName nodeIdentity provides) compatKeys; + # Every merged aspect carries a synthetic `__functor` on `provides`; it is + # pipeline machinery, not a delivery target. Registering it produced one + # inert policy per aspect that re-dispatched on every iteration. + compatKeys = builtins.filter ( + k: !(schemaEntityKindsSet ? ${k}) && !(lib.hasPrefix "__" k) + ) crossKeys; + allRegistrations = map (mkCrossPolicy nodeIdentity provides) compatKeys; hasSelfProvide = provides ? ${aspectName}; selfProvide = mkSelfProvideInclude aspect aspectName; diff --git a/nix/lib/aspects/fx/handlers/policy.nix b/nix/lib/aspects/fx/handlers/policy.nix index 26b007c8..bf744239 100644 --- a/nix/lib/aspects/fx/handlers/policy.nix +++ b/nix/lib/aspects/fx/handlers/policy.nix @@ -8,14 +8,21 @@ let "register-aspect-policy" = { param, state }: let - entry = { - inherit (param) fn ownerIdentity; - }; + entry = { inherit (param) fn; }; + # The registry is the one scoped field that merges rather than appends, + # so a repeated key is a silent drop. Derive it here from the owner's + # identity instead of letting each caller name its own: a caller that + # reaches for a local name registers `tools/to-users` for every aspect + # owning a sub-aspect called `tools`, and all but the last vanish. + # `label` separates several registrations by one owner (one per + # `provides` key); an owner registering once needs none. + label = param.label or null; + registryKey = if label == null then param.ownerIdentity else "${param.ownerIdentity}/${label}"; in { resume = null; state = scopedMerge state "scopedAspectPolicies" state.currentScope { - ${param.name} = entry; + ${registryKey} = entry; }; }; }; diff --git a/nix/lib/aspects/types.nix b/nix/lib/aspects/types.nix index 6ff60e2b..a535572a 100644 --- a/nix/lib/aspects/types.nix +++ b/nix/lib/aspects/types.nix @@ -553,7 +553,29 @@ let # matching mergeWithAspectMeta behavior for root aspects — including # the rule that a name the wrapper defines itself keeps its own value # and stays classified. - providesChildren = builtins.removeAttrs (merged.provides or { }) [ "_module" ]; + # `_` is the write alias for `provides`. aspectSubmodule wires it with + # mkAliasOptionModule, but a nested key never reaches that submodule: + # `_` is structural, so an alias write arrives here as a plain key and + # is then discarded by the `_` this wrapper publishes below. Fold it + # into the provides source so both spellings mean the same thing at + # every depth. A `_` read back off another wrapper carries __functor; + # that is the read shorthand, not a write, and stays out of provides. + writtenUnderscore = + let + v = merged._ or null; + in + lib.optionalAttrs (builtins.isAttrs v && !(v ? __functor)) v; + # Both spellings arrive as a content wrapper when the key is defined in + # more than one file, carrying `__contentValues` / `__provider` / `_` + # alongside the real children. Those are wrapper machinery, not + # provides children: unfiltered they surface as `provides` keys, enter + # `__providesForwarded`, and `_` (not `__`-prefixed) registers an inert + # cross-provide policy. Filtering here covers `provides` and `_` at + # once, and the single-def path is unaffected because a raw attrset + # carries none of these keys. + providesChildren = lib.filterAttrs (k: _: !(structuralKeysSet ? ${k}) && !(lib.hasPrefix "__" k)) ( + (merged.provides or { }) // writtenUnderscore + ); unshadowedProvides = builtins.filter (k: !(merged ? ${k})) (builtins.attrNames providesChildren); provider = (typeCfg.providerPrefix or [ ]) ++ [ keyName ]; # A key names a candidate child aspect when it is neither structural, @@ -604,7 +626,12 @@ let __contentValues = flatDefs; __provider = provider; __providesForwarded = unshadowedProvides; - _ = underscoreAt provider annotatedMerged; + # Root aspects publish `provides` and `_` as one value — provides- + # children plus the all-children functor (mergeWithAspectMeta's + # syntheticProvides). Match that here so the two spellings are + # interchangeable for reading as well as writing, at any depth. + provides = providesChildren // underscoreAt provider annotatedMerged; + _ = providesChildren // underscoreAt provider annotatedMerged; } // lib.optionalAttrs singleFn { __functor = _self: (builtins.head flatDefs).value; @@ -661,7 +688,30 @@ let internal = true; visible = false; description = "Provider path tracking aspect provenance"; - type = lib.types.listOf lib.types.str; + # NOT listOf: that type accumulates, and a node has exactly one + # position. Two files defining one aspect path each inject the same + # chain (providerType.merge, wrapperToAspect), and concatenating them + # yields ["a" "a"] — a chain every descendant then inherits. Agreeing + # definitions collapse; genuinely different ones are an ambiguity den + # cannot resolve, so it says so rather than picking one. + type = lib.types.mkOptionType { + name = "aspectChain"; + description = "aspect provenance chain"; + check = v: builtins.isList v && builtins.all builtins.isString v; + merge = + loc: defs: + let + distinct = lib.unique (map (d: d.value) defs); + in + if distinct == [ ] then + [ ] + else if builtins.length distinct == 1 then + builtins.head distinct + else + throw "den: conflicting provenance for ${locName loc}: ${ + lib.concatMapStringsSep " vs " (c: "[${lib.concatStringsSep " " c}]") distinct + }"; + }; default = typeCfg.providerPrefix or [ ]; }; options.collisionPolicy = lib.mkOption { @@ -681,12 +731,21 @@ let typeCfg: lib.types.submodule ( { name, config, ... }: + let + # The chain this aspect's children hang off. `meta.provider` defaults to + # `typeCfg.providerPrefix`, but providerType.merge overrides it when it + # re-types an included nested aspect (wrapperToAspect injects the chain + # from __provider). Reading the static typeCfg there truncates the chain + # to the aspect's own name, so `alpha/tools` and `beta/tools` both hand + # their children the prefix ["tools"] and the children collide. + childProviderPrefix = config.meta.provider ++ [ config.name ]; + in { freeformType = lib.types.lazyAttrsOf ( aspectKeyType ( typeCfg // { - providerPrefix = (typeCfg.providerPrefix or [ ]) ++ [ config.name ]; + providerPrefix = childProviderPrefix; } ) ); @@ -733,7 +792,7 @@ let providerType ( typeCfg // { - providerPrefix = (typeCfg.providerPrefix or [ ]) ++ [ config.name ]; + providerPrefix = childProviderPrefix; } ) ); diff --git a/templates/ci/modules/features/deadbugs/aspect-chain-doubling.nix b/templates/ci/modules/features/deadbugs/aspect-chain-doubling.nix new file mode 100644 index 00000000..028a376c --- /dev/null +++ b/templates/ci/modules/features/deadbugs/aspect-chain-doubling.nix @@ -0,0 +1,40 @@ +# `meta.provider` is a node's position, not an accumulating set. Two files +# defining one aspect path each inject the same chain, and a `listOf` type +# concatenated them into ["a" "a"] — which every descendant then inherited as +# its own prefix, corrupting the whole subtree's identities. +{ denTest, ... }: +{ + flake.tests.deadbugs.aspect-chain-doubling = { + + test-agreeing-definitions-collapse = denTest ( + { den, ... }: + { + imports = [ + { den.aspects.igloo.provides.shared = den.aspects.a.tools; } + { den.aspects.igloo.provides.shared = den.aspects.a.tools; } + ]; + + den.hosts.x86_64-linux.igloo.users.tux = { }; + den.aspects.a.tools.nixos.environment.etc."t".text = "y"; + + expr = den.aspects.igloo.provides.shared.meta.provider or [ ]; + expected = [ "a" ]; + } + ); + + # CONTROL: a single definition was never affected, so a passing multi-def + # case above is only meaningful next to this. + test-control-single-definition = denTest ( + { den, ... }: + { + den.hosts.x86_64-linux.igloo.users.tux = { }; + den.aspects.a.tools.nixos.environment.etc."t".text = "y"; + den.aspects.igloo.provides.shared = den.aspects.a.tools; + + expr = den.aspects.igloo.provides.shared.meta.provider or [ ]; + expected = [ "a" ]; + } + ); + + }; +} diff --git a/templates/ci/modules/features/deadbugs/issue-670-same-named-sub-aspects.nix b/templates/ci/modules/features/deadbugs/issue-670-same-named-sub-aspects.nix new file mode 100644 index 00000000..379e4e64 --- /dev/null +++ b/templates/ci/modules/features/deadbugs/issue-670-same-named-sub-aspects.nix @@ -0,0 +1,115 @@ +# Issue #670: two unrelated aspects each declaring a same-named sub-aspect that +# delivers via `provides.to-users` — only one of the two lands, the other is +# silently dropped. +# +# Two independent collisions on one cause: an identity string built from a chain +# that had lost its head. `aspectSubmodule` handed its children a provider prefix +# read from the static `typeCfg`, but `providerType.merge` rewrites that chain on +# the value when it re-types an included nested aspect, so `alpha/tools` and +# `beta/tools` both gave their children the prefix ["tools"]. The two delivered +# aspects then shared one identity and gate dedup dropped one of them; the two +# cross-provide policies also shared one `scopedAspectPolicies` key and last-win +# dropped the other. +{ denTest, ... }: +{ + flake.tests.deadbugs.issue-670-same-named-sub-aspects = { + + test-same-named-sub-aspects = denTest ( + { den, tuxHm, ... }: + { + den.hosts.x86_64-linux.igloo.users.tux = { }; + + den.aspects.igloo.includes = [ + den.aspects.alpha + den.aspects.beta + ]; + + den.aspects.alpha = { + includes = [ den.aspects.alpha.tools ]; + tools.provides.to-users.homeManager.home.sessionVariables.ALPHA = "yes"; + }; + + den.aspects.beta = { + includes = [ den.aspects.beta.tools ]; + tools.provides.to-users.homeManager.home.sessionVariables.BETA = "yes"; + }; + + expr = { + alpha = tuxHm.home.sessionVariables.ALPHA or ""; + beta = tuxHm.home.sessionVariables.BETA or ""; + }; + expected = { + alpha = "yes"; + beta = "yes"; + }; + } + ); + + # The delivery assertions above pass for any fix that merely keeps the two + # registrations apart. This pins the cause: the two sub-aspects must hold + # DISTINCT identities, which is what `hasAspect` reads. + test-same-named-sub-aspects-have-distinct-identities = denTest ( + { den, igloo, ... }: + { + den.hosts.x86_64-linux.igloo.users.tux = { }; + + den.aspects.igloo.includes = [ + den.aspects.alpha + den.aspects.beta + ]; + + den.aspects.alpha = { + includes = [ den.aspects.alpha.tools ]; + tools.nixos.environment.etc."alpha".text = "yes"; + }; + + den.aspects.beta = { + includes = [ den.aspects.beta.tools ]; + tools.nixos.environment.etc."beta".text = "yes"; + }; + + expr = { + alphaTools = igloo.environment.etc ? "alpha"; + betaTools = igloo.environment.etc ? "beta"; + }; + expected = { + alphaTools = true; + betaTools = true; + }; + } + ); + + # CONTROL: same shape, distinct sub-aspect names — reporter says this works. + test-control-distinct-sub-aspect-names = denTest ( + { den, tuxHm, ... }: + { + den.hosts.x86_64-linux.igloo.users.tux = { }; + + den.aspects.igloo.includes = [ + den.aspects.alpha + den.aspects.beta + ]; + + den.aspects.alpha = { + includes = [ den.aspects.alpha.atools ]; + atools.provides.to-users.homeManager.home.sessionVariables.ALPHA = "yes"; + }; + + den.aspects.beta = { + includes = [ den.aspects.beta.btools ]; + btools.provides.to-users.homeManager.home.sessionVariables.BETA = "yes"; + }; + + expr = { + alpha = tuxHm.home.sessionVariables.ALPHA or ""; + beta = tuxHm.home.sessionVariables.BETA or ""; + }; + expected = { + alpha = "yes"; + beta = "yes"; + }; + } + ); + + }; +} diff --git a/templates/ci/modules/features/deadbugs/multidef-provides-internals.nix b/templates/ci/modules/features/deadbugs/multidef-provides-internals.nix new file mode 100644 index 00000000..a01e9d83 --- /dev/null +++ b/templates/ci/modules/features/deadbugs/multidef-provides-internals.nix @@ -0,0 +1,76 @@ +# A `provides` (or `_`) key defined in more than one file merges into a content +# wrapper, which carries `__contentValues` / `__provider` / `_` beside the real +# children. Those leaked out as provides children: they surfaced as keys of the +# published `provides`, entered `__providesForwarded`, and `_` — not being +# `__`-prefixed — registered an inert cross-provide policy of its own. +# +# `__functor` IS expected in each list: root publishes provides as +# `providesChildren // { __functor = …; }` (mergeWithAspectMeta's +# syntheticProvides) and nested keys match that shape deliberately. +{ denTest, ... }: +{ + flake.tests.deadbugs.multidef-provides-internals = { + + test-multidef-underscore-hides-wrapper-internals = denTest ( + { den, ... }: + { + imports = [ + { den.aspects.alpha.tools._.one.nixos.environment.etc."1".text = "y"; } + { den.aspects.alpha.tools._.two.nixos.environment.etc."2".text = "y"; } + ]; + + den.hosts.x86_64-linux.igloo.users.tux = { }; + + expr = builtins.attrNames (den.aspects.alpha.tools.provides or { }); + expected = [ + "__functor" + "one" + "two" + ]; + } + ); + + # The same leak arrives through the `provides` spelling with no `_` involved, + # so this is the arm proving the fix is not specific to the alias fold. + test-multidef-provides-hides-wrapper-internals = denTest ( + { den, ... }: + { + imports = [ + { den.aspects.alpha.tools.provides.one.nixos.environment.etc."1".text = "y"; } + { den.aspects.alpha.tools.provides.two.nixos.environment.etc."2".text = "y"; } + ]; + + den.hosts.x86_64-linux.igloo.users.tux = { }; + + expr = builtins.attrNames (den.aspects.alpha.tools.provides or { }); + expected = [ + "__functor" + "one" + "two" + ]; + } + ); + + # CONTROL: the shape nested keys are being held to. A root aspect was never + # affected, so the two arms above are only meaningful beside it. + test-control-root-provides-shape = denTest ( + { den, ... }: + { + imports = [ + { den.aspects.alpha.provides.one.nixos.environment.etc."1".text = "y"; } + { den.aspects.alpha.provides.two.nixos.environment.etc."2".text = "y"; } + ]; + + den.hosts.x86_64-linux.igloo.users.tux = { }; + + expr = builtins.attrNames (den.aspects.alpha.provides or { }); + expected = [ + "__functor" + "one" + "two" + ]; + } + ); + + }; +} diff --git a/templates/ci/modules/features/deadbugs/parametric-provides-fan.nix b/templates/ci/modules/features/deadbugs/parametric-provides-fan.nix new file mode 100644 index 00000000..168389a6 --- /dev/null +++ b/templates/ci/modules/features/deadbugs/parametric-provides-fan.nix @@ -0,0 +1,34 @@ +# A parametric aspect fanned per user registers one cross-provide policy PER FAN +# INSTANCE, and that is load-bearing: each instance's `provides` already carries +# its own entity binding, so collapsing them to the ctxId-free base identity +# delivers the last instance's binding to every user. Keyed on the base identity +# this test reads [ "/opt/vic" ] for tux. +{ denTest, ... }: +{ + flake.tests.deadbugs.parametric-provides-fan = { + + test-parametric-provides-keeps-per-instance-binding = denTest ( + { den, tuxHm, ... }: + { + den.hosts.x86_64-linux.igloo.users = { + tux = { }; + vic = { }; + }; + + den.aspects.igloo.includes = [ den.aspects.tools ]; + + den.aspects.tools = + { user, ... }: + { + provides.to-users.homeManager.home.sessionPath = [ "/opt/${user.userName}" ]; + }; + + # Value, not count: a length-only assertion reads 1 under both keyings + # and cannot see that the wrong user's binding arrived. + expr = tuxHm.home.sessionPath or [ ]; + expected = [ "/opt/tux" ]; + } + ); + + }; +} diff --git a/templates/ci/modules/public-api/nested-provides-forwarding.nix b/templates/ci/modules/public-api/nested-provides-forwarding.nix index 5f54396a..1158724b 100644 --- a/templates/ci/modules/public-api/nested-provides-forwarding.nix +++ b/templates/ci/modules/public-api/nested-provides-forwarding.nix @@ -109,5 +109,73 @@ }; } ); + + # `_` is documented as an alias for `provides`. aspectSubmodule wires it via + # mkAliasOptionModule, which a nested key never reaches — writes through it + # used to land on a structural key the content wrapper then overwrote. + test-underscore-write-alias-on-nested-aspect = denTest ( + { den, tuxHm, ... }: + { + den.hosts.x86_64-linux.igloo.users.tux = { }; + + den.aspects.igloo.includes = [ den.aspects.alpha ]; + + den.aspects.alpha = { + includes = [ den.aspects.alpha.tools ]; + tools._.to-users.homeManager.home.sessionVariables.ALPHA = "yes"; + }; + + expr = tuxHm.home.sessionVariables.ALPHA or ""; + expected = "yes"; + } + ); + + # The written value reads back through `_` as well as through `provides`, + # matching how a root aspect publishes both. + test-underscore-reads-back-on-nested-aspect = denTest ( + { den, igloo, ... }: + { + den.hosts.x86_64-linux.igloo.users.tux = { }; + + den.aspects.igloo.includes = [ den.aspects.alpha.tools._.shared ]; + + den.aspects.alpha.tools._.shared.nixos.environment.etc."shared".text = "yes"; + + expr = { + viaUnderscore = den.aspects.alpha.tools._ ? shared; + viaProvides = den.aspects.alpha.tools.provides ? shared; + delivered = igloo.environment.etc."shared".text or ""; + }; + expected = { + viaUnderscore = true; + viaProvides = true; + delivered = "yes"; + }; + } + ); + + # `_` called as the all-children shorthand still works alongside the alias. + test-underscore-shorthand-still-collects-children = denTest ( + { den, igloo, ... }: + { + den.hosts.x86_64-linux.igloo.users.tux = { }; + + den.aspects.igloo.includes = [ (den.aspects.alpha.tools._ { }) ]; + + den.aspects.alpha.tools = { + one.nixos.environment.etc."one".text = "yes"; + two.nixos.environment.etc."two".text = "yes"; + }; + + expr = { + one = igloo.environment.etc ? "one"; + two = igloo.environment.etc ? "two"; + }; + expected = { + one = true; + two = true; + }; + } + ); }; }