Open
Conversation
4 tasks
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 39 minutes and 3 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughA new JRuby language extension module is added to the project, enabling Ruby script execution support. The implementation includes build configuration with embedded dependency management, a core extension class, language definition with JRuby ScriptingContainer lifecycle management, script context handling, and a library wrapper for converting Ruby methods to Java callables with both synchronous and asynchronous execution paths. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant JRubyExt as JRubyExtension
participant JRubyLang as JRubyLanguageDefinition
participant Container as ScriptingContainer
participant FWrapper as FWrapper
participant RubyRT as JRuby Runtime
Client->>JRubyExt: init()
JRubyExt->>JRubyExt: Start daemon thread
JRubyExt->>Container: Create ScriptingContainer
Container->>RubyRT: Initialize
JRubyExt->>Container: runScriptlet("p 'Ruby Pre-Loaded'")
Container->>RubyRT: Execute preload
JRubyExt->>Container: terminate()
Client->>JRubyExt: getLanguage()
JRubyExt->>JRubyLang: Construct (swap ClassLoader)
JRubyLang->>Container: Create ScriptingContainer
JRubyLang->>Container: setCurrentDirectory(cwd)
JRubyLang->>FWrapper: retrieveLibs()
FWrapper->>Container: Inject libraries
Client->>JRubyLang: exec(script)
JRubyLang->>JRubyLang: runInstance()
JRubyLang->>Container: runScriptlet(script, filename)
Container->>RubyRT: Execute Ruby code
RubyRT-->>Container: Return result
Client->>FWrapper: methodToJava(rubyMethod)
FWrapper->>FWrapper: Create RubyMethodWrapper
Note over FWrapper: Wrapper stores RubyMethod
Note over FWrapper: await=true for sync
Client->>FWrapper: wrapper.apply(args)
FWrapper->>FWrapper: inner_apply()
FWrapper->>RubyRT: Bind thread
FWrapper->>RubyRT: Call RubyMethod with Java args
RubyRT->>RubyRT: Convert IRubyObject return
RubyRT-->>FWrapper: Return Java Object
FWrapper->>RubyRT: Unbind thread
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
extension/ruby/src/main/java/com/jsmacrosce/jsmacros/jruby/client/JRubyExtension.java (1)
26-26: Static singleton state tieslanguageDefinitionto classloader lifetime.Using a
staticfield means theJRubyLanguageDefinition(and its cachedrunner) survives across Core reloads while the extension classloader is alive. IfCoreis ever recreated (e.g. config reload), subsequent calls togetLanguage(runner)return the definition bound to the old runner. Consider making it an instance field onJRubyExtension.This mirrors how other extensions may already do it, so it may be an intentional existing pattern — flagging for awareness rather than blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/ruby/src/main/java/com/jsmacrosce/jsmacros/jruby/client/JRubyExtension.java` at line 26, The static field languageDefinition in JRubyExtension holds JRubyLanguageDefinition (and its cached runner) for the classloader lifetime and can return a stale definition after Core reloads; change languageDefinition from a static to an instance field on JRubyExtension, update any references (e.g., getLanguage(runner) and any constructors or factory methods that access JRubyExtension.languageDefinition) to use the instance field, and ensure JRubyLanguageDefinition is created/initialized per JRubyExtension instance so the cached runner is bound to the current Core instance rather than the classloader.extension/ruby/build.gradle.kts (1)
22-23: Consider failing fast whenstonecutter.activeis empty/absent.
rootProject.file("stonecutter.active").readText().trim()will yield an empty string (or throw FNF) and thenproject(":common:")will resolve to an invalid path producing a confusing error. The rootstonecutter.gradle.kts(Line 173-179) already doesthrow GradleException("stonecutter.active is empty; set an active version first")— worth adopting the same guard here (and in the graalpy script too).This is the same pattern used by
extension/graal/python/build.gradle.kts, so any fix should be applied consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/ruby/build.gradle.kts` around lines 22 - 23, The assignment that computes val minecraftVersion via rootProject.file("stonecutter.active").readText().trim() should fail fast when the file is missing or empty: replace the direct read with a guard that checks existence and non-empty content and throws a GradleException with the same message used in stonecutter.gradle.kts ("stonecutter.active is empty; set an active version first") if the file is absent or yields an empty string; apply the same pattern you used in extension/graal/python/build.gradle.kts and the graalpy script so all places that compute minecraftVersion (the symbol minecraftVersion and the rootProject.file("stonecutter.active") read) behave consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@extension/ruby/src/main/java/com/jsmacrosce/jsmacros/jruby/client/JRubyExtension.java`:
- Around line 98-108: The traceStack method can NPE on
current.getFileName()==null and risks unbounded recursion; update traceStack to
guard the file name before constructing a File (use a null-check and only create
BaseWrappedException.GuestLocation with a File when current.getFileName() !=
null, otherwise fall back to HostLocation or a GuestLocation variant that omits
the File), and replace the recursive walker with an iterative loop that builds
the BaseWrappedException chain tail-first (or enforce a max depth) to avoid
StackOverflowError; locate and change the traceStack method and its creation of
BaseWrappedException.GuestLocation/HostLocation to implement these fixes.
- Around line 67-75: The lazy initialization of languageDefinition in
getLanguage is not thread-safe; change getLanguage to perform initialization
under a lock (simplest: make the method synchronized) so only one thread can
construct JRubyLanguageDefinition and perform the Thread context-classloader
swap; alternatively implement double-checked locking with languageDefinition
declared volatile and the JRubyLanguageDefinition construction inside the
synchronized block—ensure the context-classloader set/reset
(Thread.currentThread().setContextClassLoader(...)) remains inside the
synchronized region so initialization is atomic.
In
`@extension/ruby/src/main/java/com/jsmacrosce/jsmacros/jruby/language/impl/JRubyLanguageDefinition.java`:
- Around line 60-65: The call in JRubyLanguageDefinition that passes
ctx.getCtx().getFile().getParentFile().toPath() into the execution context can
NPE because getParentFile() may be null even when getFile() is non-null; change
the ternary to first obtain File f = ctx.getCtx().getFile(), then File parent =
f == null ? null : f.getParentFile(), and pass parent != null ? parent.toPath()
: null to the runner; update the same pattern in the file-based overload
mentioned (the earlier runScriptlet overload) to guard against a null parent
directory as well.
- Line 49: Replace the FileReader usage in JRubyLanguageDefinition where
instance.runScriptlet(new FileReader(ctx.getCtx().getFile()),
ctx.getCtx().getFile().getAbsolutePath()) is called: open the file with
Files.newBufferedReader(ctx.getCtx().getFile().toPath(), StandardCharsets.UTF_8)
and pass that Reader to instance.runScriptlet(Reader, String) inside a
try-with-resources (or explicitly close the Reader) so the reader is correctly
UTF-8 decoded and always closed to avoid file-handle leaks.
In
`@extension/ruby/src/main/java/com/jsmacrosce/jsmacros/jruby/library/impl/FWrapper.java`:
- Around line 134-136: The compare method in FWrapper currently casts the Object
result of inner_apply(o1, o2) directly to int causing ClassCastException when
JRuby returns a Long; change compare in class FWrapper to obtain the numeric
result as a Number and call intValue() (e.g., call inner_apply(...) into a
Number variable and return number.intValue()), ensuring you handle nulls or
non-number returns appropriately; this references the methods FWrapper.compare
and inner_apply (and the Ruby-to-Java conversion performed by callFn).
- Around line 48-52: callFn pushes a new JRuby scope via
ThreadContext.pushNewScope but never pops it, causing scope stack leaks when
invoked from apply/test/compare/accept; fix by wrapping the call in a
try/finally that always pops the scope (use ThreadContext.popScope() or the
matching pop method on ThreadContext) so the scope is removed even on
exceptions, i.e., in callFn obtain the ThreadContext, pushNewScope, then in
finally call the matching pop method before returning or rethrowing.
---
Nitpick comments:
In `@extension/ruby/build.gradle.kts`:
- Around line 22-23: The assignment that computes val minecraftVersion via
rootProject.file("stonecutter.active").readText().trim() should fail fast when
the file is missing or empty: replace the direct read with a guard that checks
existence and non-empty content and throws a GradleException with the same
message used in stonecutter.gradle.kts ("stonecutter.active is empty; set an
active version first") if the file is absent or yields an empty string; apply
the same pattern you used in extension/graal/python/build.gradle.kts and the
graalpy script so all places that compute minecraftVersion (the symbol
minecraftVersion and the rootProject.file("stonecutter.active") read) behave
consistently.
In
`@extension/ruby/src/main/java/com/jsmacrosce/jsmacros/jruby/client/JRubyExtension.java`:
- Line 26: The static field languageDefinition in JRubyExtension holds
JRubyLanguageDefinition (and its cached runner) for the classloader lifetime and
can return a stale definition after Core reloads; change languageDefinition from
a static to an instance field on JRubyExtension, update any references (e.g.,
getLanguage(runner) and any constructors or factory methods that access
JRubyExtension.languageDefinition) to use the instance field, and ensure
JRubyLanguageDefinition is created/initialized per JRubyExtension instance so
the cached runner is bound to the current Core instance rather than the
classloader.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 621f874c-a6d9-4769-907d-7ef831ca23f0
📒 Files selected for processing (9)
extension/ruby/build.gradle.ktsextension/ruby/src/main/java/com/jsmacrosce/jsmacros/jruby/client/JRubyExtension.javaextension/ruby/src/main/java/com/jsmacrosce/jsmacros/jruby/language/impl/JRubyLanguageDefinition.javaextension/ruby/src/main/java/com/jsmacrosce/jsmacros/jruby/language/impl/JRubyScriptContext.javaextension/ruby/src/main/java/com/jsmacrosce/jsmacros/jruby/library/impl/FWrapper.javaextension/ruby/src/main/resources/META-INF/services/com.jsmacrosce.jsmacros.core.extensions.Extensionextension/ruby/src/main/resources/jsmacrosce.ext.jruby.jsonsettings.gradle.ktsstonecutter.gradle.kts
Adds a JRuby 9.4.5.0 language extension as :extension:ruby, providing .rb script support. New ExtensionSpec(extId = "jruby") entry in stonecutter.gradle.kts wires it into the multi-version build. JRuby is embedded into the output jar under META-INF/jsmacroscedeps/. Port of grepsedawk/JsMacros-Ruby (an unofficial 1.21.8 patch of wagyourtail's original jsmacros-ruby), adapted to CE's API: split LanguageExtension / LibraryExtension interfaces, Core<?, ?> generic on language/context constructors, non-singleton profile access (ctx.runner.profile rather than Core.getInstance().profile). Script bindings are exposed as local variables context / event / file (no dollar prefix). This restores the pre-139c8ce wagyourtail convention — legacy scripts that reference bare "context" break under the post-139c8ce global-only form. Fixes applied atop the straight port: - Daemon flags + names on the preload thread and the JavaWrapper async thread (upstream spawned non-daemon threads that could prevent JVM shutdown). - Removed a dead Semaphore + unused local in FWrapper.inner_accept. - Extracted a shared callFn helper to deduplicate the three near-identical JRuby-call blocks in FWrapper.RubyMethodWrapper. Polish per PR review: - JRubyExtension.getLanguage: synchronized lazy init to remove the classloader-swap race between concurrent callers. - JRubyExtension.wrapException: null-guard StackTraceElement.getFileName (can be null for synthetic frames) and rewrite the recursive walker iteratively so deep JRuby backtraces cannot StackOverflowError. - JRubyLanguageDefinition: read script files with Files.newBufferedReader(UTF_8) inside try-with-resources instead of the platform-default, never-closed FileReader. - JRubyLanguageDefinition: parentPathOf helper null-guards both getFile() and getParentFile() so bare filenames do not NPE. - JRubyLanguageDefinition: move event/file/context bindings into runInstance; both exec overloads just hand in a scriptlet runner. - FWrapper.callFn: pop the pushed JRuby scope in a finally so the scope stack does not accumulate across callback invocations. - FWrapper.compare: convert the Ruby result via Number.intValue instead of an unchecked Object to int cast (JRuby returns Long). - FWrapper: drop the redundant await parameter threaded through innerAccept — it was always this.await. Camel-case inner_accept / inner_apply. - build.gradle.kts: fail fast with a clear GradleException when stonecutter.active is missing or empty, matching the root script. Intentionally dropped from upstream: - JRubyConfig / useGlobalContext option — never registered upstream (the addOptions call was commented out) and JRubyLanguageDefinition never reads the option. - assets/jsmacros/jruby/lang/en_us.json — its only key was for the unregistered config option. Build matrix: :extension:ruby:build is verified on stonecutter.active = 1.21.8. Extension has no direct MC-API dependencies, so the same compiled classes are reused across stonecutter versions and will inherit 26.1 compatibility automatically once that lands. Runtime verification: in-process ServiceLoader smoke test via jshell on JDK 21 discovers JRubyExtension, resolves getExtensionName -> "jruby", minCoreVersion=2.0.0, maxCoreVersion=2.1.0, getDependencies returns the embedded jruby-complete-9.4.5.0.jar URL, implements both LanguageExtension and LibraryExtension, and ScriptingContainer resolves from the embedded jar. Not smoke-tested inside MC's run environment.
grepsedawk
force-pushed
the
feat/ruby-extension
branch
from
f742d59 to
cf43602
Compare
This was referenced Apr 23, 2026
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
1 participant
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.
Summary
Adds a JRuby 9.4.5 language extension as
:extension:ruby, providing.rbscript support. NewExtensionSpec(extId = "jruby")entry instonecutter.gradle.ktswires it into the multi-version build. JRuby is embedded into the output jar underMETA-INF/jsmacroscedeps/.Port of grepsedawk/JsMacros-Ruby (an unofficial 1.21.8 patch of wagyourtail's original
jsmacros-ruby), adapted to CE's API: splitLanguageExtension/LibraryExtensioninterfaces,Core<?, ?>generic on language/context constructors, non-singletonprofileaccess (ctx.runner.profilerather thanCore.getInstance().profile).Fixes applied atop the straight port
JavaWrapperasync thread (upstream spawned non-daemon threads that could prevent JVM shutdown).Semaphore+ unused local inFWrapper.inner_accept— the semaphore was released but never acquired.callFnhelper to deduplicate the three near-identical JRuby-call blocks inFWrapper.RubyMethodWrapper.Script bindings
Bindings exposed as local variables
context/event/file(no$prefix). This restores the pre-139c8ce wagyourtail convention. Legacy scripts that reference barecontextbreak under the post-139c8ce global-only form — this restores compatibility.Vestigial bits dropped from upstream
JRubyConfig/useGlobalContextoption — was never registered (theaddOptionscall was commented out) andJRubyLanguageDefinitionnever reads the option.assets/jsmacros/jruby/lang/en_us.json— its only key was for the unregistered config option.Build matrix
Built
:extension:ruby:jaragainst each stonecutter version:Extension has no direct MC-API dependencies, so the same compiled classes are reused across stonecutter versions and will inherit 26.1 compatibility automatically once that lands.
Testing
In-process ServiceLoader smoke test via
jshellon JDK 21:ServiceLoader.load(Extension.class, <classloader>)discoversJRubyExtensiongetExtensionName()→"jruby",minCoreVersion=2.0.0,maxCoreVersion=2.1.0getDependencies()returns the embeddedMETA-INF/jsmacroscedeps/jruby-complete-9.4.5.0.jarURLLanguageExtensionandLibraryExtensionorg.jruby.embed.ScriptingContainerresolves from itNot smoke-tested inside MC's run environment.
Test plan
.rbscript via the mod UI and verify it executesJavaWrapper.methodToJavacan register a Ruby callback that the host invokes