fix(index): wire startup check, repair healthcheck and CMD_TARGET#97
Conversation
check_provider_registration hardcoded the embedding provider name to "sentence-transformers" and was never called outside its own unit test, so it silently stopped protecting startup once TEI mode (VEKTRA_EMBEDDING_PROVIDER=tei) was added. Call it at the end of _step_5_register_providers with the configured vector_store_provider and sparse_embedding_provider, and check the "default" alias instead of the hardcoded name so it validates whichever embedding provider is active. Adds a regression test for a TEI-only registry (default + tei, no sentence-transformers). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The qdrant/qdrant:v1.17.0 image has no curl, so ["CMD", "curl", "--fail", "http://localhost:6333/healthz"] always failed with "exec: curl: executable file not found in $PATH", permanently reporting the container unhealthy in every deployment using the qdrant profile (verified on a live stack). bash is present in the image, so switch to a bash-only check that speaks raw HTTP over /dev/tcp and greps the status line. Verified empirically: recreated the live container with the new healthcheck and it flipped from unhealthy (FailingStreak 10235) to healthy (ExitCode 0) on the first probe. Also audited the TEI healthcheck (same curl pattern) against the pinned cpu-1.6 image: curl is present there, so it is left unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The entrypoint's CMD_TARGET="${1:-${CMD_TARGET:-server}}" prefers the
positional arg over the env var, but the Dockerfile's CMD ["server"]
meant Docker always passed "server" as $1, so CMD_TARGET could never
win: `CMD_TARGET=migrate docker run image` silently booted a full server
instead of running the one-shot migration (this hung a production
deployment script). Remove the Dockerfile CMD - the entrypoint already
defaults CMD_TARGET to "server" when no argument is given, so
`docker run image` and `docker run image migrate` are unaffected. Fix
the comment that claimed both forms already worked.
Verified: grepped the repo for anything relying on the image's default
CMD (compose files, CI workflows, scripts, docs) - nothing does;
docker-compose.yml already sets CMD_TARGET=server as an env var for the
vektra service, so its runtime behavior is unaffected. Confirmed the
dispatch logic itself in isolation (sh, no Docker) for all four
invocation combinations, including reproducing the prior bug for
contrast. Did not perform a full image build (expensive, out of scope
per instructions).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
test_store_honors_deterministic_ids_and_parent_linkage and test_store_falls_back_to_random_id_on_non_uuid patched DocumentChunkOrm and asserted on the mock's call_args_list, which tests the mock's bookkeeping rather than the real ORM mapping and hides mapping errors. .coderabbit.yaml's path instructions explicitly forbid mocking SQLAlchemy ORM classes. Both tests now let store() construct real DocumentChunkOrm instances and capture them via a session.add side effect (_make_session() already stubs session.add as a MagicMock), asserting on the captured instances' .id/.parent_id attributes instead. Also annotates BUG-021 in .s2s/BACKLOG.md with a related parity gap found in the same review: QdrantVectorStoreProvider.retrieve() filters by namespace_id only, unlike PgvectorProvider.retrieve() which also filters by index_version. Not fixed here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t chunk-storage claim The sync ingest response returns new/exists/alias, never "indexed" (that is an async job status), and MarkdownExtractor has always handled text/markdown. The agent instructions claimed chunk text lives in document_chunks; that table is written only by PgvectorProvider, so in Qdrant mode it is empty for every namespace and the Qdrant payload is the only source of chunk text. Recorded in BUG-021 as the root cause of the reindex no-op: run_reindex reads that empty table and reports success after writing nothing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fvadicamo
added
bug
github-actions
Bot
added
component:index
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR updates provider startup validation, Docker command dispatch and Qdrant healthchecks, Qdrant and ingest documentation, changelog entries, and pgvector tests that inspect persisted ORM instances. ChangesCore runtime and documentation updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several bug fixes, test refactorings, and documentation updates. Key changes include activating the check_provider_registration startup check using the 'default' embedding alias, updating the Qdrant healthcheck in docker-compose.yml to use a bash-only TCP check, and removing the default CMD in the Dockerfile to ensure the CMD_TARGET environment variable works as intended. Additionally, unit tests were updated to avoid mocking SQLAlchemy ORM classes, and documentation was corrected regarding API response statuses and chunk storage behavior. There are no review comments to evaluate, and I have no further feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
1 participant
Summary
Follow-ups from the review of PRs #93/#95, plus two deployment bugs found while shipping v0.6.0 to the system instance.
Fixes
1.
check_provider_registrationwas dead code (vektra-index/startup.py,vektra-app/main.py)ARCH-057 step 5 was documented but never wired: nothing called the function outside its own unit test, and it hardcoded
embedding/sentence-transformerswhile the app registersembedding/defaultplus the active alias. It is now called at the end of_step_5_register_providersand checks thedefaultalias, so it validates the provider that is actually in use. A regression test pins the TEI-only registry case (CodeRabbit flagged this as "TEI startup will fail" — it does not, precisely because the function was dead; the real defect was the orphaned validator).2. Qdrant healthcheck never passed (
docker-compose.yml)The check shelled out to
curl, which theqdrant/qdrantimage does not ship, so the container was reportedunhealthyin every deployment using theqdrantprofile (confirmed on a live stack:exec: "curl": executable file not found in $PATH). Nothing depends on it being healthy today, so nothing was breaking — but it would break the moment someone addeddepends_on: condition: service_healthy. Replaced with a bash-only/dev/tcpprobe of/healthz, verified against the pinnedv1.17.0image by flipping the live container fromunhealthytohealthy. The TEI healthcheck was audited too: that image does shipcurl, so it is unchanged.3.
CMD_TARGET=migratenever worked (Dockerfile,docker/entrypoint.sh)The entrypoint documented two invocation forms, but the Dockerfile's
CMD ["server"]was always passed as$1, which the precedence chain favours over the env var. The env-var form therefore booted a full server instead of running a one-shot migration. This is not theoretical: it hung the deployment script during the v0.6.0 rollout, waiting forever on a migration container that had turned itself into a web server. Removed theCMD(the entrypoint already defaults toserverwith no argument);docker run <image> migrateis unaffected.4. Tests no longer mock the SQLAlchemy ORM class (
vektra-index/tests/test_pgvector_unit.py)Two tests patched
DocumentChunkOrmand asserted on constructor kwargs, which hides real mapping errors and violates this repo's own path instructions. They now capture the real ORM instances through thesession.addintercept and assert on their attributes.5. Docs corrected (
docs/reference/api.md,docs/getting-started/index.md,.claude/CLAUDE.md)The ingest reference documented the sync response
statusas"indexed"(the code returnsnew/exists/alias;indexedis an async job status) and omitted Markdown from the supported formats even thoughMarkdownExtractorhas always been registered fortext/markdown. More importantly, the agent instructions claimed chunk text lives in the Postgresdocument_chunkstable: that table is written only by the pgvector provider, so withVEKTRA_VECTOR_STORE_PROVIDER=qdrantit is empty for every namespace and the Qdrant payload is the sole source of chunk text. That is also the root cause of the reindex no-op tracked under BUG-021 —run_reindexreads the empty table, re-embeds nothing, and reports success. Recorded in the backlog entry.Backlog
BUG-021 annotated with the root cause above, and with a parity gap found in review:
QdrantVectorStoreProvider.retrieve()does not scope byindex_version, unlike the pgvector provider. Not fixed here.Verification
make lintclean (ruff, mypy, import-linter 8/8 contracts).make test: 699 passed, 3 skipped, no regressions.CMDremoval was verified by grepping every compose file, workflow and doc for a dependency on the image's default command (none:docker-compose.ymlalready setsCMD_TARGET: serveras an env var) and by testing the entrypoint dispatch logic in isolation. Not verified with a full image build.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
curl.Documentation
newstatus.Tests