ALL-206: Decouple local reranker service topology from Bedrock-only stacks

[christabone]

TL;DR

Decouple local reranker service startup from Bedrock/none topologies while still requiring it for local_transformers.

Summary

• Removed hard Weaviate depends-on links to reranker-transformers in dev/prod/test compose files.
• Set RERANKER_WAIT_FOR_STARTUP=false in Weaviate so Bedrock/none stacks can boot without local reranker.
• Added a shared provider helper and updated up-core, test prep, and review prep scripts so reranker-transformers runs only for local_transformers.
• Updated helper tests to assert both provider selections.

Alternatives

• We preferred script/compose startup gating over profile-only behavior to keep service settings and module wiring stable.

Test Plan

• bash scripts/tests/test_prepare_test_stack.sh
• bash scripts/tests/test_symphony_human_review_prep.sh

[claude]

PR Review: ALL-206 — Decouple local reranker service topology

Summary

This PR decouples the reranker-transformers container from Weaviate's hard depends_on in all compose files, gating it instead on the RERANK_PROVIDER environment variable. When the provider is local_transformers, the reranker service is included in startup; for bedrock or none, it is skipped. A shared helper library (scripts/lib/rerank_provider_common.sh) centralizes the provider detection logic, and RERANKER_WAIT_FOR_STARTUP: "false" is set in Weaviate so it can boot without blocking on the reranker module.

BLOCKING Issues

None found.

Non-blocking observations

Warning: Race window in make up-core for local_transformers mode — Makefile:109-116

With the depends_on constraint removed, make up-core starts reranker-transformers and weaviate concurrently (no --wait flag). Combined with RERANKER_WAIT_FOR_STARTUP: "false", Weaviate won't block for the reranker to become healthy. Early rerank queries during the startup window could fail. The test and review prep scripts use --wait which mitigates this, but the dev up-core target does not. This is low-risk since it's a local dev workflow and Weaviate handles lazy module connection gracefully, but worth noting.

Suggestion: Duplicated assert_not_contains helper — scripts/tests/test_prepare_test_stack.sh:17-23 and scripts/tests/test_symphony_human_review_prep.sh:17-23

Identical function defined in both test files. Could be extracted to a shared test helper, but this is minor and consistent with how assert_contains / assert_count are already handled in these files.

What looks good

• Clean extraction of shared provider logic into rerank_provider_common.sh with normalize_rerank_provider and rerank_provider_requires_local_service — keeps the conditional logic DRY across Makefile, test prep, and review prep scripts.
• Comprehensive test coverage: both none and local_transformers provider paths are tested with proper positive and negative assertions (assert_not_contains).
• pids_limit preserved in all Docker compose files.
• No secrets, no security concerns in the changes.
• ENABLE_MODULES and RERANKER_INFERENCE_API left wired in Weaviate config as documented in the PR ("keep service settings and module wiring stable") — the module is harmless when unused.

Overall Assessment

Clean, well-structured PR. The shared library approach is the right call for keeping provider detection consistent. Tests are thorough. No blocking issues.

---

Need help? Comment @claude followed by your request.

[christabone]

@claude Please review these recent changes.

• Updated PR head: 4cf99b9
• Context: addressed prior PR feedback

[claude]

Re-review: ALL-206 — Decouple local reranker service topology

Reviewing commit 4cf99b9 after prior feedback was addressed.

Summary

This PR decouples the reranker-transformers container from Weaviate's hard depends_on in all compose files (dev, prod, test), gating it on RERANK_PROVIDER via scripts instead. A shared helper library (scripts/lib/rerank_provider_common.sh) centralizes provider detection logic. RERANKER_WAIT_FOR_STARTUP: "false" is set in Weaviate so non-local-reranker stacks can boot cleanly.

Previously raised issues — status

Race window in make up-core (Warning) — Addressed. The up-core Makefile target now uses docker compose up -d --wait, ensuring all services (including reranker-transformers when applicable) are healthy before the command returns. This was missing in the earlier revision.

BLOCKING Issues

None found.

Non-blocking observations

Suggestion: Duplicated assert_not_contains helper — scripts/tests/test_prepare_test_stack.sh:17-23 and scripts/tests/test_symphony_human_review_prep.sh:17-23

Identical function in both test files. Could be extracted to a shared test helper alongside assert_contains / assert_count, but this is minor and consistent with the existing test pattern.

Review checklist

• Fallback patterns: The ${RERANK_PROVIDER:-none} defaults are legitimate configuration defaults (not set = no reranking), not fallbacks masking missing data. ${BEDROCK_RERANK_MODEL_ARN:-...} is a standard model ARN default. No blocking fallback patterns found.
• Dead code: No unreferenced code introduced. The reranker-transformers service definitions remain in compose files (needed for local_transformers mode). ENABLE_MODULES and RERANKER_INFERENCE_API are intentionally left wired (harmless when unused, per PR description).
• pids_limit: Not touched in any compose file — preserved.
• Security: No secrets or credentials in code. Env vars are properly parameterized.
• Tests: Both provider paths (none and local_transformers) are tested with positive and negative assertions. Existing tests updated to match new behavior.

Overall Assessment

Clean PR. The prior race-window concern has been addressed with --wait in the Makefile target. The shared library approach keeps provider detection DRY, and tests cover both topology paths thoroughly. No blocking issues.

---

Need help? Comment @claude followed by your request.

[christabone]

@claude Please review these recent changes.

• Updated PR head: 94890f2
• Context: addressed prior PR feedback

[claude]

Re-review: ALL-206 — Decouple local reranker service topology

Reviewing commit 94890f2 after prior feedback was addressed.

Summary

This PR decouples the reranker-transformers container from Weaviate's hard depends_on in all compose files (dev, prod, test), gating it on RERANK_PROVIDER via scripts instead. A shared helper library (scripts/lib/rerank_provider_common.sh) centralizes provider detection logic. RERANKER_WAIT_FOR_STARTUP: "false" is set in Weaviate so non-local-reranker stacks can boot without blocking on the module.

Previously raised issues — status

Issue	Status
Race window in make up-core (Warning) — up-core started services without --wait, meaning Weaviate could accept rerank queries before the reranker was healthy	Addressed. up-core now uses docker compose up -d --wait, ensuring all services are healthy before returning.
Duplicated assertion helpers (Suggestion) — assert_contains/assert_count duplicated across both test files	Addressed. Extracted to shared scripts/tests/lib/assertions.sh with a new assert_not_contains helper added as well. Both test files now source the shared lib.

BLOCKING Issues

None found.

Review checklist

• Fallback patterns: ${RERANK_PROVIDER:-none} and ${BEDROCK_RERANK_MODEL_ARN:-...} are legitimate configuration defaults (unset = no reranking / standard model ARN), not fallbacks masking broken data. normalize_rerank_provider normalizing empty to "none" is input sanitization. No blocking fallback patterns.
• Dead code: No unreferenced code introduced. Old inline assertion functions properly removed from both test files after extraction to shared lib. reranker-transformers service definitions remain in compose files (needed when RERANK_PROVIDER=local_transformers). ENABLE_MODULES and RERANKER_INFERENCE_API stay wired — harmless when unused and needed when the reranker runs.
• pids_limit: Not touched in any compose file — verified preserved in docker-compose.yml (lines 113, 301, 388) and docker-compose.production.yml (lines 127, 276, 349).
• Security: No secrets or credentials in code. Env vars properly parameterized.
• Tests: Both provider paths (none and local_transformers) tested with positive and negative assertions. Retry behavior verified. The new assert_not_contains adds proper negative-case coverage confirming reranker-transformers is excluded when not needed.

Overall Assessment

Clean PR. Both issues from previous reviews have been addressed. The shared helper library keeps provider detection DRY, the --wait flag closes the startup race window, and the extracted assertion lib eliminates test duplication. No blocking issues.

---

Need help? Comment @claude followed by your request.

[gitguardian]

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
9412085	Triggered	Generic Password	87179d4	scripts/tests/test_reranker_compose_profiles.sh	View secret

🛠 Guidelines to remediate hardcoded secrets

1. Understand the implications of revoking this secret by investigating where it is used in your code.
2. Replace and store your secret safely. Learn here the best practices.
3. Revoke and rotate this secret.
4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

• following these best practices for managing and storing secrets including API keys and other credentials
• install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

---

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}