feat: add cargo fmt check command to package.json#6
Conversation
…h dependency; update pnpm-lock.yaml for dependency resolution
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughCI replaces an action with an inline docker run; TypeScript typings broaden many async returns to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
__test__/stat.spec.ts (1)
110-129:⚠️ Potential issue | 🟡 MinorMissing Windows platform guard for pre-epoch timestamp test.
fs.utimesSyncon Windows doesn't reliably support pre-epoch (negative ms) timestamps — the call may silently no-op, causing themtimeMs < 0assertion to fail. Thecp.spec.tstimestamp test already uses the correct pattern:if (process.platform === 'win32') { t.pass('Skipping ...') return }🛡️ Proposed fix
test('statSync: atime Date should be correct for pre-epoch (negative ms) timestamp', (t) => { const dir = tmpDir() const file = join(dir, 'pre-epoch.txt') nodeFs.writeFileSync(file, 'x') + if (process.platform === 'win32') { + t.pass('Skipping pre-epoch timestamp test on Windows') + return + } // -500 ms = 1969-12-31T23:59:59.500Z🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__test__/stat.spec.ts` around lines 110 - 129, The test "statSync: atime Date should be correct for pre-epoch (negative ms) timestamp" can fail on Windows because nodeFs.utimesSync may silently no-op for negative timestamps; add a Windows guard at the start of this test (the function containing tmpDir(), file creation, nodeFs.utimesSync, statSync calls and assertions) that checks if process.platform === 'win32' then calls t.pass('Skipping pre-epoch timestamp test on Windows') and returns, so the rest of the test (using preEpoch = new Date(-500), nodeFs.utimesSync(file, preEpoch, preEpoch), const hyperStat = statSync(file), const nodeStat = nodeFs.statSync(file), and assertions on hyperStat.mtimeMs and mtime equality) only runs on platforms that support negative timestamps.
🧹 Nitpick comments (3)
src/mkdtemp.rs (1)
27-28: Considerkind = "raw-dylib"or a module-levelexternblock
raw-dylibmakes the compiler generate an import library at build time rather than requiring one at link time, and is only valid for Windows targets — a good fit here. Alternatively, lifting theexternblock and its#[link]attribute to module scope (guarded by#[cfg(windows)]) is the more conventional pattern as thelinkattribute is designed to specify the name of a native library that the compiler should link with for the items within anexternblock, and most real-world examples show it at module level rather than inside a function body.♻️ Optional refactor — module-level placement with `raw-dylib`
Move the
externblock and its#[link]to module scope, outsidegenerate_random_suffix:+#[cfg(windows)] +#[link(name = "bcrypt", kind = "raw-dylib")] +unsafe extern "system" { + fn BCryptGenRandom( + h_algorithm: *mut std::ffi::c_void, + pb_buffer: *mut u8, + cb_buffer: u32, + dw_flags: u32, + ) -> i32; +} fn generate_random_suffix() -> String { // ... #[cfg(windows)] { - // BCryptGenRandom via Windows CNG API - // Link against bcrypt.lib on MSVC to resolve BCryptGenRandom. - #[link(name = "bcrypt")] - extern "system" { - fn BCryptGenRandom( ... ) -> i32; - } const BCRYPT_USE_SYSTEM_PREFERRED_RNG: u32 = 0x00000002; let ret = unsafe { BCryptGenRandom(...) }; // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mkdtemp.rs` around lines 27 - 28, The current #[link(name = "bcrypt")] extern block is inside the function; move it to module scope and guard it with #[cfg(windows)] so the compiler links correctly (or alternatively change the attribute to #[link(kind = "raw-dylib, name = "bcrypt")] for Windows to generate an import library). Concretely, lift the extern "system" { fn BCryptGenRandom(...) } declaration and its #[link(...)] to top-level (outside generate_random_suffix) under #[cfg(windows)], leaving generate_random_suffix to call BCryptGenRandom, and keep non-Windows conditional code as-is.package.json (1)
56-56:cargo fmt -- --checkworks; consider usingcargo fmt --check(direct flag) and hooking it into CI.
--checkinstructs rustfmt to exit with an error code if the input is not formatted correctly, and will also print any found differences — making it suitable for failing CI on unformatted code. Bothcargo fmt -- --check(pass-through to rustfmt) andcargo fmt --check(direct cargo-fmt flag) are equivalent.--checkchecks formatting without modifying files, useful for CI.The
fmt:checkscript is currently a standalone entry not called by any other composite script. If formatting conformance is intended to be enforced in CI, ensure the workflow explicitly callspnpm fmt:check(or the equivalent).♻️ Optional: prefer the direct flag form
- "fmt:check": "cargo fmt -- --check", + "fmt:check": "cargo fmt --check",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 56, Update the "fmt:check" npm script to use the direct cargo-fmt flag by changing the command for the "fmt:check" script from "cargo fmt -- --check" to "cargo fmt --check", and ensure CI invokes this script (e.g., add a step that runs "pnpm fmt:check" or equivalent) so formatting failures cause the workflow to fail; target the "fmt:check" script entry in package.json and the repository CI workflow (where steps are defined) to make these changes.src/stat.rs (1)
11-21: New helpers are correct;system_time_to_mscould mirrorsecs_nanos_to_ms's decomposition style for consistency.
secs_nanos_to_mscorrectly keeps integer seconds and nanoseconds separate before converting to f64.system_time_to_mstakes the shortcutas_secs_f64() * 1000.0, which computes floating-point seconds before multiplying. For practical filesystem timestamps the precision is equivalent, but the inconsistency is worth noting.♻️ Optional: align with `secs_nanos_to_ms` style
fn system_time_to_ms(t: std::time::SystemTime) -> f64 { use std::time::UNIX_EPOCH; match t.duration_since(UNIX_EPOCH) { - Ok(d) => d.as_secs_f64() * 1000.0, - Err(e) => -(e.duration().as_secs_f64() * 1000.0), + Ok(d) => (d.as_secs() as f64) * 1000.0 + (d.subsec_nanos() as f64) / 1_000_000.0, + Err(e) => { + let d = e.duration(); + -((d.as_secs() as f64) * 1000.0 + (d.subsec_nanos() as f64) / 1_000_000.0) + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stat.rs` around lines 11 - 21, Update system_time_to_ms to mirror secs_nanos_to_ms by decomposing the Duration into integer seconds and nanoseconds then converting to milliseconds (i.e., extract d.as_secs() and d.subsec_nanos(), convert them via secs_nanos_to_ms or the same formula) for consistency; handle the Err case similarly by extracting e.duration() components and returning the negated milliseconds. Ensure you reference secs_nanos_to_ms, system_time_to_ms, and the Duration methods as the points to modify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.d.ts`:
- Around line 136-148: Remove the misleading callback-style JSDoc that documents
readdir(path, options, callback) and `@returns` {void}; instead either delete this
JSDoc block entirely or replace it with a short JSDoc matching the actual
declaration readdir(path, options?): Promise<unknown> so consumers see the
promise-based signature (ensure references to callback, `@returns` void, and
callback param types are removed).
- Line 48: The declaration file currently types all async functions as
Promise<unknown> (e.g., access, exists, stat, readFile, readdir and the other
listed async APIs), losing the concrete JsValue types from the Rust Task
implementations; update the TypeScript declarations to emit the concrete return
types (e.g., access(): Promise<void|boolean as appropriate>, exists():
Promise<boolean>, stat(): Promise<Stats>, readFile(): Promise<string|Buffer>,
readdir(): Promise<string[]|Dirent[]|...>) by fixing the NAPI-RS codegen/config
(or upgrading NAPI-RS) so the actual Task::JsValue types are propagated, and
regenerate index.d.ts; additionally remove or relocate the incorrect JSDoc block
that describes a callback-style API for readdir (the async readdir takes
ReaddirOptions and returns a Promise), so the docs match the promise-based
signature.
In `@src/mkdir.rs`:
- Around line 42-44: The error message in the fs::create_dir_all(...) error map
uses the io::Error variable `e` instead of the directory `path`, causing the OS
error text to be interpolated into Error::from_reason; update the closure passed
to fs::create_dir_all in src/mkdir.rs so it formats the path (e.g., using
path.to_string_lossy()) into the message (like the non-recursive branch does)
and keep the same descriptive prefix "ENOENT: no such file or directory, mkdir
'{}'" but substitute the path string for the placeholder instead of `e`.
---
Outside diff comments:
In `@__test__/stat.spec.ts`:
- Around line 110-129: The test "statSync: atime Date should be correct for
pre-epoch (negative ms) timestamp" can fail on Windows because nodeFs.utimesSync
may silently no-op for negative timestamps; add a Windows guard at the start of
this test (the function containing tmpDir(), file creation, nodeFs.utimesSync,
statSync calls and assertions) that checks if process.platform === 'win32' then
calls t.pass('Skipping pre-epoch timestamp test on Windows') and returns, so the
rest of the test (using preEpoch = new Date(-500), nodeFs.utimesSync(file,
preEpoch, preEpoch), const hyperStat = statSync(file), const nodeStat =
nodeFs.statSync(file), and assertions on hyperStat.mtimeMs and mtime equality)
only runs on platforms that support negative timestamps.
---
Nitpick comments:
In `@package.json`:
- Line 56: Update the "fmt:check" npm script to use the direct cargo-fmt flag by
changing the command for the "fmt:check" script from "cargo fmt -- --check" to
"cargo fmt --check", and ensure CI invokes this script (e.g., add a step that
runs "pnpm fmt:check" or equivalent) so formatting failures cause the workflow
to fail; target the "fmt:check" script entry in package.json and the repository
CI workflow (where steps are defined) to make these changes.
In `@src/mkdtemp.rs`:
- Around line 27-28: The current #[link(name = "bcrypt")] extern block is inside
the function; move it to module scope and guard it with #[cfg(windows)] so the
compiler links correctly (or alternatively change the attribute to #[link(kind =
"raw-dylib, name = "bcrypt")] for Windows to generate an import library).
Concretely, lift the extern "system" { fn BCryptGenRandom(...) } declaration and
its #[link(...)] to top-level (outside generate_random_suffix) under
#[cfg(windows)], leaving generate_random_suffix to call BCryptGenRandom, and
keep non-Windows conditional code as-is.
In `@src/stat.rs`:
- Around line 11-21: Update system_time_to_ms to mirror secs_nanos_to_ms by
decomposing the Duration into integer seconds and nanoseconds then converting to
milliseconds (i.e., extract d.as_secs() and d.subsec_nanos(), convert them via
secs_nanos_to_ms or the same formula) for consistency; handle the Err case
similarly by extracting e.duration() components and returning the negated
milliseconds. Ensure you reference secs_nanos_to_ms, system_time_to_ms, and the
Duration methods as the points to modify.
| } | ||
|
|
||
| export declare function access(path: string, mode?: number | undefined | null): Promise<void> | ||
| export declare function access(path: string, mode?: number | undefined | null): Promise<unknown> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, explore the repository structure to understand the project layout
find . -type f -name "*.rs" -o -name "*.toml" -o -name "*.d.ts" | head -20
echo "---"
ls -laRepository: CoderSerio/hyper-fs
Length of output: 2295
🏁 Script executed:
# Check the index.d.ts file to confirm the Promise<unknown> issue
head -250 index.d.ts | tail -200Repository: CoderSerio/hyper-fs
Length of output: 6578
🏁 Script executed:
# Look for Rust source files with NAPI annotations
fd -e rs . | head -20Repository: CoderSerio/hyper-fs
Length of output: 328
🏁 Script executed:
# Check the Rust lib.rs to see how async functions are defined and annotated
cat -n src/lib.rsRepository: CoderSerio/hyper-fs
Length of output: 1305
🏁 Script executed:
# Look at a few specific async function implementations to check their return types
echo "=== src/exists.rs ===" && cat src/exists.rs
echo -e "\n=== src/stat.rs ===" && cat src/stat.rs
echo -e "\n=== src/read_file.rs ===" && cat src/read_file.rsRepository: CoderSerio/hyper-fs
Length of output: 9227
🏁 Script executed:
# Check Cargo.toml to see NAPI-RS version and configuration
cat Cargo.tomlRepository: CoderSerio/hyper-fs
Length of output: 629
🏁 Script executed:
# Check the build.rs to see if there's any custom NAPI-RS configuration
cat build.rsRepository: CoderSerio/hyper-fs
Length of output: 99
🏁 Script executed:
# Verify the JSDoc placement concern by looking at the exact structure around readdir
sed -n '130,155p' index.d.ts | cat -nRepository: CoderSerio/hyper-fs
Length of output: 1020
Async functions return Promise<unknown>, losing type safety compared to sync variants.
All async functions return Promise<unknown> while their sync counterparts retain specific types: existsSync(): boolean vs exists(): Promise<unknown>, statSync(): Stats vs stat(): Promise<unknown>, readFileSync(): string | Buffer vs readFile(): Promise<unknown>. This forces consumers to add type assertions on every async result, defeating the purpose of TypeScript declarations.
The Rust source code correctly specifies return types in Task implementations (type JsValue = bool, type JsValue = Stats, etc.), but NAPI-RS 3.0.0 fails to propagate these types to the generated TypeScript declarations. The fix requires updating NAPI-RS or modifying the code generation configuration to extract and emit the actual JsValue types.
Also applies to: 56, 64, 68, 72, 76, 94, 98, 113, 117, 121, 130, 134, 167, 176, 180, 184, 188, 190, 215, 219, 223, 227, 231, 239
JSDoc block (lines 136–148) describes a callback-style API that doesn't match the promise-based function signature. The JSDoc mentions a callback parameter and @returns {void}, but the actual readdir function is async and takes ReaddirOptions, not a callback. Move or remove this documentation block to avoid confusing consumers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.d.ts` at line 48, The declaration file currently types all async
functions as Promise<unknown> (e.g., access, exists, stat, readFile, readdir and
the other listed async APIs), losing the concrete JsValue types from the Rust
Task implementations; update the TypeScript declarations to emit the concrete
return types (e.g., access(): Promise<void|boolean as appropriate>, exists():
Promise<boolean>, stat(): Promise<Stats>, readFile(): Promise<string|Buffer>,
readdir(): Promise<string[]|Dirent[]|...>) by fixing the NAPI-RS codegen/config
(or upgrading NAPI-RS) so the actual Task::JsValue types are propagated, and
regenerate index.d.ts; additionally remove or relocate the incorrect JSDoc block
that describes a callback-style API for readdir (the async readdir takes
ReaddirOptions and returns a Promise), so the docs match the promise-based
signature.
There was a problem hiding this comment.
This is caused by N-API itself and won't be resolved for now. We may add a TypeScript type library for it later. 🐰
There was a problem hiding this comment.
@CoderSerio, thank you for clarifying! I understand—the Promise<unknown> return types are a limitation of N-API's code generation rather than something that can be fixed in the Rust implementation itself. A separate TypeScript type library sounds like a practical workaround to provide the type safety at the consumer level when you're ready to tackle it. Appreciate the transparency on the technical constraints here! 🐰
✏️ Learnings added
Learnt from: CoderSerio
Repo: CoderSerio/hyper-fs PR: 6
File: index.d.ts:48-48
Timestamp: 2026-02-18T20:40:33.494Z
Learning: In the hyper-fs project (CoderSerio/hyper-fs), the async functions return `Promise<unknown>` instead of specific types (e.g., `Promise<Stats>`, `Promise<boolean>`) due to a known limitation of N-API's code generation. This won't be fixed in the index.d.ts file directly; instead, a separate TypeScript type library may be added in the future to provide proper type safety for consumers.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| /** * Reads the contents of a directory. | ||
| * @param {string | Buffer | URL} path | ||
| * @param {string | { | ||
| * encoding?: string; | ||
| * withFileTypes?: boolean; | ||
| * recursive?: boolean; | ||
| * }} [options] | ||
| * @param {( | ||
| * err?: Error, | ||
| * files?: string[] | Buffer[] | Dirent[] | ||
| * ) => any} callback | ||
| * @returns {void} | ||
| */ |
There was a problem hiding this comment.
Misleading JSDoc: documents a callback-based API that doesn't exist here.
This JSDoc block describes a callback-style readdir(path, options, callback) signature with @returns {void}, but the actual declaration on line 134 is readdir(path, options?): Promise<unknown>. The comment appears to be copied from Node.js's callback-based fs.readdir documentation and doesn't match this promise-based API surface.
If this file is truly auto-generated, this block will either be overwritten or is a manual addition that shouldn't be here. Either way, it should be removed to avoid confusing consumers.
Proposed fix: remove the misplaced JSDoc
export declare function readdir(path: string, options?: ReaddirOptions | undefined | null): Promise<unknown>
-
-/** * Reads the contents of a directory.
- * `@param` {string | Buffer | URL} path
- * `@param` {string | {
- * encoding?: string;
- * withFileTypes?: boolean;
- * recursive?: boolean;
- * }} [options]
- * `@param` {(
- * err?: Error,
- * files?: string[] | Buffer[] | Dirent[]
- * ) => any} callback
- * `@returns` {void}
- */
+
export interface ReaddirOptions {📝 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.
| /** * Reads the contents of a directory. | |
| * @param {string | Buffer | URL} path | |
| * @param {string | { | |
| * encoding?: string; | |
| * withFileTypes?: boolean; | |
| * recursive?: boolean; | |
| * }} [options] | |
| * @param {( | |
| * err?: Error, | |
| * files?: string[] | Buffer[] | Dirent[] | |
| * ) => any} callback | |
| * @returns {void} | |
| */ | |
| export declare function readdir(path: string, options?: ReaddirOptions | undefined | null): Promise<unknown> | |
| export interface ReaddirOptions { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.d.ts` around lines 136 - 148, Remove the misleading callback-style
JSDoc that documents readdir(path, options, callback) and `@returns` {void};
instead either delete this JSDoc block entirely or replace it with a short JSDoc
matching the actual declaration readdir(path, options?): Promise<unknown> so
consumers see the promise-based signature (ensure references to callback,
`@returns` void, and callback param types are removed).
CoderSerio
force-pushed
the
feat/carbon-dev
branch
from
3e107a9 to
7b4ac0e
Compare
…turn Promise<unknown> instead of Promise<void>; enhance CI workflow for testing with improved Docker run command; refine timestamp handling in stat functions for better precision and compatibility
CoderSerio
force-pushed
the
feat/carbon-dev
branch
from
7b4ac0e to
b15e90d
Compare
… validation and handle Windows-specific errors
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
.github/workflows/CI.yml (1)
226-234: LGTM — droppingaddnab/docker-run-actionin favour of an inlinedocker runis the right call.The replacement is functionally equivalent and transparent: the same
PLATFORM/IMAGEoutputs are consumed, the workspace is bind-mounted at the identical path to avoid any path-sensitive behaviour, andsh -lc(login shell) correctly populatesPATHbefore corepack/pnpm are invoked.One optional hardening worth considering: without
--user, the container runs asrootand any files written during the test run will be owned by root in the mounted workspace. Adding--user "$(id -u):$(id -g)"avoids this, although it has no practical impact here since no steps follow the test that need to read or clean those files.🔧 Optional: run container as the host user
- name: Test bindings run: | docker run --rm \ -e CI=true \ -e GITHUB_ACTIONS=true \ + --user "$(id -u):$(id -g)" \ -v "${{ github.workspace }}:${{ github.workspace }}" \ -w "${{ github.workspace }}" \ --platform "${{ steps.docker.outputs.PLATFORM }}" \ "${{ steps.docker.outputs.IMAGE }}" \ sh -lc "corepack enable && pnpm test"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/CI.yml around lines 226 - 234, Add the Docker "--user" flag to the existing docker run invocation so the container runs as the host user instead of root; update the docker run command that currently includes environment variables (CI, GITHUB_ACTIONS), volume bind-mount, working dir (-w), --platform and IMAGE outputs, and the sh -lc "corepack enable && pnpm test" invocation by inserting --user "$(id -u):$(id -g)" among the docker run options to ensure files created in the mounted workspace are owned by the host user.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__test__/stat.spec.ts`:
- Around line 127-133: The conditional comparison is contradictory because
hyperStat.mtime.getTime() is asserted to be -500, so change the guard that
decides whether to compare to Node's value: replace the current check on
nodeStat.mtime.getTime() with a check that Node also reports a negative
pre-epoch time (e.g., use nodeTime < 0 && nodeTime > -1e15) before asserting
t.is(hyperStat.mtime.getTime(), nodeTime); locate the two uses of
hyperStat.mtime.getTime() and nodeStat.mtime.getTime() in the test and update
the conditional accordingly.
In `@src/realpath.rs`:
- Around line 37-55: The code is calling GetShortPathNameW (producing 8.3 short
names) inside realpath; remove the GetShortPathNameW call and the associated
buffer/truncate/OsString conversion (refs: GetShortPathNameW, buf, short,
resolved) and simply return the canonicalized path after stripping the verbatim
prefix (i.e., replace the whole GetShortPathNameW branch with: let s =
resolved.to_string_lossy().to_string(); return Ok(strip_verbatim_prefix(s));).
If you need further normalization on Windows consider using GetLongPathNameW or
GetFinalPathNameByHandleW instead of GetShortPathNameW.
In `@src/stat.rs`:
- Around line 11-13: The private helper secs_nanos_to_ms is only used from code
inside a #[cfg(unix)] block, causing a dead-code warning on non-Unix targets;
fix it by adding the #[cfg(unix)] attribute to the secs_nanos_to_ms function
declaration so the function is compiled only on Unix targets (leave the function
name and signature unchanged so callers in the existing #[cfg(unix)] block
continue to resolve).
In `@src/utimes.rs`:
- Around line 71-82: secs_to_filetime currently casts a signed ticks_100ns i128
to u32 parts which silently wraps for negative (pre-1601) or out-of-range
values; update secs_to_filetime to validate ticks_100ns before breaking into
lo/hi: if ticks_100ns < 0 then either clamp to 0 (saturating to the minimum
FILETIME) or change the function signature to return Result<FILETIME, Error> and
return Err for out-of-range lower bound, and also check if ticks_100ns >
u64::MAX and handle/err on overflow; adjust callers of secs_to_filetime to
handle the Result if you choose the error path and ensure
FILETIME.dwLowDateTime/dwHighDateTime are computed only from a validated
non-negative u64 tick value (use ticks as u64 after validation).
- Around line 121-129: Capture the OS error immediately after detecting
SetFileTime failure (when ok == 0) before calling CloseHandle(h): call
std::io::Error::last_os_error() into a local variable (e.g., e) right away, then
call unsafe { CloseHandle(h) }, and finally return
Err(Error::from_reason(format!("{}, utimes '{}'", e, path.to_string_lossy())));
this ensures CloseHandle cannot clobber the thread-local error code used in
Error::from_reason.
---
Duplicate comments:
In `@index.d.ts`:
- Around line 136-148: The JSDoc for readdir is describing a callback-style API
and void return but the actual declaration is promise-based (readdir(path,
options?): Promise<unknown>); update the JSDoc for the readdir symbol to reflect
the promise signature: remove the callback parameter description, ensure the
`@param` for path and options match the types used in the declaration (string |
Buffer | URL and the options object with encoding/withFileTypes/recursive), and
change the `@returns` to describe the returned Promise (e.g., Promise<string[] |
Buffer[] | Dirent[]> or Promise<unknown> to match the declaration); keep the doc
concise and consistent with the function signature.
- Line 48: The async declarations like access, exists, stat, readFile, readdir,
lstat, etc. currently return Promise<unknown> causing a type-safety regression;
update each async function declaration (e.g. access, exists, stat, readFile,
readdir and the other symbols referenced) to return the proper concrete Promise
type that matches their sync counterparts (for example Promise<boolean> for
exists, Promise<fs.Stats> or equivalent for stat/lstat, Promise<string | Buffer>
for readFile, Promise<string[]> for readdir, etc.), or explicitly annotate the
correct external types imported in this .d.ts so consumers don’t need to cast;
ensure every listed symbol’s declaration is changed from Promise<unknown> to the
correct Promise<T> type.
---
Nitpick comments:
In @.github/workflows/CI.yml:
- Around line 226-234: Add the Docker "--user" flag to the existing docker run
invocation so the container runs as the host user instead of root; update the
docker run command that currently includes environment variables (CI,
GITHUB_ACTIONS), volume bind-mount, working dir (-w), --platform and IMAGE
outputs, and the sh -lc "corepack enable && pnpm test" invocation by inserting
--user "$(id -u):$(id -g)" among the docker run options to ensure files created
in the mounted workspace are owned by the host user.
f445c8d to
4cd9ceb
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mkdir.rs (1)
41-49:⚠️ Potential issue | 🟡 MinorAdd ancestor inspection to handle Windows ENOTDIR scenario
The test
mkdirSync: recursive should throw ENOTDIR when ancestor is a file(line 57-68 in__test__/mkdir.spec.ts) fails on Windows becausecreate_dir_allreturnsNotFoundinstead ofNotADirectorywhen an ancestor path component is a file. Windows raisesERROR_PATH_NOT_FOUND, which Rust maps toErrorKind::NotFound, causingmkdir_errorto returnENOENTrather thanENOTDIR. Node.js normalizes this scenario toENOTDIRon all platforms.Note that
rmdir.rsalready includes a workaround for this using string matching (e.to_string().contains("Not a directory")at line 21), confirming the issue is real. The proposed fix below is more robust: inspect ancestors whenNotFoundis returned to determine ifENOTDIRis the actual error.Proposed fix
ErrorKind::NotFound => { + // On Windows, a path-through-file scenario is reported as NotFound. + // Inspect ancestors to determine if ENOTDIR is the true error. + if let Some(parent) = path.parent() { + let mut cur = parent; + loop { + if cur.exists() { + if !cur.is_dir() { + return Error::from_reason(format!( + "ENOTDIR: not a directory, mkdir '{}'", + path_display + )); + } + break; + } + match cur.parent() { + Some(p) => cur = p, + None => break, + } + } + } Error::from_reason(format!( "ENOENT: no such file or directory, mkdir '{}'", path_display )) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mkdir.rs` around lines 41 - 49, When fs::create_dir_all(path) returns an Err with ErrorKind::NotFound, inspect the ancestor paths collected in the earlier loop (the ancestors Vec and current variable in mkdir.rs) to detect if any ancestor exists and is a file; if so convert the error to ENOTDIR before calling mkdir_error. Concretely: after the create_dir_all(...) .map_err branch, match Err(e) where e.kind() == ErrorKind::NotFound and iterate over the ancestors (and the original path components as needed) checking fs::metadata(ancestor).map(|m| m.is_file()) to determine if any ancestor is a file; if found, construct an error that maps to ENOTDIR (call mkdir_error with that ENOTDIR-like error) otherwise keep existing behavior. Ensure this logic references the existing mkdir_error function and the ancestors Vec in mkdir.rs.
🧹 Nitpick comments (2)
src/utimes.rs (1)
83-87:FILE_ATTRIBUTE_NORMALis documented as "valid only when used alone" — drop it for directories.For a directory,
flags = FILE_ATTRIBUTE_NORMAL | FILE_FLAG_BACKUP_SEMANTICS. The Windows SDK documentsFILE_ATTRIBUTE_NORMALas valid only when used alone, and forOPEN_EXISTINGthe attribute bits are read from the existing file anyway. The cleaner approach is to not setFILE_ATTRIBUTE_NORMALwhen also settingFILE_FLAG_BACKUP_SEMANTICS.♻️ Proposed fix
- let mut flags = FILE_ATTRIBUTE_NORMAL; - if path.is_dir() { - // Directories require FILE_FLAG_BACKUP_SEMANTICS. - flags |= FILE_FLAG_BACKUP_SEMANTICS; - } + let flags = if path.is_dir() { + // Directories require FILE_FLAG_BACKUP_SEMANTICS. + FILE_FLAG_BACKUP_SEMANTICS + } else { + FILE_ATTRIBUTE_NORMAL + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utimes.rs` around lines 83 - 87, The code currently initializes flags with FILE_ATTRIBUTE_NORMAL and then ORs FILE_FLAG_BACKUP_SEMANTICS for directories; change this so FILE_ATTRIBUTE_NORMAL is not combined with FILE_FLAG_BACKUP_SEMANTICS. Specifically, in src/utimes.rs adjust the flags initialization and the path.is_dir() branch (the variable named flags) so that for directories flags is set to FILE_FLAG_BACKUP_SEMANTICS only, and for non-directories it remains FILE_ATTRIBUTE_NORMAL (or 0 if that was intended), ensuring you do not OR FILE_ATTRIBUTE_NORMAL with FILE_FLAG_BACKUP_SEMANTICS.__test__/realpath.spec.ts (1)
51-58: Windowsinocomparison may be trivially true on some filesystems/older Node versions.
nodeFs.statSyncon Windows returnsino === 0on FAT32 or older Node.js runtimes, sonodeHyper.ino === nodeNode.inoreduces to0 === 0regardless of whether both paths resolve to the same file. The same applies to the async variant at Lines 80–86.Consider a stronger assertion that verifies the paths are accessible files:
♻️ Suggested alternative
- const nodeHyper = nodeFs.statSync(hyperResult) - const nodeNode = nodeFs.statSync(nodeResult) - t.true(nodeHyper.ino === nodeNode.ino && nodeHyper.dev === nodeNode.dev, 'same file') + // Resolve both through node:fs to a canonical form and verify they both exist + t.is(nodeFs.realpathSync(hyperResult), nodeFs.realpathSync(nodeResult))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__test__/realpath.spec.ts` around lines 51 - 58, On Windows the ino comparison can be unreliable; replace the ino/dev equality check (using nodeFs.statSync on hyperResult and nodeResult) with stronger assertions: verify both paths are regular files using the stats' isFile() (e.g., nodeHyper.isFile() and nodeNode.isFile()) and then assert their canonical resolved paths match by calling nodeFs.realpathSync on hyperResult and nodeResult (or, if you prefer not to call realpathSync, compare a stable combination like size and mtime from nodeHyper and nodeNode). Update both the sync block (where nodeFs.statSync is used) and the async variant accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__test__/mkdir.spec.ts`:
- Around line 57-68: The test expecting ENOTDIR for mkdirSync(recursive) when an
ancestor is a file can fail on Windows because Rust's fs::create_dir_all yields
ERROR_PATH_NOT_FOUND mapped to ENOENT; update the error mapping in the Rust
helper mkdir_error (in src/mkdir.rs) so that when creating directories
recursively and an ancestor is a file you normalize Windows
ERROR_PATH_NOT_FOUND/NotFound into NotADirectory/ENOTDIR before it surfaces to
mkdirSync; adjust the mapping logic in mkdir_error to detect this
Windows-specific case and convert it to a NotADirectory error so the test (and
mkdirSync) see ENOTDIR consistently.
In `@src/mkdir.rs`:
- Around line 89-92: The code in mkdir.rs uses ErrorKind::NotADirectory
(referenced in the match arm for mkdir error handling) which requires Rust 1.83;
either add a rust-version = "1.83" entry to Cargo.toml to declare this
requirement, or change the mkdir.rs error handling to match rmdir.rs by
replacing the ErrorKind::NotADirectory branch with a string check (e.g., inspect
the error via e.to_string().contains("Not a directory") or equivalent) so it
compiles on older Rust versions; update the match arm surrounding
ErrorKind::NotADirectory in mkdir.rs accordingly to keep behavior consistent.
In `@src/stat.rs`:
- Around line 124-136: stat_error currently maps every non-PermissionDenied
error to ENOENT; update stat_error to preserve more specific errno info by (1)
checking err.kind() for PermissionDenied and NotFound and returning
EACCES/ENOENT respectively, and (2) for other cases inspect err.raw_os_error()
and match common platform errno values (e.g. libc::ELOOP -> "ELOOP: too many
symbolic links", libc::ENOTDIR -> "ENOTDIR: not a directory", libc::ENAMETOOLONG
-> "ENAMETOOLONG: name too long") and return Error::from_reason with those
messages, falling back to a generic message (including the raw errno if present)
if no match; modify the messages created by stat_error and keep using the same
function name Error::from_reason for consistency.
---
Outside diff comments:
In `@src/mkdir.rs`:
- Around line 41-49: When fs::create_dir_all(path) returns an Err with
ErrorKind::NotFound, inspect the ancestor paths collected in the earlier loop
(the ancestors Vec and current variable in mkdir.rs) to detect if any ancestor
exists and is a file; if so convert the error to ENOTDIR before calling
mkdir_error. Concretely: after the create_dir_all(...) .map_err branch, match
Err(e) where e.kind() == ErrorKind::NotFound and iterate over the ancestors (and
the original path components as needed) checking fs::metadata(ancestor).map(|m|
m.is_file()) to determine if any ancestor is a file; if found, construct an
error that maps to ENOTDIR (call mkdir_error with that ENOTDIR-like error)
otherwise keep existing behavior. Ensure this logic references the existing
mkdir_error function and the ancestors Vec in mkdir.rs.
---
Duplicate comments:
In `@src/stat.rs`:
- Around line 12-14: Add a platform guard to avoid dead-code warnings by
annotating the secs_nanos_to_ms function with #[cfg(unix)]; locate the function
named secs_nanos_to_ms in src/stat.rs and place #[cfg(unix)] immediately above
its definition so it only compiles on Unix targets.
In `@src/utimes.rs`:
- Around line 116-134: The code currently calls CloseHandle(handle) before
reading std::io::Error::last_os_error(), which can clobber the error; change the
control flow around SetFileTime/CloseHandle so the OS error is captured before
closing the handle: after the unsafe SetFileTime call, check if ok == 0 and if
so call let e = std::io::Error::last_os_error() (and build the
Error::from_reason using e and path.to_string_lossy()), then CloseHandle(handle)
and return the Err; otherwise, proceed to CloseHandle(handle) for the success
path. Ensure you reference SetFileTime, ok, last_os_error, CloseHandle(handle),
and Error::from_reason when making the change.
- Around line 72-81: The secs_to_filetime function currently casts a
possibly-negative ticks_100ns to u32 which silently wraps for pre-1601 times;
fix by validating ticks_100ns before casting: after computing ticks_100ns (i128)
check if ticks_100ns < 0 and if so set ticks_100ns = 0 (or otherwise return a
sentinel/Error if you prefer) to avoid wrap, then compute dwLowDateTime and
dwHighDateTime from the non-negative ticks_100ns and cast to u32; update the
code paths that call secs_to_filetime if you change the return behavior (e.g.,
to Result) so callers handle the error/sentinel.
---
Nitpick comments:
In `@__test__/realpath.spec.ts`:
- Around line 51-58: On Windows the ino comparison can be unreliable; replace
the ino/dev equality check (using nodeFs.statSync on hyperResult and nodeResult)
with stronger assertions: verify both paths are regular files using the stats'
isFile() (e.g., nodeHyper.isFile() and nodeNode.isFile()) and then assert their
canonical resolved paths match by calling nodeFs.realpathSync on hyperResult and
nodeResult (or, if you prefer not to call realpathSync, compare a stable
combination like size and mtime from nodeHyper and nodeNode). Update both the
sync block (where nodeFs.statSync is used) and the async variant accordingly.
In `@src/utimes.rs`:
- Around line 83-87: The code currently initializes flags with
FILE_ATTRIBUTE_NORMAL and then ORs FILE_FLAG_BACKUP_SEMANTICS for directories;
change this so FILE_ATTRIBUTE_NORMAL is not combined with
FILE_FLAG_BACKUP_SEMANTICS. Specifically, in src/utimes.rs adjust the flags
initialization and the path.is_dir() branch (the variable named flags) so that
for directories flags is set to FILE_FLAG_BACKUP_SEMANTICS only, and for
non-directories it remains FILE_ATTRIBUTE_NORMAL (or 0 if that was intended),
ensuring you do not OR FILE_ATTRIBUTE_NORMAL with FILE_FLAG_BACKUP_SEMANTICS.
| fn stat_error(path: &Path, err: std::io::Error) -> Error { | ||
| if err.kind() == ErrorKind::PermissionDenied { | ||
| Error::from_reason(format!( | ||
| "EACCES: permission denied, stat '{}'", | ||
| path.to_string_lossy() | ||
| )) | ||
| } else { | ||
| Error::from_reason(format!( | ||
| "ENOENT: no such file or directory, stat '{}'", | ||
| path.to_string_lossy() | ||
| )) | ||
| } | ||
| } |
There was a problem hiding this comment.
stat_error silently collapses all non-PermissionDenied errors into ENOENT.
Several distinct failure modes are reachable from fs::metadata / fs::symlink_metadata and would be misreported:
| Actual error | Actual errno | Reported by stat_error |
|---|---|---|
| Symlink loop | ELOOP |
ENOENT ❌ |
| Path component not a directory | ENOTDIR |
ENOENT ❌ |
| Name too long | ENAMETOOLONG |
ENOENT ❌ |
This can produce confusing diagnostic messages for callers. Consider matching additional ErrorKind variants:
🛠️ Proposed fix
fn stat_error(path: &Path, err: std::io::Error) -> Error {
if err.kind() == ErrorKind::PermissionDenied {
Error::from_reason(format!(
"EACCES: permission denied, stat '{}'",
path.to_string_lossy()
))
+ } else if err.kind() == ErrorKind::NotFound {
+ Error::from_reason(format!(
+ "ENOENT: no such file or directory, stat '{}'",
+ path.to_string_lossy()
+ ))
} else {
Error::from_reason(format!(
- "ENOENT: no such file or directory, stat '{}'",
+ "Error: {}, stat '{}'",
+ err,
path.to_string_lossy()
))
}
}📝 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.
| fn stat_error(path: &Path, err: std::io::Error) -> Error { | |
| if err.kind() == ErrorKind::PermissionDenied { | |
| Error::from_reason(format!( | |
| "EACCES: permission denied, stat '{}'", | |
| path.to_string_lossy() | |
| )) | |
| } else { | |
| Error::from_reason(format!( | |
| "ENOENT: no such file or directory, stat '{}'", | |
| path.to_string_lossy() | |
| )) | |
| } | |
| } | |
| fn stat_error(path: &Path, err: std::io::Error) -> Error { | |
| if err.kind() == ErrorKind::PermissionDenied { | |
| Error::from_reason(format!( | |
| "EACCES: permission denied, stat '{}'", | |
| path.to_string_lossy() | |
| )) | |
| } else if err.kind() == ErrorKind::NotFound { | |
| Error::from_reason(format!( | |
| "ENOENT: no such file or directory, stat '{}'", | |
| path.to_string_lossy() | |
| )) | |
| } else { | |
| Error::from_reason(format!( | |
| "Error: {}, stat '{}'", | |
| err, | |
| path.to_string_lossy() | |
| )) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/stat.rs` around lines 124 - 136, stat_error currently maps every
non-PermissionDenied error to ENOENT; update stat_error to preserve more
specific errno info by (1) checking err.kind() for PermissionDenied and NotFound
and returning EACCES/ENOENT respectively, and (2) for other cases inspect
err.raw_os_error() and match common platform errno values (e.g. libc::ELOOP ->
"ELOOP: too many symbolic links", libc::ENOTDIR -> "ENOTDIR: not a directory",
libc::ENAMETOOLONG -> "ENAMETOOLONG: name too long") and return
Error::from_reason with those messages, falling back to a generic message
(including the raw errno if present) if no match; modify the messages created by
stat_error and keep using the same function name Error::from_reason for
consistency.
4cd9ceb to
44ac7e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/utimes.rs (1)
83-87:FILE_ATTRIBUTE_NORMAL | FILE_FLAG_BACKUP_SEMANTICSis slightly non-idiomatic for directories.The MSDN documentation for
CreateFileWstates thatFILE_ATTRIBUTE_NORMAL"is valid only when used alone." For directories (OPEN_EXISTING+FILE_FLAG_BACKUP_SEMANTICS), the file-attributes portion ofdwFlagsAndAttributesis effectively ignored, so this works in practice, but usingFILE_FLAG_BACKUP_SEMANTICSalone (or0 | FILE_FLAG_BACKUP_SEMANTICS) for the directory branch better matches API intent.♻️ Suggested adjustment
- let mut flags = FILE_ATTRIBUTE_NORMAL; + let flags; if path.is_dir() { // Directories require FILE_FLAG_BACKUP_SEMANTICS. - flags |= FILE_FLAG_BACKUP_SEMANTICS; + flags = FILE_FLAG_BACKUP_SEMANTICS; + } else { + flags = FILE_ATTRIBUTE_NORMAL; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utimes.rs` around lines 83 - 87, The flags handling uses FILE_ATTRIBUTE_NORMAL OR'ed with FILE_FLAG_BACKUP_SEMANTICS for directories; change the directory branch so that when path.is_dir() is true you set flags = FILE_FLAG_BACKUP_SEMANTICS (or 0 | FILE_FLAG_BACKUP_SEMANTICS) instead of OR'ing with FILE_ATTRIBUTE_NORMAL. Update the code that assigns to flags (the variable named flags used for CreateFileW) so FILE_ATTRIBUTE_NORMAL is only used for non-directory files and directories pass only FILE_FLAG_BACKUP_SEMANTICS.Cargo.toml (1)
25-30: Consider upgradingwindows-systo the latest0.61.xfor build-time improvements.
windows-sys0.59.0 is two minor versions behind the current latest (0.61.2, released 2025-09-25). Starting from 0.60,windows-syswas updated to usewindows-linkandraw-dylibunconditionally, which can meaningfully reduce build time. The declared features (Win32_Foundation,Win32_Security,Win32_Storage_FileSystem) are correct and sufficient for all usages insrc/utimes.rs. No API-breaking changes are expected for these features.♻️ Suggested upgrade
[target.'cfg(windows)'.dependencies] -windows-sys = { version = "0.59.0", features = [ +windows-sys = { version = "0.61.0", features = [ "Win32_Foundation", "Win32_Security", "Win32_Storage_FileSystem", ] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` around lines 25 - 30, Update the windows-sys dependency declaration by bumping its version from "0.59.0" to the latest 0.61.x series to pick up build-time improvements; locate the Cargo.toml dependency block that declares target.'cfg(windows)'.dependencies and the windows-sys entry (currently version "0.59.0" with features "Win32_Foundation", "Win32_Security", "Win32_Storage_FileSystem") and change the version to "0.61" (or a specific 0.61.x like "0.61.2"), leaving the feature list unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__test__/realpath.spec.ts`:
- Around line 51-58: The Windows branch's file-identity check using
nodeFs.statSync(...).ino/dev is unreliable (zeros on many Windows filesystems);
replace that check by comparing resolved canonical paths: call
nodeFs.realpathSync(hyperResult) === nodeFs.realpathSync(nodeResult) for the
sync test (instead of the ino/dev comparison) and for the async test use
nodeFs.promises.realpath(hyperResult) and nodeFs.promises.realpath(nodeResult)
and assert equality; keep the existing
t.true(hyperResult.endsWith('real-target.txt')) unchanged.
---
Duplicate comments:
In `@src/stat.rs`:
- Around line 124-136: The stat_error function incorrectly maps every
non-PermissionDenied error to an ENOENT message; update stat_error (function
name: stat_error) to distinguish ErrorKind::NotFound from other ErrorKind
variants and preserve/or include the actual error kind/message for unexpected
kinds (e.g., ELOOP, ENOTDIR, ENAMETOOLONG). Specifically: keep the
PermissionDenied branch, add a branch that handles ErrorKind::NotFound and
returns the ENOENT message, and add a default branch that uses err.kind() (or
err.to_string()/raw OS error) to construct a more accurate Error::from_reason so
non-NotFound errors are reported with their real kind/message rather than being
collapsed to ENOENT.
In `@src/utimes.rs`:
- Around line 116-134: The code calls CloseHandle(handle) before reading the OS
error from std::io::Error::last_os_error(), which can clobber the thread-local
error; change the flow so that after SetFileTime returns (checking ok == 0) you
immediately capture the error (e.g., let e = std::io::Error::last_os_error())
before calling CloseHandle(handle), then close the handle and return
Err(Error::from_reason(format!("{}, utimes '{}'", e, path.to_string_lossy())));
keep the existing symbols (SetFileTime, ok, handle, CloseHandle, last_os_error,
Error::from_reason, path.to_string_lossy) to locate and update the code.
---
Nitpick comments:
In `@Cargo.toml`:
- Around line 25-30: Update the windows-sys dependency declaration by bumping
its version from "0.59.0" to the latest 0.61.x series to pick up build-time
improvements; locate the Cargo.toml dependency block that declares
target.'cfg(windows)'.dependencies and the windows-sys entry (currently version
"0.59.0" with features "Win32_Foundation", "Win32_Security",
"Win32_Storage_FileSystem") and change the version to "0.61" (or a specific
0.61.x like "0.61.2"), leaving the feature list unchanged.
In `@src/utimes.rs`:
- Around line 83-87: The flags handling uses FILE_ATTRIBUTE_NORMAL OR'ed with
FILE_FLAG_BACKUP_SEMANTICS for directories; change the directory branch so that
when path.is_dir() is true you set flags = FILE_FLAG_BACKUP_SEMANTICS (or 0 |
FILE_FLAG_BACKUP_SEMANTICS) instead of OR'ing with FILE_ATTRIBUTE_NORMAL. Update
the code that assigns to flags (the variable named flags used for CreateFileW)
so FILE_ATTRIBUTE_NORMAL is only used for non-directory files and directories
pass only FILE_FLAG_BACKUP_SEMANTICS.
| if (process.platform === 'win32') { | ||
| const nodeHyper = nodeFs.statSync(hyperResult) | ||
| const nodeNode = nodeFs.statSync(nodeResult) | ||
| t.true(nodeHyper.ino === nodeNode.ino && nodeHyper.dev === nodeNode.dev, 'same file') | ||
| } else { | ||
| t.is(hyperResult, nodeResult) | ||
| } | ||
| t.true(hyperResult.endsWith('real-target.txt')) |
There was a problem hiding this comment.
Windows ino/dev comparison may be trivially true on some filesystems.
The reference/fs.js included in this repo explicitly notes that dev/ino always return 0 on Windows (older libuv/Node.js behavior), and on non-NTFS volumes (ReFS, FAT32, network shares) ino can still be 0 with modern Node.js. When both nodeHyper.ino and nodeNode.ino are 0, the assertion nodeHyper.ino === nodeNode.ino && nodeHyper.dev === nodeNode.dev is trivially true regardless of whether the paths resolve to the same file, making the test vacuous on those configurations.
The unconditional t.true(hyperResult.endsWith('real-target.txt')) at line 58 still provides meaningful coverage, so this is not a full gap, but the Windows branch's ino/dev check could be replaced with a more robust comparison — e.g. comparing path normalization via nodeFs.realpathSync, consistent with the approach used in the non-symlink tests at lines 22-26.
♻️ More robust Windows assertion
if (process.platform === 'win32') {
- const nodeHyper = nodeFs.statSync(hyperResult)
- const nodeNode = nodeFs.statSync(nodeResult)
- t.true(nodeHyper.ino === nodeNode.ino && nodeHyper.dev === nodeNode.dev, 'same file')
+ t.is(nodeFs.realpathSync(hyperResult), nodeFs.realpathSync(nodeResult))
} else {
t.is(hyperResult, nodeResult)
}The same pattern applies to the async test at lines 80-86.
📝 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.
| if (process.platform === 'win32') { | |
| const nodeHyper = nodeFs.statSync(hyperResult) | |
| const nodeNode = nodeFs.statSync(nodeResult) | |
| t.true(nodeHyper.ino === nodeNode.ino && nodeHyper.dev === nodeNode.dev, 'same file') | |
| } else { | |
| t.is(hyperResult, nodeResult) | |
| } | |
| t.true(hyperResult.endsWith('real-target.txt')) | |
| if (process.platform === 'win32') { | |
| t.is(nodeFs.realpathSync(hyperResult), nodeFs.realpathSync(nodeResult)) | |
| } else { | |
| t.is(hyperResult, nodeResult) | |
| } | |
| t.true(hyperResult.endsWith('real-target.txt')) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@__test__/realpath.spec.ts` around lines 51 - 58, The Windows branch's
file-identity check using nodeFs.statSync(...).ino/dev is unreliable (zeros on
many Windows filesystems); replace that check by comparing resolved canonical
paths: call nodeFs.realpathSync(hyperResult) === nodeFs.realpathSync(nodeResult)
for the sync test (instead of the ino/dev comparison) and for the async test use
nodeFs.promises.realpath(hyperResult) and nodeFs.promises.realpath(nodeResult)
and assert equality; keep the existing
t.true(hyperResult.endsWith('real-target.txt')) unchanged.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests