BUG: aspect being called as if it was a module-function#408
Closed
Conversation
Collaborator
|
Looking into this, I'm reproducing this on my end as well. |
Collaborator
|
I have the root cause and a fix, working on the cleanest possible version of the fix. |
sini
added a commit
that referenced
this pull request
Apr 8, 2026
Return { includes = [fn] } instead of a bare function, matching the
POC pattern. The function result lives in includes where
parametric.applyIncludes handles it with withIdentity, preserving the
aspect name through resolution. Fixes #408.
sini
added a commit
that referenced
this pull request
Apr 8, 2026
Return { includes = [fn] } instead of a bare function, matching the
POC pattern. The function result lives in includes where
parametric.applyIncludes handles it with withIdentity, preserving the
aspect name through resolution. Fixes #408.
sini
added a commit
to sini/den
that referenced
this pull request
Apr 9, 2026
Bare function aspects (e.g., { host, ... }: { nixos... }) merged with
static config from separate modules now work correctly. Fixes vic#408.
Three changes:
- take.nix: carryAttrs preserves name through successful take calls
- types.nix: coercedTo in aspectsType wraps non-module functions into
{ includes = [fn] } so they aren't treated as module functions
- perHost-perUser.nix: returns { includes = [fn] } matching the POC
pattern, so parametric.applyIncludes handles identity via withIdentity
vic
pushed a commit
that referenced
this pull request
Apr 9, 2026
Return { includes = [fn] } instead of a bare function, matching the POC
pattern. The function result lives in includes where
parametric.applyIncludes handles it with withIdentity, preserving the
aspect name through resolution. Fixes #408.
sini
added a commit
to sini/den
that referenced
this pull request
Apr 11, 2026
PR vic#410 added coercedProviderType to make `{host, ...}: {nixos = ...}` aspects merge with sibling static `.nixos = ...` defs (fixes vic#408). The predicate was too broad: any non-module function got wrapped into `{ includes = [fn] }`, which silently broke "factory" aspects like: den.aspects.facter = reportPath: { nixos = ...; }; den.aspects.igloo.includes = [ (den.aspects.facter "/path") ]; Coercing turns den.aspects.facter into a full aspect with the default functor. Calling `(facter "/path")` invokes the default functor in a non-static branch with the string as `ctx` — the user's `reportPath` is discarded and the config body never materializes. Narrow the predicate with `lib.functionArgs v != {}`. Context fns have destructured named args (non-empty functionArgs) and still get coerced. Factory fns with a bare positional arg stay typed as providerFnType, whose merge wraps the underlying function via `__functor = _: eth.merge loc defs` so `(aspect arg)` correctly dispatches to the user function. Bisected to d266c3a (PR vic#410). Minimal repro verified broken at that commit and working at v0.15.0. Fix passes the new deadbugs test plus all 278 existing CI tests including deadbugs-issue-408 (context-fn + static merge still works). Fixes vic#429.
vic
pushed a commit
that referenced
this pull request
Apr 11, 2026
PR #410 added coercedProviderType to make `{host, ...}: {nixos = ...}` aspects merge with sibling static `.nixos = ...` defs (fixes #408). The predicate was too broad: any non-module function got wrapped into `{ includes = [fn] }`, which silently broke "factory" aspects like: den.aspects.facter = reportPath: { nixos = ...; }; den.aspects.igloo.includes = [ (den.aspects.facter "/path") ]; Coercing turns den.aspects.facter into a full aspect with the default functor. Calling `(facter "/path")` invokes the default functor in a non-static branch with the string as `ctx` — the user's `reportPath` is discarded and the config body never materializes. Narrow the predicate with `lib.functionArgs v != {}`. Context fns have destructured named args (non-empty functionArgs) and still get coerced. Factory fns with a bare positional arg stay typed as providerFnType, whose merge wraps the underlying function via `__functor = _: eth.merge loc defs` so `(aspect arg)` correctly dispatches to the user function. Bisected to d266c3a (PR #410). Minimal repro verified broken at that commit and working at v0.15.0. Fix passes the new deadbugs test plus all 278 existing CI tests including deadbugs-issue-408 (`context-fn + static` merge still works). Fixes #429.
sini
added a commit
to sini/den
that referenced
this pull request
Apr 11, 2026
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.
sini
added a commit
to sini/den
that referenced
this pull request
Apr 12, 2026
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.
sini
added a commit
to sini/den
that referenced
this pull request
Apr 13, 2026
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.
sini
added a commit
that referenced
this pull request
Apr 13, 2026
# 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.
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.
EDIT: we need a new CI action that runs bogus when it detect we change a .nix file on templates/bogus. Instead of having to approve this for templates to run at CI. #246
Fails with