Improve gradle files - #356
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughGradle centralizes Java module arguments, toolchain selection, native-image configuration, Makefile testing, formatting, PMD, and Javadoc validation. Build caching, dependency declarations, and wrapper verification are updated. CI separates Spotless and native checks. The README documents Java 21 requirements and Gradle properties. Merge scripts now share native executable and Java fat-jar dispatch logic. Test classes are made package-private and final. Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 10
🤖 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 @.github/workflows/claude-code-review.yml:
- Line 36: Pin all referenced workflow actions to reviewed full immutable commit
SHAs: update actions/checkout in .github/workflows/claude-code-review.yml lines
36 and .github/workflows/claude.yml line 45, and anthropics/claude-code-action
in both files at line 51. Preserve automated dependency updates through
Dependabot.
- Around line 8-13: Add per-pull-request concurrency to the workflow triggered
by pull_request_target, using a group that uniquely identifies the PR and
setting cancel-in-progress to true so superseded synchronize runs are canceled
while preserving the existing event types.
In @.github/workflows/claude.yml:
- Around line 44-47: Update the “Checkout repository” step using
actions/checkout@v7 to set persist-credentials to false alongside fetch-depth,
preventing checkout credentials from remaining available to the subsequent
Claude action.
In `@build.gradle`:
- Around line 160-170: Update the doFirst block for the relevant Exec task to
set JAVA21_HOME to the same resolved Java 21 toolchain path as JAVA_HOME,
preventing an ambient JAVA21_HOME from overriding the selected toolchain while
preserving lazy provisioning via java21Home.get().
- Around line 32-45: Update the repositories block to restore a resolver that
hosts org.plumelib:require-javadoc:3.0.0-SNAPSHOT, while preserving the existing
Maven Central and snapshot-only repository configuration. Ensure
requireJavadoc(libs.require.javadoc) can resolve the declared artifact during
the check task.
- Around line 345-365: Guard the recursive scan in the Javadoc task’s doLast
block before calling destinationDir.eachFileRecurse: check that destinationDir
exists and is a directory, and skip the stylesheet cleanup when it is
unavailable. Preserve the existing CSS import removal behavior for valid
destination directories.
- Around line 107-111: Update the Java toolchain setup around javaLauncher by
resolving testJavaVersion or the system Java specification version before the
launcherFor spec executes, then pass that captured value to
JavaLanguageVersion.of(String). Remove the project property lookup and manual
Integer.parseInt call from the launcherFor closure while preserving the existing
fallback order.
In `@src/main/sh/plumelib-merge-common.sh`:
- Around line 82-92: Make the `javacInternalPackages` export list in the shell
script mechanically verifiable against the corresponding Gradle value,
preferably by generating the `--add-exports` arguments from the six package
names or adding a CI assertion that compares both lists. Preserve the existing
Java invocation and ensure future list drift causes a clear validation failure.
- Around line 41-43: Update run_plumelib_merge so its temporary variables,
including subcommand, java_home, and java_command, cannot collide with or
unintentionally overwrite variables in a sourcing script; use a consistent
plumelib_ prefix for these function-local names and update all references within
the function.
- Around line 55-61: Update the java_home selection logic to use JAVA21_HOME
when it is set, including when JAVA_HOME is unset, while preserving the existing
behavior of preferring JAVA21_HOME when it differs from JAVA_HOME and falling
back to JAVA_HOME otherwise. Ensure the flow matches the error message’s
contract that JAVA21_HOME alone is sufficient.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 64df544d-771e-4669-84e6-97fc7a00b669
📒 Files selected for processing (12)
.github/workflows/claude-code-review.yml.github/workflows/claude.yml.github/workflows/gradle-native-compile.ymlREADME.mdbuild.gradlegradle.propertiesgradle/libs.versions.tomlsettings.gradlesrc/main/sh/merge-driver.shsrc/main/sh/merge-tool.shsrc/main/sh/plumelib-merge-common.shsrc/test/resources/Makefile
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/claude-code-review.yml (1)
52-60: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winDo not run Claude from the privileged PR checkout.
The
pull_request_targetjob checks out the untrustedpull_request.head.shainto the workspace root, then runs Claude in that tree while the job has write token permissions andCLAUDE_CODE_OAUTH_TOKEN.persist-credentials: falseonly prevents Git from persisting credentials; it does not isolate the workspace. Keep the trusted base checkout at the workspace root, check out the PR intopath: pr-head, and pass it with--add-dir pr-head.Proposed isolation
- uses: actions/checkout@v7 with: - ref: ${{ github.event.pull_request.head.sha }} + fetch-depth: 1 persist-credentials: false + + - uses: actions/checkout@v7 + with: + ref: ${{ github.event.pull_request.head.sha }} + path: pr-head + persist-credentials: false ... claude_args: | + --add-dir pr-head --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr *)"🤖 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 @.github/workflows/claude-code-review.yml around lines 52 - 60, Update the checkout configuration in the workflow so the trusted base revision remains at the workspace root and the untrusted pull request SHA is checked out under path pr-head. Update the Claude invocation to include --add-dir pr-head, ensuring Claude runs with the PR code as an added directory rather than replacing the privileged root checkout; keep persist-credentials: false..github/actions/spotless-p2/action.yml (1)
1-74: 🧹 Nitpick | 🔵 TrivialHeads-up: GrEclipse formatter's future in Spotless is uncertain.
This action, the retry script, and the
build.gradlecache wiring invest meaningfully in working aroundgreclipse()'s P2-provisioning flakiness. Per Spotless maintainers, GrEclipse has known package-metadata incompatibilities with the current P2 resolution layer (Solstice), with a possible deprecation warning around mid-2026 and removal roughly two months after that if unresolved. Worth tracking upstream so this provisioning investment doesn't become moot, and having a migration plan (e.g., another Groovy formatter) as a fallback.🤖 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 @.github/actions/spotless-p2/action.yml around lines 1 - 74, Track Spotless upstream’s GrEclipse/Solstice compatibility and deprecation status, and document a migration plan for replacing greclipse() if it is deprecated or removed. Keep the current caching and retry implementation unchanged until an alternative formatter is selected and validated.
🤖 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 `@src/main/sh/plumelib-merge-common.sh`:
- Around line 77-86: Update the Java home selection logic in the shown
conditional so JAVA21_HOME is selected whenever it is set and differs from an
explicitly set JAVA_HOME, while also allowing JAVA21_HOME alone to be used when
JAVA_HOME is unset. Preserve PLUMELIB_MERGE_JAVA_HOME precedence and the
existing JAVA_HOME fallback.
---
Outside diff comments:
In @.github/actions/spotless-p2/action.yml:
- Around line 1-74: Track Spotless upstream’s GrEclipse/Solstice compatibility
and deprecation status, and document a migration plan for replacing greclipse()
if it is deprecated or removed. Keep the current caching and retry
implementation unchanged until an alternative formatter is selected and
validated.
In @.github/workflows/claude-code-review.yml:
- Around line 52-60: Update the checkout configuration in the workflow so the
trusted base revision remains at the workspace root and the untrusted pull
request SHA is checked out under path pr-head. Update the Claude invocation to
include --add-dir pr-head, ensuring Claude runs with the PR code as an added
directory rather than replacing the privileged root checkout; keep
persist-credentials: false.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 10199769-0722-4fd9-875c-be69496bef1a
📒 Files selected for processing (12)
.github/actions/spotless-p2/action.yml.github/scripts/provision-spotless-p2.sh.github/workflows/claude-code-review.yml.github/workflows/claude.yml.github/workflows/gradle-native-compile.yml.github/workflows/gradle.ymlREADME.mdbuild.gradlegradle.propertiesgradle/libs.versions.tomlsettings.gradlesrc/main/sh/plumelib-merge-common.sh
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/claude-code-review.yml (1)
97-97: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winScope the
gh prBash allowlist to read-only review commands.
Bash(gh pr *)allows anygh prsubcommand and, due shell-command-string semantics, can enable command chaining after the matched token. Since the job and token are write-enabled forpull-requests, an injected PR comment could steer Claude toward unintended mutations like editing, linking, or closing PRs. Gate only the intended read/query commands plus the inline comment tool, and avoid allowing genericBash(...).🤖 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 @.github/workflows/claude-code-review.yml at line 97, Update the allowedTools configuration in the workflow to replace the broad Bash(gh pr *) pattern with explicit read-only gh pr subcommands required by the review job, while retaining mcp__github_inline_comment__create_inline_comment. Do not allow generic Bash or write-capable PR commands such as edit, comment, close, or link.Source: MCP tools
🤖 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 @.github/workflows/claude-code-review.yml:
- Line 27: Update the pull_request_target workflow around the author_association
condition and checkout steps so the primary workspace remains checked out at the
trusted base revision. Check out the pull request head into a separate
directory, then pass that directory to Claude using --add-dir; do not replace
the base workspace with head.sha, while preserving the existing permission and
trigger behavior.
In @.github/workflows/gradle-native-compile.yml:
- Line 5: Add workflow-level concurrency settings near the existing “on” trigger
in the native compile workflow, using a group keyed by workflow name and ref.
Enable cancellation of in-progress runs for non-release branches while
preserving concurrent runs for release branches.
In @.github/workflows/gradle.yml:
- Around line 29-31: Update the cache path in the Gradle workflow’s cache
configuration to ~/.gradle/spotless-p2, matching the directory configured by
build.gradle for Greclipse’s Spotless P2 cache; leave the existing cache key and
restore-keys unchanged.
- Line 27: Update the actions/cache reference in the workflow to use a
full-length immutable commit SHA instead of the mutable v6 tag, optionally
retaining the version as a trailing comment.
In `@build.gradle`:
- Around line 134-135: Update the JUnit test task’s javaLauncher configuration
to use the existing testJavaVersion value instead of
Runtime.version().feature(), ensuring -PtestJavaVersion selects the launcher
Java version for both test modes.
---
Outside diff comments:
In @.github/workflows/claude-code-review.yml:
- Line 97: Update the allowedTools configuration in the workflow to replace the
broad Bash(gh pr *) pattern with explicit read-only gh pr subcommands required
by the review job, while retaining
mcp__github_inline_comment__create_inline_comment. Do not allow generic Bash or
write-capable PR commands such as edit, comment, close, or link.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 62282529-2060-4690-a1c4-f96793f221cc
📒 Files selected for processing (11)
.github/workflows/claude-code-review.yml.github/workflows/claude.yml.github/workflows/gradle-native-compile.yml.github/workflows/gradle.ymlREADME.mdbuild.gradlegradle/libs.versions.tomlsrc/main/sh/merge-driver.shsrc/main/sh/merge-tool.shsrc/main/sh/plumelib-merge-common.shsrc/test/resources/Makefile
| check-latest: true | ||
| cache: 'gradle' | ||
| - name: Cache Spotless p2 data | ||
| uses: actions/cache@v6 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files .github/workflows | sed -n '1,120p'
echo "== gradle workflow relevant lines =="
if [ -f .github/workflows/gradle.yml ]; then
nl -ba .github/workflows/gradle.yml | sed -n '1,80p'
fi
echo "== usages of action references =="
rg -n 'uses:\s*[^#]+' .github/workflows || true
echo "== security/action lint config candidates =="
rg -n 'zizmor|audit|ghaction|actionlint|pinned|SHAs|commit-oid|actions/cache' -S .github . 2>/dev/null \
| sed -n '1,200p' || trueRepository: plume-lib/merging
Length of output: 415
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== gradle workflow first 100 lines =="
awk '{printf "%7d\t%s\n", NR, $0}' .github/workflows/gradle.yml | sed -n '1,120p'
echo "== all uses lines =="
grep -RInE '^\s*uses:\s*' .github/workflows | sed -n '1,200p'
echo "== workflow security references =="
grep -RInE 'zizmor|pin|SHA|commit|pinned|actions/cache|dangerous-clones|script-injection' .github . 2>/dev/null | sed -n '1,200p'Repository: plume-lib/merging
Length of output: 6491
🌐 Web query:
GitHub actions security best practices pin actions full Git SHA actions/cache mutable tag
💡 Result:
To secure your GitHub Actions workflows against supply chain attacks and ensure build reproducibility, you should pin all third-party actions to their full-length (40-character) Git commit SHA rather than using mutable version tags or branch names [1][2][3]. Why Pin to Full Git SHA Version tags (e.g., @v4) and branches (e.g., @main) are mutable pointers that can be re-targeted at any time by repository maintainers [2][4][3]. If a maintainer's account is compromised, an attacker can push malicious code to an existing tag, causing your workflows to execute unauthorized code with your pipeline's secrets and permissions [2][5][4]. Pinning to a specific, full-length commit SHA makes the reference immutable; your workflow will continue to execute the exact code you audited, regardless of changes upstream [1][4][3]. Recommended Implementation Use the full 40-character SHA and include the human-readable version as a trailing comment [2][6][7]. This allows dependency management tools like Dependabot and Renovate to continue tracking updates while maintaining security [2][5][7]. Example: - uses: actions/cache@cdf6c1f # v5.0.3 GitHub allows organization and repository administrators to enforce this practice via the allowed actions policy, which can mandate that all actions must be pinned to full commit SHAs [1][8][9]. Note on GitHub Actions Cache The actions/cache action itself is subject to similar security considerations. It is critical to note that cache keys are also mutable labels [10]. An attacker with execution access to a workflow can potentially overwrite cache entries, leading to "cache poisoning" [11][10]. GitHub has mitigated this by implementing read-only cache tokens for untrusted triggers (e.g., events that can be triggered by non-collaborators) [11]. To further protect your environment, minimize the privileges granted to workflows, pin your actions to immutable SHAs, and ensure your cache management strategies align with the principle of least privilege [3][11][10].
Citations:
- 1: https://docs.github.com/en/actions/reference/security/secure-use
- 2: https://starsling.dev/best-practices/github-actions/pin-action-shas
- 3: https://samsond.github.io/posts/why-tags-fail/
- 4: https://safeguard.sh/resources/blog/github-actions-pinning-sha-vs-tag
- 5: https://emmer.dev/blog/pin-your-github-actions-to-protect-against-mutability/
- 6: https://safeguard.sh/resources/blog/how-to-pin-github-actions-to-shas-correctly
- 7: Enable immutable releases actions/cache#1709
- 8: https://github.blog/changelog/2025-08-15-github-actions-policy-now-supports-blocking-and-sha-pinning-actions/
- 9: https://www.romainlespinasse.dev/posts/github-actions-commit-sha-pinning/
- 10: https://pipebreach.com/posts/cache-poisoning-ci-cd-/
- 11: https://github.blog/changelog/2026-06-26-read-only-actions-cache-for-untrusted-triggers/
Pin actions/cache to a full commit SHA.
actions/cache@v6 is a mutable tag, so the workflow can execute code different from what was reviewed. Replace it with a full-length commit SHA used as the uses reference, keeping the version as a trailing comment if one is needed.
🧰 Tools
🪛 zizmor (1.26.1)
[error] 27-27: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/gradle.yml at line 27, Update the actions/cache reference
in the workflow to use a full-length immutable commit SHA instead of the mutable
v6 tag, optionally retaining the version as a trailing comment.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/claude-code-review.yml:
- Around line 92-102: Correct the comments near persist-credentials and
use_commit_signing in both workflow sections (including the later occurrence):
state that persist-credentials: false prevents checkout credentials from being
written to .git/config, and that use_commit_signing: true uses the GitHub
Contents API commit path instead of git CLI commits, avoiding credential writes
through that path. Remove the inaccurate claim that commit signing suppresses an
existing git config rewrite.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b19d02d4-1d70-42da-b3b8-af06672a2f39
📒 Files selected for processing (1)
.github/workflows/claude-code-review.yml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@build.gradle`:
- Around line 103-109: Define the missing Java-version matrix in the native CI
workflow configuration consumed by graalvm/setup-graalvm, using the expected
matrix key and supported versions such as 21 and 25. Ensure the existing
nativeCompile configuration in graalvmNative remains unchanged and the setup
reference matches the defined matrix key.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0ef06186-df40-4576-a097-8b4caf7bb6da
📒 Files selected for processing (4)
build.gradlegradle.propertiesgradle/libs.versions.tomlsettings.gradle
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.github/workflows/gradle.yml:
- Around line 12-16: Add a workflow-level permissions block in
.github/workflows/gradle.yml granting only read access needed for checkout and
spotlessCheck, so both spotless and build jobs use least-privilege GITHUB_TOKEN
permissions.
In @.github/workflows/prek.yaml:
- Line 27: Pin all four actions/cache references to the same full-length commit
SHA instead of the mutable v6 tag, preserving the v6 version as a trailing
comment: .github/workflows/prek.yaml:27 (restore),
.github/workflows/gradle.yml:49 (restore), .github/workflows/gradle.yml:74
(save), and .github/workflows/gradle.yml:90 (save).
In `@build.gradle`:
- Around line 313-343: Harden deleteStaleLockFile against concurrent lock-file
changes: catch IOException from lockFile.getText and return when the file
disappears or cannot be read, treating a missing lock as the desired state.
Before deleting a lock associated with a dead PID, re-read its contents
immediately before lockFile.delete and delete only if it still contains the same
PID; otherwise leave the replacement lock untouched.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 16cfcb2b-7093-4456-8d7f-d91caba320dd
📒 Files selected for processing (7)
.github/workflows/gradle-native-compile.yml.github/workflows/gradle.yml.github/workflows/prek.yamlbuild.gradlegradle.propertiesprek.tomlsrc/main/sh/plumelib-merge-common.sh
| # a partial entry can carry a stale lock file, which build.gradle's | ||
| # "deleteStaleSpotlessP2Lock" task deletes. | ||
| - name: Restore Spotless P2 cache | ||
| uses: actions/cache/restore@v6 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Every new actions/cache reference uses a mutable tag. This PR adds four actions/cache@v6 references across two workflows. A tag can be retargeted upstream, so each of these jobs can execute code that was never reviewed while holding the default GITHUB_TOKEN. The shared fix is to pin each reference to a full-length commit SHA and keep the version as a trailing comment.
.github/workflows/prek.yaml#L27-L27: pinactions/cache/restore@v6to a full commit SHA..github/workflows/gradle.yml#L49-L49: pinactions/cache/restore@v6to the same commit SHA..github/workflows/gradle.yml#L74-L74: pinactions/cache/save@v6to a full commit SHA..github/workflows/gradle.yml#L90-L90: pinactions/cache/save@v6to the same commit SHA.
🧰 Tools
🪛 zizmor (1.29.0)
[error] 27-27: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
📍 Affects 2 files
.github/workflows/prek.yaml#L27-L27(this comment).github/workflows/gradle.yml#L49-L49.github/workflows/gradle.yml#L74-L74.github/workflows/gradle.yml#L90-L90
🤖 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 @.github/workflows/prek.yaml at line 27, Pin all four actions/cache
references to the same full-length commit SHA instead of the mutable v6 tag,
preserving the v6 version as a trailing comment: .github/workflows/prek.yaml:27
(restore), .github/workflows/gradle.yml:49 (restore),
.github/workflows/gradle.yml:74 (save), and .github/workflows/gradle.yml:90
(save).
Source: Linters/SAST tools
| static void deleteStaleLockFile(File cacheDirectory, Logger logger) { | ||
| File lockFile = new File(new File(cacheDirectory, "metadata"), ".lock") | ||
| if (!lockFile.isFile()) { | ||
| return | ||
| } | ||
| long pid | ||
| try { | ||
| pid = Long.parseLong(lockFile.getText("UTF-8").trim()) | ||
| } catch (NumberFormatException e) { | ||
| // Solstice creates the lock file and then writes its PID into it, so a lock file that holds | ||
| // no PID usually belongs to a live process that is between those two steps. A process that | ||
| // is killed between them leaves the file empty forever, and Solstice parses the file outside | ||
| // the "try" block that handles a lock it cannot take, so every later build then dies with a | ||
| // NumberFormatException. Wait out the race before assuming that this is the latter case. | ||
| if (System.currentTimeMillis() - lockFile.lastModified() > EMPTY_LOCK_FILE_GRACE_MILLIS | ||
| && lockFile.delete()) { | ||
| logger.lifecycle("Deleted abandoned Spotless P2 lock file that holds no PID: ${lockFile}") | ||
| } | ||
| return | ||
| } | ||
| // A PID that the operating system has recycled looks like the original owner, which leaves the | ||
| // lock file in place. That is the behavior that this method is avoiding, but erring in this | ||
| // direction merely fails to improve on Solstice, whereas deleting a live process's lock file | ||
| // would let two processes write the cache at once. | ||
| if (ProcessHandle.of(pid).isPresent()) { | ||
| return | ||
| } | ||
| if (lockFile.delete()) { | ||
| logger.lifecycle("Deleted stale Spotless P2 lock file of dead process ${pid}: ${lockFile}") | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Harden deleteStaleLockFile against concurrent access to the lock file.
Two problems exist in this method, and both come from reading and deleting the lock file in separate, unsynchronized steps.
-
Delete-after-check race. Between
ProcessHandle.of(pid).isPresent()at line 337 andlockFile.delete()at line 340, another build can observe the same stale lock, delete it, and take a fresh lock. This method then deletes that live process's lock file, which is the exact outcome that the comment at lines 333-336 says must not happen. Re-read the file immediately before deleting and confirm that it still holds the same dead PID. -
Unhandled
IOException.lockFile.getText("UTF-8")at line 320 catches onlyNumberFormatException. If another build deletes the lock file between theisFile()check at line 315 and the read,getTextthrowsFileNotFoundException, and everyspotlessGroovyGradle*task fails through thedependsOnat line 364. A missing lock file is the desired state, so it must not fail the build.
🛡️ Proposed fix
long pid
try {
pid = Long.parseLong(lockFile.getText("UTF-8").trim())
+ } catch (IOException e) {
+ // Another process deleted the lock file, or it became unreadable. Either way, there is
+ // nothing to clean up.
+ return
} catch (NumberFormatException e) { if (ProcessHandle.of(pid).isPresent()) {
return
}
+ // Another process may have taken the lock since the read above. Confirm that the file still
+ // names the same dead process before deleting it.
+ String current
+ try {
+ current = lockFile.getText("UTF-8").trim()
+ } catch (IOException e) {
+ return
+ }
+ if (current != Long.toString(pid)) {
+ return
+ }
if (lockFile.delete()) {🤖 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 `@build.gradle` around lines 313 - 343, Harden deleteStaleLockFile against
concurrent lock-file changes: catch IOException from lockFile.getText and return
when the file disappears or cannot be read, treating a missing lock as the
desired state. Before deleting a lock associated with a dead PID, re-read its
contents immediately before lockFile.delete and delete only if it still contains
the same PID; otherwise leave the replacement lock untouched.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/sh/plumelib-merge-common.sh (1)
89-101: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftValidate the selected Java major version before launching the fat jar.
plumelib_java_commandis validated to be executable, but the jar’s class files require Java 21; Java 17 or earlier fromPLUMELIB_MERGE_JAVA_HOME,JAVA21_HOME,JAVA_HOME, orPATHcan pass and fail at launch withUnsupportedClassVersionError. Add a major-version check after selecting the command and before invoking the jar, and report the selected source variable/path when the runtime is below Java 21 because some callers ignore this script’s status.🤖 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 `@src/main/sh/plumelib-merge-common.sh` around lines 89 - 101, After the Java command selection in the surrounding merge flow, validate that plumelib_java_command is executable and reports a major version of at least 21 before launching the fat jar. Parse the runtime version from the selected command, and when it is below 21, emit an explicit error naming the selected source variable or path and return a failure status so callers that ignore the script status still receive a clear diagnostic.Source: MCP tools
🤖 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 `@src/main/sh/plumelib-merge-common.sh`:
- Around line 18-22: Update the validation checks for PLUMELIB_MERGE_EXECUTABLE
and the Java launcher paths at all three validation sites to require both a
regular file and executable permissions. Preserve each existing non-empty path
guard, and add the regular-file check before the existing executable check so
directories are rejected during validation.
---
Outside diff comments:
In `@src/main/sh/plumelib-merge-common.sh`:
- Around line 89-101: After the Java command selection in the surrounding merge
flow, validate that plumelib_java_command is executable and reports a major
version of at least 21 before launching the fat jar. Parse the runtime version
from the selected command, and when it is below 21, emit an explicit error
naming the selected source variable or path and return a failure status so
callers that ignore the script status still receive a clear diagnostic.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a5f63f69-95e1-42ff-8516-7842eda20000
📒 Files selected for processing (6)
.github/workflows/gradle.yml.github/workflows/prek.yamlbuild.gradlegradle/libs.versions.tomlsrc/main/sh/plumelib-merge-common.shsrc/test/resources/Makefile
💤 Files with no reviewable changes (1)
- gradle/libs.versions.toml
| plumelib_executable="$PLUMELIB_MERGE_EXECUTABLE" | ||
| if [ -n "$plumelib_executable" ] && [ ! -x "$plumelib_executable" ]; then | ||
| echo "$0: PLUMELIB_MERGE_EXECUTABLE is not an executable file: $plumelib_executable" >&2 | ||
| exit 2 | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
file=src/main/sh/plumelib-merge-common.sh
tmp=$(mktemp -d)
trap 'rm -rf "$tmp"' EXIT
mkdir "$tmp/native-dir" "$tmp/java-dir"
chmod 755 "$tmp/native-dir" "$tmp/java-dir"
test -x "$tmp/native-dir"
test ! -f "$tmp/native-dir"
for range in '18,22' '50,55' '107,114'; do
if ! sed -n "${range}p" "$file" | rg -q -- '-f'; then
echo "Missing regular-file check in lines ${range}" >&2
exit 1
fi
doneRepository: plume-lib/merging
Length of output: 197
🏁 Script executed:
#!/bin/sh
set -eu
printf 'Relevant file excerpt:\n'
sed -n '1,130p' src/main/sh/plumelib-merge-common.sh
printf '\nShell test behavior for directory with execute permission:\n'
tmp=$ (mktemp -d)
trap 'rm -rf "$tmp"' EXIT
mkdir "$tmp/native-dir" "$tmp/java-dir"
chmod 755 "$tmp/native-dir" "$tmp/java-dir"
printf "test -x native-dir: " && test -x "$tmp/native-dir" && echo true || echo false
printf "test -f native-dir: " && test -f "$tmp/native-dir" && echo true || echo false
printf "test -f java-dir: " && test -f "$tmp/java-dir" && echo true || echo false
printf '\nCurrent validation sites containing plumbing variables:\n'
rg -n 'plumelib_executable|plumelib_java_command|-x|PLUMELIB_MERGE' src/main/sh/plumelib-merge-common.shRepository: plume-lib/merging
Length of output: 6240
🏁 Script executed:
#!/bin/sh
set -eu
printf 'Relevant validation sites:\n'
sed -n '18,22p' src/main/sh/plumelib-merge-common.sh
printf '\n'
sed -n '47,52p' src/main/sh/plumelib-merge-common.sh
printf '\n'
sed -n '103,109p' src/main/sh/plumelib-merge-common.sh
printf '\nShell test behavior for temporary directories with execute permission:\n'
tmpdir=$(mktemp -d)
trap 'rm -rf "$tmpdir"' EXIT
mkdir "$tmpdir/native-dir" "$tmpdir/java-dir"
chmod 755 "$tmpdir/native-dir" "$tmpdir/java-dir"
test -x "$tmpdir/native-dir" && native_x=true || native_x=false
test -f "$tmpdir/native-dir" && native_f=true || native_f=false
printf 'native-dir: -x=%s, -f=%s\n' "$native_x" "$native_f"
test -x "$tmpdir/java-dir" && java_x=true || java_x=false
test -f "$tmpdir/java-dir" && java_f=true || java_f=false
printf 'java-dir: -x=%s, -f=%s\n' "$java_x" "$java_f"
test ! -f "$tmpdir/native-dir" && direct_is_not_regular=true || direct_is_not_regular=false
test ! -f "$tmpdir/java-dir" && direct_is_not_regular2=true || direct_is_not_regular2=false
printf 'directory checks: native non-regular=%s, java non-regular=%s\n' "$direct_is_not_regular" "$direct_is_not_regular2"Repository: plume-lib/merging
Length of output: 1307
Require a regular executable file at each validation point.
-x is also true for searchable directories, so PLUMELIB_MERGE_EXECUTABLE or a Java launcher directory can pass the checks at lines 18, 50, and 107 and fail only during invocation. Add [ -f "$path" ] before [ -x "$path" ] at these three sites, keeping the empty-path guard.
Also applies to: 50-50, 107-107
🤖 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 `@src/main/sh/plumelib-merge-common.sh` around lines 18 - 22, Update the
validation checks for PLUMELIB_MERGE_EXECUTABLE and the Java launcher paths at
all three validation sites to require both a regular file and executable
permissions. Preserve each existing non-empty path guard, and add the
regular-file check before the existing executable check so directories are
rejected during validation.
Source: MCP tools
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/prek.yaml:
- Around line 43-44: Update the “Delete any restored Spotless P2 lock file”
workflow step to remove metadata/.lock from the computed Gradle cache directory
used by the restore step, including when GRADLE_USER_HOME is set, rather than
hardcoding ~/.gradle.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ed05b8c4-b4a8-4f2e-aaec-966a565de4e1
📒 Files selected for processing (5)
.github/workflows/gradle.yml.github/workflows/prek.yaml.gitignorebuild.gradlegradle.properties
| - name: Delete any restored Spotless P2 lock file | ||
| run: rm -f ~/.gradle/caches/p2-data/metadata/.lock |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use the computed cache directory to delete the lock file.
If GRADLE_USER_HOME is set, Line 34 restores p2-data from that directory. Line 44 deletes a lock file only from ~/.gradle. The restored metadata/.lock can remain and cause the Spotless provisioning timeout.
Proposed fix
- run: rm -f ~/.gradle/caches/p2-data/metadata/.lock
+ run: rm -f "${{ steps.spotless-p2-dir.outputs.dir }}/metadata/.lock"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Delete any restored Spotless P2 lock file | |
| run: rm -f ~/.gradle/caches/p2-data/metadata/.lock | |
| - name: Delete any restored Spotless P2 lock file | |
| run: rm -f "${{ steps.spotless-p2-dir.outputs.dir }}/metadata/.lock" |
🤖 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 @.github/workflows/prek.yaml around lines 43 - 44, Update the “Delete any
restored Spotless P2 lock file” workflow step to remove metadata/.lock from the
computed Gradle cache directory used by the restore step, including when
GRADLE_USER_HOME is set, rather than hardcoding ~/.gradle.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gradle/libs.versions.toml (1)
49-54: 🩺 Stability & Availability | 🟠 MajorUse a resolvable
require-javadocversion.The change sets
org.plumelib:require-javadocto3.0.0. As of August 7, 2026, Maven Central lists2.0.0as the latest release and does not list3.0.0. (central.sonatype.com)Unless another configured release repository provides
3.0.0, thechecktask will fail during dependency resolution. Use2.0.0or publish3.0.0to an allowed repository.#!/usr/bin/env bash set -euo pipefail rg -n -C3 'require-javadoc|requireJavadoc|repositories' build.gradle gradle/libs.versions.toml for version in 3.0.0 2.0.0; do url="https://repo1.maven.org/maven2/org/plumelib/require-javadoc/${version}/require-javadoc-${version}.pom" if curl -fsSI "$url" >/dev/null; then echo "$version: available" else echo "$version: unavailable" fi done🤖 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 `@gradle/libs.versions.toml` around lines 49 - 54, Update the require-javadoc dependency version in the version catalog entry to a resolvable release, using 2.0.0 unless an allowed repository is configured to provide 3.0.0; keep the existing org.plumelib:require-javadoc module unchanged.
🤖 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.
Outside diff comments:
In `@gradle/libs.versions.toml`:
- Around line 49-54: Update the require-javadoc dependency version in the
version catalog entry to a resolvable release, using 2.0.0 unless an allowed
repository is configured to provide 3.0.0; keep the existing
org.plumelib:require-javadoc module unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3e571103-905f-42ab-9053-e07a3fe1bced
📒 Files selected for processing (13)
.github/workflows/gradle.ymlbuild.gradlegradle/libs.versions.tomlgradle/wrapper/gradle-wrapper.propertiessrc/test/java/name/fraser/neil/plaintext/DmpLibraryTest.javasrc/test/java/org/plumelib/merging/AdjacentDynamicProgrammingTest.javasrc/test/java/org/plumelib/merging/ConflictedFileTest.javasrc/test/java/org/plumelib/merging/Diff3FileTest.javasrc/test/java/org/plumelib/merging/EndToEndTest.javasrc/test/java/org/plumelib/merging/JavaAnnotationsMergerTest.javasrc/test/java/org/plumelib/merging/JavaImportsMergerTest.javasrc/test/java/org/plumelib/merging/JavaLibraryTest.javasrc/test/java/org/plumelib/merging/RDiffTest.java
1 participant
No description provided.