3.0.1 — RouteManager refactor, review hardening, and 3.0 docs - #55
Merged
Conversation
The public-repo infra-leak scrub (3.0.0) redacted real tailnet/proxy IPs to placeholders like "<tailnet-peer-ip>" — but some lived in TEST FIXTURES (ProxyListenerManagerTests asserting isTailnetHost / GlobalProtect-shadow classification), which then fed non-IP strings and failed 4 tests. The app was unaffected (it uses the CIDR ranges, not host literals), and CI never caught it (the mac runner was down). Restore the fixtures with SYNTHETIC IPs that preserve each assertion's CIDR intent (100.100.x tailnet, 100.120.x in the 100.112/12 GP-shadow, 203.0.113.x non-tailnet) without re-introducing real infra. Also fix the LiveProxyEgress env fallbacks and a redacted comment in RouteManager.
The "reconcile listeners + conditionally reapply routes" tail was copied across 7 locations (~12 mutation entry points) in RoutesTab, RulesTab, MenuBarViews and ControlSurface, diverging on three axes (standalone-reconcile, reapply-condition, async-context). Centralize into one @mainactor helper `reconcileAfterConfigChange(reconcileListeners:reapplyRoutes:sendNotification:)` on RouteManager (Option B — the helper does NOT persist; each site keeps its own saveConfig, so there is zero timing change). Every per-site policy is now an explicit argument, so no site changes behavior: RoutesTab reapplies only under usesCustomEngine, the RulesTab family always, addRoute never, and ControlSurface on `mode` OR custom — and its socket response still returns only after routes apply (inline await, no Task). setRoutingMode's heavier path is untouched. Strictly behavior-preserving; 710 tests green.
…outes (US-001) The install-epilogue (build activeRoutes from the ownership entries minus batch failures -> two-population orphan cleanup -> epoch-guard -> commit activeRoutes/ lastUpdate -> hosts file) was duplicated ~50 lines each in applyAllRoutesInternal and applyRoutesFromCache. Extract it into one private @mainactor helper `commitAppliedRoutes(routesToAdd:allSourceEntries:batchFailedDests:epoch:logLabel:) -> Bool`; both paths now call it. (The architect map corrected the premise: only the two full-replace paths share this tail — the backgroundDNSRefresh/ performDNSRefresh incremental-diff family is a genuinely different shape and is left alone; the custom twin applyCustomRoutesInternal is a separate follow-up.) STRICTLY behavior-preserving, verified by byte-diff (the suite does not drive the apply-tail, so green tests are necessary-not-sufficient): the kernel batch (routesToAdd) is byte-identical; allSourceEntries gains a `gateway` field that embeds, per append site, the exact gateway its paired routesToAdd entry uses (catch-alls/service-ranges -> local gateway, inverse CIDRs/domain IPs -> routeGateway), so the in-memory ActiveRoute.gateway is recovered identically. The epoch-guard->commit window stays await-free; segment A (helper batch add, with its divergent empty-set/helper-absent handling) and segment G (notify/verify) stay in each caller's head. Net -27 lines; 710 tests green.
Complete the apply-epilogue dedup: the custom engine's install-tail (segment B-F) was the byte-identical third copy. Fold it into the shared commitAppliedRoutes (logLabel "Custom "). allSourceEntries 2-tuple -> 3-tuple, embedding the compiler's per-rule gateway (proxy/iface:utunX/local token verbatim). Provably value-identical: RouteCompiler.compile claims each destination at most once (CIDR containment; exact-equality is containment), so the old routesToAdd.first(where:)?.gateway always returned that entry's own gateway and the ?? fallback was dead. routesToAdd (the kernel batch) is byte-identical; only the in-memory allSourceEntries gained a gateway field. Three apply-tail copies -> one helper. -44 lines; 710 tests green incl. 42 test-covered custom-engine tests.
… coverage (US-003) Adds 63 tests (710 -> 773), 0 failures. Trap 7 (the suite didn't drive the apply-tail): extract the pure two-population stale-route partition out of commitAppliedRoutes into a testable static `partitionStaleRoutes(active:applied:attempted:)` (behavior-preserving — the set math is only relocated, proven executably against the verbatim old formula) and cover it exhaustively (StaleRoutePartitionTests, 11 tests incl. the disjoint / union==stale invariants and the old-inline-math equivalence). Broaden edge-case coverage of the pure seams (52 tests): RouteCompiler CIDR-containment + GP catch-all guard, RuleResolver first-match asymmetry, Config.derive + preparedForCustomMode migration, CredentialTemplate single-pass cross-contamination guard, IfconfigParser flag/inet parsing, ifaceGateway multi-VPN matching, RuleDestinationBuilder/HookGenerator, and ProxyForwarder authority parsing (incl. the documented bracketed-IPv6 behavior).
…lity) Whole-codebase review hygiene, all behavior-preserving (classic Bypass / VPN Only route application unchanged; verified byte-identical + 773 tests green): - Log file moved off world-readable /tmp to ~/Library/Logs/VPNBypass/ (0700 dir, O_NOFOLLOW + 0600), with a cached formatter and a persistent append handle instead of per-line formatter-alloc + open/seek/close. - HelperManager and NotificationManager log via the shared logger instead of print(), so helper install/update/XPC and notification failures reach the log file and the in-app Logs tab. - saveDNSCache/loadDNSCache surface encode/write/corruption errors (and the cache is written 0600) instead of swallowing them. - Helper version probe distinguishes "timed out" (slow) from "unreachable" (broken) and retries once before reinstalling, so a busy helper no longer triggers a spurious admin-password prompt. - DNS resolver losers are killed on cancellation; the flap-recheck sleep honors cancellation; the modern-path cdhash pin uses the same uniform shell/AppleScript escaping as the legacy installer. - Removed verified-dead code: interfaceTypeName, applyRouteForRange, updateNetworkStatus(NWPath), detectAndApplyRoutes(), showOnTop(), clearHostsFile(), HelperProgressProtocol, the vestigial hasPromptedKey, write-only hasCompletedInitialStartup / lastSuccessfulVPNCheck, and an unused import.
- ThemeTests: replace ~40 no-assertion `_ = Theme.x` bodies with real inequality/non-empty assertions (comparing translucent members against opaque anchors where opacity siblings collide). - RouteManagerIntegrationTests: testDetectedDNSServerDisplayDefaultNil now asserts the default is nil instead of discarding it. - StaleRoutePartitionTests: de-tautologize the two tests that re-derived the implementation formula; assert hand-computed expected sets so a formula regression is actually caught.
…oadmap - README: document the three routing modes (Bypass / VPN Only / Custom), custom routes + rules, the multiple egresses (multi-VPN, HTTP/SOCKS5 proxy, Tailscale-peer), the vpnb CLI, the seven Settings tabs, and the cdhash-pinned, NE-free privileged helper. - CHANGELOG: add the missing 3.0.0 and 3.0.1 entries. - ROADMAP / MULTI-ROUTE-DESIGN: mark v3.0 shipped, the vpnb CLI done, and correct the routing engine to the entitlement-free path (no Network Extension) instead of NETransparentProxy. - Helper/Info.plist: version 1.5.0 -> 1.6.0 to match HelperConstants. - Record the whole-codebase review and its deferred hardening backlog in docs/CODE-REVIEW-3.0.1.md.
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR centralizes config-change reconciliation, refactors route application and helper handling, hardens logging and DNS cache persistence, adds edge-case tests, and refreshes release docs plus helper version metadata. ChangesRouteManager, helper, and app flow
Tests
CI
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/VPNBypassCore/HelperManager.swift (1)
442-466: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winSilent no-op if
NSAppleScript(source:)init fails.Unlike
installHelperLegacy(which setsinstallationErrorand logs when script creation fails, lines 415-416),ensureCDHashPinned()'sNSAppleScript(source: script)?.executeAndReturnError(&error)swallows a nil-init failure with no log at all — the cdhash pin write would silently never happen and the helper falls back to identifier-only auth with zero trace.As per coding guidelines: "Handle errors properly in Swift code — don't ignore errors, handle them appropriately."
🛡️ Proposed fix
let script = "do shell script \"\(Self.appleScriptStringEscaped(shellCommand))\" with administrator privileges" var error: NSDictionary? - NSAppleScript(source: script)?.executeAndReturnError(&error) + guard let appleScript = NSAppleScript(source: script) else { + RouteManager.shared.log(.warning, "cdhash pin write (modern path): failed to create AppleScript — helper will use identifier-only") + return + } + appleScript.executeAndReturnError(&error) if let error = error {🤖 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 `@Sources/VPNBypassCore/HelperManager.swift` around lines 442 - 466, The cdhash pin write path in ensureCDHashPinned() silently ignores a nil NSAppleScript(source:) result, so add explicit handling for script creation failure before executeAndReturnError. Use the existing logging pattern from installHelperLegacy and RouteManager.shared.log to record a warning when the AppleScript cannot be created, and keep the current error handling for executeAndReturnError so both failure modes are visible.Source: Coding guidelines
🤖 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.
Inline comments:
In `@docs/GOAL.md`:
- Around line 44-50: The GOAL.md section has duplicate “2026-07-04 —
continuation” headings that will trip markdownlint; update the second
continuation heading to a unique title or combine the two continuation blocks
into one. Use the duplicated heading text in the markdown section as the target
and keep the surrounding continuation content intact while making the headings
distinct.
In `@Sources/VPNBypassCore/RouteManager.swift`:
- Around line 871-882: The cancellation check inside runProcessSyncSafe is
ineffective because it runs on vpnBypassProcessQueue.async, where
Task.isCancelled will not reflect TaskGroup.cancelAll(). Update the polling loop
in RouteManager’s runProcessSyncSafe to use a cancellation probe passed in from
the caller, or check for cancellation before dispatching the GCD work so the
process can be terminated promptly instead of running to timeout.
---
Outside diff comments:
In `@Sources/VPNBypassCore/HelperManager.swift`:
- Around line 442-466: The cdhash pin write path in ensureCDHashPinned()
silently ignores a nil NSAppleScript(source:) result, so add explicit handling
for script creation failure before executeAndReturnError. Use the existing
logging pattern from installHelperLegacy and RouteManager.shared.log to record a
warning when the AppleScript cannot be created, and keep the current error
handling for executeAndReturnError so both failure modes are visible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b38e19ea-c308-48cd-893d-6551408624af
📒 Files selected for processing (32)
Helper/Info.plistREADME.mdROADMAP.mdSources/VPNBypassCore/ControlSurface.swiftSources/VPNBypassCore/HelperManager.swiftSources/VPNBypassCore/HelperProtocol.swiftSources/VPNBypassCore/MenuBarViews.swiftSources/VPNBypassCore/NotificationManager.swiftSources/VPNBypassCore/RouteManager.swiftSources/VPNBypassCore/RoutesTab.swiftSources/VPNBypassCore/RulesTab.swiftSources/VPNBypassCore/SettingsView.swiftSources/VPNBypassCore/VPNBypassApp.swiftTests/VPNBypassTests/ConfigDeriveEdgeCaseTests.swiftTests/VPNBypassTests/CredentialTemplateEdgeCaseTests.swiftTests/VPNBypassTests/HookGeneratorEdgeCaseTests.swiftTests/VPNBypassTests/IfaceGatewayEdgeCaseTests.swiftTests/VPNBypassTests/IfconfigParserEdgeCaseTests.swiftTests/VPNBypassTests/LiveProxyEgressTests.swiftTests/VPNBypassTests/ProxyForwarderTests.swiftTests/VPNBypassTests/ProxyListenerManagerTests.swiftTests/VPNBypassTests/RouteCompilerEdgeCaseTests.swiftTests/VPNBypassTests/RouteManagerIntegrationTests.swiftTests/VPNBypassTests/RuleDestinationBuilderEdgeCaseTests.swiftTests/VPNBypassTests/RuleResolverEdgeCaseTests.swiftTests/VPNBypassTests/StaleRoutePartitionTests.swiftTests/VPNBypassTests/ThemeTests.swiftdocs/AUTOPILOT-WORKLOG.mddocs/CHANGELOG.mddocs/CODE-REVIEW-3.0.1.mddocs/GOAL.mddocs/MULTI-ROUTE-DESIGN.md
💤 Files with no reviewable changes (1)
- Sources/VPNBypassCore/HelperProtocol.swift
Comment on lines
871
to
+882
| while process.isRunning && Date() < deadline { | ||
| // If a racing resolver already won (the enclosing TaskGroup called | ||
| // cancelAll()), stop polling and kill this loser instead of running it | ||
| // out to its full timeout — its result would be discarded anyway. | ||
| if Task.isCancelled { | ||
| process.terminate() | ||
| Thread.sleep(forTimeInterval: 0.05) | ||
| return nil | ||
| } | ||
| Thread.sleep(forTimeInterval: 0.01) // 10ms poll interval | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Swift Task.isCancelled value inside DispatchQueue.async closure outside task context
💡 Result:
When you access Task.isCancelled inside a DispatchQueue.async closure, it behaves according to the execution context of that closure [1]. If the DispatchQueue.async block is not executing within a Swift Concurrency Task (which is the case for standard closures dispatched to a global or custom queue), the static property Task.isCancelled will return false [1]. The static Task.isCancelled property is implemented using withUnsafeCurrentTask, which attempts to look up the current task context [1]. If there is no Task associated with the current execution context, it returns false by default [1]. Key points regarding this behavior: 1. Context Sensitivity: Task.isCancelled is context-aware [1]. It is designed to report the cancellation status of the "current" task context [2]. When running on a Dispatch queue that is not bridged to a Task, there is no such context [1]. 2. Implementation: The property essentially calls withUnsafeCurrentTask { task in task?.isCancelled?? false } [1]. Because no Task exists in that specific Dispatch closure, the closure passed to withUnsafeCurrentTask receives nil, resulting in a return value of false [1]. 3. Separation of Concerns: DispatchQueue operates on the older GCD concurrency model, whereas Task.isCancelled is a feature of the modern Swift Concurrency model [3][1]. These two systems do not automatically share task state unless you explicitly wrap the Dispatch work in a Task or use modern concurrency primitives [1]. In summary, checking Task.isCancelled outside of a structured task context will always return false, regardless of whether you are inside a DispatchQueue.async block or any other non-task-associated execution block [1].
Citations:
- 1: https://forums.swift.org/t/question-how-do-the-static-members-of-task-work/77295
- 2: https://github.com/swiftlang/swift-evolution/blob/main/proposals/0504-task-cancellation-shields.md
- 3: https://developer.apple.com/documentation/swift/task?changes=_3
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('Sources/VPNBypassCore/RouteManager.swift')
lines = p.read_text().splitlines()
for start,end in [(700,760),(840,900),(930,980)]:
print(f"\n--- {start}-{end} ---")
for i in range(start, min(end, len(lines))+1):
print(f"{i:4d}: {lines[i-1]}")
PYRepository: GeiserX/VPN-Bypass
Length of output: 8512
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('Sources/VPNBypassCore/RouteManager.swift')
lines = p.read_text().splitlines()
targets = ['func runProcessAsync', 'func runProcessParallel', 'func runProcessSyncSafe']
for t in targets:
print(f"\n=== {t} ===")
for idx,line in enumerate(lines, start=1):
if t in line:
start=max(1, idx-25); end=min(len(lines), idx+140)
for i in range(start, end+1):
print(f"{i:4d}: {lines[i-1]}")
break
PYRepository: GeiserX/VPN-Bypass
Length of output: 23612
Move cancellation out of the GCD worker
Task.isCancelled won’t observe TaskGroup.cancelAll() inside vpnBypassProcessQueue.async, so this loop still runs to timeout. Pass a cancellation probe into runProcessSyncSafe or guard before dispatching.
🤖 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 `@Sources/VPNBypassCore/RouteManager.swift` around lines 871 - 882, The
cancellation check inside runProcessSyncSafe is ineffective because it runs on
vpnBypassProcessQueue.async, where Task.isCancelled will not reflect
TaskGroup.cancelAll(). Update the polling loop in RouteManager’s
runProcessSyncSafe to use a cancellation probe passed in from the caller, or
check for cancellation before dispatching the GCD work so the process can be
terminated promptly instead of running to timeout.
…iler (US-007) The default Bypass / VPN Only apply path built its kernel-route set inline inside applyAllRoutesInternal, interleaved with DNS resolution and cache mutation, so the one code path a regression would leak had no test seam. Lift the pure route collection (catch-all injection, the first-wins dedup of both kernel destinations and (source,destination) ownership pairs, inverse CIDRs, service ranges) into a side-effect-free ClassicRouteCompiler mirroring the custom-mode RouteCompiler. Behavior-preserving: the old inline loop emitted routes in TaskGroup completion order (already non-deterministic run-to-run) and everything downstream (the helper batch-add and the activeRoutes tracking set) is order-independent — only the deduplicated route SET is observable, and commitAppliedRoutes reads only .destination from routesToAdd. The compiler emits that identical set. Adds 13 ClassicRouteCompilerTests covering catch-all, dedup, CIDR/service ranges, Bypass-vs-VPN-Only differences, and two leak-guard invariants (every ownership destination has a kernel route; each destination maps to exactly one gateway). Suite 773 -> 786, 0 failures.
The cached fast-path (applyRoutesFromCache) had its own verbatim copy of the apply head — the VPN-Only catch-all block and the routesToAdd/allSourceEntries dedup quartet. Route it through the same ClassicRouteCompiler the live path now uses, so the leak-critical head has exactly one definition. Set-equivalent: CIDR ranges and host IPs never string-collide, and the services-before-domains first-wins order is preserved by the collection order. The impure part (reading dnsDiskCache, seeding dnsCache) stays in the caller. Suite 786, 0 failures.
…ilures (US-011) Two correctness fixes from the review: 1. Timeout -> orphaned kernel routes (a leak surviving disconnect and quit). On an XPC batch-add DEADLINE timeout the helper may be slow (not dead) and have installed some/all routes without confirming. addRoutesBatch reported them all failed, so the caller recorded none in activeRoutes and removeAllRoutes never tore them down. Report an INDETERMINATE result (no confirmed failures) on timeout so the caller records the attempted destinations as active; a later teardown removes them (removeRoutesBatch tolerates already-absent routes). The XPC-error / nil-proxy paths still report all-failed — those never installed. removeRoutesBatch's timeout fallback stays all-failed (keeps tracking routes that may still be installed). A warning is logged so the timeout isn't silent. 2. updateHostsFile/modifyHostsFile now return Bool (@discardableResult). The two DNS-refresh sites that logged "hosts file updated" unconditionally now report a warning when the hosts write fails, instead of claiming success while /etc/hosts silently drifts from the kernel routes. Suite 786, 0 failures.
…t classes)
Full clean rebuild surfaced two pre-existing latent warnings:
- SMAppService.register() is a synchronous throwing API, so `await` was a no-op
("no async operations occur within await expression"). Dropped it.
- Three test classes accessed the @mainactor RouteManager.shared singleton from a
nonisolated XCTestCase (a Swift 6 language-mode error). Marked them @mainactor,
matching the 12 test classes that already were.
Whole package now builds with 0 warnings; suite 786, 0 failures.
…alidator (US-014) - removeAllRoutes now removes the catch-all destinations (0.0.0.0/1 + 128.0.0.0/1, or a custom 0.0.0.0/0) in their own fast batch BEFORE the per-host routes. If a time-capped quit cuts teardown short, the full-tunnel-defeating catch-alls are already gone rather than stranded (which would force all traffic at a dead gateway). - Documented that CommandRouter.isValidCIDR validates a rule MATCH pattern (where /0 = "match any IPv4" is intentional) and is deliberately distinct from RouteManager.isValidCIDR's route-destination rule (which rejects /0) — the review flagged this as drift, but it is a correct difference of purpose, not a bug. Suite 786, 0 failures.
The live apply path resolved a 100-domain batch all at once; each domain fans out to ~5 dig/curl subprocesses, so a large batch could spawn ~500 concurrent processes — a fork storm that oversubscribes the GCD thread pool. Replace the fire-all TaskGroup with a sliding window (16 in-flight), bounding subprocess concurrency to ~80 while keeping throughput high. Behavior-preserving: identical resolution and results, reassembled in input order. Suite 786, 0 failures.
Expand the 3.0.1 changelog to the full behavior-preserving hardening set (leak fix, hosts-failure surfacing, teardown ordering, DNS cap, routing-core test net, log hardening), and add a Status block to the code-review record marking what shipped vs what is held for Sergio's machine/decision (audit-token XPC, god-class split, DNS-refresh unification, helper-batch parallelization, US-014 leftovers).
…eScript nil, dup heading) - runProcessSyncSafe runs on a raw GCD queue (vpnBypassProcessQueue.async), not a Swift Task, so the Task.isCancelled check added earlier was always false — dead code that never killed a losing resolver. Removed it and documented that prompt loser-kill needs withTaskCancellationHandler + a cancellation flag (deferred). A loser simply runs out its own 1-3s timeout (harmless, wasted CPU). - ensureCDHashPinned() now logs + returns if NSAppleScript(source:) init fails, instead of silently no-op'ing the cdhash pin write (matching installHelperLegacy). - Made the two duplicate "2026-07-04 — continuation" GOAL.md headings distinct (markdownlint). Build clean, 0 warnings, suite green.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Sources/VPNBypassCore/RouteManager.swift (1)
4407-4453: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSynchronous disk write on
@MainActorin a documented hot path.
handle.write(contentsOf:)runs synchronously on the main actor, and this method's own doc comment notes it's called "hundreds of lines per apply/refresh." The caching of the formatter/handle is a real improvement over the old per-line formatter + reopen, but the write itself is still blocking on the UI thread per call. Consider batching or moving the write to a background serial queue if this becomes a perceptible stall during large applies.🤖 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 `@Sources/VPNBypassCore/RouteManager.swift` around lines 4407 - 4453, The `RouteManager.log(_:_:)` hot path still performs a synchronous `handle.write(contentsOf:)` on `@MainActor`, which can block the UI during large apply/refresh bursts. Update the logging path in `RouteManager` to avoid writing directly on the main actor by batching log entries or handing them off to a background serial queue, while keeping the existing `logFormatter`/`logFileHandle` reuse and preserving the same file output behavior.
🤖 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.
Inline comments:
In `@Sources/VPNBypassCore/RouteManager.swift`:
- Around line 1667-1718: The sliding-window cap was added to the domain resolver
loop, but the host-resolution path in resolveHostsParallel still starts every
item in a 100-item batch concurrently, which can recreate the
fork-storm/oversubscription problem. Update resolveHostsParallel to use the same
bounded in-flight pattern as the current batch resolver (for example via
withTaskGroup and a maxConcurrentResolves limit), and make sure the doc comment
reflects the actual concurrency limit instead of “up to 100 concurrent.”
---
Nitpick comments:
In `@Sources/VPNBypassCore/RouteManager.swift`:
- Around line 4407-4453: The `RouteManager.log(_:_:)` hot path still performs a
synchronous `handle.write(contentsOf:)` on `@MainActor`, which can block the UI
during large apply/refresh bursts. Update the logging path in `RouteManager` to
avoid writing directly on the main actor by batching log entries or handing them
off to a background serial queue, while keeping the existing
`logFormatter`/`logFileHandle` reuse and preserving the same file output
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 084cd8f8-0c54-4485-a45f-0926d5938264
📒 Files selected for processing (12)
Sources/VPNBypassCore/ClassicRouteCompiler.swiftSources/VPNBypassCore/CommandRouter.swiftSources/VPNBypassCore/HelperManager.swiftSources/VPNBypassCore/RouteManager.swiftTests/VPNBypassTests/ClassicRouteCompilerTests.swiftTests/VPNBypassTests/CleanDomainTests.swiftTests/VPNBypassTests/ConfigDeriveEdgeCaseTests.swiftTests/VPNBypassTests/IPValidationExtendedTests.swiftdocs/AUTOPILOT-WORKLOG.mddocs/CHANGELOG.mddocs/CODE-REVIEW-3.0.1.mddocs/GOAL.md
✅ Files skipped from review due to trivial changes (6)
- Sources/VPNBypassCore/CommandRouter.swift
- Tests/VPNBypassTests/CleanDomainTests.swift
- docs/GOAL.md
- docs/CODE-REVIEW-3.0.1.md
- docs/AUTOPILOT-WORKLOG.md
- docs/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- Tests/VPNBypassTests/ConfigDeriveEdgeCaseTests.swift
- Sources/VPNBypassCore/HelperManager.swift
Comment on lines
1667
to
1718
| while index < allDomains.count { | ||
| let endIndex = min(index + batchSize, allDomains.count) | ||
| let batch = Array(allDomains[index..<endIndex]) | ||
|
|
||
| // Resolve DNS in parallel (truly parallel - nonisolated) | ||
| let dnsResults = await withTaskGroup(of: (domain: String, source: String, ips: [String]?).self) { group in | ||
| for item in batch { | ||
| group.addTask { | ||
| let ips = await Self.resolveIPsParallel(for: item.domain, userDNS: userDNS, fallbackDNS: fallbackDNS) | ||
| return (item.domain, item.source, ips) | ||
| } | ||
|
|
||
| // Sliding-window resolve: cap in-flight domain resolutions. Each domain fans out to | ||
| // ~5 dig/curl subprocesses, so an unbounded 100-wide batch could spawn ~500 concurrent | ||
| // processes — a fork storm that oversubscribes the GCD pool. Bound the in-flight count | ||
| // so subprocess concurrency stays ~80. Results are reassembled in input order. | ||
| let maxConcurrentResolves = 16 | ||
| let dnsResults = await withTaskGroup(of: (idx: Int, ips: [String]?).self) { group in | ||
| var next = 0 | ||
| while next < batch.count && next < maxConcurrentResolves { | ||
| let i = next, item = batch[i] | ||
| group.addTask { (i, await Self.resolveIPsParallel(for: item.domain, userDNS: userDNS, fallbackDNS: fallbackDNS)) } | ||
| next += 1 | ||
| } | ||
|
|
||
| var results: [(domain: String, source: String, ips: [String]?)] = [] | ||
| for await result in group { | ||
| var results: [(idx: Int, ips: [String]?)] = [] | ||
| while let result = await group.next() { | ||
| results.append(result) | ||
| if next < batch.count { | ||
| let i = next, item = batch[i] | ||
| group.addTask { (i, await Self.resolveIPsParallel(for: item.domain, userDNS: userDNS, fallbackDNS: fallbackDNS)) } | ||
| next += 1 | ||
| } | ||
| } | ||
| return results | ||
| return results.sorted { $0.idx < $1.idx } | ||
| } | ||
|
|
||
| // Collect routes from DNS results and cache for hosts file | ||
|
|
||
| for result in dnsResults { | ||
| let item = batch[result.idx] | ||
| if let ips = result.ips, !ips.isEmpty { | ||
| // DNS succeeded - use fresh IPs and update disk cache | ||
| // DNS succeeded - use fresh IPs and update caches | ||
| if let firstIP = ips.first { | ||
| dnsCache[result.domain] = firstIP | ||
| dnsCache[item.domain] = firstIP | ||
| } | ||
| dnsDiskCache[result.domain] = ips // Update persistent cache | ||
|
|
||
| for ip in ips { | ||
| // Dedup kernel operations | ||
| if !seenDestinations.contains(ip) { | ||
| seenDestinations.insert(ip) | ||
| routesToAdd.append((destination: ip, gateway: routeGateway, isNetwork: false, source: result.source)) | ||
| } | ||
| // Dedup (source, destination) ownership pairs | ||
| let key = "\(result.source)|\(ip)" | ||
| if !seenSourceDests.contains(key) { | ||
| seenSourceDests.insert(key) | ||
| allSourceEntries.append((destination: ip, source: result.source)) | ||
| } | ||
| } | ||
| } else if let cachedIPs = dnsDiskCache[result.domain], !cachedIPs.isEmpty { | ||
| dnsDiskCache[item.domain] = ips // Update persistent cache | ||
| resolvedGroups.append(ClassicRouteCompiler.ResolvedGroup(source: item.source, ips: ips)) | ||
| } else if let cachedIPs = dnsDiskCache[item.domain], !cachedIPs.isEmpty { | ||
| // DNS failed but we have cached IPs - use them as fallback | ||
| log(.info, "Using cached IPs for \(result.domain)") | ||
| log(.info, "Using cached IPs for \(item.domain)") | ||
| if let firstIP = cachedIPs.first { | ||
| dnsCache[result.domain] = firstIP | ||
| } | ||
| for ip in cachedIPs { | ||
| if !seenDestinations.contains(ip) { | ||
| seenDestinations.insert(ip) | ||
| routesToAdd.append((destination: ip, gateway: routeGateway, isNetwork: false, source: result.source)) | ||
| } | ||
| let key = "\(result.source)|\(ip)" | ||
| if !seenSourceDests.contains(key) { | ||
| seenSourceDests.insert(key) | ||
| allSourceEntries.append((destination: ip, source: result.source)) | ||
| } | ||
| dnsCache[item.domain] = firstIP | ||
| } | ||
| resolvedGroups.append(ClassicRouteCompiler.ResolvedGroup(source: item.source, ips: cachedIPs)) | ||
| } else { | ||
| failedDomains.insert(result.domain) | ||
| failedDomains.insert(item.domain) | ||
| failedCount += 1 | ||
| } | ||
| } | ||
|
|
||
| index += batchSize | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and inspect the cited regions.
git ls-files Sources/VPNBypassCore/RouteManager.swift
wc -l Sources/VPNBypassCore/RouteManager.swift
sed -n '1630,1735p' Sources/VPNBypassCore/RouteManager.swift
printf '\n--- resolveHostsParallel ---\n'
rg -n "resolveHostsParallel|resolveRuleDestinations|up to 100 concurrent|100 concurrent|maxConcurrentResolves" Sources/VPNBypassCore/RouteManager.swift
sed -n '1950,2045p' Sources/VPNBypassCore/RouteManager.swiftRepository: GeiserX/VPN-Bypass
Length of output: 11168
Cap resolveHostsParallel too. It still launches every host in a 100-item batch at once, so custom-mode can hit the same fork-storm / GCD oversubscription problem as the legacy path. The doc comment saying “up to 100 concurrent” is stale now that the main resolver is windowed.
🤖 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 `@Sources/VPNBypassCore/RouteManager.swift` around lines 1667 - 1718, The
sliding-window cap was added to the domain resolver loop, but the
host-resolution path in resolveHostsParallel still starts every item in a
100-item batch concurrently, which can recreate the fork-storm/oversubscription
problem. Update resolveHostsParallel to use the same bounded in-flight pattern
as the current batch resolver (for example via withTaskGroup and a
maxConcurrentResolves limit), and make sure the doc comment reflects the actual
concurrency limit instead of “up to 100 concurrent.”
The self-hosted mac-mini runner is down (its whole fleet is offline on a macOS-26/.NET-runtime break), which blocked the required `test` check and the release DMG build. Standard GitHub-hosted runners — including macOS — are free and unlimited on public repositories (confirmed against GitHub's billing docs + the 2026 pricing changelog; only the larger/-xlarge SKUs are charged), so a public repo like this pays nothing. - ci.yml (test) and release.yml (universal DMG) -> macos-latest, which ships full Xcode (XCTest + a Swift 6 compiler for `sending`). - Dropped ci.yml's fork guard: GitHub-hosted runners are throwaway VMs, so the self-hosted fork-PR attack surface is gone and fork PRs can safely get CI. - codeql.yml -> macos-latest; stale.yml -> ubuntu-latest (a bot job needs no macOS).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In @.github/workflows/ci.yml:
- Line 33: The CI workflow currently uses actions/checkout in a way that leaves
persisted Git credentials available for later steps, which is unsafe before
running swift test on PR-controlled code. Update the checkout configuration in
the workflow to disable persisted credentials for the checkout step, keeping the
GitHub token out of local Git config while preserving the existing job flow.
- Line 39: The Xcode selection step in the CI workflow uses a non-portable
version sort, which can fail on macOS before tests start. Update the XCODE_APP
lookup in the workflow to use a portable version-aware selection approach, such
as a compatible sort implementation or a fallback to gsort -V, so the latest
Xcode is chosen reliably across macOS environments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 231a76bc-47ec-48e9-9bfe-567fed5f07c3
📒 Files selected for processing (4)
.github/workflows/ci.yml.github/workflows/codeql.yml.github/workflows/release.yml.github/workflows/stale.yml
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/release.yml
- actions/checkout with persist-credentials: false, so the GitHub token isn't left in git config while running PR-controlled code (fork PRs now run here). - Replace the non-portable `ls | sort -V` Xcode pick with maxim-lobanov/setup-xcode @latest-stable (already used by release.yml) — reliable across runner images.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Behavior-preserving 3.0.1: the two deferred RouteManager god-class refactors, more thorough testing, a whole-codebase review with its safe fixes applied, and the docs brought up to the 3.0 routing-manager reality.
What's here
Refactors (US-001/002) — verified byte-identical, zero routing change
commitAppliedRoutes(...)(3 copies → 1), and folded the custom twin in.reconcileAfterConfigChange(reconcileListeners:reapplyRoutes:sendNotification:).Testing (US-003) — extracted the pure stale-route set algebra into a testable seam and broadened edge-case coverage. Suite 710 → 773.
Review + fixes (US-004) — 7-perspective
/review-code!over the whole codebase. Applied the safe, behavior-preserving findings:/tmpto~/Library/Logs/VPNBypass/(owner-only,O_NOFOLLOW) with a cached formatter + persistent handle (closes a perf hot-path and a security finding at once).print().saveDNSCache/loadDNSCachesurface errors; the helper version probe distinguishes slow-vs-broken and retries before reinstalling; DNS resolver losers are killed on cancellation; unified cdhash-pin escaping.docs/CODE-REVIEW-3.0.1.mdfor a dedicated, on-device-tested hardening milestone — deliberately not bundled into a behavior-preserving patch.Docs (US-006) — README rewritten for the three modes, custom routes/rules, the multi-VPN / HTTP-SOCKS5-proxy / Tailscale-peer egresses, the
vpnbCLI, the 7 Settings tabs, and the cdhash-pinned NE-free helper. CHANGELOG/ROADMAP/MULTI-ROUTE-DESIGN refreshed.Verification
swift build: clean (0 warnings).swift test: 773 tests, 3 skipped, 0 failures (the 3 skips are the opt-in live-egress tests).Merging cuts the 3.0.1 release via
auto-tag— holding for review + explicit go-ahead.Summary by CodeRabbit
Bug Fixes
Documentation
Tests
Chores