Latest Changes from Bushel v3.0.0-alpha.2

[leogdion]

No description provided.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6f5b2840-e0c2-469b-8024-f450d2a6680a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch subrepo

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

Review Summary

This PR ports recent BushelKit changes from the v3.0.0-alpha.2 line: a new BushelHarvestCore module (host/guest command-and-control protocol types + wire framing), a platform-target rename/change, a BushelLogging import tweak, and a batch of formatting/test cleanups. Overall the new protocol types are clean, Sendable/Codable, well-documented, and the new framing code shows real security awareness (bounded message size, socket timeouts). A few things below should be addressed before merge, most importantly #1.

🔴 Must fix

1. New BushelHarvestCoreTests target isn't registered — the new tests never run
Tests/BushelHarvestCoreTests/*.swift (6 files, ~230 tests) are added, but there's no corresponding target definition. Every existing test suite has a matching file in Package/Sources/Tests/ (e.g. BushelFoundationTests.swift, BushelUtilitiesTests.swift) that's referenced in the testTargets: { … } block in Package/Sources/Index.swift. This PR adds no Package/Sources/Tests/BushelHarvestCoreTests.swift and doesn't touch the testTargets block (confirmed via diff and current Index.swift). Since Package.swift is generated from Package/Sources/, swift test will not compile or execute any of the new BushelHarvestCoreTests — they're dead code from CI's perspective. Please add the target definition and register it, then re-run ./Scripts/package.sh . to regenerate Package.swift.

2. Package-wide minimum macOS version silently dropped from 15 → 12
Package/Sources/Platforms/MinimumPlatforms.swift (renamed from WWDC2023.swift) changes SupportedPlatform.macOS(.v15) to .v12. This is applied via .supportedPlatforms { MinimumPlatforms() } at the package level, so it lowers the deployment target for every target, not just the new dependency-free BushelHarvestCore. This directly contradicts the documented requirement in CLAUDE.md ("macOS: … macOS 15+ deployment target") and could let macOS-15-only APIs elsewhere in the tree silently become unreachable/misguarded on older OS versions (or force new #available checks throughout). If the intent is only to let HarvestBinKit/BushelHarvestCore run on older guest OSes, that should be scoped to that target specifically (if the builder DSL supports per-target platform overrides) rather than lowering it package-wide. At minimum, please confirm this is intentional and update CLAUDE.md to match.

🟡 Worth addressing

3. Zero test coverage for HarvestMessageFraming.swift
This is the most risk-bearing new file — it parses an attacker/peer-controlled 4-byte length prefix, enforces maxMessageLength, and configures socket timeouts. The rest of BushelHarvestCore (plain data models) is well covered, but there are no tests for framing/oversized-message rejection, partial-read looping, HarvestFramingError cases, or HarvestConfiguration constants. Given the explicit security rationale in the doc comments (preventing multi-GB allocations from a hostile peer), a test asserting messageTooLarge is thrown for an oversized prefix would be valuable.

4. Loggable.swift: internal import → @_exported import on non-Darwin

-  internal import FelinePineSwift
+  @_exported import FelinePineSwift

This changes FelinePineSwift from a module-private import to a re-exported one on non-Darwin platforms, which changes BushelLogging's public API surface (consumers now transitively get FelinePineSwift's symbols) with no explanation in the PR. If this is fixing a real compile error (e.g., a public API needing the type to be visible to callers), consider a short comment explaining why, so it isn't accidentally reverted later.

5. URLSessionTests.swift — indentation is broken and coverage narrowed
The whole file was wrapped in #if !os(Android) (good instinct — mirrors FileManagerTests.swift), but the result mixes 2/4/6-space indentation inconsistently and looks like it wasn't run through swift-format. Please run ./Scripts/lint.sh (or FORMAT_ONLY=1 ./Scripts/lint.sh) on this file. Separately, all of the "happy path" tests (testFetchLastModifiedWithValidURL, testFetchDataWithValidURL, testFetchDataWithoutLastModifiedTracking, testFetchLastModifiedReturnsNilGracefully) were removed, leaving only error-path tests. Dropping flaky live-network tests is reasonable, but consider replacing them with a mocked URLProtocol/URLSession so the success path still has coverage instead of just deleting it outright.

🟢 Minor / nits

• Sources/BushelUtilities/Extensions/Date+RFC2822.swift: the new fallback chain adds a force-unwrap for a case that's already unreachable via the original ?? .gmt:

  timeZone: TimeZone(identifier: "GMT") ?? TimeZone(secondsFromGMT: 0) ?? TimeZone(identifier: "UTC")!

  If this is a Linux-portability fix (.gmt may not exist in swift-corelibs-foundation), a single documented fallback would be simpler and avoid the !.
• Several new BushelHarvestCore source/test files are missing a trailing newline (HarvestCommand.swift, CommandProtocolTests.swift, HarvestAuthTokenTests.swift, HarvestCommandTests.swift, HarvestEventTests.swift, HarvestRequestTests.swift, HarvestResponseTests.swift) — swift-format/lint will likely flag these.
• HarvestCommand.name collapses the .file/.network/.clipboard associated String payload into a generic "file.operation" / "network.operation" / "clipboard.operation" string, discarding the actual operation description — reduces usefulness for logging/debugging.
• Time-based ordering assertions (HarvestEventTests.eventSequencing, HarvestRequestTests.timestampOrdering) rely on Thread.sleep(forTimeInterval: 0.01) plus < comparisons on Date — these can be flaky under CI load/clock coarsening; consider injecting a clock or comparing with >=.
• HarvestResponseTests.jsonStructureWithSuccessFalse asserts on the exact pretty-printed JSON spacing ("\"success\" : false"), which is somewhat brittle across Foundation implementations (Darwin vs swift-corelibs-foundation on Linux) given this package explicitly supports Linux.

Nice touches worth calling out

• HarvestConfiguration.maxMessageLength / ioTimeoutSeconds with clear doc comments explaining the DoS rationale — good defensive design.
• The FileManager.swift extension reformat looks like a legitimate indentation fix (previous extension body wasn't nested correctly under extension FileManager { … }).
• Good Sendable/Codable hygiene throughout the new BushelHarvestCore types.

---

Automated review via Claude Code.

[claude]

Review: BushelHarvestCore module + supporting changes

Reviewed the diff (base v3.0.0-alpha.4, mostly the new BushelHarvestCore target plus formatting/CI fixes carried over from prior commits). This PR already went through one round of self-review (the d2843cc commit registers the test target, hardens framing, adds EINTR retry, etc.), and it shows — the code is in solid shape. Notes below.

Strengths

• HarvestMessageFraming is well hardened for a length-prefixed socket protocol: it checks the length prefix against HarvestConfiguration.maxMessageLength before allocating the body buffer (prevents a hostile/malformed peer from forcing a multi-GB allocation), retries on EINTR instead of failing, and applies SO_RCVTIMEO/SO_SNDTIMEO so a stalled peer can't hang I/O indefinitely.
• Good test coverage of the tricky edges: socketpair round-trip, zero-length body, oversize body, EOF before the length prefix, and truncated body — these are exactly the cases that tend to be skipped in framing code.
• HarvestConfiguration's doc comment clearly documents that the channel is unencrypted/unauthenticated and is only safe over the host↔guest VirtioSocket — good, since privileged commands (SSH enable, shutdown/restart) travel over it.
• HarvestAuthToken.isExpired(asOf:) and HarvestFramingError: Equatable are nice additions for testability.
• HarvestCommand.name (used presumably for logging) correctly omits the raw token from .security(.authenticate) rather than interpolating it — avoids an easy credential-leak-via-logs mistake.
• Registering BushelHarvestCoreTests as an actual test target is a real fix — per the commit message, ~900 lines of tests existed but were never compiled/run before this PR.

Findings

1. Minor regression: unnecessary force-unwrap in Date+RFC2822.swift.

   timeZone: TimeZone(identifier: "GMT") ?? TimeZone(secondsFromGMT: 0) ?? TimeZone(identifier: "UTC")!

   The previous code (TimeZone(secondsFromGMT: 0) ?? .gmt) had a guaranteed-safe, non-optional fallback (TimeZone.gmt). The new chain ends in a force-unwrap, which is strictly worse from a safety standpoint even though "UTC" will never actually fail to resolve — it reintroduces a crash risk that didn't exist before, in a file whose only job is defensive fallbacks. Consider TimeZone(identifier: "GMT") ?? .gmt instead — same effect, no !.

2. Global deployment target lowered from macOS 15 → macOS 12 for the whole package, not just the new target. Package/Sources/Platforms/WWDC2023.swift → MinimumPlatforms.swift changes SupportedPlatform.macOS(.v15) to .v12. This PlatformSet is applied once, globally, to Package(...).supportedPlatforms { ... } — there's no per-target override, so this floor now applies to every product in the package, including macOS-Virtualization-framework-dependent modules (BushelMacOSCore, BushelMachine, etc.), not just the new BushelHarvestCore (whose stated goal is guest-side compatibility on older macOS). If that's intentional and verified (the last commit message says the full package builds and tests pass on the lowered floor), it's fine, but it's a fairly consequential platform-support change to bundle silently into a feature PR — worth calling out explicitly in the PR description, and worth double-checking whether a per-target platform override would be safer/more precise than a global floor drop.

Minor nits

• Package/Sources/Tests/BushelHarvestCoreTests.swift uses curly smart quotes ("Software") in its license header, while every other new file in this PR uses straight quotes. The last commit's message claims license headers were normalized — this one file seems to have been missed.
• HarvestCommand.metadata is declared public var while every other stored property on that struct is let. Minor inconsistency in an otherwise fully-immutable value type — probably intentional (post-hoc annotation?) but worth a comment if so.

Test coverage

Good — the new BushelHarvestCoreTests target (once actually wired up, per the last commit) covers commands, requests/responses, auth token expiry, framing edge cases, and configuration. This is a meaningful improvement over the prior state where these tests existed but weren't compiled.

Overall

This is a well-scoped, carefully hardened addition for a new host↔guest control-channel module, and the self-review commit already addressed the substantive security gaps (unbounded allocation, EINTR handling, timeouts). Only real ask is confirming the global macOS 12 floor drop is intentional and doesn't silently break other Virtualization-framework-dependent targets; the force-unwrap in Date+RFC2822.swift is a trivial one-line fix.

🤖 Automated review generated with Claude Code