fix(api): check legacy tools path for binaries by imnotjames · Pull Request #146 · grimmory-tools/grimmory

imnotjames · 2026-03-23T17:22:34Z

Description

the kepubify / ffprobe binaries may be on disk but they're in the "data"
directory under tools rather than under the current directory

in some cases, like proxmox or other manual installs, this helps make the
transition to grimmory easier

Changes

Summary by CodeRabbit

Internal Improvements
- Refined how the system search path is built and how candidates are selected; returned file paths are now normalized (may change path formatting).
Tests
- Added unit tests that simulate filesystem responses to verify primary and fallback search resolution.

coderabbitai · 2026-03-23T17:22:40Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7df2e7c0-ae6e-4864-8c2b-25b190a53fe3

📥 Commits

Reviewing files that changed from the base of the PR and between 8ae6629 and 1de7044.

📒 Files selected for processing (2)

booklore-api/src/main/java/org/booklore/util/FileService.java
booklore-api/src/test/java/org/booklore/util/FileServiceTest.java

🚧 Files skipped from review as they are similar to previous changes (1)

booklore-api/src/main/java/org/booklore/util/FileService.java

📝 Walkthrough

Walkthrough

Extracted search-path construction into a new private getSystemSearchPath(); findSystemFile now iterates candidates from that path and returns the resolved candidate as an absolute, normalized Path; added tests that statically mock Files.isRegularFile to exercise primary and fallback resolution.

Changes

Cohort / File(s)	Summary
FileService implementation `booklore-api/src/main/java/org/booklore/util/FileService.java`	Added private `getSystemSearchPath()` that builds a colon-separated search path starting with `bin`, then a legacy `tools` directory from `appProperties.getPathConfig()`, and appends the environment `PATH` when present. `findSystemFile(String)` now iterates `getSystemSearchPath().split(":")`, and returns `Paths.get(path).resolve(filename).toAbsolutePath().normalize()` when a regular file is found.
FileService tests `booklore-api/src/test/java/org/booklore/util/FileServiceTest.java`	Added `FindSystemFileTest` using `MockedStatic<Files>` to stub `Files.isRegularFile(...)`. Tests assert resolution to `bin/example` when present, and fallback to `.../tools/example` when the first candidate is absent then present.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I hopped through folders, sniffed each track,

bin first, then tools, I never looked back.
Mocked paws checked the stones along the way,
Found the file — hooray! — and bounded off to play.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix(api): check legacy tools path for binaries' accurately and concisely describes the main change: adding logic to check a legacy tools directory for binary files.
Description check	✅ Passed	The description addresses what the PR does (checking legacy tools path for binaries) and provides context (ease migration in manual/Proxmox installs), but lacks a 'Linked Issue' field and provides minimal detail in the Changes section.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@booklore-api/src/main/java/org/booklore/util/FileService.java`:
- Around line 171-174: findSystemFile currently returns Path objects like
possiblePath that may be relative; update the return to convert the resolved
Path to an absolute, normalized path by calling .toAbsolutePath().normalize() on
possiblePath before returning to ensure downstream binary execution is
cwd-independent (refer to the possiblePath variable in findSystemFile).
- Around line 151-168: The method getSystemSearchPath and its caller
findSystemFile currently hardcode ":" and build a relative "bin" path; change
them to use platform-safe File.pathSeparator instead of ":" and construct the
local bin path from appProperties.getPathConfig() (e.g.,
Path.of(appProperties.getPathConfig(), "bin").toString()) so both "bin" and
legacyToolsPath are absolute/consistent, build the final search string by
joining localBin, legacyToolsPath and System.getenv("PATH") (handling null), and
update findSystemFile to split using File.pathSeparator (or
Pattern.quote(File.pathSeparator)) rather than ":".

In `@booklore-api/src/test/java/org/booklore/util/FileServiceTest.java`:
- Around line 312-339: The tests assume the local-bin path is "bin/example"
relative to cwd, which masks ignoring appProperties.getPathConfig(); update both
tests so the expected Path is built from the configured local-bin path instead
of a hardcoded "bin" string: use the Path returned by
appProperties.getPathConfig().getLocalBin() (or the appropriate accessor on
appProperties.getPathConfig()) and resolve "example" against it, keep the
Files.isRegularFile mocking the same, and assert that
fileService.findSystemFile("example") equals that configured-path result.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f733c3e7-26ae-416a-8ccb-7df98193131f

📥 Commits

Reviewing files that changed from the base of the PR and between 3c7550c and 0df7f2e.

📒 Files selected for processing (2)

booklore-api/src/main/java/org/booklore/util/FileService.java
booklore-api/src/test/java/org/booklore/util/FileServiceTest.java

coderabbitai

♻️ Duplicate comments (1)

booklore-api/src/main/java/org/booklore/util/FileService.java (1)

167-168: ⚠️ Potential issue | 🟠 Major

Use the same separator for split as for path construction.

getSystemSearchPath() builds with File.pathSeparator, but findSystemFile() still splits on ":". This makes parsing inconsistent and breaks compatibility on non-colon-separated environments.

Suggested fix

+import java.util.regex.Pattern;
...
-        String[] searchPaths = getSystemSearchPath().split(":");
+        String[] searchPaths = getSystemSearchPath().split(Pattern.quote(File.pathSeparator));

#!/bin/bash
set -euo pipefail

# Verify current separator construction vs parsing behavior.
nl -ba booklore-api/src/main/java/org/booklore/util/FileService.java | sed -n '151,175p'
rg -n 'File\.pathSeparator|getSystemSearchPath\(\)\.split\(":\"\)' booklore-api/src/main/java/org/booklore/util/FileService.java

# Expected:
# - path is built using File.pathSeparator
# - split should use Pattern.quote(File.pathSeparator), not ":"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@booklore-api/src/main/java/org/booklore/util/FileService.java` around lines
167 - 168, The findSystemFile method currently splits the search path with a
hardcoded ":" which mismatches how getSystemSearchPath builds the path using
File.pathSeparator; update findSystemFile to split on the actual system
separator (use Pattern.quote(File.pathSeparator) or otherwise derive the
separator from File.pathSeparator) so parsing is consistent across platforms
(locate the split call in findSystemFile and replace the literal ":" with a
split that uses File.pathSeparator).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@booklore-api/src/main/java/org/booklore/util/FileService.java`:
- Around line 167-168: The findSystemFile method currently splits the search
path with a hardcoded ":" which mismatches how getSystemSearchPath builds the
path using File.pathSeparator; update findSystemFile to split on the actual
system separator (use Pattern.quote(File.pathSeparator) or otherwise derive the
separator from File.pathSeparator) so parsing is consistent across platforms
(locate the split call in findSystemFile and replace the literal ":" with a
split that uses File.pathSeparator).

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: bc535fd6-e6f4-431a-b09e-69c830eaeb53

📥 Commits

Reviewing files that changed from the base of the PR and between 76023d6 and 8ae6629.

📒 Files selected for processing (1)

booklore-api/src/main/java/org/booklore/util/FileService.java

the kepubify / ffprobe binaries may be on disk but they're in the "data" directory under `tools` rather than under the current directory

imnotjames · 2026-03-23T18:46:01Z

Rebased and cleaned up a few things.

balazs-szucs

Thank you, looks good!

* develop: fix(entrypoint): ensure books directory exists and is writable by the target user (grimmory-tools#119) fix(metadata): fix metadata fetching relying for filename "first" instead of better fields like ISBN in Goodreads/Bookdrop (grimmory-tools#85) chore(build): migrate Gradle build scripts from Groovy to Kotlin DSL (grimmory-tools#100) perf(ui): captures some low hanging fruit around excessive re-renders in the app (grimmory-tools#158) fix(api): check legacy tools path for binaries (grimmory-tools#146)

the kepubify / ffprobe binaries may be on disk but they're in the "data" directory under `tools` rather than under the current directory

imnotjames force-pushed the fix/-/legacy-tools-path branch from 41b65b2 to 0df7f2e Compare March 23, 2026 18:28

imnotjames marked this pull request as ready for review March 23, 2026 18:28

coderabbitai Bot reviewed Mar 23, 2026

View reviewed changes

Comment thread booklore-api/src/main/java/org/booklore/util/FileService.java

Comment thread booklore-api/src/main/java/org/booklore/util/FileService.java Outdated

Comment thread booklore-api/src/test/java/org/booklore/util/FileServiceTest.java

coderabbitai Bot reviewed Mar 23, 2026

View reviewed changes

fix(api): check legacy tools path for binaries

1de7044

the kepubify / ffprobe binaries may be on disk but they're in the "data" directory under `tools` rather than under the current directory

imnotjames force-pushed the fix/-/legacy-tools-path branch from 8ae6629 to 1de7044 Compare March 23, 2026 18:45

balazs-szucs approved these changes Mar 24, 2026

View reviewed changes

balazs-szucs merged commit 1ea6a4f into grimmory-tools:develop Mar 24, 2026
9 checks passed

zachyale pushed a commit that referenced this pull request Apr 17, 2026

fix(api): check legacy tools path for binaries (#146)

d935cab

the kepubify / ffprobe binaries may be on disk but they're in the "data" directory under `tools` rather than under the current directory

zachyale pushed a commit that referenced this pull request Apr 22, 2026

fix(api): check legacy tools path for binaries (#146)

5d84097

the kepubify / ffprobe binaries may be on disk but they're in the "data" directory under `tools` rather than under the current directory

imnotjames deleted the fix/-/legacy-tools-path branch April 28, 2026 20:47

coderabbitai Bot mentioned this pull request May 31, 2026

Allow local path configurations via env vars #1592

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(api): check legacy tools path for binaries#146

fix(api): check legacy tools path for binaries#146
balazs-szucs merged 1 commit into
grimmory-tools:developfrom
imnotjames:fix/-/legacy-tools-path

imnotjames commented Mar 23, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Mar 23, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

imnotjames commented Mar 23, 2026

Uh oh!

balazs-szucs left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

imnotjames commented Mar 23, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

imnotjames commented Mar 23, 2026

Uh oh!

balazs-szucs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

imnotjames commented Mar 23, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Mar 23, 2026 •

edited

Loading