feat: log the caller's peer address on register_session under DEV_MODE (#145) by evansenter · Pull Request #147 · evansenter/agent-event-bus

evansenter · 2026-08-10T18:11:49Z

Closes #145 (the instrumentation half — the issue stays open until the process is named).

What

register_session's log line now carries the caller's peer address and port, under DEV_MODE:

register_session(name="evansenter", cwd="$HOME") → session=brave-trex from 127.0.0.1:54321

Then, while the churn is live:

lsof -i :54321

The port is the identifying half. The bus is loopback/tailnet, so the address is nearly always 127.0.0.1 and says nothing; the ephemeral port is what maps back to a PID. It comes from the ASGI scope (scope["client"]), so no new dependency.

Deliberately narrow

This is instrumentation for one diagnosis, not a permanent log line:

Gated on DEV_MODE, off by default. A bus serving real sessions must not start recording a peer for every registration forever.
register_session only. The churn arrives as register/unregister pairs and the registration alone names the process. get_events runs every few seconds per session; a peer on each would drown the log the diagnosis is being read from.
Appended to the existing line, not logged separately, so the peer arrives already paired with the session_id that call minted — matching a port to a PID is useless if you can't tell which registration it belongs to.

Edges

A scope with no client (unix socket, or an ASGI server that omits it) logs from unknown peer rather than dropping the suffix — the silent version would read as "DEV_MODE isn't on" to an operator who just turned it on. A malformed client value takes the same path instead of raising inside the logging path.

The peer is read in __call__ because the scope is the only place it exists, and _log_tool_call — which runs in a worker thread per the #112 invariant — never sees it.

Tests

Five new tests in TestPeerLogging, each mutation-tested:

Mutation	Tests killed
Remove the `DEV_MODE` gate	`test_nothing_is_logged_without_dev_mode`
Log the peer for every tool	`test_other_tools_stay_quiet_even_under_dev_mode`
Return `None` instead of `"unknown peer"`	`test_missing_peer_is_named_rather_than_dropped`, `test_malformed_client_does_not_break_logging`
Remove the suffix entirely	`test_register_session_logs_the_peer_under_dev_mode` + the two above

make check clean: 654 passed, ruff format + lint pass.

Follow-up

Once the process is named, the fix is likely on its side (register once and keep the session alive) or it just needs killing. The pid + start-timestamp addition to /health proposed off #139 would make this class of question answerable without lsof at all — not included here, since it's a separate surface.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MbSWrbyPaRCrYmyMf4KzsS

Generated by Claude Code

#145) Something on the bus host registers and immediately unregisters a session ~1.5x/min, minting ~2,000 rows/day that no client will ever poll. The usual suspects are ruled out, and register_session records nothing about WHERE the call came from, so there is no way to name the process. Log the peer address and port on the register_session line. The port is the identifying half - the bus is loopback/tailnet, so the address is nearly always 127.0.0.1 and says nothing, while the ephemeral port is what `lsof -i :PORT` maps back to a PID while the churn is live. It comes from the ASGI scope (scope["client"]), so no new dependency. Deliberately narrow, because this is instrumentation for one diagnosis rather than a permanent log line: - Gated on DEV_MODE. Off by default, so a bus serving real sessions does not start recording a peer for every registration forever. - register_session only. The churn arrives as register/unregister pairs and the registration alone names the process; get_events runs every few seconds per session and a peer on each would drown the log the diagnosis is being read from. - Appended to the existing line rather than logged separately, so the peer arrives already paired with the session_id that call minted - matching a port to a PID is useless if you cannot tell which registration it belongs to. A scope with no client (unix socket, or an ASGI server that omits it) logs "from unknown peer" rather than dropping the suffix, which would read as "DEV_MODE isn't on" to an operator who just turned it on. The peer is read in __call__ because the scope is the only place it exists and _log_tool_call, which runs in the worker thread per #112, never sees it. Each of the five tests was mutation-tested: removing the gate, widening past register_session, dropping the unknown-peer label, and removing the suffix entirely each fail at least one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MbSWrbyPaRCrYmyMf4KzsS

claude · 2026-08-10T18:16:22Z

+    does not start recording a peer for every registration forever.
+
+    The PORT is the identifying half. The bus is loopback or tailnet, so the
+    address is nearly always 127.0.0.1 and says nothing; the ephemeral port is


[Suggestion] Worth one line here (and in the CLAUDE.md block) on the tailscale serve case. TailscaleAuthMiddleware exists because remote clients arrive through a Tailscale reverse proxy, and for those requests scope["client"] is the local tailscaled socket — so a remote culprit logs as 127.0.0.1:<tailscaled ephemeral port> and lsof -i :PORT names tailscaled rather than the caller. That is indistinguishable from a genuinely local caller in the line as written, and the docstring reading "the address is nearly always 127.0.0.1 and says nothing" is exactly what would hide it.

The distinguisher is already in the scope: proxied requests carry Tailscale-User-Login, which is why middleware.py:356 bypasses auth only for 127.0.0.1 and ::1. Even just documenting "if the peer is loopback but the request was Tailscale-authenticated, the port belongs to tailscaled" would keep the operator from eliminating the wrong candidates. The evidence in #145 points at a local process, so this is a caveat rather than a gap.

claude · 2026-08-10T18:16:32Z

+    flipping it in a test does not depend on import order. An operator flips
+    it by restarting the server with DEV_MODE=1 either way.
+    """
+    if not os.environ.get("DEV_MODE"):


[Suggestion] DEV_MODE is not a quiet switch on the host this is meant to be used on. helpers.py:177 fires send_notification() for every register_session, list_sessions, publish_event, and the get_events resume paths whenever DEV_MODE is set, and server.py:67 drops the logger to DEBUG. So the operator who restarts the bus with DEV_MODE=1 to read one port also gets a desktop notification per registration (~1.5/min from the churn alone, plus every real session that touches the bus) for as long as the diagnosis runs.

Issue #145 floated AGENT_EVENT_BUS_LOG_PEER as an alternative gate; a dedicated variable (or accepting either name) would let peer logging be turned on by itself. Non-blocking — the current gate works, and it is the first option the issue named.

claude · 2026-08-10T18:16:36Z

+    # so rather than dropping the suffix, which would read as "not enabled"
+    # to an operator who just turned this on.
+    client = scope.get("client")
+    if not client:


[Suggestion] Note only, no change requested here: the falsy-client branch is well motivated, but the scope that would reach it cannot get this far while auth is on. TailscaleAuthMiddleware.__call__ does scope.get("client", ("", 0))[0] at middleware.py:355, and a scope carrying an explicit client: None (the uvicorn unix-socket case) subscripts None and raises TypeError before RequestLoggingMiddleware is entered. So from unknown peer is reachable in practice only under AGENT_EVENT_BUS_AUTH_DISABLED=1, which is roughly what test_missing_peer_is_named_rather_than_dropped models.

The defensive handling is still right here; if the unix-socket deployment ever becomes real, middleware.py:355 is the line that needs the same treatment, e.g. (scope.get("client") or ("", 0))[0].

claude · 2026-08-10T18:16:44Z

+                )
            else:
-                logger.info(f"{caller_prefix}{tool_colored}() {arrow} {result_str}")
+                logger.info(f"{caller_prefix}{tool_colored}() {arrow} {result_str}{peer_suffix}")


[Suggestion] The no-args branch of the peer suffix is untested. All five TestPeerLogging cases send arguments={"name": "evansenter"}, so args_str is always truthy and only the branch above ever runs — deleting {peer_suffix} from this line leaves the suite green. The branch is reachable for a register_session call whose only argument is session_id, since that key is filtered out into caller_prefix and leaves args_without_session empty.

One extra _run(...) case with empty arguments, or parametrizing the happy-path test over both shapes, would close it.

claude

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review — Round 1

Summary

Narrow, well-scoped instrumentation for issue #145. The peer is read in __call__ (the only place the ASGI scope exists, and correctly off the worker thread per the #112 invariant), gated on DEV_MODE, limited to register_session via PEER_LOGGED_TOOLS, and appended to the same line as the session_id that call minted. With DEV_MODE unset the path is a pure no-op: peer stays None, peer_suffix stays empty, and both logger.info branches render exactly as before. Missing and malformed client values both degrade to from unknown peer rather than raising inside the logging path. TestPeerLogging covers the gate, the tool scoping, and both degraded-peer paths, and the CLAUDE.md block documents why the port rather than the address is the identifying half.

Findings

Four suggestions posted inline, none blocking:

middleware.py:425 — DEV_MODE also switches on _dev_notify (a desktop notification per tool call) and DEBUG logging on the very host being diagnosed; issue #145 floated AGENT_EVENT_BUS_LOG_PEER as a quieter gate.
middleware.py:418 — behind tailscale serve, scope["client"] is the local tailscaled socket, so a remote caller logs as loopback and lsof names tailscaled; worth a documented caveat.
middleware.py:431 — note that a client: None scope raises in TailscaleAuthMiddleware at middleware.py:355 before reaching this branch, so unknown peer is reachable only with auth disabled.
middleware.py:556 — the no-args logger.info branch never sees peer_suffix in the tests; removing it there leaves the suite green.

Verdict

APPROVE - No blocking findings.

Automated review by Claude Code

…ted branch Three of the four findings from the automated review, each verified against the code first. 1. `tailscale serve` made the label misleading (middleware.py:418). A proxied request is terminated by the LOCAL tailscaled and forwarded here, so scope["client"] is tailscaled's socket: `lsof -i :PORT` names tailscaled while the real caller is somewhere on the tailnet. Unmarked, that is byte-identical to a genuinely local caller, and the old docstring ("the address is nearly always 127.0.0.1 and says nothing") is exactly what would have an operator eliminate every remote candidate. Rather than only documenting it, the distinguisher already in the scope - the identity header TailscaleAuthMiddleware authenticates on - now marks the line: `from 127.0.0.1:54321 via tailscale`. 2. DEV_MODE is not a quiet switch (middleware.py:425). It also fires _dev_notify (a desktop notification per tool call) and drops the logger to DEBUG, so an operator who flips it on the bus host to read one port takes ~1.5 notifications/min from the churn alone, plus every real session, for as long as the diagnosis runs. AGENT_EVENT_BUS_LOG_PEER - the gate #145 named first - now turns peer logging on by itself. DEV_MODE still works, so nothing that relied on it changes. 3. The no-args logger.info branch never saw the peer suffix in tests (middleware.py:556): every case sent a `name` argument, so only the other branch ran and deleting the suffix there left the suite green. The branch is reachable - session_id is filtered out into the caller prefix, so a call carrying only that renders through it. Now covered. The fourth finding was a note rather than a request, but it named a real latent crash: TailscaleAuthMiddleware did `scope.get("client", ("", 0))[0]`, which subscripts None for a unix-socket scope carrying an explicit client=None - a 500 on every request before any handler ran. `or` instead of a .get default; "" is not in TRUSTED_IPS, so such a request falls through to the header check like any other untrusted peer. Left half-hardened, this module would have had the defensive branch in the new code and the crash in the old. Five new tests, each mutation-tested: dropping the suffix from the no-args branch, removing the LOG_PEER gate, removing the tailscale marker, applying it unconditionally, and reverting the null-client fix each fail exactly one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MbSWrbyPaRCrYmyMf4KzsS

evansenter · 2026-08-10T18:21:05Z

Addressed all four in 2de0c64. Verified each against the code before acting.

middleware.py:418 — tailscale serve. Correct, and worth more than a docstring line: the distinguisher is already in the scope, so the label now carries it rather than relying on the operator remembering the caveat.

register_session(name="x") → session=brave-trex from 127.0.0.1:54321 via tailscale

An unmarked loopback peer is a genuinely local process; a marked one means the port belongs to tailscaled and the caller is somewhere on the tailnet. The old docstring line ("the address is nearly always 127.0.0.1 and says nothing") was the part most likely to cause the wrong elimination, so it's rewritten to say the port identifies a direct connection.

middleware.py:425 — DEV_MODE isn't quiet. Confirmed at helpers.py:177 and server.py:67. AGENT_EVENT_BUS_LOG_PEER=1 now enables peer logging on its own; DEV_MODE still works, so nothing that relied on it changes. Documented in CLAUDE.md as the one to prefer for a live diagnosis, with the notification-rate reasoning.

middleware.py:556 — untested no-args branch. This is the failure mode worth catching: the branch was reachable (a call whose only argument is session_id renders through it, since that key is filtered into the caller prefix) and deleting the suffix there left the suite green. Covered now, and the mutation confirms it fails.

middleware.py:431 — note on reachability. You marked it as no change requested, but the mechanism you described is a real latent crash, so I took it: scope.get("client", ("", 0))[0] subscripts None for an explicit client=None, which is a 500 on every request before any handler runs. Now (scope.get("client") or ("", 0))[0]. "" isn't in TRUSTED_IPS, so such a request falls through to the header check like any other untrusted peer — no behaviour change for scopes that reach it today. Leaving it would have put the defensive branch in the new code and the crash in the old one, in the same module.

Five new tests, each mutation-tested — dropping the suffix from the no-args branch, removing the LOG_PEER gate, removing the tailscale marker, applying it unconditionally, and reverting the null-client fix each fail exactly one test. Suite: 659 passed, format + lint clean.

Generated by Claude Code

#141 landed on main while this was open. One conflict, in CLAUDE.md's environment-variable list: main expanded it to document _BRIDGE_LOG / _BRIDGE_ERR as install-time-only, this branch added _LOG_PEER to the bus list. Kept both - main's expanded clause with _LOG_PEER inserted. No source overlap: #141 touched bridge.py, the plists and the installer, none of which this branch goes near. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MbSWrbyPaRCrYmyMf4KzsS

claude · 2026-08-10T18:29:02Z

+    # Tailscale injects is the only thing in the scope that separates them -
+    # the same header TailscaleAuthMiddleware authenticates on.
+    headers = dict(scope.get("headers") or [])
+    if TailscaleAuthMiddleware.TAILSCALE_USER_HEADER in headers:


[Suggestion] The marker keys on header presence, while TailscaleAuthMiddleware keys on a non-empty value (tailscale_user = headers.get(...) / if not tailscale_user at lines 367-369). A request carrying tailscale-user-login: with an empty value would be labelled via tailscale here but treated as having no identity there — and since loopback bypasses auth entirely, any local process can set that header itself and get marked as proxied. Given the marker exists precisely to stop an operator eliminating the wrong set of candidates, if headers.get(TailscaleAuthMiddleware.TAILSCALE_USER_HEADER): would make the two agree on what counts as a Tailscale identity.

claude · 2026-08-10T18:29:09Z


-    def _log_tool_call(self, request_body: bytes, response_body: bytes) -> None:
-        """Parse and log one MCP tool call (runs in a worker thread)."""
+        `peer` is None unless DEV_MODE is on (#145); see _peer_label.


[Suggestion] Stale after the AGENT_EVENT_BUS_LOG_PEER addition — peer is now also non-None when only that variable is set. Something like "peer is None unless peer logging is on (#145); see _peer_logging_enabled" keeps it accurate without naming both switches in two places.

claude · 2026-08-10T18:29:15Z

        )


+# Tools whose log line carries the caller's peer address under DEV_MODE


[Suggestion] Same staleness as the _log_tool_call docstring: "under DEV_MODE" is now one of two switches. "under peer logging (#145)" would cover both.

claude · 2026-08-10T18:29:21Z

+# `from 127.0.0.1:54321 via tailscale` (Tailscale's identity header is the
+# only thing in the ASGI scope that distinguishes them). An unmarked
+# loopback peer is a genuinely local process.
+AGENT_EVENT_BUS_LOG_PEER=1 agent-event-bus


[Suggestion] The documented form is a foreground invocation, but the host with the #145 churn is presumably the launchd-supervised bus, and scripts/com.evansenter.agent-event-bus.plist only templates PATH, PYTHONPATH, AGENT_EVENT_BUS_ICON, _LOG, and _ERR — there is no DEV_MODE or _LOG_PEER entry, so neither switch reaches the installed server without hand-editing the plist and reloading. Worth one line saying so. This is pre-existing for DEV_MODE, but this block is the first place a reader is told to turn one of them on for a live diagnosis.

claude

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review — Round 2

Summary

All four round-1 items are addressed, and two went further than the note asked: the tailscale serve caveat became a real via tailscale marker derived from the scope rather than a docstring warning, and the latent scope.get("client", ("", 0))[0] crash on an explicit client=None is fixed, with a test pinning the fall-through to the header check. AGENT_EVENT_BUS_LOG_PEER gives the instrumentation its own switch without changing what DEV_MODE already did, and the previously-untested no-args logger.info branch now carries the suffix under a test that fails when it is removed. Gating is still a per-call os.environ.get, matching helpers.py:179 and server.py:67; _peer_label returns None before touching the scope when neither switch is set, so the disabled path stays a no-op and both logger.info branches render exactly as before. PEER_LOGGED_TOOLS still scopes the suffix to register_session, and both degraded-peer paths still resolve to from unknown peer rather than raising inside the logging path.

Previously Addressed (Filtered)

All four round-1 findings — DEV_MODE not being a quiet switch, the tailscale serve caveat, the client=None reachability note, and the untested no-args branch — are Implemented in 2de0c64 and are not re-raised.

Findings

Four suggestions posted inline, none blocking: a presence-vs-non-empty mismatch between the new via tailscale marker and the header check TailscaleAuthMiddleware itself performs, two docstrings that still say DEV_MODE where there are now two switches, and an operational note that neither switch is templated into the launchd plist — which is where the churn being diagnosed presumably lives.

Verdict

APPROVE - No blocking findings.

Automated review by Claude Code

1. The `via tailscale` marker keyed on header PRESENCE while TailscaleAuthMiddleware keys on a non-empty VALUE (`if not tailscale_user`), so a request carrying an empty tailscale-user-login would be labelled proxied here and rejected as anonymous there. The two now agree. Also spelled out what the marker is NOT: loopback bypasses auth, so a local process can set the header itself - it narrows the candidate set, it does not authenticate anything. 2. Two comments still said "under DEV_MODE" after AGENT_EVENT_BUS_LOG_PEER became a second switch. Both now say "peer logging" and point at _peer_logging_enabled, so the switch list lives in one place. 3. The CLAUDE.md block tells an operator to turn this on for a live diagnosis, but the form documented is a FOREGROUND invocation and scripts/com.evansenter.agent-event-bus.plist templates only PATH, PYTHONPATH, _ICON, _LOG and _ERR - so neither switch reaches the launchd-supervised bus, which is presumably the host with the churn. Says so now, with both ways round it. Left as a documented caveat rather than templating the variable into the plist: that is an installer change, and this is meant to be temporary instrumentation. One new test, mutation-tested: reverting the marker to a presence check fails it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MbSWrbyPaRCrYmyMf4KzsS

evansenter · 2026-08-10T18:35:37Z

Round 2 addressed in 4da688d. All four taken.

middleware.py:470 — presence vs non-empty. Confirmed at lines 367-369: the auth path treats an empty value as no identity, so keying the marker on presence would label a request "proxied" that the auth layer would have rejected. Now headers.get(...). Since the marker's whole job is to stop the wrong candidates being eliminated, the two disagreeing was the one inconsistency that could cost the diagnosis.

Your second point — a local process can set the header itself, because loopback bypasses auth — is right and is now stated in the comment. The marker narrows the candidate set; it does not authenticate anything, and I'd rather that be written down than inferred.

middleware.py:530 and :406 — stale "DEV_MODE". Both now say "peer logging" and point at _peer_logging_enabled, so the switch list lives in exactly one place.

CLAUDE.md:221 — the supervised bus doesn't see either switch. Verified: the plist templates only PATH, PYTHONPATH, _ICON, _LOG, _ERR. This was the most useful of the four, because the block is specifically instructions for a live diagnosis on that host and the documented form silently wouldn't work there. Documented with both ways round it (add to EnvironmentVariables and reload, or stop the service and run foreground).

I did not template _LOG_PEER into the plist. That's an installer change, and #141 just reworked that script; this is meant to be temporary instrumentation, so a documented caveat seemed the smaller commitment. Happy to be overruled.

Also merged main (#141) in 3b087c5 — one conflict in CLAUDE.md's env-var list, resolved by keeping main's expanded _BRIDGE_LOG/_BRIDGE_ERR clause with _LOG_PEER added. No source overlap.

660 passed, format + lint clean. The new test fails when the marker is reverted to a presence check.

Generated by Claude Code

claude · 2026-08-10T18:39:00Z

+    headers = dict(scope.get("headers") or [])
+    if headers.get(TailscaleAuthMiddleware.TAILSCALE_USER_HEADER):
+        return f"{host}:{port} via tailscale"
+    return f"{host}:{port}"


[Suggestion] An IPv6 peer renders unbracketed, so the label does not say where the address ends and the port begins: a ::1 caller logs from ::1:54321, and a tailnet caller (the plist ships a commented-out HOST=0.0.0.0 for exactly that deployment) logs from fd7a:115c:a1e0::1:54321. The docstring right above says the port is the identifying half, and this is the one format where the reader has to know to split on the last colon to find it.

Bracketing the host when it contains a colon gives the conventional form, matches what lsof/ss print back, and leaves every IPv4 line — the common case, and every existing test — unchanged.

claude · 2026-08-10T18:39:09Z

+    # Advisory, not proof: loopback bypasses auth entirely, so a local
+    # process CAN set this header itself and be marked as proxied. It
+    # narrows the candidate set; it does not authenticate anything.
+    headers = dict(scope.get("headers") or [])


[Suggestion] This dict is built for every /mcp POST once peer logging is on, but only register_session ever consumes the result — PEER_LOGGED_TOOLS is checked in _log_tool_call, which cannot run sooner because the tool name lives in the request body. So during a live diagnosis the get_events poll from every session (a few seconds apart, per the docstring) pays for a header dict on the event loop that is then discarded.

It is a handful of microseconds and not worth restructuring the call site for. If you want it cheap without moving anything, scanning the raw header pairs for the one key short-circuits on the first match instead of materializing the whole dict.

claude · 2026-08-10T18:39:12Z

+        # every request, before any handler ran. "" is not in TRUSTED_IPS, so
+        # such a request falls through to the header check exactly as an
+        # unknown peer should.
+        client_ip = (scope.get("client") or ("", 0))[0]


[Suggestion] The fix here is right, and the comment explaining why or rather than a .get default is worth keeping. Note that line 366 just below still has the same shape for the other key: dict(scope.get("headers", [])) raises TypeError on a scope carrying an explicit headers: None, exactly as client: None did here.

No known server sends that today, and _peer_label already uses the defensive form (scope.get("headers") or [] at line 477) — so this is only an inconsistency between two lines that now disagree about how to read the same scope. Matching them would keep the next reader from wondering which one is deliberate.

claude · 2026-08-10T18:39:20Z

+# `from 127.0.0.1:54321 via tailscale` (Tailscale's identity header is the
+# only thing in the ASGI scope that distinguishes them, and loopback bypasses
+# auth - so the marker narrows the candidates, it does not authenticate).
+# An unmarked loopback peer is a genuinely local process.


[Suggestion] This sentence is firmer than the code comment it summarizes. _peer_label hedges the marker in one direction ("Advisory, not proof ... a local process CAN set this header itself and be marked as proxied"), but this line asserts the converse without a hedge — that no marker implies a local caller.

That holds for tailscale serve, which is the only proxy this doc names, and the surrounding block is otherwise careful to say what it can and cannot prove. Softening to something like "an unmarked loopback peer is a local process, as far as anything in the scope can tell" would keep the paragraph to its own standard without weakening the advice.

claude

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review — Round 3

Summary

All four round-2 items landed: the via tailscale marker now keys on a non-empty header value (middleware.py:478), matching the if not tailscale_user check that TailscaleAuthMiddleware itself performs; both stale "under DEV_MODE" docstrings now say "peer logging" and point at _peer_logging_enabled; and the CLAUDE.md block gained the launchd caveat, whose claim I checked against scripts/com.evansenter.agent-event-bus.plist — it really does template only PATH, PYTHONPATH, _ICON, _LOG, _ERR. test_empty_identity_header_is_not_treated_as_proxied and test_direct_calls_are_not_marked_as_proxied pin both sides of the marker, so it cannot silently become presence-keyed again.

The disabled path is still a genuine no-op — _peer_label returns None before touching the scope, peer_suffix stays empty, and both logger.info branches render byte-identically. Env-var truthiness matches the repo idiom (helpers.py:179, server.py:67, bridge.py:2132). PEER_LOGGED_TOOLS still scopes the suffix to register_session, and both degraded-peer shapes (absent client, unpack failure) resolve to from unknown peer rather than raising inside the logging path.

Previously Addressed (Filtered)

All eight prior findings across rounds 1-2 — the DEV_MODE-is-not-quiet gate, the tailscale serve caveat, the client=None crash at middleware.py:360, the untested no-args branch, the presence-vs-non-empty marker mismatch, the two stale docstrings, and the plist note — are implemented and are not re-raised.

Findings

Four suggestions posted inline, none blocking: unbracketed IPv6 peers in the label, the header dict being built on the event loop for every /mcp POST when only register_session consumes it, the sibling scope.get("headers", []) that still has the explicit-None shape this PR just fixed for client, and one CLAUDE.md sentence that asserts more than the code comment beside it does.

Verdict

APPROVE - No blocking findings. Nothing this round names a way the instrumentation produces a wrong result; the remaining items are readability and consistency.

Automated review by Claude Code

1. An IPv6 peer rendered unbracketed, so the label hid the very thing it exists to show: `from ::1:54321` gives no way to see where the address ends and the port begins, and a tailnet peer (fd7a:...) is worse. Now `[::1]:54321` - the conventional form, and what lsof/ss print back. IPv4 is untouched. 2. The header lookup materialized a dict on the event loop for every /mcp POST once peer logging is on, while only register_session consumes it (the tool name lives in the request body, so PEER_LOGGED_TOOLS cannot be checked until _log_tool_call, in the worker thread). Scanning the pairs short-circuits on the first match instead. Microseconds, but it was pure waste on the loop and the fix is smaller than the comment. 3. TailscaleAuthMiddleware still read `dict(scope.get("headers", []))` - the same explicit-None shape this PR just fixed one line above for `client`. Two reads of one scope now agree on how to read it; leaving them different is how the next person picks the wrong one. 4. The CLAUDE.md caveat asserted the converse of what the code comment hedges: the comment says the marker cannot prove a caller IS proxied, while the doc said an unmarked peer IS local. Softened to what the scope can actually support. Three new tests. Mutation-tested: removing the bracketing, applying it unconditionally, and reverting the headers-None read each fail one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MbSWrbyPaRCrYmyMf4KzsS

evansenter · 2026-08-10T18:42:10Z

Round 3 addressed in 8daf8f9. All four taken.

IPv6 bracketing was the one with teeth — the label was hiding the exact thing it exists to show. from ::1:54321 and from fd7a:115c:a1e0::1:54321 both leave the reader splitting on the last colon to find the port. Now [::1]:54321. IPv4 unchanged, and a test pins both directions so it can't become unconditional either.

The header dict on the event loop — correct that only register_session consumes it and the filter can't run until _log_tool_call. Took the scan; it short-circuits and the code came out shorter than the comment explaining it.

dict(scope.get("headers", [])) — took this one for the same reason I took the client=None note last round: the module would otherwise have the defensive form on one line and the crash shape on the next, which is worse than either consistently.

The CLAUDE.md sentence — fair, and the sharpest of the four as a review point. The code comment hedges that the marker can't prove a caller is proxied; the doc then asserted the converse, that no marker means local. Now: "a local process as far as anything in the scope can tell — tailscale serve is the only proxy this deployment puts in front of the bus, but nothing here proves the absence of another."

One correction to my own work in passing: my first test_ipv4_peers_are_not_bracketed asserted "[" not in ..., which fails against the unmutated code — the line is full of ANSI escapes and those are all brackets. Caught by the mutation run, now asserting "[127.0.0.1]" not in line.

663 passed, format + lint clean. Three new tests, each killed by its mutation.

Since rounds 2 and 3 have both been readability and consistency with no wrong-result findings, I'll merge on green rather than run a fourth round — the approve stands.

Generated by Claude Code

claude · 2026-08-10T18:44:48Z

+    # short-circuits on the first match instead of materializing a dict that
+    # every get_events poll then discards.
+    wanted = TailscaleAuthMiddleware.TAILSCALE_USER_HEADER
+    identity = next((v for k, v in (scope.get("headers") or []) if k == wanted), None)


[Suggestion] This is now the one unguarded scope read in a function that guards the read four lines above it. host, port = client is wrapped in try/except (TypeError, ValueError), but a header entry that is not a 2-item sequence raises ValueError out of this generator - and unlike _log_tool_call (which is wrapped, and runs after the response), _peer_label is called at line 523 before await self.app(...), so a raise here fails the request rather than costing a log line.

Not filed as Important: uvicorn always yields 2-tuples, so this needs a non-spec-conforming ASGI server to reach, and the dict(...) form it replaced would have raised identically - no regression. But the asymmetry within one function is the same argument you made for taking the client=None and headers=None notes in earlier rounds: better consistently defensive than defensive on one line and bare on the next. Widening the existing except to cover the scan closes it.

claude · 2026-08-10T18:44:55Z

+    flipping it in a test does not depend on import order. An operator flips
+    it by restarting the server either way.
+    """
+    return bool(os.environ.get("DEV_MODE") or os.environ.get("AGENT_EVENT_BUS_LOG_PEER"))


[Suggestion] AGENT_EVENT_BUS_LOG_PEER=0 enables peer logging - any non-empty value is truthy here.

This matches the repo idiom exactly (helpers.py:179, server.py:67, bridge.py:2132 all do the same), which is a real argument for leaving it alone. The reason it is slightly sharper for this variable than for DEV_MODE: the CLAUDE.md block documents it as AGENT_EVENT_BUS_LOG_PEER=1 agent-event-bus, and a variable documented with an explicit =1 invites an operator to reach for =0 to turn it back off - where DEV_MODE is idiomatically set/unset rather than valued. Since the intended workflow is "flip it on for a diagnosis, flip it off after", one sentence in the CLAUDE.md block ("unset it, do not set it to 0") would cost less than changing the idiom.

claude · 2026-08-10T18:45:05Z

+    if ":" in str(host):
+        host = f"[{host}]"
+
+    # Behind `tailscale serve` the connection is terminated by the LOCAL


[Suggestion] _peer_label is now 66 lines, of which roughly 50 are comment - this block alone is 23 lines of prose for the 4 lines of code beneath it. Each paragraph individually earned its place (each one is a prior round finding written down), but the accumulation means the marker mechanism is harder to see than its rationale, and a reader gets through four separate arguments before reaching identity = next(...).

Purely presentational, and I would rather it stay verbose than lose the reasoning. If it is worth compressing: the "truthiness, not presence" and "advisory, not proof" paragraphs are both pinned by tests (test_empty_identity_header_is_not_treated_as_proxied, test_direct_calls_are_not_marked_as_proxied), so they could shrink to a line each pointing at those; and the "scanned rather than dict()-ed" paragraph is about performance rather than correctness, so it reads naturally as a one-liner. Given this is temporary instrumentation with a stated removal condition, it may simply not be worth the churn.

claude

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review — Round 4

Summary

All four round-3 items landed and check out against the code. IPv6 hosts are bracketed at middleware.py:469, with test_ipv6_peers_are_bracketed / test_ipv4_peers_are_not_bracketed pinning both directions — and the corrected "[127.0.0.1]" not in line assertion is the right shape, since the ANSI escapes really would have made "[" not in line vacuous. The header dict() on the event loop became a short-circuiting scan at middleware.py:496. The sibling scope.get("headers", []) in TailscaleAuthMiddleware now uses the same or [] form as the client line, with test_explicit_null_headers_does_not_raise covering it. And the CLAUDE.md sentence now hedges to what the scope can actually tell ("a local process as far as anything in the scope can tell"), matching the comment beside the marker rather than asserting the converse.

I re-verified the two external claims the docs make. scripts/com.evansenter.agent-event-bus.plist templates only PATH, PYTHONPATH, _ICON, _LOG, _ERR, so the supervised-bus caveat is accurate. And _dev_notify (helpers.py:177) does fire per tool call from server.py, so the "DEV_MODE is not a quiet switch" rationale for a separate AGENT_EVENT_BUS_LOG_PEER holds.

The disabled path remains a genuine no-op: _peer_label returns None before reading the scope, peer_suffix stays empty, and both logger.info branches render byte-identically.

Previously Addressed (Filtered)

All twelve findings from rounds 1-3 — the DEV_MODE-is-not-quiet gate, the tailscale serve caveat and marker, the client=None crash, the untested no-args branch, the presence-vs-non-empty marker mismatch, the two stale docstrings, the plist note, IPv6 bracketing, the event-loop header dict, the sibling headers read, and the over-asserting CLAUDE.md sentence — are implemented and are not re-raised.

Findings

Three suggestions posted inline, none blocking: the header scan is the one unguarded scope read left in a function that guards the read above it, AGENT_EVENT_BUS_LOG_PEER=0 reads as enabled, and the comment-to-code ratio in _peer_label has grown across rounds.

Verdict

APPROVE - No blocking findings. Nothing this round names a way the instrumentation produces a wrong result; rounds 2, 3 and 4 have all been readability and consistency, which reads as converged.

Automated review by Claude Code

claude Bot reviewed Aug 10, 2026

View reviewed changes

claude Bot previously approved these changes Aug 10, 2026

View reviewed changes

evansenter dismissed claude[bot]’s stale review via 2de0c64 August 10, 2026 18:20

claude Bot reviewed Aug 10, 2026

View reviewed changes

claude Bot previously approved these changes Aug 10, 2026

View reviewed changes

evansenter dismissed claude[bot]’s stale review via 4da688d August 10, 2026 18:35

claude Bot reviewed Aug 10, 2026

View reviewed changes

claude Bot previously approved these changes Aug 10, 2026

View reviewed changes

evansenter dismissed claude[bot]’s stale review via 8daf8f9 August 10, 2026 18:41

claude Bot reviewed Aug 10, 2026

View reviewed changes

claude Bot approved these changes Aug 10, 2026

View reviewed changes

		)


		# Tools whose log line carries the caller's peer address under DEV_MODE

Conversation

evansenter commented Aug 10, 2026

What

Deliberately narrow

Edges

Tests

Follow-up

Uh oh!

claude Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

evansenter commented Aug 10, 2026

Uh oh!

claude Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

evansenter commented Aug 10, 2026

Uh oh!

claude Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

evansenter commented Aug 10, 2026

Uh oh!

claude Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants