refactor: A-tier cleanup (dead code, stale comments, redundant handling) - #473
Merged
Conversation
_run_notification_check's call to drain_meeting_events was always a no-op in this process: the event queue it reads from is an in-process queue.Queue owned by tether_premium.bot.scheduling.events, populated by a WS-listener thread that only ever starts inside bot/message_handler.py's run_polling() (the BOT process). The API process (where _run_notification_check runs) cannot see that queue. Meeting-event draining happens exclusively via the bot's polling loop, unaffected by this change. Delete the dead block (import, _notify_meeting_send closure, meeting_send lambda, drain_meeting_events call) and update the surrounding docstrings (module header, _run_notification_check) to state this explicitly instead of implying meeting events are handled here.
Drop the unused now_ts local (dead "reserved for future observability" placeholder that was never read) and the two empty else: pass branches (a None estimate already means "nothing to write" without needing an explicit no-op branch to say so). Also fixes a stale docstring claim that tether-premium's drain_meeting_events contributes a "meeting" component via this same mechanism -- confirmed there are zero writers of that component anywhere in the codebase today (see A7). Behavior unchanged: anchor/followup components are still written only when a non-None estimate is available.
_check_anchor_transitions already wraps its entire body in a catch-all except Exception (logs at ERROR, returns None) -- it can never raise, so the outer try/except at its call site in _run_notification_check was dead code that could never trigger. Removed; the inner catch remains the sole (and sufficient) error boundary. The sibling check_followups() call keeps its own try/except since that function has no internal catch-all.
…_node_response Both were imported at the top of _build_node_response() but never called directly in its body -- all actual usage happens in _add_children(), which already has its own local import of the same names. No behavior change.
- tether_mcp/server.py: header comment claimed "9 Consolidated MCP Tools" but 16 tools are registered today (verified: `grep -c '^@mcp.tool()'`). Updated to state the actual count and point at the consolidation spec as a separate tracked effort, rather than asserting a target as fact. - api/routes/internal.py: module docstring and /notifications/check's docstring both asserted "Called by Fly.io cron [machines/every minute]" -- unverified (no Fly scheduled-machine config exists anywhere in this repo; the actual trigger mechanism/cadence is out-of-band deployment config). Reworded to describe the endpoint's actual contract (internal, token-gated, notify_due-gated so safe at any cadence) without asserting a specific unverified scheduling mechanism.
Verified zero writers anywhere in tether or tether-premium (grepped both checkouts for the "meeting" component string) -- the reservation was never used and, per A1, the code path that would have used it (API-process meeting drain) was itself dead. Removed the Literal value, rewrote the docstring paragraph to state plainly that meeting events don't fit this module's model rather than describing a reservation nothing consumes, and deleted the test that only existed to exercise that reservation.
_get_client previously constructed a brand-new redis.asyncio client (and its underlying connection pool) on every single gated call -- e.g. every ~30s polling tick in bot/message_handler.py's run_polling, or every cron invocation of _run_notification_check -- rather than reusing one client per process. Added a module-level dict cache keyed by REDIS_URL so real (URL-based) clients are constructed once and reused. Test-injection paths are deliberately untouched: an explicitly-passed redis_client is still returned as-is every call (never substituted with a cached instance), and the fakeredis `server=` path still returns a fresh FakeRedis wrapper per call (sharing state via the FakeServer, not via a cached client object) -- exactly the behavior every existing test already depends on. 4 new tests cover the caching behavior and confirm both test-injection paths remain uncached.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Behavior-preserving A-tier cleanup pass per review-findings-2026-07-04.md, one commit per item:
drain_meeting_eventsblock inapi/routes/internal.py(_EVENT_QUEUEis process-local; listener only starts in the bot process, so this code path never ran in the API process) + stale docstring refs ininternal.py/shared/notify_due.py._recompute_and_cache_due(~35 -> ~15 lines): removed unusednow_ts, emptyelse: passbranches, debug log._check_anchor_transitions(inner handler already catches all).get_children/get_node_tree_distanceimports intether_mcp/tools/read_context.py_build_node_response.server.py, cron wording ininternal.py)."meeting"component fromnotify_dueComponentLiteral + its docstring paragraph (verified zero writers).notify_due._get_client(keptredis_client/servertest-injection params; fakeredis tests still pass).A4 (premium doc-cruft) and A8 (scope_mode deletion) are intentionally excluded — A4 belongs to the separate 0c-p brief, A8 is deferred to 1b to avoid two PRs churning
permissions.py.Test plan
origin/devbaseline (missingclaude_agent_sdkmodule in this env + in-flight SDK migration test failures) — zero new failures introduced by this branch.grepconfirms no remaining references to removed symbols.