UN-4123 [FEAT] Managed-Redis compatibility: TLS, connection health, and a configurable metrics database by muhammad-ali-e · Pull Request #2287 · Zipstack/unstract

muhammad-ali-e · 2026-09-17T07:26:07Z

What

Makes an encrypted connection to Redis possible, so the platform can run against a managed endpoint (Memorystore, ElastiCache, Azure Cache — the last of which disables its non-TLS port by default).

The scheme is the switch. {prefix}URL (falling back to REDIS_URL) goes to redis.Redis.from_url, and rediss:// selects TLS on its own — there is no separate "use TLS" flag to forget, and redis:// behaves exactly as today. Discrete host/port vars stay the primary path: they need no percent-encoding, and they are what the Helm chart, every sample.env, and the non-Python services (api-hub, llm-whisperer) read.

Also in scope, because they are the same class of silent failure:

{prefix}SSL now falls back to REDIS_SSL, plus a CA-certificate option.
The Django cache and the Socket.IO/kombu manager can finally use TLS (both hardcoded redis://).
The tool-sidecar and tool-container env allowlists carry TLS settings and REDIS_DB.
health_check_interval defaults to 30s.
The sdk1 metrics database is now configurable (METRICS_REDIS_DB), which is what makes a single-database endpoint reachable — see below.

Why

Password-only auth to an external Redis already worked, so TLS was the missing half. Chart-side support for pointing at an external endpoint is UN-4122 (cloud repo); this is the OSS half.

Everything is additive and inert by default — with nothing configured, the local/in-cluster path builds exactly the client it built before.

Two redis-py behaviours bite silently, and both are now handled and pinned by tests:

The URL path beats a db= kwarg. from_url('rediss://h:6380/5', db=1) yields db 5. sdk1 metrics asks for db=1 explicitly, so a URL carrying a path would have moved its keys into another service's keyspace with nothing to indicate it. The path is stripped when an override is given.
ssl=True into a ConnectionPool does not fail at construction. The pool defers its kwargs to the connection class, so platform-service (max_connections=10) started healthy, kept the plain Connection class, and raised TypeError: AbstractConnection.__init__() got an unexpected keyword argument 'ssl' on its first command. Pooled TLS now selects SSLConnection.

Three more silent failures fixed:

A forgotten per-prefix SSL flag is a plaintext client dialling a TLS port. CACHE_REDIS_SSL and MANUAL_REVIEW_REDIS_SSL had to be set separately; they now inherit REDIS_SSL and can still override it.
django-redis 5.4.0 ignores DB and USERNAME from OPTIONS. Verified against the installed version — ConnectionFactory.make_connection_params reads only PASSWORD and the two timeouts; DB: 3 yields db=None, while redis://h:6379/3 yields db=3. The db now travels in the LOCATION URL, so the backend cache stops sitting on db 0 while every other service honours REDIS_DB: with REDIS_DB=N, workers RPUSH log_history_queue to db N and the backend LPOPs an empty db 0. USERNAME is deliberately not restored — auth stays password-only as the built-in default user, which is what a managed AUTH string is; named ACL users cannot work platform-wide while django-redis discards the username.
kombu reads TLS off the scheme but defaults ssl_cert_reqs to CERT_NONE — encrypted while accepting any certificate. The Socket.IO manager URL now carries an explicit ssl_cert_reqs, since KombuManager takes a URL rather than connection kwargs.

Single-database endpoints

A whole class of managed Redis exposes db 0 only: Azure Managed Redis, Redis Enterprise / Redis Cloud, and every cluster-mode service (ElastiCache and Memorystore included). Unstract used dbs 0, 1, 5 and 8, so those tiers were out of reach.

MetricsMixin hardcoded create_redis_client(db=1) — after a sweep of every create_redis_client() call site, the only place left in the codebase that chose a database in code rather than in configuration. (backend/utils/cache_service.py:71 looks like a second one but already takes db as a parameter.)

METRICS_REDIS_DB defaults to 1, the value that was hardcoded, so an unset var leaves every existing deployment byte-identical. A single-database deployment sets it to 0 alongside CACHE_REDIS_DB and FILE_ACTIVE_CACHE_REDIS_DB — those three are the whole of Unstract's on-prem map, since dbs 5 and 8 belong to api-hub and the llm-whisperer portal, both cloud-only. Multi-database stays the default and needs no configuration at all.

Read per instance rather than once at import, so a malformed value raises inside __init__'s existing try/except — costing the metric — instead of killing the process at import time.

The tool-container allowlist carries the key; the sidecar allowlist deliberately does not, because the sidecar publishes logs and never imports sdk1. Without the first, the setting would apply everywhere except the containers doing the work — the same partial, silent failure as the LOG_TRANSPORT allowlist miss in UN-3755.

Worth knowing: against a db-0-only endpoint today, this does not crash. redis-py connects lazily, so SELECT 1 fails on the first command inside the existing try/except — you lose the time-taken metric and gain error noise. So this converts a degraded state into a clean one rather than fixing an outage.

How

# either
REDIS_SSL=true
REDIS_SSL_CERT_REQS=required
REDIS_SSL_CA_CERTS=/etc/ssl/redis-ca.pem   # only where the CA isn't publicly trusted (Memorystore)

# or
REDIS_URL=rediss://:<password>@<host>:6380/0?ssl_cert_reqs=required

For a single-database endpoint, move the whole map together:

CACHE_REDIS_DB=0
FILE_ACTIVE_CACHE_REDIS_DB=0
METRICS_REDIS_DB=0

CACHE_REDIS_DB and FILE_ACTIVE_CACHE_REDIS_DB must always match — the workers write file_active:* to one and the backend reads them from the other. Splitting them stops active-file dedup finding anything, with no error and no log; every file is reprocessed as new. The chart-side PR fails the render on a mismatch.

health_check_interval defaults to 30s ({prefix}HEALTH_CHECK_INTERVAL, 0 disables). Only the two worker caches set it before, so a connection killed while parked — managed failover, or Azure Cache's 10-minute idle reaper — was discovered by a real command failing on it. This applies with or without TLS.

Retries are deliberately not enabled globally. retry_on_timeout would re-issue blocking BLPOP/BLMOVE calls whose reply was lost, risking consuming a second message rather than recovering the first — the trap already documented at pg_queue/result_backend.py:152.

Can this PR break any existing features. If yes, please list possible items. If no, please explain why.

No. Every new setting is opt-in and the default path is unchanged — the first test in the new suite pins plaintext/localhost/db0 with no SSL kwargs.

Two intentional behaviour changes on the existing path, both called out for review:

health_check_interval now defaults to 30s instead of 0. Effect: redis-py sends a PING before reusing a connection idle longer than that. Set REDIS_HEALTH_CHECK_INTERVAL=0 to restore the old behaviour.
The Django cache LOCATION now carries the db path. For REDIS_DB unset or 0 — every shipped config — the connection is identical. Where REDIS_DB=N, the backend cache moves from db 0 to db N, which is the fix described above; that deployment is currently split-brained with its own workers.
METRICS_REDIS_DB defaults to 1 — the literal it replaces. No shipped config sets it, so every deployment resolves to the same database on the same line.

Sidecar/tool env keys are forwarded only when set, so an unconfigured deployment sees no new variables.

Database Migrations

None.

Env Config

All optional, all defaulting to current behaviour: REDIS_URL / {prefix}URL, REDIS_SSL, REDIS_SSL_CERT_REQS, REDIS_SSL_CA_CERTS, REDIS_HEALTH_CHECK_INTERVAL, METRICS_REDIS_DB. Documented in backend/, runner/, platform-service/ and workers/ sample.env.

Relevant Docs

Module docstring in unstract/core/.../cache/redis_client.py covers URL-vs-discrete precedence and why discrete stays primary.

Merge order — this PR goes FIRST

Merge this before unstract-cloud#1774. The two ship as a pair, and the chart depends on a fallback added here.

#1774 stops rendering CACHE_REDIS_SSL into the worker ConfigMap (a rendered literal shadows the real value when REDIS_SSL arrives from an existingSecret or ESO). That is only safe because this PR makes _resolve_redis_env fall back {prefix}SSL → REDIS_SSL. On main today there is no fallback:

ssl = os.getenv(f"{env_prefix}SSL", "false").strip().lower() == "true"

so the chart landing first would leave the worker cache alone on a plaintext connection whenever TLS is enabled — degraded to no-cache with only a warning, nothing failing loudly.

Nothing in this PR depends on the chart, so it is safe to merge on its own.

Related Issues or PRs

UN-4123. Pairs with UN-4122 (cloud chart: external/managed Redis endpoint).

Not covered here, tracked separately: api-hub builds a credential-free redis:// URL and needs a one-line fix before AUTH is enabled anywhere, and llm-whisperer supports a password but has no TLS support. Both live in their own repos.

Dependencies Versions

No changes. Behaviour verified against the pinned redis-py 5.2.1, kombu 5.5.4, django-redis 5.4.0.

Notes on Testing

5 new tests in unstract/sdk1/tests/test_metrics_redis_db.py — default, override, and that a malformed value costs the metric rather than the process.
21 new tests in unstract/core/tests/test_redis_client_config.py — defaults, discrete TLS (including the pooled regression), URL mode (scheme, percent-decoded password, db precedence, per-prefix URLs), and password-only auth.
5 added to runner/tests/test_sidecar_log_transport.py, beside the existing LOG_TRANSPORT ones.
Suites green: core 136, runner 10, Redis-related workers 51.
Django settings verified by rendering both modes: plaintext → redis://localhost:6379/0 with no pool kwargs; TLS → rediss://cache.example:6380/3 with ssl_cert_reqs/ssl_ca_certs, and the Socket.IO URL gaining ?ssl_cert_reqs=required.
Live dual-mode run, done. A local stack was switched in-cluster Redis -> TLS Redis (docker/docker-compose-redis-tls.yaml, requirepass + --port 0 --tls-port 6380) -> back, with a real API execution completing in each mode. That run is what caught the CA being read inside the ssl gate, and the Django-cache/Socket.IO URL gap that left API results unreadable.
Still unproven: the container-based tool path. The executions above ran the structure tool, which runs in-process, so no tool container or sidecar was spawned. The sidecar's env forwarding is asserted by unit tests but not exercised end to end — worth one run with a classifier or text_extractor workflow before relying on TLS in production.

🤖 Generated with Claude Code

… healthy Makes an encrypted connection to Redis possible so the platform can run against a managed endpoint (Memorystore / ElastiCache / Azure Cache, which disables its non-TLS port by default). Chart-side support for pointing at an external Redis is UN-4122; password-only auth already worked, so what was missing was TLS. Everything here is additive and inert by default: with nothing configured, the local/in-cluster path builds exactly the client it built before. **The scheme is the switch.** `{prefix}URL` (falling back to REDIS_URL) is handed to redis.Redis.from_url, and `rediss://` selects TLS on its own — no separate "use TLS" flag to forget, and `redis://` behaves as today. Discrete host/port vars remain the primary path: they need no percent-encoding, and they are what the Helm chart, every sample.env and the non-Python services (api-hub, llm-whisperer) read. Two redis-py behaviours that bite silently, both handled and pinned by tests: * The URL path beats a `db=` kwarg. sdk1 metrics asks for db=1 explicitly, so a URL ending in /5 would have moved its keys into another service's keyspace with nothing to show for it. The path is stripped when an override is given. * `ssl=True` into a ConnectionPool does NOT fail at construction — the pool defers kwargs to the connection class, so platform-service (max_connections=10) started healthy, kept the PLAIN Connection class, and raised `TypeError: AbstractConnection.__init__() got an unexpected keyword argument 'ssl'` on its first command. Pooled TLS now selects SSLConnection instead. Also fixed, because they are the same class of silent failure: * `{prefix}SSL` falls back to REDIS_SSL. Enabling TLS platform-wide previously meant remembering CACHE_REDIS_SSL and MANUAL_REVIEW_REDIS_SSL too, and a forgotten one is a plaintext client dialling a TLS port. * django-redis 5.4.0 ignores both DB and USERNAME from OPTIONS (verified against the installed 5.4.0: make_connection_params reads only PASSWORD and timeouts). The db now travels in the LOCATION URL, so the backend cache stops sitting on db 0 while every other service honours REDIS_DB — with REDIS_DB=N the workers RPUSH log_history_queue to db N and the backend LPOPs an empty db 0. USERNAME is deliberately NOT restored: auth stays password-only as the built-in `default` user, which is what a managed AUTH string is. * kombu reads TLS off the scheme but defaults ssl_cert_reqs to CERT_NONE — encrypted while accepting any certificate. Socket.IO's manager URL carries an explicit ssl_cert_reqs, since KombuManager takes a URL, not kwargs. * The sidecar and tool-container environments are hand-picked allowlists (the trap that made the LOG_TRANSPORT fix necessary). TLS settings and REDIS_DB now reach both, and only when actually set — an empty string reads as "configured" to os.getenv and would suppress the fallback. health_check_interval now defaults to 30s, configurable via {prefix}HEALTH_CHECK_INTERVAL. Only the two worker caches set it before, so a connection killed while parked — managed failover, or Azure Cache's 10-minute idle reaper — was discovered by a real command failing. This is the one intentional behaviour change on the existing path, and it applies with or without TLS. Retries are deliberately NOT enabled globally: retry_on_timeout would re-issue blocking BLPOP/BLMOVE calls whose reply was lost, which risks consuming a second message rather than recovering the first. Tests: 21 new in unstract/core/tests/test_redis_client_config.py, 5 added to the runner sidecar suite. Core 136, runner 10, Redis-related worker tests 51 — green. Django settings verified by rendering both modes: plaintext yields redis://host:6379/0 with no pool kwargs; TLS yields rediss://…/3 plus ssl_cert_reqs/ssl_ca_certs, and the Socket.IO URL gains ?ssl_cert_reqs=required. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

greptile-apps · 2026-09-17T07:30:23Z

via Greptile

Confidence Score: 5/5

The PR appears safe to merge, with all previous findings resolved and no new actionable issue introduced since the previous review.

Summary

This PR adds managed-Redis compatibility across the shared Redis client, Django cache, Socket.IO/Kombu transport, tool containers, and metrics collection.

Supports URL-based and discrete TLS configuration, including custom certificate authorities.
Adds proactive Redis connection health checks and configurable metrics database selection.
Propagates Redis configuration into tool and sidecar environments.
Adds a local TLS Redis setup and regression coverage for connection derivation.
The only change since the previous review is a non-functional pytest type-annotation cleanup.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    ENV[Redis environment settings] --> CLIENT[Shared Redis client factory]
    ENV --> DJANGO[Django cache]
    ENV --> SOCKET[Socket.IO / Kombu URL]
    ENV --> RUNNER[Runner environment allowlist]
    RUNNER --> SIDECAR[Tool sidecar Redis client]
    CLIENT --> SERVICES[Platform services and workers]
    CLIENT --> METRICS[SDK metrics database]
    SOCKET --> BACKEND[Backend event subscriber]
    SOCKET --> LOGWORKER[Log-consumer publisher]

_{Reviews (8) · Last reviewed commit: "UN-4123 [FIX] Import pytest as a module ..."}

…gap it found A managed Redis differs from the dev one in two ways that matter to this change — it requires AUTH and speaks TLS — and neither was reachable from a laptop, so the TLS path had no way to be exercised before merge. `docker-compose-redis-tls.yaml` runs a Redis that does both on port 6380, alongside the normal `unstract-redis`, so a developer can flip between the two and confirm BOTH still work. Its plaintext listener is disabled (`--port 0`), matching Azure Cache's default: a component that fails to pick up the TLS settings cannot then quietly succeed over plaintext and hide the bug. **Running it immediately found one.** URL mode carries TLS in the scheme and never sets `{prefix}SSL`, but the CA was read inside that flag's branch — so a `rediss://` URL verified against the system trust store alone and could not talk to any server with a privately-signed certificate. That is exactly the case the CA option exists for (Memorystore's CA is Google-managed and not publicly trusted). The read moved out of the gate; consumers decide whether it applies, so a `redis://` URL still ignores it. Two regression tests cover both directions. Verified live against the container, with the new client code: url rediss + CA -> OK [SSLConnection] roundtrip url rediss, no CA -> FAIL CERTIFICATE_VERIFY_FAILED (expected) discrete REDIS_SSL=true + CA -> OK [SSLConnection] roundtrip plaintext against TLS port -> FAIL connection closed (expected) TLS, wrong password -> FAIL AuthenticationError (expected) pooled TLS (platform-service) -> OK [SSLConnection] ping plain local redis (mode A) -> OK [Connection] ping The negatives matter as much as the positives: they show a misconfigured client fails loudly rather than silently degrading to plaintext. redis-tls/README.md carries the switch-over runbook, including the one deliberate asymmetry — the runner uses `ssl_cert_reqs=none`, because it forwards its settings to tool sidecars and those get no CA mount (their environment is an allowlist and only the shared log dir is mounted). Encrypted without verification still exercises the forwarding fix and the handshake; a real managed endpoint does not hit this, since ElastiCache and Azure chain to public CAs. Certificates are gitignored — generate-certs.sh writes them locally, and the keys are unencrypted dev material. Core tests now 138. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Syncs with main, which has since removed the Celery execution transport from the workers, backend and SDK (UN-4078 / #2284). No conflicts: that change and this one touch different halves — it removed a transport, this one changes how the Redis CLIENT is built. Re-checked rather than assumed, because "merged cleanly" says nothing about whether the change still makes sense: * Socket.IO still rides kombu over Redis (backend/utils/log_events.py, workers/log_consumer/tasks.py) — so the rediss:// manager URL is still needed and still on the live log-streaming path. * The settings TLS block and the sidecar/tool env forwarding both survived intact. * Tests: core 160, runner 10, green. * Live re-probe against the local TLS Redis: managed-like URL mode and plain local Redis both connect.

The repo ignores *.sh, so `git add -A` skipped the script and the committed README referenced a file that did not exist in the branch — the dev harness was unusable for anyone cloning it. Force-added, as every other tracked .sh in this repo is. Found by running the harness from a fresh checkout rather than the worktree it was written in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Found on a live run against the TLS Redis, not by reading the code: an API deployment came back `execution_status: COMPLETED` with `result: null`. `create_redis_client` honoured REDIS_URL from the start, but settings/base.py still built its own URL from REDIS_HOST/REDIS_PORT — so with a URL configured the WORKERS moved to the managed endpoint while the BACKEND stayed on the in-cluster one. Nothing errors in that state; the two simply stop sharing a keyspace. The execution really did run, its result really was cached — into the URL's Redis — and the backend looked for it in the other server and found nothing. Verified by key: `api_results:b2dd28d9…:d4355e01…` existed only in the TLS instance. REDIS_URL now drives both the cache LOCATION and SOCKET_IO_MANAGER_URL. The CA is appended as a query parameter rather than passed separately, because BOTH consumers parse it out of the URL — confirmed against the pinned redis-py 5.2.1 (-> SSLConnection with ssl_ca_certs) and kombu 5.5.4 (-> CERT_REQUIRED + ssl_ca_certs). An explicit ssl_ca_certs already in the URL is left alone, and a plaintext redis:// URL never gets one. In URL mode the db and credentials are dropped from OPTIONS: they travel in the URL, and passing both invites one silently winning over the other — the same class of bug as the db kwarg losing to the URL path in redis-py. Tests: backend/backend/tests/test_redis_settings_derivation.py, 9 cases. They execute the real source range from settings/base.py rather than a copy of the logic, since the block runs at import and cannot be called. Covers the plaintext default unchanged, the db-in-LOCATION fix, TLS pool kwargs, kombu's CERT_NONE default, and URL mode in both directions.

for more information, see https://pre-commit.ci

Spotted while documenting the Helm keys, not at runtime — which is the only reason it was caught before someone hit it in a cluster. The chart configures ONE endpoint for the platform and sets CACHE_REDIS_DB=1 for the worker cache. With the endpoint given as REDIS_URL, the CACHE_REDIS_ prefix inherits that URL (no CACHE_REDIS_URL of its own) and took the db from its path — so the worker cache would quietly move from db 1 to db 0 and sit beside every other key. Nothing errors; the keys just relocate. A prefix's own {prefix}DB now applies to an INHERITED url. An explicit {prefix}URL is untouched, since it names its db deliberately, and an explicit db= argument still beats both (sdk1 metrics depends on that). Three tests pin the precedence chain: inherited URL + prefix db, explicit prefix URL, and the db= argument. Suite now 26.

for more information, see https://pre-commit.ci

… TLS too Review caught that enabling REDIS_SSL moved the BACKEND's Socket.IO manager to rediss:// while workers/log_consumer/tasks.py kept a hardcoded redis://. Against a TLS-only endpoint that publisher simply cannot connect, and execution-log events stop reaching the UI — the backend meanwhile looking perfectly healthy. The cause is that both files assembled this URL by hand, so they could drift; the fix is one builder in unstract.core that both import, not two corrected copies. Two more holes it closes, both specific to kombu taking a URL and nothing else: * ssl_cert_reqs was absent in URL mode. Kombu defaults a rediss:// URL to CERT_NONE, so TLS was encrypting traffic to a server it never authenticated — the connection an operator believes is verified is exactly the one that isn't. * ssl_ca_certs never reached Socket.IO at all. The Django cache gets the CA through pool kwargs, so against a privately-signed server (Memorystore) the cache would work while WebSocket delivery silently died. An explicit ssl_cert_reqs already in the URL is left alone, and a plaintext redis:// URL gains no TLS query. Verified live, not just in unit tests: kombu connects to the local TLS Redis with CERT_REQUIRED against the generated CA. 9 new cases in TestSocketIoUrl; core + backend suites 180 green, runner 10, worker log-stream 17. The backend's settings test moved its Socket.IO assertions to the core suite, which is where that logic now lives.

for more information, see https://pre-commit.ci

…e-database endpoints sdk1's MetricsMixin hardcoded create_redis_client(db=1) — the last place in the codebase that chose a Redis database in code rather than in configuration, and the only code blocker to running against a SINGLE-DATABASE endpoint. That matters because a whole class of managed Redis exposes db 0 only: Azure Managed Redis, Redis Enterprise / Redis Cloud, and every cluster-mode service (ElastiCache and Memorystore included). Against those, this client's SELECT 1 fails on first command — caught by the existing try/except, so the run survives but the time-taken metric is silently lost and the logs fill with errors. METRICS_REDIS_DB defaults to 1, the value that was hardcoded, so an unset var leaves every existing deployment byte-identical. A single-database deployment sets it to 0 alongside CACHE_REDIS_DB and FILE_ACTIVE_CACHE_REDIS_DB — those three are the whole of Unstract's on-prem database map. Multi-database stays the default and needs no configuration at all. Read per instance rather than once at import, so a malformed value raises inside __init__'s existing try/except (costing the metric) instead of killing the process at import time. The tool-container allowlist gains the key too. Tool containers build their own Redis client from a hand-picked environment, not an inherited one, so without this entry the setting would apply everywhere EXCEPT the containers doing the work — the same silent, partial failure as the LOG_TRANSPORT allowlist miss in UN-3755. The SIDECAR allowlist deliberately does not: it publishes logs and never imports sdk1. Note for operators, documented in workers/sample.env: CACHE_REDIS_DB and FILE_ACTIVE_CACHE_REDIS_DB must always match. Workers write file_active:* to one and the backend reads from the other, so splitting them stops active-file dedup finding anything — no error, just files reprocessed as new. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

SonarCloud python:S9084. `from pytest import MonkeyPatch` was only there to give the fixture a type annotation for ruff ANN001; `pytest.MonkeyPatch` does the same job without the from-import. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

github-actions · 2026-09-22T08:23:51Z

Unstract test results

Per-group results

Status	Group	Tier	Passed	Failed	Skipped	Duration (s)
✅	`e2e-api-deployment`	e2e	3	0	0	20.2
✅	`e2e-coowners`	e2e	1	0	0	1.4
✅	`e2e-etl`	e2e	1	0	0	8.4
✅	`e2e-login`	e2e	2	0	0	0.8
✅	`e2e-prompt-studio`	e2e	1	0	0	7.0
✅	`e2e-smoke`	e2e	2	0	0	0.7
✅	`e2e-workflow`	e2e	1	0	0	19.9
❌	`frontend`	unit	0	1	0	0.0
✅	`integration-backend`	integration	598	0	26	57.0
✅	`integration-connectors`	integration	1	0	7	8.2
❌	`integration-workers`	integration	159	5	1	55.6
❌	`ui`	e2e	0	1	0	0.0
✅	`unit-backend`	unit	1291	0	1	44.9
✅	`unit-connectors`	unit	63	0	0	9.3
✅	`unit-core`	unit	172	0	0	2.6
✅	`unit-platform-service`	unit	15	0	0	2.4
✅	`unit-rig`	unit	120	0	0	4.5
✅	`unit-runner`	unit	10	0	0	2.9
✅	`unit-sdk1`	unit	585	0	0	27.9
✅	`unit-workers`	unit	1362	0	1	123.5
	TOTAL		4387	7	36	397.3

Critical paths

⚠️ Critical paths not yet covered

workflow-execution-fan-out — Multi-file workflow execution fans out to file-processing workers and rejoins. (declared coverage: no groups declared)

✅ Covered critical paths

auth-login — covered by e2e-login
adapter-register-llm — covered by integration-backend
workflow-author — covered by integration-backend
co-owner-manage — covered by integration-backend, e2e-coowners
workflow-create-execute — covered by e2e-workflow
api-deployment-provision — covered by integration-backend
api-deployment-auth — covered by integration-backend
api-deployment-run — covered by e2e-api-deployment
mcp-server-auth — covered by integration-backend
mcp-platform-auth — covered by integration-backend
platform-key-whoami — covered by integration-backend
prompt-studio-author — covered by integration-backend
prompt-studio-fetch-response — covered by e2e-prompt-studio
connector-register-test — covered by integration-backend
pipeline-etl-execute — covered by e2e-etl
usage-aggregate-read — covered by integration-backend
usage-token-tracking — covered by e2e-api-deployment
callback-result-delivery — covered by e2e-api-deployment

muhammad-ali-e

Standardized PR Review — FOLLOWUP

Verdict: REQUEST CHANGES

Summary — Critical: 0 · High: 3 · Medium: 13 · Low: 6 · Lenses run: 16/16

Reviewed under unstract plugin v0.18.1 · head 3690a07ff · base 53269a1fc.

This is a careful, well-evidenced PR — the reasoning in the comments is better than most, the tests assert on real connection objects rather than mocks, and the two redis-py traps it documents both reproduce exactly as described. Nothing here breaks an existing deployment: the default path is genuinely inert. What holds it up is that the TLS it introduces does not authenticate the server, and that ssl_cert_reqs is resolved three different ways, so half the platform ignores the operator's setting — including in the dev recipe this PR ships to verify the work.

Prior context fetched

All six gh calls ran clean. My 6 prior reviews / 6 inline comments on this PR are all replies acknowledging Greptile's findings — none asserts a problem of my own, so by the prompt's parsing rules I carry zero prior findings into this run. There is therefore no status table. Greptile's six P1/P2s are another reviewer's ledger; I spot-checked them and all six are genuinely fixed at the current head, but I do not mark them resolved on that reviewer's behalf.

Since-boundary: my last replies were at 62b0224b7; 14d38c4a3 and 3690a07ff landed after.

Scope change

YES — 14d38c4a3 added the METRICS_REDIS_DB feature after my last pass: a new public helper, a new env var, a new test file, and a new key in the tool-container allowlist. New surface, so all 16 lenses were run over the whole diff rather than a targeted re-check.

How this was verified

Every third-party behavioural claim below was executed against the pinned packages (redis-py 5.2.1, kombu 5.5.4, django-redis 5.4.0), not recalled. Two of the PR's own load-bearing claims — the URL path beating a db= kwarg, and ssl=True into a ConnectionPool raising only on first command — reproduce exactly as documented.

I also rejected a proposed finding that Sentinel+TLS raises TypeError: redis-py's SentinelConnectionPool pops ssl and selects SentinelManagedSSLConnection itself, so the master path is fine. The real Sentinel issue is narrower and is finding #9.

Lens checklist

#	Lens	Result
1	Spec & intent	See #6, #12
2	Architectural fit & precedent	See #11, #8
3	Correctness & edge cases	See #2, #7, #10
4	Security	See #1, #9
5	Data integrity & migrations	See #6 (assessed by me)
6	Concurrency	Clean (assessed by me — the health-check PING precedes the command; the blocking `BLPOP`/`BLMOVE` socket-timeout contracts at `result_backend.py:188` and `redis_stream_consumer.py:56` are unaffected)
7	API & contract compatibility	See #2, #5
8	Reliability & resilience	See #10, #12 (assessed by me)
9	Performance & cost	Clean — one extra PING per connection idle past 30s (assessed by me)
10	Observability	See #7, #8
11	Operational safety	See #6, #8, #12 (assessed by me)
12	LLM/agent	N/A — touches sdk1 plumbing only; no prompts, model config, or tool-call paths (assessed by me)
13	Testing	See #16 (assessed by me)
14	Dependencies & build	Clean — no dependency changes (assessed by me)
15	Code quality	See #11
16	Doc & comment accuracy	See #3, #4, #13, #14, #15

Low findings (not individually anchored)

redis_client.py:71-79 — _strip_url_db_path blanks the socket path of a unix:// URL, and does not strip a ?db= query param, which redis-py also honours over a db= kwarg. The docstring's "lets the explicit argument apply" is true only for the path form.
redis_client.py:45-46 — "30s is redis-py's own documented recommendation" is unsupported: redis-py 5.2.1's own default is 0 and the shipped distribution carries no such recommendation. (The companion claim, that 0 is what every client except the two worker caches used before, does check out.)
redis_client.py:154-156 — "KombuManager takes a URL, not connection kwargs" is false; it accepts connection_options, and workers/log_consumer/tasks.py:65-72 passes them in the Sentinel branch.
redis_client.py:8-13 — the module docstring says a URL "overrides the discrete host/port/credential vars" with no qualification, but Sentinel mode never reads it (build_socketio_redis_url's own docstring scopes itself correctly at :164-165).
redis_client.py:120-122 — health_check_interval=0 changed from "disabled" to "unset" on a public shared-library function, so a caller can no longer disable it through the API. Latent: no in-repo caller passes 0 today. int | None = None would keep 0 meaning zero.
redis_client.py:205 — int(os.getenv(f"{prefix}PORT", ...)) now runs during Django settings import via build_socketio_redis_url, so a set-but-empty REDIS_PORT="" (a blank ConfigMap key) fails the backend at boot rather than at first use.

Open questions

integration-workers reports 5 failures in the CI summary table while the gating jobs are all green. Known-flaky, or worth a look before merge?
Is hostname verification (#1) something you want on by default, or opt-in behind {prefix}SSL_CHECK_HOSTNAME?
The PR notes the container-based tool path is still unproven end to end. Findings #8 and #12 both live on that path — worth doing that classifier/text_extractor run before this is relied on.

Assumptions

The chart points every Redis prefix at the same host — verified: all *_REDIS_HOST values are unstract-platform-redis-master. That is what keeps #5 at Medium rather than High.
No shipped config sets a non-zero REDIS_DB for the backend, which keeps #6 a deploy note rather than a live break.

muhammad-ali-e

Addendum to the standardized review — lens 13 (Testing)

Revised verdict: REQUEST CHANGES — Critical: 0 · High: 5 (was 3) · Medium: 13 · Low: 8

My main review noted that lens 13 was assessed by me directly because the test-analysis pass had not finished. It has now, with mutation-verified results that change the picture, so I am correcting the record rather than leaving the earlier read to stand.

Two new High findings are inline below. Both were proven by mutating the shipping source and re-running the suite — not by reading it:

test_redis_settings_derivation.py executes a source slice that excludes all four variables this PR adds, testing a hand-written copy of them instead — which is precisely what its own docstring says the approach avoids.
Discrete-mode cache credentials are never asserted; dropping the Redis password leaves the suite green.

What this does not change

My earlier assessment of unstract/core/tests/test_redis_client_config.py stands, and was independently mutation-tested: three of the four regressions you say are pinned genuinely are — removing _strip_url_db_path fails 3 tests, removing the connection_class swap fails test_pooled_tls_client_is_usable, removing the ssl_cert_reqs injection fails 2. The fourth (django-redis ignoring DB/USERNAME) is the one with no executable assertion — finding #2 above. test_metrics_redis_db.py also pins the wiring, not just the helper: reverting metrics_mixin.py:47 fails all three parametrised cases.

No test was deleted or weakened anywhere: git diff -- "*test*" is 4 files, 567 insertions, zero deletions.

Additional Medium/Low, not separately anchored

unstract/workflow-execution/.../tools_utils.py:243-258 — the tool-container allowlist has no tests at all, and its package has no unit group in tests/groups.yaml (only an optional: true placeholder pointing at a directory that does not exist). Its structural twin, the sidecar allowlist, got five tests. This is the untested half of finding #8.
workers/log_consumer/tasks.py:59 — nothing asserts the worker actually calls build_socketio_redis_url; workers/tests/test_log_stream_consumer.py:61 stubs the module into sys.modules, so reverting that line to a hand-built f-string is caught by no test in the repo. That is the anti-drift fix's own half.
No test asserts SOCKET_IO_MANAGER_URL at all, though _derive() already returns it. One line — assert urlsplit(SOCKET_IO_MANAGER_URL).netloc == urlsplit(LOCATION).netloc — would encode the PR's actual premise.
unstract/sdk1/tests/test_metrics_redis_db.py:36 constructs a real client with no env isolation, unlike the core suite's autouse _clean_redis_env. With REDIS_SENTINEL_MODE=true in the ambient shell it enters the 10-attempt backoff and does real DNS for ~7 minutes. (I hit this myself while probing.) Copy that fixture over.
TLS is never exercised over a real socket in any CI tier. docker-compose-redis-tls.yaml makes the live run reproducible but leaves it manual — and --port 0 in that overlay already guarantees a service that misses the settings cannot quietly succeed, which makes it a good e2e-redis-tls target. Worth a follow-up ticket rather than this PR.
_derive() pins itself to two source strings via str.index (reordering base.py turns every test into a collection-time ValueError) and mutates os.environ by hand rather than via monkeypatch, so an interrupt inside exec leaves the session stripped of every REDIS_* key.
Surviving mutants in redis_client.py: the max(int(raw), 0) clamp at :60 (mutating to int(raw) leaves all 35 green), the prefix-scoped {prefix}HEALTH_CHECK_INTERVAL lookup at :56, and build_socketio_redis_url's username+password branch at :176-180 — each verified working today, none pinned.

Correction I am holding to

A Sentinel + TLS TypeError was proposed again in this pass. I am rejecting it again: redis-py 5.2.1's SentinelConnectionPool.__init__ does kwargs.pop("ssl", False) and selects SentinelManagedSSLConnection itself, which I executed. The master path is fine. The real Sentinel gap is the plaintext discovery connection in finding #9 of the main review, and the absence of any Sentinel test — which stands.

… once Two review findings, both reproduced against the pinned redis-py 5.2.1. 1. TLS was never authenticating the server. redis-py defaults ssl_check_hostname to False and then OVERRIDES ssl.create_default_context()'s safe default with it, so every path this work added — discrete kwargs, Redis.from_url on rediss://, and kombu — validated the chain but not the identity. For the ElastiCache/Azure case this targets (public CA, no pinned ssl_ca_certs) that means any publicly trusted certificate for any domain is accepted, and an on-path attacker can terminate the connection and read or write execution state, cached results and log traffic. Encryption without server authentication is not what enabling TLS is understood to buy — the same argument this module already makes about kombu's CERT_NONE default. ssl_check_hostname now defaults to True, overridable per prefix via {prefix}SSL_CHECK_HOSTNAME falling back to REDIS_SSL_CHECK_HOSTNAME, and is forced off when ssl_cert_reqs is "none" because Python's ssl module raises on that combination. Applied in all four places: the discrete kwargs, the URL path, the Socket.IO/kombu URL, and the Django cache pool. The dev harness is unaffected: docker/redis-tls issues SANs for unstract-redis-managed, localhost and 127.0.0.1. 2. ssl_cert_reqs was resolved three different ways in one module, with two live consequences, both executed: * URL mode never read it. `REDIS_URL=rediss://host:6380/0` with REDIS_SSL_CERT_REQS=none gave the data-plane clients redis-py's default while build_socketio_redis_url emitted ?ssl_cert_reqs=none — one process, two verification policies, and nothing to say which was intended. * Prefixed clients had no generic fallback, unlike {prefix}SSL and {prefix}SSL_CA_CERTS which this work already gave one. REDIS_ honoured "none" while CACHE_REDIS_ and MANUAL_REVIEW_REDIS_ stayed "required", so the worker cache alone failed verification — and cache_backends.py catches that, logs a warning and sets available=False, degrading silently to no-cache. Now resolved once, outside the ssl gate and with the generic fallback, exactly as ssl_ca_certs already was; applied in both the discrete and URL paths, with a setting already present in the URL's query string still winning. One existing expectation updated deliberately, not to reach green: test_ssl_switches_scheme_and_pool_kwargs asserted the exact pool-kwargs dict and now includes ssl_check_hostname, because the rendered behaviour changed on purpose. Eight new tests cover prefix inheritance of cert_reqs, URL mode, the URL-query precedence, the hostname default, the forced-off case, the opt-out and the Socket.IO URL. 53 green across the core and backend Redis suites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

SonarCloud python:S3776 — the hostname-verification branch added in 3bc8aaa pushed the function's cognitive complexity from under the limit to 17. The assembly of a URL from the discrete vars was the part that did not belong: that function exists to get kombu's TLS settings into a query string, and it was also a URL builder. Moved to _compose_redis_url, leaving one expression at the call site. No behaviour change — 53 tests green across the core and backend Redis suites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…n the credentials Two testing findings, both mutation-verified by the reviewer and re-verified here. 1. The harness re-implemented the very lines this work adds. Its docstring says it executes the shipping source "instead of re-implementing it, which would test a copy rather than the thing that ships" — but the exec'd slice started at REDIS_SENTINEL_MODE (base.py:531), while REDIS_SSL, REDIS_SSL_CERT_REQS, REDIS_SSL_CA_CERTS and REDIS_URL are defined at :112-121. All four ran from a hand-written prelude instead, so the shipping definitions were untested and the copy could not diverge visibly. The prelude now keeps only the three imports and splices in the real definitions range. Both of the reviewer's mutations, which previously left the suite green, now fail: a typo'd REDIS_URI (which re-introduces the exact REDIS_URL-ignored regression this file exists to pin) fails 9 tests, and a flipped REDIS_SSL default fails test_plaintext_is_unchanged. 2. Discrete mode's cache credentials were never asserted. Discrete mode is the default for every existing on-prem and Helm deployment and the only path where the password reaches Redis through OPTIONS — in URL mode it rides inside the URL. Removing the PASSWORD assignment left the suite green while the cache authenticated as nobody: every read and write failing at runtime, nothing failing at import. That block is now built conditionally in this PR, so the credentials are newly reachable-or-not depending on a branch. Three cases added: the password reaches OPTIONS (mutation-verified red), DB and USERNAME are passed through, and URL mode does NOT duplicate them. The USERNAME assertion pins the stated invariant — django-redis 5.4.0 discards it, so password-only auth currently holds by accident of the pinned version; the test makes a bump that starts honouring it a deliberate decision rather than a surprise. Also documented in docker/redis-tls/README.md why the recipe now works: the reviewer was right that REDIS_SSL_CERT_REQS was ignored in URL mode, and the fix in 3bc8aaa is what makes the documented dev setup correct as written. The URL query form is noted as an equivalent that wins if both are set. 61 green across the backend, core and sdk1 Redis suites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…TLS downgrade Three review findings, all reproduced. 1. A declared-but-BLANK METRICS_REDIS_DB disabled every sdk1 metric. An empty variable is this repo's own convention for "leave the default" (CACHE_REDIS_PASSWORD=, REDIS_SSL_CA_CERTS=), and every other variable in this work treats it that way — this one raised int(""). The exception was caught, so the claim that it costs the metric rather than the process held, but the consequence was every LLM and x2text timing silently missing platform-wide, once per instrumented call, behind a log line naming Redis rather than the variable. Blank now means unset; an unparseable value falls back to db 1 and says which variable it came from. The old test codified "" as malformed, so it was rewritten rather than adjusted: losing every metric is a bad trade for a typo. 2. REDIS_HEALTH_CHECK_INTERVAL reached neither container allowlist, though _resolve_health_check_interval reads it in the sidecar and in every tool container, three sample.env files document it, and the PR body names REDIS_HEALTH_CHECK_INTERVAL=0 as THE way to restore the previous behaviour for this work's one intentional default change. That lever stopped at the pod boundary. Added to both allowlists and both forwarding tuples. Benign in itself — an extra PING on connections idle past 30s — but it is exactly the trap the comments beside it were written to warn about. 3. REDIS_SSL=true beside an older plaintext REDIS_URL downgraded silently and then crashed the Django cache. The URL wins for the connection, so traffic went in clear while the operator believed TLS was on; meanwhile the cache's pool kwargs were gated on the flag alone, handing ssl_cert_reqs to a plain redis.Connection — verified: TypeError on first use, in a request rather than at startup. The pool kwargs now follow the EFFECTIVE scheme, and _resolve_redis_env logs an error when a URL's scheme disagrees with {prefix}SSL, since that combination is never deliberate. Tests: blank/unparseable/metrics-still-enabled for the sdk1 db, and TestSchemeFlagMismatch for the cache — the latter mutation-verified red against the flag-only gate. 63 green across the backend, core and sdk1 Redis suites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

SonarCloud python:S1192 — the scheme-mismatch check added in 178f1c7 made "rediss://" a third occurrence. Extracted as _TLS_SCHEME with a note on why it carries weight here: the scheme IS the TLS switch for redis-py, kombu and django-redis alike, which is the reason URL mode has no separate flag. No behaviour change; 65 green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

… the cache moves db 1. With REDIS_SSL=true, Sentinel deployments encrypted the master connection and left DISCOVERY in plaintext. _build_connection_kwargs returns from its auth-only branch before the ssl block, and redis-py builds the Sentinel clients from those kwargs alone — so against a Sentinel still accepting plaintext the Sentinel password went out in clear while the operator believed one switch had turned TLS on everywhere; against a TLS-only Sentinel, discovery failed through the full ten-attempt backoff first. The asymmetry predates this work, but this work is what makes REDIS_SSL the documented one-switch way to enable TLS, which is what brings operators to it. The discovery connections now follow {prefix}SSL rather than getting a switch of their own: a Sentinel node IS a redis-server and takes the same tls-port configuration, so the two never差 in practice. Verified — discovery is now SSLConnection and the master stays SentinelManagedSSLConnection. The review also checked and rejected a TypeError on the master path: SentinelConnectionPool pops `ssl` and selects the SSL class itself. That matches what this module found earlier and is why only the auth-only branch needed changing. 2. The Django cache's move from db 0 to REDIS_DB is correct — django-redis 5.4.0 ignores OPTIONS["DB"], so this cache always sat on db 0 — but it is a one-way RELOCATION of live data, not the inert change the PR describes. CacheService wraps get_redis_connection("default"), so log_history_queue, the rate-limit counters and the dashboard caches move with it, and a rolling deploy has old pods on db 0 while new pods read db N. No shipped config sets a non-zero REDIS_DB for the backend, which is what keeps it survivable — but it should not be silent, so it now logs a warning naming the database and what stays behind. Tests: Sentinel TLS on both planes, and the plaintext Sentinel unchanged — there was no Sentinel+TLS coverage at all before. 67 green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

SonarCloud python:S3776 — giving Sentinel's discovery connections TLS in 59df87e duplicated the TLS block into both branches of _build_connection_kwargs and pushed it to complexity 17. Extracted as _tls_kwargs, which fixes the duplication and the complexity together. That duplication is exactly how the bug arose in the first place: the auth-only branch returned before the TLS block, so one plane was encrypted and the other was not. A single helper means the two cannot drift again — worth more than the complexity score on its own. No behaviour change; 67 green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…e tense Three doc-accuracy findings. The first is the one that would have caused harm. 1. workers/sample.env told operators to set FILE_ACTIVE_CACHE_REDIS_DB in the WORKERS' environment, where nothing reads it — it is read only by the backend (settings/base.py, consumed in file_history_helper.py). Following that guidance sets it inertly, leaves the operator believing the pair is aligned, and produces exactly the silent dedup failure the same paragraph warns about. The same block called three keys "the whole of Unstract's on-prem database map". There are FOUR: REDIS_DB also selects a database — for the Django cache LOCATION and every REDIS_-prefixed client — so collapsing the other three onto a db-0-only endpoint while REDIS_DB stays non-zero still leaves clients on a database the endpoint does not have. It is the easiest to miss because it is already set, higher up the same file. Rewritten as a per-process table, since which ENV a key goes in is the whole point. 2. "USERNAME is deliberately NOT restored" sat eleven lines above code setting both USERNAME and DB. Both are genuinely inert — django-redis 5.4.0's make_connection_params reads PASSWORD and the two timeouts and nothing else — but a reader working out why username auth never reaches Redis found a comment and code that disagreed, with two dead OPTIONS keys looking load-bearing. The comment now says what is true: they are passed for readability and discarded, auth stays password-only, and the tests pin it so a dependency bump that starts honouring USERNAME is a visible change rather than a silent one. 3. Several comments narrated an INTERMEDIATE state of this branch as shipped history — "the backend learned TLS while the worker kept a hardcoded redis://", "create_redis_client honoured REDIS_URL from the start while this module built its own". Neither was ever true on main: both conditions existed only between commits here. Post-merge they read as a shipped incident, and this repo writes RCAs from exactly this kind of comment. The rationale was right and is kept — only the tense changed, to the hazard the code prevents rather than events that never happened. 67 green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…ken, and the CA caveat Three review findings. 1. The database precedence could not be stated, because there were two rules. An INHERITED generic URL honoured {prefix}DB — deliberately, since the chart ships one REDIS_URL with CACHE_REDIS_DB=1 beside it — while a prefix's OWN url did not. Worse than reported: an explicit REDIS_DB beside a pathless REDIS_URL gave db None, neither the env value nor a sane default. Now uniform: a URL supplies host, port and credentials; the database is {prefix}DB when explicitly set, otherwise the URL's path, otherwise 0; an explicit db= argument beats all of them. Stated in the module docstring and in workers/sample.env, where an operator meets it. One existing expectation changed deliberately: a prefix's own URL no longer keeps its db against an explicitly-set {prefix}DB. That test encoded exactly the inconsistency being removed. Leaving {prefix}DB unset gives the URL's db, which is covered by its own case. Not changed: the reviewer also suggested dropping URL inheritance per prefix. That would silently DOWNGRADE the worker cache to plaintext under a rediss:// URL, since the discrete path needs CACHE_REDIS_SSL and the scheme carries TLS instead — so inheritance stays and the rule is documented rather than removed. 2. docker/redis-tls/README.md put the workers' cache on db 1 while the backend kept FILE_ACTIVE_CACHE_REDIS_DB at its default 0 — the workers writing file_active:* to one database and the backend reading another, which is the silent dedup failure workers/sample.env warns about in this same PR. Anyone following it validated the TLS work on a quietly broken config. The recipe now sets FILE_ACTIVE_CACHE_REDIS_DB=1 and says why. 3. REDIS_SSL_CA_CERTS is forwarded into tool containers and sidecars as a PATH, and the runner mounts only the shared logs volume — so unless an image bakes the CA in at that path, load_verify_locations() raises on a file that is not there: the sidecar cannot build its LogPublisher and tool logs are lost. Kept forwarded, because the baked-in case is real and is the only way this works today, but the caveat now lives in runner/sample.env and at both allowlists rather than only in the dev README. 73 green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Two review findings, both about the same failure mode: a second copy of something. 1. base.py hand-built the TLS-query append ten lines above calling the shared builder, and the two copies already disagreed — unstract.core added ssl_cert_reqs AND ssl_ca_certs, base.py only ssl_ca_certs, so one process held two verification policies for one endpoint. That is precisely the drift the shared builder was introduced to prevent, reintroduced in the same change, and it is the mechanism behind the URL-mode cert_reqs bug fixed earlier. Extracted as ensure_tls_query_params — "given a Redis URL, add the TLS settings it is missing" — called from both. build_socketio_redis_url is now the thin wrapper that adds the discrete-vars fallback on top. Anything handing a URL to a library that reads TLS only from the query string wants this; kombu and django-redis both do. The helper now uses safe="/" so a CA path stays readable rather than arriving percent-encoded; two expectations that pinned %2F were updated to match, and the result still round-trips through urlsplit/parse_qs. 2. Added the allowlist test the review asked for — the one assertion that would have caught the REDIS_HEALTH_CHECK_INTERVAL omission from BOTH container lists and the METRICS_REDIS_DB divergence between them. It reads both constants files as text (neither package is importable here, and the question is what the source declares) and asserts every variable create_redis_client reads is forwarded, plus that METRICS_REDIS_DB is in the tool list and deliberately NOT in the sidecar's, since the sidecar publishes logs and never imports sdk1. Mutation-verified: removing the health-check entry from the sidecar list fails it. The review's other four gaps were closed by the fixes they accompanied — prefix inheritance of ssl_cert_reqs, URL mode plus the env var, Sentinel with TLS, and REDIS_SSL beside a plaintext URL — as was the sdk1 test that codified a blank value as malformed. 76 green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

sonarqubecloud · 2026-09-23T04:12:04Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

greptile-apps Bot reviewed Sep 17, 2026

View reviewed changes

Comment thread backend/backend/settings/base.py

Comment thread unstract/core/src/unstract/core/cache/redis_client.py Outdated

Comment thread backend/backend/settings/base.py

Comment thread backend/backend/settings/base.py Outdated

greptile-apps Bot reviewed Sep 21, 2026

View reviewed changes

Comment thread docker/redis-tls/README.md

muhammad-ali-e and others added 4 commits September 21, 2026 11:11

[pre-commit.ci] auto fixes from pre-commit.com hooks

74ff1e3

for more information, see https://pre-commit.ci

greptile-apps Bot reviewed Sep 21, 2026

View reviewed changes

Comment thread backend/backend/settings/base.py Outdated

muhammad-ali-e and others added 5 commits September 21, 2026 17:40

[pre-commit.ci] auto fixes from pre-commit.com hooks

fd8e733

for more information, see https://pre-commit.ci

[pre-commit.ci] auto fixes from pre-commit.com hooks

cf020fe

for more information, see https://pre-commit.ci

muhammad-ali-e changed the title ~~UN-4123 [FEAT] Support TLS to Redis, and keep pooled/idle connections healthy~~ UN-4123 [FEAT] Managed-Redis compatibility: TLS, connection health, and a configurable metrics database Sep 22, 2026

muhammad-ali-e marked this pull request as draft September 22, 2026 15:59

muhammad-ali-e commented Sep 22, 2026

View reviewed changes

Comment thread backend/backend/tests/test_redis_settings_derivation.py Outdated

Comment thread backend/backend/tests/test_redis_settings_derivation.py

muhammad-ali-e and others added 9 commits September 22, 2026 22:37

Conversation

muhammad-ali-e commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Single-database endpoints

How

Can this PR break any existing features. If yes, please list possible items. If no, please explain why.

Database Migrations

Env Config

Relevant Docs

Merge order — this PR goes FIRST

Related Issues or PRs

Dependencies Versions

Notes on Testing

Uh oh!

greptile-apps Bot commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 5/5

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 22, 2026

Unstract test results

Per-group results

Critical paths

⚠️ Critical paths not yet covered

Uh oh!

muhammad-ali-e left a comment

Choose a reason for hiding this comment

Standardized PR Review — FOLLOWUP

Prior context fetched

Scope change

How this was verified

Lens checklist

Low findings (not individually anchored)

Open questions

Assumptions

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

muhammad-ali-e left a comment

Choose a reason for hiding this comment

Addendum to the standardized review — lens 13 (Testing)

What this does not change

Additional Medium/Low, not separately anchored

Correction I am holding to

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud Bot commented Sep 23, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

muhammad-ali-e commented Sep 17, 2026 •

edited

Loading

greptile-apps Bot commented Sep 17, 2026 •

edited

Loading