Harden the bridge installer; correct two claims the bus host disproved

[evansenter]

#139's round-2 review follow-ups (they merged on its approval before they were pushed), plus corrections to two claims that running #139 on the real bus host proved false — both of them mine.

Rebased onto ffdfcec (#143). Note the docs corrections now land in docs/BRIDGE.md, not guide.md: #138 moved the bridge operator docs there while this branch was open, and the rebase resolved my original guide.md hunks away with the section they lived in. Same corrections, re-landed in the file the content moved to.

Corrections from the host

launchd appends; it does not truncate. After a kill -9 respawn, the bridge's .err still held the previous PID's startup lines and both webhook entries. So:

• The "crash loop eats its own evidence" caveat I documented doesn't exist.
• The append-mode file handler it motivated is moot.
• ThrottleInterval's stated justification loses that half.

The same false claim lived in the bus plist — which is where I copied it from — so it's corrected there too. The .err-vs-.log split is unaffected and now stated on its actual cause: basicConfig is stderr-only.

/health's registered is cached, so my boot-order check wasn't reproducible. It reports the startup result and is never re-verified. Against a bridge that make install-bridge just left registered, unloading the bus leaves /health still saying 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".

Notably the /health bullet further down that same file already said the row isn't re-verified — the checklist I added in #139 contradicted a caveat sitting a few hundred lines above it.

Installer hardening

Round-2 suggestions, all against code #139 added:

• /tmp symlink hazard — the import preflight wrote stderr to a predictable path. 2> follows symlinks, so a pre-planted file would be truncated as the installing user, and it leaked on interrupt. Now mktemp under $TMPDIR with a trap.
• uvicorn invisible to the preflight — it's imported lazily inside main(), so the one dependency whose absence causes the crash-loop the preflight exists to prevent was the one it couldn't see.
• mkdir -p for log parents — an AGENT_EVENT_BUS_BRIDGE_LOG outside the data dir left launchd unable to open StandardOutPath, so the job just failed to start.
• Health poll waited on the wrong instance — it broke on the first answer, which on a re-install comes from the outgoing process still holding port 8082 through its shielded stop-join-unregister. Now waits out ThrottleInterval, but only when a live instance was actually displaced.
• _BRIDGE_LOG / _BRIDGE_ERR documented as install-time only — unlike every other _BRIDGE_* name they're read by the installer and baked into the plist; setting them for a running bridge does nothing.
• bridge.py's module docstring no longer defers PATH/supervision to "the supervision story" that Supervise the RFC #122 bridge as a LaunchAgent #139 is.

Declined: reflowing the supervision sections below the backend bullet list. The suggestion is right that the bullets now attach to the wrong heading, but that reflow is larger than the fix and worth doing deliberately rather than as a rider — and #138 has since moved the whole file, so it wants doing against the new layout.

Verified on the host, not just here

• Fall back to CLAUDE_CODE_SESSION_ID for CLI session attribution #137's fallback attributes a non-interactive zsh -c publish to the real session id (event 4638) — precondition confirmed inside the subshell (AGENT_EVENT_BUS_SESSION_ID empty, CLAUDE_CODE_SESSION_ID present).
• Supervise the RFC #122 bridge as a LaunchAgent #139 crash restart: respawn in ~1s, webhook list shows exactly one row, log reads Removed stale bridge webhook #2 → Registered bridge webhook #3.
• Supervise the RFC #122 bridge as a LaunchAgent #139 boot order: registered: false while bus-less, flipped to true in 5s unaided.

make check green on the rebase: 649 passed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AFp3uCezpvbeyAMYnNrQ4R

[claude]

[Suggestion] The retracted truncation claim survives here. This PR corrects "launchd TRUNCATES" → "launchd APPENDS" in both plists, guide.md, and CLAUDE.md — but this comment still reads "the one case where the log-truncation caveat bites hardest, since every respawn wipes the previous traceback", and it is now the last place in the repo asserting it (grep -rn truncat over the non-test sources finds only this line and the unrelated mktemp rationale two lines down).

The preflight is still worth having — an import crash-loop under KeepAlive is a bad state regardless of whether the log survives it — so the justification just needs restating without the truncation half, the same way ThrottleInterval's comment in the plist was rewritten.

[claude]

[Suggestion] The gate is a real improvement over probing immediately. Two things worth noting as follow-ups rather than fixes here:

1. REPLACED_LIVE_BRIDGE is set from launchctl list | grep -q "$LABEL$", which reports that the job is loaded — not that a process is alive. A job that is loaded but currently dead (crashed and sitting out its throttle, or a prior install left it loaded with nothing running) takes the full 12s wait for no handoff. Cosmetic, but the name promises more than the check delivers.
2. 12s bounds ThrottleInterval, not the outgoing instance shutdown. The comment is right that the port is held through the shielded stop-join-unregister — if that unregister retries against a slow or unreachable bus and outlives the sleep, the poll first answer still comes from the dying instance and the script prints its registered: value. The same shape exists on the fresh-install path when an unsupervised uv run agent-event-bus-bridge already holds 8082: the LaunchAgent instance refuses on the hook lock, launchctl list still shows the job loaded, and /health answers from the manual process.

The durable fix is the one this comment already identifies as missing: /health carries nothing instance-specific. Adding pid (or a start timestamp) to the payload would let the poll assert it is talking to the new process and drop the fixed sleep entirely.

Also non-blocking: none of the installer changes here are covered by a test — the repo has no harness for scripts/*.sh, so that is a gap in the project shape rather than in this PR.

[claude]

[Suggestion] Good call removing the forward-reference to "the supervision story" from the PR that is it. The same stale deferral is still in README.md:111-113:

Run it as uv run agent-event-bus-bridge from the checkout - unlike the CLI, nothing symlinks it onto PATH until the supervision story lands.

That is the only bridge mention in the README, and it never names make install-bridge — so a reader starting there gets the pre-#139 picture. Worth the same one-line update this docstring just got.

[claude]

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

Code Review — Round 1

Summary

Two empirically-grounded doc corrections (launchd appends rather than truncates; /health registered is the startup result, not a bus-liveness probe) plus four installer hardening fixes. I checked the code-side claims against the tree:

• uvicorn in the preflight — confirmed: bridge.py:2130 imports it lazily inside main(), and it is a top-level project dependency, so a stale venv really could pass the old check and then crash-loop. The preflight also matches the runtime environment the plist sets (PYTHONPATH=<project>/src, venv python, -m agent_event_bus.bridge), so it exercises the same import path.
• /health correction — confirmed: registered is derived from registration_state["webhook_id"] at bridge.py:1186, and that state is only written by startup registration and shutdown unregistration. "Startup result, never re-verified" is accurate, and the new step-2 wording in guide.md now agrees with the /health bullet at guide.md:597 instead of contradicting it.
• mktemp + trap — a nice side effect beyond the symlink fix: the old rm -f sat only on the success path, so the early exit 1 on import failure leaked the file it had just written. The EXIT trap covers both paths.
• launchd append semantics — matches launchd opening StandardOutPath/StandardErrorPath in append mode, and the host observation is the right way to have settled it.

Findings

Three Suggestions, all posted inline. The highest-value one: the retracted truncation claim survives in the one place this PR did not sweep — scripts/install-bridge-launchagent.sh:30 still justifies the preflight with "every respawn wipes the previous traceback", which is exactly the claim corrected in both plists, guide.md, and CLAUDE.md.

Verdict

APPROVE - No blocking findings. Nothing here names a way the code produces a wrong result; the remaining feedback is documentation consistency and a follow-up on the health-poll heuristic.

---

Automated review by Claude Code

[claude]

[Suggestion] The 12s pre-wait makes the failure message downstream misleading. Line 143 still reads:

/health did not answer within 20s; check $BRIDGE_ERR_FILE.

On the re-install path the script has already been waiting 12s before the poll starts, so an operator who sees that line has actually been sitting for ~32s. The 20s was loose in the other direction too: each iteration is curl --max-time 2 plus sleep 0.5, so 40 iterations bound at 20s only when every probe fails instantly, and at ~100s when they all hang.

Cheapest fix is to drop the number entirely: /health did not answer; check $BRIDGE_ERR_FILE. The elapsed time is not what the operator needs — the log path is.

[claude]

[Suggestion] "registered is the startup result and is never re-verified" sits two lines below "within ~30s it flips to true with no intervention". As written the two read as contradicting each other — the same shape as the contradiction this PR is fixing.

Both halves are right, they just need different words. registration_state["webhook_id"] (bridge.py:1186) is written after startup, by the register_with_retry background thread — which is exactly what makes the flip happen. What never happens is re-validation against the bus once that thread succeeds and stops.

Wording that keeps the corollary without appearing to deny the flip the step above depends on: "the result of the last registration attempt, never re-checked against the bus afterwards".

[claude]

[Suggestion] The preflight now hand-mirrors the runtime lazy-import set, and nothing keeps the two in sync.

Today that set is exactly one name — a grep for indented import lines in bridge.py returns only import uvicorn at line 2125 — so the check is complete as written, and the comment above correctly explains why uvicorn had to be named. But a second deferred import added inside main() later silently reopens precisely the hole this commit closed, and the symptom is the same crash-loop with no failing install to point at it.

A one-line pointer in the comment (keep in sync with the deferred imports in bridge.py:main()) is probably the whole fix. Having the preflight actually drive main() far enough to trigger them costs more than it buys.

[claude]

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

Code Review — Round 2

Summary

Round 1 raised three Suggestions and all three are resolved: the retracted truncation claim is gone from the installer preflight comment (a grep for truncat over the non-test sources now finds only the unrelated mktemp rationale), REPLACED_LIVE_BRIDGE is renamed REPLACED_LOADED_JOB with both honest limits written into the comment, and README.md:111-115 now names make install-bridge instead of deferring to the supervision story. The corrections themselves re-verify against the tree.

Verified this round

• Preflight completeness — a grep for indented import lines in bridge.py returns exactly one hit, import uvicorn at line 2125. So import agent_event_bus.bridge, uvicorn is the complete lazy-import set, not a partial patch, and it matches the runtime environment the plist sets (venv python, PYTHONPATH=<project>/src, -m agent_event_bus.bridge).
• The /health correction — bridge.py:1186 derives registered from registration_state.get("webhook_id"), written only by the registration thread and popped on unregister. Nothing re-checks it against the bus, so the corollary holds.
• The .err / .log split — basicConfig in main() gets no stream=, so bridge records go to stderr; uvicorn.run configures only its own loggers and routes uvicorn.access to stdout. The plist comment, CLAUDE.md, docs/BRIDGE.md, and the installer Logs: labelling all agree.
• No stale survivors — the corrections moved out of guide.md into docs/BRIDGE.md cleanly, and guide.md retains no bridge-log or /health claims left to contradict them.
• mkdir -p for the log parents — correctly ordered before the plist is written, and covers both overridable paths.

Findings

Three Suggestions, posted inline: a stale within-20s in the poll-failure message now that the re-install path waits 12s first, a wording tension in the docs/BRIDGE.md boot-order step between never re-verified and the flip it depends on, and a drift note on the preflight hand-mirroring the lazy-import set. Nothing names a way the code produces a wrong result — the installer changes are additive guards, and the doc changes replace claims the host disproved with ones it confirmed.

Previously Addressed (Filtered)

Not re-raised: the launchctl list job-loaded-versus-process-alive gap and the 12s-bounds-throttle-not-shutdown gap (round 1, Suggestion — both now documented in the comment as accepted limits, with the instance-specific /health payload named as the durable follow-up); the absence of a test harness for scripts/*.sh (round 1, Suggestion — a project-shape gap rather than a gap in this PR).

Verdict

APPROVE - No blocking findings. All remaining feedback is documentation-consistency polish.

---

Automated review by Claude Code

[claude]

[Suggestion] The 12 is a bare literal that has to stay >= the plist’s ThrottleInterval, but nothing links the two. com.evansenter.agent-event-bus-bridge.plist sets <integer>10</integer>; if that is ever raised (say to 30, to slow a noisy crash loop), this gate silently under-waits and the poll is answered by the dying instance again — the exact failure the gate was added to close, reintroduced by an edit in a different file. The comment above explains why 12, but a reader editing the plist has no reason to come here.

Cheap options: name the coupling in the plist comment too ("install-bridge-launchagent.sh waits this out on re-install — keep them in sync"), or hoist THROTTLE_INTERVAL=10 to the top of the script next to LABEL and sleep the sum with the margin visible as a margin.

Secondary, same neighbourhood: the failure message at line 143 still reads "/health did not answer within 20s". On the re-install path the user has now waited ~32s by then. "within 20s of the restart throttle", or just dropping the number, would keep it honest.

(Both dissolve alongside the fixed sleep once /health carries a pid or start timestamp, as the comment already identifies.)

[claude]

[Suggestion] The install-time-only nature of AGENT_EVENT_BUS_BRIDGE_LOG / _ERR landed in CLAUDE.md but not here, and this paragraph is exactly where an operator lands when they want to move these files. BRIDGE.md is the operator doc; CLAUDE.md is the doc for whoever is editing the repo — the reader who needs "you must set this on make install-bridge itself, not on the running bridge" is the former, and they have no reason to open the latter.

One sentence appended here would do it: both paths default under the data dir and are overridable via AGENT_EVENT_BUS_BRIDGE_LOG / _ERR, which the installer reads and bakes into the plist — so they must be in the environment of make install-bridge, and setting them for a running bridge does nothing.

[claude]

[Suggestion] While this row is being rewritten: it names four log files, but the bus actually writes three of its own and the third is missing. com.evansenter.agent-event-bus.plist hardcodes StandardOutPath to agent-event-bus.stdout (not substituted, unlike __ERR_FILE__), so the bus has agent-event-bus.log (the Python FileHandler, server.py:77), agent-event-bus.err (launchd stderr capture) and agent-event-bus.stdout (launchd stdout capture) — the last undocumented, and the one file AGENT_EVENT_BUS_LOG cannot move.

Minor, but it rubs against the correction this PR is making: the new plist comment says "launchd APPENDS to these" pointing at .stdout and .err, while this row names .log and .err. A reader reconciling the two finds a file in each place that is absent from the other.

Separately, that plist comment says "verified empirically" without naming the evidence, where the bridge plist cites the specific kill -9 observation. Same launchd behaviour so the generalisation is sound — saying it was verified on the bridge’s files would make the provenance auditable the same way the bridge’s does.

[claude]

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

Code Review — Round 2

Summary

Round 1 s three Suggestions all landed. I verified each against the tree rather than taking the diff s word for it:

• Truncation claim fully retracted. grep -rn truncat over src/, scripts/, docs/, CLAUDE.md, README.md now returns only install-bridge-launchagent.sh:35 (the unrelated mktemp symlink rationale) plus unrelated payload-truncation hits in server.py/middleware.py/tests. The installer comment round 1 flagged as the last holdout — "every respawn wipes the previous traceback" — is gone, restated as "respawning every ThrottleInterval forever, never serving a delivery, with /health never answering to say so". grep -rn "append-mode\|file handler\|file logger" finds only the two places that now deny needing one.
• README updated. README.md:111-115 names make install-bridge and points at docs/BRIDGE.md; the "until the supervision story lands" deferral is gone repo-wide (grep -rn "supervision story" returns no hits).
• REPLACED_LIVE_BRIDGE renamed to REPLACED_LOADED_JOB, and both limits round 1 named are now written into the comment above the gate rather than left implicit. Renaming to match what launchctl list actually reports is the right call.

Spot-checks on the corrections themselves:

• .err vs .log split — confirmed. bridge.py:2133 calls basicConfig with no stream= (stderr), and bridge.py:2156 runs uvicorn.run with the default log config, whose access handler targets stdout and whose disable_existing_loggers is False, so the root stderr handler survives the dictConfig. Bridge records land in .err, uvicorn access in .log, as documented in all three places.
• Bus plist comment — server.py:77 is logging.FileHandler(LOG_FILE), default mode a, and AGENT_EVENT_BUS_LOG is threaded through the plist s EnvironmentVariables. "The Python logger also appends to its own .log file" is accurate.
• _BRIDGE_LOG / _BRIDGE_ERR install-time only — confirmed: zero hits under src/; the only readers are install-bridge-launchagent.sh:16-17 and the plist placeholders.
• /health correction — bridge.py:1186 derives registered from registration_state["webhook_id"], written only by startup registration and shutdown unregistration. The new step-2 wording now agrees with the /health bullet at docs/BRIDGE.md:299-302 instead of contradicting it.
• mktemp plus trap — the EXIT trap covers the early exit 1 path the old rm -f missed, on top of the symlink fix. The uvicorn addition matches the plist s runtime environment (PYTHONPATH=<project>/src, venv python), so it exercises the real import path.

Previously Addressed (Filtered)

All three round-1 Suggestions are implemented, so none are re-raised. The round-1 note that scripts/*.sh has no test harness is a project-shape gap already stated, likewise not re-raised.

Findings

Three Suggestions, posted inline: the 12 literal s undeclared coupling to the plist s ThrottleInterval (plus the now-understated "within 20s" message), the install-time-only note landing in CLAUDE.md but not the operator doc, and the undocumented agent-event-bus.stdout that the reworded log-files row still omits. Nothing names a way this produces a wrong result.

Verdict

APPROVE - No blocking findings. The two corrections are load-bearing — they retract a caveat that was motivating a follow-up that would have been pure cost — and the installer hardening is sound.

---

Automated review by Claude Code

[claude]

[Suggestion] The analogy in the closing clause is the one part of this paragraph that does not hold to the standard the rest of the PR is applying.

_BRIDGE_LOG / _BRIDGE_ERR are install-time only in the strong sense: zero readers under src/, so they do nothing at runtime in any launch context. The bus pair is not symmetric with that:

• AGENT_EVENT_BUS_LOG is read at runtime - server.py:64 resolves LOG_FILE from it at import, and server.py:76 even creates its parent, so DEV_MODE=1 AGENT_EVENT_BUS_LOG=/tmp/x.log agent-event-bus really does relocate the log. It is install-time-only under launchd, because the plist bakes it into EnvironmentVariables - a property of the supervision, not of the variable.
• AGENT_EVENT_BUS_ERR has no Python reader at all, so it genuinely is the same shape as the bridge pair.

So the clause is right for _ERR and overstated for _LOG. Wording like "as with the bus _ERR (and, under launchd, _LOG)" keeps the point without flattening the distinction.

Minor on its own - but this PR exists because a claim was carried between files without being re-checked, which is why it seemed worth naming rather than passing over.

[claude]

[Suggestion] Good fix, and it makes the identical gap in the bus installer visible by contrast: scripts/install-launchagent.sh:29-30 does mkdir -p "$HOME/Library/LaunchAgents" and mkdir -p "$HOME/.claude" - never $HOME/.claude/contrib/agent-event-bus, and never the parents of an overridden AGENT_EVENT_BUS_LOG / _ERR. That plist writes StandardErrorPath to __ERR_FILE__ and StandardOutPath to a hardcoded .stdout under the data dir. install-systemd.sh:27 does create the data dir, so the launchd bus installer is the outlier of the three.

In practice the bus mostly survives it because server.py:76 creates LOG_FILE.parent - but that runs after launchd has already tried to open its two capture files, so on a genuinely fresh box it is the first run .err / .stdout that are lost. Out of scope for this PR; worth a one-liner mirroring line 67 whenever that file is next touched.

Second, smaller: the comment asserts that the job simply fails to start when the parent is missing. Every other launchd behavioural claim this PR touches now carries its evidence (the kill -9 observation, cited in both plists) precisely because an uncited one turned out to be false. This one is uncited, and the real behaviour may be the softer variant - launchd logging the open failure and the job running with those descriptors unusable. Verifying it on the host, or softening the wording to "may fail to start", would keep the claim hygiene in this file uniform.

[claude]

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

Code Review — Round 4

Summary

The latest push (3ee0587) is comment- and doc-only, and it closes all three round-3 Suggestions: the 12 / ThrottleInterval coupling is now stated in the plist comment as well as the script, the install-time-only note for _BRIDGE_LOG / _ERR landed in docs/BRIDGE.md (the operator doc) alongside CLAUDE.md, and the bus third log file agent-event-bus.stdout is now named in the log-files row. The stale "within 20s" message and the boot-order wording tension are resolved too.

Re-verified against the tree rather than taken on trust:

• Preflight completeness - a grep for indented import / from lines in bridge.py still returns exactly one hit, import uvicorn at bridge.py:2125. So import agent_event_bus.bridge, uvicorn remains the complete deferred-import set, and the new keep-in-sync pointer sits directly above it.
• The .stdout claim - scripts/com.evansenter.agent-event-bus.plist:47 hardcodes the .stdout path with no placeholder, while StandardErrorPath is __ERR_FILE__ and AGENT_EVENT_BUS_LOG is threaded through EnvironmentVariables. "The one file no env var relocates" is accurate.
• The Python-logger half - server.py:64 resolves LOG_FILE from AGENT_EVENT_BUS_LOG, server.py:77 is a default-mode (append) FileHandler. The plist new "a THIRD file, not the Python logger .log" note holds.
• /health corollary - bridge.py:1186 still derives registered from registration_state, written only by the registration thread and popped on unregister. The reworded "result of the last registration attempt, never re-checked against the bus" now covers the retry-thread flip that step 2 depends on, without appearing to deny it.

Previously Addressed (Filtered)

Not re-raised: the launchctl list job-loaded-versus-process-alive gap and the 12s-bounds-throttle-not-shutdown gap (rounds 1-2, both now written into the comment as accepted limits, with the instance-specific /health payload named as the durable fix); hoisting THROTTLE_INTERVAL into a variable (round 3 - the author took the bidirectional-comment option, which addresses the same drift); the absence of a test harness for scripts/*.sh (a project-shape gap, not a gap in this PR).

Findings

Two Suggestions, posted inline: the _LOG half of the new install-time-only analogy in CLAUDE.md is overstated (server.py:64 reads it at runtime - it is launchd that makes it install-time), and the bus installer still lacks the mkdir -p for log parents that this PR just added for the bridge, plus one uncited launchd claim in the new comment. Nothing here names a way the code produces a wrong result, and this push changes no executable line.

Verdict

APPROVE - No blocking findings. Remaining feedback is documentation precision and an out-of-scope parallel in the bus installer.

---

Automated review by Claude Code