feat(io-lib): replace custom physical I/O with Okio Multiplatform - #925
feat(io-lib): replace custom physical I/O with Okio Multiplatform#925gciatto wants to merge 18 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
A few concrete correctness/robustness issues remain (JVM file URL decoding, JS exception translation, and JVM stream closing/leak risks) that can affect runtime behavior and error handling.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refactors :io-lib local filesystem I/O to use Okio Multiplatform with an internal injectable LocalFileSystem seam, while keeping remote fetching behavior unchanged (JVM java.net.URL, JS sync-request) and preserving existing Prolog-level APIs.
Changes:
- Introduces
LocalFileSystem(OkioFileSystemseam) and adds Okio dependencies (incl. Node and Fake FS). - Migrates JVM and JS/Node local file reads/writes to Okio, adding Okio-based channel adapters for JS.
- Adds new common tests for the seam (
FakeFileSystem) and end-to-end real temp-file tests foropen/3,4.
File summaries
| File | Description |
|---|---|
| io-lib/src/commonMain/kotlin/it/unibo/tuprolog/solve/libs/io/LocalFileSystem.kt | Adds injectable Okio FileSystem seam for local I/O. |
| io-lib/src/commonMain/kotlin/it/unibo/tuprolog/solve/libs/io/UrlUtils.kt | Adds toLocalPath() expect and Okio Path integration point. |
| io-lib/src/commonMain/kotlin/it/unibo/tuprolog/solve/libs/io/channel/OkioChannels.kt | Adds Okio-backed Input/OutputChannel implementations for JS/Node. |
| io-lib/src/jvmMain/kotlin/it/unibo/tuprolog/solve/libs/io/UrlUtilsJvm.kt | Switches JVM local open/read/write to Okio; defines JVM platform filesystem. |
| io-lib/src/jvmMain/kotlin/it/unibo/tuprolog/solve/libs/io/JvmUrl.kt | Switches JVM local readAsText/readAsByteArray to Okio. |
| io-lib/src/jsMain/kotlin/it/unibo/tuprolog/solve/libs/io/UrlUtilsJs.kt | Switches JS/Node local open/read/write to Okio; keeps browser fallback. |
| io-lib/src/jsMain/kotlin/it/unibo/tuprolog/solve/libs/io/RemoteAndBrowserIO.kt | Extracts JS remote-fetch + browser-localStorage fallback utilities. |
| io-lib/src/jsMain/kotlin/it/unibo/tuprolog/solve/libs/io/JsUrl.kt | Switches JS local reads to Okio on Node, preserves browser fallback. |
| io-lib/src/jsMain/kotlin/it/unibo/tuprolog/solve/libs/io/FileSystem.kt | Removes custom Node fs/path implementation (replaced by Okio). |
| io-lib/src/commonTest/kotlin/it/unibo/tuprolog/solve/libs/io/TestLocalFileSystem.kt | Adds deterministic common tests using Okio FakeFileSystem. |
| io-lib/src/commonTest/kotlin/it/unibo/tuprolog/solve/libs/io/TestOpenLocalFile.kt | Adds real temp-file smoke tests for open/write/append round-trips. |
| io-lib/build.gradle.kts | Adds Okio + fakefilesystem + nodefilesystem dependencies. |
| gradle/libs.versions.toml | Adds Okio version and catalog aliases. |
Review details
Suppressed comments (2)
io-lib/src/jsMain/kotlin/it/unibo/tuprolog/solve/libs/io/UrlUtilsJs.kt:43
- In JS/Node mode, openOutputChannel() can throw okio.IOException while opening the sink (e.g. invalid path / permissions). Those exceptions won’t be translated to it.unibo…IOException and may escape as an uncaught exception instead of a Prolog I/O error.
val path = toLocalPath()
val sink = if (append) LocalFileSystem.appendingSink(path) else LocalFileSystem.sink(path)
return SinkOutputChannel(sink.buffer())
io-lib/src/jvmMain/kotlin/it/unibo/tuprolog/solve/libs/io/JvmUrl.kt:54
- The non-file readAsByteArray() branch opens a URL stream but never closes it, which can leak resources (HTTP connections / file descriptors).
BufferedInputStream(url.openStream()).readAllBytes()
- Files reviewed: 13/13 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| val path = toLocalPath() | ||
| val sink = if (append) LocalFileSystem.appendingSink(path) else LocalFileSystem.sink(path) | ||
| return WriterChannel(sink.buffer().outputStream()) |
| actual fun Url.openInputChannel(): InputChannel<String> = | ||
| if (isFile && isNode) { | ||
| SourceInputChannel(LocalFileSystem.source(toLocalPath()).buffer()) | ||
| } else { | ||
| InputChannel.of(readAsText()) | ||
| } |
| if (isFile) { | ||
| LocalFileSystem.source(toLocalPath()).buffer().use { it.readUtf8() } | ||
| } else { | ||
| BufferedReader(InputStreamReader(url.openStream())).lines().asSequence().joinToString("\n") |
01d3cdb to
b8e454e
Compare
|
Pushed a fix for the Windows CI failures reported on this branch ( Root cause: Fix ( Also added 🤖 Generated with Claude Code |
|
Pushed a follow-up (`6b9a16de0`) replacing the hand-rolled JS regex URL parser with the native `URL` (WHATWG, global in Node/browsers) and Node's own `url.fileURLToPath` for local-path resolution, instead of hand-rolled slash/leading-slash surgery. Deletes `Url.kt`'s `urlRegex`/`UrlField`/`parse` machinery entirely (it was already dead on JVM, which uses `java.net.URI`). A couple of harmless, spec-driven behavior differences surfaced and are reflected in the tests (not regressions): WHATWG normalizes away an explicit port matching the scheme default, and a bare host's implicit path to This is a single, self-contained, easy-to-revert commit — waiting on Windows CI to confirm it holds up there too. 🤖 Generated with Claude Code |
062d426 to
7511cca
Compare
be4dbbb to
eda8c87
Compare
Local file/byte-stream I/O in :io-lib now goes through Okio (FileSystem.SYSTEM
on JVM, NodeJsFileSystem on Node) via a small injectable LocalFileSystem seam,
replacing the hand-rolled java.io/java.nio path resolution on JVM and the
require('fs')/require('path') code on JS. Remote (http/https) fetching is
untouched (java.net.URL / sync-request), since Okio isn't an HTTP client and
the Solver's I/O is synchronous. The browser localStorage fallback is kept,
since Okio has no synchronous file system for browsers.
As a scoped, low-risk extension of the same substrate, local file *write*
support is added on JS/Node (open(File, write/append, Stream) previously
always failed there) via NodeJsFileSystem's sink/appendingSink.
Adds test coverage that didn't exist before: common tests against Okio's
FakeFileSystem for the LocalFileSystem seam (read/write/truncate/append/
flush/close/missing-file/invalid-path), and an end-to-end open/3 write+append
round-trip against a real temp file (doubling as the real-filesystem smoke
suite under both jvmTest and jsNodeTest).
Refs #923
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Url's hand-rolled JS-side regex parser (used by Url.of, exercised via consult/1, include, load, and open/3,4 whenever a Prolog atom must be re-parsed into a Url) didn't recognize `\` as a path separator and let a backslash-only path be swallowed whole into a "host" whenever it happened to end in something matching `name.ext`. Combined with backtracking around the optional (and previously discarded) drive-letter group, round-tripping a Windows absolute path through Url.toString() -> Url.of() corrupted it into a bogus Unix-rooted path (a spurious leading slash before the drive letter), causing FileNotFoundException on Node/Windows for otherwise-correct code. Fixes the regex to: accept `\` as an additional path separator, exclude backslash from the host pattern (so a plain `C:\a\b\c.pl` isn't mistaken for a dotted hostname), and normalize away the standard `file:///D:/...` URI's extra slash before the drive letter (only when a drive letter actually follows, so it doesn't rob a plain root path like `http://host:80/` of its own `/`). Verified against every URL literal already used across the test suite plus the reported Windows failure shapes, with no behavior change on non-Windows paths (mixed-radix and pure-Unix cases already covered by the existing macOS/Linux-passing suite). Adds TestWindowsPathParsing (jsTest): pure string-parsing regression tests for native Windows path round-tripping, so this class of bug is caught on every CI platform rather than only Windows. Fixes the reported failures on Windows CI: - TestClassicConsult.testConsultWorksLocally[js, node] - TestClassicInclude.testLocalInclude[js, node] - TestClassicInclude.testLocalLoad[js, node] Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…enation Url.file(path) round-tripped the raw path through a hand-built "file://"+path string parsed by java.net.URI, which is strict RFC-3986 syntax and rejects unescaped backslashes (and spaces) - breaking on Windows native paths (e.g. Okio's SYSTEM_TEMPORARY_DIRECTORY / name .toString()). File(path).toURI() already does this conversion correctly and portably (drive letters, \ -> /, percent-encoding), so delegate to it instead. Fixes TestOpenLocalFile failures on jvm/Windows CI: - testOpenForWriteThenReadBackThroughRealLocalFileSystem[jvm] - testOpenForWriteTruncatesExistingContent[jvm] - testOpenForAppendAddsToExistingContent[jvm] (The unrelated ClausesParserErrorTest StackOverflowError on Windows is in parser-theory, pre-existing, out of scope here.) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
close() only marked the channel closed, never calling writer.close() - unlike its sibling ReaderChannel, which already does this correctly. On Linux/Mac this leak is invisible (you can delete/reopen a file with a dangling open handle); Windows enforces exclusive-delete locking, so the leaked file handle from open(File, write, S) blocked deleting that file afterwards. Fixes TestOpenLocalFile[jvm] cleanup failures on Windows CI (the test bodies already passed; @AfterTest's delete was failing on the still-open handle). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…d regex Replaces Url's commonMain regex parser (urlRegex/UrlField/parse - dead code on JVM, which already uses java.net.URI) with the global WHATWG URL on JS, and delegates file:-URL <-> native-path conversion to Node's own url.fileURLToPath instead of hand-rolled leading-slash/separator surgery in toLocalPath. Trades twice-already-buggy custom parsing for the same battle-tested facility the JVM side already relies on. Behavior notes surfaced by this swap, all pre-existing platform quirks of java.net.URL vs WHATWG URL rather than regressions: - WHATWG normalizes away a port matching the scheme default (e.g. :80 for http) and a bare host's implicit path to "/"; java.net.URL does neither. TestUrl's fixtures/assertions are adjusted accordingly. - A bare native path (e.g. `C:\Users\...`, no scheme at all) parses "successfully" under WHATWG as a URL with a single-letter scheme (the drive letter), since single-letter schemes are otherwise valid URI syntax; the old regex rejected it outright (no "//" right after the colon), correctly forcing Url.of's file:// fallback. Replicated by explicitly rejecting single-letter "protocols" in JsUrl. TestWindowsPathParsing is updated to check `.path` (pure string parsing, portable to any CI host) or the *comparative* result of toLocalPath (both sides resolved against the same host) rather than hardcoded native-path strings, since fileURLToPath's actual output is host-OS-dependent by design (there's no Windows drive to resolve to on Linux/macOS) - that part can only be meaningfully validated by Windows CI itself, against the real filesystem, via TestOpenLocalFile and the consult/include/load tests. This is easy to revert on its own: it only touches JS-side URL parsing and one shared test file, independent of the earlier Okio migration and JVM fixes already on this branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
new URL(...) throws a plain JS TypeError, not a specific Kotlin exception type - same reason RemoteAndBrowserIO.kt already suppresses this rule for its own JS-interop catch (Throwable) block. Fixes detektJsMain CI failure. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…haped path On Windows, Node's fileURLToPath requires either a UNC host or a genuine <letter>: drive prefix, and throws (a TypeError) for a plain Unix-shaped absolute path like /path/to/missing/resource.pl - even though Node's own fs calls happily resolve such a path relative to the current drive. This is exactly the shape TestConsultImpl's "missing theory" and TestIncludeImpl's "missing include/load" fixtures use (shared via commonTest, so exercised on every platform), and was crashing them on Windows instead of producing the expected "file not found". toLocalPath now falls back to the URL's plain (already /-rooted) path component when fileURLToPath throws, which real fs calls resolve fine. Fixes on Windows CI: - TestClassicConsult.testConsultingMissingTheoryWorksLocally[js, node] - TestClassicInclude.testMissingInclude[js, node] - TestClassicInclude.testMissingLoad[js, node] - TestStreamsConsult.testConsultingMissingTheoryWorksLocally[js, node] Also suppresses TooGenericExceptionCaught for the same JS-interop reason as JsUrl.kt's identical pattern (a plain Throwable is genuinely all fileURLToPath throws), and adds a portable regression test (must-not-throw, since the exact resolved path is host-OS-dependent by design). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…back Matches the existing convention (Url.of's catch (_: InvalidUrlException)) for a deliberately-discarded exception. Fixes detektJsMain's SwallowedException finding. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…S exception wrapping) - toLocalPath (JVM): use toURL().toURI() instead of the raw (still percent-encoded) URL.file, so a file: URL with e.g. a space (%20) resolves to the real path instead of a literal "%20" in the filename. - JvmUrl readAsText/readAsByteArray: close the remote-branch stream (BufferedReader/BufferedInputStream) instead of leaking it - unrelated to this PR's local-file changes (this branch is untouched original code), but a real leak worth fixing since it's free. - UrlUtilsJs openInputChannel/openOutputChannel (isFile && isNode branch): wrap okio.IOException into the library's own IOException, matching what readAsText/readAsByteArray already do via readLocalFile. Previously a missing/unreadable local file on open/3,4 would let a raw okio exception escape uncaught instead of becoming a Prolog I/O error - a real regression from the old JS code, which incidentally got this via readAsText(). (WriterChannel.close() not closing its underlying Writer, also flagged by the same review, was already fixed in 91e8380.) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replaces the JVM/JS expect-actual split with one common implementation: InputChannel<String>.read() already matches TextChunkSource.readChunk(), so term parsing can run through parser-impl's platform-agnostic lexer directly, instead of requiring a JVM java.io.Reader. This also fixes the JVM version only working for solve's ReaderChannel, not any InputChannel. Widens PrologSyntaxException.toParseException from internal to public so io-lib can reuse it, and adds a parser-impl dependency to io-lib. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
57ff2ad to
f49b1fa
Compare
…ious chars
InputStoreImpl/OutputStoreImpl.setCurrent(alias) rebuilt the whole store
as just {"$current" -> channel}, discarding every other registered
alias (including the one just switched to). InputChannelFromString
(the JS/Node backend for InputChannel.of(string)) synthesized a
trailing '\n' after every line, even an empty one, so string-backed
input never behaved like the JVM's StringReader-backed channel.
Both were caught by io-lib's I/O predicate tests; those tests are
restored to their cross-platform form now that the underlying bugs
are fixed (one JS-only case is left pinned, tied to an unrelated
NumberTypeTester bug misclassifying negative integers as reals).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The integer-detection regex ("[0-9]+") didn't allow a leading '-', so
negative numbers were misclassified as decimals. On JS, this made the
DSL termify e.g. -1 as a real (-1.0) instead of an integer, which was
masking as a JS-only quirk in an io-lib test; that test is restored to
its cross-platform form now that the underlying bug is fixed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…mification
PlatformSpecificValues.MINUS_THREE on JS was pinned to Real.of("-3.0"),
encoding the NumberTypeTester regex bug just fixed in :utils (negative
integers used to be misclassified as reals on JS). Now that -3 is
correctly termified as an integer there too, update the expectation to
match, fixing the resulting failures in TestLegacyTermifier,
TestDefaultTermifier and TestMinimalLogicProgrammingScope on JS.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
bec7c60 to
63a96b0
Compare
|
@codex investigate why the failures on JVM/Windows only |
|
To use Codex here, create an environment for this repo. |
TestOpen used a FakeFileSystem with bare root-relative paths like "/parents.pl", written directly into the fake, then opened via open/3,4 through Url.file(path).toString(). That string round-trips through java.io.File/URI/URL regardless of which FileSystem LocalFileSystem.fileSystem points to, and on Windows a driveless absolute path resolves against the current drive (e.g. "C:\parents.pl"), which never matches the driveless key the fake filesystem stored it under - causing a deterministic FileNotFoundException there, and a stale read of pre-existing content in the append test. Switched to real temp files under FileSystem.SYSTEM_TEMPORARY_DIRECTORY, mirroring TestOpenLocalFile's already-portable pattern, so writes and the open/3,4-driven reads resolve through the exact same real path on every platform. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The other open/3,4 tests explicitly close the stream they open, but this one didn't, leaving the file handle open when tearDown tried to delete the temp file. On Windows, deleting a file with an open handle fails with an IOException; on Unix it's silently allowed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
Closes #923 (issue 1/2 of the I/O work; must land before the companion 2/2 ISO-I/O issue).
Refactors
:io-libso local file/byte-stream I/O is implemented on top of Okio Multiplatform, per the issue's design:LocalFileSystemseam (commonMain, internal) wrapping an injectableokio.FileSystem—FileSystem.SYSTEMon JVM,NodeJsFileSystem(from the newokio-nodefilesystemdependency) on Node — swappable in tests with Okio'sFakeFileSystem.ReaderChannel/WriterChannel(preservesread_termsupport, which depends on that concrete type). Remote (http/https) fetching is untouched (java.net.URL).open(File, write/append, Stream)previously always failed on JS) go throughNodeJsFileSystemon Node, via two new small common channel classes (SourceInputChannel/SinkOutputChannel). The browserlocalStoragefallback and thesync-request-based remote fetch are both kept as-is — Okio isn't an HTTP client and has no synchronous filesystem for browsers, so neither can be migrated; both are called out inline as deliberate, scoped exceptions.require('fs')/require('path')code and the manual Windows path massaging it needed (Okio'sPathnormalizes separators per platform on its own).gradle/libs.versions.toml: addedokio,okio-nodefilesystem,okio-fakefilesystemat3.18.1.New capability called out explicitly
Local file write support on JS/Node is added as a scoped, low-risk extension of the same substrate (a few lines via
NodeJsFileSystem.sink/appendingSink) — it only turns previously-always-failingopen(File, write, S)calls into working ones, matching the issue's explicit goal #4 ("Local file output in:io-libis implemented through Okio").Test coverage added (there was none for this before)
TestLocalFileSystem: common tests against Okio'sFakeFileSystemfor theLocalFileSystemseam — read text/bytes, create/write, overwrite/truncate, append, flush, close, missing-file, invalid-path.TestOpenLocalFile: end-to-endopen/3write+append round-trip against a real temp file (viaFileSystem.SYSTEM_TEMPORARY_DIRECTORY), doubling as the real-filesystem smoke suite the issue asks for — runs for real under both:io-lib:jvmTestand:io-lib:jsNodeTest.TestUrl,TestConsult*,TestInclude*,TestIOLibsuites are unmodified and still pass — the regression net proving Prolog-level behavior didn't move.Test plan
./gradlew :io-lib:jvmTest— passes./gradlew :io-lib:jsNodeTest— passes./gradlew :io-lib:ktlintFormat && ./gradlew :io-lib:check(detekt excluded, see below) — passes:ide:compileKotlin,:ide-plp:compileKotlin,:repl:compileKotlinJvm— passio-lib/srcforjava.io./java.nio./java.net.(only remains in the untouched remote-fetch paths),require((onlysync-requestremains),localStorage(only the browser fallback remains)Note:
detektJsMain(and otherdetekt*tasks) were excluded from thecheckrun above — they fail identically on unmodifiedmasterwith> 26.0.2.1, a pre-existing incompatibility between the pinned detekt version and JDK 26 unrelated to this change. Confirmed by running the same task againstmasterbefore making any changes.🤖 Generated with Claude Code