Skip to content

fix: guarded empty-path route skips cleanly; honor intoPath alias#600

Merged
sini merged 1 commit into
denful:mainfrom
sini:fix/route-intopath-guard
Jun 9, 2026
Merged

fix: guarded empty-path route skips cleanly; honor intoPath alias#600
sini merged 1 commit into
denful:mainfrom
sini:fix/route-intopath-guard

Conversation

@sini

@sini sini commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes two related bugs in simple policy.route, surfaced by a guarded route using intoPath.

Bug 1 — guarded empty-path route crashes when the guard is false

A guarded route whose source is the raw collector module ({ key; _file; imports }, the empty-path case) crashed with:

error: The option `_file' does not exist. Definition values:
  - In `…/flake.nix':
      { _type = "if"; condition = false; content = "foo@igloo"; }

Two causes in nix/lib/aspects/fx/route/wrap.nix guardModule:

  1. It merged the structural source module under config, so module metadata (_file/key/imports) was mis-read as option definitions.
  2. It gated with lib.mkIf — but mkIf false still requires the target option to exist, so an empty-path route into an undeclared option fails type-checking even when skipped.

The forward path (forward.nix guardFn) already handles this correctly with lib.optionalAttrs. guardModule now mirrors it: gates with optionalAttrs (dropping the subtree entirely when false) and recurses into structural imports, leaving module metadata at module level.

Bug 2 — policy.route silently drops intoPath

route only read route.path; an intoPath key (the name the forward API uses, pairing with intoClass/fromClass) was silently dropped, landing content at the class root instead of nesting. Confirmed: intoPath = [ "wrapper" ] placed content at the nixos root → error: The option 'items' does not exist.

route now accepts intoPath as the public alias for path (normalizing to the internal path), and throws den: policy.route: pass either 'intoPath' or 'path', not both if both are given. path still works.

Tests

New regression suite templates/ci/modules/deadbugs/route-intopath-conditional-leak.nix:

  • test-guarded-route-skips-cleanly — guard false → route contributes nothing, no crash.
  • test-intopath-nestsintoPath nests content at the target path.

Full CI: 866/866 passing. Formatted with just fmt.

A guarded simple route with an empty path crashed with "the option `_file'
does not exist" when the guard was false: the raw collector module
(`{ key; _file; imports }`) was merged under `config` — mis-reading module
metadata as option definitions — and gated with `mkIf`, which still requires
the target option to exist even when the condition is false.

`guardModule` now mirrors the forward path: it gates with `lib.optionalAttrs`
(dropping the subtree entirely when false) and recurses into structural
imports, leaving module metadata at module level.

Separately, `policy.route` silently dropped the `intoPath` key (only `path`
was read), landing content at the class root instead of nesting. `route` now
accepts `intoPath` as the public alias for `path` — pairing with `intoClass`
and `fromClass` to match the forward API — and throws if both are given.

Regression tests in deadbugs cover both: guard-false skips cleanly, and
`intoPath` nests at the target path.
@sini sini requested a review from vic as a code owner June 8, 2026 16:31
@github-actions github-actions Bot added the allow-ci allow all CI integration tests label Jun 8, 2026
@sini sini changed the title fix(route): guarded empty-path route skips cleanly; honor intoPath alias fix: guarded empty-path route skips cleanly; honor intoPath alias Jun 8, 2026
@fmway fmway self-requested a review June 9, 2026 12:29

@fmway fmway left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug 2 — policy.route silently drops intoPath

I was wondering why the error in the repro is different from the one in my configuration, then I realized it's because I write intoPath instead of path 😅

Just tested in my configuration, and it works well. I think this can be merged.

@sini sini merged commit 590e20a into denful:main Jun 9, 2026
44 of 52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

allow-ci allow all CI integration tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants