-
Notifications
You must be signed in to change notification settings - Fork 0
Fall back to CLAUDE_CODE_SESSION_ID for CLI session attribution #137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -231,6 +231,30 @@ def cmd_channels(args): | |
| print() | ||
|
|
||
|
|
||
| def _session_id_from_env() -> str | None: | ||
| """Session id for an omitted --session-id, in precedence order. | ||
|
|
||
| AGENT_EVENT_BUS_SESSION_ID is the explicit, tool-agnostic knob and wins. | ||
| CLAUDE_CODE_SESSION_ID is the fallback because Claude Code injects it into | ||
| every subprocess it spawns, so it is present exactly where the explicit one | ||
| tends not to be. | ||
|
|
||
| The fallback exists because setting the explicit var from a shell profile | ||
| is not reliable: the dotfiles that map one to the other live in ~/.exports, | ||
| which is sourced from ~/.zshrc - and zsh reads .zshrc for INTERACTIVE | ||
| shells only. Tool-spawned subprocesses are non-interactive, so the mapping | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] This paragraph pins the rationale to a specific external file layout ( The durable half is the shell semantics: non-interactive shells do not read rc files, so any profile-based mapping is unreachable from a tool-spawned subprocess. Stating that, with the |
||
| never runs there and publishes landed as "anonymous" (`zsh -i -c` sees it, | ||
| `zsh -c` does not). That is a property of shell startup, not of any OS - | ||
| it reproduces on Linux - so the fix belongs here, where it holds for every | ||
| shell, spawner, and machine, rather than in one shell's rc plumbing. | ||
|
|
||
| The two ids are the same value by construction: the SessionStart hook | ||
| registers on the bus with client_id = the Claude Code session id, which the | ||
| bus adopts as its session id. | ||
| """ | ||
| return os.environ.get("AGENT_EVENT_BUS_SESSION_ID") or os.environ.get("CLAUDE_CODE_SESSION_ID") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The invariant this helper documents — the two ids being the same value by construction — only holds if whoever registered passed Defaulting the |
||
|
|
||
|
|
||
| def cmd_publish(args): | ||
| """Publish an event.""" | ||
| arguments = { | ||
|
|
@@ -240,7 +264,7 @@ def cmd_publish(args): | |
| if args.channel: | ||
| arguments["channel"] = args.channel | ||
| # Use explicit --session-id, fall back to env var | ||
| session_id = args.session_id or os.environ.get("AGENT_EVENT_BUS_SESSION_ID") | ||
| session_id = args.session_id or _session_id_from_env() | ||
| if session_id: | ||
| arguments["session_id"] = session_id | ||
| # Optional structured payload fields (RFC #121) | ||
|
|
@@ -260,7 +284,7 @@ def cmd_publish(args): | |
| def cmd_events(args): | ||
| """Get recent events.""" | ||
| # Use explicit --session-id, fall back to env var (matches cmd_publish) | ||
| session_id = args.session_id or os.environ.get("AGENT_EVENT_BUS_SESSION_ID") | ||
| session_id = args.session_id or _session_id_from_env() | ||
|
|
||
| # Validate --resume requires a session id (flag or env var) | ||
| if args.resume and not session_id: | ||
|
|
@@ -467,7 +491,8 @@ def main(): | |
| p_publish.add_argument("--payload", required=True, help="Event payload") | ||
| p_publish.add_argument("--channel", default="all", help="Target channel") | ||
| p_publish.add_argument( | ||
| "--session-id", help="Your session ID (default: $AGENT_EVENT_BUS_SESSION_ID)" | ||
| "--session-id", | ||
| help="Your session ID (default: $AGENT_EVENT_BUS_SESSION_ID, else $CLAUDE_CODE_SESSION_ID)", | ||
| ) | ||
| p_publish.add_argument("--title", help="Optional short headline for the payload") | ||
| p_publish.add_argument("--tags", help="Comma-separated tags for downstream filtering") | ||
|
|
@@ -484,7 +509,8 @@ def main(): | |
| p_events.add_argument("--cursor", help="Cursor from previous call (for pagination)") | ||
| p_events.add_argument( | ||
| "--session-id", | ||
| help="Your session ID for cursor tracking (default: $AGENT_EVENT_BUS_SESSION_ID)", | ||
| help="Your session ID for cursor tracking (default: " | ||
| "$AGENT_EVENT_BUS_SESSION_ID, else $CLAUDE_CODE_SESSION_ID)", | ||
| ) | ||
| p_events.add_argument("--limit", type=int, help="Maximum number of events to return") | ||
| p_events.add_argument( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,19 @@ | |
| from agent_event_bus import cli | ||
| from conftest import make_events_args, make_publish_args | ||
|
|
||
| # Both names the CLI consults when --session-id is omitted. CLAUDE_CODE_SESSION_ID | ||
| # is injected by Claude Code into every subprocess it spawns - including the one | ||
| # running this suite - so without the scrub below an ambient value would silently | ||
| # satisfy assertions that expect NO attribution, and the suite would pass on a | ||
| # developer's machine while failing in CI (or the reverse). | ||
| SESSION_ID_ENV = ("AGENT_EVENT_BUS_SESSION_ID", "CLAUDE_CODE_SESSION_ID") | ||
|
|
||
|
|
||
| @pytest.fixture(autouse=True) | ||
| def clean_session_id_env(monkeypatch): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Consider hoisting this scrub to The reasoning in the comment above applies to the whole suite, not just this module — As a conftest autouse fixture it protects every module by construction rather than by the accident of what the other modules currently assert. |
||
| for name in SESSION_ID_ENV: | ||
| monkeypatch.delenv(name, raising=False) | ||
|
|
||
|
|
||
| class TestCallTool: | ||
| """Tests for call_tool function.""" | ||
|
|
@@ -287,6 +300,37 @@ def test_publish_explicit_session_id_overrides_env(self, mock_call): | |
| call_args = mock_call.call_args[0][1] | ||
| assert call_args["session_id"] == "explicit-123" | ||
|
|
||
| @patch("agent_event_bus.cli.call_tool") | ||
| def test_publish_falls_back_to_claude_code_session_id(self, mock_call, monkeypatch): | ||
| """publish shares the events precedence chain - it is the surface that | ||
| was actually landing as "anonymous" from tool-spawned subprocesses.""" | ||
| monkeypatch.setenv("CLAUDE_CODE_SESSION_ID", "cc-session-id") | ||
| mock_call.return_value = {"event_id": 1} | ||
|
|
||
| cli.cmd_publish(make_publish_args()) | ||
|
|
||
| assert mock_call.call_args[0][1]["session_id"] == "cc-session-id" | ||
|
|
||
| @patch("agent_event_bus.cli.call_tool") | ||
| def test_publish_explicit_env_beats_claude_code_fallback(self, mock_call, monkeypatch): | ||
| monkeypatch.setenv("AGENT_EVENT_BUS_SESSION_ID", "env-session-123") | ||
| monkeypatch.setenv("CLAUDE_CODE_SESSION_ID", "cc-session-id") | ||
| mock_call.return_value = {"event_id": 1} | ||
|
|
||
| cli.cmd_publish(make_publish_args()) | ||
|
|
||
| assert mock_call.call_args[0][1]["session_id"] == "env-session-123" | ||
|
|
||
| @patch("agent_event_bus.cli.call_tool") | ||
| def test_publish_omits_session_id_when_neither_is_set(self, mock_call): | ||
| """The autouse scrub removes both, so this pins that an unattributed | ||
| publish stays unattributed rather than picking up an ambient id.""" | ||
| mock_call.return_value = {"event_id": 1} | ||
|
|
||
| cli.cmd_publish(make_publish_args()) | ||
|
|
||
| assert "session_id" not in mock_call.call_args[0][1] | ||
|
|
||
|
|
||
| class TestCmdEvents: | ||
| """Tests for events command.""" | ||
|
|
@@ -980,6 +1024,54 @@ def test_no_env_no_flag_omits_session_id(self, mock_call, monkeypatch): | |
| call_args = mock_call.call_args[0][1] | ||
| assert "session_id" not in call_args | ||
|
|
||
| @patch("agent_event_bus.cli.call_tool") | ||
| def test_claude_code_session_id_is_the_fallback(self, mock_call, monkeypatch): | ||
| """THE reason this fallback exists. The dotfiles that map | ||
| CLAUDE_CODE_SESSION_ID -> AGENT_EVENT_BUS_SESSION_ID live in ~/.exports, | ||
| sourced from ~/.zshrc - and zsh reads .zshrc for INTERACTIVE shells | ||
| only, so a tool-spawned (non-interactive) subprocess never runs the | ||
| mapping and publishes landed as "anonymous". Claude Code injects | ||
| CLAUDE_CODE_SESSION_ID into that subprocess regardless, so reading it | ||
| here fixes the attribution for every shell and machine at once.""" | ||
| monkeypatch.setenv("CLAUDE_CODE_SESSION_ID", "cc-session-id") | ||
|
|
||
| cli.cmd_events(make_events_args()) | ||
|
|
||
| assert mock_call.call_args[0][1]["session_id"] == "cc-session-id" | ||
|
|
||
| @patch("agent_event_bus.cli.call_tool") | ||
| def test_explicit_env_var_beats_the_claude_code_fallback(self, mock_call, monkeypatch): | ||
| """AGENT_EVENT_BUS_SESSION_ID is the tool-agnostic knob, so it wins - | ||
| an operator who sets it deliberately is not overridden by the ambient | ||
| one Claude Code happens to inject.""" | ||
| monkeypatch.setenv("AGENT_EVENT_BUS_SESSION_ID", "env-session-id") | ||
| monkeypatch.setenv("CLAUDE_CODE_SESSION_ID", "cc-session-id") | ||
|
|
||
| cli.cmd_events(make_events_args()) | ||
|
|
||
| assert mock_call.call_args[0][1]["session_id"] == "env-session-id" | ||
|
|
||
| @patch("agent_event_bus.cli.call_tool") | ||
| def test_flag_beats_both_env_vars(self, mock_call, monkeypatch): | ||
| monkeypatch.setenv("AGENT_EVENT_BUS_SESSION_ID", "env-session-id") | ||
| monkeypatch.setenv("CLAUDE_CODE_SESSION_ID", "cc-session-id") | ||
|
|
||
| cli.cmd_events(make_events_args(session_id="explicit-id")) | ||
|
|
||
| assert mock_call.call_args[0][1]["session_id"] == "explicit-id" | ||
|
|
||
| @patch("agent_event_bus.cli.call_tool") | ||
| def test_resume_satisfied_by_claude_code_session_id(self, mock_call, monkeypatch): | ||
| """--resume needs a session id from somewhere; the fallback supplies | ||
| one, so a drain hook running in a non-interactive shell no longer | ||
| exits 1 with 'requires --session-id'.""" | ||
| monkeypatch.setenv("CLAUDE_CODE_SESSION_ID", "cc-session-id") | ||
| mock_call.return_value = {"events": [], "next_cursor": None} | ||
|
|
||
| cli.cmd_events(make_events_args(resume=True)) | ||
|
|
||
| assert mock_call.call_args[0][1]["session_id"] == "cc-session-id" | ||
|
|
||
| @patch("agent_event_bus.cli.call_tool") | ||
| def test_resume_satisfied_by_env_session_id(self, mock_call, monkeypatch): | ||
| monkeypatch.setenv("AGENT_EVENT_BUS_SESSION_ID", "env-session-id") | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] The Environment variables line in the Naming Conventions section enumerates only the
AGENT_EVENT_BUS_*names.CLAUDE_CODE_SESSION_IDis now a variable this codebase reads, and it is the one consulted name that will never appear under that prefix — so someone auditing which env vars the tool depends on, working from that table, will miss it.Worth a short parenthetical there — plus
CLAUDE_CODE_SESSION_ID, read by the CLI as a session-attribution fallback — so the table stays the complete list. This block is a good explanation of why; the table is where people look for what.