Release 2.1.0 - #838
Merged
Merged
Conversation
Every other optional model-registry toggle is explicitly listed in the chart's env.config for discoverability (RERANKER_ENABLED, WITH_CHAINLIT_UI, ...). This one was missing entirely, so Helm-based deployments had no discoverable way to opt in even though the generic config map passthrough already supported it.
Removing it was justified as having no consumer. That was wrong: it is a
public API flag (metadata.spoken_style_answer, documented in the chat request
schema) and a persistent Chainlit command ("Get a conversational text answer
suitable for voice assistants"), wired end to end on develop. Deleting it was
an undeclared breaking change for any client using either.
Restore it and bring it into the library like every other type: an eighth
PromptType, seeded from its bundled template, resolvable per partition through
generation_prompt_names, and validated against the same {context}/{current_date}
placeholders as sys_prompt since the same call site renders both.
Add the regression test that was missing — nothing asserted the metadata flag
swapped the prompt, which is exactly why the removal passed a green suite.
The API now returns spoken_style_answer, which this page deliberately does not surface — it is driven by a chat metadata flag, not by anything configurable here — but the header counted every prompt the API returned, so it advertised eight prompts above seven cards. Count the managed types only. Adding the type to PROMPT_GROUPS later is all that's needed to surface it.
… flags Addresses the review on #835. - A brace in an interpolated identifier raised KeyError out of the request path. Both log lines built the format string by interpolation and passed the payload positionally, so loguru's message.format() parsed any surviving brace as a field. 'model' is client-controlled (metadata.llm_override), and llm.call is emitted outside the client's try/except, so a request naming a model 'gpt{x}' failed with LOG_LEVEL=DEBUG; a prompt named 'my{tmpl}' broke every request resolving it. Both now use a single literal placeholder with the whole line built inside the lazy callable, so nothing is rescanned. - Resolution moved onto the request path, giving chat and search a per-request Postgres dependency they did not have when prompts were read once at boot. A repository failure now degrades to the bundled template with one warning, swallowed at the single choke point rather than at each caller. ConfigError still covers the genuinely unrecoverable case. - Enrichment flags were read with a bare .get(), so a config omitting enable_image_captioning (default True) captioned during ingest while never resolving its prompt — silently ignoring the preset's selection and the library default. Defaults now come from IndexationPipelineConfig itself. Also tidies comments the reviewer flagged.
A concurrent boot against an empty database was worse than losing the seed: PgPromptRepository.create maps the unique violation to a ValidationError, _initialize_step re-raises it and ServiceContainer.initialize aborts, so the replica that loses the race fails to boot — N replicas crash-loop until one wins. Losing that race is a no-op; skip the type and carry on. This is not the pre-existing pattern the PR description claimed: preset seeding goes through an INSERT ... ON CONFLICT DO UPDATE and is race-safe. Only the model-endpoint seeding shares the check-then-insert shape. Also document the prompt library in API.mdx, which covered every sibling admin resource but not the seven /prompts routes or the partition-level generation_prompt_names field.
Hide Chainlit Sources when no evidence is available
… paths The unavailable-prompt guard could never raise what it intended: ConfigError hard-coded its own code and forwarded **kwargs, so ConfigError(msg, code='PROMPT_UNAVAILABLE') collided and the raise statement itself threw TypeError. Accept an overridable code, matching ValidationError in the same module. It survived because the raise was reasoned about, never executed. Add a test that drives it (no library default, no bundled template) and cover the rest of the service's error branches — malformed braces, unknown ids on get/update/set_default/delete, refusing to delete a type's default, and seeding skipping a type whose template is missing. Branch coverage of prompt_service goes 91% -> 96%, with every raise this PR adds now executed by a test.
`name` is embedded as a single path segment in every single-endpoint
route (GET/PUT/DELETE /model-endpoints/{model_type}/{name},
.../set-default, .../reveal-api-key, .../validate). Denylisting unsafe
values one at a time doesn't close the class: a `/` splits across path
segments (#768), and the exact values `.`/`..` are RFC 3986 dot-segments
that browsers and HTTP clients normalize out of the URL before the
request is even sent — the same "row exists, every route 404s" failure,
via a different mechanism.
Replace the denylist with one allowlist: name must start and end with an
alphanumeric character, with '.', '_', '-' allowed in between. That rules
out '/', '.', '..', and any leading/trailing separator by construction,
while still accepting realistic names like 'gpt-4.1' or 'jina_v3'. Also
caps the name at 128 characters (the DB column has no length bound
today) and mirrors the same regex in the admin UI's client-side guard —
FastAPI's validation `detail` is a list, so ApiError can't surface a
readable message for a raw 422.
RetrievalService now resolves a partition's reranker the same way QueryService._resolve_llm/_default_llm (#755) resolves chat_llm: the partition's configured preset first, falling back to the catalog default, then the static startup reranker, with the resolved endpoint logged at debug — so "which reranker ran?" is answerable from logs the way chat_llm already is. This also closes the gap #755 fixed for chat_llm, on the reranker side: a partition's `reranker` preset has no create/PATCH-time validation, so a renamed/deleted endpoint reaching `_reranker_factory` raised an unhandled KeyError instead of falling back to the catalog default (and then the static reranker if that's missing too). The fallback warning binds reranker/partition as structured Loguru context via `.bind()` rather than passing them as message-format kwargs — passed directly to `logger.warning(msg, key=val)`, they were silently dropped since the message has no `{}` placeholders to substitute into, so the log never actually recorded which reranker/partition triggered the fallback.
…, safely PgModelEndpointRepository.rename() now cascades the new name to every stored reference — partitions.embedder / partitions.chat_llm, and the endpoint-name fields embedded in pipeline_presets.config (JSONB) — in the same transaction as the rename. Before this, a renamed endpoint silently stranded every partition/preset that pointed at the old name, since nothing else in the schema updates those when the referenced row's name changes (#770). Raises NotFoundError if the row vanished between the service's existence check and this transaction (a concurrent delete), mirroring PgPipelinePresetRepository.rename. ModelEndpointService.update_model_endpoint reloads PresetService then PartitionService's in-memory caches after a rename (the same order PresetService.update_preset uses), so the cascade's DB writes actually take effect, and finally puts the until-now-unused partition_service constructor arg to use. Three concurrency gaps in that reload sequence are closed: - The DB rename commits before either reload call runs, and both `await`, so a concurrent request could resolve a name the cascade already repointed at against a registry that still only knew the old one (bare KeyError), or — if a reload call raised — never learn the new name at all until process restart. `_alias_renamed_name` makes both the old and new name resolve immediately after the rename commits, no `await` in between, closing that window regardless of how the reload calls resolve. - A rename combined with a field change (e.g. a new endpoint URL) was aliasing the *pre-update* config, since the alias copied whatever the in-memory bucket held before this call ran rather than the row this call just wrote — a reload failure right after would leave the registry silently serving stale settings under the DB-authoritative new name. The alias is now built from the fresh row. - The client-instance cache (checked before the config registry) could keep serving a client built against the pre-rename/pre-update endpoint under either name, since it was only evicted at the very end of the method — skipped entirely if a reload call raised. Both names' cached clients are now evicted right after aliasing, before either reload `await`. Finally, a DB-level race: a partition PATCH could validate chat_llm in-memory, then block behind a concurrent rename's cascade UPDATE on the exact partitions row it was about to write, then resume and write the now-renamed-away name straight back once the rename commits — stranding that partition permanently once the temporary alias above drops. rename() now LOCKs partitions IN SHARE MODE before touching anything, the same lock PgPresetRepository.delete already takes for the equivalent preset-delete-vs-assign race, and PgPartitionRepository.update_partition gets a matching DB-authoritative re-check: assigning chat_llm now runs the write and a model_endpoints existence check in one transaction that touches partitions before model_endpoints — the same order the rename cascade uses, so the two can only block on each other, never deadlock, and a write that loses the race rolls back with MODEL_ENDPOINT_NOT_FOUND instead of silently persisting a stale reference. embedder isn't covered — it has no assignment-time validation at all today, so there's nothing yet for a rename to race against on that column. Closes #768. All of the above lands from review feedback on #769 (@hedhoud, CodeRabbit) — reproduced independently as #770.
The backend trims `name` before applying the length/allowlist checks (and before persisting it), but this form validated the raw input — " gpt-4.1 " was accepted by the backend but blocked here. Validate and submit `name.trim()` consistently: the inline error, the create/update payloads, and the rename comparison all use the same trimmed value now.
fix(models): reject '/' in model-endpoint names
# Conflicts: # openrag/services/orchestrators/query_service.py # openrag/services/orchestrators/retrieval_service.py
The no-retrieval reply came from #807 and read an __init__-time snapshot this branch removes. Git auto-merged that reference without raising a conflict, so the resolution I wrote during the merge had nothing asserting it — the same shape of gap that let two stale attribute references through in the first place.
feat(prompts): DB-backed prompt library with per-preset and per-partition selection
feat(ui): admin prompt library and prompt selection in the preset/partition editors
infra/docker/ui.Dockerfile chowned nginx's runtime paths to 10001:10001 but
left USER as the base image's `nginx` (uid 101, gid 101 only), so the running
user had neither owner nor group access to them. The Helm chart matched the
chown with adminUi.podSecurityContext.runAsGroup: 10001, which only works on a
cluster whose SCC allows pinning that GID.
Switch to the same arbitrary-UID pattern api.Dockerfile already uses: chown to
group 0, chmod g+w, and USER 10001:0, with adminUi.podSecurityContext.runAsGroup
set to 0 to match. Under compose the user then owns the paths; under OpenShift's
restricted-v2 SCC an arbitrary UID writes through group 0. The base image itself
ships these paths as 101:0 for the same reason.
Nothing writes under /var/cache/nginx today — nginx-unprivileged points every
*_temp_path at /tmp and openrag-admin.conf sets `proxy_cache off` — so the
mismatch was latent rather than actively breaking uploads. The one observable
symptom was docker-entrypoint.d/10-listen-on-ipv6-by-default.sh failing to
patch /etc/nginx/conf.d/default.conf at startup.
Add regression tests pinning the image's USER/ownership and the chart's
runAsGroup, and document the 0.6.0 rag-* -> {{ fullname }}-* resource rename,
which leaves kept PVCs unmounted unless the operator overrides fullnameOverride
or migrates the data.
templates/postgres-migration-job.yaml runs the OpenRAG image with the same
pinned securityContext as its Deployment (runAsUser: 10001, runAsGroup: 0) but
never rendered a serviceAccountName, so it fell back to the namespace's
`default` ServiceAccount.
On OpenShift that puts the Job under restricted-v2 (MustRunAsRange) rather than
whatever SecurityContextConstraints is bound to the app's own ServiceAccount,
and admission rejects a runAsUser outside the namespace's assigned UID range.
Because the Job is a pre-install/pre-upgrade hook, that failure aborts the whole
release — and it only shows up once postgresProvisioning.migrationJob is enabled,
which is exactly the managed-Postgres setup the docs recommend on OpenShift.
Reuse openrag.serviceAccountName through tpl, like the Deployment does, so a
templated value such as "{{ .Release.Name }}-openrag" resolves the same way.
chore: update template & general improvement
…lished The v2.0.1 release produced a green build.yml run that built nothing: the build jobs were guarded on github.event.base_ref, which is empty for a tag pushed to a branch-protected main, so all three reported skipped while the run stayed green. The guard is fixed, but nothing in our process would have caught it, and the same class of failure can recur silently. This records the checks that prove a release shipped: assert per-job conclusions rather than the run's, resolve the manifest digest for all five registry coordinates, confirm latest matches the release and that ghcr and Docker Hub carry the same build, and read the version baked into the image so a tag placed on a pre-bump commit is caught. Includes the reconstructed v2.0.1 sequence and the rules it produced, plus the open risk that main's hardened build.yml has never yet run on a GA tag.
|
Important Review skippedToo many files! This PR contains 126 files, which is 26 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (127)
You can disable this status message by setting the 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cuts v2.1.0 from
develop(187a359e) per the adopted git flow. Feature release, not a patch bundle: it ships a DB-backed prompt library, user-management search and a partition-member picker.What's in it
Version bump (
4cb7e7ff)pyproject.toml->2.1.0,uv.lockregenerated (1 line, no dependency churn)Chart.yaml->appVersion: "2.1.0", chartversion: 0.6.1(on top of chore: update template & general improvement #633's0.6.0)values.yaml-> 3 image tags tov2.1.0(openrag, openrag-ray, openrag-admin-ui)docker-compose.yaml-> 2 image pins tov2.1.0No
v2.0.1string remains outside docs and the lock file.Verification
developdeployed and tested by a teammate at376d7c33; the delta to187a359etouches zero files underopenrag/,conf/,pyproject.tomloruv.lock— chart, docs,ui.Dockerfileand infra tests only.ui.Dockerfileis the only image input changed since v2.0.1, and it had never been built by CI (the nightly builds api + ray only). Built locally from this branch: servesGET /app/-> 200 both as the image's own user (uid 10001, gid 0) and as an arbitrary UID with gid 0 (restricted-v2 simulation), nginx workers start, no permission errors.develop, most recently on187a359e.tests/unit/infraall pass.Also adds
.github/RELEASING.md(184153ad)v2.0.1 produced a green
build.ymlrun that built nothing — three jobsskippedbehind an unusablebase_refguard. The guard is fixed, but nothing in our process would have caught it. This records the checks that prove a release actually shipped: per-job conclusions rather than the run's, manifest digests across all five registry coordinates,latestmatching the release, ghcr and Docker Hub carrying the same build, and the version baked into the image.After merge
v2.1.0on the merge commit on main ->build.ymlpublishes GA +latest.github/RELEASING.mdbefore announcing anythingbuild.ymldoesn't create one)main->developNote:
main's hardenedverify-tagguard has never run on a GA tag; this will be its first. Its logic was pre-flighted locally (tag regex, anonymous fetch of main) and it fails loud rather than skipping silently.