Route every non-ORM Redis client through utils/redis_client.py

[tomcounsell]

Closes #3003

What

Production modules built their own redis client from REDIS_URL at call time, with a hardcoded fallback to production db 0. Popoto's pool -- the one connection tests/conftest.py repoints at the claimed test database and config/redis_bootstrap.py rebuilds with retry policy -- never reached them; under test they wrote to production (measured in #2805).

utils/redis_client.py is now the one place production code obtains a client for non-ORM keys, derived from POPOTO_REDIS_DB's live pool:

Accessor	Returns
text_redis()	decode_responses=True client on popoto's host/port/db/auth, its own bounded pool, socket timeouts from settings.timeouts.redis_socket_s; cached, rebuilt when popoto's pool identity changes
bytes_redis()	POPOTO_REDIS_DB itself
derived_redis(**overrides)	fresh client on popoto's identity with caller kwargs (the two pubsub connections)
scan_keys(client, match)	bounded cursor sweep, returns (keys, truncated) -- the sanctioned replacement for KEYS

Each module keeps its one-line _get_redis() / _get_redis_connection() seam delegating to the accessor, so existing test patches on those names still hold. The redis_url constructor parameter on TelegramRelayOutputHandler and EmailOutputHandler is gone (no production caller passed it); both honour an explicitly assigned self._redis and otherwise resolve through text_redis().

Scope of this diff (corrected)

An earlier revision of this description claimed twenty-four sites across eighteen modules. That described the original pre-rebase change, not what is on the branch. utils/redis_client.py and the call sites that already import it landed on main separately as b1a506770 during the 449-commit rebase.

Measured against origin/main at the current head, this diff converts 13 raw-client seams across 13 modules:

Module	Keys	Now
agent/output_handler.py	telegram:outbox:*, email:outbox:*, metrics:*	text_redis()
bridge/dedup.py	bridge:last_event:*, msgclaim:*, dm_coverage_epoch:*	text_redis()
bridge/email_bridge.py	email:last_poll_ts, auth_failed, history:*, msgid:*	text_redis()
bridge/email_dead_letter.py	email:dead_letter:*	text_redis()
bridge/email_relay.py	email:outbox:*, email:relay:last_poll_ts	text_redis()
bridge/liveness.py	bridge:last_update_received, last_probe_ok, last_missed_recovery	text_redis()
bridge/routing.py	customer_resolver:*, resolver:failures:*	bytes_redis() (decodes its own values)
monitoring/bridge_watchdog.py	the same liveness keys bridge/liveness.py writes	text_redis()
reflections/pm_briefings/delivery.py	telegram:outbox:*	text_redis()
tools/email_history/__init__.py	email:history:*, email:threads	text_redis()
tools/react_with_emoji.py	telegram:outbox:*	text_redis()
tools/valor_email.py	email:outbox:*	text_redis()
ui/app.py	{project}:session-health:slot_reclaims, worker:slot:leases:*, worker:watchdog:actions:*, email:*	text_redis() (one module seam, three call sites)

bridge/telegram_relay.py is also touched, but its accessor seam landed with b1a506770; what this diff changes there is the send path (below). The review's independent sweep put the figure at fourteen seams; the count above is git diff -U0 origin/main...HEAD filtered to added accessor returns, one per module, and I could not reproduce a fourteenth. If the reviewer's fourteenth was agent/output_handler.py's second handler class, both handlers do resolve through the single _get_redis seam counted here.

monitoring/bridge_watchdog.py was found by re-sweeping, not by working the issue's list. It survived because the recurrence guard scanned eight hand-picked packages and monitoring/ was not among them: the guard was exactly as wide as the list that produced it, so it certified its own blind spot as clean. The watchdog reads the freeform liveness keys bridge/liveness.py writes -- precisely this issue's failure -- so it could read production while the bridge under test wrote to the claimed db, and reach a wedge verdict on the wrong database's data.

Every site reaches genuinely non-ORM keys; none touched a Popoto-managed key raw, so no ORM migration was needed. bridge/dedup.py, which the issue suspected, already reads LastProcessedRecord through the ORM.

Send-path outage visibility

Both outbox relays swept their queue with a full-keyspace KEYS, which under the accessor now inherits text_redis()'s 5s socket timeout. The resulting TimeoutError was caught by a blanket except Exception and process_outbox returned 0 -- indistinguishable from "nothing to send". The relay would keep looping and every health signal would stay green. This system has already paid for that shape once: a swallowed process_outbox exception dropped every Telegram reply for 26 hours.

• utils/redis_client.scan_keys() replaces KEYS: cursor-based SCAN, bounded per round trip by REDIS__SCAN_COUNT and in total by REDIS__SCAN_KEY_LIMIT, returning a truncation flag the caller logs.
• bridge/relay_errors.py (new) holds the failure contract: OutboxUnavailableError plus report_send_path_failure(), which logs at ERROR and captures to Sentry, and never raises (a reporting failure must not become a second outage). A RedisError from the sweep now raises and is reported rather than folded into the return value. Per-message failures -- malformed payload, rejected recipient, failed SMTP -- stay tolerant exactly as before; only a failure to reach the queue is an outage.
• Both relay loops gained an explicit except OutboxUnavailableError branch counting consecutive outages.
• The email relay's heartbeat now stamps after the sweep: a cycle that could not read the outbox must not report itself healthy.

The review named only bridge/email_relay.py:262. bridge/telegram_relay.py:1332 had the identical KEYS-under-blanket-handler shape on the Telegram send path -- literally the surface of the 26-hour outage. Both are fixed, as is the unbounded scan_iter (no count=, so bounded per round trip but unbounded in wall time) in bridge/email_dead_letter.py:82.

Sweep for other unbounded operations under a blanket handler across agent/ bridge/ tools/ ui/ monitoring/ reflections/ worker/: the remaining hits are agent/session_health.py (POPOTO_REDIS_DB.keys(...) and an smembers) and agent/session_stall_classifier.py:182 (an unbounded cursor iteration with no count=, on POPOTO_REDIS_DB, under a blanket except Exception) -- the same shape as the bridge/email_dead_letter.py case this PR fixes. The third was missed by the original sweep and is added here on re-review. Those run on popoto's own client, are not converted by this PR, and so do not inherit the new socket timeout -- out of scope here, but worth a follow-up.

Connection-policy drift (previously undisclosed)

• bridge/routing.py on the event loop. resolve_customer is async def and called sync Redis through bytes_redis(), which is popoto's client on a BlockingConnectionPool(max_connections=128, timeout=20). A pool checkout under exhaustion blocks the calling thread -- and socket_timeout does not cover a checkout, so the ceiling is the pool's 20s timeout, on the bridge's event loop. _on_resolver_failure compounds it with a synchronous IMAP STORE. All four sites (get, the failure handler, setex, the two deletes) now go through asyncio.to_thread. invalidate_customer_cache is a sync function with no production caller at all and is unchanged. Correction (re-review): this PR originally claimed the same of get_resolver_failure_count, and that was wrong. It has exactly one production caller -- bridge/email_bridge.py's _arm_resolver_unavailable_alert_if_persistent, which _process_inbound_email (async def) called synchronously. So this PR did move a blocking-pool checkout onto the bridge event loop, one frame above the four sites wrapped here, on the already-degraded resolver-unavailable path. Fixed: the whole helper now goes through asyncio.to_thread, which also moves its other three Redis round trips off the loop. TestBlockingPoolNeverReachesAnEventLoop now closes the class -- it taints every sync function reaching bytes_redis(), transitively and across import edges, and fails on any unwrapped call from a coroutine.
• text_redis() builds its own bounded pool, max_connections=REDIS__MAX_CONNECTIONS (128) with health_check_interval=REDIS__HEALTH_CHECK_INTERVAL_S (30), rather than inheriting popoto's blocking pool.
• Socket timeout. text_redis() carries a request/response timeout (redis_socket_s, 5s) where from_url carried none. No converted site issues a blocking list op (no blpop/brpop/blmove anywhere in the converted modules).
• TLS. _identity_kwargs now derives from popoto's sibling_client_kwargs whitelist plus the two things that whitelist cannot see: TLS and unix sockets. TLS lives in connection_pool.connection_class (SSLConnection), not in connection_kwargs, so a kwargs-copying derivation silently dropped it -- a rediss:// popoto pool produced a plaintext sibling. Verified by hand across redis://, rediss:// and unix://.

New settings on RedisSettings, all env-overridable and provisional: REDIS__MAX_CONNECTIONS, REDIS__HEALTH_CHECK_INTERVAL_S, REDIS__SCAN_COUNT, REDIS__SCAN_KEY_LIMIT.

Recurrence guards

tests/unit/test_redis_client_accessor.py::TestNoRawClientsInProduction walks every top-level production package by AST and fails on any redis.Redis / StrictRedis / from_url call, under any alias, outside utils/redis_client.py. The package list is enumerated from the tree rather than hand-picked -- thirteen packages, adding analytics, mcp_servers, monitoring, scripts, utils to the original eight -- and test_the_scan_covers_every_production_package asserts the list still covers the repo. That companion test is the actual fix for the monitoring/ miss: a hand-maintained list fails silently when a package is added, and its failure mode is a guard reporting green over code it never opened.

tests/unit/test_relay_send_path_outage.py::TestNoProductionKeysCall is the second guard: no send-path module may call KEYS or an unbounded scan_iter. It matches .keys as a bare attribute, not as a call -- both relays spelled the defect asyncio.to_thread(r.keys, PATTERN), a reference handed to the threadpool, so a call-only matcher reported both send paths clean while the full-keyspace KEYS sat in plain sight. That miss is recorded in an inline comment.

Why not widen .claude/hooks/validators/validate_no_raw_redis_delete.py instead. That validator is a PreToolUse hook matching the text of a Bash command, catching raw Redis typed against Popoto-managed keys at the session boundary. It never reads the repo's source. The defect here is a committed Python module constructing a client, which no command-text matcher can see and which must fail in CI, not in one agent's session. Complementary surfaces; both stay.

Tests

• New: tests/unit/test_redis_client_accessor.py (13 tests); tests/unit/test_relay_send_path_outage.py (10 tests).
• Updated: tests/unit/output_handler/ (11 constructor call sites) and tests/integration/test_message_drafter_integration.py (2) drop the removed redis_url= kwarg; tests/unit/test_bridge_relay.py (21 sites) mocks scan instead of keys; test_ui_app.py patches ui.app._get_redis instead of redis.Redis.from_url.

All runs serial (-n 0) via scripts/pytest-clean.sh, because the machine's fifteen test-DB slots are contended by sibling lanes. Measured after the rebase onto 8e6a0f607 and after uv sync --extra dev (pydantic-ai-slim / pydantic-graph 2.40.0 -> 2.41.0, confirmed installed in the worktree venv). Exit codes captured, not inferred; every set exited 0.

Files	Result
tests/unit/output_handler/	108 passed
test_relay_send_path_outage.py, test_redis_client_accessor.py, test_bridge_relay.py, test_dead_letters.py	126 passed
test_bridge_watchdog.py, test_bridge_liveness.py, test_ui_app.py, test_dedup.py, test_routing.py, test_email_bridge.py	308 passed, 1 skipped
test_email_history.py, test_valor_email.py, test_react_with_emoji.py, test_tool_call_delivery.py, test_session_executor_runner_dispatch.py, test_settings.py, test_env_completeness.py	225 passed
tests/integration/test_message_drafter_integration.py	8 passed

tests/unit/output_handler/ was absent from this table in the earlier revision, which is exactly why a directory with 61 failures reached review. It is green now.

Red-before-green on both guards, replayed against the current base 8e6a0f607: the widened package scan reports a raw construction in main's monitoring/bridge_watchdog.py and none against this branch. The send-path AST guard reports bridge/email_relay.py:263 KEYS, bridge/telegram_relay.py:1332 KEYS, and bridge/email_dead_letter.py:84 unbounded scan_iter on the base; all three are clean on this branch.

Base

Rebased onto 8e6a0f607 (after #3168). No textual conflict, and no semantic one: #3168 rewrote monitoring/bridge_watchdog.py's is_bridge_running (pgrep -> tools.process_lookup.find_python_service_pids) and annotated kill_stale_processes, while this PR's only edit to that file is _get_watchdog_redis. ui/app.py is not in #3168's diff.

Deploy note

This changes bridge, worker and relay code. After merge: ./scripts/valor-service.sh restart, plus worker-restart and email-restart.

Docs

docs/features/redis-client-accessor.md, indexed in docs/features/README.md; the #3003 row leaves docs/bug-backlog-waves.md.

[valorengels]

Review (Judge code-quality):

Verdict: CHANGES REQUESTED — head 50145291b

Blockers

1. The removed redis_url kwarg breaks 13 existing test call sites.
agent/output_handler.py:506 narrows TelegramRelayOutputHandler.__init__ to (self, file_handler=None). Eleven call sites in tests/unit/output_handler/ and two in tests/integration/test_message_drafter_integration.py still pass redis_url=:

tests/unit/output_handler/test_output_handler_filters.py:25, 399
tests/unit/output_handler/test_output_handler_delivery.py:245, 401
tests/unit/output_handler/test_output_handler_handlers.py:206, 421
tests/unit/output_handler/test_output_handler_transport.py:54, 198, 464
tests/integration/test_message_drafter_integration.py:134, 297

Measured on this branch:

$ scripts/pytest-clean.sh -n 0 -q tests/unit/output_handler/
61 failed, 47 passed in 70.55s

E   TypeError: TelegramRelayOutputHandler.__init__() got an unexpected keyword argument 'redis_url'

All production constructor sites are clean (tools/send_message.py:270,366, tools/ask_poll.py:168, bridge/telegram_bridge.py:3143, worker/__main__.py:598), so this is a pure test-fix — but tests/unit/output_handler/ is the suite that directly exercises the class this PR changed, and it does not appear in the PR's test table. The PR's claimed "525 passed, 1 skipped" reproduces exactly on the set it names; the set is what is wrong.

Tech Debt

2. _identity_kwargs silently drops TLS. utils/redis_client.py:66-79 copies host/port/db/username/password/path out of connection_pool.connection_kwargs. TLS is not in connection_kwargs — it lives in connection_pool.connection_class (SSLConnection). With a rediss:// URL, popoto talks TLS and every converted site builds a plaintext client against the TLS port. Not reachable on today's localhost deployment, but this PR is what makes the module the fleet-wide chokepoint. Popoto ships popoto.redis_db.sibling_client_kwargs() for this job with an ssl*-aware whitelist; if it was rejected (it misses the unix-socket path case), say why in a comment.

3. The module docstring and the shipped doc overclaim what a derived client inherits. utils/redis_client.py:5-9 and docs/features/redis-client-accessor.md:16-21: "config/redis_bootstrap.py rebuilds it with retry policy at startup ... A client derived from that pool's identity follows both for free." It follows the repoint, not the retry policy — _identity_kwargs copies connection identity only, so no Retry/ExponentialBackoff and no health_check_interval=30 (config/redis_bootstrap.py:137) reach text_redis() clients. Fifteen lines later the same file says "retry policy, keepalive, protocol ... is chosen by each accessor below" — and no accessor chooses one. Two adjacent paragraphs assert opposite things; the doc repeats the wrong one.

4. _IDENTITY_KEYS is dead. utils/redis_client.py:53. grep -rn "_IDENTITY_KEYS" over the whole repo returns only the definition. _identity_kwargs hardcodes its own, divergent handling inline (it remaps path → unix_socket_path, which the constant doesn't record). Ruff won't flag a module-level constant, and the six-line comment above it reads as the source of truth.

5. The "one-line seam per module" claim doesn't hold for agent/output_handler.py. PR body and docs/features/redis-client-accessor.md:44-46 both say each module keeps a module-level _get_redis() seam. True of thirteen modules; agent/output_handler.py has only the method-local import inside TelegramRelayOutputHandler._get_redis.

Nits

6. ui/app.py:29-33 — _get_redis() is defined between the last import and logger = logging.getLogger(__name__) (line 36), splitting the module header. Every other converted module puts the seam next to its callers.

7. agent/output_handler.py:506-517 / bridge/email_bridge.py:891-897 — self._redis is never written by production code; it exists solely as a test-injection point. Legitimate seam, but the comment ("An explicitly assigned client wins") should say it is a test seam.

8. tests/unit/test_redis_client_accessor.py:63-68 — bare assignment to redis_client._cached_text_client / _cached_text_identity with no monkeypatch.setattr and no restore; it also bypasses the previous.close() in text_redis(), leaking that client's pool for the rest of the process. Every other test in the file uses monkeypatch.

9. The AST guard's alias handling is narrower than its docstring. _raw_client_constructions (tests/unit/test_redis_client_accessor.py:150-180) matches only ast.Attribute bases, so from redis import Redis; Redis(...), redis.asyncio.Redis(...), and ConnectionPool.from_url(...) all pass. No current offenders — verified by grep across all thirteen scanned packages — so this is latent, not an active miss.

Verified clean

• No stale cached client can survive a test-db repoint. identity_key = (id(pool), sorted(identity)) — the identity half fully determines the target, so even an id() reuse after GC is caught by the identity mismatch. The id(pool) component correctly forces a rebuild when configure_resilient_redis() swaps in a same-target pool at startup. No finding.
• No dead imports at converted sites. os is still used in every module that kept the import; import redis is still needed for the -> redis.Redis annotations at bridge/liveness.py:64 and monitoring/bridge_watchdog.py:209,224. The three files that dropped import os genuinely have no remaining use.
• TTLs, decode_responses, pipeline semantics preserved per site. No TTL constant or ex=/expire() call changed anywhere in the diff. bridge/routing.py's decode_responses=False is preserved: popoto's client carries no decode_responses kwarg, so it defaults to False. The only pipeline user among the converted sites is bridge/email_bridge.py:786,798 (transaction=True); pipelines check out their own connection, so sharing the client is safe.
• No converted site closes the shared client. The one .close() in monitoring/bridge_watchdog.py:102 is a logging handler. derived_redis()'s "caller owns the lifetime" contract is honored at agent/agent_session_queue.py:1068 and :1169.
• ruff check . and ruff format --check . both clean on this branch.

[valorengels]

Review (Judge risk):

Verdict: CHANGES REQUESTED — head 50145291b

Blockers

1. bridge/email_relay.py:262 — a full-keyspace KEYS now runs under a 5s socket timeout, and the timeout stops the relay.

# bridge/email_relay.py:261-262
r = await asyncio.to_thread(_get_redis_connection)
keys = await asyncio.to_thread(r.keys, EMAIL_OUTBOX_KEY_PATTERN)

_get_redis_connection() (line 67-71) now returns text_redis(), which carries socket_timeout=5.0 / socket_connect_timeout=5.0 (utils/redis_client.py:108-114; settings.timeouts.redis_socket_s default 5.0). The old redis.Redis.from_url(url, decode_responses=True) carried no socket_timeout — this call waited as long as it took.

KEYS is O(keyspace). This is a machine-global Redis that has historically carried millions of keys (#2207 recorded ~6.2M phantom AgentSession keys). Once one sweep crosses 5s it raises redis.exceptions.TimeoutError, caught by the blanket handler:

# bridge/email_relay.py:301-303
    except Exception as e:
        logger.error(f"Email relay: outbox processing error: {e}", exc_info=True)
    return sent

process_outbox returns sent=0, run_email_relay sleeps and loops forever, and the outbox grows — which does not make the next sweep faster. This is not visible in tests: a claimed test DB holds a handful of keys, so KEYS returns in microseconds.

One honest mitigation, since it bears on severity: the heartbeat write at line 267-272 comes after the KEYS, so a timeout also skips the heartbeat and email-status will report a stale relay. The outage is detectable, unlike the fully-silent #3168 shape. It is still an undisclosed behavior change on the outbound send path, and the fix is one line — either scan_iter with a bound, or derived_redis(decode_responses=True, socket_timeout=None) for this call. The KEYS itself is the underlying defect.

The PR body's disclosure ("No converted site issues a blocking list op") is accurate as far as it goes — there is no blpop/brpop/blmove anywhere in the converted modules, confirmed — but a blocking list op is not the only way to exceed a request timeout.

Tech Debt

2. bridge/routing.py now blocks the bridge event loop on popoto's capped pool. bytes_redis() returns POPOTO_REDIS_DB itself (utils/redis_client.py:82-84). Popoto builds that on a BlockingConnectionPool(max_connections=128), and redis-py's BlockingConnectionPool defaults to timeout=20 — on exhaustion a checkout blocks rather than erroring. resolve_customer is async def (bridge/routing.py:1484) and calls the synchronous client directly on the event loop (:1537-1538, same shape at :1627 and :1756). Previously each call had its own effectively-unbounded private pool, so a checkout never blocked. The resolver cache now competes with every ORM operation in the process for 128 slots, and socket_timeout does not cover a pool-checkout block. Resolution volume is low, so this is unlikely to bite — but it is a second undisclosed behavior change, and neither the PR body nor docs/features/redis-client-accessor.md mentions pool sharing.

Related: bytes_redis() hands out popoto's client object, not a copy. Nothing closes it today (checked bridge/, agent/, worker/, monitoring/, ui/, tools/, reflections/), but derived_redis's docstring says "the caller owns the connection's lifetime and closes it" three lines away — an easy contract to copy onto the wrong accessor, where .close() would disconnect() the ORM's own pool. Worth one sentence on bytes_redis.

3. text_redis() drops popoto's back-pressure cap and health check. utils/redis_client.py:109-114 builds a plain redis.Redis(...), which gets redis-py's default ConnectionPool — max_connections == 2147483648, health_check_interval == 0. Popoto deliberately caps its own pool at 128 with a BlockingConnectionPool so a burst of concurrent coroutines cannot exceed the server's maxclients, and config/redis_bootstrap.py:137 sets health_check_interval=30. This one client is now the sole Redis handle for the bridge event loop, asyncio.to_thread workers in email_relay, and the FastAPI threadpool — the exact burst profile popoto capped against, with nothing bounding concurrent sockets. Not a blocker: redis-py 7.4's default retry is Retry(ExponentialWithJitterBackoff(), 3) over ConnectionError/TimeoutError, so a stale pooled socket self-heals without health_check_interval, and tools/redis_flush_guard.py patches redis.Redis at class level so this client is still covered by the db-0 flush guard.

4. bridge/email_dead_letter.py:82 — list(r.scan_iter(f"{DEAD_LETTER_KEY_PREFIX}*")) is a full-keyspace scan with no count=, now running on the shared process-wide client. Each round trip stays under 5s (COUNT 10 default), so it will not raise, but the loop is unbounded in wall time. Pre-existing inefficiency; the conversion is what makes it share a pool.

Nits

5. Deploy note. ./scripts/valor-service.sh restart restarts the bridge and the watchdog only (scripts/valor-service.sh:309-314). This PR also touches bridge/email_bridge.py, email_relay.py, email_dead_letter.py, routing.py — all in the separate email-bridge launchd service, restarted only by email-restart (line 1304) — plus worker/ (worker-restart) and ui/app.py (its own uvicorn on 8500). Functionally benign, since both code paths resolve the same db, but the merge step should name email-restart and worker-restart explicitly rather than the one-line restart note in CLAUDE.md.

Verified clean

• monitoring/bridge_watchdog.py cannot produce a false wedge verdict from this change. A Redis error inside assess_update_flow is fail-safe (:313-320 → "bridge_update_flow_signal_unreadable — Redis error, treating as live"), and the call site at :668-675 wraps _get_watchdog_redis() itself → update_flow_live = True. The new 5s timeout can only produce "treat as live", never a restart.
• Wrong-db read in the watchdog: no change. configure_resilient_redis() is not called in the watchdog process, so popoto's import-time path reads REDIS_URL and falls back to 127.0.0.1:6379 db 0 — byte-identical to the redis://localhost:6379/0 literal the old _get_watchdog_redis used. The new popoto import is function-local (:217), which is what tests/unit/test_recovery_respawn_safety.py:826-840 requires.
• Fork / thread / event-loop safety. No os.fork, multiprocessing, or ProcessPool in bridge/, worker/, agent/, monitoring/, ui/, tools/. The dashboard is single-process (uvicorn.run(..., factory=True), no workers=); all three ui/app.py accessor sites are sync def endpoints on the threadpool, and redis-py's sync client is pool-level thread-safe. Net connection churn is better than before — the old per-call from_url built a fresh pool and socket on every _get_redis().
• No other converted site has a slow-path timeout risk. ui/app.py:594 is lrange(..., 0, 4); bridge/email_bridge.py:786,798 pipelines are bounded by HISTORY_MAX_ENTRIES; tools/email_history/__init__.py:78,240 (mget, hgetall) are CLI-only, where a timeout is a visible error.

[valorengels]

Review: CHANGES REQUESTED

Head 50145291b. Mode: independent roster (2 judges) — code-quality and risk dispatched as separate agents with the same PR context and no sight of each other's findings; per-judge comments posted above. Consensus rule any-blocker-wins. No plan document exists for #3003 (bug-backlog wave item), so this was reviewed against the issue and the PR body.

The core of this change is right and the sweep is genuinely complete. Two blockers stand between it and merge, one of which is mechanical.

Independent sweep — confirms the claim

Re-run on this branch rather than trusting the PR's count. Across all thirteen production packages (agent analytics bridge config mcp_servers models monitoring reflections scripts tools ui utils worker):

• Raw client construction outside utils/redis_client.py: zero.
• Ad-hoc REDIS_URL reads in production code: zero. The only remaining mentions are two CLI error strings (tools/valor_email.py:472, tools/valor_telegram.py:1123), the settings field description, config/redis_bootstrap.py (the sanctioned bootstrap), and docstrings.

I can't independently reproduce "24" as a historical total — six sites landed on main separately as b1a506770 before the rebase. What this diff converts is 14 seams across 13 modules, and after it, nothing in the tree builds its own client. That is the claim that matters and it holds.

The recurrence guard fires

Tested rather than read. Appending a raw client to analytics/__init__.py and re-running the scan:

E   AssertionError: Raw Redis client construction outside utils/redis_client.py. ... analytics/__init__.py:20

Reverted immediately. test_the_scan_covers_every_production_package is the right fix for the monitoring/ miss — it is what turns a hand-maintained list from a silent blind spot into a failing test. No false positives: the full 13-package scan is green on this branch, and the only exemption is utils/redis_client.py by explicit path.

tests/unit/test_redis_client_accessor.py: 13 passed. The PR's claimed set (test_bridge_watchdog, test_bridge_liveness, test_dedup, test_ui_app, test_email_relay, test_email_history, test_valor_email, test_react_with_emoji, test_dead_letters, test_tool_call_delivery, test_session_executor_runner_dispatch, test_email_bridge, test_routing) reproduces exactly: 525 passed, 1 skipped. ruff check and ruff format --check both clean.

Blockers

1. The removed redis_url kwarg breaks 13 test call sites (code-quality judge). tests/unit/output_handler/ — the suite that most directly exercises the class this PR changed — is absent from the PR's test table, and fails 61 tests with TypeError: TelegramRelayOutputHandler.__init__() got an unexpected keyword argument 'redis_url'. Two more sites in tests/integration/test_message_drafter_integration.py:134,297. Every production constructor site is clean, so this is a test fix, not a design problem — but the PR's own numbers were measured on a set that skipped it.

2. bridge/email_relay.py:262 — KEYS over the full keyspace now runs under a 5s socket timeout (risk judge). text_redis() adds socket_timeout=5.0 where from_url had none. KEYS is O(keyspace) on a Redis that has historically held millions of keys, and a TimeoutError there is swallowed by the blanket handler at :301-303, stalling the email send path in a loop that keeps looking alive. Detectable — the heartbeat write sits after the KEYS, so email-status would go stale — but it is an undisclosed behavior change on the outbound path, and the fix is one line (scan_iter with a bound, or derived_redis(..., socket_timeout=None) for this call).

The PR's disclosure sentence is accurate but too narrow. There is genuinely no blpop/brpop/blmove anywhere in the converted modules — I checked all fourteen — but a blocking list op is not the only way to exceed a request timeout.

Behavior preservation — per site

Spot-checked every converted site, not a sample. No TTL, ex=, expire(), or decode_responses value changed anywhere in the diff. bridge/routing.py's decode_responses=False is preserved through bytes_redis() (popoto's client carries no decode_responses kwarg, so it defaults to False). The only pipeline user, bridge/email_bridge.py:786,798 (transaction=True), is safe under a shared client since pipelines check out their own connection. No converted site closes the client it is handed. The text_redis() cache cannot serve a stale client across a test-db repoint: the identity half of the cache key fully determines the connection target, so an id() collision after GC still misses.

The drift that is real is all in connection policy, and it is undisclosed in both the PR body and docs/features/redis-client-accessor.md: the 5s timeout (named, but only for text_redis), bridge/routing.py moving onto popoto's BlockingConnectionPool(128, timeout=20) from an async call site, and text_redis() getting redis-py's unbounded default pool with no health_check_interval — dropping the cap popoto sets deliberately. Details in the two judge comments.

Tech Debt

• _identity_kwargs (utils/redis_client.py:66-79) silently drops TLS — rediss:// is expressed in connection_class, not connection_kwargs, so every converted site would build a plaintext client against a TLS port. Unreachable on today's localhost deployment; this PR is what makes the module the fleet-wide chokepoint.
• The module docstring and docs/features/redis-client-accessor.md:16-21 claim a derived client follows the bootstrap's retry policy "for free". It doesn't — only the repoint. Fifteen lines later the same file says each accessor chooses its own retry policy, and none does.
• _IDENTITY_KEYS (utils/redis_client.py:53) is dead — referenced nowhere in the repo, and it disagrees with the inline handling in _identity_kwargs it purports to describe.
• bridge/routing.py pool sharing and text_redis()'s unbounded pool, above.
• bridge/email_dead_letter.py:82 — unbounded full-keyspace scan_iter with no count=, now on the shared client.
• The "one-line _get_redis() seam per module" claim in the PR body and the doc doesn't hold for agent/output_handler.py.

Nits

• ui/app.py:29-33 — the seam is wedged between the imports and logger.
• self._redis on both handlers is a test-injection point never written by production code; the comment should say so.
• tests/unit/test_redis_client_accessor.py:63-68 mutates module globals by bare assignment with no restore, leaking the evicted client's pool.
• The AST guard's alias handling is narrower than its docstring: from redis import Redis, redis.asyncio.Redis, and ConnectionPool.from_url all pass. No current offenders, so latent.
• Deploy: ./scripts/valor-service.sh restart covers the bridge and watchdog only. This diff also touches the separate email-bridge service, worker/, and the dashboard — email-restart and worker-restart belong in the merge step.

Category judgment

Confirmed: no Popoto model was invented to satisfy the rule. Every converted key is genuinely freeform — outbox lists, liveness stamps, dedup claims, resolver cache, dead letters, email history. bridge/dedup.py already reads LastProcessedRecord through the ORM and its raw client only ever served bridge:last_event / msgclaim / dm_coverage_epoch. bytes_redis() for bridge/routing.py and derived_redis() for the two pubsub connections are the right categories rather than a forced text_redis().

The monitoring/ finding in this PR is the best part of it: a guard exactly as wide as the list that produced it, certifying its own blind spot. test_the_scan_covers_every_production_package is the durable fix.

[valorengels]

Review (Judge code-quality):

Head reviewed: 9237797c18cbe0895ff66318f3289793bd99a0a3 (resolved via tools/pr_head_resolver.py::resolve_pr_head_sha).

Blockers

• None from this lens. (The event-loop finding below is raised as a blocker by the risk judge; this lens classified it tech debt. See the aggregate comment for the adjudication.)

Tech Debt

1. scan_keys bounds the keys returned, not the keyspace traversed. utils/redis_client.py:167-175:

    keys: list[Any] = []
    cursor = 0
    while True:
        cursor, batch = client.scan(cursor=cursor, match=match, count=count)
        keys.extend(batch)
        if len(keys) >= limit:
            return keys[:limit], True
        if cursor == 0:
            return keys, False

The only exit other than a completed cursor is len(keys) >= limit — a cap on matching keys. On the empty-outbox case (the overwhelmingly common one) keys stays empty and the loop walks the entire cursor space at scan_count=500. Both relays poll at 0.1s (bridge/email_relay.py:55, bridge/telegram_relay.py:46). The module docstring at utils/redis_client.py:55-56 claims "scan_keys bounds both the per-round-trip work and the total returned, so neither failure mode is reachable" — the per-command timeout mode is genuinely closed, but the wall-time mode is not, and the docstring reads as if it were. A traversal budget (scanned-key cap or a cursor carried across cycles) is the bound the prose describes.

2. bytes_redis() reaches the bridge event loop through an unwrapped caller. bridge/email_bridge.py:1398, inside async def _process_inbound_email (:1321):

            _arm_resolver_unavailable_alert_if_persistent(project_key, message_id)

That sync function (:1282) calls get_resolver_failure_count(project_key) at :1299, which is bridge/routing.py:1637 r = _get_redis() → bytes_redis() → popoto's BlockingConnectionPool. This contradicts the invariant the PR itself writes three times (utils/redis_client.py:37-39, bridge/routing.py:1537-1543, docs/features/redis-client-accessor.md:57-62).

3. SCAN does not deduplicate; list_dead_letters can emit duplicates. bridge/email_dead_letter.py:86-96. Redis SCAN guarantees only at-least-once delivery across a full iteration and scan_keys returns keys with no dedup. The relays tolerate it (a repeat LPOP on a drained key is a no-op); the dead-letter listing will show the same entry twice. keys(pattern), which this replaced, never duplicated.

4. TestNoProductionKeysCall flags every .keys attribute, including dict.keys(). tests/unit/test_relay_send_path_outage.py:139-140:

            if isinstance(node, ast.Attribute) and node.attr == "keys":
                offenders.append(f"{module_path}:{node.lineno} KEYS")

The bare-attribute match is correct and deliberate (the comment at :134-138 explains why a call-only matcher missed asyncio.to_thread(r.keys, PATTERN)), but it cannot tell r.keys from payload.keys(). Verified green today; the first ordinary dict.keys() added to any of the three modules turns it red with a "full-keyspace KEYS" message. Narrowing to redis-bound names, as test_redis_client_accessor.py::_redis_bindings already does, keeps it honest.

5. docs/features/redis-client-accessor.md omits half of what shipped. The file is structured around "## The three accessors" (:31) and never mentions scan_keys, bridge/relay_errors.py, OutboxUnavailableError, report_send_path_failure, the new settings.redis.* knobs, or the second recurrence guard. A grep of docs/ for OutboxUnavailableError|scan_keys|relay_errors hits only an archived plan. The send-path failure contract — the half of this PR with a 26-hour outage behind it — is undocumented outside the source.

Nits

6. False truncated=True on an exact-boundary complete sweep. utils/redis_client.py:171-175: if the final batch brings len(keys) to exactly limit and the cursor returns 0, the limit check wins and the caller logs a spurious truncation warning. Reordering the two checks fixes it.

7. The autouse cache reset leaks the client it evicts. tests/unit/test_redis_client_accessor.py:318-322 nulls _cached_text_client without .close(), leaving a live pool per test — the same hazard test_ignores_redis_url_entirely explicitly works around at :70-71.

8. The doc asserts a per-module seam several modules do not have. docs/features/redis-client-accessor.md:67-69 says "Each module keeps a one-line _get_redis() … seam". Not true of agent/lock_policy.py:67,77, bridge/dead_letters.py:86,364,396, agent/side_effects.py, agent/enqueue_idempotency.py, agent/codex_dev_lease.py, agent/session_completion.py, which call text_redis() inline with no injection point.

Verified clean

• _identity_kwargs is correct against the real popoto.redis_db._SIBLING_CONNECTION_KEYS. The _POLICY_KEYS drop is necessary and sufficient — no whitelisted key collides with the explicit kwargs text_redis() passes, so no duplicate-keyword TypeError. The path→unix_socket_path fixup and the host/port pop are right. TLS detection via connection_class is sound and forwards the spellings both SSLConnection.__init__ and Redis.__init__ accept.
• Cache key (id(pool), sorted(identity.items())) is correct; a recycled id() can only alias a pool with identical identity items.
• Relay outage semantics: except redis.RedisError sits above the blanket except Exception in both relays; run_email_relay and relay_loop are the only production callers of process_outbox and both handle OutboxUnavailableError.
• Resource semantics: no caller closes bytes_redis(); both derived_redis() callers in agent/agent_session_queue.py close theirs.
• Test updates are faithful, not tautological. test_ui_app.py patching ui.app._get_redis is strictly better than the old redis.Redis.from_url patch. test_a_genuinely_empty_outbox_still_returns_zero is the right anti-cry-wolf counterweight.
• ruff check and ruff format --check clean. No redis_url= parameter or _redis_url attribute survives anywhere.

[valorengels]

Review (Judge risk):

Head reviewed: 9237797c18cbe0895ff66318f3289793bd99a0a3.

The merge-shape sweep (the deliberate hunt)

38dafba86 merged 157 commits of origin/main with zero textual conflicts and still broke production: this branch had removed import os from bridge/liveness.py (its only use was the raw client the accessor replaced) while main independently added two new os. call sites to the same module. Disjoint lines, clean merge, NameError at call time. Fixed in 9237797c1. This lens hunted for other instances of that class.

Merge geometry established first: git merge-base origin/main HEAD = 5f4b3e8d3 = 38dafba86^2; 38dafba86^1 = 50480aade (pre-merge branch tip); the branch's old base is 8e6a0f607. So "what the merge brought in" is git diff 8e6a0f607..5f4b3e8d3. Intersected against the modules this PR converts: only five files were touched by both sides — agent/output_handler.py, bridge/liveness.py, monitoring/bridge_watchdog.py, ui/app.py, agent/agent_session_queue.py. That is the entire merge-shape risk surface.

Sweeps run, and what each found:

• Undefined-name shape (the import os class). Every converted module checked for os. / redis. / json. use against its imports. All resolve: bridge/liveness.py:79 (the 9237797c1 fix) with 2 os. uses; email_relay.py:40, telegram_relay.py:31, bridge_watchdog.py:42, output_handler.py:15, ui/app.py:13, valor_email.py:34, react_with_emoji.py:28, delivery.py:23 all still import what they use. bridge/dedup.py, bridge/email_dead_letter.py, tools/email_history/__init__.py have zero os. uses and correctly dropped the import. Nothing found.
• Signature-change shape. redis_url removed from TelegramRelayOutputHandler.__init__ and EmailOutputHandler.__init__. All 5 production construction sites enumerated (tools/send_message.py:270,366, tools/ask_poll.py:168, bridge/telegram_bridge.py:3143, worker/__main__.py:598) plus tests. No site passes redis_url. Nothing found.
• process_outbox raise-contract shape. Only two production callers exist — bridge/email_relay.py:341, bridge/telegram_relay.py:1609 — and both grew an except OutboxUnavailableError branch in this diff. Nothing found.
• Cached-vs-fresh-client / lifetime shape. Swept agent bridge tools ui monitoring reflections worker utils for .close()/.disconnect()/connection_pool on a Redis object. The only Redis closes are agent/agent_session_queue.py:1112,1317 on derived_redis() connections (caller-owned by contract) and utils/redis_client.py:227 on the superseded cached client. Nothing found.
• Main-added callers flowing through changed behavior. Main added record_scan_outcome/get_last_scan_outcome to bridge/liveness.py (both route through the converted _get_redis(), both decode_responses=True-correct since json.loads accepts str) and assess_scan_health(r, ...) to monitoring/bridge_watchdog.py (takes r as a parameter). Neither breaks.

One genuine survivor of the class, found by following the event-loop axis rather than the name axis — finding 1 below.

Blockers

1. A bytes_redis() call still runs on the bridge event loop, and the PR body asserts the opposite.

bridge/email_bridge.py:1398, inside async def _process_inbound_email (:1321):

            _arm_resolver_unavailable_alert_if_persistent(project_key, message_id)

That sync function (:1282) calls, at :1299:

        failures = get_resolver_failure_count(project_key)

which is bridge/routing.py:1637 r = _get_redis() — and routing.py:1443-1447 now returns bytes_redis(), popoto's own client on the BlockingConnectionPool.

The PR body states: "get_resolver_failure_count and invalidate_customer_cache are sync functions with sync callers and are unchanged." The first half is false. routing.py:1536-1543 states the hazard verbatim — "Called inline, that would stall the entire bridge event loop" — and the diff wraps all four sites inside resolve_customer in asyncio.to_thread, but the call one frame up, in the same coroutine, was missed.

Blast radius, and why this is PR-introduced rather than pre-existing: before this change routing._get_redis() built a fresh from_url client per call — no shared pool, so no checkout wait. Now the call waits on popoto's BlockingConnectionPool, whose checkout blocks rather than raising and is not covered by socket_timeout, with the pool's own 20s timeout as the ceiling — on the bridge's event loop. It fires on the resolver-unavailable path, i.e. exactly when the system is already degraded and Redis/ORM pressure is highest. Fix is one await asyncio.to_thread(...) at email_bridge.py:1398.

Tech Debt

2. scan_keys bounds matched keys, not traversed keys. utils/redis_client.py:167-175 — the only exits are matched keys ≥ scan_key_limit (10000) or a completed cursor. On the empty-outbox case len(keys) stays 0, truncated never fires, and the loop walks the entire cursor space before returning; both relays call it every 0.1s. Not a regression against the old KEYS (also O(keyspace), and worse for the single-threaded server), and the silent-zero mode is genuinely closed — but utils/redis_client.py:55-56 claims "neither failure mode is reachable", and the residual is a poll cycle whose wall time scales with the whole keyspace. Debt, not a blocker.

3. The out-of-scope disclosure is incomplete. PR body: "the only remaining hits are agent/session_health.py." A sweep of agent bridge tools ui monitoring reflections worker utils for .keys(, scan_iter, execute_command found those two as claimed, plus one undisclosed site — agent/session_stall_classifier.py:182:

        for raw_key in r.scan_iter(f"{prefix}*"):

No count=, on POPOTO_REDIS_DB (:175-177), under a blanket except Exception as exc: # noqa: BLE001 at :192 — the same shape this PR fixed in email_dead_letter.py. Not converted here so it does not inherit the new socket timeout; the disclosure's substance holds, its enumeration does not.

4. The four new REDIS__* knobs are absent from .env.example, against uniform repo precedent. config/settings.py:686-733 adds max_connections, health_check_interval_s, scan_count, scan_key_limit, each documenting an Env: REDIS__… key. grep REDIS .env.example returns only :198 REDIS_URL= and :570 # TIMEOUTS__REDIS_SOCKET_S=5. The convention as actually written: docs/features/env-completeness-validation.md parses only live KEY= declarations, so commented override lines break no automated gate — but every other nested tunable group carries them (TIMEOUTS__* at :552,556,560,564,570,576,581,586,592,597,604,609; CODEX__* at :516-539; MODELS__* at :214), and docs/features/config-timeout-catalog.md:199 codifies it as step 3 of adding a knob. Operators have no discoverable surface for these, which matters most for REDIS__SCAN_KEY_LIMIT/SCAN_COUNT given finding 2. Same doc, step 4: launchd-managed processes skip the .env read, so "env-overridable" holds only off-launchd unless the key is injected into the plist.

Nits

5. False truncation on an exact boundary. utils/redis_client.py:171-175 — len(keys) >= limit precedes cursor == 0, so a sweep completing with exactly scan_key_limit keys reports truncated=True. Spurious WARNING at telegram_relay.py:1348, email_relay.py:279, email_dead_letter.py:87; no data loss.

6. The send-path guard will false-positive on any dict. tests/unit/test_relay_send_path_outage.py:139 matches a bare .keys attribute by design (correctly — asyncio.to_thread(r.keys, …) is not an ast.Call), but cannot tell r.keys from payload.keys(). Green today; the first ordinary dict.keys() in any of the three modules turns it red for an unrelated reason.

Judged against the outage history

The specific 26-hour shape is closed. bridge/email_relay.py:324-329 and bridge/telegram_relay.py:1583-1588 catch redis.RedisError before the blanket except Exception, report through report_send_path_failure, and raise — a sweep or LPOP failure can no longer arrive at the loop as sent = 0. Both loops handle it and escalate with a consecutive-outage counter. redis is module-imported in both (email_relay.py:45, telegram_relay.py:33), so the handler cannot itself NameError — worth stating, since that is the same class as the liveness.py break.

Heartbeat ordering in bridge/email_relay.py is as claimed: scan at :275, heartbeat r.set at :291-296. The inner except Exception as hb_err at :297 does swallow a RedisError, but only one raised after the sweep already proved Redis reachable — it can mask a heartbeat-write failure (degrading email-status freshness), never an outage. Per-message tolerance is preserved: _process_one's except Exception (:253) and the telegram requeue handler (:1580) still absorb message-level faults, matching bridge/relay_errors.py:15-19.

[valorengels]

Review: CHANGES REQUESTED

Head reviewed: 9237797c18cbe0895ff66318f3289793bd99a0a3 (resolved through tools/pr_head_resolver.py::resolve_pr_head_sha, not a bare gh read).
Mode: independent roster (2 judges) — code-quality and risk dispatched as separate agents with no shared reasoning, both awaited in-turn. Consensus rule any-blocker-wins. No quorum shortfall.

This is a re-review at a head that moved a long way: 50480aade (response to the prior review), 38dafba86 (a 157-commit merge of origin/main), 9237797c1 (a one-line fix for a break that merge introduced). Nothing below is carried over from the prior review; every finding was re-derived against this head.

---

Both original blockers are closed

• redis_url kwarg. Zero redis_url= call sites remain under tests/; all five production construction sites of TelegramRelayOutputHandler / EmailOutputHandler enumerated and none passes it.
• Full-keyspace KEYS under a 5s socket timeout on the send path. Closed, and closed in the shape this system's history demands. except redis.RedisError now sits above the blanket except Exception in both relays (bridge/email_relay.py:324-329, bridge/telegram_relay.py:1583-1588), reports through report_send_path_failure, and raises OutboxUnavailableError — so a sweep or LPOP failure can no longer reach the loop as sent = 0. Both loops handle it and escalate a consecutive-outage counter. The email heartbeat now stamps after the sweep (scan at :275, r.set at :291-296), so a cycle that could not read the outbox cannot report itself healthy. The two sites beyond the prior review's finding — bridge/telegram_relay.py (the Telegram send path, literally the surface of the 26-hour outage) and bridge/email_dead_letter.py's unbounded scan_iter — are fixed as claimed.

The merge-shape sweep

The highest-value question here was whether 38dafba86 left other semantic conflicts of the bridge/liveness.py shape: an import or name this branch removed that main independently started using, merging clean and failing at runtime. Merge geometry was established first (merge-base = 5f4b3e8d3 = 38dafba86^2; pre-merge tip 50480aade; old base 8e6a0f607), and the both-sides-touched surface is exactly five files: agent/output_handler.py, bridge/liveness.py, monitoring/bridge_watchdog.py, ui/app.py, agent/agent_session_queue.py.

Four sweeps run over that surface and the whole tree — undefined-name, signature-change, raise-contract, and client-lifetime — all came back empty; details and method in the risk judge comment. Independently, every name this branch removes from a module namespace was extracted mechanically from the diff and checked for surviving uses: fourteen removed bindings across twelve modules, zero surviving references. ruff check . and ruff format --check . are clean.

One genuine survivor of the class was found, on the event-loop axis rather than the name axis — the blocker below. Both judges reached it independently.

---

Blockers

1. bytes_redis() reaches the bridge event loop through an unwrapped caller, and the PR body asserts it does not.

bridge/email_bridge.py:1398, inside async def _process_inbound_email (:1321):

            _arm_resolver_unavailable_alert_if_persistent(project_key, message_id)

That sync function (:1282) calls get_resolver_failure_count(project_key) at :1299 → bridge/routing.py:1637 r = _get_redis() → routing.py:1443-1447 bytes_redis() → popoto's BlockingConnectionPool.

The PR body states "get_resolver_failure_count and invalidate_customer_cache are sync functions with sync callers and are unchanged." invalidate_customer_cache checks out; get_resolver_failure_count does not. routing.py:1536-1543 names the hazard verbatim — "Called inline, that would stall the entire bridge event loop" — and the diff correctly wraps all four sites inside resolve_customer in asyncio.to_thread; the call one frame up, in the same coroutine, was missed.

This is PR-introduced, not pre-existing. The old routing._get_redis() built a fresh from_url client per call — no shared pool, so no checkout wait. The new one waits on popoto's BlockingConnectionPool, whose checkout blocks rather than raising and is not covered by socket_timeout, ceiling 20s, on the bridge's event loop. It fires on the resolver-unavailable path — precisely when the system is already degraded. Fix is one await asyncio.to_thread(...) at email_bridge.py:1398.

Tech Debt

2. scan_keys bounds the keys returned, not the keyspace traversed. utils/redis_client.py:167-175: the only exits are matched keys ≥ scan_key_limit or a completed cursor, so on the empty-outbox case (the common one) the loop walks the whole cursor space, at a 0.1s poll interval in both relays. This is not a regression — KEYS was also O(keyspace) and worse for the single-threaded server, and the silent-zero mode is genuinely closed — but the docstring at :55-56 claims "neither failure mode is reachable", which overstates what the code does. Either add a traversal budget or correct the prose.

3. SCAN does not deduplicate; list_dead_letters can emit duplicate entries. bridge/email_dead_letter.py:86-96. Redis SCAN guarantees only at-least-once delivery across an iteration and scan_keys returns its list undeduped. Harmless for the relays (a repeat LPOP on a drained key is a no-op); the dead-letter listing will show the same entry twice, where keys(pattern) never did.

4. The out-of-scope disclosure is incomplete. The PR body says the only remaining unbounded hits are in agent/session_health.py. A fresh sweep found those two plus an undisclosed third: agent/session_stall_classifier.py:182 for raw_key in r.scan_iter(f"{prefix}*"): — no count=, on POPOTO_REDIS_DB, under a blanket except Exception at :192. Same shape as the email_dead_letter.py case this PR fixed. The disclosure's substance holds (it is not converted here, so it does not inherit the new socket timeout); its enumeration does not.

5. The four new REDIS__* knobs are absent from .env.example. config/settings.py:686-733 adds max_connections, health_check_interval_s, scan_count, scan_key_limit, each documenting an Env: REDIS__… key; .env.example carries none. To be precise about the convention rather than assume it: docs/features/env-completeness-validation.md parses only live KEY= declarations, so commented override lines break no automated gate. But the precedent is uniform — TIMEOUTS__*, CODEX__*, MODELS__* all carry commented override lines — and docs/features/config-timeout-catalog.md:199 codifies it as step 3 of adding a knob. Worth carrying from that same doc's step 4: launchd-managed processes skip the .env read, so "env-overridable" holds only off-launchd unless the key reaches the plist.

6. docs/features/redis-client-accessor.md documents only half the PR. The file is built around "## The three accessors" (:31) and never mentions scan_keys, bridge/relay_errors.py, OutboxUnavailableError, report_send_path_failure, the new settings.redis.* knobs, or the TestNoProductionKeysCall guard. A grep of docs/ for OutboxUnavailableError|scan_keys|relay_errors hits only an archived plan. The send-path failure contract is the half of this change with a 26-hour outage behind it and it lives only in source comments.

7. TestNoProductionKeysCall matches every .keys attribute, including dict.keys(). tests/unit/test_relay_send_path_outage.py:139-140. The bare-attribute match is right and the inline comment at :134-138 correctly records why a call-only matcher missed asyncio.to_thread(r.keys, PATTERN) — the guard is just over-broad. Green today; the first ordinary dict.keys() added to any of the three modules turns it red with a misleading "full-keyspace KEYS" message. Narrowing to redis-bound names, as test_redis_client_accessor.py::_redis_bindings already does, keeps the signal honest.

Nits

8. False truncated=True on an exact-boundary complete sweep. utils/redis_client.py:171-175: len(keys) >= limit is checked before cursor == 0, so a sweep that completes with exactly scan_key_limit keys reports truncation. Spurious warnings at telegram_relay.py:1348, email_relay.py:279, email_dead_letter.py:87. Swapping the two checks fixes it.

9. The autouse cache-reset fixture leaks the client it evicts. tests/unit/test_redis_client_accessor.py:318-322 nulls _cached_text_client without .close(), leaving a live pool per test — the hazard test_ignores_redis_url_entirely explicitly handles at :70-71.

10. The doc asserts a per-module seam several modules do not have. docs/features/redis-client-accessor.md:67-69 claims "Each module keeps a one-line _get_redis() … seam". agent/lock_policy.py:67,77, bridge/dead_letters.py:86,364,396, agent/side_effects.py, agent/enqueue_idempotency.py, agent/codex_dev_lease.py and agent/session_completion.py call text_redis() inline with no injection point.

---

Guards proven RED against known-bad, then restored

A guard certifying absence is worthless until it has failed on the bad input. All three were planted against and failed as designed, then the tree was restored to a clean git status and re-run green:

Guard	Known-bad planted	Result
TestNoRawClientsInProduction::test_only_the_accessor_constructs_redis_clients	monitoring/_tmp_guard_probe.py with redis.Redis.from_url(...)	RED — monitoring/_tmp_guard_probe.py:5
TestNoRawClientsInProduction::test_the_scan_covers_every_production_package	new top-level probe_pkg/probe_mod.py	RED — ['probe_pkg']
TestNoProductionKeysCall[bridge/email_relay.py]	scan_keys call reverted to asyncio.to_thread(r.keys, PATTERN)	RED — bridge/email_relay.py:275 KEYS

3 failed, 5 passed planted; 8 passed after restore. The bare-attribute match is what makes the third one work — both relays spelled the defect as a reference handed to a threadpool, and a call-only matcher would have reported both send paths clean.

Measured test results

Test set built from tests that actually import the production modules this branch changes (agent.output_handler, bridge.{dedup,email_bridge,email_dead_letter,email_relay,liveness,relay_errors,routing,telegram_relay}, monitoring.bridge_watchdog, reflections.pm_briefings, tools.{email_history,react_with_emoji,valor_email}, ui.app, utils.redis_client), not from a reused list — 97 unit files plus tests/integration/test_message_drafter_integration.py. This set includes tests/unit/test_reconciler_scan_health.py, which arrived with the merge and which a stale pre-merge list omitted.

2091 passed, 2 skipped, 250 warnings in 473.60s (0:07:53)   exit 0

Serial (-n 0) via scripts/pytest-clean.sh. Zero failures, so no failure needed adjudication against main. The 7 known-red tests in tests/unit/test_sdlc_router_oscillation.py are not in this set and did not contaminate the baseline.

Gates: ruff check . — all checks passed. ruff format --check . — 1545 files already formatted. docs/features/redis-client-accessor.md exists and is indexed in docs/features/README.md:174 (content gap noted as tech debt 6). New settings land in config/settings.py:686-733.

Visual proof gate: no-op. The diff contains no HTML, CSS, JS/TS, JSX/TSX, Vue, or template files. ui/app.py is touched, but all three changed sites are data-source seam swaps inside sync JSON helpers (_get_slot_reclaims_total, _get_worker_slot_health, _get_email_health) with no rendering change, so there is nothing a screenshot could prove.

Deploy note (repo addendum): this touches bridge/, agent/, worker/ and both relays. After merge: ./scripts/valor-service.sh restart, plus worker-restart and email-restart.

---

Verdict: CHANGES REQUESTED — 1 blocker, 6 tech debt, 3 nits. The blocker is one line. The rest of the change is sound, and the send-path hardening is the right fix for the failure this repo has actually paid for.

[valorengels]

Review: PR #3171 — re-review at 4b57cbd16

Second pass, scoped to the delta since 9237797c1 (four commits: the merge 1937170e8, then cb0252206, ac50a5001, 4b57cbd16). Head resolved through tools/pr_head_resolver.py::resolve_pr_head_sha → 4b57cbd16a996f40d291ae0e31da14e6c7d86d47.

Every pass-1 finding is addressed: the blocker, all six tech-debt items, all three nits. Verified individually, not accepted from the commit messages. Five new tech-debt items and three nits below, all in the new guard and the new scan_keys contract; none blocks.

---

Blocker 1 — fixed, and the fix is correct

bridge/email_bridge.py:1398-1404 now reads:

            await asyncio.to_thread(
                _arm_resolver_unavailable_alert_if_persistent, project_key, message_id
            )
            return

Three things had to hold, and all three do:

• Loop affinity. The helper body (:1282-1318) is time.time(), a lazy import, logger calls and sync Redis round trips. No asyncio, no get_event_loop, no contextvar reads — nothing in it assumed the loop thread.
• The in-thread import is safe. :1296 does from bridge.routing import get_resolver_failure_count from the worker thread. bridge.routing is already in sys.modules by then: _process_inbound_email imports it itself at :1352, ahead of the except ResolverUnavailableError branch at :1386. So this is a dict hit, not a first import on a non-main thread.
• Sequencing. The return still executes after the helper completes, and exception behaviour is unchanged (the helper swallows everything into logger.warning at :1317-1318). The one behavioural delta is that the coroutine now yields at the await, so a shutdown cancellation can surface here as CancelledError where the sync call was uncancellable. That is strictly better than a 20s loop stall.

The correction to the hazard's scope checks out, and the fix is still right. bridge/email_bridge.py:143's _get_redis() returns text_redis(); bridge/routing.py:1443's returns bytes_redis(). Of the helper's four Redis touches, exactly one — get_resolver_failure_count at :1299 — reaches popoto's BlockingConnectionPool. The other three run on text_redis()'s bounded pool, which raises on exhaustion under a socket timeout. Wrapping all four is the right call anyway (they are still blocking socket I/O on the loop), but see nit 6: the commit message and the inline comment do not make that distinction.

---

The guard — RED-proved here, not taken on report

Planted the known-bad by reverting the wrap to a bare synchronous call, ran TestBlockingPoolNeverReachesAnEventLoop:

1 failed, 3 passed in 13.18s
AssertionError: ... Found: ['bridge/email_bridge.py:1403 _arm_resolver_unavailable_alert_if_persistent()']

Exactly one offender, correct module, correct line, correct name — no false positives across all 13 production packages. Restored with git checkout --, git status clean, 18 passed.

It does catch the defect it was written for, and its false-positive set against the real tree is empty. But it has three breadth defects, found by exercising its own helpers against synthetic trees. All are latent today; all fail silently green, which is the expensive direction for a guard whose whole job is certifying absence. (A fourth, harmless today: attribute calls match on the trailing attribute alone, so obj.helper() on any unrelated object is flagged whenever a module-local function happens to be named helper.)

---

Tech Debt

1. A name collision inside one module silently drops taint, order-dependently. tests/unit/test_redis_client_accessor.py:343-347 builds the call graph as funcs = {node.name: _called_names(node) for node in ast.walk(tree) if isinstance(node, ast.FunctionDef)} — a dict keyed by the bare name, over a walk that descends into classes and nested defs. Two definitions sharing a name in one module: the last one wins and the first disappears. Measured on the guard's own helper:

tainted-first-then-clean -> set()
clean-first-then-tainted -> {'helper'}

Same two functions, only the source order differs. Sweeping the 13 scanned packages: 21 modules already contain duplicate sync def names; none currently shadows a bytes_redis caller, so this is latent, not live. The module-qualification work was done at the import boundary and then undone inside the module.

2. Expressions in a to_thread argument list are treated as offloaded, and they are not. _unwrapped_async_calls (:380-389) marks every descendant of the offloader Call as offloaded. Proved:

await asyncio.to_thread(sink, get_count(k))   # _unwrapped_async_calls(...) -> []

Argument expressions are evaluated eagerly, on the event loop, before to_thread is ever entered — so that line blocks the loop and the guard reports clean. The docstring at :386-387 states the opposite: "Everything in the argument list runs off the loop, whether it is called there or merely referenced for the threadpool." Only the callable reference does. This matters more than the other three because it is the shape a future offender gets "fixed" into: hand the blocking call's result to the threadpool instead of the call itself.

3. Two import shapes carry no taint edge. _imported_names (:364-375) handles ast.ImportFrom with node.level == 0 only.

• import utils.redis_client as rc … rc.bytes_redis() → no names extracted, no edge. No production site spells it that way today.
• from .routing import get_resolver_failure_count → skipped by the not node.level filter. Relative imports are in production use (agent/__init__.py:3-38, tools/email_cs/gate.py:25-26), so this one is a refactor away from live.

4. scan_keys can now return more than scan_key_limit keys. utils/redis_client.py:184-195. Moving the cursor check ahead of the limit check fixed the boundary (see below) but also removed the [:limit] cap from the completed-sweep path — return list(seen), False is uncapped. When the round that crosses the limit is also the round that closes the cursor, the overshoot ships. Measured with 600-key pages against the default scan_key_limit=10000:

page=600 total=10200 -> returned 10200, truncated False   # 200 over the limit

Reachable in practice because COUNT is a hint and real SCAN page sizes vary. The overshoot is bounded by one page, so this does not reopen the timeout-as-empty-queue mode (that is bounded per round trip by scan_count, untouched). It is a contract mismatch: config/settings.py:730 and the new .env.example line both still say "Hard ceiling on keys returned by one utils.redis_client.scan_keys call."

Recommend correcting the prose, not re-adding the cap: return list(seen)[:limit], False would silently drop keys from a completed sweep while reporting it complete, which is strictly worse than the overshoot.

5. TestNoProductionKeysCall is a hand-picked three-module list, and misses self._redis. tests/unit/test_relay_send_path_outage.py:218-221 parametrizes over bridge/email_relay.py, bridge/telegram_relay.py, bridge/email_dead_letter.py with no companion coverage test — the same shape this PR's own description identifies as having hidden monitoring/ from the other guard ("the guard was exactly as wide as the list that produced it, so it certified its own blind spot as clean"). TestNoRawClientsInProduction got test_the_scan_covers_every_production_package for exactly this; this guard did not.

Compounding it, _client_bound_names (:152-181) tracks only Assign/AnnAssign targets, so an instance-held client is invisible:

await asyncio.to_thread(self._redis.keys, P)   # _keys_offenders(...) -> []

agent/output_handler.py:512-517 holds a client as self._redis, is a send path, and is not in the scanned list.

---

Nits

6. The blocker's stated reason is imprecise, and a fix maintained for the wrong reason gets maintained wrongly. cb0252206's message and the inline comment at email_bridge.py:1398-1402 both describe "four Redis round trips, and the first reaches popoto's BlockingConnectionPool". Only one of the four is the blocking-pool hazard; the other three are text_redis(), a bounded pool that raises rather than blocks and is covered by socket_timeout. Also, _get_redis() is a cached-client lookup, not a round trip. The wrap-all decision is correct; the comment should say why — the three text calls are still blocking socket I/O on the loop, just with a far smaller ceiling — rather than implying a uniform hazard.

7. PR body test counts are stale. It states test_redis_client_accessor.py (13 tests) and test_relay_send_path_outage.py (10 tests). Measured at this head: 18 and 14.

8. The PR body still asserts the universal seam that 4b57cbd16 corrected. The "What" section reads "Each module keeps its one-line _get_redis() / _get_redis_connection() seam delegating to the accessor". docs/features/redis-client-accessor.md now correctly says the opposite — six modules call text_redis() inline with no injection point, and _get_redis is a name rather than a contract. The body and the doc it points at disagree.

---

Pass-1 findings, re-verified at this head

#	Finding	Status
Blocker 1	Blocking-pool checkout on the bridge event loop	Fixed, verified above; guard RED-proved here
TD 2	Docstring overclaimed "neither failure mode is reachable"	Fixed — utils/redis_client.py:52-58 now states the traversal limit plainly and records why a traversal budget was rejected
TD 3	SCAN duplicates reaching list_dead_letters	Fixed — first-seen-order dedupe; measured ['a','b','a','c'] -> ['a','b','c']
TD 4	Incomplete out-of-scope disclosure	Fixed — agent/session_stall_classifier.py:182 now named in the body
TD 5	Four REDIS__* knobs absent from .env.example	Fixed — all four carry commented override lines, with the launchd caveat
TD 6	Doc covered only half the PR	Fixed — three new sections cover scan_keys, the send-path failure contract, and the tunables table
TD 7	TestNoProductionKeysCall matched every .keys	Fixed — see RED proof below
Nit 8	False truncated=True on an exact-boundary sweep	Fixed — measured: exactly limit keys → truncated False; limit+1 → truncated True
Nit 9	Autouse fixture leaked the evicted client	Fixed — :505-515 closes before dropping
Nit 10	Doc claimed a seam several modules lack	Fixed — both groups named, plus the _get_redis-is-not-a-contract warning

TD 7 RED-proved on a real module, not a synthetic tree. Planted _bad = await asyncio.to_thread(r.keys, 'email:outbox:*') and _ok = payload.keys() on adjacent lines immediately after the real sweep in bridge/email_relay.py:

clean module -> []
planted     -> ['bridge/email_relay.py:276 KEYS']

The client-bound reference is reported; the dict call on the next line is ignored. Narrowing works as claimed.

---

The merge — tooling and plan docs only, confirmed

1937170e8 merged 24 commits of main. Checked on the axis that caught the last one (branch removes a name, main independently adds a use of it in the same module — clean textual merge, NameError at call time).

That failure requires a module touched by both sides. There is none:

• main side (1937170e8^1...1937170e8^2) — 6 files: docs/plans/improvement-controller-lane-{3,5,6}-*.md, tools/sdlc_next_skill.py, tests/unit/test_sdlc_next_skill.py, tests/unit/test_sdlc_router_oscillation.py.
• branch side — 31 files under agent/ bridge/ config/ monitoring/ reflections/ tools/email_history/ tools/react_with_emoji.py tools/valor_email.py ui/ utils/ plus tests and docs/features/.
• Intersection: empty. tools/sdlc_next_skill.py is the only production file main brought in, and it is not on the branch's list.

git show --stat on the merge commit equals the main-side diff exactly — the merge carries no resolution content of its own. Both merge-arrived test files are in the test set below and passed.

---

Measured results at 4b57cbd16

Test set rebuilt from tests that actually import the production modules changed in this delta (bridge/email_bridge.py, utils/redis_client.py) plus the merge-arrived tools/sdlc_next_skill.py, with the three .env declaration suites added for the .env.example change — 42 files, scripts/pytest-clean.sh:

1121 passed, 101 warnings in 82.77s (0:01:22)   exit 0

Zero failures, so nothing needed adjudication against main. test_validate_no_module_scope_env.py::test_repo_census_is_a_monotonic_ratchet (#3313) is not in this set. The 7 test_sdlc_router_oscillation.py fixtures arrived green with the merge and are in the set.

Guard runs, in order: planted known-bad → 1 failed, 3 passed; restored, clean git status → 18 passed.

Gates: ruff check . — all checks passed. ruff format --check . — 1545 files already formatted.

Visual proof gate: no-op. The delta touches one Python call site, one Python helper, .env.example and two markdown files. No HTML, CSS, JS/TS, JSX/TSX, Vue or template files.

Deploy note (repo addendum): this touches bridge/, agent/, worker/ and both relays. After merge: ./scripts/valor-service.sh restart, plus worker-restart and email-restart.

---

Verdict: APPROVED — 0 blockers, 5 tech debt, 3 nits.

The blocker fix is correct on all three axes it had to be, and the guard behind it earns its place: it caught the live offender with no false positives across 13 packages, and it closes the class rather than pinning the line. Its breadth defects are all latent and all worth a follow-up — tech debt 2 most of all, because a guard that goes green on to_thread(sink, blocking_call()) will eventually certify the next instance of exactly this outage as clean.