fix(module): stop scanning modules APIScanner can never resolve - #5353
Conversation
setupSandbox() scanned every registered module's class index against the system classloader, including modules loaded from the application path - whose classes are only ever reachable through their own ModuleClassLoader, never the system classloader. That scan was guaranteed to fail for those modules, and pointless even when it succeeded: JavaModuleClassLoader#loadClass only consults this permission set for classes loaded by a non-module classloader, so module-to-module API access was never gated by it anyway. Restrict the scan to the engine module and any dev-mode classpath modules - the only ones whose classes are actually resolvable via the system classloader setupSandbox() uses. See MovingBlocks/gestalt#167, which downgrades the resulting ClassNotFoundException logging but doesn't address the underlying scan. Co-Authored-By: soloturn <soloturn@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughModuleManager now tracks successfully registered classpath modules and restricts sandbox API scanning to the engine module plus those tracked modules, excluding application-path modules. ChangesClasspath sandbox scanning
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
BenjaminAmos
left a comment
There was a problem hiding this comment.
@API exposure for non-classpath modules appears to be completely broken. Fortunately, it does not seem like any core modules make significant use of it.
Summary
setupSandbox()looped over every registered module and scanned each one'sclass index against the system classloader. That's only ever resolvable for
the engine module (and, in dev mode, modules explicitly loaded onto the JVM
classpath) — modules loaded from the application/module path live only in
their own
ModuleClassLoader, so the scan was guaranteed to fail for them.It was also pointless even on success:
JavaModuleClassLoader#loadClassonly consults this permission set for classes from a non-module
classloader, so it never gated module-to-module API access anyway.
modules, eliminating the routine
ClassNotFoundExceptionnoise at startupwithout changing sandbox behavior for anything that mattered.
Test plan
gradle :engine:compileJavapasses (verified locally)APIScannerno longer logs class-not-foundfor modules loaded from
modules/at startupRelated
doesn't address this underlying scan