Repository navigation
FIX: keep template conditions until their parameters are rendered #3098
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
|
|
||
| from __future__ import annotations | ||
|
|
||
| import functools | ||
| import logging | ||
| import re | ||
| import uuid | ||
|
|
@@ -23,7 +24,7 @@ | |
| from pyrit.models.seeds.seed_origin import SeedOrigin | ||
|
|
||
| if TYPE_CHECKING: | ||
| from collections.abc import Iterator | ||
| from collections.abc import Callable, Iterator | ||
| from pathlib import Path | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
@@ -81,25 +82,66 @@ def __repr__(self) -> str: | |
| """ | ||
| return f"{{{{ {self._undefined_name} }}}}" if self._undefined_name else "" | ||
|
|
||
| # A placeholder cannot decide a branch or a loop: answering now would drop the | ||
| # {% if %} or {% for %} tags, and the later render could not decide again. | ||
| def __iter__(self) -> Iterator[object]: | ||
| """ | ||
| Return an empty iterator to prevent iteration over undefined variables. | ||
| Defer rendering instead of iterating over an unresolved variable. | ||
|
|
||
| Returns: | ||
| Iterator[object]: Empty iterator. | ||
| Raises: | ||
| _DeferRenderError: Always. | ||
|
|
||
| """ | ||
| return iter([]) | ||
| raise _DeferRenderError(self._undefined_name) | ||
|
|
||
| def __bool__(self) -> bool: | ||
| """ | ||
| Evaluate as truthy to avoid falsey-branch side effects. | ||
| Defer rendering instead of testing an unresolved variable. | ||
|
|
||
| Returns: | ||
| bool: Always True. | ||
| Raises: | ||
| _DeferRenderError: Always. | ||
|
|
||
| """ | ||
| raise _DeferRenderError(self._undefined_name) | ||
|
|
||
| def __eq__(self, other: object) -> bool: | ||
| """ | ||
| return True # Ensures it doesn't evaluate to False | ||
| Defer rendering instead of comparing an unresolved variable. | ||
|
|
||
| Raises: | ||
| _DeferRenderError: Always. | ||
|
|
||
| """ | ||
| raise _DeferRenderError(self._undefined_name) | ||
|
|
||
| def __ne__(self, other: object) -> bool: | ||
| """ | ||
| Defer rendering instead of comparing an unresolved variable. | ||
|
|
||
| Raises: | ||
| _DeferRenderError: Always. | ||
|
|
||
| """ | ||
| raise _DeferRenderError(self._undefined_name) | ||
|
|
||
| __hash__ = Undefined.__hash__ | ||
|
|
||
|
|
||
| class _DeferRenderError(Exception): | ||
| """Raised when an unresolved variable would decide a branch or a loop.""" | ||
|
|
||
|
|
||
| def _deferring(function: Callable[..., Any]) -> Callable[..., Any]: | ||
| # Jinja tests such as `is defined` and the `default` filter check the value's type, not its truth. | ||
| # functools.wraps keeps Jinja's pass_environment marker, so the value may not be the first argument. | ||
| @functools.wraps(function) | ||
| def deferring(*args: Any, **kwargs: Any) -> Any: | ||
| for arg in args: | ||
| if isinstance(arg, PartialUndefined): | ||
| raise _DeferRenderError(arg._undefined_name) | ||
| return function(*args, **kwargs) | ||
|
|
||
| return deferring | ||
|
|
||
|
|
||
| class Seed(BaseModel): | ||
|
|
@@ -223,11 +265,16 @@ def render_template_value_silent(self, **kwargs: Any) -> str: | |
|
|
||
| # Create a Jinja template with PartialUndefined placeholders | ||
| env = SandboxedEnvironment(undefined=PartialUndefined) | ||
| env.tests = {name: _deferring(test) for name, test in env.tests.items()} | ||
| env.filters["default"] = env.filters["d"] = _deferring(env.filters["default"]) | ||
| is_jinja_template = env.from_string(self.value) | ||
|
|
||
| try: | ||
| # Render the template with the provided kwargs | ||
| return is_jinja_template.render(**kwargs) | ||
| except _DeferRenderError: | ||
| # A missing parameter decides a branch or a loop - preserve the template as-is | ||
| return self.value | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Must Fix: Returning the original template here loses values supplied in earlier rendering passes. This breaks the existing from pyrit.datasets import TextJailBreak
template = TextJailBreak(
string_template="Style: {{ style }}. {% if prompt %}{{ prompt }}{% endif %}",
style="brief",
)
template.get_jailbreak("Explain rainbows")This returns Please preserve the already supplied values while deferring the unresolved condition, or retain and forward the bound context throughout these callers. Add regression tests for the construction/partial/final rendering chain, not just a final render that supplies everything again.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, thanks. In e97c4a0 the deferral applies only to the load-time render of trusted templates ( When the load render defers, each parameter it was given (the dataset paths) that the template uses is written in front of the template as a New tests for the chains:
These cases fail on df3331e and pass now. I also ran every shipped jailbreak template (650) through construction and |
||
| except Exception as e: | ||
| logger.error("Error rendering template: %s", e) | ||
| return self.value | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must Fix: This assignment fails the required type check with the locked dependencies:
_deferringreturnsCallable[..., Any], which does not preserve the signatures of the functions in Jinja's inferredenv.testsdictionary. The new dictionary is therefore not assignable toenv.tests. The repository'sty-checkpre-commit hook checks all ofpyrit, so this blocks that gate even though the runtime tests pass.Please preserve the wrapped callable's argument and return types, then rerun the type-check hook with the locked environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in e97c4a0.
_deferringis now typed(function: T) -> TwithTbound toCallable[..., Any], so each wrapped test and thedefaultfilter keep their own type andenv.testsaccepts the new table. With the locked ty 0.0.84,ty check pyrit/models/seedspasses, andty check pyritreports the same diagnostics asmainhere (only imports of optional extras I do not have installed).