Phase 2.5: DNS-based interception, SQLite storage, new providers#4
Phase 2.5: DNS-based interception, SQLite storage, new providers#4
Conversation
…sor providers Replace IP-based host routes with /etc/hosts sentinel IPs + DoH resolution + native TCP forwarder using IP_BOUND_IF for lo0 bypass. This solves AWS Bedrock and Cursor rotating IP issues — DNS interception catches all traffic regardless of IP rotation. - Helper now manages /etc/hosts entries (BEGIN/END AgentTap block) + DNS flush - DoH resolver (Cloudflare + Google fallback) bypasses /etc/hosts for real IPs - Native C forwarder (agenttap-fwd) with IP_BOUND_IF on physical interface - SQLite trace storage with WAL mode, session detection, query endpoints - Add Bedrock (us-east-1, us-west-2, eu-west-1) and Cursor providers - Debug server: /traces, /traces/:id, /sessions, /stats endpoints - Source app detection from User-Agent (Claude Code, Cursor, Aider, etc.)
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 44 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughAdds DNS-based macOS transparent interception with a native interface‑bound TCP forwarder, DoH resolution, host-sentinel 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 |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
…p to v0.3.0 - Bedrock and Cursor providers disabled by default — Node.js apps need restart after NODE_EXTRA_CA_CERTS is set via launchctl, so enabling these by default breaks already-running Claude Code/Cursor sessions - Remove rejectUnauthorized:false on upstream TLS — the forwarder tunnel passes real server certs through correctly (verified with Anthropic + OpenAI) - Align version to 0.3.0 across package.json, electrobun.config.ts, version.json
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
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)
266-365:⚠️ Potential issue | 🟠 MajorDon’t finalize normal HTTP responses on socket shutdown.
This handler still treats
end/closeas a successful completion path for non-SSE traffic, so an upstream disconnect or mid-stream TLS failure can be stored as a complete trace with a truncated body. Keep shutdown-based finalization limited to SSE and only mark normal responses complete onceContent-Lengthor the chunked terminator has been satisfied.As per coding guidelines, "Never rely on HTTP
end/closeevents for response completion; detect completion by Content-Length, chunked encoding terminator0\r\n\r\n, or connection close for SSE".🤖 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 266 - 365, The handler finalizes and records non-SSE responses on upstream 'end'/'close', allowing truncated traces; change wireUpstreamHandlers so finalize() is only invoked on 'end'/'close' when isSSE is true. Remove finalize() calls from upstream.on("end") and upstream.on("close") for non-SSE flows (just end/destroy clientSocket), rely on the existing content-length and CHUNKED_TERMINATOR logic in the upstream.on("data") path (which uses parseResponseHead, contentLength, isChunked, dechunk and CHUNKED_TERMINATOR) to call finalize() when the response is actually complete, and keep finalize() guarded by the finalized flag.
🧹 Nitpick comments (8)
src/bun/proxy/upstream-forwarder.ts (1)
31-44: Add production guard for CWD-derived path search.
native-bridge.tsexplicitly checksisDev()before searching CWD-derived paths to "avoid library hijack in production". This file always walks up fromprocess.cwd(), which could allow execution of a malicious binary if bundle paths are missing.The current order (bundle paths first) mitigates this, but adding an explicit dev-mode check provides defense in depth.
Proposed fix
+function isDev(): boolean { + return !import.meta.dir.includes(".app/Contents/"); +} + function findForwarderBinary(): string | null { const name = "agenttap-fwd"; const candidates = [ // App bundle: Contents/MacOS/ join(import.meta.dir, "..", "..", "MacOS", name), // App bundle: Contents/Resources/ join(import.meta.dir, "..", "Resources", name), ]; - // Dev mode: native/build/ - let dir = process.cwd(); - for (let i = 0; i < 10; i++) { - try { - const stat = Bun.file(join(dir, "package.json")); - if (stat.size > 0) { - candidates.push(join(dir, "native", "build", name)); - break; - } - } catch { /* keep walking */ } - const parent = join(dir, ".."); - if (parent === dir) break; - dir = parent; + // Only search CWD-derived paths in dev mode (avoid binary hijack in production) + if (isDev()) { + let dir = process.cwd(); + for (let i = 0; i < 10; i++) { + try { + const stat = Bun.file(join(dir, "package.json")); + if (stat.size > 0) { + candidates.push(join(dir, "native", "build", name)); + break; + } + } catch { /* keep walking */ } + const parent = join(dir, ".."); + if (parent === dir) break; + dir = parent; + } } return candidates.find((p) => existsSync(p)) ?? null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/proxy/upstream-forwarder.ts` around lines 31 - 44, The CWD upward walk that pushes candidates based on process.cwd() should only run in development; add an explicit isDev() guard around the loop that walks from process.cwd() so production avoids deriving paths from the CWD (leaving existing candidate order and bundle-path checks intact). Specifically, wrap the existing loop that references process.cwd(), join(dir, "package.json"), candidates, and name with a check like if (isDev()) { ... } so the code path that searches parent directories is skipped in production.src/bun/net/interface-bind.ts (2)
90-92: Consider logging ifSO_REUSEADDRfails.The return value is discarded. While non-critical, logging at debug level helps diagnose obscure socket issues.
- lib.symbols.setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, ptr(oneBuf), 4); + const reuseResult = lib.symbols.setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, ptr(oneBuf), 4) as number; + if (reuseResult !== 0) { + console.debug(`[InterfaceBind] SO_REUSEADDR failed (non-fatal)`); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/net/interface-bind.ts` around lines 90 - 92, Check the return value from the lib.symbols.setsockopt call that sets SO_REUSEADDR (the line using fd, SOL_SOCKET, SO_REUSEADDR, ptr(oneBuf), 4) and log a debug message if it indicates failure; specifically capture the integer return and, on non-zero, call the module's debug logger (or console.debug) with a clear message including fd and the errno/return code to aid diagnosis. Ensure you still pass ptr(oneBuf) and keep the current behavior if setsockopt succeeds.
109-117: Bind failure is silently swallowed.Comment says it's non-fatal, but the caller has no way to know whether the socket is bound to the requested port or an ephemeral one. Consider returning a status object or logging at debug level.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/net/interface-bind.ts` around lines 109 - 117, The bind failure is currently swallowed in interface-bind.ts (makeSockaddrIn + lib.symbols.bind returning bindResult) so callers cannot tell whether the socket was bound to the requested localPort or an ephemeral port; update the function that currently returns fd to return a status object (e.g., { fd, bound: boolean, requestedPort: number, actualPort?: number }) or at minimum return a tuple/[fd, bound] and set bound = bindResult === 0, and add a debug log when bindResult !== 0; ensure callers of this function are updated to handle the new return shape and/or check the bound flag.src/bun/system/pf-manager.ts (2)
42-46: Unused parameters:ifaceandgateway.These are declared but never used in the function body. Leftover from IP-based approach?
🧹 Remove if not needed
export async function applyRules( domains: string[], - iface?: string, - gateway?: string, ): Promise<boolean> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/system/pf-manager.ts` around lines 42 - 46, The applyRules function declares unused parameters iface and gateway; to fix, either remove these parameters from the function signature and update any callers to stop passing them, or if they were intended to affect behavior, wire them into the function logic (e.g., use iface to select the interface and gateway to set route/next-hop) and ensure tests/call sites pass the appropriate values; reference the applyRules(domains: string[], iface?: string, gateway?: string) declaration when making the change.
117-126: Conditioncommands.length > 0is always true.The array is initialized with one element on line 118, so this check never fails.
- const commands = [`pfctl -a "${ANCHOR_NAME}" -F all 2>/dev/null`]; - if (commands.length > 0) { - const result = await executePrivileged(commands.join(" ; ") + " ; true"); + const result = await executePrivileged(`pfctl -a "${ANCHOR_NAME}" -F all 2>/dev/null ; true`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/system/pf-manager.ts` around lines 117 - 126, The condition checking commands.length > 0 is redundant because commands is initialized with one element; remove the array-length guard and simply invoke executePrivileged with the constructed command for clearing the PF anchor, or build the commands array only when needed; update the pf-clear block around ANCHOR_NAME to call executePrivileged(commands.join(" ; ") + " ; true") directly and keep the existing error handling that logs result.error on failure (ensure you update the code paths in pf-manager.ts where commands and executePrivileged are used).src/bun/storage/trace-writer.ts (1)
34-54: Traces queryable by provider, model, timestamp — sourceApp via session JOIN.Per the coding guideline, traces must be queryable by provider, model, timestamp, and source app. The trace record stores
provider,model,timestamp, andsession_id. Source app is on the session, requiring a JOIN for that query dimension.This is a reasonable design (avoids duplicating sourceApp on every trace), but confirm the query patterns in
trace-db.tssupport this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/storage/trace-writer.ts` around lines 34 - 54, The trace record currently omits source_app (stored on sessions), so update the trace read/query functions (e.g., queryTraces / getTraces in trace-db.ts) to JOIN the sessions table on trace.session_id and include sessions.source_app in the SELECT and WHERE clauses so queries can filter by provider, model, timestamp and source_app; ensure the JOIN uses trace.session_id = sessions.id and that any pagination/sort logic still uses trace.timestamp; keep insertTrace unchanged but add necessary tests for filtering by source_app.src/bun/index.ts (1)
296-301: Consider validating trace ID format.The ID is extracted directly from the URL path and passed to
getTraceById. If IDs have a known format (e.g., UUID), validating prevents unnecessary DB queries for malformed input.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/index.ts` around lines 296 - 301, The code extracts `id` via `const id = path.slice("/traces/".length)` and calls `getTraceById(id)` without validating format; add a validation step (e.g., UUID regex or your service's ID pattern) before calling `getTraceById`, return a 400 Bad Request (Response.json with status 400) for malformed IDs, and only call `getTraceById` when the ID passes validation; update the block that checks `if (path.startsWith("/traces/")) { ... }` accordingly.src/bun/net/interface-detector.ts (1)
26-32: Minor:en0may be checked twice in the fallback loop.The array
["en0", ...Object.keys(ifaces).filter((n) => n.startsWith("en")).sort()]includesen0explicitly, then again from the filter if it exists. Not a bug (first match returns), but slightly redundant.🔧 Optional fix
- for (const enName of ["en0", ...Object.keys(ifaces).filter((n) => n.startsWith("en")).sort()]) { + const enInterfaces = Object.keys(ifaces).filter((n) => n.startsWith("en")).sort(); + // Prioritize en0 by moving it to front if present + const orderedEn = enInterfaces.includes("en0") + ? ["en0", ...enInterfaces.filter((n) => n !== "en0")] + : enInterfaces; + for (const enName of orderedEn) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/net/interface-detector.ts` around lines 26 - 32, The loop that prefers "en0" uses ["en0", ...Object.keys(ifaces).filter((n) => n.startsWith("en")).sort()] which can include "en0" twice; update the iterable used in the for (const enName of ...) to dedupe "en0" (e.g., filter out "en0" from the spread or construct a Set) so "en0" appears only once when iterating over ifaces; adjust the code around the for loop that references ifaces and enName to use the deduped list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@native/Helper/HelperTool.swift`:
- Around line 314-325: Replace the loose ip.hasPrefix("127.0.1.") check in the
entries validation loop with a strict IPv4 literal check: match the whole string
against a pattern like ^127\.0\.1\.(\d{1,3})$ (or use an IPv4 parser) then parse
the captured last octet and ensure it is within the allowed sentinel range (e.g.
1..254); keep returning the same error shape on failure. Update the validation
in the for entry in entries loop (variables ip, domain, id) in HelperTool.swift
so malformed values like 127.0.1.999 are rejected and only valid 127.0.1.x
addresses with the bounded last octet pass.
- Around line 127-148: verifyAnchors currently reads arbitrary anchorName from
params without checking the allowlist, letting callers query any PF anchor; add
the same allowlist guard used in applyPfRules() and clearRules() by calling
HelperTool.isAllowedAnchor(anchorName) after extracting anchorName and return an
error response (e.g., ["id": id, "success": false, "error": "Not allowed
anchor"]) when it fails; ensure the early-return shape matches the existing
responses so rdrResult/filterResult are only executed for allowed anchors.
- Around line 109-110: Do not unconditionally flip net.inet.ip.forwarding;
instead read the current value with run("/usr/sbin/sysctl", args: ["-n",
"net.inet.ip.forwarding"]) before changing in the code that currently uses
run("/usr/sbin/sysctl", args: ["-w", "net.inet.ip.forwarding=1"]) (and the
counterpart that writes it back), only call the sysctl write if the current
value is 0, and store that original value in a persistent/local state so the
clear/teardown path restores it to the original value (rather than always
forcing 0); apply the same read-then-conditional-write-and-restore logic for the
related write at the other occurrence (lines ~219-220).
In `@src/bun/proxy/proxy-server.ts`:
- Around line 245-250: The upstream TLS connection currently disables
certificate verification by setting rejectUnauthorized: false in the tls.connect
call that creates upstream; change this to enforce verification
(rejectUnauthorized: true) and remove the blanket TODO; if you need a temporary
bypass, gate it behind an explicit, opt-in debug/config flag (e.g., an env var
like ALLOW_INSECURE_UPSTREAM or a runtime option) checked where tls.connect is
called (use UPSTREAM_TIMEOUT, domain and the existing tunnel variable), so
production defaults always verify certificates.
In `@src/bun/proxy/upstream-forwarder.ts`:
- Around line 80-86: The exit handler for fwdProcess can race with
stopForwarder() and log an "unexpected exit" even when stopForwarder
intentionally cleared it; to fix, capture the current fwdProcess reference when
you register the exited callback (e.g., const proc = fwdProcess) and inside the
fwdProcess.exited.then handler only log/clear if the global fwdProcess still
equals that captured reference (and then set it to null), so the handler ignores
exits for processes already cleared by stopForwarder; update the handler around
fwdProcess.exited and ensure stopForwarder continues to set fwdProcess = null.
In `@src/bun/storage/trace-db.ts`:
- Around line 80-106: The activeSessions Map is only keyed by provider, so
getOrCreateSession merges sessions across different source_app values; change
the cache key to include sourceApp (e.g., composite key `${provider}:${sourceApp
?? ''}`) and update all usages in getOrCreateSession (lookup, update, closing,
and set) to use that composite key so sessions are tracked per
provider+sourceApp while keeping the same SESSION_GAP_MS logic and DB
INSERT/UPDATE behavior; ensure ActiveSession entries still contain provider and
sourceApp fields and that the timestamp/closing logic uses the matched entry.
- Around line 194-207: The current API only exposes
getRecentTraces/getTraceById/getRecentSessions and lacks filtering by provider,
model, timestamp range, and source app; implement a new query function (e.g.,
getTracesFiltered or extend getRecentTraces) that accepts optional filters:
provider, model, startTimestamp, endTimestamp, sourceApp, limit, offset; build a
parameterized SQL SELECT that applies WHERE clauses only for provided filters
and, to support source_app, JOIN traces to sessions on session_id
(sessions.source_app) so traces can be filtered by source app; ensure the
function returns the same Record<string, unknown>[] shape and uses
getDB().query(...).all(...) with proper parameter binding and ordering by
timestamp DESC.
In `@src/bun/system/redirect-manager.ts`:
- Around line 144-156: verifyAndRepairRules currently only verifies PF rules via
verifyRulesActive() and therefore misses DNS/hosts interception breakage; add a
DNS interception health check (e.g., verifyDnsInterception or extend
verifyRulesActive to include DNS) and call it alongside verifyRulesActive()
inside verifyAndRepairRules, and when DNS check fails invoke the corresponding
repair routine (e.g., repairDnsInterception or extend applyRules to restore
hosts/AgentTap entries using activeDomains) so both PF and DNS states are
validated and repaired. Include references to verifyAndRepairRules,
verifyRulesActive, applyRules and activeDomains when making the change.
- Around line 100-109: When applyRules(allDomains) fails after
setNodeCATrust(certPath) has already run, revert the NODE_EXTRA_CA_CERTS change
so new Node/Bun processes don't continue trusting the AgentTap CA; update the
flow around setNodeCATrust, applyRules, and stopTransparentProxy so that on
pf/apply failure you call a rollback function (e.g., unsetNodeCATrust or restore
previous NODE_EXTRA_CA_CERTS value) and ensure cleanup runs before returning the
error. Locate setNodeCATrust(certPath) and add a failure-path call to the
rollback/unset routine (and/or wrap in try/finally) so the launchctl environment
is restored when applyRules or subsequent steps fail.
- Around line 176-191: The current setNodeCATrust and clearNodeCATrust always
overwrite/unset the launchd NODE_EXTRA_CA_CERTS which loses any preexisting user
setting; change them to preserve and restore the prior value: in setNodeCATrust
call launchctl getenv NODE_EXTRA_CA_CERTS (e.g. via Bun.spawnSync) and store the
returned value in a module-level variable (e.g. previousNodeExtraCACerts) before
calling launchctl setenv to the new certPath; in clearNodeCATrust, if
previousNodeExtraCACerts is non-empty call launchctl setenv to restore it,
otherwise call launchctl unsetenv, and update logs accordingly; keep using the
existing function names setNodeCATrust and clearNodeCATrust.
---
Outside diff comments:
In `@src/bun/proxy/proxy-server.ts`:
- Around line 266-365: The handler finalizes and records non-SSE responses on
upstream 'end'/'close', allowing truncated traces; change wireUpstreamHandlers
so finalize() is only invoked on 'end'/'close' when isSSE is true. Remove
finalize() calls from upstream.on("end") and upstream.on("close") for non-SSE
flows (just end/destroy clientSocket), rely on the existing content-length and
CHUNKED_TERMINATOR logic in the upstream.on("data") path (which uses
parseResponseHead, contentLength, isChunked, dechunk and CHUNKED_TERMINATOR) to
call finalize() when the response is actually complete, and keep finalize()
guarded by the finalized flag.
---
Nitpick comments:
In `@src/bun/index.ts`:
- Around line 296-301: The code extracts `id` via `const id =
path.slice("/traces/".length)` and calls `getTraceById(id)` without validating
format; add a validation step (e.g., UUID regex or your service's ID pattern)
before calling `getTraceById`, return a 400 Bad Request (Response.json with
status 400) for malformed IDs, and only call `getTraceById` when the ID passes
validation; update the block that checks `if (path.startsWith("/traces/")) { ...
}` accordingly.
In `@src/bun/net/interface-bind.ts`:
- Around line 90-92: Check the return value from the lib.symbols.setsockopt call
that sets SO_REUSEADDR (the line using fd, SOL_SOCKET, SO_REUSEADDR,
ptr(oneBuf), 4) and log a debug message if it indicates failure; specifically
capture the integer return and, on non-zero, call the module's debug logger (or
console.debug) with a clear message including fd and the errno/return code to
aid diagnosis. Ensure you still pass ptr(oneBuf) and keep the current behavior
if setsockopt succeeds.
- Around line 109-117: The bind failure is currently swallowed in
interface-bind.ts (makeSockaddrIn + lib.symbols.bind returning bindResult) so
callers cannot tell whether the socket was bound to the requested localPort or
an ephemeral port; update the function that currently returns fd to return a
status object (e.g., { fd, bound: boolean, requestedPort: number, actualPort?:
number }) or at minimum return a tuple/[fd, bound] and set bound = bindResult
=== 0, and add a debug log when bindResult !== 0; ensure callers of this
function are updated to handle the new return shape and/or check the bound flag.
In `@src/bun/net/interface-detector.ts`:
- Around line 26-32: The loop that prefers "en0" uses ["en0",
...Object.keys(ifaces).filter((n) => n.startsWith("en")).sort()] which can
include "en0" twice; update the iterable used in the for (const enName of ...)
to dedupe "en0" (e.g., filter out "en0" from the spread or construct a Set) so
"en0" appears only once when iterating over ifaces; adjust the code around the
for loop that references ifaces and enName to use the deduped list.
In `@src/bun/proxy/upstream-forwarder.ts`:
- Around line 31-44: The CWD upward walk that pushes candidates based on
process.cwd() should only run in development; add an explicit isDev() guard
around the loop that walks from process.cwd() so production avoids deriving
paths from the CWD (leaving existing candidate order and bundle-path checks
intact). Specifically, wrap the existing loop that references process.cwd(),
join(dir, "package.json"), candidates, and name with a check like if (isDev()) {
... } so the code path that searches parent directories is skipped in
production.
In `@src/bun/storage/trace-writer.ts`:
- Around line 34-54: The trace record currently omits source_app (stored on
sessions), so update the trace read/query functions (e.g., queryTraces /
getTraces in trace-db.ts) to JOIN the sessions table on trace.session_id and
include sessions.source_app in the SELECT and WHERE clauses so queries can
filter by provider, model, timestamp and source_app; ensure the JOIN uses
trace.session_id = sessions.id and that any pagination/sort logic still uses
trace.timestamp; keep insertTrace unchanged but add necessary tests for
filtering by source_app.
In `@src/bun/system/pf-manager.ts`:
- Around line 42-46: The applyRules function declares unused parameters iface
and gateway; to fix, either remove these parameters from the function signature
and update any callers to stop passing them, or if they were intended to affect
behavior, wire them into the function logic (e.g., use iface to select the
interface and gateway to set route/next-hop) and ensure tests/call sites pass
the appropriate values; reference the applyRules(domains: string[], iface?:
string, gateway?: string) declaration when making the change.
- Around line 117-126: The condition checking commands.length > 0 is redundant
because commands is initialized with one element; remove the array-length guard
and simply invoke executePrivileged with the constructed command for clearing
the PF anchor, or build the commands array only when needed; update the pf-clear
block around ANCHOR_NAME to call executePrivileged(commands.join(" ; ") + " ;
true") directly and keep the existing error handling that logs result.error on
failure (ensure you update the code paths in pf-manager.ts where commands and
executePrivileged are used).
🪄 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: 648e61f6-d805-419a-8aa4-49eb1b54f419
📒 Files selected for processing (20)
AGENTS.mdROADMAP.mdResources/version.jsonnative/Forwarder/agenttap-fwd.cnative/Helper/HelperTool.swiftnative/Makefilesrc/bun/dns/doh-resolver.tssrc/bun/index.tssrc/bun/net/interface-bind.tssrc/bun/net/interface-detector.tssrc/bun/proxy/proxy-server.tssrc/bun/proxy/transparent-proxy.tssrc/bun/proxy/upstream-forwarder.tssrc/bun/sse/sse-reassembler.tssrc/bun/storage/trace-db.tssrc/bun/storage/trace-writer.tssrc/bun/system/helper-client.tssrc/bun/system/pf-manager.tssrc/bun/system/redirect-manager.tssrc/shared/types.ts
[Repo Mapping Lane]Inspected hot spots
Suspicious coupling / regressions
Untouched but still load-bearing
|
|
[Performance Lane] I walked the request path end-to-end (transparent proxy -> DoH -> forwarder -> SSE buffering -> trace writer -> debug API). A few scale blockers stand out:
Fix: resolve on cache miss only, keep a TTL-based memory cache, and refresh stale entries in the background.
Fix: add bounded/truncated capture, incremental SSE parsing/token accounting, and persist either reassembled body or chunk metadata for streams, not both.
Fix: move writes behind a bounded async queue/batcher and wrap the per-trace DB mutations in a single transaction.
Fix: switch to a single-process
Fix: cap Some good foundations here: WAL + |
[Repo Mapping Lane]Inspected hot spots
Suspicious coupling / regression risks
|
|
[Architecture Lane] I reviewed the full capture/control path, with extra focus on the PR’s new DNS-based interception, helper lifecycle, forwarder, proxy data path, and SQLite storage. I specifically inspected Main findings:
Recommendations:
|
|
[DNS/Redirect Trace Lane] I traced the branch end-to-end from provider selection ->
|
|
[Privacy/Data Lane]
Secrets/dependency pass: I did not find newly hardcoded credentials in the PR diff, and |
|
[Storage/API Lane] Reviewed the storage/query path in What I checked:
Findings from this lane are being posted separately to the reviewer summary. |
|
[Proxy Path Lane] Focused review on Findings:
Positives:
|
|
[Privilege Security Lane] Reviewed the privileged helper, helper socket trust boundary, Blocking findings:
Checks completed:
|
|
[Verification Lane] I reviewed this as a verification/completion pass, with focus on the PR’s claims around DNS-based interception, cleanup semantics, provider defaults, and session/cross-process state. Verdict: the DNS-based design is present in code, but the PR’s completion claims are not fully substantiated yet. I would not treat this as merge-ready in its current state. Key findings:
Fresh verification evidence:
Remaining uncertainty:
|
|
[Native Code Lane] Findings after reviewing
Checked items:
|
[Repo Mapping Lane]Hot spots reviewed: Untouched but regression-relevant: Suspicious coupling: Single |
|
[Test Engineering Lane] Highest-risk coverage gaps:
Coverage that looks acceptable:
I did not find any existing automated test suite in the repo, and CI currently runs build-only jobs. |
|
[Critic Lane] Verdict: REJECT This PR improves the happy path, but it also hardens several fragile assumptions into the architecture and introduces at least two release-blocking failures. Critical blockers
Major issues
Residual risks / missing safeguards
What I would require before merging
|
|
[Architecture Lane] I did a full architecture pass over the redirect, helper, proxy/forwarder, DoH, CA, and storage layers in this branch. I found four architecture issues worth fixing before this lands:
Residual risk I inspected but do not consider a blocker for this PR: the proxy core now imports the macOS-only forwarder/DoH strategy directly ( |
|
[Proxy Path Lane] I traced the transparent path end to end:
Positive note: the SNI extraction path itself looked sound in this pass. Buffering the ClientHello in |
[DNS/Redirect Trace Lane]Traced path: provider toggle -> Blocking findings:
Risky transition edge:
What I did verify as coherent:
|
|
[Native Code Lane] I concentrated on the native/helper lane (
Checked items:
Positive notes:
|
|
[Privilege Security Lane] I found security blockers in the privilege boundary and local-control surface.
What I checked:
|
|
[Storage/API Lane] Checked:
Findings:
|
|
[Performance Lane] Reviewed with a performance/scale lens, focusing on request-path overhead, per-request DoH, forwarder lifecycle/readiness, SQLite/WAL behavior, trace buffering, SSE handling, and debug/query APIs. Blocking findings:
Positive notes:
Recommendation: REQUEST CHANGES |
|
[Test Engineering Lane] I did not find adequate automated coverage for any of the requested areas. The closest guard is the existing macOS build workflow, but it still does not compile or bundle the new forwarder, so even packaging/build verification is not yet adequate.
If you want a minimal first-pass test plan, I’d start with: (a) a CI assertion that the app artifact contains |
|
[Privacy/Data Lane] I reviewed the PR with a privacy/data-handling focus: storage, logging, debug endpoints, helper trust boundaries, source-app detection, trace fields, retention assumptions, and accidental leakage paths. Findings:
What I checked beyond the blockers:
From a privacy/data lane, I would block on items 1 and 2 before shipping, and I would want item 3 resolved or explicitly scoped/documented before this feature is enabled for real user traffic. |
Tighten helper access, cleanup, packaging, trace privacy, and forwarder reliability so capture failures do not leave stale state or leak local secrets. Add focused regression tests and CI coverage for session bucketing, SSE parsing, and DoH caching.
Fix the remaining actionable review items around truncated trace completion, DNS watchdog verification, NODE_EXTRA_CA_CERTS restoration, trace filtering, and helper validation. Tighten the debug/query surface and add regression coverage for source-app filtering while leaving low-value docstring churn out of this PR.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/bun/system/pf-manager.ts (1)
106-138:⚠️ Potential issue | 🟠 Major
clearRules()can report success while/etc/hostsis still poisoned.If
helperClient.isReadyis false, this path skipsremoveHostsEntries()entirely and can still returntrueafter only flushing PF. Stopping capture then leaves the sentinel mappings behind and provider domains stay blackholed until a later cleanup. Treat helper unavailability as a failure here, or fall back to a privileged hosts cleanup path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/system/pf-manager.ts` around lines 106 - 138, In clearRules(), ensure /etc/hosts cleanup isn't skipped when helperClient.isReady is false: either treat helper unavailability as a failure (return false and set success=false) or implement a privileged fallback that calls the existing executePrivileged flow to remove the sentinel host entries; update logic around helperClient.removeHostsEntries and the else branch that currently only flushes PF so that removeHostsEntries is invoked (or an equivalent privileged hosts-cleanup) before returning success in clearRules().src/bun/proxy/proxy-server.ts (1)
330-352:⚠️ Potential issue | 🟠 MajorBedrock EventStream responses are still being dropped.
This path marks AWS EventStream as streaming but replaces the body with
""and never attaches raw chunks, so the persisted trace has no usable response payload and token extraction is skipped. If Bedrock is treated as supported, keep the raw frames/chunks until a real decoder exists.
♻️ Duplicate comments (4)
native/Helper/HelperTool.swift (1)
144-165:⚠️ Potential issue | 🟠 Major
verifyAnchorsstill missing anchor allowlist check.This was flagged in a previous review. Unlike
applyPfRulesandclearRules, this function doesn't callHelperTool.isAllowedAnchor(anchorName), allowing any authenticated user to query arbitrary PF anchors.🛡️ Proposed fix
private func verifyAnchors(id: String, params: [String: Any]) -> [String: Any] { guard let anchorName = params["anchorName"] as? String else { return ["id": id, "success": false, "error": "Missing anchorName"] } + + guard HelperTool.isAllowedAnchor(anchorName) else { + return ["id": id, "success": false, "error": "Invalid anchor name"] + } let rdrResult = run("/sbin/pfctl", args: ["-a", anchorName, "-sn"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/Helper/HelperTool.swift` around lines 144 - 165, verifyAnchors currently skips the anchor allowlist check; update HelperTool.verifyAnchors(id:params:) to call HelperTool.isAllowedAnchor(anchorName) after extracting anchorName and before running pfctl, returning ["id": id, "success": false, "error": "Not allowed anchor"] (or similar) when the check fails, and only proceed to run rdr/filter commands and build the success/data response when isAllowedAnchor(anchorName) returns true.src/bun/system/redirect-manager.ts (1)
181-196:⚠️ Potential issue | 🟠 MajorPreserve the user's existing
NODE_EXTRA_CA_CERTS.These helpers still overwrite the launchd value on enable and always
unsetenvit on disable/startup cleanup. That breaks unrelated Node/Bun tooling and makes the rollback path destructive. Save the previous value beforesetenvand restore it instead of blindly unsetting.🤖 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 181 - 196, Change the helpers so they preserve and restore any existing launchd NODE_EXTRA_CA_CERTS instead of unconditionally overwriting/unsetting: in setNodeCATrust(certPath) read the current value via launchctl getenv NODE_EXTRA_CA_CERTS, store that previous value in a module-scoped variable (e.g., prevNodeExtraCACerts), then call launchctl setenv with certPath; in clearNodeCATrust() check prevNodeExtraCACerts—if it is non-empty restore it with launchctl setenv, otherwise call launchctl unsetenv; keep the existing logging and error handling but mention restoration vs unsetting.src/bun/system/pf-manager.ts (1)
141-149:⚠️ Potential issue | 🟠 MajorHealth checks still verify PF only, not DNS interception.
verifyRulesActive()returns healthy onhasRdralone. If the AgentTap block disappears from/etc/hosts, the watchdog andverifyCapture()still report success while interception is already broken. Verify the helper-managed hosts block alongside the anchor state.Based on learnings, "DNS-based traffic interception via
/etc/hosts+ PF rdr on lo0 + DoH upstream resolution + native TCP forwarder withIP_BOUND_IF".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/system/pf-manager.ts` around lines 141 - 149, verifyRulesActive currently treats hasRdr as sufficient; update it to also verify the helper-managed hosts (AgentTap block) so DNS interception is checked. When calling helperClient.verifyAnchors(ANCHOR_NAME) (or a dedicated helperClient.verifyHosts() if available), require both PF anchor state and the hosts-block indicator (e.g., result.data?.hasRdr === true && result.data?.hasHosts === true) before returning true; if the verifyAnchors response doesn't include hosts info, add a follow-up helperClient call to validate the /etc/hosts block presence and include that in the combined boolean result. Ensure error handling remains but return false when either check fails.src/bun/storage/trace-db.ts (1)
261-274:⚠️ Potential issue | 🟠 MajorExpose filtered trace queries, including source app.
The read API still only supports “recent traces”, “trace by id”, and sessions. There is no way to query traces by provider, model, timestamp range, or source app, and
getRecentTraces()does not joinsessions.source_appat all. Add a filtered trace query surface here and have/tracescall it.As per coding guidelines, "Traces must be queryable by provider, model, timestamp, and source app".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/storage/trace-db.ts` around lines 261 - 274, The current API only exposes getRecentTraces, getTraceById and getRecentSessions and does not allow filtering by provider, model, timestamp range or sessions.source_app; add a new exported function (e.g., getTraces(filters)) that builds a parameterized SQL query joining traces to sessions (JOIN sessions ON traces.session_id = sessions.id) to SELECT traces.* plus sessions.source_app and support filters: provider, model, source_app, timestamp_from, timestamp_to, plus limit/offset; ensure it uses getDB().query(...).all(...) and safe parameter binding, update /traces to call this new getTraces function, and either update getRecentTraces to include sessions.source_app or mark it as a thin wrapper around getTraces({limit, offset}) so all trace queries can filter by provider, model, timestamp and source app.
🧹 Nitpick comments (5)
src/bun/dns/doh-resolver.test.ts (1)
32-34: Add a concurrent lookup case forinflightdedupe.Line 32 and Line 33 are sequential, so this verifies cache reuse but not concurrent request coalescing. Add a
Promise.alltest for the same domain.Proposed test addition
test("resolveViaDoH reuses cached answers for repeated lookups", async () => { @@ expect(await resolveViaDoH("api.example.com")).toEqual(["1.2.3.4"]); expect(await resolveViaDoH("api.example.com")).toEqual(["1.2.3.4"]); expect(calls).toBe(1); }); + +test("resolveViaDoH deduplicates concurrent lookups", async () => { + let calls = 0; + + global.fetch = (async (_input: RequestInfo | URL, _init?: RequestInit) => { + calls++; + await new Promise((r) => setTimeout(r, 10)); + return new Response(JSON.stringify({ + Status: 0, + Answer: [{ name: "api.example.com", type: 1, TTL: 60, data: "1.2.3.4" }], + }), { + status: 200, + headers: { "content-type": "application/dns-json" }, + }); + }) as typeof fetch; + + const [a, b] = await Promise.all([ + resolveViaDoH("api.example.com"), + resolveViaDoH("api.example.com"), + ]); + + expect(a).toEqual(["1.2.3.4"]); + expect(b).toEqual(["1.2.3.4"]); + expect(calls).toBe(1); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/dns/doh-resolver.test.ts` around lines 32 - 34, The test currently only exercises sequential cache reuse; add a concurrent lookup case to verify inflight dedupe by invoking two simultaneous resolveViaDoH("api.example.com") calls with Promise.all and asserting both results equal ["1.2.3.4"] and that the underlying request counter calls remains 1 (the test uses the calls variable). Locate the existing expectations around resolveViaDoH and replace or augment the second sequential expect with a Promise.all([...]) call, then assert the returned array values and calls === 1 to confirm coalescing.src/bun/native-bridge.ts (1)
51-56: Directory traversal assumes specific bundle structure.
join(import.meta.dir, "..", "..", "..")works ifimport.meta.diris atContents/Resources/app/.... If the bundle structure changes or code moves deeper, this breaks silently. Consider a more robust approach.💡 More robust alternative
private getAppContentsDir(): string | null { const idx = import.meta.dir.indexOf(".app/Contents/"); if (idx === -1) return null; return import.meta.dir.substring(0, idx + ".app/Contents".length); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/native-bridge.ts` around lines 51 - 56, The current getAppContentsDir method assumes a fixed depth by using join(import.meta.dir, "..", "..", ".."), which can break if the bundle layout changes; update getAppContentsDir to locate the ".app/Contents" substring inside import.meta.dir (e.g., using indexOf or lastIndexOf) and return the path up through ".app/Contents" (or null if not found) rather than relying on relative ../ traversal, ensuring you reference the getAppContentsDir function and import.meta.dir when making the change.src/bun/storage/trace-db.test.ts (1)
29-43: Consider adding a test for session expiry after gap.Current tests verify reuse within the gap, but there's no test confirming a new session is created after the gap expires (e.g.,
timestamp + SESSION_GAP_MS + 1000).💡 Additional test case
test("getOrCreateSession creates new session after gap expires", () => { const provider = `openai-${crypto.randomUUID()}`; const timestamp = Date.now(); const first = getOrCreateSession(provider, timestamp, "Cursor"); const afterGap = getOrCreateSession( provider, timestamp + SESSION_GAP_MS + 1000, "Cursor", ); expect(afterGap).not.toBe(first); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/storage/trace-db.test.ts` around lines 29 - 43, Add a test that verifies getOrCreateSession creates a new session when the gap expires: call getOrCreateSession(provider, timestamp, "Cursor") to get first, then call getOrCreateSession(provider, timestamp + SESSION_GAP_MS + 1000, "Cursor") and assert the second result is not the same object as first (expect(afterGap).not.toBe(first)); use the same provider generation pattern (e.g., ``openai-${crypto.randomUUID()}``) and Date.now() for timestamp to mirror existing tests.native/Forwarder/agenttap-fwd.c (1)
258-265: Fork-per-connection limits scalability.Each tunnel spawns a new process. Under load with many concurrent API requests, this becomes expensive. Consider a thread pool or async I/O model for production use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/Forwarder/agenttap-fwd.c` around lines 258 - 265, The current fork-per-connection model (pid = fork() with handle_client(client)) is not scalable; replace it with a pooled worker thread model or async I/O to avoid spawning a process per request. Implement a fixed-size pthread pool that accepts client sockets into a concurrent queue and have worker threads call handle_client(client) (or convert handle_client to be thread-safe), or refactor to non-blocking sockets with an event loop (epoll/select) that dispatches read/write to async handlers; ensure you close descriptors in the parent/worker appropriately and protect any shared state used by handle_client with mutexes.native/Helper/HelperTool.swift (1)
304-307: Hardcoded cert name "AgentTap CA" for deletion.
delete-certificate -c "AgentTap CA"assumes a specific common name. If the CA cert's CN differs, deletion silently fails. This is acceptable since the function returns success anyway with a warning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/Helper/HelperTool.swift` around lines 304 - 307, The code currently deletes a hardcoded certificate name via the run(...) call using "delete-certificate -c 'AgentTap CA'", which can silently fail if the CA CN differs; update the logic in HelperTool.swift (the function that constructs `result = run("/usr/bin/security", args: [...])`) to first locate the exact certificate identifier (SHA-1 or SHA-256 hash) or accept the certificate common name as a parameter, e.g., call `security find-certificate -a -Z "/Library/Keychains/System.keychain"` and parse the matching certificate's SHA hash, then call `security delete-certificate -Z <SHA>` (or use the provided CN parameter) instead of always using "AgentTap CA", and propagate/report errors when deletion fails rather than returning success with only a warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@native/Forwarder/agenttap-fwd.c`:
- Line 195: The handshake write call write(client_fd, "OK\n", 3) is not checked
so a failed send could let the code call relay() with a dead client; update the
handshake logic in agenttap-fwd.c to check the return value (and errno) from
write(), handle short/failed writes by closing client_fd and returning/aborting
the path instead of calling relay(), and log an error (or return a non-zero
status) when write returns <= 0; focus changes around the function that performs
the handshake and the subsequent call to relay() so you adjust behavior when
write fails.
In `@src/bun/storage/trace-writer.ts`:
- Around line 58-75: The trace uses different timestamps for session bucketing
and storage: getOrCreateSession(...) is called with completedAt
(trace.request.timestamp + trace.durationMs) but insertTrace(...) writes
timestamp: new Date(trace.request.timestamp).toISOString(), which can make
sessions end before they start; fix by using the same completion timestamp for
both paths—compute completedAtDate = new Date(completedAt).toISOString() (or
pass completedAt as a Date) and use that value for insertTrace.timestamp (or
alternatively change getOrCreateSession to accept the request timestamp and
separately update the session end with completedAt); ensure getOrCreateSession,
insertTrace and any sessions.ended_at update use the same completedAt basis.
- Around line 55-96: The listener in startTraceWriter currently performs
getOrCreateSession and insertTrace synchronously on each "trace" event (blocking
traceEmitter.emit); change it to only enqueue the incoming CapturedTrace into an
in-memory queue and return immediately, and implement a separate async worker
(e.g., a batched flush loop using setImmediate/setInterval or a background
Promise) that dequeues traces, calls getOrCreateSession and insertTrace off the
emit path, and retries/logs errors; reference startTraceWriter, traceEmitter,
CapturedTrace, getOrCreateSession and insertTrace when moving logic so the
listener becomes a fast enqueue-only function and all DB/session work runs in
the background with batching and error handling.
---
Outside diff comments:
In `@src/bun/system/pf-manager.ts`:
- Around line 106-138: In clearRules(), ensure /etc/hosts cleanup isn't skipped
when helperClient.isReady is false: either treat helper unavailability as a
failure (return false and set success=false) or implement a privileged fallback
that calls the existing executePrivileged flow to remove the sentinel host
entries; update logic around helperClient.removeHostsEntries and the else branch
that currently only flushes PF so that removeHostsEntries is invoked (or an
equivalent privileged hosts-cleanup) before returning success in clearRules().
---
Duplicate comments:
In `@native/Helper/HelperTool.swift`:
- Around line 144-165: verifyAnchors currently skips the anchor allowlist check;
update HelperTool.verifyAnchors(id:params:) to call
HelperTool.isAllowedAnchor(anchorName) after extracting anchorName and before
running pfctl, returning ["id": id, "success": false, "error": "Not allowed
anchor"] (or similar) when the check fails, and only proceed to run rdr/filter
commands and build the success/data response when isAllowedAnchor(anchorName)
returns true.
In `@src/bun/storage/trace-db.ts`:
- Around line 261-274: The current API only exposes getRecentTraces,
getTraceById and getRecentSessions and does not allow filtering by provider,
model, timestamp range or sessions.source_app; add a new exported function
(e.g., getTraces(filters)) that builds a parameterized SQL query joining traces
to sessions (JOIN sessions ON traces.session_id = sessions.id) to SELECT
traces.* plus sessions.source_app and support filters: provider, model,
source_app, timestamp_from, timestamp_to, plus limit/offset; ensure it uses
getDB().query(...).all(...) and safe parameter binding, update /traces to call
this new getTraces function, and either update getRecentTraces to include
sessions.source_app or mark it as a thin wrapper around getTraces({limit,
offset}) so all trace queries can filter by provider, model, timestamp and
source app.
In `@src/bun/system/pf-manager.ts`:
- Around line 141-149: verifyRulesActive currently treats hasRdr as sufficient;
update it to also verify the helper-managed hosts (AgentTap block) so DNS
interception is checked. When calling helperClient.verifyAnchors(ANCHOR_NAME)
(or a dedicated helperClient.verifyHosts() if available), require both PF anchor
state and the hosts-block indicator (e.g., result.data?.hasRdr === true &&
result.data?.hasHosts === true) before returning true; if the verifyAnchors
response doesn't include hosts info, add a follow-up helperClient call to
validate the /etc/hosts block presence and include that in the combined boolean
result. Ensure error handling remains but return false when either check fails.
In `@src/bun/system/redirect-manager.ts`:
- Around line 181-196: Change the helpers so they preserve and restore any
existing launchd NODE_EXTRA_CA_CERTS instead of unconditionally
overwriting/unsetting: in setNodeCATrust(certPath) read the current value via
launchctl getenv NODE_EXTRA_CA_CERTS, store that previous value in a
module-scoped variable (e.g., prevNodeExtraCACerts), then call launchctl setenv
with certPath; in clearNodeCATrust() check prevNodeExtraCACerts—if it is
non-empty restore it with launchctl setenv, otherwise call launchctl unsetenv;
keep the existing logging and error handling but mention restoration vs
unsetting.
---
Nitpick comments:
In `@native/Forwarder/agenttap-fwd.c`:
- Around line 258-265: The current fork-per-connection model (pid = fork() with
handle_client(client)) is not scalable; replace it with a pooled worker thread
model or async I/O to avoid spawning a process per request. Implement a
fixed-size pthread pool that accepts client sockets into a concurrent queue and
have worker threads call handle_client(client) (or convert handle_client to be
thread-safe), or refactor to non-blocking sockets with an event loop
(epoll/select) that dispatches read/write to async handlers; ensure you close
descriptors in the parent/worker appropriately and protect any shared state used
by handle_client with mutexes.
In `@native/Helper/HelperTool.swift`:
- Around line 304-307: The code currently deletes a hardcoded certificate name
via the run(...) call using "delete-certificate -c 'AgentTap CA'", which can
silently fail if the CA CN differs; update the logic in HelperTool.swift (the
function that constructs `result = run("/usr/bin/security", args: [...])`) to
first locate the exact certificate identifier (SHA-1 or SHA-256 hash) or accept
the certificate common name as a parameter, e.g., call `security
find-certificate -a -Z "/Library/Keychains/System.keychain"` and parse the
matching certificate's SHA hash, then call `security delete-certificate -Z
<SHA>` (or use the provided CN parameter) instead of always using "AgentTap CA",
and propagate/report errors when deletion fails rather than returning success
with only a warning.
In `@src/bun/dns/doh-resolver.test.ts`:
- Around line 32-34: The test currently only exercises sequential cache reuse;
add a concurrent lookup case to verify inflight dedupe by invoking two
simultaneous resolveViaDoH("api.example.com") calls with Promise.all and
asserting both results equal ["1.2.3.4"] and that the underlying request counter
calls remains 1 (the test uses the calls variable). Locate the existing
expectations around resolveViaDoH and replace or augment the second sequential
expect with a Promise.all([...]) call, then assert the returned array values and
calls === 1 to confirm coalescing.
In `@src/bun/native-bridge.ts`:
- Around line 51-56: The current getAppContentsDir method assumes a fixed depth
by using join(import.meta.dir, "..", "..", ".."), which can break if the bundle
layout changes; update getAppContentsDir to locate the ".app/Contents" substring
inside import.meta.dir (e.g., using indexOf or lastIndexOf) and return the path
up through ".app/Contents" (or null if not found) rather than relying on
relative ../ traversal, ensuring you reference the getAppContentsDir function
and import.meta.dir when making the change.
In `@src/bun/storage/trace-db.test.ts`:
- Around line 29-43: Add a test that verifies getOrCreateSession creates a new
session when the gap expires: call getOrCreateSession(provider, timestamp,
"Cursor") to get first, then call getOrCreateSession(provider, timestamp +
SESSION_GAP_MS + 1000, "Cursor") and assert the second result is not the same
object as first (expect(afterGap).not.toBe(first)); use the same provider
generation pattern (e.g., ``openai-${crypto.randomUUID()}``) and Date.now() for
timestamp to mirror existing tests.
🪄 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: ba0178a8-3201-4108-8ee5-1bb384022bea
📒 Files selected for processing (26)
.github/workflows/build.yml.github/workflows/release.ymlAGENTS.mdResources/version.jsonelectrobun.config.tsnative/Forwarder/agenttap-fwd.cnative/Helper/HelperTool.swiftnative/Makefilepackage.jsonsrc/bun/ca/ca-manager.tssrc/bun/dns/doh-resolver.test.tssrc/bun/dns/doh-resolver.tssrc/bun/index.tssrc/bun/native-bridge.tssrc/bun/proxy/proxy-server.tssrc/bun/proxy/transparent-proxy.tssrc/bun/proxy/upstream-forwarder.tssrc/bun/sse/sse-reassembler.test.tssrc/bun/sse/sse-reassembler.tssrc/bun/storage/trace-db.test.tssrc/bun/storage/trace-db.tssrc/bun/storage/trace-writer.tssrc/bun/system/helper-client.tssrc/bun/system/pf-manager.tssrc/bun/system/redirect-manager.tssrc/shared/types.ts
✅ Files skipped from review due to trivial changes (6)
- Resources/version.json
- src/bun/ca/ca-manager.ts
- package.json
- src/bun/sse/sse-reassembler.test.ts
- src/shared/types.ts
- electrobun.config.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/bun/proxy/transparent-proxy.ts
- src/bun/dns/doh-resolver.ts
- src/bun/system/helper-client.ts
- src/bun/proxy/upstream-forwarder.ts
| freeaddrinfo(res); | ||
|
|
||
| /* Signal success — client can now start TLS over this tunnel */ | ||
| write(client_fd, "OK\n", 3); |
There was a problem hiding this comment.
Check write() return value for handshake response.
If write(client_fd, "OK\n", 3) fails (e.g., client disconnected), the code proceeds to relay() with a potentially broken connection. Should check the return value before continuing.
🛠️ Proposed fix
- write(client_fd, "OK\n", 3);
+ if (write(client_fd, "OK\n", 3) != 3) {
+ close(upstream);
+ goto done;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@native/Forwarder/agenttap-fwd.c` at line 195, The handshake write call
write(client_fd, "OK\n", 3) is not checked so a failed send could let the code
call relay() with a dead client; update the handshake logic in agenttap-fwd.c to
check the return value (and errno) from write(), handle short/failed writes by
closing client_fd and returning/aborting the path instead of calling relay(),
and log an error (or return a non-zero status) when write returns <= 0; focus
changes around the function that performs the handshake and the subsequent call
to relay() so you adjust behavior when write fails.
| export function startTraceWriter(): void { | ||
| traceEmitter.on("trace", (trace: CapturedTrace) => { | ||
| try { | ||
| const sourceApp = detectSourceApp(trace.request.headers); | ||
| const completedAt = trace.request.timestamp + trace.durationMs; | ||
| const safeRequestUrl = sanitizeTraceUrl(trace.request.url); | ||
| const requestHeaders = redactHeaders(trace.request.headers); | ||
| const responseHeaders = redactHeaders(trace.response.headers); | ||
| const sessionId = getOrCreateSession( | ||
| trace.provider, | ||
| completedAt, | ||
| sourceApp, | ||
| ); | ||
|
|
||
| insertTrace({ | ||
| id: trace.id, | ||
| session_id: sessionId, | ||
| provider: trace.provider, | ||
| domain: trace.domain, | ||
| timestamp: new Date(trace.request.timestamp).toISOString(), | ||
| model: trace.model ?? null, | ||
| request_method: trace.request.method, | ||
| request_url: safeRequestUrl, | ||
| request_headers: JSON.stringify(requestHeaders), | ||
| request_body: trace.request.body, | ||
| response_status: trace.response.status, | ||
| response_headers: JSON.stringify(responseHeaders), | ||
| response_body: trace.response.isStreaming ? null : trace.response.body, | ||
| response_chunks: trace.response.chunks ? JSON.stringify(trace.response.chunks) : null, | ||
| is_streaming: trace.response.isStreaming ? 1 : 0, | ||
| tokens_in: trace.tokensIn ?? null, | ||
| tokens_out: trace.tokensOut ?? null, | ||
| duration_ms: trace.durationMs, | ||
| error: null, | ||
| }); | ||
| } catch (err) { | ||
| console.error("[TraceWriter] Failed to write trace:", err); | ||
| } | ||
| }); | ||
|
|
||
| console.log("[TraceWriter] Listening for traces"); | ||
| } |
There was a problem hiding this comment.
Move trace persistence off the emit path.
This listener does getOrCreateSession() and insertTrace() inline for every trace event. Since traceEmitter.emit() is synchronous, the proxy response path is now blocked on SQLite writes and session aggregate updates. Queue traces and flush them asynchronously instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun/storage/trace-writer.ts` around lines 55 - 96, The listener in
startTraceWriter currently performs getOrCreateSession and insertTrace
synchronously on each "trace" event (blocking traceEmitter.emit); change it to
only enqueue the incoming CapturedTrace into an in-memory queue and return
immediately, and implement a separate async worker (e.g., a batched flush loop
using setImmediate/setInterval or a background Promise) that dequeues traces,
calls getOrCreateSession and insertTrace off the emit path, and retries/logs
errors; reference startTraceWriter, traceEmitter, CapturedTrace,
getOrCreateSession and insertTrace when moving logic so the listener becomes a
fast enqueue-only function and all DB/session work runs in the background with
batching and error handling.
…vailable When the privileged helper wasn't ready, clearRules() would skip removing sentinel /etc/hosts entries but still return success=true. This left 127.0.1.x entries that blackhole traffic to intercepted domains. Now properly reports failure when helper is unavailable and there are active entries to clean up.
- Move full app state behind auth (new /state endpoint), /status now returns only health check info without requiring token - Remove double-await dead code in privilege-executor.ts - Clear activeSessions/insertStmt on DB close to prevent stale state - Use completion timestamp (not request start) for trace rows to match session bucketing - Remove dead dns-cache.ts (superseded by DoH resolver's built-in cache) - Remove unused getRecentTraces export
1 participant
Summary
/etc/hostssentinel IPs + DoH resolution + native TCP forwarder (IP_BOUND_IF). Solves AWS Bedrock and Cursor rotating IP issues — DNS interception catches all traffic regardless of IP rotation.agenttap-fwd): connects viaIP_BOUND_IFon physical interface (en0), bypassing lo0 entirely. Universal binary (arm64 + x86_64)./etc/hostsentries (# BEGIN/END AgentTapblock) + DNS cache flush viadscacheutil -flushcache+killall -HUP mDNSResponder./etc/hostsfor real upstream IP resolution.api2.cursor.sh), both enabled by default./traces,/traces/:id,/sessions,/statsfor querying stored traces.Phases covered
Architecture change
Test plan
cd native && make build-helper && make build-fwdbunx electrobun devcurl localhost:19876/start/etc/hostsentries added for enabled providerscurl -sk https://api.anthropic.com/v1/messagesand confirm trace capturedcurl localhost:19876/tracescurl localhost:19876/sessionscurl localhost:19876/stats/etc/hostsentries cleaned uplsof -i :8443 -i :18443Summary by CodeRabbit
Release Notes
New Features
Bug Fixes / Reliability
Documentation