fix(saveables): keep saveable discovery out of the interop assemblies

[DooDesch]

Linked issue

Fixes #369

Contributor checklist

• I linked an issue opened before this PR, or explained an allowed exception above.
• I checked existing issues and PRs for duplicate or overlapping work.
• This PR addresses one focused problem or request.

Summary

On IL2CPP, SaveableAutoRegistry.DiscoverSaveableTypes called Assembly.GetTypes() on Assembly-CSharp and Assembly-CSharp-firstpass on the first save load of every session. Its name-prefix filter does not match either interop assembly. That call can end the process with an uncatchable 0x80131506 inside GenericSaveablesPatches.BeforeBaseLoaders.

Discovery now goes through ReflectionUtils.GetDerivedClasses<Saveable>(), the bounded helper from #289. It only enumerates assemblies that reference S1API, and it already skips Assembly-CSharp. The existing checks are unchanged: no S1API types, only direct subclasses of Saveable, no abstract, interface or open generic types.

ProductEffectPatches.GetNpcWrapperTypeById walked every loaded assembly with no filter. It now uses GetDerivedClasses<NPC>() for the same reason.

Compatibility

• Public/protected API: none changed. Both methods are private.
• Existing defaults and behavior: same types found. A direct Saveable subclass and an NPC wrapper have to reference S1API, so the reference filter drops no candidate. Discovery order now follows the scan plan's assembly order, which is the loaded-assembly order as before. Instances are still created once and cached.
• Stable IDs, saves, and network payloads: unchanged.

Validation

Built against 0.4.7f12 reference assemblies.

Mono

• dotnet build S1API.sln -c MonoMelon --no-restore -p:AutomateLocalDeployment=false: 0 errors
• dotnet test S1API.Tests/S1API.Tests.csproj -c MonoMelon --no-restore --no-build: 800 passed

IL2CPP

• dotnet build S1API.sln -c Il2CppMelon --no-restore -p:AutomateLocalDeployment=false: 0 errors
• dotnet test S1API.Tests/S1API.Tests.csproj -c Il2CppMelon --no-restore --no-build: 774 passed

New SaveableAutoRegistryTests freeze the discovery result: a direct concrete subclass is registered; an indirect subclass, an abstract subclass and S1API's own types are not. Repeated calls return the same instance.

Runtime evidence

IL2CPP, game 0.4.7f12, MelonLoader 0.7.x, a co-op mod set of about 20 mods, host a lobby and load a save:

S1API	Loads	Result
3.2.1 release	16	13 died with 0x80131506 in DiscoverSaveableTypes
this branch	10	9 loaded; 1 hung in MelonLoader bootstrap before any mod loaded

In every 3.2.1 crash dump, DiscoverSaveableTypes runs at tier 0, so its loop index is still on the stack. Mapped against dumpdomain, the index points at Il2CppAssemblies\Assembly-CSharp.dll in all 9 dumps. With this branch, a mod's Saveable still loads its data in all 9 loads (OnLoaded log line from the mod). The Mono runtime was not exercised in game; on Mono the change only narrows which assemblies are enumerated.

Documentation

None. Both changed methods are private.

Summary by CodeRabbit

• Bug Fixes
  • Improved discovery of supported saveable types and NPC wrappers, while continuing to exclude unsupported types.
• Tests
  • Added coverage for saveable type discovery and consistent results across repeated lookups.

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 44 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 882ca531-f550-4a33-b641-daba76402ec7

📥 Commits

Reviewing files that changed from the base of the PR and between 3ed3ea6 and efe2667.

📒 Files selected for processing: 2

• S1API.Tests/Saveables/SaveableAutoRegistryTests.cs
• S1API/Internal/Patches/ProductEffectPatches.cs

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 0f36b941-2b3b-4912-8b1b-ed9658f040cd

📥 Commits

Reviewing files that changed from the base of the PR and between c843c9d and 3ed3ea6.

📒 Files selected for processing: 3

• S1API.Tests/Saveables/SaveableAutoRegistryTests.cs
• S1API/Internal/Patches/ProductEffectPatches.cs
• S1API/Saveables/SaveableAutoRegistry.cs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

Saveable and NPC wrapper discovery now use ReflectionUtils.GetDerivedClasses<T>() instead of scanning all loaded assemblies. New tests check saveable discovery filters and instance caching.

Changes: Type Discovery

Layer / File(s)	Summary
Saveable discovery and caching S1API/Saveables/SaveableAutoRegistry.cs, S1API.Tests/Saveables/SaveableAutoRegistryTests.cs	SaveableAutoRegistry uses ReflectionUtils.GetDerivedClasses<Saveable>() and retains direct-inheritance filtering. Tests check candidate inclusion and exclusion, and verify that repeated discovery returns the same instance.
NPC wrapper discovery S1API/Internal/Patches/ProductEffectPatches.cs	ProductEffectPatches uses ReflectionUtils.GetDerivedClasses<NPC>(). It retains the existing abstract-type, NPCId, and duplicate-ID handling.

Priority: ⬆️ High

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: ifbars

Merge Risk: ⚪ Minimal · up to 3ed3e

The change avoids scanning the assemblies implicated in save-load crashes, with no confirmed new regression. It is ready to merge after normal checks.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title clearly identifies the main fix: routing saveable discovery away from interop assemblies. It is concise and specific.
Description check		The description follows the repository template. It includes the linked issue, completed checklist, focused summary, compatibility impact, Mono and IL2CPP validation, runtime evidence, and documentati…
Linked Issues check		Issue #369 requires both discovery methods to use ReflectionUtils.GetDerivedClasses<T>() so IL2CPP interop assemblies are not scanned with Assembly.GetTypes(). At the reviewed head, `SaveableAutoR…
Out of Scope Changes check		The reviewed changes are limited to the two discovery paths required by issue #369, related comments and imports, and focused SaveableAutoRegistry tests. The tests support the discovery and caching …

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[ifBars]

Like I mentioned in #369, I can reach the unsafe scan locally but haven’t been able to trigger the fatal error. Do you have a small Polyfill setup that reproduces it? I’d rather leave this open until I can test the crash itself against the PR.

I’m also a bit hesitant about switching both paths to GetDerivedClasses<T>(). First saveable discovery was slower in my local runs 83–90 ms on 3.2.1 versus 160–208 ms here even though the NPC lookup was faster. Could we just skip the generated game/interop assemblies in the existing scans, or is there a reason that wouldn’t cover the Polyfill case?