Phase 2: Transparent traffic capture via pf redirect#3
Merged
Conversation
- DNS resolver (system + dig @1.1.1.1 fallback) with disk-persistent cache - Transparent proxy: raw TLS listener with SNI extraction, reuses Phase 1 TLS terminators - Privilege executor via osascript for admin operations - pf firewall manager: anchor-based rules + host routes for provider IP redirect - Interface detector: finds physical interface IP to avoid loopback redirect loop - Redirect manager: orchestrates DNS→proxy→pf→routes lifecycle with hourly refresh - Graceful shutdown: SIGINT/SIGTERM clean up pf rules and host routes - Stale rule cleanup on startup (crash recovery)
|
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:
📝 WalkthroughWalkthroughAdds a transparent TLS capture stack and supporting subsystems: DNS resolver and disk-backed cache, network interface detection, privileged command executor, PF redirect manager with persisted host routes, transparent TCP/TLS proxy, redirect orchestration, app wiring changes (index), and a CI workflow tweak for post-build diagnostics and artifact path. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun/proxy/proxy-server.ts (1)
31-53:⚠️ Potential issue | 🟠 MajorMake domain TLS startup single-flight and rejectable.
This function only stores the server after all awaits complete, so concurrent first requests for the same domain can create duplicate terminators and leak the loser. It also never rejects on
listen()failure, so a bind error hangs the caller. Cache the in-flight creation promise and reject fromserver.once("error").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/proxy/proxy-server.ts` around lines 31 - 53, getOrCreateDomainTLS can spawn duplicate servers and hang on listen errors because it only caches after awaits and doesn't reject on server.listen failure; fix by caching an in-flight Promise for the port in domainServers immediately when creation starts (so concurrent calls reuse it), create the TLS server via tls.createServer and attach server.once("error", err => reject(err)) and server.once("listening", ...) to resolve the promise, and ensure on rejection you close the server and remove the cached in-flight entry; update callers to retrieve the resolved port from the stored promise instead of assuming a stored {server, port} until creation completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Around line 58-63: The upload-artifact step named "Upload app artifact" uses
actions/upload-artifact@v4 with if-no-files-found: warn which allows CI to pass
when path: build/ is missing; change the step configuration to set
if-no-files-found: error so the upload fails loudly when no artifacts are found,
ensuring packaging regressions surface (reference the "Upload app artifact" step
and the if-no-files-found property on actions/upload-artifact@v4).
In `@src/bun/dns/dns-cache.ts`:
- Around line 40-48: The DNS resolver currently throws from resolveIPs(domain)
and therefore skips the stale-cache fallback; wrap the await resolveIPs(domain)
call in a try/catch inside the function in dns-cache.ts so that any thrown error
falls back to returning existing?.ips (the stale cache) instead of propagating,
and only call cache.set(...) and persistCache() when resolveIPs succeeds;
reference the ips variable, existing, cache.set, resolveIPs(domain), and
persistCache() when making the change.
- Around line 17-24: The code currently trusts and casts arbitrary JSON from
cacheFilePath to CacheEntry; change dns-cache.ts to strictly validate each
parsed entry before inserting into cache: after JSON.parse, ensure each value is
an object with expected keys (e.g., ips as an array, ttl/expiry as numbers),
verify every ips element matches a literal IPv4 regex (no shell metacharacters
or CIDR), skip or reject malformed entries, and only then set(cacheKey,
validatedEntry as CacheEntry); reference symbols: cacheFilePath, CacheEntry,
cache (and the loop over Object.entries) so the pf-manager.ts commands cannot be
fed tampered values.
- Around line 63-70: persistCache currently calls Bun.write without awaiting it
so write failures escape the try/catch and may be lost on shutdown; change
persistCache to be async, await Bun.write(cacheFilePath, JSON.stringify(obj,
null, 2)) inside the existing try/catch, and then update resolveWithCache to
await persistCache() where it calls it so errors are properly observed and the
cache is persisted before shutdown.
In `@src/bun/dns/dns-resolver.ts`:
- Around line 18-31: The resolveViaDig function currently hardcodes Cloudflare
by spawning dig with "@1.1.1.1", which bypasses the system resolver; change this
to use the system resolver or make the upstream DNS configurable: remove the
"@1.1.1.1" token from the Bun.spawn args in resolveViaDig so dig uses the system
resolver, or read a configurable upstream (e.g., from an environment variable or
a config value) and only append "@<upstream>" when that variable is set; ensure
the spawn arguments, error handling and tests reflect the new behavior.
In `@src/bun/index.ts`:
- Around line 139-142: When disableTransparentCapture() rejects the catch block
currently sets state.captureStatus = "error" but leaves state.proxyStatus at
"stopping"; update that catch handler to also reset state.proxyStatus (e.g., set
state.proxyStatus = "error" or another appropriate terminal state) so lifecycle
stays consistent, then call buildTrayMenu(); reference the catch around
disableTransparentCapture(), state.captureStatus, state.proxyStatus, and
buildTrayMenu() to locate the change.
In `@src/bun/net/interface-detector.ts`:
- Around line 22-27: The fallback that chooses the first non-internal IPv4 (loop
over ifaces, variables name/v4/ip) can pick VPN/bridge adapters; instead run and
parse `route -n get default` to extract the "interface:" value and select that
interface from ifaces, returning its IPv4 address; if the route command fails or
the interface isn't found, fall back to the existing loop. Update the logic in
interface-detector.ts to prefer the route-derived interface name before using
the current first-non-internal-IPv4 heuristic.
In `@src/bun/proxy/transparent-proxy.ts`:
- Around line 39-45: Don’t assume the TLS ClientHello arrives in a single data
event: replace the single-use listener on clientSocket (the
clientSocket.once("data", ...) block) with an accumulator that appends incoming
chunks and repeatedly calls extractSNI(buffer) until it returns a domain; once a
domain is returned remove the data listener and continue normal processing, and
if extractSNI remains null after a configurable maxHandshakeBytes (sane limit)
or a short timeout, destroy the socket and clean up the listener to avoid
resource leaks; ensure you reference extractSNI, clientSocket.on/once removal,
and the handshake-size and timeout checks in the implementation.
- Around line 74-85: The startup Promise in startTransparentProxy() currently
only resolves on the server "listening" callback, so listen failures never
settle; modify the transparentServer "error" handler to reject that Promise (and
clear/close transparentServer) so startTransparentProxy() can fail fast and
enableTransparentCapture() won't stay in "starting" with a non-null
transparentServer; specifically, wire the Promise's reject into the
transparentServer.on("error", ...) handler (and ensure any resources are cleaned
or transparentServer is set to null) while preserving the existing log via
console.error.
In `@src/bun/system/pf-manager.ts`:
- Around line 110-117: cleanupStaleRules currently only flushes the PF anchor
and doesn't remove leftover loopback host routes when activeHostRoutes is empty;
update cleanupStaleRules to also scan the system routing table for any host
routes pointing to the loopback (lo0) that match the manager's CIDR set and
remove them (e.g., by invoking an existing removeHostRoute/removeRoute helper or
adding a small helper to run the appropriate `route delete` commands). Reference
cleanupStaleRules, activeHostRoutes, and clearRules (and add or call
removeHostRoute/removeRoute) so the function always clears both PF rules and any
stale loopback host routes left from a previous run.
- Around line 70-84: The clearRules() implementation currently appends
"2>/dev/null; true" to PF flush and route delete commands which masks failures;
remove the trailing "; true" (and optionally the stderr redirection) from the
commands pushed for ANCHOR_NAME and for each entry in activeHostRoutes so
executePrivileged can observe and return failures, then rely on
executePrivileged's result handling (the existing result.success check) to log
and surface errors instead of always succeeding; ensure you update places
referencing ANCHOR_NAME, activeHostRoutes, and executePrivileged so failures
during PF flush or route deletes are not swallowed.
- Around line 52-60: addHostRoutes currently appends `; true` to each `route
add` so the shell always exits 0 and executePrivileged can't detect real
failures; change addHostRoutes to run the route adds in a way that preserves
failure status and treat only the "already exists"/"File exists" message as
non-fatal: remove the trailing `; true`, execute each `route add` (or execute
the joined commands but capture per-command stderr/exit), inspect result.error
(or per-command stderr) and if it contains an "already exists" message consider
that success, otherwise log the actual error and return false; reference
addHostRoutes, executePrivileged and enableTransparentCapture to ensure
enableTransparentCapture only proceeds when routes were really installed.
In `@src/bun/system/redirect-manager.ts`:
- Around line 149-160: The code sets resolvedIPs = newIPs before calling
clearRules(), applyRules(rules), and addHostRoutes(allIPs), which can leave
state inconsistent if reapply fails; change the flow so you do NOT assign
resolvedIPs/new state until after await clearRules(), await applyRules(rules),
and await addHostRoutes(allIPs) all succeed, and if any of those calls throws,
roll back or leave resolvedIPs unchanged (or disable redirects) and surface the
error; reference the variables/functions resolvedIPs, newIPs/newSet,
clearRules(), applyRules(rules), and addHostRoutes(allIPs) when making this
change.
- Around line 171-173: getRedirectCount currently returns the total number of
domain/IP pairs by flattening resolvedIPs, but enableTransparentCapture only
installs rules for unique IPs so shared endpoints are double-counted; change
getRedirectCount to build a Set of all IPs from the arrays in resolvedIPs (use
resolvedIPs.values()) and return the size of that Set so only unique IPs are
counted (update the getRedirectCount function accordingly, referencing
resolvedIPs and getRedirectCount).
---
Outside diff comments:
In `@src/bun/proxy/proxy-server.ts`:
- Around line 31-53: getOrCreateDomainTLS can spawn duplicate servers and hang
on listen errors because it only caches after awaits and doesn't reject on
server.listen failure; fix by caching an in-flight Promise for the port in
domainServers immediately when creation starts (so concurrent calls reuse it),
create the TLS server via tls.createServer and attach server.once("error", err
=> reject(err)) and server.once("listening", ...) to resolve the promise, and
ensure on rejection you close the server and remove the cached in-flight entry;
update callers to retrieve the resolved port from the stored promise instead of
assuming a stored {server, port} until creation completes.
🪄 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: 355217a1-3922-4e52-bc8d-b5cf15aeddf4
📒 Files selected for processing (11)
.github/workflows/build.ymlsrc/bun/dns/dns-cache.tssrc/bun/dns/dns-resolver.tssrc/bun/index.tssrc/bun/net/interface-detector.tssrc/bun/proxy/proxy-server.tssrc/bun/proxy/transparent-proxy.tssrc/bun/system/pf-manager.tssrc/bun/system/privilege-executor.tssrc/bun/system/redirect-manager.tssrc/shared/types.ts
Critical fixes: - Validate DNS cache entries with IPv4 check (prevents command injection) - Buffer TLS ClientHello across multiple packets with size/timeout limits - Remove '; true' masking from host routes — detect real failures - Persist host routes to disk for crash recovery cleanup Major fixes: - Single-flight domain TLS creation (prevent duplicate servers) - Stale-cache fallback on DNS resolver exceptions - Await Bun.write() in cache persistence - Use system resolver instead of hardcoded Cloudflare 1.1.1.1 - Detect default-route interface via 'route -n get default' - Propagate listen failures in transparent proxy startup - Defer DNS state commit until pf/route reapply succeeds Minor fixes: - Reset proxyStatus to 'error' on stop failure - Deduplicate redirect count with Set - CI artifact upload: warn → error for missing files
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/bun/system/redirect-manager.ts (2)
34-58:⚠️ Potential issue | 🟠 MajorDelay publishing redirect state until startup actually succeeds.
activeProvidersandresolvedIPsare updated before proxy startup and pf/route installation finish. If any later step fails,getRedirectCount()and subsequent flows will still see providers/IPs as active even though capture never came up. Keep both in locals and assign only after all startup steps succeed.Suggested fix
- activeProviders = providers.filter((p) => p.enabled); + const nextProviders = providers.filter((p) => p.enabled); - if (activeProviders.length === 0) { + if (nextProviders.length === 0) { return { success: false, error: "No providers enabled", redirectCount: 0 }; } - const allDomains = activeProviders.flatMap((p) => p.domains); - resolvedIPs.clear(); + const allDomains = nextProviders.flatMap((p) => p.domains); + const nextResolvedIPs = new Map<string, string[]>(); for (const domain of allDomains) { const ips = await resolveWithCache(domain); if (ips.length > 0) { - resolvedIPs.set(domain, ips); + nextResolvedIPs.set(domain, ips); console.log(`[Redirect] ${domain} -> ${ips.join(", ")}`); } else { console.warn(`[Redirect] Failed to resolve ${domain}`); } } - const allIPs = [...new Set([...resolvedIPs.values()].flat())]; + const allIPs = [...new Set([...nextResolvedIPs.values()].flat())]; if (allIPs.length === 0) { return { success: false, @@ if (!routeOk) { await clearRules(); stopTransparentProxy(); return { success: false, error: "Failed to add host routes", redirectCount: 0 }; } + activeProviders = nextProviders; + resolvedIPs = nextResolvedIPs; + // 5. Start DNS refresh timerAlso applies to: 86-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/system/redirect-manager.ts` around lines 34 - 58, The code updates module-level state (activeProviders and resolvedIPs) before completing startup (interface detection, DNS resolution, proxy and pf/route installation), causing getRedirectCount() to reflect active providers even if startup fails; change redirect-manager.ts to use local variables (e.g., localActiveProviders, localResolvedIPs) while performing startup steps (getPhysicalInterface(), resolveWithCache(), proxy start and pf/route installation), only assigning them to the module-level activeProviders and resolvedIPs after all steps succeed, and ensure any early failures return without mutating the shared state so getRedirectCount() remains accurate.
114-118:⚠️ Potential issue | 🟠 MajorDon’t ignore
falsefrom pf-manager operations.These helpers report failure with
false. HeredisableTransparentCapture()logs success even if cleanup failed, andrefreshDNS()still adds host routes after a failedapplyRules(). That can leave redirects active after “Disabled” or temporarily route traffic to loopback without an rdr rule.Suggested fix
export async function disableTransparentCapture(): Promise<void> { @@ - await clearRules(); + const cleared = await clearRules(); + if (!cleared) { + throw new Error("Failed to clear pf rules"); + } stopTransparentProxy(); resolvedIPs.clear(); activeProviders = []; console.log("[Redirect] Disabled"); } @@ - await clearRules(); - const pfOk = await applyRules(rules); - const routeOk = await addHostRoutes(allIPs); + const cleared = await clearRules(); + if (!cleared) { + console.error("[Redirect] DNS refresh failed to clear old rules"); + return; + } + + const pfOk = await applyRules(rules); + if (!pfOk) { + console.error("[Redirect] DNS refresh failed to apply pf rules"); + await applyRules([...oldSet].map((ip) => ({ ip, destPort: 443, proxyPort: PROXY_PORT }))); + await addHostRoutes([...oldSet]); + return; + } + + const routeOk = await addHostRoutes(allIPs);Also applies to: 157-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/system/redirect-manager.ts` around lines 114 - 118, The disable flow is ignoring false return values from pf-manager helpers which can leave redirects active; update disableTransparentCapture(), clearRules()/applyRules() usages and refreshDNS() flow so you check their boolean results and bail on failure: if clearRules()/applyRules() or disableTransparentCapture() returns false, log an error and do not call stopTransparentProxy(), refreshDNS(), add host routes to resolvedIPs or set activeProviders; only print "[Redirect] Disabled" after all cleanup calls succeed. Also apply the same return-value checks to the related calls around the code referenced at lines 157-159 (the other disable path) to prevent reporting success when pf operations failed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun/index.ts`:
- Around line 133-165: The toggle handler must early-return if a transition is
already in progress to prevent reentrancy: add a guard at the top of the toggle
routine that checks state.captureStatus (and/or state.proxyStatus) for
"starting" or "stopping" (or use a new boolean like state.isTransitioning) and
simply return without calling enableTransparentCapture or
disableTransparentCapture when true; when initiating either path set the
transitioning flag (or set captureStatus/proxyStatus immediately to
"starting"/"stopping"), and clear it in both success and catch branches of
enableTransparentCapture and disableTransparentCapture (where you already mutate
state.captureStatus/proxyStatus and call buildTrayMenu) so subsequent clicks are
ignored until the operation finishes. Ensure you reference the existing symbols
state.captureStatus, state.proxyStatus, enableTransparentCapture,
disableTransparentCapture, and buildTrayMenu when implementing the guard and
flag handling.
In `@src/bun/proxy/proxy-server.ts`:
- Around line 334-341: The shutdown race happens because cleanupDomainServers()
clears domainServers but in-flight createDomainTLS() calls can still resolve and
repopulate the map, leaving orphaned listeners; fix by adding a shutdown
coordination mechanism: introduce a shared boolean/flag (e.g. isShuttingDown)
and/or an AbortController you pass into createDomainTLS() so in-flight creations
can be cancelled, update createDomainTLS() and its callers to accept and honor
the abort signal, and in the resolver path of createDomainTLS() check
isShuttingDown (or signal.aborted) before inserting into domainServers — if
shutdown is active immediately close the newly created server instead of storing
it; finally ensure cleanupDomainServers() sets the flag/aborts controllers,
closes any servers, and clears domainServers to prevent later repopulation.
- Around line 62-69: The server currently uses server.once("error", reject)
which only covers listen-time errors; after startup attach a persistent error
handler to avoid unhandled errors later: after you resolve the port and store
the server in domainServers (refer to the server and domainServers symbols),
call server.on("error", tlsClientError) or a similar long-lived handler that
logs/handles TLS and other runtime errors; keep the initial once listener for
the listen() promise but add the persistent server.on("error", ...) immediately
after startup/storage so later errors don't crash the process.
In `@src/bun/system/redirect-manager.ts`:
- Around line 99-100: The setInterval call that assigns refreshTimer calls
refreshDNS() without handling its returned Promise, so any throw becomes an
unhandled rejection; update the timer callback to call refreshDNS and handle
rejections (e.g., wrap in an async IIFE or call refreshDNS().catch(...)) and log
errors with the existing logger, and optionally take recovery actions (retry,
backoff, or clearInterval) on repeated failures; make the change around
refreshTimer, refreshDNS, and DNS_REFRESH_INTERVAL so the timer never yields
unhandled Promise rejections.
---
Duplicate comments:
In `@src/bun/system/redirect-manager.ts`:
- Around line 34-58: The code updates module-level state (activeProviders and
resolvedIPs) before completing startup (interface detection, DNS resolution,
proxy and pf/route installation), causing getRedirectCount() to reflect active
providers even if startup fails; change redirect-manager.ts to use local
variables (e.g., localActiveProviders, localResolvedIPs) while performing
startup steps (getPhysicalInterface(), resolveWithCache(), proxy start and
pf/route installation), only assigning them to the module-level activeProviders
and resolvedIPs after all steps succeed, and ensure any early failures return
without mutating the shared state so getRedirectCount() remains accurate.
- Around line 114-118: The disable flow is ignoring false return values from
pf-manager helpers which can leave redirects active; update
disableTransparentCapture(), clearRules()/applyRules() usages and refreshDNS()
flow so you check their boolean results and bail on failure: if
clearRules()/applyRules() or disableTransparentCapture() returns false, log an
error and do not call stopTransparentProxy(), refreshDNS(), add host routes to
resolvedIPs or set activeProviders; only print "[Redirect] Disabled" after all
cleanup calls succeed. Also apply the same return-value checks to the related
calls around the code referenced at lines 157-159 (the other disable path) to
prevent reporting success when pf operations failed.
🪄 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: 23dbe4d1-a701-4248-a220-523b1a184743
📒 Files selected for processing (9)
.github/workflows/build.ymlsrc/bun/dns/dns-cache.tssrc/bun/dns/dns-resolver.tssrc/bun/index.tssrc/bun/net/interface-detector.tssrc/bun/proxy/proxy-server.tssrc/bun/proxy/transparent-proxy.tssrc/bun/system/pf-manager.tssrc/bun/system/redirect-manager.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/bun/dns/dns-resolver.ts
- src/bun/proxy/transparent-proxy.ts
- src/bun/system/pf-manager.ts
- Guard toggle during starting/stopping to prevent re-entry
- Add persistent server.on("error") handler after TLS listen succeeds
- Generation-based shutdown to cancel in-flight domain TLS creation
- Catch refreshDNS() promise rejections in setInterval
- Defer activeProviders/resolvedIPs until all startup steps succeed
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun/proxy/proxy-server.ts`:
- Around line 41-49: The cleanup of domainInFlight must be guarded by promise
identity to avoid deleting a newer in-flight entry when an older promise
resolves; update the catch block around createDomainTLS (where you set const
promise = createDomainTLS(domain) and domainInFlight.set(domain, promise)) and
the cleanupDomainServers() removal to only call domainInFlight.delete(domain) if
domainInFlight.get(domain) === promise (i.e., compare the stored promise to the
resolving/rejecting promise before deleting) so that only the exact promise
instance removes its map entry and you preserve newer in-flight promises.
In `@src/bun/system/redirect-manager.ts`:
- Around line 31-33: The enable/disable/refresh flow mutates shared module state
(activeProviders, resolvedIPs, interval timer) without guarding concurrent
calls; serialize these transitions by introducing a single in-flight guard
(e.g., a module-scoped Promise lock or a numeric sessionId + isActive flag) that
every entrypoint (enableTransparentCapture, disableTransparentCapture,
refreshDNS) awaits before mutating state, canceling or ignoring stale work.
Ensure enableTransparentCapture checks the current sessionId/isActive before
publishing activeProviders/resolvedIPs and sets/clears the interval only when
still the active session; ensure disableTransparentCapture clears the interval
and increments/invalidate the session so any pending refreshDNS or older enable
call is a no-op. Use the unique symbols enableTransparentCapture,
disableTransparentCapture, refreshDNS, activeProviders, resolvedIPs, and the
timer/interval variable to locate where to add the guard and session checks.
- Around line 165-167: The code currently calls addHostRoutes(allIPs) even when
applyRules(rules) (pfOk) fails, breaking atomicity; update the flow in the
function containing clearRules, applyRules, and addHostRoutes so that after
awaiting applyRules(rules) you short-circuit on a false pfOk (e.g., skip calling
addHostRoutes and propagate or return the failure), i.e., only invoke
addHostRoutes(allIPs) when pfOk is true and preserve/return routeOk accordingly;
reference the clearRules, applyRules, addHostRoutes calls and the pfOk/routeOk
variables to locate and change the logic.
🪄 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: 7f925c1e-c701-490d-b7e0-33869861db8d
📒 Files selected for processing (3)
src/bun/index.tssrc/bun/proxy/proxy-server.tssrc/bun/system/redirect-manager.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bun/index.ts
Comment on lines
+41
to
+49
| const promise = createDomainTLS(domain); | ||
| domainInFlight.set(domain, promise); | ||
|
|
||
| try { | ||
| const port = await promise; | ||
| return port; | ||
| } catch (err) { | ||
| domainInFlight.delete(domain); | ||
| throw err; |
There was a problem hiding this comment.
Guard domainInFlight cleanup by promise identity.
Line 48 and Line 84 delete by domain key unconditionally. After cleanupDomainServers() clears the map, an older promise can settle later and wipe out a newer in-flight entry for the same domain. That defeats the single-flight guard and can spin up duplicate per-domain TLS listeners during stop/start churn.
Suggested fix
export async function getOrCreateDomainTLS(domain: string): Promise<number> {
const existing = domainServers.get(domain);
if (existing) return existing.port;
// Single-flight: reuse in-flight creation for concurrent requests
const inFlight = domainInFlight.get(domain);
if (inFlight) return inFlight;
const promise = createDomainTLS(domain);
domainInFlight.set(domain, promise);
try {
- const port = await promise;
- return port;
- } catch (err) {
- domainInFlight.delete(domain);
- throw err;
+ return await promise;
+ } finally {
+ if (domainInFlight.get(domain) === promise) {
+ domainInFlight.delete(domain);
+ }
}
}
...
domainServers.set(domain, { server, port });
- domainInFlight.delete(domain);
return port;
}Also applies to: 83-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun/proxy/proxy-server.ts` around lines 41 - 49, The cleanup of
domainInFlight must be guarded by promise identity to avoid deleting a newer
in-flight entry when an older promise resolves; update the catch block around
createDomainTLS (where you set const promise = createDomainTLS(domain) and
domainInFlight.set(domain, promise)) and the cleanupDomainServers() removal to
only call domainInFlight.delete(domain) if domainInFlight.get(domain) ===
promise (i.e., compare the stored promise to the resolving/rejecting promise
before deleting) so that only the exact promise instance removes its map entry
and you preserve newer in-flight promises.
- Guard domainInFlight cleanup by promise identity - Serialize capture transitions with promise-based lock - Short-circuit route updates after PF failure in DNS refresh
- Add Trace Streaming API section (Phase 3.4): external app hooks via WebSocket, Unix socket, webhooks, and pipe mode so commercial apps can consume the trace stream without modifying AgentTap - Add AI SDK evaluation note: consider @ai-sdk/* packages for provider response parsing instead of hand-rolling schemas - Mark Phase 2 items as complete (DNS, pf rules, transparent proxy, privilege escalation, interface detection, serialized transitions) - Add Linux testing note (Alex to test on Sprites sandbox) - Update version milestones to reflect current progress
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ROADMAP.md`:
- Around line 107-113: The markdown fenced code blocks around the pf rule ("rdr
pass on lo0 proto tcp from any to <provider-ip> port 443 -> 127.0.0.1 port
8443") and the route command ("/sbin/route add -host <provider-ip> 127.0.0.1")
are missing language identifiers; update the triple-backtick fences to include
appropriate languages (for example, use ```pf for the rdr pass line and ```bash
for the /sbin/route line) so the blocks satisfy MD040 and improve readability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
- Privileged helper (LaunchDaemon) eliminates repeated admin password prompts — one-time install via osascript, then all pf/route operations go through the helper's Unix domain socket (JSON line protocol) - Helper client in TypeScript with typed commands: applyPfRules, addHostRoutes, clearRules, getVersion - pf-manager rewritten to use helper when available, osascript fallback - Tray menu now shows visual indicator (● when capturing) and "Starting…"/"Stopping…" labels during transitions - Debug HTTP server on localhost:19876 with /status, /start, /stop - GitHub Actions release workflow: builds app + helper, creates DMG, publishes GitHub Release, auto-updates homebrew cask
Derives version from git tag and updates electrobun.config.ts, package.json, and startup log before building. Tag is the single source of truth — no manual version bumps needed.
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.
Summary
osascriptadmin prompt for pf/route operations (XPC in future phase)Architecture
New files
src/bun/dns/dns-resolver.tssrc/bun/dns/dns-cache.tssrc/bun/net/interface-detector.tssrc/bun/proxy/transparent-proxy.tssrc/bun/system/privilege-executor.tssrc/bun/system/pf-manager.tssrc/bun/system/redirect-manager.tsTest results (12/12 component tests passing)
Manual test plan
bun run devpfctl -a com.agenttap/redirect -sr)Summary by CodeRabbit
New Features
Improvements