Cleanup pass before feature work - #138
Conversation
… schema parity Cleanup pass ahead of feature work. No behavior change. - Delete _get_implicit_channels: it took a session_id it never read and always returned None. Its three tests asserted that constant; they are replaced by TestBroadcastModel, which pins the actual design decision (channels are metadata, not subscriptions) by observing that a session sees events on channels it does not belong to. - Single-source the event wire shape. _event_to_dict and _webhook_payload built identical dicts differing only in "id" vs "event_id", each with its own do-not-break-this comment. Both now delegate to _event_wire_dict, so a field added for one consumer reaches the other and the two cannot drift on signal_level. Both names are kept: tests pin the delivery contract against _webhook_payload specifically. - Extract _preview() for the payload truncation duplicated in the DM notification and the publish dev-notify. - Rebuild storage.get_events' WHERE clause from a condition list instead of four copies of the same append-or-initialize block. - Add TestSchemaParity. _init_db defines the schema for fresh installs while the @migration registry upgrades existing databases; until now nothing checked that the two agree, and _init_db's docstring could only assert it. A migrated v1 database is now compared column-for-column and index-for- index against a fresh one, so a change landing in one path and not the other fails here instead of as "no such column" on a live bus. - server.py's module docstring was missing list_channels. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N11TJkcFv2Bn4YQvbYsQ37
…l_tool
call_tool lived in cli.py but is imported by bridge.py, and it terminated
the process on failure: sys.exit(1) for connection errors, and for
everything else either sys.exit(1) or a re-raise depending on a `debug`
flag. A daemon whose HTTP helper can kill it is the wrong shape, and the
bridge had grown three catch sites that reverse-engineered intent from the
exit code - `isinstance(e, SystemExit) and e.code == 1` - each wrapped in a
paragraph of comments explaining why that inference held.
call_tool now raises BusUnreachableError when nothing answers and lets every
other failure propagate with its own type. It never prints and never exits;
main() owns that policy in one place, and a handler's own sys.exit for a
logical error still passes through (SystemExit is not an Exception). The
`debug` parameter is gone from call_tool - it only ever controlled printing
and exiting - and stays a main()-level flag.
The bridge gains BridgeRegistrationError for its own named checks (a
non-list listing, a missing webhook_id, a refused removal), replacing the
SystemExits it raised to signal "retryable" to register_with_retry. That
loop now catches Exception plainly and logs the exception itself; the
exit-code archaeology and the debug=True call sites are gone.
The failure messages get better as a side effect. Registration retries used
to log "SystemExit(1)" for a down bus, a 401, and a malformed body alike;
they now log the real cause and the URL:
Registration on http://... failed (BusUnreachableError('Cannot connect to
agent event bus at http://...')); retrying in 1s
Verified end to end: the CLI still prints its start hint and exits 1 against
a dead bus, and a bridge started before the bus retries, registers when it
comes up, and spools a DM to its wake file.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N11TJkcFv2Bn4YQvbYsQ37
Two cleanups that were blocking the same thing - the webhook surface being half-built, and the guide being mostly about a component most sessions never run. set_webhook_active ------------------ The `active` column, the storage method, and `list_webhooks(active_only=)` have existed since webhooks landed, but nothing in MCP or the CLI could set `active` to 0. In production it was therefore always 1, which made `active_only` - and `webhook list --all` - a distinction without a difference. Adds the MCP tool and `webhook disable/enable <id>` on the CLI. Pausing keeps the registration, its filters, and its secret; unregistering stays permanent. That is the useful state for an endpoint that is failing or noisy but that you intend to bring back - including a bridge whose hook you want to silence without losing its row. storage.list_webhooks now defaults to active_only=True, matching the MCP tool. The two disagreed, and the unsafe direction is asymmetric: a caller that forgets the argument while deciding whom to deliver to would otherwise wake every webhook its owner had deliberately paused. Every existing caller passes it explicitly, so nothing changes today. Tests cover the round trip, the unknown-id path, both CLI verbs (one handler behind two subcommands - the mapping is what can silently invert), and the part that actually matters: a paused webhook drops out of the real dispatch matcher, not just the listing. docs/BRIDGE.md -------------- guide.md was 658 lines, 300 of them the experimental bridge - a section longer than the entire rest of the API guide, in a file served as an MCP resource and therefore paid for in every session that reads it. The bridge material is operator documentation for a daemon, not usage guidance for a session. Moved verbatim to docs/BRIDGE.md, which gains headings (it had none, being 300 lines of unbroken prose) and a status banner. guide.md keeps a short pointer plus the one fact a woken session actually needs: drain wake/<session_id>.jsonl, one JSON event per line. The guide is now 373 lines. README points at the new file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N11TJkcFv2Bn4YQvbYsQ37
…ropping user data _init_db defined the current schema while the @migration registry upgraded existing databases, and two more mechanisms had grown alongside them: bare try/except ALTER TABLE statements inside _init_db, and a duplicate CREATE for the webhooks table that migration v3 already owns. Four ways to change a schema, two of which no test could see. Now: _init_db only ever CREATEs, migrations only ever ALTER. - @migration(5, "backfill_pre_registry_columns") takes over adding sessions.last_cursor and events.channel from the inline try/excepts. Both checks are conditional, so it is a no-op on any database that has them - which is every database at v4. - Dropped the duplicate webhooks CREATE. Migrations run for fresh installs too (version 0 -> SCHEMA_VERSION), so v3 covers that case, exactly as v4 already covers idx_events_correlation. - TestSchemaParity's v1 fixture now omits last_cursor and channel, so it actually exercises migration 5. Built WITH them, the migration could rot while the test stayed green. The two destructive legacy paths are gone, and neither is replaced by silence: - _migrate_db_location moved the user's only copy out of a pre-rename path, with a plain file move that is WAL-unsafe by the repo's own rules. The rename was January; the move has had months to run everywhere it was going to. It now reports the stale file and prints the sqlite3 .backup command, leaving the file untouched. Reporting is not optional: a stale old-path database would otherwise present as a brand-new empty bus with the real history sitting unnoticed on disk. - _migrate_sessions_schema ran an unconditional DROP TABLE sessions on a pre-RFC-#29 pid-based schema. That was a defensible clean break when the schema was weeks old; it is now just a destructive statement aimed at user data, in a file whose own guidance forbids adding one. It refuses with instructions instead, and a test asserts the rows survive the refusal. Verified on a real upgrade, not just fixtures: a v4 database built by the PREVIOUS revision of this file, populated with sessions, events (meta, correlation_id, channel), and a secret-carrying webhook, opens at v5 with every field intact, a single version row, and a schema identical column-for- column and index-for-index to a fresh v5 install. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N11TJkcFv2Bn4YQvbYsQ37
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # failure still funneled into a bare exit is a connection error, which | ||
| # the retry loop renders as exactly that. | ||
| existing = call_tool("list_webhooks", {"active_only": True}, url=config.bus_url, debug=True) | ||
| existing = call_tool("list_webhooks", {"active_only": True}, url=config.bus_url) |
There was a problem hiding this comment.
[Important] The startup sweep only sees active rows, but this PR is what makes a row inactive in the first place.
register_with_bus's own docstring states the invariant it is protecting: "the bus neither dedupes by URL nor deactivates failing hooks, so each stale row would duplicate every wake. Remove matching URLs before registering." _register_webhook_impl confirms it — it calls storage.add_webhook unconditionally, with no URL uniqueness check.
Until this PR, active was always 1 in production (as the PR body says), so passing active_only=True here was harmless. Now set_webhook_active / webhook disable can leave a paused row at exactly this hook_url — and pausing a noisy endpoint you intend to bring back is the documented use case for the feature, which the bridge's own endpoint fits precisely.
Fails when: an operator runs agent-event-bus-cli webhook disable <bridge's id> because the bridge is spamming wakes, then restarts the bridge → the sweep skips the paused row and register_webhook adds a second row at the same URL; webhook enable <old id> to resume → two active webhooks at one URL → every session: DM is delivered twice, spooling duplicate lines into the wake file (only deduped if the consuming hook honors the event_id contract). unregister_from_bus on shutdown removes only the newer id, so the paused row then leaks indefinitely.
Fix is one argument: sweep with active_only=False. A stale row at this bridge's URL is stale whether or not someone paused it, and the removal is a delete either way.
| snapshot = {} | ||
| for table in tables: | ||
| columns = { | ||
| row[1]: row[2].upper() for row in conn.execute(f"PRAGMA table_info({table})") |
There was a problem hiding this comment.
[Suggestion] _schema_snapshot compares the declared type only, so the parity guard is weaker than the class docstring advertises ("column-for-column").
PRAGMA table_info returns (cid, name, type, notnull, dflt_value, pk) — row[3] and row[4] are right there and unused. As written, a NOT NULL or a DEFAULT that lands in one path but not the other passes silently. That is not hypothetical: sessions.display_id is TEXT NOT NULL from _init_db's CREATE but plain nullable TEXT on a migrated database (migration v2 adds it with a bare ALTER TABLE ... ADD COLUMN display_id TEXT), and this test is green on that divergence today.
Same for indexes: only names are compared, so an index on the right table over the wrong columns still matches. Selecting name, sql from sqlite_master for type='index' would close both gaps.
(The display_id divergence is pre-existing, not introduced here — but this PR is what adds the guard that claims to catch exactly this class of drift.)
| self.db_path.parent.mkdir(parents=True, exist_ok=True) | ||
| shutil.move(str(OLD_DB_PATH), str(self.db_path)) | ||
| logger.info("Database migration complete") | ||
| return # Already at the current location, nothing to say |
There was a problem hiding this comment.
[Suggestion] The legacy-path warning is once-only by construction, which undercuts the "silence is not an option" reasoning in the docstring above.
This early return fires as soon as data.db exists — and the very first startup that emits the warning is also the one that creates data.db. So the operator gets exactly one log line, on the boot where they are least likely to be reading logs, and every restart afterwards is silent while the bus runs empty and the real history sits at the old path.
A cheap strengthening: keep checking the legacy paths for as long as the current database is still empty (no sessions and no events), rather than gating on db_path.exists(). That repeats the warning while it is still actionable and goes quiet once the operator has either restored the old file or started accumulating real history — without nagging someone who deliberately left the old copy in place.
|
|
||
|
|
||
| @mcp.tool() | ||
| async def set_webhook_active(webhook_id: int, active: bool) -> dict: |
There was a problem hiding this comment.
[Suggestion] tests/test_hardening.py::TestAsyncToolWrappers::test_all_tools_are_async enumerates the tool functions by hand, and set_webhook_active was not added to that list — so the new tool is not covered by the guard for the concurrency invariant CLAUDE.md calls out ("Never put blocking work directly in a tool function - it freezes the whole server (#112)").
The implementation here is correct (async wrapper delegating to _run_sync), so this is coverage, not a defect. Deriving that list from the MCP registry instead of a literal would make the guard cover every future tool automatically — the same one-mechanism instinct the @migration consolidation in 0f5fe74 applies to schemas.
| # (SystemExit is not an Exception) passes through untouched. | ||
| try: | ||
| args.func(args) | ||
| except BusUnreachableError as e: |
There was a problem hiding this comment.
[Suggestion] This arm ignores --debug, unlike the generic arm three lines below.
agent-event-bus-cli --debug sessions against a down bus prints the friendly two-line message and exits 1, with no traceback — the one case where the user explicitly asked for the stack. Hoisting if args.debug: raise above both arms (or repeating it here) makes --debug mean the same thing for every failure.
| loopback literal, or any value listed with `--allowed-hosts` - the same | ||
| allowlist covers both endpoints, so a monitoring probe arriving through | ||
| the same reverse proxy as the deliveries passes on the proxy's Host. | ||
| ### Delivery outcomes |
There was a problem hiding this comment.
[Suggestion] ### Delivery outcomes is nested under ## Security and operation, but the action field (spool, tmux, spool-cooldown, spool-unmapped, spool-tmux-failed) is delivery semantics, not security — it is the section an operator debugging "why did my session not wake" goes looking for, and under a security heading it is easy to miss in a rendered TOC. Promoting it to ## (or moving it next to ## Backends) would match how it is actually used.
Also minor: the new ## headings at lines 168 and 200 sit directly against the preceding list item with no blank line. CommonMark lets an ATX heading interrupt a paragraph, so both render correctly on GitHub, but a blank line before each matches the rest of the file.
There was a problem hiding this comment.
Code Review — Round 1
Summary
Four well-scoped cleanup commits: dead code removal, a shared event wire-shape builder, call_tool becoming a raise-not-exit library function, the webhook pause/resume surface, and consolidating schema change onto the @migration registry. The reasoning in the commit bodies and docstrings is unusually good, the two destructive legacy paths (shutil.move of the user's only copy, unconditional DROP TABLE sessions) are exactly the right things to remove, and TestSchemaParity is a genuinely valuable guard.
I checked the load-bearing pieces by hand rather than taking the descriptions on trust:
_init_dbcreating onlyschema_version/sessions/events/indexes and letting migration v3 ownwebhooksis correct —_run_migrationsrunscurrent+1 .. SCHEMA_VERSIONand fresh installs enter at version 0, so v3 does create the table.- Migration v5 is conditional on both columns and is a genuine no-op at v4.
- The
get_eventsWHERE rebuild is behavior-identical, including thesince_id == 0→if since_idswap (a negative cursor producedid > -Nbefore and still does) and dropping the redundantlen(event_types) > 0. _preview's conditional-expression precedence is right (+binds tighter than the ternary).register_with_retrynarrowing fromexcept (SystemExit, Exception)toexcept Exceptionis safe: everySystemExitremaining inbridge.pyis in the CLI/lock-acquisition paths, none reachable fromregister_with_bus.- The
storage.list_webhooksdefault flip has no production caller relying on the old default —get_matching_webhooksand_list_webhooks_implboth pass it explicitly. - The
guide.md→docs/BRIDGE.mdmove is content-complete: same sections, same order.
Verdict
REQUEST_CHANGES — one Important finding on bridge.py:1360. The bridge's stale-webhook sweep lists active webhooks only, an assumption that held solely because nothing could ever set active = 0. This PR is what removes that guarantee: a paused row at the bridge's own hook URL now survives the sweep, a restart registers a second row at that URL, and re-enabling the paused one makes the bus dispatch every DM to the bridge twice. One-argument fix (active_only=False).
Everything else is a Suggestion and does not block: the TestSchemaParity snapshot ignoring NOT NULL/DEFAULT and index columns, the once-only legacy-DB warning, set_webhook_active missing from the hand-maintained async-tool guard list, --debug not applying to the BusUnreachableError arm, and the ### Delivery outcomes heading nesting.
Process note: the gh api ... --input - heredoc form specified by the review prompt is rejected by a shell-safety guard in this environment, so the six findings were posted as individual inline review comments via the pulls/comments endpoint rather than in a single comments array. They are all attached to the correct file and line. I also could not run the test suite here (uv is outside the tool allowlist), so the 584-passing claim rests on the author's verification and CI.
Automated review by Claude Code
One real defect, introduced by this PR, plus five suggestions - all valid. bridge sweep (Important) ------------------------ register_with_bus swept stale rows at its own hook URL with active_only=True. That was correct only because nothing could set active=0; set_webhook_active removes the guarantee, and pausing a hook that is spamming you is exactly what that feature is for - so the bridge's own endpoint is a likely target. Missing the paused row means a restart adds a SECOND row at the same URL, re-enabling the first makes the bus deliver every DM twice, and shutdown unregisters only the newer id so the paused one leaks. Swept with active_only=False: a row at this URL is stale whether or not someone paused it, and the removal is a delete either way. The regression test drives the real bus tool implementations rather than a canned listing, so the paused row is genuinely absent from an active-only listing. Reverting the argument reproduces two rows at one URL. TestSchemaParity was weaker than advertised ------------------------------------------- _schema_snapshot compared declared type only, so a NOT NULL or DEFAULT landing in one path stayed invisible - and one already had: sessions.display_id is TEXT NOT NULL from _init_db but nullable on a migrated database, because migration v2 can only ALTER TABLE ADD COLUMN (SQLite has no ALTER COLUMN, as v2's own comment records). The guard was green on that. Now compares (type, notnull, pk, default) and index CREATE statements rather than index names. The display_id divergence is recorded in KNOWN_CONSTRAINT_DIVERGENCES with the reason and the follow-up (a table rebuild, which is its own change); everything not listed there fails. Even for a listed column the declared type is still compared, and a second test pins the list so an allowance nobody rechecks cannot become permanent. legacy-path warning was once-only --------------------------------- It gated on "db_path does not exist", true on exactly one boot - the one that creates it. So the operator got a single line at the moment they are least likely to be reading logs, then silence while the bus ran empty. Now keyed on "this database is still empty": repeats while it is actionable, stops on its own once real history accumulates. three smaller ones ------------------ - test_all_tools_are_async hand-listed the tools and set_webhook_active was not in it. Derived from the registry instead, matched by tool TYPE rather than duck-typing on .fn/.name - conftest patches a MagicMock into the server module and a MagicMock answers hasattr for everything. A companion test pins the roster so a shape change fails loudly instead of scanning nothing and passing vacuously. - --debug now applies to BusUnreachableError too. It was checked only in the generic arm, so the one failure a user reaching for the flag is most likely to be debugging was the one that swallowed the stack. - docs/BRIDGE.md: `Delivery outcomes` promoted out of `Security and operation` (it is delivery semantics, and the section someone debugging a missed wake looks for), and blank lines restored before the headings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N11TJkcFv2Bn4YQvbYsQ37
|
All six findings confirmed and fixed in
I did not close the One correction to the index half: index divergence isn't reachable today. Both
589 tests pass, lint and format clean. Generated by Claude Code |
…-assessment-zeetx5
| the entry comes out of the list - rather than silently exempting a | ||
| column that no longer needs exempting. | ||
| """ | ||
| assert KNOWN_CONSTRAINT_DIVERGENCES == {("sessions", "display_id")}, ( |
There was a problem hiding this comment.
[Suggestion] This assertion is tautological - it compares the constant against its own literal, so it can only fail when someone edits KNOWN_CONSTRAINT_DIVERGENCES without editing this line. That makes it a change-detector, not the check the name and docstring describe.
The docstring claims: "If a rebuild migration ever fixes sessions.display_id, this fails and the entry comes out of the list." It would not. A rebuild migration that made display_id NOT NULL on migrated databases too would leave this test green (the constant is untouched) and test_migrated_v1_matches_fresh_install green (the exemption only ever relaxes a comparison) - so the stale allowance would survive exactly the event it is supposed to detect, and the blind spot it opens is permanent from then on.
Making it self-invalidating costs about the same as the current line: build the two databases, snapshot both, and assert the divergence is observed - migrated["sessions"]["columns"]["display_id"][1] != fresh["sessions"]["columns"]["display_id"][1] - with a message saying the exemption is now unnecessary and should be deleted. Then the entry comes out of the list because a test told you to, which is what the docstring promises.
(The exemption itself is right, and the reasoning in the comment above KNOWN_CONSTRAINT_DIVERGENCES is the correct call - a table rebuild on live user data is its own change.)
|
|
||
|
|
||
| def _schema_snapshot(db_path) -> dict: | ||
| """Per table: columns as name → (type, notnull, default), and indexes as |
There was a problem hiding this comment.
[Suggestion] The docstring says columns are name -> (type, notnull, default), but the tuple built below is 4-wide and ordered (type, notnull, pk, default) - pk is missing from the description and default is in the wrong position. _compare_columnss failure message gets it right, so the two disagree about the same tuple.
Worth fixing because the snapshot shape is what a future reader indexes into: the [0] in _compare_columnss known-divergence branch is "the declared type" only by reference to this line.
| assert any("bus unreachable" in r.message for r in caplog.records) | ||
|
|
||
| def test_unregister_logs_real_cause_for_non_connection_failures(self, tmp_path, caplog): | ||
| """The except-Exception arm: with debug=True a 401/timeout/bad body |
There was a problem hiding this comment.
[Suggestion] Stale reference to the parameter this PR removes: "with debug=True a 401/timeout/bad body re-raises out of call_tool". There is no debug parameter on call_tool anymore - those failures re-raise unconditionally now, which is what makes this tests assertion hold.
The test itself is correct and still valuable (it pins that the shutdown log names the real cause instead of the connection-error misdiagnosis); only the docstrings explanation of why is out of date. test_call_tool_failure_contract further down was updated for the same change, so this one just got missed.
There was a problem hiding this comment.
Code Review — Round 2
Summary
Round 1s single blocking finding is fixed, and fixed at the right depth. register_with_bus now sweeps with active_only=False, and the regression test drives the real server._register_webhook_impl / _set_webhook_active_impl / _list_webhooks_impl rather than a canned listing — so the paused row is genuinely absent from an active-only view and reverting the one argument reproduces two rows at one URL. The five suggestions were all taken too, and two came back stronger than asked: TestSchemaParity now compares (type, notnull, pk, default) plus index CREATE SQL, with the pre-existing sessions.display_id divergence recorded explicitly; and the legacy-path warning is re-keyed from "the file does not exist" to "this database is still empty", which is the condition that actually tracks whether the advice is still actionable.
Re-checked by hand rather than on trust:
- The sweeps URL match still reads
wh.get("url"), and_list_webhooks_implemitsurlfor inactive rows too, so widening the listing does not change the match key. register_with_retrynarrowing toexcept Exceptionremains safe after the round-1 edits — everySystemExitinbridge.pyis still in the CLI / lock-acquisition paths, andbridge_hook_urlcannot raise one.- The
cli.main()reorder is behavior-preserving off the--debugpath:BusUnreachableErrorstill prints the same two lines and exits 1, now via anisinstancecheck inside the single arm. _is_empty()is safe at its new call site —_warn_about_legacy_db_locationmoved after_init_db, sosessions/eventsalways exist, and thelegacy is Noneshort-circuit means the two COUNT queries only run when a pre-rename file is actually present.test_hardening.pys registry scan anchors ontype(server.register_session), which conftests_notify_dm_recipientMagicMock does not satisfy;test_tool_coverage_includes_every_known_toolpins the roster so a vacuous scan fails loudly rather than passing.storage.list_webhookss default flip still has no caller relying on the old default:get_matching_webhooksand_list_webhooks_implpass it explicitly, and the only bare calls are in tests over active-only fixtures.- Fresh-install ordering after dropping the duplicate
webhooksCREATE is still correct:_init_dbcreates, then migrations run 0 to 5, so v3 creates webhooks, v4 adds its index, v5 is a no-op on a fresh CREATE. - Docs stayed in step:
set_webhook_activeis inguide.md,README.md, andCLAUDE.md, and the guides bridge pointer is an absolute GitHub URL, not a repo-relative path an MCP reader could not follow.
Previously Addressed (Filtered)
All six round-1 items — bridge sweep, schema-parity strength, once-only legacy warning, async-tool guard list, --debug on the unreachable arm, BRIDGE.md heading nesting — are implemented and are not re-raised.
Verdict
APPROVE - No blocking findings. Three non-blocking suggestions posted inline, all against test/docstring surface added by the round-1 fix.
Note: uv/pytest are outside the tool allowlist here, so the 584-passing claim rests on CI and the authors verification; this review is by inspection. Findings were posted as individual inline comments via the pulls/comments endpoint, since this environments command guard rejects the single-call heredoc form.
Automated review by Claude Code
Three non-blocking suggestions, all against surface the round-1 fix added. test_the_recorded_divergences_are_still_real asserted the constant against its own literal - a change-detector, not the check its name promised. Its docstring claimed a rebuild migration fixing sessions.display_id would fail it; it would not. The constant would be untouched (green here) and the parity test green too, since an exemption only ever relaxes a comparison - so the stale allowance would have survived exactly the event meant to retire it, and its blind spot would have been permanent from then on. It now OBSERVES the divergence: build both databases, snapshot both, assert the exempt column still differs on (notnull, pk, default) - the fields the exemption relaxes, type being compared for exempt columns anyway. An entry that has stopped exempting anything now fails with an instruction to delete it. Verified non-vacuous: adding an entry for a column that does not diverge fails with "'events.channel' ... is now exempting nothing". Also two stale docstrings: - _schema_snapshot described its column tuple as (type, notnull, default); it is (type, notnull, pk, default), and _compare_columns indexes [0] and [1:] against that order. - test_unregister_logs_real_cause_for_non_connection_failures still explained itself in terms of debug=True, a parameter this PR removes from call_tool. Those failures propagate unconditionally now, which is what makes the assertion hold. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N11TJkcFv2Bn4YQvbYsQ37
|
Round-2 suggestions fixed in The tautological exemption check — you're right, and the docstring was the giveaway: it promised a behavior the assertion couldn't deliver. Asserting the constant against its own literal can only fail when someone edits both lines, so a rebuild migration fixing It now observes the divergence. I compare
The 596 passing, lint and format clean, branch current with Generated by Claude Code |
| - Startup is idempotent: stale active webhooks at this bridge's URL (from | ||
| unclean exits) are removed before registering, so restarts never stack | ||
| duplicate deliveries. The sweep matches the URL being registered NOW - | ||
| after changing `--port` or `--hook-url`, drop the row at the old URL | ||
| yourself (`agent-event-bus-cli webhook list` / `webhook unregister`) or | ||
| the bus keeps dispatching to the dead address forever. Because that sweep |
There was a problem hiding this comment.
[Suggestion] Two bits of this bullet were outrun by the round-1 fix.
- "stale active webhooks at this bridge URL ... are removed before registering" — the sweep now lists
active_only=Falseprecisely so it also removes paused rows (bridge.py:1367). The word "active" here now describes the bug that was fixed rather than the behavior. - The recovery instruction for a changed
--port/--hook-urlpoints atagent-event-bus-cli webhook list, which defaults to active-only. A row left at the old URL that someone had paused will not appear, so the operator follows the instruction and concludes there is nothing to clean up.webhook list --allshows both — and, now thatwebhook disableexists, paused rows are a state operators can actually reach.
Suggested: drop "active" from the first sentence, and make the pointer webhook list --all.
| | `notify` | System notification | | ||
| | `register_webhook` | Register HTTP endpoint for push notifications | | ||
| | `list_webhooks` | List registered webhooks | | ||
| | `set_webhook_active` | Pause/resume a webhook without unregistering | |
There was a problem hiding this comment.
[Suggestion] The MCP tool table gained set_webhook_active, but the CLI examples block further down (the agent-event-bus-cli webhook register / list / unregister group, ~L140-147) was not updated with the two new subcommands: agent-event-bus-cli webhook disable 1 and ... webhook enable 1, plus a note that paused webhooks only show up under webhook list --all.
CLAUDE.md asks for CLI help, MCP docstrings and guide.md to move together; guide.md and the MCP docstring did, so this is the one surface left behind. Same note for CLAUDE.md itself: its architecture tree and See Also section do not mention the new docs/BRIDGE.md.
| # state. Shutdown stays best-effort, so a surprise is a warning here | ||
| # rather than the sweep's retryable SystemExit - but the log must not | ||
| # rather than the sweep's retryable BridgeRegistrationError - but the | ||
| # log must not |
There was a problem hiding this comment.
[Suggestion] The comment wrap got mangled when the longer BridgeRegistrationError replaced SystemExit: "...rather than the sweep retryable BridgeRegistrationError - but the" / "log must not" / "assert a removal it never checked" now breaks mid-clause across three lines with an orphaned two-word line. Reflowing the paragraph would restore it to one sentence.
|
|
||
| # Check if we need to migrate from pid to client_id schema | ||
| self._migrate_sessions_schema(conn) | ||
| self._reject_prehistoric_schema(conn) |
There was a problem hiding this comment.
[Suggestion] _reject_prehistoric_schema runs after the CREATE TABLE IF NOT EXISTS schema_version above it, and after _connect has issued PRAGMA journal_mode=WAL. Python sqlite3 opens implicit transactions only before DML, so that CREATE is autocommitted and survives the RuntimeError - a refused prehistoric database comes away with a new empty schema_version table and converted to WAL journalling.
User rows are untouched, which is what actually matters and what the test asserts, so this is cosmetic. But the docstring frames it as "refusing is right: the operator keeps their rows and decides", and the file is in fact modified. Moving the check ahead of the schema_version CREATE - it only needs PRAGMA table_info(sessions) - would make the refusal a genuine no-write, and let the docstring say so.
| for table, column in KNOWN_CONSTRAINT_DIVERGENCES: | ||
| migrated_spec = migrated[table]["columns"][column] | ||
| fresh_spec = fresh[table]["columns"][column] | ||
| assert migrated_spec[1:] != fresh_spec[1:], ( |
There was a problem hiding this comment.
[Suggestion] The rewrite closes the retirement blind spot round 2 named, and closes it correctly — an exemption that no longer exempts anything now fails with a message that says to delete it. But the literal it replaced was also doing a second job that nothing covers now: pinning the roster.
The old == {("sessions", "display_id")} failed whenever the set grew. The new loop only asks "does each listed entry still diverge?" — so a future contributor who hits a real parity failure on, say, events.channel can make test_migrated_v1_matches_fresh_install green by adding ("events", "channel") to the set, and both tests stay green: the parity test relaxes for it, and this test confirms it genuinely diverges (which it does — that is why they added it). The comment above the constant calls the list "the honest boundary of the guard", and its growth is exactly the event worth a deliberate stop.
The two checks are complementary rather than alternatives — keeping the literal alongside the new observation costs one line and restores the addition guard, with the docstring for each saying which direction it watches (growth vs. staleness).
| migrated = _schema_snapshot(migrated_path) | ||
|
|
||
| for table, column in KNOWN_CONSTRAINT_DIVERGENCES: | ||
| migrated_spec = migrated[table]["columns"][column] |
There was a problem hiding this comment.
[Suggestion] These two lookups raise KeyError rather than the intended failure when an entry names a table or column that no longer exists.
That is precisely the scenario the docstring is built around: a rebuild migration that closes the sessions.display_id divergence will very likely rename the table (sessions_new → sessions is the standard SQLite rebuild dance) or, in another case, drop the column outright. The test does fail, which is the important part — but it fails with a bare KeyError: "display_id" instead of the carefully written "the entry is now exempting nothing - delete it" message, and a reader has to reconstruct why.
An assert column in migrated[table]["columns"] (and the same for fresh) ahead of the comparison, with a message saying a stale entry names a column that no longer exists, makes the two retirement paths — the constraint converged, or the column went away — report the same conclusion.
| # state. Shutdown stays best-effort, so a surprise is a warning here | ||
| # rather than the sweep's retryable SystemExit - but the log must not | ||
| # rather than the sweep's retryable BridgeRegistrationError - but the | ||
| # log must not |
There was a problem hiding this comment.
[Suggestion] Reflow artifact from the SystemExit → BridgeRegistrationError rename: the longer name pushed the line over and # log must not is now a two-word orphan mid-sentence, so the comment reads "…but the / log must not / assert a removal it never checked". Rewrapping the paragraph restores it — cosmetic only, and the surrounding comments in this file are otherwise unusually well kept.
There was a problem hiding this comment.
Code Review — Round 2
Summary
All six round-1 findings are genuinely fixed, and two of the fixes are better than what was asked for. I re-derived the load-bearing ones rather than trusting the reply:
bridge.py:1367— the sweep now listsactive_only=False, andtest_startup_sweep_removes_a_paused_row_at_our_urldrives the real_register_webhook_impl/_list_webhooks_impl/_unregister_webhook_impl, so the paused row is genuinely invisible to an active-only listing rather than mocked around. The pre-existing sweep test also now pins the listing argument, so it cannot drift back silently.storage.py:305— keyed on_is_empty()rather thandb_path.exists(), with_init_db()correctly moved ahead of the warning sosessions/eventsexist to count. Ordering inside the helper is right too: thelegacy is Noneearly return fires before_is_empty(), so the common case costs nothing. The three-restart test and the stops-once-an-event-lands test cover both directions.cli.py:658—--debugchecked first,BusUnreachableErrordemoted to anisinstanceinside the single handler.SystemExitis not anException, so a handler own logical-error exit still passes through with its code (pinned bytest_handler_sys_exit_passes_through_untouched).test_hardening.py— matching ontype(server.register_session)rather than duck-typing is correct given conftest patches aMagicMockover_notify_dm_recipient; anisinstanceagainstFunctionToolwill not match it. Theassert toolsanchor plus the pinned 11-name roster closes the vacuous-pass hole.test_storage.py:846—test_the_recorded_divergences_are_still_realnow observes the divergence instead of asserting the constant against its own literal. The reasoning in that docstring is right: the literal form would have survived precisely the rebuild migration meant to retire the exemption. Comparingspec[1:]while_compare_columnsstill comparesspec[0]for exempt columns is the correct split.
I also re-checked the parts round 1 reasoned about that this round could have disturbed: _init_db still only CREATEs, migration v5 is still conditional and a true no-op at v4, the get_events WHERE rebuild is still behavior-identical (including negative cursors and the truthiness swap on since_id), no SystemExit remains reachable from register_with_bus now that register_with_retry narrowed to except Exception, and the flipped storage.list_webhooks default has no caller relying on the old one (get_matching_webhooks and _list_webhooks_impl pass it explicitly; conftest.py:68 passes False; test_list_webhooks registers only active rows).
Nothing in this round names a way the code produces a wrong result. Four Suggestions posted inline — three are documentation the round-1 fix outran, one is a mangled comment wrap.
Previously Addressed (Filtered)
All six round-1 findings verified fixed above and not re-raised. The sessions.display_id NOT NULL divergence is explicitly Deferred (recorded in KNOWN_CONSTRAINT_DIVERGENCES, now with a self-invalidating guard) and not re-raised.
Verdict
APPROVE - No blocking findings.
Process note: the single-call gh api heredoc form is rejected by this environment shell-safety guard, so the four findings were posted as individual inline review comments via the pulls/comments endpoint. All are attached to the correct file and line. I could not run the suite here (pytest/ruff are outside the tool allowlist), so the 589-passing claim rests on the author verification and CI.
Automated review by Claude Code
There was a problem hiding this comment.
Code Review — Round 3
Summary
The only change since round 2 is 0089f54 — the round-2 suggestion on test_the_recorded_divergences_are_still_real, taken, plus two docstring corrections. It is a correct fix: the test now builds both databases and asserts the exemption is observed (migrated_spec[1:] != fresh_spec[1:]), so a rebuild migration that closes the sessions.display_id divergence fails here with a message telling the author to delete the entry — which is exactly what the old literal assertion could not do.
Checked rather than taken on trust:
- The new test compares
[1:](notnull, pk, default) and deliberately excludes[0], which is consistent with_compare_columns: the type is already compared for exempt columns, so an entry matching on all three constraint fields is exempting nothing. The two functions agree on the tuple layout, and the corrected_schema_snapshotdocstring now names it in the right order withpkincluded. _build_v1_dbis reused for the migrated fixture, so this test exercises the same v1 to v5 path astest_migrated_v1_matches_fresh_install— including migration 5, whose whole point is that the fixture omitssessions.last_cursorandevents.channel.- The
test_bridge.pydocstring fix is accurate:call_toolhas nodebugparameter anymore, and a 401, timeout, or bad body does now propagate with its own type, which is what makes that assertion hold.
Nothing in the earlier commits changed this round, and the round-1 and round-2 verifications of those still stand: the active_only=False sweep, the fresh-install migration ordering after dropping the duplicate webhooks CREATE, the list_webhooks default flip, the get_events WHERE rebuild, and the _event_wire_dict consolidation.
Previously Addressed (Filtered)
All six round-1 items and all three round-2 suggestions are implemented; none are re-raised.
Verdict
APPROVE - No blocking findings. Three non-blocking suggestions posted inline: the roster-growth guard that the rewritten divergence test no longer provides, a KeyError instead of the intended message when an exemption names a column that has gone away, and a comment reflow artifact in bridge.py left by the SystemExit to BridgeRegistrationError rename.
Note: pytest and uv are outside the tool allowlist here, so the 596-passing claim rests on CI and the author verification; this review is by inspection. Findings were posted as individual inline comments via the pulls/comments endpoint, since the command guard in this environment rejects the single-call heredoc form.
Automated review by Claude Code
Docs the round-1 fix outran, two test-guard gaps, and a refusal that wrote before it refused. Docs the sweep fix outran ------------------------- - docs/BRIDGE.md described the startup sweep as removing "stale ACTIVE webhooks". Since the sweep now lists active_only=False, that word named the bug rather than the behavior. Its recovery instruction after a --port or --hook-url change also pointed at `webhook list`, which hides paused rows - so an operator with a disabled row at the old URL would follow the instruction and conclude there was nothing to clean up. Now `webhook list --all`, with the reason stated. Only reachable because this PR added `webhook disable`. - README's CLI examples never gained disable/enable (its MCP table did), and CLAUDE.md's architecture tree and See Also never gained docs/BRIDGE.md. CLAUDE.md asks for CLI help, MCP docstrings and guide.md to move together; these were the surfaces left behind. Guarding the guard, both directions ----------------------------------- Round 2 had me replace a literal roster assertion with an observation, which closed the staleness hole but silently opened an addition hole: a real parity failure could be silenced by appending its column to KNOWN_CONSTRAINT_DIVERGENCES, and both tests would stay green - the parity test relaxes for whatever is listed, and the staleness test would confirm the new entry genuinely diverges, which is exactly why someone added it. The two watch opposite directions and are both one line, so both are kept: test_no_new_divergences_have_been_recorded (growth, deliberately a change-detector) and test_the_recorded_divergences_are_still_real (staleness). The staleness test also raised a bare KeyError when an entry named a column that had gone away - the likeliest retirement path, since a SQLite rebuild renames the table. Both retirement shapes now report the same conclusion: delete the entry. A refusal that wrote first -------------------------- _reject_prehistoric_schema ran after _init_db's CREATE TABLE IF NOT EXISTS schema_version, and DDL autocommits, so a refused database came away with an empty schema_version table - while the docstring said the operator keeps their file and decides. Moved ahead of every CREATE. Verified: a refused pid-schema database now contains only its own `sessions` table, rows intact. The docstring also now states the one thing that does still happen - opening the file asserts journal_mode=WAL, which is format, not content. Also reflowed a comment left mangled by the SystemExit -> BridgeRegistrationError rename. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N11TJkcFv2Bn4YQvbYsQ37
|
Round-3 suggestions fixed in
README / CLAUDE.md — right, and by the repo's own rule: CLAUDE.md asks for CLI help, MCP docstrings and The roster-growth guard — this is the one I'd call a genuine miss on my part, not just staleness. Taking round 2's suggestion, I replaced the literal with an observation and treated them as alternatives; they're complementary. The hole you describe is real: appending a column to The The refusal writing before it refused — correct, and worth more than cosmetic given the docstring's claim. DDL autocommits, so a refused database came away with an empty Only its own table remains. I also documented the one thing that does still happen — opening the file asserts The 597 passing, lint and format clean, branch current with Generated by Claude Code |
| rows = conn.execute("SELECT id FROM sessions").fetchall() | ||
| conn.close() | ||
| assert row is None, "New DB should not have old_marker table (wasn't overwritten)" | ||
| assert rows == [("old-1",)], "the refusal must not have touched user rows" |
There was a problem hiding this comment.
[Suggestion] The reorder this commit makes is correct, but nothing pins it.
_reject_prehistoric_schema now runs before the CREATE TABLE IF NOT EXISTS schema_version, and its new docstring makes that a promise: "Runs before _init_db creates anything, so the refusal adds no tables of its own." This test asserts only that the user rows survive — which was already true under the old ordering. Move the call back below the CREATE and every test still passes, leaving the docstring quietly false.
A one-line addition here would close it, on the same connection you already open:
tables = {r[0] for r in conn.execute("SELECT name FROM sqlite_master WHERE type=(?)", ("table",))}
assert tables == {"sessions"}, "the refusal must not have created scaffolding of its own"That also documents the boundary the docstring is careful about: schema_version absent (content), journal_mode=WAL asserted (format).
| yourself (`agent-event-bus-cli webhook list --all` / `webhook unregister`) | ||
| or the bus keeps dispatching to the dead address forever. Use `--all`: | ||
| plain `webhook list` hides paused rows, so a disabled one at the old URL | ||
| would look like nothing to clean up. Because that sweep |
There was a problem hiding this comment.
[Suggestion] The --all fix landed twice in one bullet, and the reflow left a short line mid-sentence.
The parenthetical two lines up already says agent-event-bus-cli webhook list --all; the following sentence then re-explains why. One of the two is enough — keeping the parenthetical and dropping "Use --all: plain webhook list hides paused rows, so a disabled one at the old URL would look like nothing to clean up" reads cleanly, or keep the prose and leave the parenthetical bare.
Either way the paragraph wants a rewrap: "would look like nothing to clean up. Because that sweep" ends short at ~40 columns while its neighbours run to ~75.
Separately, this bullet is now doing a lot — startup idempotence, the changed-URL recovery procedure, --all, the two flock singletons, the run-two-bridges requirements, and the out-of-scope multi-user case. Splitting at "Because that sweep can't tell a stale row from a live peer's" would give the locking story its own bullet without touching a word of it.
There was a problem hiding this comment.
Code Review — Round 5
Summary
The only change since round 3 is d68c9bc — all three round-3 suggestions taken, plus one behavioral reorder in storage.py. Nothing here names a way the code produces a wrong result.
Verified by hand rather than on trust:
storage.py:420—_reject_prehistoric_schemamoved above theCREATE TABLE IF NOT EXISTS schema_version. Safe: the function only runsPRAGMA table_info(sessions), so it has no dependency onschema_versionexisting, and_get_schema_versionalready tolerates the table being absent (sqlite3.OperationalError→ 0). On a fresh database the PRAGMA returns nothing and the guard does not fire; on a refusal theRuntimeErrorpropagates out of_connectbeforeconn.commit(), and thefinallystill closes. The docstring is now exact about what remains —journal_mode=WALis asserted in_connectbefore any statement here — which is format, not content.test_storage.py:891-907— theKeyErrorhole is closed in both directions and at both snapshots. A rebuild migration that renamessessionsor dropsdisplay_idnow fails with the intended "the entry is stale, delete it" message instead of a bareKeyError, and the loop runs before the[1:]comparison so the guard cannot be reached through a missing key.test_storage.py:846—test_no_new_divergences_have_been_recordedrestores the roster-growth guard round 3 flagged, and the two tests now cover opposite directions without overlapping: this one fails when the set grows,test_the_recorded_divergences_are_still_realfails when an entry stops exempting anything. Neither can be satisfied by editing only the other.bridge.py:1436— comment reflowed, one sentence again; no code change.- Docs —
README.md's CLI block now carrieswebhook disable/enableand the--allcaveat,BRIDGE.mdno longer says the sweep is active-only (it is not, since round 1), andCLAUDE.mdgained thedocs/BRIDGE.mdpointer.guide.md,README.md, andCLAUDE.mdall listset_webhook_active, so the three surfaces CLAUDE.md asks to move together did.
I also re-derived the load-bearing pieces from earlier rounds against the current tree, since this commit touched storage.py and bridge.py: the sweep still lists active_only=False and still matches on wh.get("url"); _list_webhooks_impl still emits url and webhook_id for inactive rows; migration v5 is still conditional on both columns; _init_db still only CREATEs, with webhooks owned by v3 which runs for fresh installs at version 0; the get_events WHERE rebuild is still behavior-identical; and storage.list_webhooks's flipped default still has no caller relying on the old one. The webhook disable/enable path is wired correctly end to end — cmd_webhook_set_active derives active from args.webhook_command, whose dest matches the add_subparsers(dest="webhook_command") at cli.py:605, and _set_webhook_active_impl returns success: False for an unknown id, which the CLI turns into exit 1.
Previously Addressed (Filtered)
All six round-1 findings, all three round-2 suggestions, and all three round-3 suggestions are implemented; none re-raised. The sessions.display_id NOT NULL divergence stays Deferred (recorded in KNOWN_CONSTRAINT_DIVERGENCES, now guarded from both staleness and growth).
Verdict
APPROVE - No blocking findings. Two non-blocking suggestions posted inline, both against surface this round's own fix added: the new _init_db ordering is not pinned by any test (the refusal test asserts only that user rows survive, which held under the old ordering too), and the --all guidance now appears twice in one BRIDGE.md bullet with a ragged rewrap.
Note: pytest/uv are outside the tool allowlist in this environment, so the 596-passing claim rests on CI and the author's verification; this review is by inspection. Findings were posted as individual inline comments via the pulls/comments endpoint, since this environment's command guard rejects the single-call heredoc form.
Automated review by Claude Code
Both round-4 suggestions, against surface round 3's own fix added. The reorder in d68c9bc moved _reject_prehistoric_schema above _init_db's CREATE TABLE IF NOT EXISTS schema_version, and its docstring turned that into a promise: "runs before _init_db creates anything, so the refusal adds no tables of its own". Nothing held it there. The existing test asserts only that user rows survive - true under the old ordering too - so moving the call back would have left every test green and the docstring quietly false. I had verified the behavior by hand and never pinned it, which is the same gap in a different place. test_refuses_and_preserves_the_rows now also asserts the refused database contains only its own `sessions` table, on the connection it already opens. Reverting the reorder fails it with "the refusal created scaffolding of its own: ['schema_version', 'sessions']". The message names the boundary the docstring is careful about: schema_version absent (content), journal_mode=WAL asserted (format). docs/BRIDGE.md: the --all fix landed twice in one bullet - once in the parenthetical, then again as a sentence re-explaining it. Kept the parenthetical with a short inline reason and dropped the sentence. Split the bullet at "Because that sweep can't tell a stale row from a live peer's", so the locking story stands on its own instead of trailing six other topics, and rewrapped the paragraph the edit left ragged. Verified word-for-word identical to the previous revision apart from the intended de-duplication. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N11TJkcFv2Bn4YQvbYsQ37
|
Both round-4 suggestions fixed in The unpinned ordering — you're right, and it's the same gap twice: I verified the no-scaffolding behavior by hand in the previous round and never pinned it, which is exactly the mistake that made the round-2 tautological-test finding possible. The existing assertion only covered user rows, which held under the old ordering too, so moving the call back would have left the suite green and the docstring false.
Message states the boundary deliberately, since that's the distinction the docstring is careful about. The duplicated I rewrapped the paragraph my edit left ragged, and since that's the kind of change that quietly eats words, I diffed the whitespace-normalized text against the previous revision — identical apart from the intended de-duplication. 597 passing, lint and format clean. Generated by Claude Code |
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
| result = call_tool("list_webhooks", arguments, url=args.url) | ||
|
|
||
| if not result: | ||
| print("No webhooks registered") |
There was a problem hiding this comment.
[Suggestion] webhook list without --all now has a reachable state it did not have before this PR: registrations exist, but every one of them is paused. In that state this prints No webhooks registered, which is the one thing that is not true.
It matters because it is the exact case docs/BRIDGE.md sends an operator into - "after changing --port/--hook-url, drop the row at the old URL yourself (webhook list --all ... --all because plain list hides paused rows)". Someone who reaches for the plain form first gets told there is nothing there rather than being nudged toward --all.
Since args.all is right here, the message can distinguish the two:
if not result:
print("No webhooks registered" if args.all else "No active webhooks (try --all)")
return
|
|
||
| A **separate unit** from the bus (`com.evansenter.agent-event-bus-bridge`) | ||
| because 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 |
There was a problem hiding this comment.
[Suggestion] "The unit pins --backend spool" names a flag the unit does not use. scripts/com.evansenter.agent-event-bus-bridge.plist has no --backend in ProgramArguments; it sets AGENT_EVENT_BUS_BRIDGE_BACKEND=spool in EnvironmentVariables (plist:33-34).
Worth the word change for two reasons: an operator verifying the claim greps the plist for --backend and finds nothing, and the two are not equivalent under "pins" - an env default yields to an explicit --backend tmux, a flag in ProgramArguments would not. The plist own comment is precise about this ("Pinned rather than inherited. launchd does not run a shell..."); the prose ported from #139 rounds it off.
| self.db_path.parent.mkdir(parents=True, exist_ok=True) | ||
| shutil.move(str(OLD_DB_PATH), str(self.db_path)) | ||
| logger.info("Database migration complete") | ||
| legacy = next((p for p in (OLD_CONTRIB_DB_PATH, OLD_DB_PATH) if p.exists()), None) |
There was a problem hiding this comment.
[Suggestion] next(...) reports only the first legacy path that exists, and the warning stops once this database is non-empty - so if both OLD_CONTRIB_DB_PATH and OLD_DB_PATH are present, restoring the contrib one silences the function permanently and ~/.claude/event-bus.db is never mentioned.
Not a regression: _migrate_db_location had the same first-match precedence. But it moved the file, so the unmentioned one at least sat next to visible evidence something had happened; now reporting is the entire value of the function, and the docstring case against silence ("the real history sitting unnoticed on disk") applies just as well to the second file.
One line, and the message already handles a list naturally:
legacy = [p for p in (OLD_CONTRIB_DB_PATH, OLD_DB_PATH) if p.exists()]
if not legacy:
return...then name the newest as the restore source and mention the rest. Genuinely marginal - two pre-rename paths on one machine is unlikely - so equally fine to leave it and note the precedence in the docstring instead.
There was a problem hiding this comment.
Code Review — Round 6
Summary
Two changes since round 5: 63782cf (both round-4/5 suggestions taken) and 33fd9ad (merge of main carrying issue 139 bridge supervision, ported into docs/BRIDGE.md). Nothing in either names a way the code produces a wrong result, and nothing that worked in an earlier round is broken now.
Verified by hand rather than on trust:
tests/test_storage.py:1128—test_refuses_and_preserves_the_rowsnow snapshotssqlite_masteron the connection it already had open and asserts the table set is exactlysessions. That is the assertion the previous revision was missing: the row check alone held under the old ordering too. The reorder it pins is safe —_reject_prehistoric_schemaonly runsPRAGMA table_info(sessions),_get_schema_versionalready tolerates a missingschema_versiontable, and on a fresh file the PRAGMA returns nothing so the guard does not fire.docs/BRIDGE.md:179-196— the duplicated--allguidance is gone (parenthetical kept, the re-explaining sentence dropped) and the locking story is now its own bullet rather than trailing six other topics.- The merge is a content resolution, not a textual one. PR 139 supervision section landed in
guide.mdbridge docs — the section this branch moved — and its whole point contradicts what this branch relocated. All of it is present indocs/BRIDGE.md: "Running it supervised (macOS)", "Verifying supervision", the rewritten closing Supervision section, and the PATH parenthetical with its now-false "that lands with the supervision story" tail removed. The status banner and the README bridge paragraph — both stale in a way 139 could not see, since they live in files this branch created or had already edited — were corrected too. The deliberate deviation (webhook list --allin the crash-restart check, where 139 wrotewebhook list) is right: this branch is what makes a row invisible to a plain listing, and "is there exactly one row at this URL" is exactly the question--allanswers correctly. - The conftest change main brought composes. The
CLAUDE_CODE_SESSION_IDscrub moved fromtest_cli.pytoconftest.pyas an autouse fixture;test_cli.pystill importspytestforpytest.raises/parametrizein the round-1TestMainErrorHandlingclass, so the removal leaves no unused import.
Re-derived the load-bearing pieces from earlier rounds against the merged tree, since the merge touched cli.py:
--debugis still a top-level parser flag (cli.py:481), soargs.debugexists on every path reaching theexcept Exceptionarm inmain();SystemExitis not anException, so a handler own logical-error exit still passes through.- Migration ordering is still sound after v5 took over the two inline ALTERs: nothing in migrations 2-4 reads or writes
sessions.last_cursororevents.channel, so moving them from before_run_migrationsto position 5 changes no outcome._init_dbCREATEs declare both (storage.py:439,:450), so v5 is a true no-op on a fresh install. - The bridge sweep still lists
active_only=Falseand still matches onwh.get("url");_list_webhooks_implstill emitsurl,webhook_id, andactivefor inactive rows. storage.list_webhooksflipped default still has no caller relying on the old one (get_matching_webhooksand_list_webhooks_implpass it explicitly;conftest.py:86passesFalse).set_webhook_activeis covered end to end —TestSetWebhookActivedrives the impls includingget_matching_webhooks(the part that matters), the CLI pins both verb-to-state directions, andtest_tool_coverage_includes_every_known_toolhas it in the pinned 11-name roster.
Previously Addressed (Filtered)
All six round-1 findings, three round-2, three round-3, and both round-4/5 suggestions are implemented; none re-raised. The sessions.display_id NOT NULL divergence stays Deferred in KNOWN_CONSTRAINT_DIVERGENCES, guarded from both growth and staleness.
Verdict
APPROVE - No blocking findings. Converged at round 6 — remaining feedback is non-blocking. Three Suggestions posted inline: the plain webhook list empty-message now that all-paused is reachable, docs/BRIDGE.md naming a --backend flag the LaunchAgent does not use, and the legacy-path warning reporting only the first of two possible stale databases.
Note: pytest/uv are outside the tool allowlist here, so the 597-passing claim rests on CI and the author verification; this review is by inspection. Findings were posted as individual inline comments via the pulls/comments endpoint, since this environment command guard rejects the single-call heredoc form.
Automated review by Claude Code
#138 moved the bridge operator docs out of guide.md into docs/BRIDGE.md while this branch was open, so the rebase resolved both of my guide.md corrections away - the surrounding section no longer existed there. The plist and CLAUDE.md halves survived; these two did not. Same corrections, same file the content moved to. 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 .err-vs-.log split stands and is stated on its actual cause: basicConfig is stderr-only. The boot-order check was not reproducible as written. /health's `registered` is the startup result, cached and never re-verified, so unloading the bus under a bridge that install-bridge just left registered leaves /health reporting `true`. Added the missing restart step and the corollary - /health answers "did I register at startup", not "am I registered now" - which the /health bullet further down this same file already implied. make check green: 613 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AFp3uCezpvbeyAMYnNrQ4R
#138 moved the bridge operator docs out of guide.md into docs/BRIDGE.md while this branch was open, so the rebase resolved both of my guide.md corrections away - the surrounding section no longer existed there. The plist and CLAUDE.md halves survived; these two did not. Same corrections, same file the content moved to. 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 .err-vs-.log split stands and is stated on its actual cause: basicConfig is stderr-only. The boot-order check was not reproducible as written. /health's `registered` is the startup result, cached and never re-verified, so unloading the bus under a bridge that install-bridge just left registered leaves /health reporting `true`. Added the missing restart step and the corollary - /health answers "did I register at startup", not "am I registered now" - which the /health bullet further down this same file already implied. make check green: 613 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AFp3uCezpvbeyAMYnNrQ4R
#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.
2 participants
Audit-driven cleanup ahead of feature development. Four cleanup commits plus a review-fix commit, each independently reviewable. No feature work; the only user-visible addition is the webhook pause/resume tool that closes a half-built surface.
596 tests pass (569 on main before this branch), lint and format clean. Merged up to
main— includes #137.c7f5c70— dead code, duplicated wire shape, schema-parity guard_get_implicit_channelsdeleted. It took asession_idit never read and always returnedNone. Its three tests asserted that constant;TestBroadcastModelreplaces them by asserting the actual design decision — a session sees events on channels it doesn't belong to._event_to_dictand_webhook_payloadbuilt identical dicts, differing only inidvsevent_id, each with its own do-not-break-this comment. Both now delegate to_event_wire_dict, so a field added for one consumer reaches the other and the two can't drift onsignal_level. Both names kept — tests pin the delivery contract against_webhook_payloadspecifically._preview()for truncation duplicated in two places;storage.get_eventsrebuilds its WHERE clause from a condition list instead of four copies of the same block.TestSchemaParityadded._init_dbdefines the schema for fresh installs while the@migrationregistry upgrades existing databases, and nothing checked that they agree. A migrated database is now compared against a fresh one.144357d—call_toolraises instead of exitingcall_toollives incli.pybut is imported bybridge.py, and it terminated the process on failure. The bridge had three catch sites reverse-engineering intent from the exit code —isinstance(e, SystemExit) and e.code == 1— each wrapped in a paragraph explaining why that inference held.It now raises
BusUnreachableErrorwhen nothing answers and lets every other failure propagate with its own type. It never prints and never exits;main()owns that policy in one place, and a handler's ownsys.exitfor a logical error still passes through. Thedebugparameter is gone fromcall_tool(it only ever controlled printing and exiting) and stays amain()-level flag. The bridge gainsBridgeRegistrationErrorfor its own named checks.Net −60 lines of source, and the diagnostics improve. Registration retries used to log
SystemExit(1)for a down bus, a 401, and a malformed body alike:3ec41d8— webhook pause/resume, and bridge docs out of the MCP guideset_webhook_active. Theactivecolumn, the storage method, andlist_webhooks(active_only=)have existed since webhooks landed, but nothing in MCP or the CLI could setactiveto 0 — so in production it was always 1, makingactive_onlyandwebhook list --alla distinction without a difference. Adds the MCP tool andwebhook disable/enable <id>. Pausing keeps the registration, filters, and secret; unregistering stays permanent.storage.list_webhooksnow defaults toactive_only=True, matching the MCP tool. The two disagreed, and the unsafe direction is asymmetric: a caller that forgets the argument while deciding whom to deliver to would otherwise wake every webhook its owner had deliberately paused. Every existing caller passes it explicitly, so nothing changes today.docs/BRIDGE.md.guide.mdwas 658 lines, 300 of them the experimental bridge — longer than the entire rest of the API guide, in a file served as an MCP resource and therefore paid for in every session that reads it. Moved verbatim, with headings added (it had none) and a status banner. The guide keeps a pointer plus the one fact a woken session needs: drainwake/<session_id>.jsonl, one JSON event per line.0f5fe74— one mechanism for schema changeFour ways to change a schema existed, two invisible to tests. Now
_init_dbonly CREATEs and migrations only ALTER:@migration(5, "backfill_pre_registry_columns")takes over addingsessions.last_cursorandevents.channelfrom the inlinetry/except ALTER TABLEblocks. Conditional, so it's a no-op on any database at v4.webhooksCREATE — migration v3 owns it and runs for fresh installs too.TestSchemaParity's v1 fixture now omits those two columns, so it actually exercises migration 5.Two destructive legacy paths removed, neither replaced by silence:
_migrate_db_locationmoved the user's only copy out of a pre-rename path using a plain file move that is WAL-unsafe by this repo's own rules. It now reports the stale file and prints thesqlite3 .backupcommand, leaving it untouched — reporting matters, or a stale old-path database presents as a brand-new empty bus with the real history unnoticed on disk._migrate_sessions_schemaran an unconditionalDROP TABLE sessionson a pre-RFC-RFC: Session coherence across claude --continue calls #29 schema. It refuses with instructions instead, and a test asserts the rows survive the refusal.398f64d— review round 1One real defect this PR had introduced, plus five suggestions — all valid, all fixed:
bridge.py(blocking).register_with_busswept stale rows at its own hook URL withactive_only=True. Correct only while nothing could setactive = 0— whichset_webhook_activechanges. A paused row at the bridge's URL survived the sweep, a restart added a second row there, re-enabling the first made the bus deliver every DM twice, and shutdown unregistered only the newer id. Swept withactive_only=False. The regression test drives the real bus tool implementations, so reverting the argument reproduces two rows at one URL.TestSchemaParitywas weaker than its own description. It compared declared type only, soNOT NULL/DEFAULTdivergences passed silently — and one already did:sessions.display_idisNOT NULLfresh, nullable migrated, because migration v2 can onlyADD COLUMN. Now compares(type, notnull, pk, default)and indexCREATEstatements. That one divergence is recorded inKNOWN_CONSTRAINT_DIVERGENCESwith its reason (closing it needs a table rebuild — its own change); anything unlisted fails, type is still compared for listed columns, and a second test pins the list.set_webhook_activewas missing from the hand-listed async-tool guard. Derived from the registry instead, matched by tool type (duck-typing on.fn/.namecatches conftest'sMagicMock), with a companion test pinning the roster.--debugdidn't apply toBusUnreachableError— the one failure a user reaching for the flag is most likely debugging. Now checked first, uniform everywhere.docs/BRIDGE.md:Delivery outcomespromoted out ofSecurity and operation; blank lines before headings.d5bc7a2— mergemainBrings in #137 (
CLAUDE_CODE_SESSION_IDfallback), which touched the same two CLI handlers this PR refactored. Auto-merged cleanly, and verified as more than textual:_session_id_from_env()and thedebug-lesscall_toolsignature coexist, precedence still holds (--session-id>AGENT_EVENT_BUS_SESSION_ID>CLAUDE_CODE_SESSION_ID), and the new webhook verbs still work against a live bus.Database verification
Given
CLAUDE.md's protection rules, this was verified on a real upgrade rather than fixtures. A v4 database built by the previous revision ofstorage.py, populated with sessions, events (meta,correlation_id,channel), and a secret-carrying webhook, opens at v5 with every field intact, a single version row, and a schema matching a fresh v5 install.Also exercised against a live server and bridge: CLI error paths and exit codes; a bridge started before the bus retrying, registering when it came up, and spooling a DM to its wake file; and the full
webhook register → disable → list --all → enablecycle.SCHEMA_VERSIONmoves 4 → 5, so back up before deploying:Not done
Left from the audit, unaddressed: duplicate SSE parsing in
middleware.pyandcli.py; ~4.5s of the suite spent on real webhook backoff sleeps;make fmtlinting.while CI lintssrc tests; no coverage measurement; the staledocs/superpowers/specs/design doc. Also noted: closing thesessions.display_idconstraint divergence needs a table-rebuild migration.Generated by Claude Code