Skip to content

fix(niagara): validate create/duplicate/template destinations before source load - #108

Closed
kunkunGames wants to merge 3 commits into
tumourlove:masterfrom
kunkunGames:agent/niagara-validate-package-path
Closed

fix(niagara): validate create/duplicate/template destinations before source load#108
kunkunGames wants to merge 3 commits into
tumourlove:masterfrom
kunkunGames:agent/niagara-validate-package-path

Conversation

@kunkunGames

@kunkunGames kunkunGames commented Jul 24, 2026

Copy link
Copy Markdown

Goal

Reject malformed Niagara create, duplicate, and template-save destinations before loading source assets, invoking AssetTools, or calling CreatePackage.

Plain-English summary

The destination should be validated before looking up a source asset or template. Invalid input now returns a normal tool error without loading the source, creating an empty package, or entering the duplicate/save flow.

Improvements

  • Validates seven owning entry points: system, stateless emitter, HLSL script, system duplicate, NPC, effect type, and emitter template.
  • Validates template-based create_system destinations before template lookup and load.
  • Reuses the create_system owner guard from create_system_from_spec instead of adding a duplicate validation layer.
  • Covers the current stateless-emitter CreatePackage path.
  • Uses FPackageName helpers consistently after validation.
  • Exercises a real valid Niagara create/save/delete lifecycle with an isolated package name.

Before → After

Before After
Some paths only checked for a slash before package or AssetTools calls Every owning destination must be a valid writable Unreal long package name
Duplicate and template-save paths could load the source before rejecting the destination Destination errors return before source or template load
The stateless-emitter package path was outside the earlier guard set All current direct Niagara CreatePackage owners are protected
The valid regression reused a fixed package and removed its file directly Each run uses a GUID package and deletes it through the Editor Asset API after file notifications settle

Side-effect analysis

  • Malformed destination: loads no source/template asset and creates no package.
  • Template-based create: performs template lookup and duplication only after destination validation succeeds.
  • create_system_from_spec: parses the spec and delegates to the validated owner; it has no separate package owner.
  • Valid /Game/... destination: preserves existing factory initialization, Asset Registry, dirtying, and save behavior.
  • Empty or missing required fields retain their existing dedicated errors.
  • The new EditorScriptingUtilities dependency is private and is used only by the automation fixture cleanup.

Implementation

  • Applied the shared package-path validator to seven Niagara owning handlers.
  • Connected direct package creation, AssetTools duplication, and template-save destinations to validated paths.
  • Added a malformed scenario matrix plus a valid system-creation regression.
  • Replaced fixed-path, direct-file cleanup with a GUID fixture and delayed Editor Asset API deletion.
  • Declared the cleanup API's owning module explicitly and synchronized the Niagara specification and changelog.

Shared backlog bookkeeping (read before merging out of order)

Docs/specs/SPEC_MonolithCore.md tracked this work as ~24 of 80 CreatePackage call sites guarded; remaining ~56 ... follow-up backlog, while MonolithPackagePathValidator.h separately claimed ~77 sites remain unguarded. Both numbers were already inconsistent on master and go stale every time an owner is wired.

This PR replaces both with a routed-owner list plus a named remaining backlog, and points the header at the spec so there is a single place to update.

That shared line is updated only in this PR, not in #106 or #107. All three would otherwise edit the same single line and conflict on merge. The list therefore names #106, #107, and #108 as routed owners — if any of those is dropped or deferred, say so and I will trim the line in one commit.

Verification

  • UE 5.8 MonolithPRHostEditor Win64 Development build: passed (11 actions after dependency invalidation).
  • Monolith.Niagara.PackagePath.*: 2/2 passed.
    • Malformed matrix covers seven owners, template create, and spec delegation (9 scenarios).
    • Valid Niagara system creation, save, delayed Editor Asset API deletion, and post-notification absence passed.
  • No test-window warnings or errors.
  • Generated NS_ValidPackagePath_*.uasset residue count: 0.
  • git diff --check: passed.

@kunkunGames
kunkunGames marked this pull request as ready for review July 25, 2026 01:23
@kunkunGames kunkunGames changed the title fix: validate Niagara asset package paths fix(niagara): validate create/duplicate/template destinations before source load Jul 25, 2026
…ite count

SPEC_MonolithCore.md and the MonolithPackagePathValidator.h header carried
two different stale numbers for the same backlog ("~24 of 80 guarded" vs
"the other ~77 remain unguarded"), and both go stale on every PR that wires
a new owner.

Replaces the counts with a routed-owner list plus a named remaining backlog,
and points the header at the spec so there is one place to update. Adds the
Blueprint struct handlers (tumourlove#106), Material creation families (tumourlove#107), and the
Niagara entry points (tumourlove#108) as routed owners.

The shared line is updated only here, not in tumourlove#106 and tumourlove#107, because all three
would otherwise edit the same single line and conflict on merge.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kunkunGames

Copy link
Copy Markdown
Author

Conflict/merge-order note for my six open PRs is in #104 (comment #104 (comment)).

Short version: a pairwise trial merge shows these PRs conflict only in CHANGELOG.md (plus Docs/specs/SPEC_MonolithCore.md for the #104 × #112 pair) — no source conflicts. Merge in any order that suits your batch; I will rebase the remainder and re-verify with a build plus automation tests.

@tumourlove

Copy link
Copy Markdown
Owner

Shipped in v0.21.3 — thanks. Seven Niagara entry points validate the destination before loading a source asset, touching AssetTools, or creating a package, and the hand-rolled FindLastChar('/') splitting is replaced with FPackageName::GetLongPackagePath / GetLongPackageAssetName. Confirmed create_system_from_spec delegates to the guarded HandleCreateSystem rather than needing its own check, as you said.

Two changes from the branch:

I dropped the EditorScriptingUtilities addition to MonolithNiagara.Build.cs. It was only needed by the valid-path test fixture, and I did not take those fixtures — WITH_DEV_AUTOMATION_TESTS is 1 in Development editor builds, which is what ships, so those tests register in end users' Session Frontend and write a real NiagaraSystem into their project with cleanup behind two latent waits. An aborted run leaves residue. The malformed-path cases create nothing, so those are the ones worth having, and I put the real coverage on the validator itself where it is a pure function.

On the shared backlog line: you were right that it was inconsistent on master and right to flag the merge-order coupling. I went further and deleted the count rather than fixing it — bookkeeping that three separate changes have to edit in lockstep is bookkeeping that ends up wrong. It is now a pointer to grep ValidatePackagePath for current owners.

Thanks for the ordering note in the description, incidentally — it is the reason I did not end up with three conflicting edits to one line.

I land contributor fixes as my own commits rather than merging the branch — shipped history stays single-author, reporter credited in the release notes.

https://github.com/tumourlove/monolith/releases/tag/v0.21.3

@tumourlove tumourlove closed this Jul 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants