Skip to content

Supervise the RFC #122 bridge as a LaunchAgent - #139

Merged
evansenter merged 2 commits into
mainfrom
claude/codebase-review-next-steps-vphsgo
Aug 10, 2026
Merged

Supervise the RFC #122 bridge as a LaunchAgent#139
evansenter merged 2 commits into
mainfrom
claude/codebase-review-next-steps-vphsgo

Conversation

@evansenter

Copy link
Copy Markdown
Owner

The bridge merged in #135 with no install target — it ran only as a foreground uv run agent-event-bus-bridge, so it died with its shell and never survived a reboot. The bus has had a LaunchAgent all along (which is why it has been up continuously); the bridge simply never had one.

Adds com.evansenter.agent-event-bus-bridge.plist, install/uninstall scripts, and make install-bridge / make uninstall-bridge.

Design decisions worth reviewing

A separate unit, not an addition to install-server. A bus host doesn't have to run a bridge, and the bridge is experimental while the bus isn't. Bundling them would make every bus install start a daemon most machines don't want.

Boot ordering needs no launchd machinery. launchd has no dependency ordering, so the bridge can start before the bus is listening — but register_with_retry already backs off 1s→30s until the bus answers rather than exiting, and its docstring anticipates exactly the "same supervisor launches both" case. A cold boot in the wrong order self-corrects. That's why the installer prints registered: false as a status line, not an error.

KeepAlive is safe with the singleton lock. flock releases when the dead process's fd closes, so the replacement acquires it cleanly instead of refusing to start, and the startup sweep reclaims the webhook row the dead instance left registered. ThrottleInterval is stated explicitly rather than left to the default because it's load-bearing here — it bounds how fast a crash loop rewrites the log.

--backend spool is pinned in the unit. tmux additionally needs wake/panes.json maintained by something session-side, which doesn't exist. AGENT_EVENT_BUS_URL is deliberately left unset so the bridge's loopback default applies — correct on the machine hosting the bus, and the comment explains what changes if the bus moves off-box.

Documented caveat, not a silent one

The bridge logs via basicConfig (stderr only), so launchd's capture is its log — and launchd truncates that on every process start. A crash loop overwrites the evidence of earlier iterations. ThrottleInterval keeps the surviving window useful. Giving the bridge its own append-mode file handler, like the bus has, is the follow-up that removes this properly; I kept it out of here to keep the diff focused on supervision.

Verification

The suite mocks launchd entirely, so guide.md gains a four-item manual checklist — and two of them check claims this unit's own comments make rather than taking them on trust:

  1. Crash restartkill -9; launchd respawns within ThrottleInterval. Proves the flock actually releases and the sweep reclaims the row instead of duplicating it.
  2. Boot order — unload the bus, load the bridge (registered: false), then load the bus; it should flip to true within ~30s unaided.
  3. Reboot — the actual requirement.
  4. Clean unload — webhook row gone, port 8082 free.

What none of it covers: whether a session actually wakes. The spool line lands, but nothing drains it until #134.

make check green: 576 passed. Plist parses via plistlib; both scripts pass bash -n.

Also: three #137 follow-ups

Deferred at that merge with a note that they'd ride the next PR:

  • Hoisted the session-id env scrub from test_cli.py to conftest.py, so test_structured_payload.py and test_signal_levels.py are protected by construction rather than by the accident of what they currently assert.
  • Generalized _session_id_from_env's docstring off the specific ~/.exports / ~/.zshrc paths this repo doesn't own, keeping the durable half (rc files are interactive-only).
  • Added CLAUDE_CODE_SESSION_ID to the CLAUDE.md env-var table — the one consulted name that will never carry the AGENT_EVENT_BUS_ prefix.

Not taken, from the same review: defaulting --client-id in register/unregister from the same helper. The gap needs a SessionStart hook that omits --client-id, which isn't the current setup (dotfiles session-start.sh:74 passes it), and auto-adopting the session id as client_id would change registration semantics — the (machine, client_id) dedupe means a manual register would resume the real session rather than mint a new one. That deserves its own change.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AFp3uCezpvbeyAMYnNrQ4R


Generated by Claude Code

The bridge merged in #135 with no install target: it ran only as a foreground
`uv run agent-event-bus-bridge`, so it died with its shell and never came back
after a reboot. The bus has had a LaunchAgent all along, which is why it has
been up continuously; the bridge simply never had one.

Adds com.evansenter.agent-event-bus-bridge.plist plus install/uninstall
scripts and `make install-bridge` / `make uninstall-bridge`.

A SEPARATE unit from the bus, not an addition to install-server: a bus host
does not have to run a bridge, and the bridge is experimental while the bus is
not. The unit pins --backend spool (tmux additionally needs wake/panes.json
maintained by something session-side) and leaves AGENT_EVENT_BUS_URL unset so
the bridge's own loopback default applies - the correct value on the machine
hosting the bus.

Boot ordering needs no launchd machinery: register_with_retry already backs
off 1s->30s until the bus answers rather than exiting, and its docstring
anticipates exactly the same-supervisor-launches-both case. A cold boot in the
"wrong" order self-corrects, so the installer reports registered:false as a
status line rather than an error.

KeepAlive is safe with the singleton lock: flock releases when the dead
process's fd closes, so the replacement acquires it cleanly, and the startup
sweep reclaims the webhook row the dead instance left behind. ThrottleInterval
is stated explicitly because it is load-bearing - it bounds how fast a crash
loop rewrites the log.

Documented caveat rather than a silent one: the bridge logs via basicConfig
(stderr only), so launchd's capture IS its log, and launchd truncates that on
every restart. Giving the bridge an append-mode file handler like the bus has
is the follow-up that removes it.

guide.md gains a supervision section and a four-item verification checklist -
crash restart, boot order, reboot, clean unload. Two of those check claims the
unit's own comments make (flock release, retry backoff) rather than taking
them on trust; the suite mocks launchd entirely, so they have to be manual.

Also takes three #137 review follow-ups deferred at that merge:
- hoist the session-id env scrub from test_cli.py to conftest.py, so
  test_structured_payload.py and test_signal_levels.py are covered by
  construction rather than by the accident of what they currently assert
- generalize _session_id_from_env's docstring off the specific ~/.exports and
  ~/.zshrc paths this repo does not own, keeping the durable half (rc files
  are interactive-only)
- add CLAUDE_CODE_SESSION_ID to the CLAUDE.md env-var table, the one
  consulted name that will never carry the AGENT_EVENT_BUS_ prefix

Not taken, from the same review: defaulting --client-id in register/unregister
from the same helper. The gap it closes needs a SessionStart hook that omits
--client-id, which is not the current setup (dotfiles session-start.sh:74
passes it), and auto-adopting the session id as client_id would change
registration semantics - the (machine, client_id) dedupe means a manual
`register` would resume the real session rather than mint a new one. Worth its
own change, not a rider on this one.

make check green: 576 passed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AFp3uCezpvbeyAMYnNrQ4R
# backoff rather than die (register_with_retry), so this is a warning and not a
# hard failure - but starting a bridge on a box with no bus is almost always a
# mistake worth naming at install time rather than discovering in the log.
if ! launchctl list | grep -q "com.evansenter.agent-event-bus$"; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Important] This check is correctly anchored with $ — but the three pre-existing probes for the bus label are not, and the new label is a superstring of the old one:

  • Makefile:149 (make restart) — launchctl list | grep -q "com.evansenter.agent-event-bus"
  • scripts/install-launchagent.sh:29,48grep -q "$LABEL", LABEL="com.evansenter.agent-event-bus"
  • scripts/dev.sh:15 — same unanchored $LABEL

launchctl list prints the label at end-of-line, so a loaded com.evansenter.agent-event-bus-bridge row matches all of them.

Fails when: the bridge LaunchAgent is loaded and the buss launchctl loaddoes not leave a job registered (invalid plist after a badsedsubstitution,Load failed: 5, or the bus plist absent) → make restartandinstall-launchagent.sh:48match the *bridge* row, printService restarted successfully/Agent Event Bus installed and running!, fire the success notification, and exit 0 while the bus is down — the exact failure those checks exist to catch. Same root cause in dev.sh: on a bridge-only box it prints Stopping LaunchAgent for dev mode..., sets SERVICE_WAS_RUNNING=true, and on exit launchctl load`s a bus that was never running (or errors if the plist does not exist).

Fix is the $ you already used here, applied to those three sites.

### Running it supervised (macOS)

```
make install-bridge # LaunchAgent: starts at login, restarts on crash

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] guide.md:660-664 (further down, outside this diff) still reads:

Supervision is deliberately out of scope for the v1 prototype: there is no make install-bridge, launchd plist, or systemd unit yet - run the bridge in a terminal (or your own supervisor) while experimenting.

That now directly contradicts the section added here. Since guide.md is the canonical agent-event-bus://guide resource an agent reads to decide what exists, the stale paragraph is the one it may hit first. Worth trimming it to the still-true half (no systemd unit yet; tmux still needs panes.json).

Comment thread Makefile
echo "uninstall-bridge is macOS-only (LaunchAgent)."; \
exit 1; \
fi
./scripts/uninstall-bridge-launchagent.sh

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] make uninstall (documented in CLAUDE.md as "Remove everything") does not touch the bridge unit, so after make install-bridge; make uninstall the bridge LaunchAgent stays loaded, KeepAlive keeps respawning it, and it backs off forever against a bus that no longer exists — while uninstall-cli.sh has just removed the agent-event-bus-cli that the bridge teardown notes tell you to verify with. Either invoke uninstall-bridge-launchagent.sh from uninstall when the bridge plist is present, or add a line to uninstall output pointing at make uninstall-bridge.

Comment thread scripts/install-bridge-launchagent.sh Outdated

echo ""
echo "Bridge installed and running."
echo " Logs: $BRIDGE_LOG_FILE"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Logs: points at StandardOutPath, but the bridge own records are not there: main() calls logging.basicConfig(...) with no stream=, which defaults to stderr, so every logger.info/logger.debug from bridge.py lands in ...-bridge.err. The .log file receives only uvicorn access lines. The failure paths in this script already say "check $BRIDGE_ERR_FILE", which is right — but a reader following Logs: will tail the file without the bridge messages in it. Consider labelling them Access log: / Bridge log:, which also makes the plist comment caveat concrete at install time.

Comment thread scripts/install-bridge-launchagent.sh Outdated
# Give it a moment to bind and attempt registration before probing. The
# listener binds during startup; registration runs in a background thread and
# may still be backing off against a bus that is not up yet.
sleep 2

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] On a re-install over a live bridge, launchctl unload returns as soon as SIGTERM is delivered, so the outgoing process may still hold the singleton flock and port 8082 when launchctl load starts the replacement. The replacement then exits on the lock — correctly, since that ordering is exactly what protects the webhook row — and KeepAlive retries only after ThrottleInterval, i.e. up to ~10s later. The fixed sleep 2 lands inside that window, so a perfectly healthy idempotent re-install can report /health did not answer yet. Polling /health for ~15s instead of a flat sleep would let the probe report what actually happened.

Related, same area: install-bridge does not uv sync, so a stale .venv missing a bridge dependency produces an import crash-loop — the one case where the documented log-truncation caveat bites hardest, since each respawn wipes the traceback from the previous one.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Code Review — Round 1

Summary

A well-argued supervision unit. The plist mirrors the bus unit closely, and the design claims check out against the code: AGENT_EVENT_BUS_BRIDGE_BACKEND=spool is genuinely read by build_parser (bridge.py:1516), python -m agent_event_bus.bridge resolves through the existing __main__ guard, and the KeepAlive-is-safe-with-the-flock argument holds for a reason stronger than stated — main() unregisters the webhook row in its finally before closing the singleton fds, so a replacement cannot register ahead of the outgoing instance cleanup even if launchd is quick. registered: false-as-a-status-line is the right call given register_with_retry. The 137 follow-ups are clean; hoisting the session-id scrub to conftest.py is ordered correctly relative to the patch.dict/monkeypatch.setenv tests that set those names deliberately.

Findings

  • 1 Important — the new label com.evansenter.agent-event-bus-bridge is a superstring of the bus label, and the three pre-existing launchctl list | grep probes for the bus are unanchored. install-bridge-launchagent.sh anchors its own check with $; Makefile:149, scripts/install-launchagent.sh:29,48, and scripts/dev.sh:15 did not need that until this PR, and now do.
  • 4 Suggestionsmake uninstall leaves the bridge supervised; guide.md:660-664 still says make install-bridge does not exist; the installer Logs: label points at the file that does not carry the bridge own records; the unload-then-load handoff can make a healthy re-install look unhealthy inside the 2s probe.

Verdict

REQUEST_CHANGES - the bridge label silently satisfies the bus own did-it-start checks, so make restart and install-launchagent.sh can report success and exit 0 while the bus is down. Everything else is non-blocking.


Automated review by Claude Code

Round-1 review of the LaunchAgent. All five findings are defects this PR
introduced.

IMPORTANT - the new label com.evansenter.agent-event-bus-bridge is a
SUPERSTRING of the bus label, and three pre-existing probes were unanchored.
launchctl list prints the label at end-of-line, so a loaded bridge row matched
all of them. Verified: `printf 'com.evansenter.agent-event-bus-bridge' | grep -q
'com.evansenter.agent-event-bus'` succeeds.

Fails when the bridge is loaded and the bus's launchctl load leaves no job
registered (bad sed substitution, Load failed: 5, absent plist): `make restart`
and install-launchagent.sh match the BRIDGE row, print "Service restarted
successfully" / "installed and running", fire the success notification, and
exit 0 while the bus is down - the exact failure those checks exist to catch.
dev.sh has the same root cause, and would `launchctl load` a bus that was never
running.

Anchored every probe, bus and bridge alike - the bridge ones are not
exploitable today but carry the same latent trap. Note the Makefile needs $$
for a literal $: a bare $" expands as a make variable and would have dropped
the anchor silently.

Four suggestions, all taken:
- `make uninstall` ("Remove everything") left the bridge supervised, so
  KeepAlive respawned it forever against a bus that no longer existed - while
  uninstall-cli.sh had just removed the CLI the teardown notes tell you to
  verify with. It now tears the bridge down first when its plist is present.
- guide.md still said there is no `make install-bridge` or launchd plist,
  contradicting the section this PR adds - in the canonical resource an agent
  reads to decide what exists. Trimmed to the still-true half (no systemd unit;
  Linux runs its own supervisor).
- The installer's "Logs:" label pointed at StandardOutPath, but bridge.py logs
  via basicConfig with no stream=, which is stderr - so the .log carries only
  uvicorn access lines and a reader following that pointer sees no bridge
  records at all. Relabelled by what each file actually receives.
- The flat `sleep 2` before probing /health lands inside the ThrottleInterval
  window on a re-install: unload returns on SIGTERM delivery, so the outgoing
  process can still hold the flock when the replacement starts, the replacement
  exits on the lock (correctly), and KeepAlive retries ~10s later. Polls for
  20s instead, so a healthy idempotent re-install stops reporting as failed.

Also preflights `import agent_event_bus.bridge` against the venv. A stale venv
would otherwise become an import crash-loop under KeepAlive - the case where
the log-truncation caveat bites hardest, since each respawn wipes the previous
traceback. Checking rather than running `uv sync --no-dev` here, which would
silently strip dev deps from a venv just set up with `make dev`.

make check green: 576 passed. All four scripts pass bash -n.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AFp3uCezpvbeyAMYnNrQ4R
# every respawn wipes the previous traceback. Checking beats syncing: `uv sync
# --no-dev` (what install-server runs) would silently strip pytest/ruff from a
# venv someone just set up with `make dev`.
if ! PYTHONPATH="$PROJECT_DIR/src" "$VENV_PYTHON" -c "import agent_event_bus.bridge" 2>/tmp/bridge-import-check.$$; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The preflight imports agent_event_bus.bridge, but uvicorn is imported lazily inside main() (src/agent_event_bus/bridge.py:2129), not at module scope — so the one dependency whose absence would produce exactly the KeepAlive import-crash-loop this check exists to prevent is the one it cannot see. starlette / anyio / requests are module-level and are covered.

In practice uvicorn>=0.30.0 is a direct entry in [project.dependencies], so a venv that can import starlette almost certainly has it — which is why this is a Suggestion rather than a defect. Widening the check costs nothing: add , uvicorn to the -c import list.

# re-install as a failure. 20s covers the throttle with room to spare.
HEALTH=""
for _ in $(seq 1 40); do
HEALTH="$(curl -fsS --max-time 2 http://127.0.0.1:8082/health 2>/dev/null || true)"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The poll breaks on the first successful /health, which on a re-install over a live bridge is answered by the outgoing process.

launchctl unload returns once SIGTERM is delivered; the old bridge still holds port 8082 through the shielded stop-join-unregister in its lifespan. So iteration 1 gets a 200 from the instance that is about to die, HEALTH is set, the loop breaks, the launchctl list check below (which reports only whether the job is loaded, not whether a process is alive) passes, and the script prints "Bridge installed and running" plus the outgoing registered: value — while the replacement has in fact just exited on the singleton flock and will not exist for another ~ThrottleInterval.

End state is still correct (KeepAlive brings it back), so this is reporting accuracy rather than breakage. But the comment above the loop presents the poll as covering the handoff window, and breaking on the first response cannot tell the two instances apart. Telling them apart needs something instance-specific in /health (a start timestamp, or the pid), or a short unconditional wait past ThrottleInterval before the first probe.

# venv someone just set up with `make dev`.
if ! PYTHONPATH="$PROJECT_DIR/src" "$VENV_PYTHON" -c "import agent_event_bus.bridge" 2>/tmp/bridge-import-check.$$; then
echo "Error: the venv cannot import agent_event_bus.bridge:"
sed 's/^/ /' /tmp/bridge-import-check.$$

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] /tmp/bridge-import-check.$$ is a predictable path in a world-writable sticky directory, and the 2> redirect follows symlinks — a pre-planted /tmp/bridge-import-check.<pid> symlink would be truncated as the installing user. The file also leaks if the script is interrupted between creation and the rm -f.

mktemp plus a trap ... EXIT handles both, and lines up with the $TMPDIR-aware convention CLAUDE.md already documents for the bridge hook-lock dir.

keeps the surviving window useful. Giving the bridge its own file handler is
the follow-up that removes this caveat.

### Verifying supervision

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] These two new H3 sections sit between the backend code block (lines 337-340) and the - **spool**: / - **tmux**: bullet list that documents those backends (line 392 onward). The bullets now read as a continuation of "Verifying supervision" rather than of the backend intro they belong to — and the spool bullet is the longest, most load-bearing block in the section, so the misattachment is noticeable read top-to-bottom.

Moving "Running it supervised" and "Verifying supervision" to after the backend bullet list would keep the backend prose contiguous.

Comment thread CLAUDE.md
| Bridge hook-lock dir | `$XDG_RUNTIME_DIR/agent-event-bus-bridge-<uid>/`, else the system temp dir (`$TMPDIR`, or `/tmp` when unset; macOS: per-user `/var/folders/.../T`) - zero-byte, uid-scoped `hook.<hash>.lock` files, machine-scoped so a same-URL double-start refuses regardless of `$HOME`. Create-and-verified private (not adopted). Safe to remove when no bridge is running |

**Environment variables**: `AGENT_EVENT_BUS_*` prefix (e.g., `_DB`, `_LOG`, `_ERR`, `_URL`, `_AUTH_DISABLED`, `_ICON`, `_TESTING`, `_SESSION_ID`; bridge: `_BRIDGE_PORT`, `_BRIDGE_BACKEND`, `_BRIDGE_COOLDOWN`, `_BRIDGE_SECRET`, `_BRIDGE_HOOK_URL`, `_BRIDGE_BIND`, `_BRIDGE_ALLOWED_HOSTS`, `_WAKE_DIR`)
**Environment variables**: `AGENT_EVENT_BUS_*` prefix (e.g., `_DB`, `_LOG`, `_ERR`, `_URL`, `_AUTH_DISABLED`, `_ICON`, `_TESTING`, `_SESSION_ID`; bridge: `_BRIDGE_PORT`, `_BRIDGE_BACKEND`, `_BRIDGE_COOLDOWN`, `_BRIDGE_SECRET`, `_BRIDGE_HOOK_URL`, `_BRIDGE_BIND`, `_BRIDGE_ALLOWED_HOSTS`, `_BRIDGE_LOG`, `_BRIDGE_ERR`, `_WAKE_DIR`).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] _BRIDGE_LOG and _BRIDGE_ERR are listed alongside _BRIDGE_PORT, _BRIDGE_BACKEND, and the rest, but they are a different kind of variable: every other _BRIDGE_* name is read by bridge.py at runtime, whereas these two are read only by scripts/install-bridge-launchagent.sh:16-17 and baked into the plist at install time. Setting them in the environment of a running bridge does nothing.

The Operations section already carries exactly this warning for the bus _LOG / _ERR pair (they must be in the environment of make install-server itself); the bridge pair needs the same note against make install-bridge.

Related: the installer runs mkdir -p "$DATA_DIR" only, so an AGENT_EVENT_BUS_BRIDGE_LOG pointing outside the data directory leaves launchd unable to open StandardOutPath. A mkdir -p "$(dirname "$BRIDGE_LOG_FILE")" would cover it.

(`uv run` from the repo checkout: the console script lives in the project
venv - unlike `agent-event-bus-cli`, nothing symlinks it onto PATH yet.
That lands with the supervision story.)
venv - unlike `agent-event-bus-cli`, nothing symlinks it onto PATH.)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This line was correctly de-staled (dropping "That lands with the supervision story"), but the same claim survives verbatim in src/agent_event_bus/bridge.py:28-30:

(from the repo checkout - the console script lives in the project venv;
nothing puts it on PATH yet, that lands with the supervision story)

The supervision story is this PR. Worth pointing that module docstring at make install-bridge alongside the uv run form, since it is the first thing a reader of bridge.py sees.

Same class of leftover, non-blocking: tests/test_cli.py:1017 still names ~/.exports / ~/.zshrc in a docstring — the exact repo-specific paths this PR just generalized out of _session_id_from_env.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Code Review — Round 2

Summary

The single blocking finding from round 1 is fully resolved. Every launchctl list | grep probe for the bus label is now anchored — Makefile:154, scripts/install-launchagent.sh:33,53, scripts/dev.sh:15, and the new installer bus check at install-bridge-launchagent.sh:44 — and I checked the rest of the tree for stragglers: scripts/uninstall-launchagent.sh never used that pattern, so the sweep is complete.

I also re-verified the two design claims the unit comments make, at the source rather than on trust. KeepAlive is safe with the singleton lock: _acquire_singleton_locks(config) runs at bridge.py:2158, before uvicorn.run and therefore before the lifespan registration sweep. That ordering is what makes a crash-looping replacement harmless — it exits on the flock upstream of any bus mutation, so it cannot sweep away a live instance webhook row. Clean handoff: main() unregisters in its finally before closing the singleton fds, so cleanup by an outgoing instance cannot be overtaken by a new registration.

Previously Addressed (Filtered)

  • [Important] Unanchored launchctl list | grep probes for the bus label — implemented.
  • [Suggestion] make uninstall leaving the bridge supervised; stale guide.md:660 supervision note; installer Logs: label pointing at the file without bridge records; flat 2s health probe — all four implemented.

Findings

6 Suggestions, none blocking, posted inline. The most substantive: the installer import preflight cannot see uvicorn, which is imported lazily inside main() — the one dependency whose absence produces exactly the KeepAlive import-crash-loop that preflight exists to prevent. Narrow in practice, since uvicorn is a direct [project.dependencies] entry, so a venv that imports starlette will have it. The rest: the health poll answering from the outgoing instance on re-install (reporting accuracy, correct end state), a predictable /tmp path, guide.md section placement orphaning the backend bullet list, install-time-only env vars documented as runtime ones, and a stale bridge.py module docstring.

Also confirmed as non-issues: the conftest autouse scrub is ordered correctly relative to the patch.dict and monkeypatch.setenv tests that set those names deliberately, and pytest is still used in test_cli.py after the fixture moved out.

Verdict

APPROVE - No blocking findings. The supervision unit works for its stated purpose and its comments hold up against the code.


Automated review by Claude Code

@evansenter
evansenter merged commit f487b9c into main Aug 10, 2026
18 checks passed
evansenter pushed a commit that referenced this pull request Aug 10, 2026
…env docs

Round-2 review follow-ups from #139, which merged on its approval before these
were pushed. All are against code that PR added.

- The import preflight wrote stderr to a predictable /tmp path. The 2>
  redirect follows symlinks, so a pre-planted file of a guessable name would
  be truncated as the installing user, and the file leaked if the script was
  interrupted before the rm. Now mktemp under $TMPDIR with a trap on EXIT,
  matching the $TMPDIR-aware convention CLAUDE.md documents for the hook-lock
  dir.
- The preflight imported agent_event_bus.bridge only, but uvicorn is imported
  lazily INSIDE main() (bridge.py:2129), so the one dependency whose absence
  produces exactly the KeepAlive import-crash-loop the preflight exists to
  prevent was the one it could not see. Now imports both.
- The installer created only DATA_DIR, so an AGENT_EVENT_BUS_BRIDGE_LOG
  pointing outside it left launchd unable to open StandardOutPath and the job
  simply failed to start. Creates each log file's parent.
- The health poll broke on the FIRST answer, which on a re-install comes from
  the OUTGOING instance: launchctl unload returns on SIGTERM delivery, and the
  old bridge holds port 8082 through its shielded stop-join-unregister, so the
  script reported "installed and running" plus the outgoing registered: value
  while the replacement had just exited on the flock. /health carries nothing
  instance-specific to tell them apart, so wait out ThrottleInterval first -
  and only when a live instance was actually displaced, so a fresh install
  pays nothing.
- CLAUDE.md listed _BRIDGE_LOG / _BRIDGE_ERR beside the runtime _BRIDGE_*
  names, but unlike every other one they are read only by the installer and
  baked into the plist: setting them for a running bridge does nothing. Noted
  as install-time only, the same warning the bus's _LOG / _ERR pair carries.
- bridge.py's module docstring still deferred PATH/supervision to "the
  supervision story"; #139 is that story. Points at make install-bridge.

Not taken: moving the guide's two new supervision sections below the backend
bullet list. The suggestion is right that the bullets now read as a
continuation of the wrong heading, but the reflow is larger than the fix and
the section is legible as-is; worth doing deliberately rather than as a rider.

make check green: 576 passed. Installer passes bash -n.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AFp3uCezpvbeyAMYnNrQ4R
evansenter pushed a commit that referenced this pull request Aug 10, 2026
A content conflict, not a textual one. #139 added a supervision section to
guide.md's bridge docs - the section this branch moved to docs/BRIDGE.md -
and its whole point is that the claims this branch relocated are now false:
the bridge HAS an install target.

Resolved by keeping guide.md's pointer (the section lives in docs/BRIDGE.md
now) and porting every one of #139's additions there:

- "Running it supervised (macOS)" - make install-bridge / uninstall-bridge,
  the separate-unit rationale, boot ordering being a non-issue, and the
  launchd log-truncation caveat.
- "Verifying supervision" - the four manual checks, two of which test claims
  the unit's own comments make.
- The rewritten closing Supervision section (macOS only, no systemd yet).
- The PATH parenthetical, which no longer ends "yet. That lands with the
  supervision story."

Then fixed what #139 could not know was stale, because it lives in files
this branch created or had already edited:

- docs/BRIDGE.md's status banner still read "No install target, no
  supervision story".
- README's bridge paragraph still said nothing symlinks it onto PATH "until
  the supervision story lands".

One deliberate deviation from #139's text: its crash-restart check says
`webhook list` should show one row; ported as `webhook list --all`, since
this branch is what makes a row invisible to a plain listing, and "is there
exactly one row at this URL" is precisely the question `--all` exists to
answer correctly.

Verified rather than assumed: every content word of main's post-#139 bridge
section is present in docs/BRIDGE.md (the only absences are punctuation
artifacts and the word "active", removed on purpose in round 3 when the
sweep stopped being active-only). CLAUDE.md auto-merged keeping both sides.
The bridge still registers after retrying a down bus, spools a DM, and
leaves exactly one webhook row. 597 passing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N11TJkcFv2Bn4YQvbYsQ37
evansenter pushed a commit that referenced this pull request Aug 10, 2026
Both were verified wrong on the real machine after #139 merged, and both were
mine.

launchd APPENDS to StandardOutPath/StandardErrorPath; it does not truncate.
After a kill -9 respawn the bridge's .err still held the prior PID's startup
lines and both webhook entries. So the crash-loop-eats-its-own-evidence caveat
does not exist, the append-mode file handler it motivated is moot, and
ThrottleInterval's justification drops that half. The same false claim lived in
the BUS plist, which is where I copied it from - corrected there too.

/health's `registered` is the STARTUP result, cached and never re-verified, so
guide.md's boot-order check was not reproducible as written: against a bridge
that install-bridge just left registered, unloading the bus leaves /health
still reporting registered:true. The false state only appears when the bridge
itself starts bus-less. Added the missing "restart the bridge" step and stated
the corollary plainly - /health is not a bus-liveness probe; it answers "did I
register at startup", not "am I registered now". (The /health bullet further up
already said the row is not re-verified; the checklist I added contradicted it.)

The .err-vs-.log split is unaffected and still documented: bridge records go to
.err because basicConfig is stderr-only, .log carries uvicorn access lines.

Verified on the bus host alongside these: #137's fallback attributes a
non-interactive publish to the real session id (event 4638), and #139's crash
restart reclaims exactly one webhook row (stale #2 removed, #3 registered).

make check green: 576 passed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AFp3uCezpvbeyAMYnNrQ4R
evansenter added a commit that referenced this pull request Aug 10, 2026
Audit-driven cleanup. No feature work beyond one tool that closes a
half-built surface. SCHEMA_VERSION 4 -> 5; back up before deploying.

Dead code and duplication
- _get_implicit_channels deleted: it took a session_id it never read and
  always returned None. Its three tests asserted that constant; replaced by
  TestBroadcastModel, which pins the design decision instead.
- _event_to_dict and _webhook_payload built identical dicts differing only
  in "id" vs "event_id". Both now delegate to _event_wire_dict, so a field
  added for one consumer reaches the other and they cannot drift on
  signal_level.
- _preview() for duplicated truncation; storage.get_events rebuilds its
  WHERE clause from a condition list instead of four copies of one block.

call_tool no longer exits the process
It lives in cli.py but bridge.py imports it, and it called sys.exit on
failure - so the bridge had three catch sites inferring intent from
`isinstance(e, SystemExit) and e.code == 1`. It now raises
BusUnreachableError when nothing answers and lets everything else propagate
with its own type; main() owns the exit policy. Net -60 lines, and
registration retries log the real cause instead of "SystemExit(1)" for a
down bus, a 401, and a bad body alike.

Webhook pause/resume
The `active` column, the storage method, and list_webhooks(active_only=)
existed since webhooks landed, but nothing could set active=0 - so it was
always 1 and `active_only` was a distinction without a difference. Adds the
set_webhook_active tool and `webhook disable/enable <id>`. Pausing keeps the
registration, filters, and secret. storage.list_webhooks now defaults to
active_only=True, matching the tool; the unsafe direction is asymmetric.

One mechanism for schema change
_init_db only CREATEs, migrations only ALTER. Migration 5 takes over the two
inline try/except ALTERs (conditional, a no-op at v4); the duplicate
webhooks CREATE is gone since migration v3 runs for fresh installs too. Two
destructive legacy paths removed, neither replaced by silence: the
pre-rename DB location is reported rather than moved with a WAL-unsafe file
move, and a pre-RFC-#29 pid schema is refused with instructions rather than
silently DROP TABLE'd.

TestSchemaParity guards all of it: a migrated database is compared to a
fresh one on (type, notnull, pk, default) and index SQL. The one known
divergence (sessions.display_id, which migration v2 can only ADD COLUMN) is
recorded explicitly, guarded from both growth and staleness.

Docs
guide.md was 658 lines, 300 of them the experimental bridge - in a file
served as an MCP resource and paid for by every session that reads it. That
material now lives in docs/BRIDGE.md, with #139's supervision docs folded in
and the claims it invalidated corrected.

Verification
The v4 -> v5 upgrade was exercised on a database built by the previous
revision of storage.py, populated with sessions, events (meta,
correlation_id, channel), and a secret-carrying webhook: every field intact,
one version row, schema matching a fresh install. Also exercised against a
live server and bridge - CLI error paths, a bridge started before the bus
retrying and then registering, a DM reaching its wake file, and the full
webhook register/disable/list --all/enable cycle.

Six review rounds. Round 1 caught a real defect this PR introduced: the
bridge's stale-webhook sweep listed active rows only, an assumption that
held solely because nothing could set active=0 until this PR. Later rounds
were test guards and documentation accuracy.

Before deploying:
  sqlite3 ~/.claude/contrib/agent-event-bus/data.db ".backup $HOME/.claude/contrib/agent-event-bus/data.db.backup-$(date +%Y%m%d-%H%M%S)"
evansenter pushed a commit that referenced this pull request Aug 10, 2026
Round-1 review of #141 found the retraction was incomplete in exactly the file
this PR edits.

install-bridge-launchagent.sh still justified the import preflight with "the
one case where the log-truncation caveat bites hardest, since every respawn
wipes the previous traceback" - the claim this PR corrects in both plists,
guide.md and CLAUDE.md. It was the last assertion of it in the non-test
sources. The preflight is still worth having, so the justification is restated
without that half: an import crash-loop under KeepAlive respawns every
ThrottleInterval forever, never serves a delivery, and never answers /health to
say so.

README.md carried the same stale deferral the bridge.py docstring just lost -
"nothing symlinks it onto PATH until the supervision story lands" - and never
named make install-bridge. It is the only bridge mention in the README, so a
reader starting there got the pre-#139 picture.

Also renames REPLACED_LIVE_BRIDGE -> REPLACED_LOADED_JOB and states the gate's
two honest limits: `launchctl list` reports the JOB is loaded, not that a
process is alive, so a loaded-but-dead job pays the wait for no handoff; and
12s bounds ThrottleInterval, not the outgoing shutdown, which an unregister
retrying against a slow bus can outlive. Both dissolve once /health carries
something instance-specific (a pid or start timestamp) - noted in the comment
as the durable fix rather than left implicit.

make check green: 576 passed. Installer passes bash -n.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AFp3uCezpvbeyAMYnNrQ4R
evansenter pushed a commit that referenced this pull request Aug 10, 2026
…env docs

Round-2 review follow-ups from #139, which merged on its approval before these
were pushed. All are against code that PR added.

- The import preflight wrote stderr to a predictable /tmp path. The 2>
  redirect follows symlinks, so a pre-planted file of a guessable name would
  be truncated as the installing user, and the file leaked if the script was
  interrupted before the rm. Now mktemp under $TMPDIR with a trap on EXIT,
  matching the $TMPDIR-aware convention CLAUDE.md documents for the hook-lock
  dir.
- The preflight imported agent_event_bus.bridge only, but uvicorn is imported
  lazily INSIDE main() (bridge.py:2129), so the one dependency whose absence
  produces exactly the KeepAlive import-crash-loop the preflight exists to
  prevent was the one it could not see. Now imports both.
- The installer created only DATA_DIR, so an AGENT_EVENT_BUS_BRIDGE_LOG
  pointing outside it left launchd unable to open StandardOutPath and the job
  simply failed to start. Creates each log file's parent.
- The health poll broke on the FIRST answer, which on a re-install comes from
  the OUTGOING instance: launchctl unload returns on SIGTERM delivery, and the
  old bridge holds port 8082 through its shielded stop-join-unregister, so the
  script reported "installed and running" plus the outgoing registered: value
  while the replacement had just exited on the flock. /health carries nothing
  instance-specific to tell them apart, so wait out ThrottleInterval first -
  and only when a live instance was actually displaced, so a fresh install
  pays nothing.
- CLAUDE.md listed _BRIDGE_LOG / _BRIDGE_ERR beside the runtime _BRIDGE_*
  names, but unlike every other one they are read only by the installer and
  baked into the plist: setting them for a running bridge does nothing. Noted
  as install-time only, the same warning the bus's _LOG / _ERR pair carries.
- bridge.py's module docstring still deferred PATH/supervision to "the
  supervision story"; #139 is that story. Points at make install-bridge.

Not taken: moving the guide's two new supervision sections below the backend
bullet list. The suggestion is right that the bullets now read as a
continuation of the wrong heading, but the reflow is larger than the fix and
the section is legible as-is; worth doing deliberately rather than as a rider.

make check green: 576 passed. Installer passes bash -n.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AFp3uCezpvbeyAMYnNrQ4R
evansenter pushed a commit that referenced this pull request Aug 10, 2026
Both were verified wrong on the real machine after #139 merged, and both were
mine.

launchd APPENDS to StandardOutPath/StandardErrorPath; it does not truncate.
After a kill -9 respawn the bridge's .err still held the prior PID's startup
lines and both webhook entries. So the crash-loop-eats-its-own-evidence caveat
does not exist, the append-mode file handler it motivated is moot, and
ThrottleInterval's justification drops that half. The same false claim lived in
the BUS plist, which is where I copied it from - corrected there too.

/health's `registered` is the STARTUP result, cached and never re-verified, so
guide.md's boot-order check was not reproducible as written: against a bridge
that install-bridge just left registered, unloading the bus leaves /health
still reporting registered:true. The false state only appears when the bridge
itself starts bus-less. Added the missing "restart the bridge" step and stated
the corollary plainly - /health is not a bus-liveness probe; it answers "did I
register at startup", not "am I registered now". (The /health bullet further up
already said the row is not re-verified; the checklist I added contradicted it.)

The .err-vs-.log split is unaffected and still documented: bridge records go to
.err because basicConfig is stderr-only, .log carries uvicorn access lines.

Verified on the bus host alongside these: #137's fallback attributes a
non-interactive publish to the real session id (event 4638), and #139's crash
restart reclaims exactly one webhook row (stale #2 removed, #3 registered).

make check green: 576 passed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AFp3uCezpvbeyAMYnNrQ4R
evansenter pushed a commit that referenced this pull request Aug 10, 2026
Round-1 review of #141 found the retraction was incomplete in exactly the file
this PR edits.

install-bridge-launchagent.sh still justified the import preflight with "the
one case where the log-truncation caveat bites hardest, since every respawn
wipes the previous traceback" - the claim this PR corrects in both plists,
guide.md and CLAUDE.md. It was the last assertion of it in the non-test
sources. The preflight is still worth having, so the justification is restated
without that half: an import crash-loop under KeepAlive respawns every
ThrottleInterval forever, never serves a delivery, and never answers /health to
say so.

README.md carried the same stale deferral the bridge.py docstring just lost -
"nothing symlinks it onto PATH until the supervision story lands" - and never
named make install-bridge. It is the only bridge mention in the README, so a
reader starting there got the pre-#139 picture.

Also renames REPLACED_LIVE_BRIDGE -> REPLACED_LOADED_JOB and states the gate's
two honest limits: `launchctl list` reports the JOB is loaded, not that a
process is alive, so a loaded-but-dead job pays the wait for no handoff; and
12s bounds ThrottleInterval, not the outgoing shutdown, which an unregister
retrying against a slow bus can outlive. Both dissolve once /health carries
something instance-specific (a pid or start timestamp) - noted in the comment
as the durable fix rather than left implicit.

make check green: 576 passed. Installer passes bash -n.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AFp3uCezpvbeyAMYnNrQ4R
evansenter pushed a commit that referenced this pull request Aug 10, 2026
…env docs

Round-2 review follow-ups from #139, which merged on its approval before these
were pushed. All are against code that PR added.

- The import preflight wrote stderr to a predictable /tmp path. The 2>
  redirect follows symlinks, so a pre-planted file of a guessable name would
  be truncated as the installing user, and the file leaked if the script was
  interrupted before the rm. Now mktemp under $TMPDIR with a trap on EXIT,
  matching the $TMPDIR-aware convention CLAUDE.md documents for the hook-lock
  dir.
- The preflight imported agent_event_bus.bridge only, but uvicorn is imported
  lazily INSIDE main() (bridge.py:2129), so the one dependency whose absence
  produces exactly the KeepAlive import-crash-loop the preflight exists to
  prevent was the one it could not see. Now imports both.
- The installer created only DATA_DIR, so an AGENT_EVENT_BUS_BRIDGE_LOG
  pointing outside it left launchd unable to open StandardOutPath and the job
  simply failed to start. Creates each log file's parent.
- The health poll broke on the FIRST answer, which on a re-install comes from
  the OUTGOING instance: launchctl unload returns on SIGTERM delivery, and the
  old bridge holds port 8082 through its shielded stop-join-unregister, so the
  script reported "installed and running" plus the outgoing registered: value
  while the replacement had just exited on the flock. /health carries nothing
  instance-specific to tell them apart, so wait out ThrottleInterval first -
  and only when a live instance was actually displaced, so a fresh install
  pays nothing.
- CLAUDE.md listed _BRIDGE_LOG / _BRIDGE_ERR beside the runtime _BRIDGE_*
  names, but unlike every other one they are read only by the installer and
  baked into the plist: setting them for a running bridge does nothing. Noted
  as install-time only, the same warning the bus's _LOG / _ERR pair carries.
- bridge.py's module docstring still deferred PATH/supervision to "the
  supervision story"; #139 is that story. Points at make install-bridge.

Not taken: moving the guide's two new supervision sections below the backend
bullet list. The suggestion is right that the bullets now read as a
continuation of the wrong heading, but the reflow is larger than the fix and
the section is legible as-is; worth doing deliberately rather than as a rider.

make check green: 576 passed. Installer passes bash -n.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AFp3uCezpvbeyAMYnNrQ4R
evansenter pushed a commit that referenced this pull request Aug 10, 2026
Both were verified wrong on the real machine after #139 merged, and both were
mine.

launchd APPENDS to StandardOutPath/StandardErrorPath; it does not truncate.
After a kill -9 respawn the bridge's .err still held the prior PID's startup
lines and both webhook entries. So the crash-loop-eats-its-own-evidence caveat
does not exist, the append-mode file handler it motivated is moot, and
ThrottleInterval's justification drops that half. The same false claim lived in
the BUS plist, which is where I copied it from - corrected there too.

/health's `registered` is the STARTUP result, cached and never re-verified, so
guide.md's boot-order check was not reproducible as written: against a bridge
that install-bridge just left registered, unloading the bus leaves /health
still reporting registered:true. The false state only appears when the bridge
itself starts bus-less. Added the missing "restart the bridge" step and stated
the corollary plainly - /health is not a bus-liveness probe; it answers "did I
register at startup", not "am I registered now". (The /health bullet further up
already said the row is not re-verified; the checklist I added contradicted it.)

The .err-vs-.log split is unaffected and still documented: bridge records go to
.err because basicConfig is stderr-only, .log carries uvicorn access lines.

Verified on the bus host alongside these: #137's fallback attributes a
non-interactive publish to the real session id (event 4638), and #139's crash
restart reclaims exactly one webhook row (stale #2 removed, #3 registered).

make check green: 576 passed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AFp3uCezpvbeyAMYnNrQ4R
evansenter pushed a commit that referenced this pull request Aug 10, 2026
Round-1 review of #141 found the retraction was incomplete in exactly the file
this PR edits.

install-bridge-launchagent.sh still justified the import preflight with "the
one case where the log-truncation caveat bites hardest, since every respawn
wipes the previous traceback" - the claim this PR corrects in both plists,
guide.md and CLAUDE.md. It was the last assertion of it in the non-test
sources. The preflight is still worth having, so the justification is restated
without that half: an import crash-loop under KeepAlive respawns every
ThrottleInterval forever, never serves a delivery, and never answers /health to
say so.

README.md carried the same stale deferral the bridge.py docstring just lost -
"nothing symlinks it onto PATH until the supervision story lands" - and never
named make install-bridge. It is the only bridge mention in the README, so a
reader starting there got the pre-#139 picture.

Also renames REPLACED_LIVE_BRIDGE -> REPLACED_LOADED_JOB and states the gate's
two honest limits: `launchctl list` reports the JOB is loaded, not that a
process is alive, so a loaded-but-dead job pays the wait for no handoff; and
12s bounds ThrottleInterval, not the outgoing shutdown, which an unregister
retrying against a slow bus can outlive. Both dissolve once /health carries
something instance-specific (a pid or start timestamp) - noted in the comment
as the durable fix rather than left implicit.

make check green: 576 passed. Installer passes bash -n.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AFp3uCezpvbeyAMYnNrQ4R
evansenter added a commit that referenced this pull request Aug 10, 2026
#141)

#139's round-2 review follow-ups, plus corrections to two claims that running
#139 on the real bus host proved false.

launchd APPENDS to StandardOutPath/StandardErrorPath rather than truncating, so
the crash-loop-eats-its-own-evidence caveat and the append-mode file handler it
motivated are both moot. The same false claim lived in the bus plist and is
corrected there too. The .err-vs-.log split stands, restated on its actual
cause: basicConfig is stderr-only.

/health's `registered` is the result of the last registration attempt, never
re-checked against the bus afterwards, so the boot-order check was not
reproducible as written - unloading the bus under an already-registered bridge
leaves /health reporting true. Added the missing restart step and the corollary:
/health is not a bus-liveness probe.

Installer hardening: mktemp+trap for the preflight stderr capture (the old
predictable path let `2>` follow a symlink), uvicorn named explicitly because
bridge.py imports it lazily inside main(), mkdir -p for log parents, and a
health poll that waits out ThrottleInterval only when a live instance was
actually displaced.

Docs corrections land in docs/BRIDGE.md rather than guide.md: #138 moved the
bridge operator docs there while this branch was open.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants