feat(api/ocr): offer to repair the missing OCR native alias, on the user's terms - #464
Open
davidyoung8196504567-sudo wants to merge 11 commits into
Conversation
Commons.loadTesseract() has been setting libTesseractLoaded on a false premise: loadNatives() not throwing means Legerix loaded its own file by absolute path. It says nothing about the library tess4j binds when OCR runs, because tess4j resolves the short name "tesseract" through JNA at its own static init and can land somewhere else entirely. The banner then reported a version string, which describes the extracted file rather than the bound one. Investigating Legerix#20 across macOS, two Linux tiers and native Windows showed both halves of that going wrong in the field: a host binding Homebrew's tesseract while the suite stayed green, and a host where the bundled payload ships only versioned filenames so the JNA lookup misses and OCR fails outright. Adds NativeProvenance, which reports rather than enforces — a wrong binding is a diagnosis problem, not a crash, and the point is that it is currently invisible: - captures the tier directory loadNatives() already returns and we discarded - after the first real OCR call, resolves the short names tess4j uses and logs any that landed outside that directory. Deliberately after OCR: before it the check proves nothing, and asking JNA for an unresolved name would itself trigger the load under test. A copy that is byte-identical to ours is reported as equivalent, not as a warning, so tess4j's own temp extraction does not cry wolf - flags natives that are not Legerix's own when Legerix is shaded into our jar. Its extraction reads getProtectionDomain().getCodeSource(), so a fat jar makes that our jar; comparing code sources is exact, where comparing filenames would misfire on the large transitive set shipped for Windows - turns the JNA link failure into a diagnosis. "Reinstall OculiX" is the wrong advice when the payload is present and merely lacks the unversioned alias JNA asks for, which is what happens on a Mac with no system tesseract The banner now names the natives directory instead of only a version, and isBindingVerified() stays false until a real OCR call has been observed, so it can never read as "fine" merely because nothing was checked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018XD7mNUrETDESsPY22Dubv
davidyoung8196504567-sudo
requested review from
adriancostin6 and
julienmerconsulting
as code owners
September 4, 2026 06:04
Fleet review found two defects that pointed in opposite directions, and both were in claims the code made about itself rather than in the logic. resolveQuietly() called NativeLibrary.getInstance(name), which loads when the name is not cached. For any name the consumer had not bound — all three of them on Windows — the check therefore performed the very short-name resolution it exists to observe, and could then report a library OCR never touched as the one servicing it. On Linux it is worse than misleading: speculatively mapping a system tesseract alongside our bundled leptonica is how the crash we reported upstream begins. A diagnostic must not be able to cause the fault it looks for. It now reads JNA's own cache and reports only what is genuinely bound, matching on getName() rather than on the map key so it does not depend on how JNA composes that key. If the cache cannot be read it reports nothing rather than forcing a load. Confirmed on this Mac: the check previously reported three resolved names and now reports two, because the third was never bound by the consumer at all — the old code was manufacturing the observation it reported. isBindingVerified() could also return true having checked nothing. allInside started true and every unbound name hit continue, so the all-unbound case — the normal Windows one — came out verified. Its own javadoc promised the opposite. Now requires at least one observation. Also narrowed two sentences to what was actually checked: the warning speaks of the name being bound outside the bundled directory rather than asserting what is servicing OCR, and the link-failure note no longer generalises from one filename to "the bundled files carry version suffixes only". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018XD7mNUrETDESsPY22Dubv
davidyoung8196504567-sudo
force-pushed
the
feat/native-alias-self-heal
branch
from
September 4, 2026 16:15
d146edc to
340b8bf
Compare
verifyBinding() logged the byte-identical case and fell through without touching allInside, so bindingVerified stayed true while OCR was serviced from outside the extraction directory. That is a false pass in the normal Windows case, where tess4j binds its own byte-identical temp copy. The question this check answers is one of path, not content: if a short name resolves outside our directory then our directory lost, whatever the bytes say. Content equality cannot distinguish "our payload reached by another route" from "a different build that happens to match", and under a shaded jar the directory's own contents may not be ours either — so suppressing the signal returned success in exactly the case the check exists to detect. It now lowers the message severity and leaves the flag false. Carried back from the self-heal branch so this PR is correct on its own. The two are stacked, so merging both in either order already produced this result; the exposure was this PR merging alone if the other stalls, which would have shipped the false pass with nothing following to correct it. Found by a fleet reviewer reading the diffs rather than my summary of them, after an earlier review of these PRs turned out to have been of the summary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018XD7mNUrETDESsPY22Dubv
…ser's terms Builds on oculix-org#463, which detects that the bundled natives lack the unversioned filename JNA's exact-name lookup asks for. Detecting it does not help a user whose OCR is broken today, and the repair is a single symlink. It is not done silently. This writes into a cache directory OculiX did not create, and people hold views about that, so it is a stored three-state choice: ASK (default, the IDE prompts), AUTO (repair at every startup), NEVER (never write). NEVER still prints the exact ln -s command, so declining never leaves someone worse off than not being asked. Re-asserted on every startup rather than once at install. The cache is keyed by version, so an upgrade extracts a fresh directory and any alias made earlier is gone — the same reason a package manager re-points its "latest" link after each build instead of assuming it holds. Idempotent, so if Legerix ever ships these itself this quietly does nothing. Which aliases are needed was measured across macOS, both Linux x86-64 tiers and native Windows, and the rules are not symmetric: - unversioned only for the JNA lookup. A versioned alias satisfies isVersionedName, enters JNA's version-pooling fallback and competes with whatever the distro ships — reintroducing the bug it is meant to fix - Linux additionally needs versioned liblept.so.5, because the bundled tesseract's ELF NEEDED is that literal string and no such file exists. Safe only alongside the unversioned link, which wins the exact-name pass first - Windows needs nothing and is excluded: tess4j binds fully versioned names it ships itself, and NTFS symlinks would need admin or Developer Mode Aliases always target the real versioned library, never another alias — the directory scan uses NOFOLLOW_LINKS, so a link made earlier in the same pass cannot become the target of a later one and leave a chain that breaks when the middle link goes. Verified on a Mac with no system tesseract, where OCR previously failed outright with UnsatisfiedLinkError: after repair all three short names resolve inside the bundled directory, isBindingVerified() is true, and OCR returns its text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018XD7mNUrETDESsPY22Dubv
Completes the ASK path from the previous commit, which until now meant "diagnose but do not write". The API decides what is needed; the IDE asks. Shown once the window is up rather than during startup, so the first thing a user meets is the IDE and not a dialog about shared libraries. Silent unless there is something to repair, so a healthy install never sees it. Three answers, and the difference between them matters: - Add the links, with "do this automatically after updates" checked by default. Remembering is the point rather than a convenience: the cache is keyed by version, so the links are gone again after the next update unless we are allowed to re-add them. - Not now leaves the preference untouched and the offer returns next start. Declining once is not declining forever. - Never stores the refusal and prints the ln -s commands, so saying no does not leave someone worse off than never having been asked. Wording avoids implementation vocabulary — no symlinks, no JNA, no unversioned filenames. It says OCR will not run, what OculiX proposes to do, and that nothing outside its own cache folder is touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018XD7mNUrETDESsPY22Dubv
…advice
Fleet review found the versioned Linux alias gated behind the unversioned set
being incomplete:
if (runningLinux() && !missing.isEmpty() && ...)
so a directory that already has libtesseract.so, libleptonica.so and liblept.so
yields an empty list, and liblept.so.5 is never created although it is absent.
That population is precisely the people who followed the three-symlink advice we
gave before measuring the ELF side — the ones most likely to believe they are
already fixed. Without that file, System.load of the bundled tesseract fails on
NEEDED liblept.so.5; with it, resolution reaches the bundled copy. The condition
is now evaluated independently.
Two further things the review caught, both cases of a claim outrunning the code
that backs it:
- targetFor() excluded only the one alias name it was building, so on a tier
that already ships unversioned files — darwin does, darwin-aarch64 does not —
an alias could be pointed at another unversioned file, the exact indirection
the NOFOLLOW comment says it prevents. It now excludes every name this class
creates, and restricts targets to the platform's own object format.
- aliasesTargetTheRealLibraryNeverAnotherAlias() asserted only that the target
was not itself a symlink, which is weaker than its name. Its fixture also mixed
Mach-O and ELF filenames, a combination no shipped tier produces, so
targetFor() could match across formats and the test would still pass. The
fixture is now single-format, the assertion checks the target is a real
versioned library, and a separate test covers the cross-format case directly.
Adds a test for the zero-observation path that the previous test only named, via
a package-private reset hook. That path was the one this class got wrong.
LINUX BRANCH NOT VERIFIED HERE: this machine is macOS, so runningLinux() is
false and the liblept.so.5 condition cannot be exercised. Reviewed by reading and
sent to the Linux host to confirm.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018XD7mNUrETDESsPY22Dubv
… DT_NEEDED Two findings from a second round of fleet review, each caught by the host whose platform the other could not see. targetFor() filtered candidates with endsWith(platformSuffix()), which is ".so" on Linux. Linux tiers ship libtesseract.so.5 and libleptonica.so.6 — neither ENDS with ".so". The filter matched nothing, targetFor returned null for every name, and both alias paths are gated on it being non-null, so missingAliases() returned an empty list on every Linux tier. The repair was inert there, and silently: "nothing missing" is indistinguishable from "already correct". It was introduced while restricting targets to the platform's object format — correct on darwin, vacuous on Linux, in the branch this machine cannot exercise. It now reads the magic bytes instead of matching an extension, which is what the restriction was for. Also makes the candidate order deterministic: Files.list() has no defined order, so a directory with two candidates would have failed intermittently rather than consistently. Also replaces the hardcoded liblept.so.5 with the value read from the shipped tesseract's DT_NEEDED. Hardcoding was right on three of four Linux tiers and wrong on linux-x86-64-legacy, which needs no such alias; reading it is exact on all four and needs no caveat. The ELF reader was validated against all eight published Linux assets and reproduces the SONAME/NEEDED matrix measured independently on two other hosts. The byte-identity fix this commit used to carry now lives in the provenance guard branch this one is stacked on, so that PR is correct if it merges alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018XD7mNUrETDESsPY22Dubv
Found by a reviewer running the real methods rather than a transcription of them. extractionDir is a mutable static, and the first touch of Commons from anywhere triggers its static init — which loads the natives and calls recordExtraction() with the real tier directory, silently replacing whatever a caller had set. createAliases() read the field into a local, then called missingAliases() which read the field again independently. The alias NAMES therefore came from one read and the directory WRITTEN TO from another, with a filesystem write in between. Observed rather than theorised: a caller recorded a scratch directory, asked for the missing aliases and got the right list for it, then called createAliases() and had three symlinks created in the real cache instead — the directory the installed application uses. In production this is usually benign, because loadNatives() records the correct directory before anything asks. The exposure is any second recordExtraction — a different tier, a diagnostic, a test — being reverted underneath the caller. That is check-then-act on shared mutable state where the consequence is files on disk, which is not a race worth leaving open on the grounds that the common path avoids it. The directory is now captured once at the public entry point and threaded through. recordExtraction() also logs when it replaces a different value, so a silent re-point becomes a visible one. Adds a test that a list computed for one directory cannot be written into another, since that is the property rather than any particular call order. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018XD7mNUrETDESsPY22Dubv
The previous commit threaded the path through missingAliases and targetFor but left createAliases reading the static itself, which only closed the window inside the call. The one between a caller's recordExtraction and the call remained, because the first touch of Commons anywhere triggers its static init and re-points the field. Demonstrated on this machine rather than reasoned about: a probe recorded a scratch directory, received the correct alias list for it, and then created the alias in the real cache instead. Forcing Commons to initialise before recording made the same probe behave correctly, which isolates the cause. Adds createAliases(Path), mirroring missingAliases(Path), so a caller can name the directory rather than hope the field still holds it. The no-argument forms remain for the production path, where loadNatives records before anything asks. The straddle test now uses the explicit form and leaves the field pointing somewhere else, so it proves the guarantee rather than the call order. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018XD7mNUrETDESsPY22Dubv
ubuntu24 built the previous commit and showed the intra-call fix was not enough: recordExtraction -> createAliases is correct, but recordExtraction -> missingAliases -> createAliases still misroutes, because the no-argument missingAliases triggers the Commons static init that re-points the field and the following createAliases then captures the new value at its own entry. That two-call shape is not a harness curiosity, it is the ASK flow: check to build the prompt, wait for the user, repair. The IDE version has a human sitting in the middle, which is the longest window this pattern can have. Both production callers now capture the extraction directory once and pass it to every subsequent call. manualCommand gains a Path overload for the same reason, so the commands printed to a user who declines describe the directory that was actually inspected. In production both reads normally yield the same directory, because loadNatives records it before anything asks. This matters wherever the recorded directory is not Legerix's own: a diagnostic, a test, a second tier, or any future caller. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018XD7mNUrETDESsPY22Dubv
…the footgun ubuntu24 re-measured on f69591f and nearly reported a false green: the raw no-argument pair produced four correct links when run after other tests in the same JVM, and zero when run alone in a fresh one. Commons had already been initialised by the earlier test, so the field re-point had already happened and recordExtraction was the last write. Whether the pair misroutes depends on test order — a check that cannot fire, looking like a pass, which is the same species as the blocker it was meant to catch. Both production callers already pass a Path, so the no-argument forms had no callers outside the test class. They are removed rather than deprecated: this class is new in this branch, nothing external depends on it, and making the directory a required argument turns the property into something the compiler enforces instead of something a comment asks for. Every test now names the directory it is talking about, so none of them depends on whether something else initialised Commons first. Verified that a full run leaves the real cache byte-identical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018XD7mNUrETDESsPY22Dubv
davidyoung8196504567-sudo
force-pushed
the
feat/native-alias-self-heal
branch
from
September 4, 2026 21:42
3d4ae5b to
71fbad5
Compare
Collaborator
|
@davidyoung8196504567-sudo — the decision on this one, with its reasoning and the rest of your September batch, is in #469. It is not a merge, and I would rather be contradicted there than agreed with here. |
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
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.
Why
#463 detects that the bundled natives lack the unversioned filename JNA's exact-name lookup asks for. That diagnosis doesn't help a user whose OCR is broken today, and the repair is a single symlink.
The choice is the user's
This writes into a cache directory OculiX did not create, and people hold views about that. So it's a stored three-state preference rather than a silent side effect:
OCR_ALIAS_POLICYASK(default)AUTONEVERln -scommandNEVERprinting the command is deliberate: declining should never leave someone worse off than not being asked.Why every startup, not once at install
The cache is keyed by version, so an upgrade extracts a fresh directory and any alias made earlier is gone. Same reason a package manager re-points its
latestlink after each build instead of assuming it holds. The check is idempotent and near-free — and if Legerix ever ships these aliases itself, it quietly does nothing.Which aliases, and why the rules aren't symmetric
Measured across macOS arm64, both Linux x86-64 tiers and native Windows:
isVersionedName, enters JNA's version-pooling fallback and competes with whatever the distro ships — reintroducing the bug it is meant to fix.liblept.so.5, because the bundled tesseract's ELFNEEDEDis that literal string and no file of that name exists. Safe only alongside the unversioned link, which wins the exact-name pass first so the pool is never reached.Aliases always target the real versioned library, never another alias: the directory scan uses
NOFOLLOW_LINKS, so a link created earlier in the same pass can't become a later one's target and leave a chain that breaks when the middle link goes.Verification — Linux results measured on a Linux host, not inferred here
This was developed on macOS, where
runningLinux()is false and the Linux path cannot be exercised at all. Every Linux claim below was measured in situ on Ubuntu 24.04 against the shippedv5.5.0-9payload, and re-measured on the current head rather than carried forward:missingAliases(dir)linux-x86-64as shipped[libtesseract.so, libleptonica.so, liblept.so, liblept.so.5]linux-x86-64-legacy[libtesseract.so, libleptonica.so, liblept.so]— correctly no versioned alias[liblept.so.5]createAliases(dir)The third row is the one that matters most: it is the population who followed earlier three-symlink advice, whose aliases exist so nothing looks missing, and who are still one file short of satisfying tesseract's ELF
NEEDED.The versioned name is derived, not assumed — read from the shipped tesseract's
DT_NEEDEDrather than hardcoded. Hardcodingliblept.so.5was right on three of the four Linux tiers and wrong onlinux-x86-64-legacy, which needs no such alias. The ELF reader was validated against all eight published Linux assets and reproduces a SONAME/NEEDED matrix measured independently on two other hosts, 4 of 4.The Mach-O branch was exercised against a real Mach-O and a real ELF in one directory: it aliases the former and ignores the latter. The Windows branch is unexercised — stated rather than implied.
Design notes worth reading before the diff
On a Mac with no system tesseract, where OCR previously failed outright:
Before
After
NativeProvenanceTestis now 10 tests, including that no versioned alias is ever proposed for the JNA lookup, that aliases never chain, and that declining still yields a runnable command.The
ASKpromptIncluded, as a third commit. Shown once the window is up rather than during startup, so the first thing a user meets is the IDE and not a dialog about shared libraries — and silent unless there is something to repair.
The wording deliberately avoids implementation vocabulary — no symlinks, no JNA, no unversioned filenames. It says what is broken, what OculiX proposes to do, and what it will not touch.
Not now leaves the preference untouched so the offer returns next start — declining once is not declining forever. Never stores the refusal and prints the
ln -scommands.Verified end to end on a Mac with the aliases removed and the preference reset to
ASK: the dialog appears, "Add the links" is the default button, choosing it creates all three aliases targeting the real libraries, andOCR_ALIAS_POLICYis stored asAUTObecause the checkbox was left ticked.What review changed, and why the API looks like this
Four rounds of review across three hosts found five defects, of which two would have shipped silently:
targetFor()filtered by extension —endsWith(".so")matches neitherlibtesseract.so.5norlibleptonica.so.6, so the repair was inert on every Linux tier, and "nothing missing" is indistinguishable from "already correct". It now sniffs the object format, which is what the restriction was always for.extractionDiris mutable static, and the first touch ofCommonsre-points it via lazy init. Check-then-repair therefore straddled two directories — and that pattern is the ASK flow, with a human sitting between the check and the write. Both callers now capture the directory once and pass it everywhere.Commons, which in a shared-fork test suite means it depends on test order — a check that cannot fire, looking like a pass. Making the directory a required argument lets the compiler enforce the property instead of a comment requesting it.Two smaller ones: byte-identical content outside the extraction directory no longer counts as verified (the question is one of path, not content), and
isBindingVerified()can no longer return true having observed nothing — previously the normal Windows case.