feat: auto-detect local vs remote bus in make logs - #120
Conversation
Previously `make logs` hardcoded `tail -f` on the local log file, which silently showed an empty/stale log for users configured as clients (`make install-client REMOTE_URL=...`) pointed at a Tailscale-hosted bus. Detection now parses `claude mcp list` (falling back to `$AGENT_EVENT_BUS_URL`), validates the URL scheme, and SSH-tails the remote host when non-local. Override with `make logs BUS_HOST=<host>`. Closes #119 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review by Claude. Summary: Small focused change that fixes the silent make logs case for client-mode installs by auto-detecting the bus host from claude mcp list or AGENT_EVENT_BUS_URL and SSH-tailing remote hosts. Security hardening against SSH option injection and IPv6 literals is appropriate. Findings: 0 Critical, 0 Important, 3 Suggestions (posted below as inline follow-up). Verdict: APPROVE. Linked issue 119 is fully addressed via option 2 (auto-detect in the existing logs target). Automated review by Claude Code.
| if [ -z "$$HOST" ]; then \ | ||
| CLAUDE_CMD=$$(command -v claude || echo "$$HOME/.local/bin/claude"); \ | ||
| if [ -x "$$CLAUDE_CMD" ]; then \ | ||
| URL=$$("$$CLAUDE_CMD" mcp list 2>/dev/null | awk '/^agent-event-bus:/ {print $$2}'); \ |
There was a problem hiding this comment.
[Suggestion] The awk parser depends on the current claude mcp list text format (colon-terminated name in field 1, URL in field 2). If that output ever changes (extra prefix, different separator, extra decoration), detection silently fails and falls through to the local tail, which reproduces the exact bug this PR is fixing. A short code comment noting the format dependency would help future debugging, or using claude mcp get agent-event-bus with structured output (if supported) would be a more stable contract.
There was a problem hiding this comment.
Moved the format-dependency note into the target's header comment block (Makefile:146-148). Kept the text parser since claude mcp list has no structured output flag today. Resolved in c770c0f.
| HOST=$$(echo "$$URL" | sed -E 's|https?://||; s|/.*||; s|:[0-9]+$$||; s|^\[||; s|\]$$||'); \ | ||
| fi; \ | ||
| fi; \ | ||
| if [ -z "$$HOST" ] || [ "$$HOST" = "localhost" ] || [ "$$HOST" = "127.0.0.1" ] || [ "$$HOST" = "::1" ]; then \ |
There was a problem hiding this comment.
[Suggestion] Consider adding 0.0.0.0 to the local-host list. If the bus is bound to 0.0.0.0 and that literal ends up in the MCP URL (uncommon, but possible), it is currently classified as remote and triggers an SSH attempt to 0.0.0.0.
There was a problem hiding this comment.
Added in c770c0f (Makefile:163). Now: localhost, 127.0.0.1, ::1, and 0.0.0.0 all fall through to local tail.
| tail -f ~/.claude/contrib/agent-event-bus/agent-event-bus.log; \ | ||
| else \ | ||
| echo "Tailing remote bus at $$HOST (Ctrl-C to exit)..."; \ | ||
| ssh -t -- "$$HOST" 'tail -f ~/.claude/contrib/agent-event-bus/agent-event-bus.log'; \ |
There was a problem hiding this comment.
[Suggestion] The log path is now repeated on lines 163 and 166 (and appears in the restart error message earlier in the file). Extracting a Make variable at the top of the Makefile would keep the three references in sync. Minor DRY nit.
There was a problem hiding this comment.
Expanded scope per user direction — see PR comment above. TL;DR: introduced AGENT_EVENT_BUS_LOG / AGENT_EVENT_BUS_ERR env vars (mirroring existing AGENT_EVENT_BUS_DB pattern), wired end-to-end through Python, Makefile, install scripts, and launchd/systemd template substitution. c770c0f.
Addresses PR #120 review feedback: - Add shell no-op comment noting the awk parser's dependency on `claude mcp list` text format (suggestion #1) - Treat 0.0.0.0 as local in `make logs` host check (suggestion #2) - Centralize hardcoded log/err paths via AGENT_EVENT_BUS_* env vars (suggestion #3, expanded per user direction) New env vars follow the existing AGENT_EVENT_BUS_DB pattern: - AGENT_EVENT_BUS_LOG: Python `LOG_FILE` + Makefile `$(LOG_FILE)` - AGENT_EVENT_BUS_ERR: launchd/systemd StandardError redirect + Makefile `$(ERR_FILE)` End-to-end wiring: - src/agent_event_bus/server.py — LOG_FILE reads env var - Makefile — defines $(LOG_FILE) / $(ERR_FILE) Make vars - scripts/com.evansenter.agent-event-bus.plist — __ERR_FILE__ placeholder - scripts/agent-event-bus.service — __LOG_FILE__ / __ERR_FILE__ placeholders - scripts/install-launchagent.sh + install-systemd.sh — resolve env vars (with defaults) and substitute into templates; echoes reflect override - CLAUDE.md — document new env vars in Operations section Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses PR #120 review feedback: - Add shell no-op comment noting the awk parser's dependency on `claude mcp list` text format (suggestion #1) - Treat 0.0.0.0 as local in `make logs` host check (suggestion #2) - Centralize hardcoded log/err paths via AGENT_EVENT_BUS_* env vars (suggestion #3, expanded per user direction) New env vars follow the existing AGENT_EVENT_BUS_DB pattern: - AGENT_EVENT_BUS_LOG: Python `LOG_FILE` + Makefile `$(LOG_FILE)` - AGENT_EVENT_BUS_ERR: launchd/systemd StandardError redirect + Makefile `$(ERR_FILE)` End-to-end wiring: - src/agent_event_bus/server.py — LOG_FILE reads env var - Makefile — defines $(LOG_FILE) / $(ERR_FILE) Make vars - scripts/com.evansenter.agent-event-bus.plist — __ERR_FILE__ placeholder - scripts/agent-event-bus.service — __LOG_FILE__ / __ERR_FILE__ placeholders - scripts/install-launchagent.sh + install-systemd.sh — resolve env vars (with defaults) and substitute into templates; echoes reflect override - CLAUDE.md — document new env vars in Operations section Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Feedback AddressedImplemented
Expanded beyond original feedbackPer discussion with the user, the DRY fix was broadened to introduce env var overrides consistent with the existing
End-to-end wiring (set env var → Not changedKept the remote SSH tail path hardcoded (the remote host has its own config and we can't know its override from the local invocation). Commit: c770c0f |
There was a problem hiding this comment.
Prompt: evansenter/dotfiles/.../claude-review.md
Code Review
(Note: gh api/Write sandbox blocked; posting findings inline in the body as a last-resort fallback per the review prompt.)
SUMMARY
The core auto-detection in make logs is well-scoped and fully addresses issue 119 via option 2. The URL sanitization (scheme/path/port/IPv6-bracket stripping) and the SSH option-injection guard (ssh -t -- HOST) are appropriate defenses. However, the AGENT_EVENT_BUS_LOG / AGENT_EVENT_BUS_ERR env-var override feature added beyond original feedback has an integration gap: the vars are not propagated to the running service environment, so the Python logger writes to the default path at runtime regardless of the override.
PREVIOUSLY ADDRESSED (FILTERED)
All three suggestions from the prior review (awk format dependency note, adding 0.0.0.0 to local-host list, extract log path Make variable) were addressed in the latest commit. These have not been re-raised.
FINDINGS
[Important] src/agent_event_bus/server.py:46 -- AGENT_EVENT_BUS_LOG never reaches the running service
AGENT_EVENT_BUS_LOG is read at module import time, but neither scripts/agent-event-bus.service nor scripts/com.evansenter.agent-event-bus.plist sets it in the service runtime environment -- both only set AGENT_EVENT_BUS_ICON. The advertised pipeline (CLAUDE.md L161-164: set before make install-server so server writes to custom paths) is broken:
- Linux: install-systemd.sh substitutes LOG_FILE into StandardOutput=append:..., so systemd redirects stdout to the custom path, but the Python logger, running without AGENT_EVENT_BUS_LOG in its env, writes to the default ~/.claude/contrib/agent-event-bus/agent-event-bus.log. Normal logger.info(...) output ends up at the default path; the custom path only gets whatever framework output leaks to stdout.
- macOS: the plist has no LOG_FILE placeholder at all (only ERR_FILE), so AGENT_EVENT_BUS_LOG does literally nothing for log redirection, even though install-launchagent.sh echoes the custom path in the Logs line as if it did.
make logs then tails the user-specified custom path (which is mostly empty) while the real logs sit at the default path.
Fix: add Environment=AGENT_EVENT_BUS_LOG=LOG_FILE to scripts/agent-event-bus.service (and substitute in install-systemd.sh), and add a matching AGENT_EVENT_BUS_LOG entry to the plist EnvironmentVariables dict (and substitute in install-launchagent.sh). _ERR does not need the same treatment, it is only consumed by the install-time redirect, not by Python at runtime.
[Suggestion] Makefile:165 -- URL-sanitization and SSH guard lack automated coverage
The sed pipeline (scheme/path/port/IPv6-bracket stripping) and the ssh -t -- injection guard are tested only manually per the PR description. A small shell-based test (pytest parametrize + subprocess invoking make -n logs with various AGENT_EVENT_BUS_URL values, or a standalone bash test against an extracted helper) would protect these against regression. The new test_server.py::TestLogFileEnvVar tests cover only module-level env-var resolution, not the detection logic.
[Suggestion] scripts/install-launchagent.sh:51 -- Misleading Logs echo on macOS
This echo advertises the custom LOG_FILE path, but the plist has no LOG_FILE placeholder (only ERR_FILE). When the user sets AGENT_EVENT_BUS_LOG=/custom/path, the message prints the custom path even though nothing in the actual launched service redirects logs there (see the server.py:46 finding above). Once the service-env propagation is fixed this echo becomes accurate; until then it is misleading.
[Suggestion] CLAUDE.md:163 -- Update Naming Conventions examples list
The Naming Conventions section lists env vars as AGENT_EVENT_BUS_* prefix (e.g., _DB, _URL, _AUTH_DISABLED, _ICON, _TESTING). The new _LOG and _ERR should be added there for discoverability; future readers grepping the conventions section will not have to scroll to Operations to learn about them.
VERDICT
REQUEST_CHANGES -- 1 Important (env-var override does not reach the running service), 3 Suggestions.
Automated review by Claude Code
There was a problem hiding this comment.
Code Review
Summary. The make logs auto-detect and SSH hardening are tight, and the prior reviews three suggestions (awk format comment, 0.0.0.0 in the local list, LOG_FILE/ERR_FILE Make variables) are all addressed. One new finding on the AGENT_EVENT_BUS_LOG/AGENT_EVENT_BUS_ERR override plumbing introduced alongside the main change.
Previously Addressed (Filtered).
- [Suggestion] awk dependency on
claude mcp listtext format — addressed via inline code comments atMakefile:151-152. - [Suggestion] Add
0.0.0.0to the local-host list — addressed atMakefile:167. - [Suggestion] DRY the log path references — addressed via
LOG_FILE/ERR_FILEMake variables atMakefile:4-5.
Findings. (Posted here because inline gh api submission was blocked by this sandbox — last-resort fallback per the review prompt.)
[Important] scripts/agent-event-bus.service:14 — The installer substitutes __LOG_FILE__ / __ERR_FILE__ into StandardOutput= / StandardError=, but it never adds Environment=AGENT_EVENT_BUS_LOG=... / Environment=AGENT_EVENT_BUS_ERR=... to the unit. Because server.py reads AGENT_EVENT_BUS_LOG at import time (server.py:46) and the service process only inherits the Environment= keys listed in the unit (PYTHONPATH, AGENT_EVENT_BUS_ICON), the Python FileHandler silently falls back to ~/.claude/contrib/agent-event-bus/agent-event-bus.log even when the operator set AGENT_EVENT_BUS_LOG=/foo.log before make install-server.
Net effect: the documented override (CLAUDE.md:160-163) produces two files — systemd captures stdout to /foo.log (nearly empty in production, since the logger writes via FileHandler, not stdout), while the actual log lines go to the default path. make logs with the env var still set then tails the empty custom file, which is exactly the silent-log regression this PR is trying to prevent.
Fix: add Environment=AGENT_EVENT_BUS_LOG=__LOG_FILE__ and Environment=AGENT_EVENT_BUS_ERR=__ERR_FILE__ to the systemd unit (and the equivalent EnvironmentVariables entries in com.evansenter.agent-event-bus.plist) so the Python process sees the same paths the service file was generated with. A test that asserts the generated service/plist contains both the StandardOutput= path and the matching Environment= entry would lock this in.
[Suggestion] Makefile:173 — The remote tail hardcodes ~/.claude/contrib/agent-event-bus/agent-event-bus.log, so if the remote bus was installed with AGENT_EVENT_BUS_LOG=/foo.log this tail will be silent for the same reason that motivated this PR. Not in scope to fix now (it would require reading the remote install-time path), but worth a code comment here flagging the assumption so a future debugging session does not repeat the original diagnosis path.
Verdict. REQUEST_CHANGES — one Important issue on env var propagation to the installed service, plus one Suggestion.
Automated review by Claude Code
Addresses PR #120 review feedback (Important): the env var override was read by Python at import time, but the launchd plist and systemd unit never passed the vars into the service process. That meant the documented override produced two disjoint log files (redirect to the custom path, Python FileHandler to the default) — reintroducing the silent-log bug this PR was supposed to fix. - scripts/com.evansenter.agent-event-bus.plist: add AGENT_EVENT_BUS_LOG and AGENT_EVENT_BUS_ERR to the EnvironmentVariables dict. - scripts/agent-event-bus.service: add matching Environment= entries. - scripts/install-launchagent.sh: substitute __LOG_FILE__ alongside existing __ERR_FILE__ substitution. - Makefile: add header comment on remote tail path assumption (the remote's AGENT_EVENT_BUS_LOG can't be known locally). - tests/test_server.py: add TestMakeLogsHostDetection with 8 parametrized URL → HOST extraction cases (hostname, port, IPv6, dash-prefix, non-http, empty). Duplicates the Makefile sed pipeline; keep in sync. - CLAUDE.md: list _LOG and _ERR in the Naming Conventions example line. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Feedback Addressed (round 2)Implemented
Commit: 1195e48 All 314 tests passing locally (was 306). |
There was a problem hiding this comment.
Prompt: evansenter/dotfiles/.../claude-review.md
Code Review
Summary. Round 2 fixes all land cleanly. The AGENT_EVENT_BUS_LOG / AGENT_EVENT_BUS_ERR overrides now propagate to the running service via Environment= in the systemd unit (scripts/agent-event-bus.service:10-11) and the EnvironmentVariables dict in the plist (scripts/com.evansenter.agent-event-bus.plist:26-29), so the Python FileHandler and the launchd/systemd stdout-stderr redirects all point at the same file. The awk-format, 0.0.0.0, DRY, URL-sanitization-tests, naming-conventions, and remote-tail-hardcoding suggestions from the prior two rounds are all addressed. Two minor follow-up suggestions noted inline.
Previously Addressed (Filtered).
- [Important] server.py:46 env var never reaches service runtime - addressed in round 2 (systemd unit + plist Environment= entries, installer substitutes the log-file and err-file placeholders in both templates).
- [Suggestion] Remote tail path hardcoded - addressed via the target header comment block (Makefile:150-156).
- [Suggestion] URL-sanitization tests - addressed via TestMakeLogsHostDetection (8 parametrized URL-to-host cases).
- [Suggestion] Naming Conventions missing _LOG / _ERR - addressed at CLAUDE.md:28.
- [Suggestion] Misleading Logs echo on macOS - addressed now that the plist actually honors AGENT_EVENT_BUS_LOG.
- [Suggestion] awk format dependency / 0.0.0.0 / LOG_FILE Make variable - all addressed in round 1.
Verdict. APPROVE - 0 Critical, 0 Important, 2 Suggestions. Linked issue 119 is fully addressed via option 2 (auto-detect in the existing logs target).
Automated review by Claude Code
| class TestMakeLogsHostDetection: | ||
| """Tests for the host-detection pipeline in the Makefile's `logs` target. | ||
|
|
||
| The sed/grep pipeline is at Makefile:154-157. These tests duplicate it via |
There was a problem hiding this comment.
[Suggestion] Docstring line reference is stale. The sed/grep pipeline moved to roughly Makefile:167-168 after the env-var expansion (lines 154-157 now point at the target comment block and the logs: declaration, not the pipeline). A quick tweak to the line range would keep the sync-risk note accurate.
| AGENT_EVENT_BUS_DB=/path/to/db.sqlite agent-event-bus | ||
|
|
||
| # Override log/error file paths (set before `make install-server` so the | ||
| # launchd/systemd templates are generated with the custom paths) |
There was a problem hiding this comment.
[Suggestion] These two overrides are shown as bare assignments, but unlike AGENT_EVENT_BUS_DB=/path/to/db.sqlite agent-event-bus just above (inline env), _LOG / _ERR are install-time bindings - they need to be in the environment of make install-server itself so the sed substitution picks them up. A reader copy-pasting these bare lines into a shell will set them for that shell only and may still run make install-server without them exported. Clearer: export them and then run make install-server, or prefix them on the make invocation to match the _DB example style above.
- CLAUDE.md: show AGENT_EVENT_BUS_LOG/ERR prefixed on `make install-server` rather than as bare shell assignments. Bare assignments set vars only for the current shell and would NOT flow into the install script's sed, making the previous example a real copy-paste footgun. - tests/test_server.py: correct Makefile line reference in TestMakeLogsHostDetection docstring (pipeline moved to 166-168 after the env-var refactor). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Feedback Addressed (round 3)Implemented
Commit: e22f101 |
There was a problem hiding this comment.
Code Review
Summary
Round 4 review: all prior feedback (across 3 rounds) is fully addressed. The auto-detect logic, env-var propagation through install scripts -> service templates -> Python runtime, security hardening (ssh -t --, IPv6 bracket stripping, scheme validation), and test coverage are all in good shape. Linked issue #119 is fully resolved via option 2 (auto-detect in the existing logs target).
Previously Addressed (Filtered)
- [Important] server.py:46 env var never reaches running service - fixed in round 2 (systemd Environment= + plist EnvironmentVariables entries with LOG_FILE/ERR_FILE substitution).
- [Suggestion] awk format dependency note - addressed in round 1 (Makefile header comment block).
- [Suggestion] 0.0.0.0 in local-host list - addressed in round 1 (Makefile:171).
- [Suggestion] DRY log/err paths - addressed in round 1 via LOG_FILE/ERR_FILE Make variables (Makefile:4-5).
- [Suggestion] URL-sanitization automated coverage - addressed in round 2 via TestMakeLogsHostDetection (8 parametrized URL -> HOST cases incl. injection attempt).
- [Suggestion] Naming Conventions missing _LOG/_ERR - addressed in round 2 (CLAUDE.md:28).
- [Suggestion] Misleading macOS Logs: echo - addressed in round 2 once the plist actually honored AGENT_EVENT_BUS_LOG.
- [Suggestion] Remote tail path hardcoded - addressed in round 2 via comment block at Makefile:154-156.
- [Suggestion] Stale Makefile line reference in test docstring - addressed in round 3 (tests/test_server.py:1272).
- [Suggestion] Bare-assignment env vars in CLAUDE.md Operations example - addressed in round 3 (inline-env style on make install-server with explanatory note).
Verdict
APPROVE - 0 Critical, 0 Important, 0 Suggestions. All prior review feedback has been resolved across rounds 1-3.
Automated review by Claude Code
Summary
make logsnow auto-detects whether the event bus is local or on a remote Tailscale host and tails accordingly — fixing the silent-empty-log case when an MCP client is pointed at a remote busBUS_HOST=<host>Make arg →claude mcp list→$AGENT_EVENT_BUS_URL→ local fallbackssh -t --), and IPv6 literalsCloses #119
Test plan
make check— 304 tests passingclaude mcp listpointing atmac-mini.tailac7b3c.ts.net→ resolves to SSHBUS_HOST=localhost/127.0.0.1/[::1]→ local tailstdio://...) → falls through to local tailhttps://-oProxyCommand=id/mcp) →ssh -- "$HOST"treats as hostname, no flag parsinghttp://[::1]:8080/mcp) → strips brackets, matches local checkssh -t mac-mini.tailac7b3c.ts.net 'tail -f ...'in a second terminal (reviewer welcome to try)🤖 Generated with Claude Code