Deploy Redis in k3s and pin the orchestrator to the Redis message store (#2662)

[jwbron]

Fixes #2662.

Problem

The k3s manifests deployed no Redis and passed no REDIS_HOST / EGG_MESSAGE_STORE_BACKEND to the orchestrator, so auto backend selection always fell back to the in-memory MessageStore:

• RedisMessageStore had zero live end-to-end coverage (unit tier only, via fakeredis).
• Messages did not survive orchestrator restarts — the #3076 mid-phase-restart loss risk, and the replay source for consensus-tracker reconstruction.
• Since Design: coordination state is served, not replicated — retire git/prompt choreography for agent state exchange #3077 slice-6 ([issue-3077][slice-6/6] Bounded durability: fail-loud... #3144), every deployed boot logged the MESSAGE_STORE_AUTO_FALLBACK_TO_MEMORY marker and /api/v1/health reported a permanently degraded message_store component.

The gap had a third layer the issue didn't name: the redis client package was not installed in the orchestrator image at all (absent from orchestrator/requirements.txt and pyproject runtime deps — only fakeredis existed, in dev deps). Deploying Redis + env alone would have changed nothing: the import redis inside _create_message_store() fails before the backend is ever probed.

Changes

egg-redis image (config/redis/Dockerfile): stock Redis pinned at 8.6.4-alpine, built and published through the local loopback-registry supply chain like every other egg image — no public image references in the manifests. Wired into EGG_ALL_IMAGES, make build, the deploy tag rewrite, the local overlay images list, and await-egg-deploy.sh.

Manifests (k8s/base/redis-deployment.yaml, redis-service.yaml): single replica, Recreate (never deadlocks on the RWO volume), 1Gi PVC with appendonly yes so messages also survive Redis pod restarts, maxmemory 256mb + noeviction so memory pressure fails writes loudly instead of silently dropping stream entries, hardened securityContext matching the existing deployments, redis-cli ping probes. Agent pods cannot reach it — egg-agents is default-deny egress with no Redis allow rule.

Orchestrator wiring: EGG_MESSAGE_STORE_BACKEND=redis — explicit, not auto. The silent fallback is exactly what the slice-6 degraded flag exists to catch; in explicit mode a Redis outage raises, and since the store singleton is created lazily and a failed creation leaves it unset, the orchestrator naturally retries until Redis is reachable (no startup-ordering dependency). Plus REDIS_HOST/REDIS_PORT, and redis>=5.0,<7.0 in orchestrator/requirements.txt + pyproject runtime deps.

End-to-end coverage (integration_tests/test_message_store_backend.py), running against the cluster make test-integration deploys:

1. the deployed Deployment pins EGG_MESSAGE_STORE_BACKEND=redis (makes the health check non-vacuous — explicit mode cannot silently fall back);
2. /api/v1/health carries no fallback marker;
3. a live XADD/XRANGE/DEL round-trip from inside the orchestrator pod through the production selection path in _create_message_store() — exercising the env wiring, the in-image client, Service DNS, and the live Redis, and asserting the selected backend class is RedisMessageStore.

Docs: concurrent-execution.md's claim that production stores messages in Redis Streams is now true; coordination-state.md's wipe-semantics section notes the accidental-loss row can no longer occur in-cluster.

Out of scope

Message-routing test coverage on top of the store (#2640 / #2661). Selection-precedence semantics are unchanged (frozen by #3077 HITL Q3) — this PR only changes deployment config, deps, tests, and docs.

Validation

• make lint clean; kubectl kustomize k8s/overlays/local/ renders with the image substitution and env in place.
• orchestrator/tests/test_redis_message_store.py + test_message_store.py: 123 passed.
• New integration tests collect cleanly; they exercise the live cluster on the next make redeploy / make test-integration.

[james-in-a-box]

Autofix tracking

{"Test/Integration Tests / Integration Tests": 2}

[jwbron]

Live-canary finding, fixed in 2b2dc9f. Deployed this branch to the local cluster and submitted a throwaway pipeline as the first real traffic through the Redis backend. Writes and non-blocking reads worked immediately (stream keys created, heartbeats flowing), but every agent long-poll errored at exactly 5s:

ERROR orchestrator.redis_message_store: Failed to read from Redis Stream pipeline_id=pipeline-e73d8284 error="Timeout reading from socket"

Root cause: the connection pool sets socket_timeout=5, and redis-py enforces that on the blocked read itself — so a single XREAD BLOCK 25000 (the standard agent wait) dies at the 5s mark and the route 500s. Agents still made progress because the wait CLI retries, but event-driven waiting was degraded to error-spam polling. This is precisely the class of bug the issue predicted (fakeredis has no sockets, so unit tier structurally cannot catch it) and the reason for deploying real Redis in CI.

Fix: blocking reads are sliced into XREAD BLOCK chunks of at most _MAX_BLOCK_MS (4s, under the socket timeout) looped against the caller's wait deadline — data still returns immediately, idle slices cost one extra round-trip each. A TimeoutError that still fires on a blocking slice degrades to an idle slice; non-blocking reads keep raising. Five new unit tests pin the cap (including its < socket_timeout relationship), both blocking branches, and the degradation scoping.

Re-running the canary (including a mid-phase orchestrator restart to exercise the #3076 durability story) after redeploying with this fix.

[jwbron]

Round-2 canary + restart drill: PASS (cluster redeployed with 2b2dc9f + 887b3ef).

• Long-poll fix verified: 12+ minutes of wrapper wait-loop long-polls (WAITING_FOR_EVENT heartbeat cycles), 0 Failed to read from Redis Stream errors. Round 1 produced 8 errors in the first 2 minutes at the same stage.
• #3076 durability drill: restarted the orchestrator mid-refine with 20 messages of live transcript. The stream not only survived but grew to 30 during the orchestrator's downtime — agents kept writing to Redis while the coordinator was gone. Startup reconciliation recognized the pipeline (pipeline has live pods, leaving RUNNING), and agent wrappers resumed against the new pod within ~9s of boot (consensus/next-action and heartbeats all 200). No fallback marker, no read errors on the new pod.
• Canary cancelled with cleanup after the drill; no artifacts left behind.

On the old in-memory backend this restart would have dropped the entire transcript and de-synced the consensus tracker. End-to-end validation of both halves of the issue (live Redis coverage + restart durability) is done.

[egg-reviewer]

No agent-mode design concerns.

This is infrastructure-only — Redis k3s manifests, the egg-redis Dockerfile, dependency pins, integration tests, and doc updates. It has no agent-prompt surface: no prompts, no model identifiers, no structured-output-for-humans, no pre-fetching, and no post-processing pipelines. The lone requests.get in the new integration test targets the orchestrator's own /api/v1/health endpoint, not the Anthropic API, and httpx is only a dependency pin — neither implicates the EGG200 direct-API-call boundary.

— Authored by egg

[jwbron]

Full live-validation record for this branch (consolidating the thread):

1. Deploy verification — make redeploy builds/publishes egg-redis through the loopback registry alongside the other images; redis pod Running with redis-data PVC Bound (1Gi local-path); await-egg-deploy.sh gates on the redis Deployment; /api/v1/health reports message_store: {status: ok}. All 3 new integration tests pass against the live cluster, including the in-pod XADD/XRANGE/DEL round-trip through the production selection path (BACKEND RedisMessageStore).
2. Round-1 canary — caught the XREAD BLOCK vs socket_timeout=5 bug within 2 minutes of real traffic (see above); fixed in 2b2dc9f40 with 5 unit tests pinning the slice-cap contract.
3. Round-2 canary + restart drill — 0 read errors over 12+ min of wrapper long-polls; mid-phase orchestrator restart survived with the transcript growing 20→30 in Redis during the downtime; wrappers resumed in ~9s (see above).

Follow-ups spawned by this validation, both out of scope here:

• Decide: keep third-party images on the local supply chain vs digest-pinned public references #3154 — image-sourcing policy (local wrapper vs digest-pinned public references), revisit on the second third-party image or GKE (Deploy egg on GKE (minimal first deploy): Terraform-provisioned cluster + GKE overlay + registry pull path #3002).
• Codify the event-pump contract: agents never wait on the bus — retire the agent-tier STAY ALIVE wait-loop idiom #3157 — codify the event-pump waiting contract (agents never block on the bus; the wrapper owns the wait) and retire the stale STAY ALIVE canon — the docs confusion surfaced while attributing the round-1 long-poll errors.

[egg-reviewer]

Review: Deploy Redis in k3s + pin orchestrator to Redis message store (#2662)

I traced the full path from the deployed redis-server args through the orchestrator's connection logic and the blocking-read code. The deployment scaffolding (Makefile/CI/overlay/await-egg-deploy.sh wiring, NetworkPolicy isolation, securityContext, dep declaration + lockfile) is correct and the lazy-retry / no-startup-ordering claim checks out. But there is one blocking issue that makes the feature non-functional end-to-end, and a second real correctness bug in the blocking-read change that ships alongside it.

---

🔴 BLOCKING 1 — Redis rejects the orchestrator's connection: protected mode + no password

k8s/base/redis-deployment.yaml runs:

redis-server --appendonly yes --dir /data --maxmemory 256mb --maxmemory-policy noeviction

No --requirepass, no --protected-mode no. Stock Redis defaults to protected-mode yes with a no-password default user. In Redis 8 the protected-mode gate is (verified in the 8.0 source):

• src/networking.c (~L1366): rejects the connection when server.protected_mode && (DefaultUser->flags & USER_FLAG_NOPASS) and the peer is not loopback. There is no bind-address exemption in this check anymore — binding to all interfaces (the compiled-in default {"*","-::*"}, src/server.h:119) does not relax it.
• src/socket.c:331 connSocketIsLocal() treats only 127.* / ::1 as local.

The orchestrator connects from a different pod via redis.egg-system.svc.cluster.local (orchestrator-deployment.yaml), i.e. a non-loopback ClusterIP source. So get_redis_message_store()'s client.ping() (orchestrator/redis_message_store.py:649) receives DENIED Redis is running in protected mode…, raises ConnectionError, and because the backend is pinned to explicit redis, _create_message_store() re-raises (message_store.py:722-723). Every message operation then 500s — inter-agent messaging, the exact thing this PR is enabling, does not work.

This is the canonical Docker/Kubernetes protected-mode failure mode (it's why Bitnami's chart forces either a password or ALLOW_EMPTY_PASSWORD=yes). The orchestrator client passes no password (get_redis_message_store has no password param, redis_message_store.py:627-646), so authentication cannot satisfy the gate either.

Why CI didn't catch it: your own integration test #3 (test_redis_streams_roundtrip_from_orchestrator_pod) is precisely the probe that would fail here — but the PR description states the integration tests have only been collected, not run live ("they exercise the live cluster on the next make redeploy / make test-integration"). The single most important validation for this PR hasn't run yet.

Fix (minimal, consistent with the current no-auth client): add --protected-mode no to the container args. The isolation protected mode would provide is already enforced by network-policies.yaml (egg-agents is default-deny egress with no Redis allow rule; only orchestrator in egg-system, which has no NetworkPolicy, can reach it). Alternatively set --requirepass <secret> — but that additionally requires threading a password=/REDIS_PASSWORD through get_redis_message_store() and the pool, plus a Secret, since the client sends no password today. Please redeploy and let test #3 actually run green before merge.

---

🔴 BLOCKING 2 — Chunked blocking read re-resolves $ per slice → message-drop window (diverges from the in-memory backend)

The new chunking in redis_message_store.py re-issues XREAD BLOCK in ≤_MAX_BLOCK_MS slices. On the from_tip path start_id is "$", and it is never advanced across idle slices:

• Fast path (L420-434): start_id stays "$"; the loop discards last_sid (messages, _ = _read_once(start_id, …)).
• wait_for_types path (L484-487): current_start = last_sid only fires when a row was read; on a pure-idle slice last_sid is None, so current_start stays "$".

$ is resolved server-side to the current tip on every re-issue. So in the gap between one XREAD BLOCK returning empty and the next being issued (~one RTT), any XADD advances the tip, and the next $ block starts after it — that message is never delivered by this call. The route then makes the loss permanent: on timeout it sets cursor = get_latest_id() (routes/messages.py:782), which now points at the missed message, and the next wait-loop call passes it as since_id and reads strictly after it (messages.py:767, from_tip disabled once since_id is set).

Contrast the in-memory backend, which snapshots the tip once at call entry under the lock (message_store.py:437-438, start_idx = len(initial_msgs)) and re-filters from that fixed index — it cannot drop a message that arrives mid-wait. The comment at message_store.py:375-377 calls this out explicitly ("so from_tip semantics are race-free against concurrent add_message calls"). The Redis path now violates that invariant, so the two backends give different results for the same sequence of events — exactly the cross-path inconsistency this kind of consensus infra can't afford (a dropped CONSENSUS_CONFIRMED/wake-up can stall a pipeline).

It is low-probability (the blind window is ~RTT per slice, and it closes as soon as any traffic flows, since a non-matching row replaces $ with a concrete id), and it's strictly better than the pre-PR state where every long-poll 500'd at the 5 s socket timeout. But it's a real, hard-to-debug loss in the critical path and the fix is cheap: resolve the tip to a concrete stream id once before the chunked loop (e.g. XREVRANGE key + 1, or "0-0" fallback when empty) and advance from that concrete id across slices, mirroring the in-memory once-at-entry snapshot. Then $ is never re-resolved mid-wait.

---

🟡 Non-blocking

1. test_health_reports_message_store_ok is vacuous in explicit-redis mode (integration_tests/test_message_store_backend.py:214-223). The health route derives message_store purely from is_memory_fallback_degraded() (routes/health.py:137-144), and that flag is only ever set on the auto→memory fallback path (message_store.py:730). In explicit redis mode the flag is structurally always False, so the component is always {"status":"ok"} — even if Redis is completely unreachable (in which case ops just 500, the health flag never flips). The docstring's claim that asserting EGG_MESSAGE_STORE_BACKEND=redis "makes the health check below non-vacuous" is backwards: pinning redis is what makes this assertion a tautology. No harm (the round-trip test #3 carries the real coverage), but the test and its docstring overstate what they verify — consider trimming it or pointing it at the round-trip's backend assertion instead.

2. _MAX_BLOCK_MS vs socket_timeout are two independent hardcoded constants (redis_message_store.py:50 = 4000; :643 socket_timeout=5). The guard test test_cap_stays_below_pool_socket_timeout asserts _MAX_BLOCK_MS < 5000 against a literal 5000, not the actual pool socket_timeout. Lower socket_timeout to 3 and the test still passes while production regresses. Derive the bound from the real value (or make socket_timeout a named constant the test references). Mitigated by the new TimeoutError→idle-slice degradation, which keeps a cap/timeout mismatch from 500ing, but the test gives false confidence as written.

3. PR-description scope mismatch. The body says this PR "only changes deployment config, deps, tests, and docs" and "selection-precedence semantics are unchanged," but it also makes a substantive change to production blocking-read logic in redis_message_store.py (the chunking + TimeoutError handling). That's the change carrying BLOCKING 2 above; it deserves a "Changes" bullet so reviewers focus on it rather than treating the PR as config-only.

4. uv.lock downgrades redis 7.3.0 → 6.4.0 — expected and an improvement (redis was previously an unconstrained transitive of the fakeredis dev-dep; it's now a pinned >=5.0,<7.0 runtime dep, so dev and prod converge). Just calling it out since it's a behavior-affecting lockfile change, not noise.

---

Verdict: request changes. BLOCKING 1 means the feature does not function in the cluster as written — the orchestrator cannot connect to this Redis. Add --protected-mode no (or a password + client wiring), then run make test-integration so test #3 proves the path live. BLOCKING 2 should be fixed in the same pass since the PR is already in this code and the divergence from the in-memory backend is a genuine message-loss path.

— Authored by egg

[james-in-a-box]

Thanks for the thorough trace — both blocking issues were real. All items addressed in commit d23d242.

🔴 BLOCKING 1 — protected mode rejects the orchestrator connection

Agreed. fixed-in-PR (commit d23d242). Added --protected-mode no to the redis-server args in k8s/base/redis-deployment.yaml, with a comment explaining that the orchestrator connects password-less from a different pod over the ClusterIP service (a non-loopback peer the Redis 8 protected-mode gate rejects), and that isolation is already provided by network-policies.yaml (egg-agents default-deny egress, no Redis allow rule). Kept the no-auth client rather than threading a password/Secret through get_redis_message_store().

🔴 BLOCKING 2 — chunked blocking read re-resolves $ per slice

Agreed. fixed-in-PR (commit d23d242). Added RedisMessageStore._resolve_tip_stream_id(), which snapshots the current tip to a concrete stream id once (XREVRANGE … COUNT 1, "0-0" for an empty/missing stream) before the chunked loop. The from_tip path now starts from that fixed id instead of $, so re-blocking across idle slices re-scans the gap rather than advancing past a mid-wait XADD — mirroring the in-memory backend's once-at-entry tip snapshot. New unit tests pin it: tip resolves to a concrete id, empty stream → "0-0", and the from_tip blocking read never issues $ to XREAD.

🟡 Non-blocking 1 — vacuous health test + backwards docstring

Agreed. fixed-in-PR (commit d23d242). Corrected both docstrings in test_message_store_backend.py: dropped the backwards "explicit redis makes the health check non-vacuous" claim from test_deployed_backend_is_explicit_redis, and added a scope caveat to test_health_reports_message_store_ok stating the marker only flips on the auto→memory path (so {"status":"ok"} is not proof Redis is reachable) and pointing at the round-trip test #3 as the real coverage. Kept the assertion as a cheap up-without-degrading regression guard rather than deleting it.

🟡 Non-blocking 2 — cap vs socket_timeout are independent literals

Agreed. fixed-in-PR (commit d23d242). Introduced _SOCKET_TIMEOUT_SEC = 5; the pool's socket_timeout/socket_connect_timeout and _MAX_BLOCK_MS = (_SOCKET_TIMEOUT_SEC - 1) * 1000 both derive from it, and test_cap_stays_below_pool_socket_timeout now asserts against _SOCKET_TIMEOUT_SEC * 1000 instead of the literal 5000. Lowering the socket timeout now regresses the test instead of silently in prod.

🟡 Non-blocking 3 — PR-description scope mismatch

Agreed. fixed-in-PR (commit d23d242) (captured here — the PR body is owned by @jwbron, so I can't edit it directly). Corrected scope: this PR is not config/deps/tests/docs-only — it makes a substantive production change to blocking-read logic in redis_message_store.py (the XREAD BLOCK chunking, the from_tip concrete-tip snapshot from BLOCKING 2, and the TimeoutError→idle-slice degradation). Backend selection-precedence semantics remain unchanged (frozen by #3077 HITL Q3); the blocking-read change is the only production-logic delta. @jwbron — please fold a "Blocking-read logic" bullet into the Changes section when convenient.

🟡 Non-blocking 4 — uv.lock redis 7.3.0 → 6.4.0

disagree (no change required) — agreed it's an improvement (redis converges from an unconstrained transitive of the fakeredis dev-dep to a pinned >=5.0,<7.0 runtime dep), and you flagged it as a callout rather than a change request, so nothing to do here.

make lint clean; the from_tip and slice-cap unit tests pass locally. Trusting CI for the full suite per the workflow's no-full-suite guidance.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns.

The delta since my last review (commit d23d242) is confined to the Redis message-store transport and its k8s deployment: the from_tip concrete-tip snapshot fix, the _SOCKET_TIMEOUT_SEC-derived block cap, --protected-mode no on the Redis pod, and test/docstring corrections. None of these touch agent-mode design surface — no prompt pre-fetching, structured-output-for-humans, post-processing pipelines, or out-of-sandbox LLM calls. If anything, the from_tip fix strengthens the BRC consensus messaging path by closing a silent message-drop window, which is aligned with reliable multi-agent coordination.

— Authored by egg

[egg-reviewer]

Re-review: Deploy Redis in k3s + pin orchestrator to Redis message store (#2662)

Re-reviewed the delta 887b3ef..d23d242 (4 files). Both blocking issues from my prior review are properly fixed — not superficially patched. I traced the data flow end-to-end again and verified the security claim against the actual manifests, and ran the targeted unit tests.

🔴 BLOCKING 1 — protected mode → RESOLVED

--protected-mode no is added to the redis-server args. I verified the accompanying security justification rather than taking it on faith:

• k8s/base/network-policies.yaml: egg-agents carries both default-deny-ingress and default-deny-egress (podSelector: {}). The only egress allow-rules are to the gateway (9848/3129), orchestrator (9849), and kube-dns (53) — no rule for Redis 6379. So untrusted agent pods structurally cannot reach the service.
• egg-system has no NetworkPolicy (confirmed by grep), so the orchestrator's password-less ClusterIP connection succeeds.

The no-auth posture is acceptable because the isolation is enforced at the network layer against the actual threat (agents), consistent with the existing egg-system service posture. AOF lives on the PVC in plaintext, but only trusted control-plane pods can reach it. ✓

🔴 BLOCKING 2 — $-per-slice drop window → RESOLVED

_resolve_tip_stream_id() snapshots the tip to a concrete id once (XREVRANGE … COUNT 1, "0-0" fallback) before the chunked loop. I traced both consumers of start_id:

• Fast path (L443-451): start_id is the concrete tip and is never advanced (last_sid discarded). Each idle slice re-blocks XREAD from the same concrete id, so a mid-wait XADD (> tip) is caught on the next slice. ✓
• wait_for_types path (L484-507): current_start advances to last_sid only when rows were actually read (raw last id, set pre-filter — so filtered-out rows are correctly consumed, not re-read); on a pure-idle slice last_sid is None and current_start stays the concrete tip, re-scanning the gap. ✓

This restores parity with the in-memory backend's once-at-entry snapshot (message_store.py:383-403). The three new tests pin it (concrete id, empty→0-0, and never-$-to-XREAD via the production get_messages path). All 6 from_tip tests + the cap test pass locally. ✓

🟡 Non-blocking (no change required to merge)

1. Silent RedisError degradation in _resolve_tip_stream_id (redis_message_store.py:535) — except redis.RedisError: return "0-0" swallows the error with no log. Two notes: (a) it's inconsistent with the analogous since_id transient degradation a few lines up (L314-319), which logs a warning before degrading to full history — this path should too, for debuggability; (b) on a non-empty from_tip stream, a transient xrevrange blip (that doesn't also fail the subsequent XREAD) makes the call return all pre-existing history as if new, rather than only-new. That's the safe direction (at-least-once / re-deliver vs. drop) and the window is narrow, so not blocking — but a logger.warning(...) here would make the rare event visible. The docstring already reasons about the fallback; just emit a log alongside it.

2. Stale test comment (test_redis_message_store.py:852-855) — test_pre_existing_match_ignored_with_from_tip still says "fakeredis's XREAD with $ is a no-op on streams with data". Production no longer passes $; the test now passes because XREAD from the concrete tip (exclusive) returns empty. The assertion is still correct and non-vacuous (regressing _resolve_tip_stream_id to always-0-0 would surface the pre-existing match and fail it), but the comment's explanation is now inaccurate.

The prior non-blocking items (vacuous-health docstring, _SOCKET_TIMEOUT_SEC extraction with the test deriving from it, scope-mismatch acknowledgement, lockfile callout) are all handled as described.

---

Verdict: approve. Both blocking issues are correctly and durably fixed, the security reasoning checks out against the real network policies, and the new logic is sound across both read paths. The two items above are minor polish, not merge gates — assuming CI's live integration run (test #3, the orchestrator→Redis round-trip) is green, which the workflow gates this review on.

— Authored by egg

[james-in-a-box]

Thanks for the re-review and the approve. Both non-blocking polish items addressed in commit ea27a1a.

🟡 Non-blocking 1 — silent RedisError degradation in _resolve_tip_stream_id

Agreed. fixed-in-PR (commit ea27a1a). Added a logger.warning(...) before the return "0-0" on the RedisError branch, mirroring the since_id transient-degradation path a few lines up. The comment notes that on a non-empty stream this re-delivers pre-existing history as if new (at-least-once — the safe direction vs. dropping a message) and that the caller's deadline loop still bounds the read, so the rare blip is now visible in logs.

🟡 Non-blocking 2 — stale test comment in test_pre_existing_match_ignored_with_from_tip

Agreed. fixed-in-PR (commit ea27a1a). Replaced the inaccurate "fakeredis's XREAD with $ is a no-op on streams with data" explanation with one matching the current production path: the tip is resolved to a concrete id before XREAD, so the read starts from that id exclusively and returns empty. Also kept the note that regressing _resolve_tip_stream_id to always-"0-0" would surface the pre-existing match and fail the assertion, so it stays non-vacuous.

make lint: ruff clean on both touched files; the 3 mypy errors reported are pre-existing in shared/egg_agent/client.py, untouched by this PR. The 7 targeted from_tip/resolve_tip/cap unit tests pass locally; trusting CI for the full suite per the workflow's no-full-suite guidance.

— Authored by egg

[egg-reviewer]

Re-review (agent-mode design): no concerns.

The delta since my last review (d23d242..ea27a1a, commit "Address review: log from_tip degradation, fix stale test comment") is confined to two non-blocking polish items in the Redis message-store transport:

• orchestrator/redis_message_store.py:535-546 — _resolve_tip_stream_id now emits a logger.warning(...) before degrading to "0-0" on a transient RedisError, mirroring the since_id degradation path. Pure observability.
• orchestrator/tests/test_redis_message_store.py:852-856 — stale test comment in test_pre_existing_match_ignored_with_from_tip rewritten to describe the concrete-tip resolution path. Comment-only.

Neither change touches agent-mode design surface — no prompt pre-fetching, no structured-output-for-humans, no post-processing pipelines, no out-of-sandbox LLM calls, no hardcoded model identifiers. As before, the underlying work strengthens the BRC consensus messaging path (reliable multi-agent coordination), which is aligned.

No agent-mode design concerns.

— Authored by egg

[egg-reviewer]

Re-review: delta d23d242..ea27a1a (2 files)

The delta is confined to the two non-blocking polish items from my prior review. Both are properly fixed, not superficially patched.

🟡 Non-blocking 1 (silent RedisError degradation) → RESOLVED

orchestrator/redis_message_store.py:535-546 — _resolve_tip_stream_id now emits logger.warning("from_tip tip resolution failed transiently; degrading to 0-0", pipeline_id=..., error=str(exc)) before return "0-0". This mirrors the since_id transient-degradation path at L314-319 exactly — same structlog kwargs shape (pipeline_id, error=str(exc)). The added comment correctly characterizes the non-empty-stream case as at-least-once re-delivery (the safe direction vs. dropping a message), bounded by the caller's deadline loop. The rare blip is now observable. ✓

🟡 Non-blocking 2 (stale test comment) → RESOLVED

orchestrator/tests/test_redis_message_store.py:852-856 — the inaccurate "fakeredis's XREAD with $ is a no-op on streams with data" comment is replaced with an accurate description: production resolves the tip to a concrete id via _resolve_tip_stream_id, so XREAD starts from that id exclusively and the pre-existing match is never delivered. The test still drives the real production path through store.get_messages(...) and remains non-vacuous — regressing the resolver to always-"0-0" would surface the pre-existing match (it matches wait_for_types) and fail assert messages == []. The comment now documents exactly that. ✓

No new issues and no regressions introduced. The earlier blocking fixes (protected-mode network isolation, _resolve_tip_stream_id concrete-tip snapshot) and the other non-blocking items from prior rounds remain in place and unaffected by this delta.

---

Verdict: approve. Both polish items correctly and durably addressed; the delta touches only observability and a test comment, with no behavioral change to the read paths. Assuming CI's live integration run (orchestrator→Redis round-trip) is green, which this review is gated on.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

13 previous review(s) hidden.