feat: use Java @-files for classpath to avoid avaje-jsonb 50K limit - #91
Conversation
Offload classpath and JVM options from RunSubprocess JSON messages into Java @-files (argument files), dramatically reducing message size. Java's launcher natively expands @filepath on the command line. - Add Argfile utility to write @-files with jvmOptions + classpath - Update all RunSubprocess sites: run, runMain, runMvnApp, repl, fix, fixCheck, tools (8 sites total) - Extend chunking from Output-only to Output+Log messages in CliClientSocketWriter (renamed MaxOutputChunkSize -> MaxMessageChunkSize) - Zero client changes needed Naming scheme: {key}-jvm-opts.txt (e.g. run-jvm-opts.txt, scalafix-jvm-opts.txt, <toolName>-jvm-opts.txt)
|
Warning Review limit reached
More reviews will be available in 32 minutes and 52 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughIntroduces a new ChangesArgfile utility and subprocess migration
CliClientSocketWriter chunking refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/ba/sake/deder/Argfile.scala`:
- Around line 35-36: The jvmOptions and classpath variables are being written
unescaped to the `@argfile`, which causes Java's argument file parser to treat
spaces and special characters as delimiters, breaking arguments that contain
whitespace. Modify the code where lines is constructed to properly escape each
argument in jvmOptions and classpath before concatenating them. Specifically,
before calling mkString on the lines sequence, apply appropriate escaping or
quoting to each element to ensure spaces and special characters are preserved
when the file is parsed by Java.
In `@server/src/ba/sake/deder/cli/CliClientSocketWriter.scala`:
- Around line 28-31: The text chunking at the grouped(MaxMessageChunkSize) call
and the text length guards do not account for JSON escaping overhead. Characters
like quotes and backslashes that require escaping in JSON can cause the final
serialized JSON output to exceed the intended size limit even though the raw
text passed the length check. Fix this by validating the actual JSON-serialized
size after calling makeMsg(chunk).toJson() instead of relying on raw text.length
checks. This applies both to the chunking logic in the foreach block where text
is grouped and to any guard conditions that validate text.length before
serialization (around lines 41-43).
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ffd7664-3f62-47a9-a906-760cc543b2fb
📒 Files selected for processing (6)
server/src/ba/sake/deder/Argfile.scalaserver/src/ba/sake/deder/CoreTasks.scalaserver/src/ba/sake/deder/RunTasks.scalaserver/src/ba/sake/deder/cli/CliClientMessageHandler.scalaserver/src/ba/sake/deder/cli/CliClientSocketWriter.scalaserver/test/src/ba/sake/deder/ArgfileSuite.scala
| text.grouped(MaxMessageChunkSize).foreach { chunk => | ||
| val json = makeMsg(chunk).toJson(spaces = 0, sort = false) | ||
| outputStream.write((json + '\n').getBytes(StandardCharsets.UTF_8)) | ||
| } |
There was a problem hiding this comment.
Chunking by raw text length can still exceed the JSON size limit after escaping.
At Line 41 and Line 43, the guard uses text.length, and at Line 28 chunking is also grouped(MaxMessageChunkSize). This does not account for JSON escaping overhead, so chunks with many escaped characters can still serialize past the intended limit (Line 29), defeating the avaje-jsonb protection goal.
Suggested fix
- private val MaxMessageChunkSize = 30_000
+ private val MaxMessageChunkSize = 30_000
@@
private def writeChunks(
text: String,
makeMsg: String => CliServerMessage,
outputStream: java.io.OutputStream
): Unit =
- text.grouped(MaxMessageChunkSize).foreach { chunk =>
- val json = makeMsg(chunk).toJson(spaces = 0, sort = false)
- outputStream.write((json + '\n').getBytes(StandardCharsets.UTF_8))
- }
+ var i = 0
+ while i < text.length do
+ var lo = 1
+ var hi = math.min(MaxMessageChunkSize, text.length - i)
+ var best = 1
+ while lo <= hi do
+ val mid = (lo + hi) >>> 1
+ val candidate = text.substring(i, i + mid)
+ val json = makeMsg(candidate).toJson(spaces = 0, sort = false)
+ if json.length <= MaxMessageChunkSize then
+ best = mid
+ lo = mid + 1
+ else hi = mid - 1
+ val chunk = text.substring(i, i + best)
+ val json = makeMsg(chunk).toJson(spaces = 0, sort = false)
+ outputStream.write((json + '\n').getBytes(StandardCharsets.UTF_8))
+ i += best
@@
- case CliServerMessage.Output(text) if text.length > MaxMessageChunkSize =>
+ case CliServerMessage.Output(text)
+ if CliServerMessage.Output(text).toJson(spaces = 0, sort = false).length > MaxMessageChunkSize =>
writeChunks(text, CliServerMessage.Output(_), outputStream)
- case CliServerMessage.Log(text, level) if text.length > MaxMessageChunkSize =>
+ case CliServerMessage.Log(text, level)
+ if CliServerMessage.Log(text, level).toJson(spaces = 0, sort = false).length > MaxMessageChunkSize =>
writeChunks(text, CliServerMessage.Log(_, level), outputStream)Also applies to: 41-44
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/ba/sake/deder/cli/CliClientSocketWriter.scala` around lines 28 -
31, The text chunking at the grouped(MaxMessageChunkSize) call and the text
length guards do not account for JSON escaping overhead. Characters like quotes
and backslashes that require escaping in JSON can cause the final serialized
JSON output to exceed the intended size limit even though the raw text passed
the length check. Fix this by validating the actual JSON-serialized size after
calling makeMsg(chunk).toJson() instead of relying on raw text.length checks.
This applies both to the chunking logic in the foreach block where text is
grouped and to any guard conditions that validate text.length before
serialization (around lines 41-43).
1 participant
Offload classpath and JVM options from RunSubprocess JSON messages into Java @-files (argument files), dramatically reducing message size. Java's launcher natively expands @filepath on the command line.
Naming scheme: {key}-jvm-opts.txt (e.g. run-jvm-opts.txt, scalafix-jvm-opts.txt, -jvm-opts.txt)
Summary by CodeRabbit