feat: add Windows support

[GingerAdonis]

Summary

• Add internal/filelock package abstracting flock(2) (Unix) and LockFileEx (Windows) behind a cross-platform Lock/TryLock/Unlock API
• Replace all golang.org/x/sys/unix flock usage in serve.go, daemonctl, sockpath, and tlsconfig with the new package
• Split Unix-only process management (Setpgid/Setsid/Kill(-pid)) into _unix.go/_windows.go build-tag pairs for childproc and daemon/spawn
• Add GOOS=windows GOARCH=amd64 go build ./... to the go-checks CI job so Windows regressions are caught on every PR

Test Plan

• All 15 packages pass: go test ./... -count=1
• Windows cross-compilation succeeds: GOOS=windows GOARCH=amd64 go build ./...
• CI go-checks job passes (includes new Windows build step)
• Release job builds hostmux-windows-amd64.exe and hostmux-windows-arm64.exe without errors

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[opencode-agent]

This PR adds Windows support through clean abstractions. The implementation is well-structured and follows Go conventions for platform-specific code.

Positive aspects:

• Clean internal/filelock abstraction with correct semantics (held=true means lock is held by another process)
• Appropriate use of build tags (//go:build !windows / //go:build windows)
• Platform limitations documented incomments (e.g., Windows process group handling)
• CI updated to verify Windows cross-compilation
• All tests pass; go vet clean

Minor observations:

1. Windows process termination semantics: The killProcess function in daemonctl_windows.go uses os.FindProcess().Kill() for both graceful and forceful termination. On Windows, there's no SIGTERM equivalent—Kill() terminates immediately. The socket-based graceful shutdown path in Stop() runs before this fallback, which mitigates the issue, but the graceful parameter is effectively ignored. Consider documenting this in the function comment.

2. Windows signal forwarding: In childproc_windows.go, CREATE_NEW_PROCESS_GROUP creates a new process group, but when forwarding os.Interrupt, only the direct child process is killed (not its subprocess tree). On Unix, Kill(-pid, sig) terminates the entire process group. This is a platform limitation worth noting in the doc comment.

3. Test coverage gap: The TestStopFallbackToSIGTERM test was moved to daemonctl_unix_test.go, meaning Windows lacks equivalent test coverage. While this requires a Windows runner, the current cross-compilation CI step only catches syntax errors, not runtime behavior. Consider adding placeholder tests or documenting the gap.

No blocking issues found. The implementation correctly handles cross-platform file locking and process management within the constraints of each OS.

opencode session  |  github run

[claude]

Review

The overall approach is clean — splitting platform-specific code into _unix.go/_windows.go pairs and abstracting flock/LockFileEx behind a thin filelock package is the right design. A few issues worth addressing before merge:

---

Bug: serve.go — SIGTERM never delivered on Windows

serve.go:236 uses signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM). On Windows, signal.Notify only delivers os.Interrupt (Ctrl+C). SIGTERM is silently ignored by the Go runtime on Windows — the daemon will never receive a graceful shutdown signal from the kill fallback path. Either:

• Split serve.go into serve_unix.go / serve_windows.go and use os.Interrupt for Windows, or
• Add os.Interrupt alongside the existing signals: signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM, os.Interrupt)

This is the only behavioral regression introduced for Windows runtime.

---

API confusion: TryLock return value is inverted from convention

// TryLock returns held=true when another process holds the lock
held, err := filelock.TryLock(f)

held=true means "we did NOT acquire it (someone else has it)". Most mutex-style APIs return true for success. The current semantics mean every caller has to mentally invert the value. Consider renaming to alreadyHeld bool or restructure the return so true means "acquired". The existing callers in daemonctl.go and sockpath.go correctly use it but the inverted convention will be a trap for future callers.

---

Test gap: TryLock contention not tested

TestTryLockAcquireRelease only tests TryLock on an unheld file (held=false path). The critical held=true path — where another process actually holds the lock — is untested. This can be done portably by opening the same file twice and acquiring the lock on the first handle before calling TryLock on the second:

func TestTryLockConflict(t *testing.T) {
    f1, _ := os.CreateTemp(t.TempDir(), "flock*")
    defer f1.Close()
    f2, _ := os.Open(f1.Name())
    defer f2.Close()

    if err := filelock.Lock(f1); err != nil {
        t.Fatalf("Lock f1: %v", err)
    }
    held, err := filelock.TryLock(f2)
    if err != nil {
        t.Fatalf("TryLock f2: %v", err)
    }
    if !held {
        t.Fatal("expected held=true when f1 holds the lock")
    }
}

Without this, the lock contention behavior (the reason the package exists) has no test coverage.

---

Minor: spawn_windows.go — CREATE_NEW_PROCESS_GROUP interaction with DETACHED_PROCESS

detachedProcess | syscall.CREATE_NEW_PROCESS_GROUP is fine for detaching the daemon, but note that DETACHED_PROCESS already implies a new console, and combining it with CREATE_NEW_PROCESS_GROUP means Ctrl+C events won't reach the daemon even if it's in the same session. This is the desired behavior for a background daemon, but worth a brief comment explaining why both flags are used together to aid future maintainers.

---

Nit: daemonctl_unix.go — unix.Signal(unix.SIGKILL) cast is redundant

sig := unix.Signal(unix.SIGKILL)
if graceful {
    sig = unix.SIGTERM
}

unix.SIGKILL is already of type unix.Signal. The explicit cast on the first line is noise.

[GingerAdonis]

Fixed in 13d67f3:

serve.go SIGTERM bug — Fixed. Added os.Interrupt to the signal.Notify call so Windows gets graceful shutdown on Ctrl+C. No need to split into separate files; the extra signal is a no-op on Unix.

TryLock naming — Pushing back on this one. held=true means "another process holds the lock" — the lock was not acquired. This matches the mental model for advisory file locking: you're asking "is the file already held?" not "did I acquire it?". Callers use it in if held { /* contention */ } blocks which reads naturally. Renaming to acquired bool (inverted meaning) would require flipping every if held to if !acquired, which is harder to read at the call site. The API doc comment on TryLock is explicit about the semantics, so future callers have a clear contract.

TestTryLockConflict — Added. Note: used os.OpenFile(path, os.O_RDWR, 0o644) instead of os.Open for the second descriptor — os.Open opens read-only, which may not support LockFileEx on Windows. Test covers the full contention/release cycle.

spawn_windows.go comment — Added explaining why both DETACHED_PROCESS and CREATE_NEW_PROCESS_GROUP are combined.

unix.Signal() cast — Removed.

[opencode-agent]

Review Summary

All prior review feedback has been properly addressed in commit 13d67f3. The implementation is clean and well-structured.

Fixes Verified:

• ✅ serve.go:236: os.Interrupt added tosignal.Notify for Windows graceful shutdown
• ✅ daemonctl_unix.go: Redundant unix.Signal() cast removed
• ✅ spawn_windows.go: Comment added explaining DETACHED_PROCESS and CREATE_NEW_PROCESS_GROUP
• ✅ TestTryLockConflict: Test addedfor lock contention case

Verification:

• All15 packages pass tests
• go vet clean
• Windows cross-compilation succeeds
• Code formatting correct (go fmt)

Minor observation (not blocking):
On Unix, os.Interrupt == syscall.SIGINT, so signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM, os.Interrupt) registers SIGINT twice. This is harmless—buffered channel (size 1), single select read, daemon exits after first signal.

No blocking issues. Ready to merge.

opencode session  |  github run

[claude]

All issues from the previous review round are resolved. A couple of minor observations on the latest state:

serve.go signal fix is a no-op
os.Interrupt == syscall.SIGINT on every platform, so adding os.Interrupt alongside syscall.SIGINT in the same signal.Notify call is idempotent — signal.Notify deduplicates by signal number via a bitset. The daemon already responded to Ctrl+C on Windows through the pre-existing syscall.SIGINT. The change is harmless but doesn't address the original concern (that SIGTERM is silently dropped on Windows). In practice this doesn't matter: on Windows the only path that signals the daemon is the Ctrl+C console event, and that was already covered. Worth a brief note in the commit message or comment so future readers aren't confused.

TestTryLockConflict looks correct
Using O_RDWR for the second descriptor is the right call — flock on Linux is per open file description, so two separate open() calls produce independent lock state. The held→release→reacquire cycle is well-tested.

No new bugs or regressions found. The platform-split approach is clean, the filelock abstraction is correct, and CI now catches Windows cross-compilation regressions. Ready to merge.

[GingerAdonis]

Acknowledged on the os.Interrupt note — you're right that os.Interrupt == syscall.SIGINT at the signal number level, so the addition is idempotent. The original SIGTERM concern is real (it's dropped on Windows) but doesn't bite us in practice: the clean shutdown path is the OpShutdown socket message, not signals. Signals are a last-resort fallback that collapses to TerminateProcess on Windows anyway via daemonctl_windows.go. Leaving the os.Interrupt addition in as self-documenting intent; it's harmless and makes the Windows behavior explicit at the call site.