feat(prompts): DB-backed prompt library with per-preset and per-partition selection - #835
Conversation
The prompts library table (named prompt templates, one is_default per type via a partial unique index) plus partitions.generation_prompt_names (JSONB) for per-partition generation-prompt selection. PgPromptRepository replaces the stub with CRUD, get_by_name, and atomic set_default; idempotent Alembic migration. Integration tests against real Postgres. Refs #772
Unified resolver resolve_prompt(prompt_type, names=[...]): first resolvable candidate name -> global default -> on-disk seed. The ordered name list is the extension point for a future per-user tier (prepend the user's name). Seeds the library from bundled templates on boot; library CRUD with a delete-default guard and atomic default promotion. Lazy prompt_service in the container + get_prompt_service provider + a "seeding prompts" startup step. Refs #772
/prompts router (admin-only): list, get, create, patch, set-default, delete, with prompt_type as a Literal (edge 422s) and a non-empty-content guard. Registered in main.py under the Prompts tag. Transport tests cover forwarding, response shaping, 422 validation, and domain-error status mapping. Refs #772
…(generation) Wire the resolution seams to the unified resolver: - Indexation preset gains contextualization/image_captioning/topic_tagging_prompt_name; the Ray indexer resolves them once per file (actor already holds the catalog), threading the resolved text into the contextualize/topic-tag stages (which gain an optional per-call system_prompt override) and the existing caption_prompt row key. - Retrieval preset gains hyde/multi_query_prompt_name (retrieval-service wiring is a follow-up; those still load from disk, no behaviour change). - Partition gains generation_prompt_names (JSONB) for sys_prompt/spoken_style_answer/ query_contextualizer, editable via UpdatePartitionRequest and threaded through PartitionRow/PartitionConfig/partition_repo; QueryService resolves them for the single owning partition (multi-partition/"all" -> global default), matching chat_llm. Refs #772
- F1: unique index on (prompt_type, name) — the selection key must be unique for get_by_name to be deterministic. Require non-empty names, and reject duplicate/rename collisions with a 409 in PromptService. - F2: expose generation_prompt_names in PartitionDetailResponse / _partition_detail (was write-only — the UI could set but not read it back). - F3: validate assigned generation_prompt_names against the library at assignment time (symmetry with chat_llm), so a typo'd name is a 422 instead of a silent fallback to the default. - Fix a stale delete() comment that referenced the removed partition_prompts FK. Refs #772
list_prompts now annotates each prompt with used_by — the number of partitions (generation_prompt_names) and presets (*_prompt_name config) that reference it by name, from one bulk aggregate (PromptRepository.reference_counts, JSONB jsonb_each_text over partitions + pipeline_presets). Surfaced as PromptResponse.used_by for the admin-UI badge. Refs #772
Move query_contextualizer off the partition's generation prompts onto the retrieval preset (query_contextualizer_prompt_name), and wire the retrieval prompt seam: hyde/multi_query/query_contextualizer now resolve named->default ->disk via PromptService instead of loading disk-only. type=single touches no prompt. Generation prompts are now purely answer-side (sys_prompt, spoken_style_answer).
…ly answer prompt Drop the spoken-style answer capability entirely: the query-service branch and metadata flag, the Chainlit SpokenStyleAnswer command (i18n), the PromptType enum member, PromptsConfig field, disk template, the prompts CHECK constraint, and the seed set (8 -> 7 managed types). The partition now selects only sys_prompt (the final-answer prompt).
Admin-editable prompts for the 4 str.format-rendered types (sys_prompt, query_contextualizer, hyde, multi_query) are validated on create/update: unknown placeholders or malformed/unbalanced braces are rejected with 422 instead of crashing the chat/retrieval hot path at .format() time (globally for a default edit). Verbatim types (contextualizer/captioning/tagger) are unrestricted.
Remove the dead prompts.spoken_style_answer config key, the API metadata doc row + example, the env-vars template row, and correct a stale comment.
reference_counts() counted only explicit namings, so a seeded default (used by every non-overriding partition via fallback) showed 0/Unused. Now count effective resolution: each partition resolves each type to a named prompt (when its partition/preset config names an existing one) or the type's default, and the default absorbs the non-overriders. Per type the counts sum to the partition total.
resolve_prompt now emits one INFO line per call — prompt type, offered candidate names, how it resolved (named/default/disk-seed), the resolved name, length, and an 80-char preview — so operators can confirm in the logs exactly which library prompt every indexation/retrieval/chat stage used.
Line-reflow only, left behind by the last two prompt commits — the format gate was red on these three files.
prompt.resolve proves which library prompt a stage resolved; it does not prove the resolved text reached the model. Add a companion llm.call DEBUG line in the vLLM and Ollama clients (chat, stream_chat, generate, and VLM image captioning) previewing every outbound message by role and length. Previews are built lazily, so nothing is paid above DEBUG, and multimodal image parts are reduced to a <image_url> marker so base64 payloads never reach the log.
prompt.resolve fired at INFO on every chat request and every indexing job, carrying prompt text; demote it to DEBUG so it pairs with llm.call and a production deployment stays quiet. Build its preview lazily too. Also stop llm.call printing its payload twice: the lazy preview was passed as a kwarg, and loguru copies kwargs into record["extra"], which the terminal formatter appends. Pass it positionally instead.
create_prompt and the rename path check for a name clash before writing, but the check and the write are not atomic: two concurrent admins both pass it and the loser reaches uix_prompts_type_name, where an unhandled UniqueViolationError became a 500 from the generic exception handler. Translate it in the repository into the same 409 the sequential path returns, distinguishing a raced default promotion, and mirroring how PgPartitionRepository already handles partition-name collisions.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a DB-backed prompt library with admin CRUD routes, global defaults, partition-specific prompt selection, runtime resolution across query, retrieval, and indexing flows, and bounded DEBUG logging for Ollama and vLLM calls. ChangesDB-backed prompt library
LLM call logging
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
openrag/services/orchestrators/retrieval_service.py (1)
262-266: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSequential per-partition prompt resolution — consider bounding/parallelizing.
_pipeline_for_partitionis awaited one partition at a time. Forhyde/multiQuerypresets this triggers a DB round-trip per partition; with the"all"sentinel expanded to every partition (the SUPER_ADMIN_MODE case this file explicitly calls out for_gather_partition_groups,#708), this loop serializes N round-trips instead of running them concurrently (optionally bounded bymax_partition_concurrency, as already done for the retrieval fan-out itself).♻️ Suggested direction
- groups: list[tuple[list[str], RetrieverPipeline, int | None]] = [] - for partition in partitions: - pipeline, default_top_k = await self._pipeline_for_partition(partition) - groups.append(([partition], pipeline, default_top_k)) - return groups + results = await self._gather_partition_groups( + [self._pipeline_for_partition(partition) for partition in partitions] + ) + return [([partition], pipeline, default_top_k) for partition, (pipeline, default_top_k) in zip(partitions, results)]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openrag/services/orchestrators/retrieval_service.py` around lines 262 - 266, Update the partition loop in _gather_partition_groups to resolve _pipeline_for_partition concurrently rather than awaiting each partition serially. Reuse the existing max_partition_concurrency pattern or apply an equivalent bounded concurrency mechanism, while preserving each partition’s pipeline/default_top_k grouping and output behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openrag/services/inference/_call_log.py`:
- Around line 25-27: Update _preview to pseudonymize email addresses in the
normalized text before returning the preview, ensuring chat messages, retrieval
context, and vision prompts never expose raw email addresses at DEBUG while
preserving the existing whitespace normalization and length truncation behavior.
- Around line 78-82: Update the _detail function to cap the number of messages
and multimodal parts included in the rendered llm.call detail, then enforce a
maximum length on the complete joined result. Preserve the existing prompt
formatting and previews while truncating or summarizing excess content so DEBUG
logs remain bounded.
- Around line 88-90: Normalize and length-bound the user-controlled caller,
model, and endpoint values in the logging path before using them. Update the
logger.bind call and the formatted debug message in the surrounding call-logging
function so both consume the same sanitized values, preventing newlines or
oversized metadata from creating forged log entries.
In `@openrag/services/orchestrators/prompt_service.py`:
- Around line 149-169: Update PromptService.resolve_prompt so its documented
always-string contract holds when _disk_seed raises FileNotFoundError or
ValueError: catch those failures, log the resolution error, and return an
appropriate string fallback instead of propagating the exception. Preserve the
existing named, database-default, and successful disk-seed resolution order.
- Around line 115-138: Update seed_defaults to validate disk-seeded content
through the existing template-validation path before persisting it, such as by
reusing create_prompt or _validate_template. If validation fails for a templated
prompt type, catch the validation error, log a warning, and skip that seed;
preserve idempotency and continue seeding other prompt types.
In `@openrag/services/orchestrators/query_service.py`:
- Around line 340-343: Guard all three request-path prompt resolutions against
exceptions and use the existing disk-seed fallbacks: update generate_query’s
query_contextualizer call and _prepare_chat’s sys_prompt call in
openrag/services/orchestrators/query_service.py (lines 340-343 and 499-501), and
extend _resolve_query_template in
openrag/services/orchestrators/retrieval_service.py (lines 159-169) so
resolve_prompt failures reach its existing load_template_by_key fallback;
alternatively centralize the handling at PromptService.resolve_prompt while
preserving these fallback behaviors.
In `@tests/unit/services/inference/test_call_log.py`:
- Around line 65-69: Update the _Exploding test sentinel used by _preview() to
override split() and raise the assertion, rather than overriding __len__. This
ensures the test detects preview construction before the length check and still
fails whenever lazy evaluation regresses.
---
Nitpick comments:
In `@openrag/services/orchestrators/retrieval_service.py`:
- Around line 262-266: Update the partition loop in _gather_partition_groups to
resolve _pipeline_for_partition concurrently rather than awaiting each partition
serially. Reuse the existing max_partition_concurrency pattern or apply an
equivalent bounded concurrency mechanism, while preserving each partition’s
pipeline/default_top_k grouping and output behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d6175b3b-2be1-4495-8277-29b3ca0a23c0
📒 Files selected for processing (50)
conf/config.yamldocs/content/docs/documentation/API.mdxdocs/content/docs/documentation/env_vars.mdi8n/en-US.jsoni8n/fr.jsonopenrag/api/main.pyopenrag/api/routers/admin/prompts.pyopenrag/api/schemas/admin/partition_schemas.pyopenrag/api/schemas/admin/prompt_schemas.pyopenrag/api/schemas/user/chat.pyopenrag/app_front.pyopenrag/core/config/indexation_pipeline.pyopenrag/core/config/infrastructure.pyopenrag/core/config/retrieval_pipeline.pyopenrag/core/indexing/contextualize.pyopenrag/core/indexing/topic_tags.pyopenrag/core/models/preset.pyopenrag/core/models/prompt.pyopenrag/core/ports/prompt_repo.pyopenrag/di/container.pyopenrag/di/providers.pyopenrag/prompts/templates/spoken_style_answer_tmpl.txtopenrag/services/inference/_call_log.pyopenrag/services/inference/ollama_client.pyopenrag/services/inference/vllm_client.pyopenrag/services/orchestrators/partition_service.pyopenrag/services/orchestrators/prompt_service.pyopenrag/services/orchestrators/query_service.pyopenrag/services/orchestrators/retrieval_service.pyopenrag/services/persistence/migrations/alembic/versions/e8f9a0b1c2d3_add_prompts_library.pyopenrag/services/persistence/partition_repo.pyopenrag/services/persistence/prompt_repo.pyopenrag/services/persistence/schema.pyopenrag/services/workers/indexer_actor.pyopenrag/services/workers/indexer_pool.pyopenrag/services/workers/stages/contextualize.pyopenrag/services/workers/stages/topic_tag.pytests/integration/repos/conftest.pytests/integration/repos/test_partition_repo.pytests/integration/repos/test_prompt_repo.pytests/integration/repos/test_prompt_service_integration.pytests/unit/api/routers/admin/test_prompt_routes.pytests/unit/di/test_container.pytests/unit/services/inference/test_call_log.pytests/unit/services/orchestrators/test_prompt_service.pytests/unit/services/orchestrators/test_query_service.pytests/unit/services/orchestrators/test_retrieval_service.pytests/unit/services/persistence/test_partition_repo.pytests/unit/services/workers/stages/test_pipeline_stages.pytests/unit/services/workers/test_pipeline_builder.py
💤 Files with no reviewable changes (10)
- openrag/prompts/templates/spoken_style_answer_tmpl.txt
- docs/content/docs/documentation/API.mdx
- i8n/fr.json
- openrag/app_front.py
- openrag/api/schemas/user/chat.py
- openrag/core/models/prompt.py
- openrag/core/config/infrastructure.py
- docs/content/docs/documentation/env_vars.md
- i8n/en-US.json
- conf/config.yaml
Addresses the review on #835: - Pseudonymize email addresses in previewed prompt text. Prompts carry user questions and retrieved context; the diagnostic value is the prompt shape, not the personal data in it. - Bound the whole llm.call record, not just each fragment: cap the number of messages and multimodal parts rendered and clamp the joined result, so a long conversation cannot turn one call into an unbounded line. - Flatten and bound the interpolated identifiers. 'model' comes from the client via metadata.llm_override, so a newline in it could forge extra log lines. - Validate bundled templates in seed_defaults. It writes straight to the repo, so it was the one path able to store content the CRUD API would reject — a malformed bundled template would become a type's default and raise inside .format() on every request that fell back to it. - resolve_prompt no longer claims it always returns a string: an unreadable bundled template now raises a typed ConfigError naming the type instead of a bare FileNotFoundError surfacing as an opaque 500.
…ndex e5f6a7b8c9d0 landed on develop while this branch was open and took the same parent as e8f9a0b1c2d3, leaving two alembic heads once the branch is merged. 'alembic upgrade head' then aborts, ServiceContainer.initialize fails, and the app serves every request with a null container — which is why the API suite saw a 500 on every endpoint while the unit suite stayed green.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openrag/services/inference/_call_log.py`:
- Around line 46-47: Update _clip so its output never exceeds the requested
limit, including when truncation adds the ellipsis; reserve one character of the
limit for "…" before slicing the original text. Preserve unchanged text when its
length is already within the limit, and ensure all callers such as
PREVIEW_CHARS, MAX_META_CHARS, and MAX_DETAIL_CHARS receive strictly capped
output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e4408a10-8e04-4bad-a9fa-f74b251b5107
📒 Files selected for processing (7)
docs/content/docs/documentation/API.mdxopenrag/services/inference/_call_log.pyopenrag/services/orchestrators/partition_service.pyopenrag/services/orchestrators/prompt_service.pyopenrag/services/persistence/migrations/alembic/versions/e8f9a0b1c2d3_add_prompts_library.pyopenrag/services/persistence/schema.pytests/unit/services/inference/test_call_log.py
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/content/docs/documentation/API.mdx
- openrag/services/persistence/migrations/alembic/versions/e8f9a0b1c2d3_add_prompts_library.py
- openrag/services/persistence/schema.py
- openrag/services/orchestrators/partition_service.py
- openrag/services/orchestrators/prompt_service.py
_clip appended the ellipsis after slicing to the limit, so every clipped span emitted limit + 1 characters and PREVIEW_CHARS / MAX_META_CHARS / MAX_DETAIL_CHARS were each one over their stated ceiling. Reserve the ellipsis inside the budget and guard a non-positive limit. The tests asserted the caps loosely (one carried an explicit '+1 for the ellipsis'); they now assert the exact ceiling, which is what made the off-by-one invisible.
Validation reduced a field expression to its root name, so a template using a
conversion, format spec or attribute/index access passed the check and then
raised when the prompt was rendered: {context!x} raises ValueError and
{context.missing} raises AttributeError. Saved as a type's global default,
either fails every request that falls back to it — the exact failure this
write-time validation exists to prevent.
Accept only plain placeholders. These prompts are prose with a few injected
values, so nothing legitimate is lost, and a test asserts every bundled
template still passes so the seed path cannot silently skip a type.
Review notes (three items that aren't inline-anchorable)1. The
|
Review summaryReview-only pass — nothing pushed to this branch. Correcting my two earlier "I'll fix this" lines: the docs section and the code fixes are all yours. CI is green on The design holds up well: one resolution seam, selection filed where the setting belongs, soft name references with a default fallback, and the one invariant that needs SQL atomicity pushed into a partial unique index rather than application bookkeeping. The migration chain is a single head ( Findings, most serious first:
Plus the two I flagged as deliberately-not-fixed above: the inert Nothing here touches the feature's core; #1 is the only one I'd consider a merge blocker, and it's a two-line change plus a test. |
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.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
openrag/api/schemas/admin/partition_schemas.py (1)
79-82: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTrim prompt names before returning the mapping.
The validator checks
v.strip()but returns the original value, so names such as" my_prompt "are accepted and persisted. Downstream lookup uses the exact name, causing the override to miss and silently fall back. Store the normalized value instead.Proposed fix
+ normalized: dict[str, str] = {} for k, v in value.items(): if not isinstance(v, str) or not v.strip(): raise ValueError(f"generation_prompt_names['{k}'] must be a non-empty prompt name") - return value + normalized[k] = v.strip() + return normalized🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openrag/api/schemas/admin/partition_schemas.py` around lines 79 - 82, Update the generation_prompt_names validator to trim each string value before returning the mapping. Preserve the existing validation for non-string and blank values, then store the normalized prompt name so downstream lookups use the trimmed value.openrag/services/orchestrators/query_service.py (2)
429-431: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winResolve workspace scope before selecting the contextualizer prompt.
generate_query()runs before the workspace block narrowspartitiontoscope.partition. For workspace requests using"all"or multiple partitions,_retrieval_prompt_name()therefore returnsNone, selecting the global contextualizer even when the workspace’s owning partition has an override. Resolve the workspace first, or pass the effective partition intogenerate_query().Based on the supplied partition-aware prompt-resolution contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openrag/services/orchestrators/query_service.py` around lines 429 - 431, Update _prepare_chat so the workspace scope is resolved before generate_query selects the contextualizer prompt, and pass the effective scope.partition when applicable. Ensure workspace requests using "all" or multiple partitions use the owning partition’s override while preserving the existing partition behavior for non-workspace requests.
340-343:⚠️ Potential issue | 🟠 MajorDuplicate: request-time prompt-store failures can still become 500s.
Both calls await
resolve_prompt()directly. A database/pool exception can escape before the bundled disk seed is tried, so chat and query generation still depend on PostgreSQL availability. Fix this atPromptService.resolve_prompt()by catching only repository access failures and keeping_log_resolution()outside that boundary.Based on learnings, prompt resolution must fall back to the bundled seed on transient repository failures.
Also applies to: 500-503
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openrag/services/orchestrators/query_service.py` around lines 340 - 343, Update PromptService.resolve_prompt() to catch only transient repository/database access failures, then continue resolving from the bundled disk seed instead of propagating the exception. Keep _log_resolution() outside the repository-error handling boundary, and preserve normal propagation for non-repository failures so chat and query generation remain available when PostgreSQL is unavailable.Source: Learnings
🧹 Nitpick comments (1)
tests/unit/services/orchestrators/test_query_service.py (1)
491-526: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the rendered spoken-style system message.
This test verifies only that
"spoken_style_answer"was requested. Retain the returned payload and assert that the inserted system message contains the"SPOKEN::"result, so a regression that resolves the correct type but inserts the wrong template cannot pass.Based on the added branch-specific regression requirement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/services/orchestrators/test_query_service.py` around lines 491 - 526, Update test_spoken_style_metadata_swaps_the_answer_prompt to retain the result of _prepare_chat when spoken_style_answer is enabled, then assert the prepared messages include a system message containing the rendered “SPOKEN::” prompt payload. Keep the existing prompt-resolution call assertions and ordinary sys_prompt fallback checks unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@openrag/api/schemas/admin/partition_schemas.py`:
- Around line 79-82: Update the generation_prompt_names validator to trim each
string value before returning the mapping. Preserve the existing validation for
non-string and blank values, then store the normalized prompt name so downstream
lookups use the trimmed value.
In `@openrag/services/orchestrators/query_service.py`:
- Around line 429-431: Update _prepare_chat so the workspace scope is resolved
before generate_query selects the contextualizer prompt, and pass the effective
scope.partition when applicable. Ensure workspace requests using "all" or
multiple partitions use the owning partition’s override while preserving the
existing partition behavior for non-workspace requests.
- Around line 340-343: Update PromptService.resolve_prompt() to catch only
transient repository/database access failures, then continue resolving from the
bundled disk seed instead of propagating the exception. Keep _log_resolution()
outside the repository-error handling boundary, and preserve normal propagation
for non-repository failures so chat and query generation remain available when
PostgreSQL is unavailable.
---
Nitpick comments:
In `@tests/unit/services/orchestrators/test_query_service.py`:
- Around line 491-526: Update test_spoken_style_metadata_swaps_the_answer_prompt
to retain the result of _prepare_chat when spoken_style_answer is enabled, then
assert the prepared messages include a system message containing the rendered
“SPOKEN::” prompt payload. Keep the existing prompt-resolution call assertions
and ordinary sys_prompt fallback checks unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e0b31ce5-b9de-41ba-b983-230f5e00ca54
📒 Files selected for processing (9)
openrag/api/schemas/admin/partition_schemas.pyopenrag/api/schemas/admin/prompt_schemas.pyopenrag/core/models/preset.pyopenrag/services/orchestrators/prompt_service.pyopenrag/services/orchestrators/query_service.pyopenrag/services/persistence/migrations/alembic/versions/e8f9a0b1c2d3_add_prompts_library.pyopenrag/services/persistence/schema.pytests/unit/services/orchestrators/test_prompt_service.pytests/unit/services/orchestrators/test_query_service.py
🚧 Files skipped from review as they are similar to previous changes (2)
- openrag/core/models/preset.py
- tests/unit/services/orchestrators/test_prompt_service.py
… 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.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openrag/services/orchestrators/prompt_service.py (1)
46-55: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not re-add
spoken_style_answerto the managed prompt library.The PR objective removes this unused prompt, but these mappings make it managed again. Because
seed_defaults()iterates_TYPE_TO_CONFIG_KEY, every startup will validate and seed aspoken_style_answerglobal default and expose an obsolete record through the admin CRUD surface. Remove both entries unless retaining this prompt is intentional.Also applies to: 117-132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openrag/services/orchestrators/prompt_service.py` around lines 46 - 55, Remove the PromptType.SPOKEN_STYLE_ANSWER entry from _PROMPT_FORMAT_FIELDS and its corresponding entry from _TYPE_TO_CONFIG_KEY. Keep spoken_style_answer unmanaged so seed_defaults() does not validate or seed it and the admin CRUD surface does not expose an obsolete default.
🧹 Nitpick comments (1)
tests/unit/services/orchestrators/test_prompt_service.py (1)
288-304: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the
get_defaultfailure path separately.
ExplodingRepo.get_by_name()always raises, so theget_default()override at Line 299-300 is never reached. Add a repository fake that returnsNonefromget_by_name()and raises only fromget_default()to cover that fallback branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/services/orchestrators/test_prompt_service.py` around lines 288 - 304, The test currently exercises only the get_by_name failure path because that method raises before get_default is called. Add a separate repository fake or test using the existing service setup where get_by_name returns None and get_default raises, then assert resolve_prompt still returns the bundled sys_prompt template.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@openrag/services/orchestrators/prompt_service.py`:
- Around line 46-55: Remove the PromptType.SPOKEN_STYLE_ANSWER entry from
_PROMPT_FORMAT_FIELDS and its corresponding entry from _TYPE_TO_CONFIG_KEY. Keep
spoken_style_answer unmanaged so seed_defaults() does not validate or seed it
and the admin CRUD surface does not expose an obsolete default.
---
Nitpick comments:
In `@tests/unit/services/orchestrators/test_prompt_service.py`:
- Around line 288-304: The test currently exercises only the get_by_name failure
path because that method raises before get_default is called. Add a separate
repository fake or test using the existing service setup where get_by_name
returns None and get_default raises, then assert resolve_prompt still returns
the bundled sys_prompt template.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 22f21d1a-33ef-4571-a536-4418fe763aae
📒 Files selected for processing (9)
openrag/core/ports/prompt_repo.pyopenrag/services/inference/_call_log.pyopenrag/services/orchestrators/prompt_service.pyopenrag/services/persistence/prompt_repo.pyopenrag/services/workers/indexer_pool.pytests/unit/services/inference/test_call_log.pytests/unit/services/orchestrators/test_prompt_service.pytests/unit/services/workers/test_indexer_pool.pytests/unit/services/workers/test_pipeline_builder.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/services/workers/test_pipeline_builder.py
- openrag/services/persistence/prompt_repo.py
- openrag/core/ports/prompt_repo.py
|
@Ahmath-Gadji all five addressed in Also restored 2294 unit tests, ruff, layer guard and the prompt integration suite are green. |
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.
|
@Ahmath-Gadji thanks — I'd missed your two non-inline comments when I replied earlier. Both now addressed in Seeding race — you're right that "pre-existing pattern" was half true: API docs — added a
CI green on |
Verification of
|
| # | Finding | Status |
|---|---|---|
| 1 | Braces in log metadata → KeyError out of the request path |
✅ fixed 309f6140 — both sites, re-ran both repros; braces pass through verbatim, laziness intact |
| 2 | Per-request Postgres dependency with no fallback | ✅ fixed 309f6140 — repo raising RuntimeError now degrades to the bundled template with one warning |
| 3 | .get(flag) dropping the model default |
✅ fixed 309f6140 — defaults read from model_fields, so they can't drift; a sparse config resolves ['image_captioning'] |
| 4 | /prompts undocumented |
✅ fixed ea730de8 — accurate against the code, including the used_by semantics and the write-time template rules |
| 5 | Stale count / misfiled comments | ✅ fixed 309f6140, count correctly back to 8 via 60aac502 |
| — | Seeding race crash-loop (I'd flagged, not asked you to fix) | ✅ fixed ea730de8 |
Each fix has a regression test that fails without it, which is the part that matters — three of these were invisible to a green suite before.
60aac502 is a good catch of your own and I'd have missed it: metadata.spoken_style_answer is a documented public flag and a Chainlit command, so dropping it was a breaking change, and the reason nothing caught it is that nothing asserted the flag swapped the prompt. Restoring it as a library type is consistent everywhere I checked — PromptType, _PROMPT_TYPE_VALUES, the migration CHECK, PromptTypeName, _TYPE_TO_CONFIG_KEY, _PROMPT_FORMAT_FIELDS (same {context}/{current_date} pair as sys_prompt, correct since the same call site renders both), the partition key whitelist, PromptsConfig, the bundled template, docs and i18n. Migration chain is still a single head.
One new finding, which the fixes brought within reach rather than caused — prompt_service.py:243-247: the raise ConfigError(..., code="PROMPT_UNAVAILABLE") throws TypeError instead of the typed error, because ConfigError hard-codes code and forwards **kwargs. Reachable via a PROMPTS_DIR missing one template — seed_defaults skips that type, so it has no default and every resolution walks to the failing disk seed. Details and repro in the inline thread.
Two things left open by design, unchanged: the inert vlm_caption_prompt_name field in the preset UI (frontend PR), and preset *_prompt_name references still not validated at write time (your follow-up #1).
… 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.
Ahmath-Gadji
left a comment
Ahmath-Gadji
left a comment
There was a problem hiding this comment.
Approving on 764a79e1. Every finding from the review is addressed, each with a regression test that fails without its fix.
The ConfigError correction went the way I'd argued against, and it's the better call: making code keyword-only is safer than the variant I sketched (it can't be passed positionally, so it can't shadow anything), the four existing ConfigError sites are untouched, and the coverage added with it addresses the actual root cause rather than the symptom — a raise that was reasoned about but never executed. 91% → 96% branch coverage on prompt_service, with every raise this PR adds now driven by a test. That's worth more than the one-line deletion I proposed.
Verified on this head:
- The original repro now yields the intended error:
ConfigError | code=PROMPT_UNAVAILABLE | status=500, message naming the type. - Re-swept every exception construction site in
openrag/(42 sites passingcode=/status_code=, checked against what each class accepts): 0 raiseTypeError, down from 1. - Local unit suite 2303 passed,
ruff checkandruff format --checkclean.
Nothing outstanding on my side. The two items left open are deliberate and stated: the inert vlm_caption_prompt_name field in the preset UI (frontend PR), and preset *_prompt_name references not validated at write time (follow-up #1 in the description).
# 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.
Ahmath-Gadji
left a comment
Ahmath-Gadji
left a comment
There was a problem hiding this comment.
Re-approving on 6cbc40dd after the develop merge. Reviewed the conflict resolution rather than just the result — both sides survive in both files:
query_service.py — develop's prepend_system_prompt, force_retrieval, the requires_retrieval conversational skip, the web_source_numbers filtering and the reshaped _prepare_completions are all intact, and each of the three answer-prompt sites now resolves through the library instead of the __init__-time snapshots this branch deletes.
retrieval_service.py — develop's _resolve_reranker / _default_reranker_name land intact, with the reranker line correctly switched to the new helper, alongside this branch's async _pipeline_for_partition and hyde/multi_query template resolution.
6cbc40dd is the part worth calling out: the no-retrieval conversational reply from #807 read self._sys_prompt_tmplt, and git auto-merged that reference without reporting a conflict — so the resolution was live but unasserted. Adding the test that proves the conversational path resolves through PromptService (and honours the partition's selection) is exactly the right response to a silent auto-merge, and the commit message says so plainly.
Verified on this head:
- No leftover references to the three removed
__init__prompt attributes anywhere inopenrag/ortests/. - Migration graph still a single head (
e8f9a0b1c2d3, 15 revisions) — the merge brought no new revisions, so the multi-head hazard this branch already hit once didn't recur. - All five original findings still hold: braces inert in both log lines, a repo failure degrading to the bundled seed, the captioning flag defaulting to
True, andConfigErrorcarryingPROMPT_UNAVAILABLE. - 2429 unit tests pass (2303 → 2429 with
develop's),ruff check+ruff format --checkclean,check_layer_imports.pyOK.
Nothing outstanding. The two open items remain deliberate: the inert vlm_caption_prompt_name field in the preset UI (frontend PR), and preset *_prompt_name references not validated at write time (follow-up #1).
2 participants
Adds DB-backed prompt management: a global prompt library with a default per
type, selected per preset (indexation/retrieval) and per partition
(generation), and wired into every stage that sends a prompt to a model.
Closes #772.
What it does
Seven prompt types (
sys_prompt,query_contextualizer,chunk_contextualizer,image_captioning,hyde,multi_query,topic_tagger) become editable rowsinstead of read-only files on disk. Resolution is one seam:
first resolvable candidate name → the type's global default → the bundled disk
seed. Callers pass precedence-ordered names, so per-user personalisation can
later prepend a name without changing the signature.
Selection follows where the setting belongs:
sys_promptgeneration_prompt_namesquery_contextualizer,hyde,multi_querychunk_contextualizer,image_captioning,topic_taggerPreset fields serialise into the existing preset JSONB (no preset migration);
partitions get one JSONB column.
Notable choices
edit takes effect with no restart. Ingest prompts resolve once per file, not
per chunk — at 2K-document batches a per-chunk lookup would be a hot-path DB
read, and per-actor caching would need cross-Ray cache coherence.
str.formatacceptonly their own placeholders, so a bad template is a 422 on save instead of a
500 on every later chat. The other three are sent verbatim and may contain
braces.
inside a locked transaction, not application-level bookkeeping.
deleted or renamed prompt falls back to the default rather than breaking
retrieval. Deleting a type's default is refused.
spoken_style_answeris removed — it had no consumer;sys_promptis theonly answer prompt.
Verification
All seven prompts were proven end-to-end on a live deployment: each was given a
uniquely-worded library prompt, selected through its own config surface, and the
logs confirmed both that it resolved (
prompt.resolve … <- named:…) and thatits text reached the model (
llm.call … system[…]: …).sys_promptwas alsoconfirmed behaviourally — the answer came back in the style the prompt asked
for, with no restart. Image captioning logs
<image_url>in place of thebase64 payload.
Both log lines sit at DEBUG (
LOG_LEVEL=DEBUG), so production stays quiet.Unit suite 2258 green; repo-integration green against Postgres (two pre-existing
failures in
test_partition_repo.pyalso fail ondevelop, unrelated).Follow-ups (deliberately not in scope)
*_prompt_namereferences are not validated at write time, whilepartition ones are — a preset typo silently falls back to the default.
used_bycounts effective resolution, so a type's default shows everypartition that would fall back to it, including partitions whose retriever
never invokes that type.
(a pre-existing pattern shared with the other seeded tables).
Summary by CodeRabbit
spoken_style_answerprompt type.