Skip to content

Consume agent-event-bus session attribution (#128) and signal levels (#129) from the hooks - #328

Merged
evansenter merged 5 commits into
mainfrom
claude/event-bus-session-attribution
Aug 9, 2026
Merged

Consume agent-event-bus session attribution (#128) and signal levels (#129) from the hooks#328
evansenter merged 5 commits into
mainfrom
claude/event-bus-session-attribution

Conversation

@evansenter

@evansenter evansenter commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Wires the hooks up to the two features that landed in evansenter/agent-event-bus#132 — without this, both deliver nothing in practice.

Session attribution (closes the dotfiles half of agent-event-bus#128)

.exports maps CLAUDE_CODE_SESSION_IDAGENT_EVENT_BUS_SESSION_ID when running inside Claude Code. Verified empirically that CLAUDE_CODE_SESSION_ID is present in Bash-tool subprocess environments, and the two ids are the same value by construction: session-start.sh registers on the bus with client_id = the CC session id, which the bus adopts as its session id. The bus CLI reads the env var when --session-id is omitted (both publish and events), so subagent and workflow-step publishes stop being anonymous. Defensive no-op outside Claude Code or on versions that don't expose the variable.

Server-side signal levels (consumes agent-event-bus#129)

session-start.sh's recent-events fetch switches from --exclude session_registered,session_unregistered to --min-level info — one canonical noise policy on the server, and it now also drops the ci_watching/task_started/ci_rerun churn the old two-type list let through.

Deliberately not migrated: the shared eventbus-collect.sh lib keeps its client-side EB_EXCLUDE denylist, now with a comment explaining why. The Stop-hook drain's bounded consume (peek N filtered events, consume with --limit N) requires peek and consume to agree on the raw event window, which client-side --exclude guarantees and server-side --min-level does not — a --min-level consume bounded to the filtered count would advance the cursor past a different raw window than the peek saw, risking consumed-but-never-surfaced events. Migrating that path needs a cursor-ack primitive on the bus (filed as agent-event-bus#134).

Verification of the .exports mapping (round-1/2 Important)

The review correctly noted that CLAUDE_CODE_SESSION_ID being present is the precondition, not the outcome — the mapping only fires if ~/.exports is evaluated in a shell where the variable is already set. Verified end-to-end in a live Claude Code session (Bash tool subprocess):

  • printenv CLAUDE_CODE_SESSION_ID → present (067fd316-…, the live session's id)
  • evaluating the .exports mapping in that shell exports AGENT_EVENT_BUS_SESSION_ID with the identical value
  • the bus CLI's env-attribution path therefore attributes to 067fd316-… — the same id the SessionStart hook registers as client_id, so publishes land non-anonymous under the registered session

Caveat: that session runs in a Linux container, not the macOS login-shell chain this repo actually configures, so the one machine-specific link (does your Bash-tool startup snapshot evaluate .exports after the session id is injected?) deserves a one-line spot check after merging: run printenv AGENT_EVENT_BUS_SESSION_ID in a Bash tool call on the Mac and compare it with the id in the SessionStart "Registered on event bus as:" line. If it comes back empty, the fallback is the bus-side one the review sketched (CLI reading CLAUDE_CODE_SESSION_ID directly) — a follow-up PR in agent-event-bus, not this one.

Round-1 review fixes (0274e91)

  • Version-skew fallback in session-start.sh: retry once with the legacy client-side --exclude denylist when the --min-level fetch fails on an out-of-date CLI, so such a machine degrades to extra noise instead of silently losing all startup context.
  • README: the split noise policy (server-side --min-level for session-start, client-side EB_EXCLUDE for the drain path) is now documented in the Event-drain architecture section, with a pointer to the lib rationale and agent-event-bus#134; the session-start entry describes the new fetch and its fallback.
  • Argv-pinning tests: both mock CLIs record events argv to MOCK_EVENTS_ARGS_FILE when set; test_session_start_uses_min_level asserts --min-level info is passed and no --exclude fallback fires, test_eventbus_lib_uses_client_side_exclude asserts eb_fetch_events keeps --exclude and never migrates to --min-level — pinning both sides of the policy split.

Round-2 review fixes (89cecd2)

  • Fallback is exit-code-gated: it now fires only when the first fetch exits 2 (argparse usage error — the flag wasn't understood), not on any empty result. A down/timed-out bus exits 1, where a retry can't help and would make every offline session start pay a second ~200ms timeout.
  • Fallback path is tested: the mock can simulate an old CLI (MOCK_REJECT_MIN_LEVEL=1 → usage error on stderr, exit 2, after recording argv); test_session_start_min_level_fallback asserts events are still surfaced and the argv file shows the failed --min-level attempt followed by the --exclude retry, in order.

Testing

bash -n clean on all changed scripts; test mocks updated to accept --min-level, record argv, and simulate an old CLI; three new tests registered (session-start noise policy, fallback path, drain-lib denylist). tests/test-hooks.sh was not runnable in this Linux container (it hangs before producing output — appears to expect a macOS/interactive environment), so CI's Hooks job is the authoritative run; please also give it one local run before merging.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AFp3uCezpvbeyAMYnNrQ4R


Generated by Claude Code

- .exports maps CLAUDE_CODE_SESSION_ID -> AGENT_EVENT_BUS_SESSION_ID for
  Claude Code shells, so subagent/workflow publishes and polls are
  attributed to the owning session instead of 'anonymous'
  (agent-event-bus#128). The two ids are the same value: session-start
  registers with client_id = the CC session id, which the bus adopts.
  No-op outside Claude Code.
- session-start.sh fetches its recent-events context with
  --min-level info (server-side, agent-event-bus#129) instead of a local
  two-type denylist - also drops ci_watching/task_started churn it
  previously let through
- eventbus-collect.sh documents why the shared drain path keeps the
  client-side EB_EXCLUDE list for now: the Stop hook's bounded consume
  needs peek and consume to agree on the raw event window, which
  server-side --min-level breaks until the bus grows a cursor-ack
  primitive
- test mocks accept --min-level

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AFp3uCezpvbeyAMYnNrQ4R

@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

Note: the gh api inline-comment submission was blocked by the sandbox command filter in this environment (heredoc JSON rejected, temp-file write denied), so findings are listed here with file:line references instead of as inline comments.

Summary

Wires the hooks to agent-event-bus session attribution (#128) and server-side signal levels (#129): .exports maps CLAUDE_CODE_SESSION_ID to AGENT_EVENT_BUS_SESSION_ID, and session-start.sh switches its recent-events fetch to --min-level info. The decision to keep the client-side EB_EXCLUDE denylist in the shared drain lib is well-reasoned and well-documented — the peek/consume cursor-window argument is correct given the drain's bounded-consume design, and filing agent-event-bus#134 for the cursor-ack primitive is the right call. CI is green, including the Hooks job that covers the tests/test-hooks.sh run the PR body asked for.

Findings

[Important] home/.exports:17 — Verification gap on the PR's core mechanism. The PR body verifies that CLAUDE_CODE_SESSION_ID is present in Bash-tool subprocess environments — but that's the precondition, not the outcome. For this mapping to fire, ~/.exports must be evaluated in a shell where the variable is already set. The shell that launched Claude Code sourced .exports before any session id existed, and the Bash tool initializes from a startup-time shell snapshot plus process env — whether the profile chain re-runs with the injected variable in scope is exactly the unverified link. If it doesn't, this is a silent no-op: publishes stay anonymous (the bug this PR exists to fix) while looking fixed, since there's no failure signal. Please verify end-to-end in a live session and note it in the PR body — e.g. a Bash tool call running printenv AGENT_EVENT_BUS_SESSION_ID and confirming it matches the session id shown by the SessionStart registration, and ideally one subagent/workflow-step publish landing non-anonymous on the bus. If it turns out the snapshot doesn't re-evaluate .exports, the fallback is straightforward (e.g. have the CLI also read CLAUDE_CODE_SESSION_ID directly, bus-side), but that's a different PR than this one.

[Suggestion] home/.claude/hooks/session-start.sh:105 — Version-skew degradation is silent and total here. On a machine whose agent-event-bus-cli predates --min-level (this repo syncs across several machines and the CLI ships from a different repo), the call now errors, stderr is suppressed, and EVENTS comes back empty — indistinguishable from no recent events, so the session quietly loses all startup context rather than just seeing extra noise. Consider a fallback retry with the old --exclude form when the first call produces nothing, or at least a note in the PR/README that the bus CLI must be upgraded first (deploy-ordering requirement).

[Suggestion] home/.claude/hooks/lib/eventbus-collect.sh:44 — This rationale comment is excellent, but it now describes a deliberate split noise policy — session-start.sh uses server-side --min-level while both drain-path hooks stay on client-side EB_EXCLUDE — and the hooks README doesn't mention it. The README's Event-drain architecture section (and the session-start.sh entry, which just says "last 20, newest first") is where the next reader will look; a two-line pointer there to this comment would prevent someone from "unifying" the drain path onto --min-level and reintroducing the consumed-but-never-surfaced hazard. CLAUDE.md asks for a README update when modifying hooks.

[Suggestion] tests/test-hooks.sh:191 — The mock updates only make the new flag tolerated — nothing asserts session-start.sh actually passes --min-level info (or that it no longer passes --exclude), and nothing would catch a drift back to the local denylist, or the reverse hazard of eb_fetch_events migrating to --min-level (which the lib comment explains would break the drain's bounded consume). A cheap assertion — have the mock record its argv to a file and check for --min-level in the session-start test and --exclude in the drain test — would pin both sides of the policy split.

Verdict

REQUEST_CHANGES - One Important finding: please add end-to-end evidence that the .exports mapping takes effect in a live Bash-tool subprocess (a one-line check settles it).


Automated review by Claude Code

…-pinning tests

Three of the four round-1 findings (the fourth - live-session verification
of the .exports mapping - is addressed in the PR body):

- session-start.sh: version-skew fallback. A CLI predating --min-level
  (agent-event-bus#129) errors with stderr suppressed, leaving EVENTS
  empty; since the CLI prints "No events" when there are genuinely none,
  empty means the flag wasn't understood - retry once with the legacy
  client-side --exclude denylist so an out-of-date machine degrades to
  extra noise instead of losing all startup context.

- hooks README: document the deliberately split noise policy -
  session-start filters server-side (--min-level info, non-consuming
  read), the drain lib keeps client-side EB_EXCLUDE (peek/consume must
  agree on the same raw event window) - with a pointer to the
  eventbus-collect.sh rationale and agent-event-bus#134 (cursor-ack).
  Also update the session-start entry's fetch description.

- test-hooks.sh: both mock CLIs record events argv to
  MOCK_EVENTS_ARGS_FILE when set; new tests pin both sides of the split:
  test_session_start_uses_min_level asserts --min-level info is passed
  and no --exclude fallback fires, test_eventbus_lib_uses_client_side_exclude
  asserts eb_fetch_events keeps --exclude and never migrates to
  --min-level.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AFp3uCezpvbeyAMYnNrQ4R

Copy link
Copy Markdown
Owner Author

Round-1 findings addressed in 0274e91 (plus a PR-body update for the Important):

[Important] .exports verification gap — PR body now carries a "Verification of the .exports mapping" section with the end-to-end evidence from a live Claude Code session: CLAUDE_CODE_SESSION_ID present in the Bash-tool subprocess, the mapping exporting AGENT_EVENT_BUS_SESSION_ID with the identical value when .exports is evaluated with the variable in scope, and the CLI's env-attribution path therefore resolving to the same id the SessionStart hook registers as client_id. One honest caveat stated there: that session is a Linux container, not the macOS login-shell chain this repo configures, so the body prescribes the one-line post-merge spot check on the Mac (printenv AGENT_EVENT_BUS_SESSION_ID vs the SessionStart registration line) and names the bus-side fallback (CLI reading CLAUDE_CODE_SESSION_ID directly) as the follow-up if the snapshot doesn't re-evaluate .exports there.

[Suggestion] silent version-skew degradation — Took the fallback-retry option. session-start.sh now retries once with the legacy client-side --exclude denylist when the --min-level fetch comes back empty. The discriminator is documented in the comment: a CLI that understands the flag prints "No events" when there are genuinely none, so a fully empty result means the flag wasn't understood. An out-of-date machine now degrades to extra noise instead of losing all startup context.

[Suggestion] README doesn't mention the split noise policy — Added a "Noise policy is deliberately split" paragraph at the top of the Event-drain architecture section (server-side --min-level for session-start's non-consuming read; client-side EB_EXCLUDE for the drain's bounded consume, with the peek/consume raw-window argument and pointers to the lib comment and agent-event-bus#134), and expanded the session-start.sh entry's fetch description to name the policy and the fallback.

[Suggestion] mocks only tolerate the flag — Took the argv-recording suggestion exactly as sketched: both mock CLIs append their events argv to MOCK_EVENTS_ARGS_FILE when set. test_session_start_uses_min_level asserts --min-level info is passed and that no --exclude appears (the mock understands the flag and returns events, so the new fallback stays idle — the same run also regression-guards the fallback against false-firing). test_eventbus_lib_uses_client_side_exclude sources the lib and asserts eb_fetch_events passes --exclude and never --min-level. Both registered in the runner; drift in either direction of the split now fails CI.

bash -n clean on both changed scripts; the suite itself hangs in this container (pre-existing, noted in the PR body), so CI's Hooks job is the authoritative run for the new tests.


Generated by Claude Code

Comment thread home/.exports
# id - so the two ids are the same value. The CLI reads this env var when
# --session-id is omitted. No-op outside Claude Code or on versions that don't
# expose CLAUDE_CODE_SESSION_ID to subprocesses.
if [[ -n "${CLAUDE_CODE_SESSION_ID:-}" && -z "${AGENT_EVENT_BUS_SESSION_ID:-}" ]]; 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] Still outstanding from round 1 (no "Feedback Addressed" response and the PR body is unchanged): the body verifies the preconditionCLAUDE_CODE_SESSION_ID is present in Bash-tool subprocess environments — but not the outcome, that this mapping actually fires there. For it to fire, .exports must be (re-)evaluated in a shell where the variable is already set; the Bash tool initializes from a startup-time profile snapshot plus process env, and whether the profile chain re-runs with the injected variable in scope is exactly the unverified link. If it does not, this is a silent no-op — publishes stay anonymous while looking fixed. A one-line check settles it: in a live session, run printenv AGENT_EVENT_BUS_SESSION_ID via the Bash tool, confirm it matches the session id from the SessionStart registration, and note the result in the PR body (ideally with one subagent/workflow-step publish landing non-anonymous on the bus).

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

This raced the response by about a minute: the PR body was updated (and a per-finding "Feedback Addressed" comment posted) shortly before this review round landed. The body now carries a "Verification of the .exports mapping" section with the end-to-end evidence: printenv CLAUDE_CODE_SESSION_ID present in a live Bash-tool subprocess, the .exports mapping exporting AGENT_EVENT_BUS_SESSION_ID with the identical value when evaluated with the variable in scope, and the CLI's env-attribution path resolving to the same id the SessionStart hook registers as client_id. It also states the one honest residual — that session is a Linux container, not the macOS login-shell chain this repo configures — and prescribes the one-line post-merge spot check on the Mac (printenv AGENT_EVENT_BUS_SESSION_ID vs the SessionStart registration line), naming the bus-side fallback (CLI reading CLAUDE_CODE_SESSION_ID directly) as the follow-up if that snapshot doesn't re-evaluate .exports.


Generated by Claude Code

Comment thread tests/test-hooks.sh
[[ "$output" == *"Registered"* ]] && [[ "$output" == *"task_completed"* ]]
}

test_session_start_uses_min_level() {

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 test pins the fallback not firing in the happy path, but the fallback branch itself (session-start.sh:114-122) is never exercised — the mock always understands --min-level, so nothing verifies that an old CLI erroring on the flag actually results in a successful --exclude retry with events still surfaced. Since the fallback is the deploy-ordering safety mechanism the README now advertises, a typo in the retry would reproduce exactly the silent-total-loss failure round 1 flagged, with green CI. Cheap to cover: teach the mock to exit 2 when --min-level is passed and an env flag (e.g. MOCK_REJECT_MIN_LEVEL=1) is set, then assert the hook output still contains the events and the argv file shows the --exclude retry after the failed --min-level attempt.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in 89cecd2, taking the sketch exactly: the session-start mock now simulates an old CLI when MOCK_REJECT_MIN_LEVEL=1 is set — usage error on stderr, exit 2, emitted after recording argv so the failed attempt is visible in the args file. New test_session_start_min_level_fallback exercises the branch end to end: the hook output must still contain the events (surfaced via the retry), and the argv file must show the --min-level attempt on line 1 followed by the --exclude retry on line 2. Registered in the runner, so a typo in the retry now fails CI instead of reproducing the silent-total-loss failure with green checks.

(Exit 2 is also now what gates the fallback in the hook itself — see the reply on the session-start.sh:114 thread.)


Generated by Claude Code

Comment thread home/.claude/hooks/session-start.sh Outdated
# errors out with stderr suppressed, leaving EVENTS empty - the CLI prints
# "No events" when there are genuinely none, so empty means the flag wasn't
# understood; retry once with the legacy client-side denylist.
if [[ -z "$EVENTS" ]]; 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 empty-EVENTS heuristic conflates "CLI predates --min-level" with "bus unreachable/timed out": when the bus is down, the first call produces empty output too, so every offline session start now pays a second ~200ms timeout in the retry. Bounded and harmless, but worth a note in the comment — or, to avoid it, capture the first call's exit code and skip the retry when it looks like a timeout rather than an argparse rejection (older CLIs exit fast with a usage error on an unknown flag).

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Took the stronger option in 89cecd2: the fallback is now exit-code-gated instead of empty-gated. The first fetch captures its exit code, and the retry fires only on exit 2 — argparse's usage error for an unrecognized flag, i.e. "the CLI didn't understand --min-level". A down or timed-out bus exits 1 (ConnectionErrorsys.exit(1) in the CLI), so offline session starts no longer pay the second ~200ms timeout; empty-with-success (a flag-aware CLI prints "No events") never fired it anyway. The comment above the branch documents the discriminator, and the new fallback test drives it through a mock that rejects the flag with exit 2.


Generated by Claude Code

@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

Note: the sandbox command filter in this environment rejected the bundled heredoc/array review submission, so the three findings were posted as individual inline comments (on home/.exports:17, tests/test-hooks.sh:350, home/.claude/hooks/session-start.sh:114) and this review carries the verdict.

Summary
Round 2 cleanly addresses all three round-1 suggestions: session-start.sh gained a version-skew fallback to the legacy --exclude denylist, the hooks README now documents the deliberate split noise policy (server-side --min-level for the non-consuming session-start fetch, client-side EB_EXCLUDE for the drain's bounded consume), and the mocks now record argv so tests pin both sides of the split. CI is green including the Hooks job. Two Important items remain: the round-1 verification request on the .exports mapping is still unanswered, and the new fallback branch has no test exercising the path where it actually fires.

Previously Addressed (Filtered)
Not re-raised — resolved in commit 0274e91:

  • [Suggestion] session-start.sh version-skew degradation — fallback retry added
  • [Suggestion] README missing the split noise-policy rationale — added to Event-drain architecture section and the session-start entry
  • [Suggestion] No assertion that --min-level is actually passed — argv-recording mocks plus two new tests added

Findings (posted as inline comments)

  • [Important] home/.exports:17 — round-1 verification gap still open: no end-to-end evidence the CLAUDE_CODE_SESSION_IDAGENT_EVENT_BUS_SESSION_ID mapping fires in a live Bash-tool subprocess (a printenv check noted in the PR body settles it)
  • [Important] tests/test-hooks.sh:350 — the new version-skew fallback branch (session-start.sh:114-122) has no test exercising the case where it fires
  • [Suggestion] home/.claude/hooks/session-start.sh:114 — empty-output heuristic also triggers on bus-unreachable, costing an extra ~200ms timeout at session start when offline

Verdict
REQUEST_CHANGES - The round-1 Important finding on home/.exports (end-to-end evidence that the mapping fires in a live Bash-tool subprocess) is still outstanding, and the new version-skew fallback branch is untested.


Automated review by Claude Code

- session-start.sh: the version-skew fallback now fires only on exit 2
  (argparse usage error - the flag wasn't understood), not on any empty
  result. A bus that is down or timed out exits 1, where a retry can't
  help and would make every offline session start pay a second ~200ms
  timeout; empty-with-success (a flag-aware CLI prints "No events")
  never fired it anyway.

- test-hooks.sh: the session-start mock can simulate an old CLI
  (MOCK_REJECT_MIN_LEVEL=1 -> usage error on stderr, exit 2, after
  recording argv). New test test_session_start_min_level_fallback
  exercises the fallback branch end to end: events still surfaced, and
  the argv file shows the failed --min-level attempt followed by the
  --exclude retry, in order.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AFp3uCezpvbeyAMYnNrQ4R
Comment thread home/.exports
# id - so the two ids are the same value. The CLI reads this env var when
# --session-id is omitted. No-op outside Claude Code or on versions that don't
# expose CLAUDE_CODE_SESSION_ID to subprocesses.
if [[ -n "${CLAUDE_CODE_SESSION_ID:-}" && -z "${AGENT_EVENT_BUS_SESSION_ID:-}" ]]; 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] Closing the loop on the round-1/2 Important: the Linux-container evidence validates the mechanism, and the residual machine-specific link (whether the macOS Bash-tool startup snapshot evaluates .exports after the session id is injected) is now honestly scoped in the PR body. Non-blocking reminder to run the prescribed spot check after merging and re-running ./bootstrap.sh -f: printenv AGENT_EVENT_BUS_SESSION_ID in a Bash tool call on the Mac, compared against the id in the SessionStart registration line. If it comes back empty, publishes remain silently anonymous (there is no failure signal), so file the bus-side fallback (CLI reading CLAUDE_CODE_SESSION_ID directly) in agent-event-bus promptly rather than letting the gap linger.

Comment thread home/.claude/hooks/session-start.sh Outdated
# ~200ms timeout. A flag-aware CLI prints "No events" when there are
# genuinely none, so empty-with-success never fires the fallback either.
if [[ -z "$EVENTS" && $FETCH_RC -eq 2 ]]; then
EVENTS=$(agent-event-bus-cli ${URL_ARGS[@]+"${URL_ARGS[@]}"} events \

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] Two small hardening ideas for this fallback, neither blocking: (1) the retry duplicates the entire fetch invocation modulo one flag - a tiny local fetch_events helper wrapping the shared invocation (URL args, --session-id, --order desc, --timeout 200, --limit 20, stderr suppression) and taking the filter flag as its argument would keep the two call sites from drifting if --limit or --timeout ever change. (2) This branch plus its test machinery (MOCK_REJECT_MIN_LEVEL) is deploy-ordering scaffolding that becomes dead code once every machine runs an agent-event-bus-cli that understands --min-level; consider a tracked cleanup issue to remove it after the fleet upgrades, so the exit-2 argparse contract does not live on as an untriggerable path.

claude[bot]
claude Bot previously approved these changes Aug 8, 2026

@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

Note: the sandbox command filter again rejected the bundled heredoc/array review submission, so the two findings were posted as individual inline comments (on home/.exports:17 and home/.claude/hooks/session-start.sh:119) and this review carries the verdict.

Summary
Round 3: both round-2 Importants are resolved. The PR body now carries the end-to-end verification of the .exports mapping (live-session evidence that CLAUDE_CODE_SESSION_ID is present in the Bash-tool subprocess and that the mapping yields the identical id the SessionStart hook registers, with the macOS-chain caveat honestly scoped to a one-line post-merge spot check and a named bus-side fallback), and the version-skew fallback is now exercised end-to-end by test_session_start_min_level_fallback (mock rejects --min-level with exit 2 after recording argv; the test asserts events still surface and the argv file shows the --min-level attempt followed by the --exclude retry, in order). The round-2 suggestion is also addressed: the fallback is gated on exit code 2 (argparse usage error) rather than empty output, so an offline bus (exit 1) does not pay a second timeout. Verified the mechanics directly: the FETCH_RC capture is correct under set -euo pipefail, argv recording precedes the rejection in the mock (which the ordered assertions depend on), and the lib test calls eb_fetch_events with a signature matching the real one. CI is green including the Hooks job that runs the three new tests.

Previously Addressed (Filtered)
Not re-raised - resolved in 89cecd2 and the PR-body update:

  • [Important] .exports verification gap - PR body now documents the live-session evidence, the residual macOS-specific link, the post-merge spot check, and the fallback plan
  • [Important] fallback branch untested - MOCK_REJECT_MIN_LEVEL plus test_session_start_min_level_fallback added and registered
  • [Suggestion] empty-output heuristic fires on bus-unreachable - now exit-code-gated (exit 2 only)

Findings (posted as inline comments)

  • [Suggestion] home/.exports:17 - post-merge spot check follow-through: run the prescribed printenv AGENT_EVENT_BUS_SESSION_ID check on the Mac; if empty, file the bus-side fallback promptly since the failure mode is silent
  • [Suggestion] home/.claude/hooks/session-start.sh:119 - factor the duplicated fetch invocation into a small helper, and track removal of the transitional fallback (plus MOCK_REJECT_MIN_LEVEL machinery) once the fleet CLI understands --min-level

Verdict
APPROVE - No Critical or Important issues remain; both findings are non-blocking suggestions.


Automated review by Claude Code

The primary fetch and the version-skew fallback differed only in their
filter flag; a local fetch_events wrapper owns the shared invocation
(URL args, --session-id, --order desc, --timeout 200, --limit 20,
stderr suppression) so the two call sites can't drift if the limits
ever change. Behavior and argv order are unchanged - the existing
argv-pinning tests cover both paths as-is.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AFp3uCezpvbeyAMYnNrQ4R

Copy link
Copy Markdown
Owner Author

Round 3 response — both suggestions addressed (fab2b1f + #329).

  1. home/.exports:17 (post-merge spot check): Acknowledged as the follow-through plan. The check itself is a live-Mac action I can't run from this container, so it stays a post-merge step exactly as prescribed: after merge + ./bootstrap.sh -f, run printenv AGENT_EVENT_BUS_SESSION_ID in a Bash tool call and compare against the SessionStart registration line. If it comes back empty, the bus-side fallback (CLI reading CLAUDE_CODE_SESSION_ID directly) gets filed in agent-event-bus immediately — agreed that a silent failure mode must not linger unfiled.

  2. session-start.sh:119 (helper + tracked cleanup): Both taken.

    • The duplicated invocation is factored into a local fetch_events helper owning the shared argv (URL args, --session-id, --order desc, --timeout 200, --limit 20, stderr suppression) and taking the filter flag as its argument — the primary passes --min-level info, the fallback passes the legacy --exclude denylist. Argv order is unchanged, so the three argv-pinning tests cover both call sites as-is (bash -n clean locally; the Hooks CI job exercises the tests).
    • Cleanup of the transitional scaffolding is now tracked as Remove session-start.sh --min-level version-skew fallback once the fleet CLI understands the flag #329, with the precise removal list (the exit-2 fallback branch, MOCK_REJECT_MIN_LEVEL, test_session_start_min_level_fallback), the fleet-upgrade precondition, and a note on which tests stay because they pin deliberate behavior rather than scaffolding.

Generated by Claude Code

@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

Note: the sandbox command filter again rejected the gh api heredoc review submission, so this review is posted via gh pr review (no inline comments were needed - there are no findings).

Summary
Round 4: the only change since the round-3 approval is fab2b1f, which factors the duplicated fetch invocation into the shared fetch_events() helper exactly as the round-3 suggestion sketched. Verified the refactor is behavior-preserving: the helper's exit code propagates correctly through the command substitution under set -euo pipefail (so the exit-2 gate on the version-skew fallback still discriminates old-CLI from bus-down), argv order is preserved (--min-level/--exclude slot in via "$@" between the shared --order desc and --timeout/--limit), stderr suppression now lives in one place, and the lib test's eb_fetch_events "sess-1" consume text asc call matches the real SESSION_ID PEEK FORMAT ORDER signature. CI is green including the Hooks job that runs the three policy-pinning tests.

Previously Addressed (Filtered)
Not re-raised:

  • [Important, rounds 1-2] .exports verification gap - PR body carries the live-session end-to-end evidence with the macOS caveat scoped to a post-merge spot check (accepted in round 3)
  • [Important, round 2] fallback branch untested - MOCK_REJECT_MIN_LEVEL + test_session_start_min_level_fallback (resolved in 89cecd2)
  • [Suggestion, round 2] empty-output heuristic fires on bus-unreachable - exit-code-gated in 89cecd2
  • [Suggestion, round 3] duplicated fetch invocation - factored into fetch_events() in fab2b1f; the companion reminder (remove the transitional fallback + MOCK_REJECT_MIN_LEVEL machinery once the fleet CLI understands --min-level, and run the printenv AGENT_EVENT_BUS_SESSION_ID spot check on the Mac) remains a post-merge follow-through, not re-raised

Verdict
APPROVE - No Critical or Important issues; the round-3 refactor suggestion is cleanly implemented and no new findings.


Automated review by Claude Code

@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

Note: the sandbox command filter again rejected the gh api heredoc review submission, so this review is posted via gh pr review (no inline comments were needed — there are no findings).

Summary
Round 5: the only change since the round-4 approval (at fab2b1f) is a0871a9, a merge of origin/main into the branch. The merge touches none of this PR's files — it only adopts main's unrelated home/.claude/settings.json change (model setting + key reordering), and the branch correctly took main's side. Re-verified the PR's substance is unchanged: the exit-2-gated version-skew fallback in session-start.sh (argv recording precedes the mock's rejection, so the ordered assertions in test_session_start_min_level_fallback are sound), the split noise policy documented in both the README and the eventbus-collect.sh rationale comment, and the .exports mapping with its verification section in the PR body. CI is green on the merge head, including the Hooks job that runs the three policy-pinning tests.

Previously Addressed (Filtered)
Not re-raised — all resolved in prior rounds:

  • [Important, rounds 1-2] .exports verification gap — PR body carries the live-session end-to-end evidence; the macOS-specific link is scoped to a post-merge spot check with a named bus-side fallback (accepted rounds 3-4)
  • [Important, round 2] fallback branch untested — MOCK_REJECT_MIN_LEVEL + test_session_start_min_level_fallback (89cecd2)
  • [Suggestion, round 2] empty-output heuristic fires on bus-unreachable — exit-code-gated (89cecd2)
  • [Suggestion, round 3] duplicated fetch invocation — factored into fetch_events() (fab2b1f); transitional-scaffolding cleanup tracked in #329

Verdict
APPROVE - No new changes to review beyond a clean merge from main; no Critical or Important issues.


Automated review by Claude Code

@evansenter
evansenter merged commit c01e831 into main Aug 9, 2026
6 checks passed
evansenter pushed a commit that referenced this pull request Aug 9, 2026
Brings in #328 (event-bus session attribution). No overlap with this
branch's files.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016yN39aeZZVCJCRBFLBgXp5
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