fix: aspect provenance collisions and the nested _ alias (#670) - #671
Merged
Merged
Conversation
Two aspects each declaring a sub-aspect called `tools` and delivering through `provides.to-users` landed only one of the two. 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 (`wrapperToAspect` injects it from `__provider`). Reading the stale prefix truncated the chain to the aspect's own name, so `alpha/tools` and `beta/tools` both gave their children the prefix ["tools"]; the two delivered aspects shared one identity and gate dedup dropped one. The cross-provide policies collided the same way. `scopedAspectPolicies` is the only scoped registry that merges rather than appends, so a repeated key is a silent drop rather than two entries — and each caller named its own key, one of them from the bare aspect name. The handler now derives the key from `ownerIdentity` plus an optional label, so no caller can choose a colliding one. Not fixed here, measured and still open: an inline named aspect written straight into `includes` gets no owner provenance, so two owners can still collide that way. The type-level prefix cannot carry it — making the `includes` type depend on `config` overflows the evaluator on performance.resolve.test-chain-100.
`_` is documented as an alias for `provides`, but only root aspects got one: aspectSubmodule wires it with mkAliasOptionModule, and a nested key never reaches that submodule. `_` is structural, so `tools._.to-users =` arrived at the content wrapper as a plain key and was then overwritten by the `_` the wrapper publishes — the write vanished with no diagnostic. Fold a written `_` into the provides source at that merge. A `_` read back off another wrapper carries `__functor`; that is the read shorthand, not a write, and stays out of provides. The read side was asymmetric too. Root publishes `provides` and `_` as one value — provides children plus the all-children functor — while a nested key published only the functor, so a value written through `_` read back through neither spelling. Nested now publishes the same value for both, leaving the two interchangeable for reading and writing at any depth.
Review found `test-underscore-reads-back-on-nested-aspect` asserted only the `provides` half of the read symmetry. Deleting the `_` half of the change left all seven tests green, so that line had no oracle at all. It now reads both spellings and the delivered content, and deleting the same line fails it. The issue-670 tests likewise asserted only end-to-end delivery, which any fix that merely keeps two registrations apart would satisfy. Added a case that exercises the identity path without the policy path. Also pins the parametric fan: a cross-provide policy registers per fan instance, and that is load-bearing — each instance's `provides` already carries its own entity binding, so keying on the ctxId-free base identity delivers the last instance's binding to every user (tux reads "/opt/vic"). Review recommended exactly that change; the test is what refutes it. That case was broken before this branch, when both instances collapsed onto one name, and is fixed by the identity keying as a side effect. Drops three mechanical findings: `__`-prefixed keys are filtered out of `crossKeys` (every aspect carries a synthetic `provides.__functor` that was registering an inert policy per aspect and re-dispatching every iteration), the unread `ownerIdentity` field is gone from the registry entry now that the key derives from it, and `writtenUnderscore` uses lib.optionalAttrs per the repo style rule.
`meta.provider` was `listOf str`, so two files defining one aspect path each injected the same chain and the module system concatenated them: `["a"]` and `["a"]` merged to `["a" "a"]`. That was survivable while children took their prefix from the static typeCfg — the corruption stopped at the node. Since 483640a a child's prefix is `config.meta.provider ++ [config.name]`, so a doubled chain propagates to every descendant and every provides child of the subtree. Agreeing definitions now collapse and genuinely different ones throw. Last-wins would have answered `["a"]` here too, but only by accident: it is silently wrong exactly when two definitions disagree, which is the failure mode this chain of fixes has been closing. `meta.handleWith` in the same file already takes a custom merge for the same reason.
A `provides` key defined in more than one file merges into a content wrapper, which carries `__contentValues`, `__provider` and `_` beside the real children. `removeAttrs [ "_module" ]` stripped none of them, so they surfaced as keys of the published `provides`, entered `__providesForwarded`, and `_` — not being `__`-prefixed — registered an inert cross-provide policy of its own. Measured `[ "_" "__contentValues" "__functor" "__provider" "one" "two" ]` where `[ "__functor" "one" "two" ]` was intended. Pre-existing, not a consequence of the `_` fold in 75845b2: the control arm reaches the identical leak through the `provides` spelling with no `_` involved. Filtering where providesChildren is built covers both spellings at once, and the single-definition path is unaffected because a raw attrset carries none of these keys.
vic
approved these changes
Sep 3, 2026
vic
enabled auto-merge (squash)
September 3, 2026 20:56
This was referenced Sep 4, 2026
sini
added a commit
that referenced
this pull request
Sep 4, 2026
Closes #674 ## Summary `mergeableType` — the freeform type route nesting evaluates modules under — deep-merges attrsets, concatenates lists, and throws on everything else. Derivations are deliberately excluded from the attrset branch so they stay opaque, so two aspects producing the same package fell to the conflict throw even though the definitions agreed. Same for the reported `enable = true` shape, where deep-merging descends to a scalar leaf with two definitions. - Accepts all-equal definitions before the conflict throw, matching NixOS's `mergeEqualOption`. Checked last, so it forces values only on the path that already threw. #574 introduced the type claiming NixOS module semantics, but `mergeEqualOption` accepts agreeing definitions and only throws on disagreement — so the divergence is latent from #574, and #671 made the path reachable, which is the reporter's own reading of the bisect. This is the same collapse-agreeing-definitions rule #671 applied to `meta.provider`, now on the route freeform type. Testing: - Adds the reported case verbatim (two aspects, one shared derivation), the reported symptom shape (a scalar leaf reached by deep-merging an `apps` attrset defined by two aspects), and a negative control pinning that disagreeing definitions still throw. ## Validation - `nix develop -c just ci`: 1118/1118, exit 0, zero failures and zero errors. `nix develop -c just fmt` a no-op at the final rev. - Reproduced first on an unfixed `origin/main` worktree at 36c8ba5: the issue's test verbatim fails with `den: the option 'shared' has conflicting definitions from multiple aspects`, so the fix is measured against a red baseline rather than a green assumption. - Probed the negative control rather than trusting its pass. Forcing the value without `tryEval` prints that same conflict error, so it fails for the guard's reason and not incidentally. - Confirmed the new suite is discovered by the CI harness with `just ci deadbugs-issue-674` (3/3). The background `just ci` log truncates to 42 lines, so the suite's absence from that log is not evidence either way and was not read as such. ## Known limitation The equality check cannot reach function-valued leaves. Measured against this nixpkgs: | expression | result | | --- | --- | | `(x: x) == (x: x)` | `false` | | `let f = x: x; in f == f` | `false` | | `{ a = 1; f = x: x; } == { a = 1; f = x: x; }` | `false` | A lambda never compares equal, not even to itself, and one lambda anywhere in an attrset makes the whole comparison false. So two aspects delivering the same *function* at one option still conflict, and no `==`-level rule can fix that — it needs identity carried alongside the value. Derivations are unaffected because Nix short-circuits their comparison on `outPath`, which is why the reported case is fully fixable here.
amanako
pushed a commit
to amanako/nix-config
that referenced
this pull request
Sep 17, 2026
- Remove old TODO referencing bundled `._` includes since it was fixed in denful/den#671 (commit 36c8ba5) - aspects under same parent resolve to distincts nodes carrying leaf identity when accessed via `._` - Add a little comment explaining the situation in user part of the settings generator - Simplify comment in lunar-scar aspect
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#670
Summary
Identity:
config.meta.providerrather than the statictypeCfg, so a nested aspect re-typed byproviderType.mergehands its children the chain that merge rewrote instead of one truncated to its own name.scopedAspectPolicieskey fromownerIdentityinside the handler, as that registry is the only scoped one that merges rather than appends and each caller was naming its own key.__-prefixed keys out ofcrossKeysbecause every merged aspect carries a syntheticprovides.__functor, which was registering an inert policy per aspect and re-dispatching it on every iteration.__contentValues/__provider/_were surfacing as provides keys and registering an inert policy.meta.provider'slistOftype with a merge that collapses agreeing definitions and throws on conflicting ones, since a node has exactly one position and concatenation turned two definitions of one path into["a" "a"].Alias:
_into the provides source at the content-wrapper merge so the documented alias works on nested aspects and not only at root, wheremkAliasOptionModulesupplies it.providesand_as one value on nested aspects, matching root, so a value written through either spelling reads back through both.Testing:
providesalready carries its own entity binding.Validation
nix develop -c just ci: 1115/1115, exit 0, zero failures and zero errors, formatting a no-op at the final rev._read half failstest-underscore-reads-back-on-nested-aspect, and restoring thelistOfprovider type failstest-agreeing-definitions-collapsewith["a" "a"]while its single-definition control stays green.providesspelling with no_involved, so it is pre-existing rather than a consequence of the alias fold, and both arms fail without the filter while the root-shape control stays green. The predicted cost of readingconfig.metainfreeformTypedid not appear:chain-100allocates 719,571 values on this branch against 728,838 on main.identity.baseKeycollapses the fan instances and delivers/opt/victo tux, so the ctxId-qualified key is load-bearing and the fan case was already broken before this branch.