BUG: static sub-aspect nixos config silently dropped when included from parametric parent#423
BUG: static sub-aspect nixos config silently dropped when included from parametric parent#423kalyanoliveira wants to merge 5 commits intovic:mainfrom
Conversation
|
As always, if someone could help me verify that I didn't have any skill issues and that this is indeed a bug, that would be great. |
|
A bug indeed, den.aspects.role._.sub.nixos.networking.networkmanager.enable = true;this should work. Please send a fix on this same PR, to run locally use (from den root directory) |
|
OMG. That's why my docker config wasn't applied. |
|
If you can simplify the test more, the better. |
|
This issue also happens with includes : |
templates/bogus/modules/bug.nix
Outdated
| den.aspects.tux.user.description = "The Penguin"; | ||
| imports = | ||
| let | ||
| a = { |
There was a problem hiding this comment.
remove all the part about the sub.enable conditional, not relevant to the bug
There was a problem hiding this comment.
I believe this to be addressed in 932bd7c
vic
left a comment
There was a problem hiding this comment.
If you send a patch solving this, no need to open other PR, just use this same one and when it works we can move the test to a proper file at deadbugs/ and revert changes at bug.nix.
Thanks for finding this one.
|
Will do: currently simplifying bug.nix and trying to work on a patch (sponsored by claude^tm) |
omg thanks for that |
send it over :) just try to make the patch be clean, no accidental complexity |
|
Honestly, I'm happy finding this is a bug because for at least a whole hour, I thought I was deeply misunderstanding something. |
|
Honestly, I don't like the fix that my latest commit introduced. The tests pass, but still, it feels like it's too much hardcoding. Here's some more context (ai generated): The latest commit fixed a bug where static and parametric sub-aspects had their nixos config silently dropped when included from a parametric parent. The root cause is in The fix detects two cases:
The tricky part is The question for you: is that acceptable, or would you prefer to define the infrastructure key list once in |
|
@kalyanoliveira please read this: https://den.oeiuwq.com/contributing and copy those terms here on the description. |
|
@vic sorry, but I cannot promise the following:
I do not understand how den works, nor do I have the time to learn it to the extent that would allow me to "promptly fix problems/bugs caused by" my "contribution." I hope you can understand this: I just play around with nix. I do not want someone else who RELIES on it having issues because of my tinkering also typo in "responsable" lol i also have NO idea what this means
to what extent would I be "legally" accountable for this contribution? anything that touches the realms of "legality" would be an instant "nope" for me I tried fixing the bug with AI, but that's it. I REALLY cannot promise anything else; I hope that you can understand this if my attempt was not good enough, or does not meet contribution standards (which, per the lasts lines of what you linked to me, it does NOT), then, uh... I guess feel free to close the PR? and then open a new one with this same bug again ofc |
|
Oh it has nothing to do with you particularly. I've also been using AI, I've tagged my own PRs as ai-assisted. The issue with AI generated code is that no AI is responsible for it, but humans can contribute code generated by it to projects which have licensing like Den. I know it is practically impossible for us to know what code was our AI trained with, all I ask is that to the best of our knowledge it is not violating GPL or other licensed code. I know you cannot know it , but we need to be explicit what Den maintainers cannot be responsible for some text they did not generate. I'm not against AI, not against you. I do use AI, so does @sini, it is practically impossible not to use something using AI today, even our code being exposed publicly is already most surely being used to train AI.
Are Den maintainers responsible for text generated on their behalf? Because when things fail, we maintainers are asked to solve it. We can choose not to, because we are not under a contract and the LICENSE mentions there's no guarantee, so the AI terms kinda contradict the LICENSE precisely because AI agents skip LICENSE awareness. We have no guarantee that AI respect LICENSEs so who is accountable in the future if some big-evil-corp says you stoled their code and contributed as part of an ai-generated contribution. Please don't take it bad, if you are not willing to use Den, that's ok, Den already uses AI contributions, we just need to be on clear terms about it. I'm not a lawyer (I'm not even an native english speaker, so there you have typos and bad grammar). I've been using AI to help me fix gramamr at documentation. I really appreciate you willing to fix Den, I'm not trying to put barriers all the contrary, you acknowledged from the start your contribution was AI generated and I really appreciate that -- the policy is going along those lines, just make that statement explicit, not trying to hide it. |
|
The AI policy has nothing to do with this particular PR code nor with you as an individual. |
|
Also contributions to the AI policy are welcome, I just redacted the first version. Even constitutions get revised. |
|
Maybe tomorrow we open a discussion about this, I'm not a dictator, I'm trying to make it clear that Den does uses AI contributions as almost any other software now. Given that, shall we even acknowledge it, or simply remove those terms and keep on with being happy doing Nix for Den. |
|
The statement about legally (is more about the LICENSE) when we used to contribute code without AI we used our brains to try come up with code so we said we "authored" it, some companies call "intellectual-property" something you were paid to create for them. Many code is regarded as intellectual-property and because it is property it surely is subject to law, that is the reason behind me writing "law" there. Again, I'm not a lawyer, I don't even like lawyers. All contributions are subject to our LICENSE (human made contrbutions) my questions is more about what happens to non-human contributions. |
|
Shall I take over this bug then? |
|
@vic: thank you so much for clarifying, I'm just now starting to understand the implications of AI generated contributions to the sustainability of den as a project I have to admit that I am still a bit confused on this entire discussion, so, @sini by all means, feel free. from what I currently gather, if I don't say "yes, I fully understand the patch that the AI made and I am taking responsibility for it," then there would potential issues with licensing. it's obvious to me that I will not be able to say "yes, I understand the patch" because, well... that would imply that I would HAVE to FULLY understand the patch. which I don't, and I probably won't in the foreseeable future: learning stuff takes time. and why the hell would one wait for me to learn things just so that this bug can be fixed, when clearly there's interest in getting it fixed:
it has been fun, but I think its time someone actually competent takes over lol |
PR vic#419 introduced applyDeep to propagate parametric context into bare-result sub-includes. But it called takeFn unconditionally on every sub, including full static aspects whose default functor (withOwn parametric.atLeast) discards owned class configs in a non-static branch. Result: `den.aspects.role._.sub.nixos.x = true` was silently dropped when role was a parametric parent that included its sub-aspect. Gate the inner re-application on canTake.upTo: a static aspect's default functor has empty functionArgs, so upTo is false and the sub is left alone for the static resolve pass. User-provided provider fns (e.g. `{ host, ... }: { nixos = ...; }`) have host in functionArgs, so upTo fires and their config is materialized. This preserves the vic#419 fix while restoring pre-vic#419 behavior for static sub-aspects. Fixes vic#423.
PR vic#419 introduced applyDeep to propagate parametric context into bare-result sub-includes. But it called takeFn unconditionally on every sub, including full static aspects whose default functor (withOwn parametric.atLeast) discards owned class configs in a non-static branch. Result: `den.aspects.role._.sub.nixos.x = true` was silently dropped when role was a parametric parent that included its sub-aspect. Gate the inner re-application on canTake.upTo: a static aspect's default functor has empty functionArgs, so upTo is false and the sub is left alone for the static resolve pass. User-provided provider fns (e.g. `{ host, ... }: { nixos = ...; }`) have host in functionArgs, so upTo fires and their config is materialized. This preserves the Fixes vic#423.
…ic#426) PR vic#419 introduced applyDeep to propagate parametric context into bare-result sub-includes. But it called takeFn unconditionally on every sub, including full static aspects whose default functor (withOwn parametric.atLeast) discards owned class configs in a non-static branch. Result: `den.aspects.role._.sub.nixos.x = true` was silently dropped when role was a parametric parent that included its sub-aspect. Gate the inner re-application on canTake.upTo: a static aspect's default functor has empty functionArgs, so upTo is false and the sub is left alone for the static resolve pass. User-provided provider fns (e.g. `{ host, ... }: { nixos = ...; }`) have host in functionArgs, so upTo fires and their config is materialized. This preserves the vic#419 fix while restoring pre-vic#419 behavior for static sub-aspects. Fixes vic#423.
|
Fix is now on main. Let me know if there are any remaining issues. @kalyanoliveira Your policy concerns are valid. We had a lively conversation on matrix, but perhaps opening a issue to continue this discussion would be the best place for it. |
Extends has-aspect.nix with the complete Groups A-J test matrix from the design spec §7.3: - Group A: sanity / transitive chains - Group B: parametric contexts, including the vic#423 and vic#413 regression shapes as explicit lock-ins - Group C: factory-function aspects (§8.1 resolved as Outcome A — factory-path identity is stable; tested via direct factory reference in includes since invoked instances become anonymous aspects) - Group D: provider sub-aspects + identity disambiguation - Group E: mutual-provider / provides chains (to-users, per-user, to-hosts) - Group F: substituteAspect, composition at different levels (NOT nested-reaching per filterIncludes.tag semantics), oneOfAspects integration - Group G: multi-class users (primary, forClass, forAnyClass, unknown class) - Group H: extensibility contract (den.schema.conf owns the option) - Group I: error cases — bad ref throws - Group J: real-world call from inside a deferred nixos module body, validating the cycle-safety guarantee for the primary intended use case (uses outer let-binding closure capture since the `host` specialArg from nix/lib/types.nix:30 lives on the den host submodule and does not propagate into OS-level deferred nixos modules) hasAspect reads config.resolved which is the output of the ctxApply + parametric resolution pipeline. Every aspect-shape recent regressions (vic#408, vic#413, vic#423, vic#429) lived in has a corresponding test here, so a future regression in parametric.nix or aspects/types.nix that affects any of these shapes fails a hasAspect test before it reaches user code. Two tests in Groups B and D are XFAILed against current behavior: function-bodied provider sub-aspects (`foo._.sub = { host, ... }: ...`) lose their `foo` provider prefix in the resolved tree (aspectPath becomes ["sub"] instead of ["foo","sub"]). The class config still materializes correctly — see deadbugs/issue-413 — but hasAspect lookup misses because the reference's path differs from the resolved tree's path. Test bodies document the gap and are wired to flip to `expected = true` once the parametric pipeline preserves provider prefix on function-bodied sub-aspects.
Extends the smoke suite with the complete regression-class matrix, organized by aspect shape: - Group A: sanity / transitive chains - Group B: parametric contexts, incl. vic#423 static-sub-in-parametric- parent and vic#413 bare-function-sub-aspect shapes as explicit regression lock-ins - Group C: factory-function aspects — direct factory reference in includes (factory-path identity is stable; inline invocation produces a differently-named sibling, tested here as a caveat) - Group D: provider sub-aspects + identity disambiguation - Group E: mutual-provider / provides chains (to-users, per-user, to-hosts) - Group F: substituteAspect, composition at different levels (NOT nested-reaching per filterIncludes.tag semantics), oneOfAspects integration - Group G: multi-class users (primary, forClass, forAnyClass, unknown class) - Group H: extensibility contract — den.schema.conf owns the option - Group I: error cases — bad ref throws - Group J: real-world call from inside a deferred nixos module body, validating the cycle-safety guarantee for the primary intended use case hasAspect reads config.resolved — the output of the ctxApply + parametric resolution pipeline. Every aspect-shape that recently produced a regression (vic#408, vic#413, vic#423, vic#429) has a lock-in test here, so a future regression in parametric.nix or aspects/types.nix that affects any of those shapes fails a hasAspect test before it reaches user code.
Extends the smoke suite with the complete regression-class matrix, organized by aspect shape: - Group A: sanity / transitive chains - Group B: parametric contexts, incl. vic#423 static-sub-in-parametric- parent and vic#413 bare-function-sub-aspect shapes as explicit regression lock-ins - Group C: factory-function aspects — direct factory reference in includes (factory-path identity is stable; inline invocation produces a differently-named sibling, tested here as a caveat) - Group D: provider sub-aspects + identity disambiguation - Group E: mutual-provider / provides chains (to-users, per-user, to-hosts) - Group F: substituteAspect, composition at different levels (NOT nested-reaching per filterIncludes.tag semantics), oneOfAspects integration - Group G: multi-class users (primary, forClass, forAnyClass, unknown class) - Group H: extensibility contract — den.schema.conf owns the option - Group I: error cases — bad ref throws - Group J: real-world call from inside a deferred nixos module body, validating the cycle-safety guarantee for the primary intended use case hasAspect reads config.resolved — the output of the ctxApply + parametric resolution pipeline. Every aspect-shape that recently produced a regression (vic#408, vic#413, vic#423, vic#429) has a lock-in test here, so a future regression in parametric.nix or aspects/types.nix that affects any of those shapes fails a hasAspect test before it reaches user code.
# feat: `entity.hasAspect <ref>` query method
## What this does
Adds a `.hasAspect` method on context entities (`host`, `user`, `home`,
and any custom entity kind that imports `den.schema.conf`). It answers
"is this aspect structurally present in my resolved tree?" from inside
class-config module bodies:
```nix
den.aspects.impermanence.nixos = { config, host, ... }: lib.mkMerge [
(lib.mkIf (host.hasAspect <zfs-root>) { /* zfs impermanence */ })
(lib.mkIf (host.hasAspect <btrfs-root>) { /* btrfs impermanence */ })
];
```
There are two variants for when the bare form isn't enough:
```nix
host.hasAspect.forClass "nixos" <facter>
user.hasAspect.forAnyClass <agenix-rekey>
```
Identity is compared by `aspectPath` (`meta.provider ++ [name]`), so
provider sub-aspects like `foo._.sub` keep their full path. Refs can be
plain aspect values (`den.aspects.facter`) or `<angle-bracket>` sugar,
they're the same thing after `__findFile` resolves.
Alongside `hasAspect`, this ships a companion `oneOfAspects` adapter.
That's the structural-decision primitive for "prefer A over B when both
are present", which is the thing you actually want when you're tempted
to use `hasAspect` to decide includes (see the guardrails section
below).
## Why
Real patterns that users hit:
- `<impermanence>` config depends on whether `<zfs-root>` or
`<btrfs-root>` is also configured on the host
- A secrets forward wants to pick `<agenix-rekey>` when present and
fall back to `<sops-nix>` otherwise
- Library aspects want to gate opt-in behavior on companion aspects
Today these get worked around with `config.*` lookups, hand-maintained
`lib.elem` checks, or structural hacks. `hasAspect` is the first-class
primitive for the read side. `oneOfAspects` is the first-class primitive
for the write side.
## Commits
Each commit is independently reviewable and builds green on its own.
### `fix(parametric): preserve meta on materialized parametric results`
Opened as it's own PR #440
### `feat(adapters): add collectPaths terminal adapter`
New public terminal adapter. Walks a resolved tree via `filterIncludes`
and returns `{ paths = [ [providerSeg..., name], ... ]; }`, depth-first,
not deduplicated. Tombstones are skipped via the `meta.excluded` check.
Ships with two small helpers exported from the same file: `pathKey`
(slash-joined path key) and `toPathSet` (list of paths to attrset-as-set
for O(1) lookups). Both are used by `hasAspect` and `oneOfAspects`, so
exporting them keeps the two consumers from duplicating the same
one-liners.
### `feat(adapters): add oneOfAspects structural-decision adapter`
`meta.adapter` that keeps the first structurally-present candidate and
tombstones the rest via `excludeAspect`:
```nix
den.aspects.secrets-bundle = {
includes = [ <agenix-rekey> <sops-nix> ];
meta.adapter = den.lib.aspects.adapters.oneOfAspects [
<agenix-rekey> # preferred
<sops-nix> # fallback
];
};
```
Complements `excludeAspect` and `substituteAspect`. The three together
cover include-this / exclude-this / swap-this-for-that. Internally it
walks the parent subtree with the raw collector (bypassing
`filterIncludes` so it doesn't re-enter itself), finds which candidates
are present, and folds `excludeAspect` over the losers. No code
duplication with `collectPaths` thanks to the shared helpers from the
previous commit.
### `feat(aspects): add has-aspect library primitives`
New file `nix/lib/aspects/has-aspect.nix` exporting:
- `hasAspectIn { tree; class; ref }` for any resolved tree, not just
entity contexts
- `collectPathSet { tree; class }` for an attrset-as-set of visible
paths
- `mkEntityHasAspect { tree; primaryClass; classes }` which builds the
functor-plus-attrs value attached to entities. Per-class path sets
are thunk-cached, so repeated calls share one traversal per class
`refKey` validates that its input has both `name` and `meta` before
reaching for `aspectPath`, throwing loudly rather than silently
producing an `<anon>` path key.
### `feat(context): add entity.hasAspect method via den.schema.conf`
The wiring. `modules/context/has-aspect.nix` is a flake-level module
that self-wires into `den.schema.conf` via
`config.den.schema.conf.imports`. Every entity type that imports `conf`
(host, user, home, and any user-defined kind) inherits `.hasAspect`
automatically. Zero changes to `nix/lib/types.nix`.
Class-protocol: prefers `classes` (list), falls back to `[ class ]`,
throws otherwise. Entities without a matching `den.ctx.<kind>` (so no
`config.resolved`) produce a call-time throw. The fallback value
preserves the functor-plus-attrs shape so `forClass` / `forAnyClass`
attribute access doesn't leak a cryptic error before reaching the real
one.
### `test(has-aspect): full regression-class coverage for entity method`
31 tests organized by aspect-construction shape. Every shape that's
produced a recent regression (`#408`, `#413`, `#423`, `#429`) has a
lock-in test. A future regression in `parametric.nix` or
`aspects/types.nix` that breaks any of these shapes trips a `has-aspect`
test before it reaches user code.
Groups cover basic and transitive chains, parametric contexts including
the static and bare-function sub-aspect shapes, factory functions,
provider sub-aspects and identity disambiguation, mutual-provider and
provides chains, `meta.adapter` composition, multi-class users, the
extensibility contract, error cases, and the primary intended use case
(calling `hasAspect` from inside a deferred `nixos` module body).
### `docs(example): add hasAspect + oneOfAspects worked examples`
User-facing pedagogical file in `templates/example/`. Three sections:
reading structure via `host.hasAspect` from a class-config body, writing
structure via `oneOfAspects` as a `meta.adapter`, and an anti-pattern
section explaining why `hasAspect` can't decide an aspect's `includes`
list with a pointer at the adapter library.
## Design guardrails
`hasAspect` is a read-only query on frozen structure. You call it from
inside class-config module bodies (`nixos = ...`, `homeManager = ...`)
or from lazy positions in aspect functor bodies. It's cycle-safe by
construction because by the time deferred class modules evaluate, the
aspect tree has already been resolved and frozen.
What it is not for: deciding an aspect's `includes` list. That's cyclic.
The tree you want to query depends on the decision you want `hasAspect`
to inform. Users who need that reach for `meta.adapter` composed via
`oneOfAspects`, `excludeAspect`, `substituteAspect`, or `filter` /
`filterIncludes`. Those run during the tree walk with full structural
visibility, so they can't cycle. The template example file has an
explicit anti-pattern section with the failing shape and the correct
rewrite.
Two tools, two jobs:
| Need | Tool | When it runs |
|---|---|---|
| Read "is X in my tree?" from module config | `hasAspect` | After the
tree is frozen, inside lazy class-module bodies |
| Decide tree structure based on "is X present?" | `meta.adapter` +
`oneOfAspects` / friends | During the tree walk, with full structural
visibility |
## Test plan
- [x] `just ci` passes 331/331 at branch tip
- [x] Each commit builds and passes tests on its own
- [x] `just fmt` is idempotent across the tree
- [x] The parametric fix doesn't regress `deadbugs/issue-413-*`,
`deadbugs/issue-423-*`, or `issue-408` tests
- [x] Full regression-class matrix covers every aspect shape that's
produced a recent bug
## Migration
None. Purely additive.
- `hasAspect` is a new option name, no conflict.
- No signature changes in `parametric.nix`, `resolve.nix`,
`ctx-apply.nix`, `types.nix`, or the other core library files.
- `adapters.nix` gains new exports (`collectPaths`, `oneOfAspects`,
`pathKey`, `toPathSet`). Nothing is renamed or removed.
- `nix/lib/aspects/default.nix` re-exports the new lib functions under
`den.lib.aspects.*` at their canonical paths.
- `modules/context/has-aspect.nix` is a new file, picked up by the
existing `import-tree` flake setup.
- Zero changes to `nix/lib/types.nix`.
## Follow-ups
- Docs pass on when to make aspects non-parametric. Users often reach
for `perHost` / `perUser` wrappers when the aspect has no actual ctx
dependence, which makes structural tooling less reliable than it could
be. Docs-only, separate PR.
- Conditional-include wrapper `onlyIf guard target`. A small adapter
sitting next to `oneOfAspects` that lets users write `includes = [
(onlyIf <zfs-root> <zfs-impermanence>) ]` for "include target iff guard
is structurally present." Same cycle-avoidance trick (decision runs in
the wrapper's own meta.adapter, walks the subtree with the raw
`collectPathsInner` collector so it doesn't re-enter itself). Reuses
every helper this PR exports. Rough spec is written up, scoped as its
own follow-up PR.
, meta carryover Add bare function include handling to resolveDeep — wraps lambdas in minimal aspect envelopes so they participate in recursive resolution. Regression tests cover: - Provider sub-aspect with bare parametric includes (vic#413/vic#423) - Static sub inside parametric parent preserves owned config (vic#426) - Factory function not incorrectly coerced (vic#437) - meta.provider survives deep resolution - 3-level deep parametric nesting
, meta carryover Add bare function include handling to resolveDeep — wraps lambdas in minimal aspect envelopes so they participate in recursive resolution. Regression tests cover: - Provider sub-aspect with bare parametric includes (vic#413/vic#423) - Static sub inside parametric parent preserves owned config (vic#426) - Factory function not incorrectly coerced (vic#437) - meta.provider survives deep resolution - 3-level deep parametric nesting
, meta carryover Add bare function include handling to resolveDeep — wraps lambdas in minimal aspect envelopes so they participate in recursive resolution. Regression tests cover: - Provider sub-aspect with bare parametric includes (vic#413/vic#423) - Static sub inside parametric parent preserves owned config (vic#426) - Factory function not incorrectly coerced (vic#437) - meta.provider survives deep resolution - 3-level deep parametric nesting
, meta carryover Add bare function include handling to resolveDeep — wraps lambdas in minimal aspect envelopes so they participate in recursive resolution. Regression tests cover: - Provider sub-aspect with bare parametric includes (vic#413/vic#423) - Static sub inside parametric parent preserves owned config (vic#426) - Factory function not incorrectly coerced (vic#437) - meta.provider survives deep resolution - 3-level deep parametric nesting
, meta carryover Add bare function include handling to resolveDeep — wraps lambdas in minimal aspect envelopes so they participate in recursive resolution. Regression tests cover: - Provider sub-aspect with bare parametric includes (vic#413/vic#423) - Static sub inside parametric parent preserves owned config (vic#426) - Factory function not incorrectly coerced (vic#437) - meta.provider survives deep resolution - 3-level deep parametric nesting
Reproduction
See
templates/bogus/modules/bug.nix.The following setup fails —
igloo.networking.networkmanager.enableisfalsewhen it should betrue:trueThe includes list is correctly populated (length 1) and
host.role._.sub.enableis correctlytrue, but the sub-aspect's nixos config never reaches the host.Fix
Making the sub-aspect a bare parametric function instead of a static attrset makes it work:
Expected behavior
Static sub-aspects should have their nixos config applied when included from a parametric parent function, just as parametric sub-aspects do.