Implement Darwin (MacOS)#169
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds macOS builds and CI coverage, Darwin-specific configuration and filesystem implementations, Linux capability handling, cross-platform tests, quota fallbacks, macOS documentation, and lower-severity websocket logging. ChangesmacOS platform support
Websocket logging levels
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 4
🤖 Fix all issues with AI agents
In `@internal/ufs/fs_darwin.go`:
- Around line 40-58: The code in UnixFS.Chtimesat uses linux-style Stat_t fields
Atim/Mtim which don't exist on macOS; replace references to st.Atim and st.Mtim
with Darwin names st.Atimespec and st.Mtimespec and convert their timespecs to
time.Time (e.g., using st.Atimespec.Unix()/UnixNano() or an equivalent
conversion) before assigning atime/mtime so the function compiles and uses the
correct Darwin fields.
In `@internal/ufs/fs_unix.go`:
- Around line 653-660: The field useOpenat2 on UnixFS is written without
synchronization causing a data race; change the UnixFS struct to use an
atomic.Bool for useOpenat2, update all reads to use fs.useOpenat2.Load() and the
write in the open path to fs.useOpenat2.Store(false) (so the branch around
fs._openat2 / fs._openat remains correct), and add the "sync/atomic" import (or
the atomic package alias used) so the race detector is silenced.
In `@internal/ufs/walk_linux.go`:
- Around line 18-33: The defensive fallback in nameFromDirent uses
name[:len(de.Name)] which can panic when ml (derived from de.Reclen) is smaller
than len(de.Name); change the fallback to return the already-sliced name (the
variable name) directly so you never slice past its length—update the fallback
in function nameFromDirent to return name instead of name[:len(de.Name)].
In `@Makefile`:
- Line 24: The cross-build target currently depends on "clean build compress"
but omits "build-darwin" and references a non-existent "compress" target; update
the cross-build dependency list to include build-darwin (so cross-build produces
darwin artifacts) and either add a well-defined compress target (e.g.,
packaging/compression step) or remove "compress" from the dependency list; edit
the Makefile entry for the cross-build target and verify the related targets
"build", "build-darwin", and "clean" are present and correctly implemented.
🧹 Nitpick comments (8)
router/router_server_ws.go (1)
78-78: Log level downgrade for websocket close failure looks intentional and consistent.This aligns with the similar changes in
router/websocket/listeners.go. However, a failed close could occasionally indicate a real issue (e.g., resource leak). Consider usingWarnas a middle ground so these don't get completely buried in production, while still not alarming on expected closes.router/websocket/listeners.go (1)
100-100: Consider keepingWarnfor websocket send failures.Unlike close errors (which are expected during shutdown), a failure to send an event over the websocket may indicate a genuine issue — broken pipe, serialization error, or resource exhaustion. Logging this at
Infolevel risks burying actionable signals in production.Warnwould be a more appropriate middle ground.Proposed change
- h.Logger().WithField("event", evt).WithField("error", err2).Info("failed to send event over server websocket") + h.Logger().WithField("event", evt).WithField("error", err2).Warn("failed to send event over server websocket")environment/settings.go (1)
120-123: Platform guard forBlkioWeightis reasonable but may be overly conservative.
BlkioWeightis a Linux cgroup feature, so guarding it makes sense for native macOS execution. However, note that Docker Desktop on macOS runs a Linux VM, and the Docker API may acceptBlkioWeightthere. If users running Wings on macOS with Docker Desktop need IO weight support, this guard would silently skip it.Consider adding a comment explaining the rationale (e.g., Darwin doesn't support cgroup blkio) so future maintainers understand the intent.
Suggested comment
+ // BlkioWeight is only supported on Linux (cgroup blkio controller). + // On macOS, Docker Desktop uses a Linux VM but may not expose this setting. if runtime.GOOS == "linux" { resources.BlkioWeight = l.IoWeight }Makefile (1)
29-29: Addtestto.PHONYdeclaration.The static analysis tool correctly flags that
testis missing from the.PHONYlist. Sincetestis a valid filename, not declaring it as phony could cause issues if a file namedtestexists.Proposed fix
-.PHONY: all build build-darwin compress clean +.PHONY: all build build-darwin cross-build compress clean test debug rmdebugmacos.md (1)
40-47: Docker socket path may vary by Docker Desktop version.The path
~/.docker/run/docker.sockis the default for recent Docker Desktop versions, but some installations use/var/run/docker.socknatively or$HOME/.docker/desktop/docker.sock. Consider adding a brief note to verify the actual socket path (e.g.,docker context inspectorls ~/.docker/run/docker.sock) before creating the symlink.config/config.go (1)
505-515: Darwin user resolution looks correct.The approach mirrors the existing rootless-mode logic (lines 525–534). Since macOS lacks
useradd/adduser, using the current user is the right call.Note: the
user.Current()→MustInt(u.Uid)→MustInt(u.Gid)pattern is now repeated in three places (darwin, distroless, rootless). Consider extracting a small helper likesetCurrentUser()to reduce duplication, but this is not blocking.♻️ Optional: extract helper to reduce duplication
+// setCurrentUser sets the system user configuration from the current OS user. +func setCurrentUser() error { + u, err := user.Current() + if err != nil { + return err + } + _config.System.Username = u.Username + _config.System.User.Uid = system.MustInt(u.Uid) + _config.System.User.Gid = system.MustInt(u.Gid) + return nil +}Then in
EnsurePelicanUser, the darwin, distroless, and rootless blocks could callsetCurrentUser()(with the distroless block keeping its env-var override logic separately).config/config_openat_linux.go (1)
15-40: Benign race on concurrent first calls toUseOpenat2().Two goroutines can both observe
openat2Setasfalseand enter the detection path concurrently. Because the syscall result is deterministic for a given kernel, the final cached value will be correct in practice, but the detection work (including theOpenat2syscall +Close) may execute more than once.A
sync.Oncewould eliminate the redundant work and make the intent clearer. Not a correctness bug given the deterministic nature of the check, so feel free to defer.internal/ufs/fs_linux.go (1)
58-72: Nit:Secfield in theUTIME_OMITtimespec.On Line 62, both
SecandNsecare set tounix.UTIME_OMIT. Perutimensat(2), only thetv_nsecfield is inspected for theUTIME_OMITsentinel —tv_secis ignored. This works correctly but is slightly misleading. Consider settingSec: 0for clarity.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
Makefileconfig/config.goconfig/config_openat_darwin.goconfig/config_openat_linux.goconfig/config_openat_linux_test.goconfig/config_openat_test.goenvironment/settings.gointernal/ufs/file.gointernal/ufs/file_darwin.gointernal/ufs/file_linux.gointernal/ufs/fs_darwin.gointernal/ufs/fs_linux.gointernal/ufs/fs_platform_test.gointernal/ufs/fs_unix.gointernal/ufs/fs_unix_test.gointernal/ufs/walk_darwin.gointernal/ufs/walk_linux.gointernal/ufs/walk_unix.gomacos.mdrouter/router_server_ws.gorouter/websocket/listeners.goserver/filesystem/filesystem_test.goserver/filesystem/stat_darwin.goserver/filesystem/stat_test.gosystem/system.go
🧰 Additional context used
🧬 Code graph analysis (11)
server/filesystem/stat_darwin.go (1)
server/filesystem/stat.go (1)
Stat(14-17)
internal/ufs/file_linux.go (1)
internal/ufs/file_darwin.go (1)
O_LARGEFILE(4-4)
internal/ufs/fs_platform_test.go (1)
server/filesystem/stat.go (1)
Stat(14-17)
config/config_openat_darwin.go (1)
config/config_openat_linux.go (1)
UseOpenat2(15-41)
config/config.go (2)
system/system.go (1)
System(60-67)system/utils.go (1)
MustInt(35-41)
config/config_openat_linux.go (3)
config/config_openat_darwin.go (1)
UseOpenat2(5-7)config/config.go (1)
Get(439-447)system/system.go (1)
System(60-67)
config/config_openat_test.go (3)
config/config.go (3)
Set(408-420)Configuration(320-379)SystemConfiguration(138-250)config/config_openat_darwin.go (1)
UseOpenat2(5-7)config/config_openat_linux.go (1)
UseOpenat2(15-41)
server/filesystem/stat_test.go (3)
server/filesystem/filesystem_test.go (1)
NewFs(20-55)server/filesystem/stat.go (1)
Stat(14-17)server/filesystem/errors.go (1)
Error(25-37)
internal/ufs/fs_unix.go (1)
internal/ufs/file.go (2)
O_DIRECTORY(169-169)O_RDONLY(152-152)
internal/ufs/fs_linux.go (3)
internal/ufs/fs_unix.go (1)
UnixFS(23-30)internal/ufs/file.go (1)
O_CLOEXEC(172-172)internal/ufs/file_linux.go (1)
O_LARGEFILE(5-5)
internal/ufs/fs_darwin.go (1)
internal/ufs/fs_unix.go (1)
UnixFS(23-30)
🪛 checkmake (0.2.2)
Makefile
[warning] 29-29: Missing required phony target "test"
(minphony)
🪛 LanguageTool
macos.md
[style] ~160-~160: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... not exist on Darwin. - unix.Getdents does not exist on Darwin; `unix.Getdirentrie...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~174-~174: Ensure spelling is correct
Context: ...at_linux.go| OriginalCTime()usingunix.Stat_t.Ctim(was//go:build linux) | | stat_darw...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~175-~175: Ensure spelling is correct
Context: ...at_darwin.go|CTime()handling bothunix.Stat_t.Ctimandsyscall.Stat_t.Ctimespec| gola...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (go)
- GitHub Check: Build and Test (ubuntu-22.04, 1.24.7, linux, amd64)
- GitHub Check: Build and Test (ubuntu-22.04, 1.25.1, linux, amd64)
🔇 Additional comments (31)
server/filesystem/filesystem_test.go (1)
34-37: Good fix for macOS symlink resolution in test setup.This correctly handles the
/var→/private/varsymlink on macOS, preventing spuriousErrBadPathResolutionfailures in tests.router/websocket/listeners.go (1)
30-33: Log level changes for websocket lifecycle events are reasonable.These log during normal connection teardown, so
Infois appropriate.internal/ufs/file_linux.go (1)
1-5: Clean platform-specific constant definition.The Linux/Darwin split for
O_LARGEFILEis correct: Darwin universally supports large files (hence0), while Linux needs the explicit flag.internal/ufs/file.go (1)
172-173: No action needed. AT_EMPTY_PATH is not referenced anywhere in the codebase and is not defined in any platform-specific files, confirming it was safely removed and is not required for the current implementation.system/system.go (2)
115-129: LGTM — Darwin-specific OS detection looks correct.Clean branching: Darwin skips
osrelease.Read()(which would fail on macOS) and returns a sensible hardcoded value, while the Linux path retains the existing fallback chain.
209-218: LGTM — Early return avoids reading a non-existent file on macOS.server/filesystem/stat_darwin.go (1)
10-22: The comment and TODO are accurate —Ctim/Ctimespecis status-change time, not creation time.On macOS, the actual file creation (birth) time is available via
Birthtimespeconsyscall.Stat_t. Since the TODO already flags this for removal and the Linux counterpart has the same semantics, this is consistent. Just noting for awareness that if the intent ever shifts to truly returning creation time on Darwin,Birthtimespecwould be the correct field.server/filesystem/stat_test.go (2)
9-29: Test looks good but may be fragile ifCTime()returns zero time.On line 23, the test asserts
CTime()is non-zero, but the Darwin implementation falls back totime.Time{}if neitherunix.Stat_tnorsyscall.Stat_tis the underlying type. If a future Go or platform change alters theSys()return type, this test would fail with a non-obvious error. Consider a more descriptive message or loggingst.Sys()type on failure for easier debugging.That said, this is minor — the current implementations should always match one of the two branches.
31-71: LGTM — Good coverage of JSON marshaling fields.The test validates the key fields (
created,modified,name) and timestamp format. Clean and readable.macos.md (1)
1-203: Well-written documentation covering both user setup and developer context.The separation between user-facing setup instructions and developer-focused code change documentation is clear and useful. The explanations for why specific settings are needed (Docker Desktop file sharing paths,
machine_id.enable: false, etc.) are particularly helpful.internal/ufs/file_darwin.go (1)
1-4: LGTM — Correct no-op constant for Darwin.internal/ufs/fs_unix_test.go (1)
37-40: LGTM — Necessary symlink resolution for macOS test compatibility.Consistent with the same pattern applied in
server/filesystem/filesystem_test.go'sNewFs().internal/ufs/walk_darwin.go (1)
9-18: LGTM — Clean Darwin-specific dirent handling.Using
Namlendirectly is the correct approach for Darwin and avoids the NUL-scanning complexity required on Linux. Theunsafe.Sliceusage is idiomatic for this pattern.config/config_openat_test.go (1)
8-28: LGTM — reasonable cross-platform smoke test.Clean setup and the OS-specific assertions are correct: Darwin's
UseOpenat2()always returnsfalse, and Linux's result is kernel-dependent.One minor note: on Linux, if
TestUseOpenat2ConfigOverride(inconfig_openat_linux_test.go) runs first, the atomic cache (openat2Set) will already be populated, so this test won't actually exercise the"auto"detection path — it'll return the cached value. Since the stated goal is just "doesn't panic," that's fine, but worth being aware of if you ever want stronger coverage of the auto path in isolation.config/config_openat_darwin.go (1)
1-7: LGTM!Clean platform stub. The comment accurately explains why
openat2is unavailable on Darwin.config/config.go (1)
804-807: LGTM — preventsosrelease.Read()failure on macOS.
/etc/os-releasedoesn't exist on macOS, so this early return is necessary to avoid a runtime error.config/config_openat_linux_test.go (1)
5-25: LGTM — clean config override test.Properly resets the atomic cache before each assertion, ensuring the config mode is re-evaluated. Both forced
"openat"and"openat2"paths are covered.internal/ufs/fs_unix.go (3)
36-49: LGTM — symlink resolution in base path is essential for macOS.On macOS,
/varis a symlink to/private/var, so without this resolution, theunsafeIsPathInsideOfBaseprefix check would fail for any fd whose resolved path goes through/private/var. Silently ignoringEvalSymlinkserrors is reasonable here — the path may not exist yet during initialization.
682-683: Good:fdPath(fd)replaces the Linux-specific/proc/self/fdapproach.This is the correct abstraction for cross-platform fd-to-path resolution (Linux uses
/proc/self/fd, Darwin usesfcntl(F_GETPATH)).
762-762: Correct:AT_FDCWDis the right choice for opening an absolute path.Using
AT_FDCWDwith the absolutefs.basePathis semantically correct and portable across Unix platforms.internal/ufs/fs_platform_test.go (4)
15-53: LGTM — clean stat validation test.Covers the essential fields (name, size, directory flag, mod time) through the
UnixFSabstraction.
55-88: Good sandbox escape test.This exercises the critical security invariant that symlinks pointing outside the sandbox root are blocked. Using
os.Symlinkdirectly (bypassingUnixFS) correctly simulates an external actor creating the escape link.
90-130: Test depends on timing — consider a more robust assertion.The 50ms sleep + 100ms tolerance window is reasonable for CI but could be flaky on heavily loaded machines where the filesystem timestamp granularity is coarse (e.g., some filesystems have 1-second or 2-second granularity like HFS+, which is relevant for macOS).
If the test target filesystem on macOS uses APFS (nanosecond granularity), this should be fine. But if run on HFS+ volumes,
origMtimeand the post-Chtimesmtime could both round to the same second, making the test pass vacuously (not actually verifying preservation). Worth noting but not blocking.
132-145: LGTM — validates platform-specific constant.Correctly asserts
O_LARGEFILEis non-zero on Linux (fromunix.O_LARGEFILE) and zero on Darwin (where large file support is default).internal/ufs/walk_unix.go (1)
187-187: LGTM — clean delegation to platform-specificgetdentswrapper.This change properly delegates OS-specific directory entry reading to the
walk_linux.goandwalk_darwin.goimplementations, with bothgetdentsandnameFromDirentcorrectly defined in each platform file.config/config_openat_linux.go (1)
10-13: LGTM on the atomic caching variables.The two package-level atomic booleans cleanly separate the "is set" flag from the "value" flag.
internal/ufs/fs_linux.go (2)
13-14: LGTM — standard/proc/self/fdapproach for Linux.
23-54: LGTM — solidopenat2wrapper with proper flag defaults andRESOLVE_BENEATH.The
EINTR/EAGAINpass-through withoutensurePathErrorwrapping is correct, as callers will retry the operation.internal/ufs/fs_darwin.go (2)
15-27: LGTM — correct use ofF_GETPATHwithruntime.KeepAliveto prevent premature GC of the buffer.
32-34: LGTM — clean stub returningENOSYS.internal/ufs/walk_linux.go (1)
10-13: LGTM — thin wrapper overGetdents.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Split Linux-only syscalls into _linux.go/_darwin.go file pairs: - ufs: openat2 (ENOSYS stub on Darwin), fdPath via F_GETPATH fcntl, Chtimesat without UTIME_OMIT, Getdirentries-based getdents, and path-based dirent Info/Open (Getdirentries corrupts the stored dirfd) - config: UseOpenat2 probe moved to a Linux file with a Darwin stub - server/filesystem: Darwin CTime; non-Linux stub for the quotas package - system: report macOS without reading os-release - environment: never set BlkioWeight on non-Linux hosts Resolve symlinks in UnixFS base paths (/var -> /private/var on macOS) and use AT_FDCWD instead of AT_EMPTY_PATH as the safePath dirfd. Also fixes a latent bug where root-level dirents got the parent directory's name as their path; harmless on Linux (which stats via dirfd+name) but exposed by Darwin's path-based dirent methods.
CTime checked for *unix.Stat_t twice; the second branch was clearly meant to handle *syscall.Stat_t, which is what Sys() returns for FileInfos produced by (*os.File).Stat — the path Filesystem.Stat uses. As a result the file API's created timestamps were the zero time on Linux. Caught by the new TestStatCTime.
parkervcp
left a comment
parkervcp
left a comment
There was a problem hiding this comment.
Some comments about using the go:build options to files so they get ignored at compile time and don't need runtime args.
Adds //go:build darwin / //go:build linux tags to the darwin- and linux-suffixed source and test files. The filename suffixes already imply these constraints, but the explicit tags make the platform scoping clear at the top of each file, per review feedback.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/ufs/fs_unix.go (1)
684-721: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winFix file descriptor leak and clean up dead code on path validation failures.
When path validation fails, the successfully opened file descriptor is returned alongside the error. Since callers (e.g.,
safePath) treat a non-nil error as a total failure, they do not close the returned descriptor, leading to a resource leak.Additionally:
- The check
if fs.useOpenat2.Load()at line 709 is dead code. The function already returns early at line 678 ifuseOpenat2is true, and the value only transitions from true to false, meaningopwill always be"openat"at this point.- The
var finalPath stringdeclaration is redundant and can be merged into the:=assignment.Close the file descriptor before returning an error and return
0to prevent callers from accidentally using an invalid or unowned descriptor.🔒️ Proposed fix to prevent resource leaks and remove dead code
- var finalPath string - finalPath, err := fdPath(fd) + finalPath, err := fdPath(fd) if err != nil { if !errors.Is(err, ErrNotExist) { + _ = unix.Close(fd) - return fd, fmt.Errorf("failed to evaluate symlink: %w", convertErrorType(err)) + return 0, fmt.Errorf("failed to evaluate symlink: %w", convertErrorType(err)) } // The target of one of the symlinks (EvalSymlinks is recursive) // does not exist. So get the path that does not exist and use // that for further validation instead. var pErr *PathError if !errors.As(err, &pErr) { + _ = unix.Close(fd) - return fd, fmt.Errorf("failed to evaluate symlink: %w", convertErrorType(err)) + return 0, fmt.Errorf("failed to evaluate symlink: %w", convertErrorType(err)) } // Update the final path to whatever directory or path didn't exist while // recursing any symlinks. finalPath = pErr.Path // Ensure the error is wrapped correctly. err = convertErrorType(err) } // Check if the path is within our root. if !fs.unsafeIsPathInsideOfBase(finalPath) { - op := "openat" - if fs.useOpenat2.Load() { - op = "openat2" - } + _ = unix.Close(fd) - return fd, &PathError{ + return 0, &PathError{ - Op: op, + Op: "openat", Path: name, Err: ErrBadPathResolution, } } // Return the file descriptor and any potential error. + if err != nil { + _ = unix.Close(fd) + return 0, err + } - return fd, err + return fd, nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/ufs/fs_unix.go` around lines 684 - 721, Update the path-validation error returns in the function containing fdPath so they close fd, return descriptor 0, and preserve the error. Remove the redundant useOpenat2 check and always use the openat operation name there, and merge finalPath declaration with its fdPath assignment..github/workflows/push.yaml (1)
29-30: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winPrevent credential persistence in checkout action.
By default, the
actions/checkoutstep persists the GitHub token in the local.gitconfig. As a security best practice, this should be explicitly disabled to prevent potential token leakage if untrusted code executes during the build or test steps, especially on public repositories or when building/uploading artifacts.
.github/workflows/push.yaml#L29-L30: Addwith: persist-credentials: falseto the checkout action in thebuild-and-testjob..github/workflows/push.yaml#L91-L92: Addwith: persist-credentials: falseto the checkout action in thetest-macosjob.🛡️ Proposed fixes
For the
build-and-testjob (.github/workflows/push.yaml#L29-L30):- name: Code checkout uses: actions/checkout@v6 + with: + persist-credentials: falseFor the
test-macosjob (.github/workflows/push.yaml#L91-L92):- name: Code checkout uses: actions/checkout@v6 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/push.yaml around lines 29 - 30, Disable credential persistence for both checkout steps by adding the checkout action input persist-credentials: false in .github/workflows/push.yaml lines 29-30 for the build-and-test job and lines 91-92 for the test-macos job.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/push.yaml:
- Around line 29-30: Disable credential persistence for both checkout steps by
adding the checkout action input persist-credentials: false in
.github/workflows/push.yaml lines 29-30 for the build-and-test job and lines
91-92 for the test-macos job.
In `@internal/ufs/fs_unix.go`:
- Around line 684-721: Update the path-validation error returns in the function
containing fdPath so they close fd, return descriptor 0, and preserve the error.
Remove the redundant useOpenat2 check and always use the openat operation name
there, and merge finalPath declaration with its fdPath assignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71d93ebc-585f-4dbe-92ac-00272a71c901
📒 Files selected for processing (31)
.github/workflows/push.yamlMakefileconfig/config.goconfig/config_openat_darwin.goconfig/config_openat_linux.goconfig/config_openat_linux_test.goconfig/config_openat_test.goenvironment/settings.gointernal/ufs/file.gointernal/ufs/file_darwin.gointernal/ufs/file_linux.gointernal/ufs/fs_darwin.gointernal/ufs/fs_linux.gointernal/ufs/fs_platform_test.gointernal/ufs/fs_unix.gointernal/ufs/fs_unix_test.gointernal/ufs/readdirmap_test.gointernal/ufs/walk_darwin.gointernal/ufs/walk_dirent_darwin.gointernal/ufs/walk_dirent_linux.gointernal/ufs/walk_linux.gointernal/ufs/walk_unix.gomacos.mdrouter/router_server_ws.gorouter/websocket/listeners.goserver/filesystem/filesystem_test.goserver/filesystem/quotas/functions_other.goserver/filesystem/stat_darwin.goserver/filesystem/stat_linux.goserver/filesystem/stat_test.gosystem/system.go
🚧 Files skipped from review as they are similar to previous changes (22)
- config/config_openat_test.go
- internal/ufs/walk_dirent_linux.go
- internal/ufs/file_linux.go
- config/config_openat_linux_test.go
- internal/ufs/file_darwin.go
- internal/ufs/walk_dirent_darwin.go
- internal/ufs/fs_linux.go
- server/filesystem/stat_darwin.go
- internal/ufs/walk_darwin.go
- router/router_server_ws.go
- config/config_openat_linux.go
- server/filesystem/stat_test.go
- Makefile
- internal/ufs/file.go
- internal/ufs/walk_linux.go
- system/system.go
- internal/ufs/fs_darwin.go
- internal/ufs/fs_platform_test.go
- internal/ufs/fs_unix_test.go
- router/websocket/listeners.go
- config/config.go
- internal/ufs/walk_unix.go
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Build and Test (ubuntu-22.04, 1.26.4, linux, amd64)
- GitHub Check: Test macOS (1.25.11)
- GitHub Check: Test macOS (1.26.4)
- GitHub Check: Build and Test (ubuntu-22.04, 1.25.11, linux, amd64)
- GitHub Check: Analyze (go)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-02T13:53:08.995Z
Learnt from: parkervcp
Repo: pelican-dev/wings PR: 171
File: server/power.go:190-203
Timestamp: 2026-03-02T13:53:08.995Z
Learning: In the server package, when quotas are enabled via config.Get().System.Quotas.Enabled, the disk space check using used >= s.DiskSpace() does not require a special guard for unlimited-disk scenarios (DiskSpace() <= 0). The filesystem handles such cases, so the existing check is sufficient. Apply this pattern to similar quota-related disk checks in the server package and ensure tests/docs reflect that unlimited-disk behavior is governed by the filesystem, not by an extra guard in code.
Applied to files:
server/filesystem/quotas/functions_other.goserver/filesystem/stat_linux.goserver/filesystem/filesystem_test.go
🪛 ast-grep (0.44.1)
internal/ufs/fs_unix.go
[warning] 660-660: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(syscallMode(mode))
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🪛 zizmor (1.26.1)
.github/workflows/push.yaml
[warning] 29-30: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 91-92: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🔇 Additional comments (7)
internal/ufs/readdirmap_test.go (1)
1-256: LGTM!server/filesystem/filesystem_test.go (1)
34-37: LGTM!config/config_openat_darwin.go (1)
1-9: LGTM!environment/settings.go (1)
145-168: LGTM!server/filesystem/quotas/functions_other.go (1)
1-36: LGTM!server/filesystem/stat_linux.go (1)
17-27: LGTM!macos.md (1)
1-137: LGTM!
When the post-open path validation in UnixFS.openat failed, the successfully opened descriptor was returned alongside the error. Callers treat a non-nil error as a total failure and never close the returned fd, leaking the descriptor. Close the descriptor and return 0 on every validation-error path, including the non-existent-symlink-target case that previously returned the fd with a non-nil error. Also drop the dead openat2 branch when building the PathError op: the openat2 path returns early before this check, so the op is always "openat".
actions/checkout persists the GitHub token in the local .git config by default. Since both jobs build and upload artifacts, set persist-credentials: false on each checkout to avoid leaking the token into the workspace.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/ufs/fs_unix.go (1)
703-724: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear the artifact error to prevent failures on benign broken symlinks.
The
if err != nilblock at line 721 will unintentionally fail the open operation and close the descriptor if the file is a benign broken symlink.Because
_openatincludesO_NOFOLLOW(line 649), it successfully opens broken symlinks and returns a validfd. TheErrNotExisthere is merely an artifact offilepath.EvalSymlinks(used within Linux'sfdPath) aggressively attempting to resolve the final target.Once we extract the target path to verify it hasn't escaped the sandbox (line 710), we should clear the error so the function successfully returns the valid file descriptor. This ensures the fallback path behaves identically to the
openat2(RESOLVE_BENEATH)fast-path, which correctly permits opening broken symlinks.💡 Proposed fix to retain broken symlink support
- // Ensure the error is wrapped correctly. - err = convertErrorType(err) + // Clear the artifact error so we return the valid descriptor for the broken symlink, + // consistent with openat2 behavior. + err = nil } // Check if the path is within our root. We always reach this point using // openat (the openat2 path returns early above), so the op is always // "openat". if !fs.unsafeIsPathInsideOfBase(finalPath) { _ = unix.Close(fd) return 0, &PathError{ Op: "openat", Path: name, Err: ErrBadPathResolution, } } - // If path validation surfaced an error (e.g. a non-existent symlink - // target), close the descriptor so callers don't leak it. - if err != nil { - _ = unix.Close(fd) - return 0, err - } - // Return the validated file descriptor. return fd, nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/ufs/fs_unix.go` around lines 703 - 724, Clear the artifact error after validating the extracted target path in the openat fallback, before the final if err != nil check. In the surrounding path-resolution flow, preserve genuine path-boundary failures while discarding the benign EvalSymlinks ErrNotExist produced for a successfully opened broken symlink, allowing the valid descriptor to be returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/ufs/fs_unix.go`:
- Around line 703-724: Clear the artifact error after validating the extracted
target path in the openat fallback, before the final if err != nil check. In the
surrounding path-resolution flow, preserve genuine path-boundary failures while
discarding the benign EvalSymlinks ErrNotExist produced for a successfully
opened broken symlink, allowing the valid descriptor to be returned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 507369d6-bf4c-482b-9d5c-2bf8eb347a84
📒 Files selected for processing (2)
.github/workflows/push.yamlinternal/ufs/fs_unix.go
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/push.yaml
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: Analyze (go)
- GitHub Check: Build and Test (ubuntu-22.04, 1.25.11, linux, arm64)
- GitHub Check: Build and Test (ubuntu-22.04, 1.26.4, linux, amd64)
- GitHub Check: Test macOS (1.25.11)
- GitHub Check: Build and Test (ubuntu-22.04, 1.25.11, linux, amd64)
- GitHub Check: Test macOS (1.26.4)
|
Re: the "clear the artifact error to prevent failures on benign broken symlinks" comment ( I looked into this and I don't think it's a real issue, so I'm leaving the code as-is. The suggestion assumes On top of that, the old code already returned a non-nil Setting |
2 participants
Makes Wings compile and run natively on macOS, so the daemon can be developed and tested on a Mac against Docker Desktop (containers still run in Docker Desktop's Linux VM; Wings itself runs on the host).
Approach
Linux-only syscalls are split into
_linux.go/_darwin.gofile pairs using Go's filename-based build constraints, with a handful ofruntime.GOOSchecks where a file split would be overkill:internal/ufs—openat2is Linux 5.6+ only, so Darwin gets anENOSYSstub and theopenatfallback path validation.fdPath()uses theF_GETPATHfcntl instead of/proc/self/fd/.Chtimesat()emulatesUTIME_OMIT(not available on Darwin) by reading and preserving current timestamps. Directory listing usesGetdirentriesinstead ofGetdents, and Darwin dirents resolveInfo()/Open()by path becauseGetdirentriesis a userspace emulation (fdopendir/readdir_r) that manipulates the directory fd's seek offset, leaving it unusable for subsequent*atcalls (EBADF).config— theUseOpenat2()runtime probe moved to a Linux-only file; Darwin always returns false.EnsurePelicanUser()uses the current user on macOS (nouseradd), matching rootless-mode semantics.server/filesystem— DarwinCTime(); a non-Linux stub for the newquotaspackage (returns explicit "unsupported" errors).system/environment— report "macOS" without readingos-release; never setBlkioWeighton non-Linux hosts (Docker Desktop rejects it, and the cgroup-v2 probe path inblkioWeightSupported()would incorrectly return true on macOS)./var→/private/var, andsafePathopens the base withAT_FDCWDrather than passingAT_EMPTY_PATHas a dirfd.Latent bug fixes (affect shared code)
Zero
createdtimestamps on Linux —stat_linux.go'sCTime()checked for*unix.Stat_ttwice; the second branch was clearly meant to handle*syscall.Stat_t, which is whatSys()returns for FileInfos produced by(*os.File).Stat— the pathFilesystem.Statuses. The file API'screatedfield has therefore been the zero time on Linux. Caught by the newTestStatCTime.Wrong dirent paths at walk roots —
internal/ufs/walk_unix.goreadDir()setrel = name(the parent directory's name) instead ofrel = childNamefor root-level entries. This was invisible on Linux because Linux dirents stat/open via dirfd+name and never readpath, but Darwin's path-basedInfo()/Open()exposed it. Fixed here with a regression test (TestReadDirRootEntryPaths).Tests
internal/ufs: platform tests for fd path resolution, openat path validation (symlink escape),Chtimesatzero-time preservation, andO_LARGEFILE; portable ReadDirMap tests (Info/Open, 100-goroutine concurrency, symlinked base) that now run on Linux CI too.config:UseOpenat2mode-override and per-platform behavior tests.server/filesystem:CTime()andStatJSON marshaling tests.macos-15) runninggo test -race ./...so Darwin support doesn't silently rot; the Linux matrix is unchanged.Also reduces log levels for expected websocket disconnect/cleanup paths (Error/Warning → Info, keeping the error as a log field), adds
build-darwin/fixedcross-buildMakefile targets, and documents macOS setup inmacos.md.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation