Repository navigation
Conversation
📝 Summary
Priority: ➖ Normal Change: Feature Merge Risk: 🟡 Moderate · up to
|
There was a problem hiding this comment.
Actionable comments posted: 16
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: Stirling-Tools/JPDFium/.coderabbit.yaml
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
92b14a9a-9131-488a-8842-433b9de197a1
📒 Files selected for processing: 14
.github/scripts/maven_metadata.py.github/scripts/upload-to-r2.sh.github/scripts/verify-central.sh.github/workflows/mirror-central-to-r2.yml.github/workflows/release.ymlCHANGELOG.mdREADME.mdbuild.gradle.ktsdocs/configuration.mddocs/installation.mddocs/native-loading.mdexamples/render-first-page/README.mdexamples/render-first-page/build.gradle.ktsjpdfium/src/main/java/stirling/software/jpdfium/panama/NativeDownloader.java
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout: 42
- GitHub Check: prep-windows-icu
- GitHub Check: Native bridge libFuzzer (ASan/UBSan, real PDFium)
- GitHub Check: build-natives (windows-arm64, windows-11-arm)
- GitHub Check: Restricted-env compat (no execstack + hermetic bundle)
- GitHub Check: Build & Verify (Compilation) (macos-14)
- GitHub Check: JMH Benchmarks (FFM Downcall Overhead Gate)
- GitHub Check: Unit & Integration E2E Tests (Linux x64)
- GitHub Check: build-natives (linux-arm64, ubuntu-24.04-arm)
- GitHub Check: GraalVM Native Image Smoke & E2E Test (Linux x64)
- GitHub Check: Concurrency Stress Tests (Linux x64)
- GitHub Check: Natural System Libvips & Downstream Integration (Linux x64)
- GitHub Check: README Quick Start Example (Linux x64)
- GitHub Check: Musl Native Detection (Alpine)
- GitHub Check: Build & Verify (Compilation) (windows-latest)
- GitHub Check: Samples E2E Suite (All 88 Feature Samples)
- GitHub Check: Publish Maven Dry Run
- GitHub Check: Build & Verify (Compilation) (ubuntu-latest)
- GitHub Check: Corpus + integration e2e
- GitHub Check: build-natives (darwin-x64, macos-15-intel)
- GitHub Check: build-natives (linux-x64, ubuntu-latest)
- GitHub Check: Bundle verify (symbols, RPATH, linkage, sizes)
- GitHub Check: build-natives (darwin-arm64, macos-14)
- GitHub Check: Samples E2E (88 features via RunAllSamples)
- GitHub Check: Robustness, Leak & Determinism Tests (Linux x64)
- GitHub Check: CLI correctness & no-corruption tests
- GitHub Check: build-natives (windows-x64, windows-latest)
- GitHub Check: Javadoc builds cleanly
- GitHub Check: Shellcheck (scripts + native/*.sh)
- GitHub Check: Flaky-test guard (3× rerun)
- GitHub Check: Module descriptor integrity
- GitHub Check: CodeQL Analysis (c-cpp)
- GitHub Check: JaCoCo coverage (non-blocking ratchet)
- GitHub Check: FFM layout vs pinned PDFium headers
- GitHub Check: OSV vulnerability scan
- GitHub Check: Java - SpotBugs + Checkstyle + PMD + Spotless
- GitHub Check: CodeQL (c-cpp)
- GitHub Check: CodeQL Analysis (java-kotlin)
- GitHub Check: C++ - clang-tidy + cppcheck + clang-format
- GitHub Check: Corpus (generate + native)
- GitHub Check: Public API compatibility (japicmp vs last release)
- GitHub Check: CodeQL (java-kotlin)
- GitHub Check: Outdated dependencies report
🧰 Additional context used
📚 Code guidelines: 1
.github/CODERABBIT_REVIEW.md — configured
📓 Path-based instructions: 2
Apply the Java 25 and FFM rules from .github/CODERABBIT_REVIEW.md.
⚙️ CodeRabbit configuration file
Files:
jpdfium/src/main/java/stirling/software/jpdfium/panama/NativeDownloader.java
Source excerpt: Use imports rather than package-qualified type names in method bodies and signatures.
📄 CodeRabbit inference engine (.github/CODERABBIT_REVIEW.md)
Files:
README.mdexamples/render-first-page/README.mdexamples/render-first-page/build.gradle.ktsdocs/configuration.mdjpdfium/src/main/java/stirling/software/jpdfium/panama/NativeDownloader.javadocs/native-loading.mdCHANGELOG.mddocs/installation.mdbuild.gradle.kts
🪛 actionlint (1.7.12)
.github/workflows/mirror-central-to-r2.yml
[error] 28-28: shellcheck reported issue in this script: SC2012:info:9:15: Use find instead of ls to better handle non-alphanumeric filenames
(shellcheck)
.github/workflows/release.yml
[error] 1068-1068: shellcheck reported issue in this script: SC2086:info:2:81: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 LanguageTool
docs/native-loading.md
[locale-violation] ~11-~11: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ... time it runs and reuse the cached copy afterwards. This is disabled by default. Enable i...
(AFTERWARDS_US)
🪛 Ruff (0.16.9)
.github/scripts/maven_metadata.py
[error] 29-29: Using xml to parse untrusted data is known to be vulnerable to XML attacks; use defusedxml equivalents
(S314)
🪛 zizmor (1.30.1)
.github/workflows/mirror-central-to-r2.yml
[warning] 24-24: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 76-76: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 3-8: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
.github/workflows/release.yml
[info] 961-961: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 962-962: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 988-988: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 990-990: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 1057-1057: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 1060-1060: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 1086-1086: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 1086-1086: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🔇 Additional comments: 5
.github/workflows/release.yml: 2
1042-1042: Gatepromoteon a successfulverify-stagingresult.
needs: [publish, verify-staging]runspromoteonly when both succeed, because the default condition issuccess(). The explicitif: ${{ !inputs.dryRun }}replaces that default. A customifwithout a status function drops the implicitsuccess()check only when it contains a status function. Here it has none, sosuccess()is still applied. No change is required.
997-997: Fix theverify-stagingcondition so a tag-less dispatch without a version does not skip verification and promote.
verify-stagingruns only for tags or wheninputs.versionis set. On aworkflow_dispatchwithout a version,verify-stagingis skipped.promotethen does not run, because a skipped dependency failssuccess(). The result is safe. Confirm that this is intended.Also applies to: 1042-1042
build.gradle.kts: 1
330-350: LGTM!.github/scripts/verify-central.sh: 2
5-9: LGTM!Also applies to: 25-29, 264-264
169-182: 🔒 Security & PrivacyThe
Authorizationcredentials are scoped to the stagingmavenrepository. The separatemavenCentraldeclaration has no credentials. Inr2mode,PORTAL_TOKENand the exportedVERIFY_TOKENare empty, so Gradle does not configure authentication for R2. No change is needed.
| def version_key(version: str) -> tuple: | ||
| parts = re.split(r"[.\-]", version) | ||
| return tuple((0, int(p), "") if p.isdigit() else (-1, 0, p) for p in parts) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the version ordering for qualifiers.
version_key mixes tuples of (0, int, "") and (-1, 0, str). The comparison is valid, but the ordering is wrong for Maven semantics in two cases. A shorter version such as 1.2.0 sorts before 1.2.0.1, which is fine. A version such as 1.2.0-rc1 splits into [1,2,0,rc1]. It then sorts after 1.2.0, because a missing part is shorter and compares lower. latest and release then point to the pre-release instead of 1.2.0. Treat a qualifier as lower than the release, for example by padding the key with a final release marker.
🐛 Proposed fix
--- "a/.github/scripts/maven_metadata.py"
+++ "b/.github/scripts/maven_metadata.py"
@@ -13,9 +13,11 @@
from pathlib import Path
def version_key(version: str) -> tuple:
- parts = re.split(r"[.\-]", version)
- return tuple((0, int(p), "") if p.isdigit() else (-1, 0, p) for p in parts)
+ base, _, qualifier = version.partition("-")
+ nums = tuple((0, int(p), "") if p.isdigit() else (-1, 0, p) for p in base.split("."))
+ # A release (no qualifier) sorts above any qualified build of the same base.
+ return nums, (0, "") if qualifier else (1, ""), qualifier
def main() -> None:📝 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.
| def version_key(version: str) -> tuple: | |
| parts = re.split(r"[.\-]", version) | |
| return tuple((0, int(p), "") if p.isdigit() else (-1, 0, p) for p in parts) | |
| def version_key(version: str) -> tuple: | |
| base, _, qualifier = version.partition("-") | |
| nums = tuple((0, int(p), "") if p.isdigit() else (-1, 0, p) for p in base.split(".")) | |
| # A release (no qualifier) sorts above any qualified build of the same base. | |
| return nums, (0, "") if qualifier else (1, ""), qualifier |
|
|
||
| versions = set(new_versions) | ||
| if live.is_file(): | ||
| root = ET.parse(live).getroot() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
Resolve the XML parse hardening question for live metadata.
Ruff flags ET.parse(live) (S314). live is downloaded from the bucket by upload-to-r2.sh. The bucket is written only by this workflow, so the exposure is low. Python's xml.etree does not expand external entities. It can still be hit by entity-expansion DoS payloads in older expat builds. Use defusedxml only if you treat the bucket as untrusted. Otherwise, accept the finding as a documented false positive.
🧰 Tools
🪛 Ruff (0.16.9)
[error] 29-29: Using xml to parse untrusted data is known to be vulnerable to XML attacks; use defusedxml equivalents
(S314)
Source: Linters/SAST tools
| } | ||
|
|
||
| cd "$REPO_DIR" | ||
| mapfile -t VERSION_DIRS < <(find . -name '*.pom' -printf '%h\n' | sed 's#^\./##' | sort -u) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Make the find call portable.
find -printf is GNU-only. The workflow runs on ubuntu-latest, so it works there. Local runs on macOS fail. Document the GNU requirement, or use find ... -exec dirname {} \;.
| done | ||
|
|
||
| find . -name 'maven-metadata.xml*' -delete | ||
| s3 sync . "$BUCKET/$PREFIX" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Add a failure recovery path for partial root uploads.
When PREFIX is empty, s3 sync . "$BUCKET/" uploads artifacts before the metadata is rewritten. If the sync or the metadata step fails midway, the root holds a partial version. The immutability check on line 33 then blocks every retry, because the version directory is no longer empty. Recovery needs manual deletion. Document the recovery step, or delete the uploaded version directories when a later step fails.
Source: Learnings
| for artifact in "${ARTIFACT_DIRS[@]}"; do | ||
| live="$artifact/maven-metadata.live.xml" | ||
| s3 cp "$BUCKET/$artifact/maven-metadata.xml" "$live" >/dev/null 2>&1 || rm -f "$live" | ||
| mapfile -t new_versions < <(printf '%s\n' "${VERSION_DIRS[@]}" | grep "^$artifact/[^/]*$" | xargs -n1 basename) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
Anchor the artifact match with -F and an escaped prefix.
grep "^$artifact/[^/]*$" treats . in the artifact path as a regex wildcard. Group IDs such as com/stirling/jpdfium contain no dots, but a future artifact path with a dot can match a different artifact. Use grep -x against a fixed prefix, or a bash loop with prefix removal.
| curl -fsSI "https://maven.stirlingpdf.com/com/stirling/jpdfium/$VERSION/jpdfium-$VERSION.pom" >/dev/null | ||
| echo "### Released com.stirling:*:$VERSION to https://maven.stirlingpdf.com" >> $GITHUB_STEP_SUMMARY |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Quote $GITHUB_STEP_SUMMARY.
actionlint reports SC2086 for the unquoted redirect target. Quote it as >> "$GITHUB_STEP_SUMMARY".
Source: Linters/SAST tools
| cleanup-staging: | ||
| name: Delete the staging copy | ||
| needs: [publish, verify-staging, promote] | ||
| if: ${{ always() && needs.publish.result == 'success' }} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
Block cleanup-staging from deleting staging while promote is still pending or skipped for a verification failure.
always() runs cleanup after verify-staging fails. The staging copy is then deleted, and the failed run cannot be inspected or re-promoted. This matches the stated design, so confirm that it is intended. Use if: always() && needs.publish.result == 'success' && (needs.promote.result != 'failure') if you want to keep staging after a failed promote.
| ## Downloading at runtime | ||
|
|
||
| As an alternative to bundling, JPDFium can download the matching native artifact from Maven Central the first time it runs and reuse the cached copy afterwards. | ||
| As an alternative to bundling, JPDFium can download the matching native artifact from https://maven.stirlingpdf.com the first time it runs and reuse the cached copy afterwards. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use American-English spelling.
Replace afterwards with afterward on this changed line.
🧰 Tools
🪛 LanguageTool
[locale-violation] ~11-~11: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ... time it runs and reuse the cached copy afterwards. This is disabled by default. Enable i...
(AFTERWARDS_US)
Source: Linters/SAST tools
| As an alternative to bundling, JPDFium can download the matching native artifact from https://maven.stirlingpdf.com the first time it runs and reuse the cached copy afterwards. | ||
|
|
||
| This is disabled by default. Enable it only when network access to Maven Central is acceptable at runtime: | ||
| This is disabled by default. Enable it only when network access to maven.stirlingpdf.com is acceptable at runtime: |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Document the configured repository.
jpdfium.native.repo can redirect downloads to an internal mirror. This sentence names only maven.stirlingpdf.com, so users can approve or allowlist the wrong endpoint. Replace it with “the configured repository”.
| static final String VERSION_PROPERTY = "jpdfium.native.version"; | ||
| static final String REPO_PROPERTY = "jpdfium.native.repo"; | ||
| static final String DEFAULT_REPO = "https://repo1.maven.org/maven2"; | ||
| static final String DEFAULT_REPO = "https://maven.stirlingpdf.com"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a regression test for the new default repository.
NativeDownloaderTest covers explicit repository URLs but does not assert repoBase() when jpdfium.native.repo is unset. Add a test that expects https://maven.stirlingpdf.com and restores the system property after the test.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
set -u
printf '%s\n' '--- NativeDownloader at reviewed head ---'
nl -ba jpdfium/src/main/java/stirling/software/jpdfium/panama/NativeDownloader.java | sed -n '1,180p'
printf '%s\n' '--- diff base..head for downloader and publishing files ---'
git diff --stat 5a35fefc30a781857f68579ce5c3d4b2f4c8ff6c 1f133528ab51b32929c9e29dd1c3a87634264535 -- \
jpdfium/src/main/java/stirling/software/jpdfium/panama/NativeDownloader.java \
.github/workflows scripts build.gradle settings.gradle gradle.properties pom.xml
git diff --unified=40 5a35fefc30a781857f68579ce5c3d4b2f4c8ff6c 1f133528ab51b32929c9e29dd1c3a87634264535 -- \
jpdfium/src/main/java/stirling/software/jpdfium/panama/NativeDownloader.java \
.github/workflows scripts build.gradle settings.gradle gradle.properties pom.xml
printf '%s\n' '--- version and native artifact references ---'
rg -n -F --glob '*.java' --glob '*.gradle' --glob '*.properties' --glob '*.yml' --glob '*.yaml' --glob '*.sh' \
-e 'jpdfium-natives-' -e 'project.version' -e 'version =' -e 'DEFAULT_REPO' -e 'MAVEN_REPO' -e 'maven.stirlingpdf.com' . || test "$?" -eq 1
printf '%s\n' '--- repository files related to publishing/mirroring ---'
rg --files .github scripts | sort | sed -n '1,240p'Repository: Stirling-Tools/JPDFium
Length of output: 40187
🏁 Script executed:
set -u
printf '%s\n' '--- release/version tags ---'
git tag --sort=version:refname | tail -80
printf '%s\n' '--- version declarations and compatibility references ---'
rg -n -F --glob '*.gradle' --glob '*.gradle.kts' --glob '*.properties' --glob '*.md' --glob '*.yml' --glob '*.yaml' \
-e '1.1.6' -e '1.2.0' -e 'jpdfium.version' -e 'supported version' -e 'Maven Central' -e 'maven.stirlingpdf.com' . || test "$?" -eq 1
printf '%s\n' '--- downloader remainder and tests ---'
nl -ba jpdfium/src/main/java/stirling/software/jpdfium/panama/NativeDownloader.java | sed -n '128,270p'
nl -ba jpdfium/src/test/java/stirling/software/jpdfium/panama/NativeDownloaderTest.java | sed -n '1,190p'
printf '%s\n' '--- R2 upload and metadata scripts ---'
nl -ba .github/scripts/upload-to-r2.sh | sed -n '1,260p'
nl -ba .github/scripts/maven_metadata.py | sed -n '1,260p'
printf '%s\n' '--- repository metadata and selected artifact availability ---'
for u in \
'https://maven.stirlingpdf.com/com/stirling/maven-metadata.xml' \
'https://maven.stirlingpdf.com/com/stirling/jpdfium/maven-metadata.xml' \
'https://maven.stirlingpdf.com/com/stirling/jpdfium-natives-linux-x64/1.1.6/jpdfium-natives-linux-x64-1.1.6.jar' \
'https://maven.stirlingpdf.com/com/stirling/jpdfium-natives-linux-x64/1.2.0/jpdfium-natives-linux-x64-1.2.0.jar' \
'https://repo1.maven.org/maven2/com/stirling/jpdfium-natives-linux-x64/1.1.6/jpdfium-natives-linux-x64-1.1.6.jar' \
'https://repo1.maven.org/maven2/com/stirling/jpdfium-natives-linux-x64/1.2.0/jpdfium-natives-linux-x64-1.2.0.jar'
do
printf 'URL %s\n' "$u"
curl -fsSIL --max-time 20 "$u" 2>&1 | sed -n '1,8p' || true
doneRepository: Stirling-Tools/JPDFium
Length of output: 30028
🏁 Script executed:
set -u
printf '%s\n' '--- local release headings and version references ---'
rg -n '^## \[[^]]+\]|Releases up to|current release|are there too' CHANGELOG.md docs README.md || true
printf '%s\n' '--- live Maven metadata ---'
for u in \
'https://maven.stirlingpdf.com/com/stirling/jpdfium/maven-metadata.xml' \
'https://maven.stirlingpdf.com/com/stirling/jpdfium-natives-linux-x64/maven-metadata.xml'
do
printf 'URL %s\n' "$u"
curl -fsSL --max-time 20 "$u" | sed -n '1,120p' || true
done
printf '%s\n' '--- all documented release/platform native artifacts ---'
for version in 1.1.6 1.2.0; do
for platform in linux-x64 linux-arm64 darwin-x64 darwin-arm64 windows-x64 windows-arm64; do
u="https://maven.stirlingpdf.com/com/stirling/jpdfium-natives-${platform}/${version}/jpdfium-natives-${platform}-${version}.jar"
status=$(curl -sS -o /dev/null -w '%{http_code} %{size_download}' --max-time 30 "$u" || printf 'curl-error')
printf '%s %s %s\n' "$version" "$platform" "$status"
done
doneRepository: Stirling-Tools/JPDFium
Length of output: 2169
🏁 Script executed:
set -u
printf '%s\n' '--- NativeDownloader download path ---'
nl -ba jpdfium/src/main/java/stirling/software/jpdfium/panama/NativeDownloader.java | sed -n '269,430p'
printf '%s\n' '--- compatibility and native-loading contracts ---'
nl -ba docs/native-loading.md | sed -n '1,140p'
nl -ba .github/workflows/compatibility.yml | sed -n '1,180p'
printf '%s\n' '--- older native artifact probes ---'
for version in 1.1.0 1.1.1 1.1.2 1.1.3 1.1.4 1.1.5; do
u="https://maven.stirlingpdf.com/com/stirling/jpdfium-natives-linux-x64/${version}/jpdfium-natives-linux-x64-${version}.jar"
printf '%s ' "$version"
curl -sS -o /dev/null -w '%{http_code} %{size_download}\n' --max-time 20 "$u" || true
doneRepository: Stirling-Tools/JPDFium
Length of output: 20448
Mirror older native artifacts or retain an automatic fallback.
NativeDownloader.DEFAULT_REPO now points to a repository that lists only 1.1.6 and 1.2.0. The project includes releases 1.1.0 through 1.1.5, but their native artifact URLs return 404. With runtime downloads enabled for one of those releases, download(...) receives a non-200 response and throws NativeLoadException.
Mirror every supported release for all six platforms, or fall back to Maven Central when the artifact is absent.
Description of Changes
release.ymlstages tostaging/<run>/, runs the six-platform verify against staging, promotes to the root only if all pass, then deletes staging. NewdryRuninput stops before promote.upload-to-r2.shrefuses to overwrite a released version and rewritesmaven-metadata.xmlfrom the live copy, so every version stays listed. Secrets come from themavenenvironment.mirror-central-to-r2.ymlcopies an existing Central version into R2 (sha1-checked) and verifies it.NativeDownloaderruntime downloads, README, install/config/native-loading docs, the example project and CHANGELOG now point at the new repository.0.0.0-citest.1built, staged, verified on all six platforms and promoted, then was deleted; metadata is back to 1.1.6 and 1.2.0 only.com.stirling, pulled JPDFium from R2, and auto-redaction worked in the running app.snapshot.yml(Central snapshots on every push to main),publish-github-packages.yml,compatibility.yml(baseline from Central), and the Maven Central README badge.Checklist
General
## [Unreleased]inCHANGELOG.mdLibrary changes
.github/workflows/compatibility.ymlexcludes the break with a reasondocs/installation.md, and the POMsNative or FFM changes
bash native/build-real.sh./gradlew :jpdfium:integrationTestnative/bridge/include/jpdfium.hand the ABI notes for any ABI changeTesting
./gradlew testpasses