RSPEED-2529: replace string concatenation with Jinja2 template rendering for system prompts#1267
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
WalkthroughAdds Jinja2-based templating for system prompts, a sandboxed cached Changes
Sequence DiagramsequenceDiagram
participant Client as API Request
participant Handler as _build_instructions()
participant Config as Configuration
participant Loader as _get_prompt_template()
participant Jinja2 as Jinja2 Engine
Client->>Handler: Request inference
Handler->>Config: Read customization.system_prompt
Handler->>Loader: Get compiled template (cached)
Loader->>Jinja2: Compile template (SandboxedEnvironment)
Jinja2-->>Loader: Template object
Loader-->>Handler: Cached template
Handler->>Handler: Build context (date, OS, version, arch)
Handler->>Jinja2: Render template with context
Jinja2-->>Handler: Rendered system prompt
Handler-->>Client: Invoke LLM with rendered prompt
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/endpoints/rlsapi_v1.py (1)
130-131: Clarify cache invalidation requirement for system_prompt refresh scenario.
@functools.lru_cache(maxsize=1)pins the first compiled prompt template for the process lifetime. The function docstring states "the system prompt does not change at runtime," butconfiguration.customization.system_promptis a reloadable configuration field. IfAppConfig.init_from_dict()is ever called at runtime (currently only used at startup), the cached template would become stale without explicit cache invalidation.While no current runtime refresh mechanism exists,
init_from_dict()clears other caches (conversation cache, quota limiters, token usage history) but omits_get_prompt_template.cache_clear(). Either:
- Document that system_prompt is intentionally immutable post-startup and update the docstring to reflect this architectural constraint, or
- Add
_get_prompt_template.cache_clear()toinit_from_dict()to support potential future configuration refresh scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/endpoints/rlsapi_v1.py` around lines 130 - 131, The lru_cache on _get_prompt_template() will pin the compiled prompt for process lifetime and can become stale if configuration.customization.system_prompt is reloaded; update AppConfig.init_from_dict() to call _get_prompt_template.cache_clear() after loading new config so the next call recompiles the updated template (refer to _get_prompt_template and AppConfig.init_from_dict()), or alternatively clarify the immutability by updating the _get_prompt_template() docstring to state system_prompt is immutable at runtime—pick one approach and implement it consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/endpoints/rlsapi_v1.py`:
- Around line 160-167: The ValueError raised when rendering the Jinja2 template
(in the env.from_string(...) block inside _build_instructions()) is not being
handled by infer_endpoint’s error pipeline, so malformed prompts bypass
_record_inference_failure and _map_inference_error_to_http_exception; update the
call site so template construction happens inside the try/except block used by
infer_endpoint (or catch ValueError around the _build_instructions() call) and
then rethrow or map the error into the existing pipeline so
_record_inference_failure and _map_inference_error_to_http_exception run for
template syntax errors; reference _build_instructions(), infer_endpoint,
_record_inference_failure, and _map_inference_error_to_http_exception when
making the change.
---
Nitpick comments:
In `@src/app/endpoints/rlsapi_v1.py`:
- Around line 130-131: The lru_cache on _get_prompt_template() will pin the
compiled prompt for process lifetime and can become stale if
configuration.customization.system_prompt is reloaded; update
AppConfig.init_from_dict() to call _get_prompt_template.cache_clear() after
loading new config so the next call recompiles the updated template (refer to
_get_prompt_template and AppConfig.init_from_dict()), or alternatively clarify
the immutability by updating the _get_prompt_template() docstring to state
system_prompt is immutable at runtime—pick one approach and implement it
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7dbc4556-b715-438d-93f7-bd8f912d5ca2
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
pyproject.tomlsrc/app/endpoints/rlsapi_v1.pytests/unit/app/endpoints/test_rlsapi_v1.py
|
@tisnik I added jinja as an explicit dependency, but it's already being pulled in as a transitive dependency by two or three packages, including starlette. |
major
force-pushed
the
RSPEED-2529/jinja2-prompt-templating
branch
from
7b6f896 to
08c44ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/unit/app/endpoints/test_rlsapi_v1.py (1)
272-283: Add one endpoint-level test for malformed template handling.You currently validate
ValueErrorat_build_instructions(), but not theinfer_endpoint()mapping path. Adding a test that asserts HTTP 500 +infer_errorevent on bad template would lock in the new error-pipeline behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/app/endpoints/test_rlsapi_v1.py` around lines 272 - 283, Add an endpoint-level test that trips the same malformed Jinja2 path through infer_endpoint so we assert the runtime mapping behavior: arrange a bad template via the existing mock_custom_prompt(bad_template) and construct a request that calls infer_endpoint (or the test client wrapper that routes to RlsapiV1.infer_endpoint) with a payload using RlsapiV1SystemInfo(os="RHEL", version="9.3", arch="x86_64"), then assert the HTTP response status is 500 and that an infer_error event/flag was emitted (or logged) as a result; reuse the existing fixtures and the _build_instructions reference to ensure the test exercises the same failure path.src/app/endpoints/rlsapi_v1.py (1)
60-60: Narrow the caught exception type instead of catching allValueErrors.Line 60 + Lines 357-363 currently treat any
ValueErroras “invalid system prompt template.” That can mislabel unrelated failures and reduce error signal quality. A dedicated exception subtype for template syntax keeps mapping precise.Also applies to: 357-363
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/endpoints/rlsapi_v1.py` at line 60, Replace the broad except ValueError used to label "invalid system prompt template" with a dedicated exception type: define a new exception class (e.g., InvalidSystemPromptTemplateError or SystemPromptTemplateError), update the template parsing/validation code to raise that specific exception instead of ValueError, and change the except ValueError blocks that emit the "invalid system prompt template" message to except InvalidSystemPromptTemplateError; also add the new exception to the module exports/imports so callers can catch it and adjust any tests/mocks accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/endpoints/rlsapi_v1.py`:
- Around line 131-159: The _get_prompt_template function is currently cached
with functools.lru_cache(maxsize=1) but takes no parameters, so it returns a
stale compiled template after config changes; modify _get_prompt_template to
accept the prompt text (e.g., add a prompt_text: str parameter that is the
resolved configuration.customization.system_prompt or
constants.DEFAULT_SYSTEM_PROMPT) and move the Jinja2 compilation inside using
that parameter, then change the decorator to `@functools.lru_cache`(maxsize=128)
(or similar) so the cache key includes the prompt text; update all call sites to
pass the resolved prompt string when invoking _get_prompt_template so
configuration reloads produce a new compiled template for changed prompts.
---
Nitpick comments:
In `@src/app/endpoints/rlsapi_v1.py`:
- Line 60: Replace the broad except ValueError used to label "invalid system
prompt template" with a dedicated exception type: define a new exception class
(e.g., InvalidSystemPromptTemplateError or SystemPromptTemplateError), update
the template parsing/validation code to raise that specific exception instead of
ValueError, and change the except ValueError blocks that emit the "invalid
system prompt template" message to except InvalidSystemPromptTemplateError; also
add the new exception to the module exports/imports so callers can catch it and
adjust any tests/mocks accordingly.
In `@tests/unit/app/endpoints/test_rlsapi_v1.py`:
- Around line 272-283: Add an endpoint-level test that trips the same malformed
Jinja2 path through infer_endpoint so we assert the runtime mapping behavior:
arrange a bad template via the existing mock_custom_prompt(bad_template) and
construct a request that calls infer_endpoint (or the test client wrapper that
routes to RlsapiV1.infer_endpoint) with a payload using
RlsapiV1SystemInfo(os="RHEL", version="9.3", arch="x86_64"), then assert the
HTTP response status is 500 and that an infer_error event/flag was emitted (or
logged) as a result; reuse the existing fixtures and the _build_instructions
reference to ensure the test exercises the same failure path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7aa21370-5164-40ca-8278-f3e98d3c2b77
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
pyproject.tomlsrc/app/endpoints/rlsapi_v1.pytests/unit/app/endpoints/test_rlsapi_v1.py
major
force-pushed
the
RSPEED-2529/jinja2-prompt-templating
branch
from
08c44ab to
4f7b91d
Compare
major
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/endpoints/rlsapi_v1.py (1)
149-152: Consider handlingSecurityErrorfrom the sandbox."Templates can still raise errors when compiled or rendered" and "If the template tries to access insecure code a SecurityError is raised." While admin-controlled templates using only simple variables (
date,os,version,arch) are unlikely to trigger this, catchingjinja2.sandbox.SecurityErrorin_build_instructions()and mapping it toValueErrorwould provide consistent error handling if a misconfigured template attempts unsafe operations.💡 Proposed enhancement in _build_instructions
def _build_instructions(systeminfo: RlsapiV1SystemInfo) -> str: ... date_today = datetime.now().strftime("%B %d, %Y") - return _get_prompt_template().render( - date=date_today, - os=systeminfo.os or "", - version=systeminfo.version or "", - arch=systeminfo.arch or "", - ) + try: + return _get_prompt_template().render( + date=date_today, + os=systeminfo.os or "", + version=systeminfo.version or "", + arch=systeminfo.arch or "", + ) + except jinja2.sandbox.SecurityError as exc: + raise ValueError( + f"System prompt template attempted unsafe operation: {exc}" + ) from exc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/endpoints/rlsapi_v1.py` around lines 149 - 152, Catch jinja2.sandbox.SecurityError inside _build_instructions() and re-raise it as a ValueError with a clear message so template sandbox violations surface consistently; specifically import jinja2.sandbox.SecurityError, wrap the existing template compilation/rendering calls in a try/except that catches SecurityError (in addition to any existing exceptions) and raise ValueError("invalid template: sandbox violation" or similar) preserving or appending the original error text for debugging, referencing the SandboxedEnvironment and the _build_instructions() function where templates are constructed and rendered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app/endpoints/rlsapi_v1.py`:
- Around line 149-152: Catch jinja2.sandbox.SecurityError inside
_build_instructions() and re-raise it as a ValueError with a clear message so
template sandbox violations surface consistently; specifically import
jinja2.sandbox.SecurityError, wrap the existing template compilation/rendering
calls in a try/except that catches SecurityError (in addition to any existing
exceptions) and raise ValueError("invalid template: sandbox violation" or
similar) preserving or appending the original error text for debugging,
referencing the SandboxedEnvironment and the _build_instructions() function
where templates are constructed and rendered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ec81fab4-674c-446b-b83e-f1960f82fb50
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
pyproject.tomlsrc/app/endpoints/rlsapi_v1.pytests/unit/app/endpoints/test_rlsapi_v1.py
…ing for system prompts
Operators can now use Jinja2 template variables ({{ date }}, {{ os }},
{{ version }}, {{ arch }}) and conditionals in system prompts. Prompts
without template syntax pass through unchanged.
Uses SandboxedEnvironment for defense-in-depth and catches
TemplateSyntaxError so malformed prompts surface a clear ValueError
instead of an opaque traceback on every request. The compiled template
is cached via lru_cache.
Signed-off-by: Major Hayden <major@redhat.com>
major
force-pushed
the
RSPEED-2529/jinja2-prompt-templating
branch
from
4f7b91d to
7f030f7
Compare
2 participants
Description
Replace the hard-coded string concatenation in
_build_instructionswith Jinja2 template rendering. Operators can now use{{ date }},{{ os }},{{ version }},{{ arch }}and conditionals ({% if os %}) in their system prompts. Plain prompts without template markers pass through unchanged.jinja2>=3.1.0SandboxedEnvironmentfor defense-in-depth against template injectionlru_cache(prompt doesn't change at runtime)TemplateSyntaxErrorcaught and re-raised asValueErrorwith the exact syntax problem; moved_build_instructionsinside the try block so template errors hit existing error mappingType of change
Tools used to create PR
Related Tickets & Documents
Checklist before requesting a review
Testing
uv run make verifypasses all linters (black, pylint, pyright, ruff, pydocstyle, mypy)uv run pytest tests/unit/app/endpoints/test_rlsapi_v1.py -vpasses all tests{{ unclosed,{% if %},{% endfor %}) raiseValueErrorwith "invalid Jinja2 syntax" messageSummary by CodeRabbit