FIX Include module colors, layout and injection items in QRCodeConverter and PDFConverter identifiers (#3245)

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Roman Lutz <romanlutz13@gmail.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: bd31d51f-1a2d-4441-8e25-50992348d470

Author: 4 people

pyrit/converter/pdf_converter.py

@@ -3,6 +3,7 @@
 
 import ast
 import hashlib
+import json
 import time
 from io import BytesIO
 from pathlib import Path
@@ -23,6 +24,9 @@
 _GENERATED_TEXT_COLOR: tuple[int, int, int] = (0, 0, 0)
 _INJECTED_TEXT_COLOR: tuple[int, int, int] = (255, 255, 255)
 
+# Default row height in mm for generated PDFs. The identifier leaves the default out, so default hashes don't move.
+_DEFAULT_ROW_HEIGHT = 10
+
 
 class PDFConverter(Converter):
     """
@@ -52,7 +56,7 @@ def __init__(
         page_width: int = 210,
         page_height: int = 297,
         column_width: int = 0,
-        row_height: int = 10,
+        row_height: int = _DEFAULT_ROW_HEIGHT,
         existing_pdf: Path | None = None,
         injection_items: list[dict[str, Any]] | None = None,
     ) -> None:
@@ -71,7 +75,8 @@ def __init__(
             column_width (int): Width of each column in the PDF. Defaults to 0 (full page width).
             row_height (int): Height of each row in the PDF. Defaults to 10.
             existing_pdf (Path | None, optional): Path to an existing PDF file. Defaults to None.
-            injection_items (list[Dict] | None, optional): A list of injection items for modifying an existing PDF.
+            injection_items (list[dict[str, Any]] | None, optional): Injection items for modifying an existing PDF.
+                Identifier values must be JSON-serializable or UTF-8 bytes, which are identified as their decoded text.
 
         Raises:
             ValueError: If the font color is invalid or the injection items are not provided as a list of dictionaries.
@@ -130,6 +135,14 @@ def _build_identifier(self) -> ComponentIdentifier:
         if self._existing_pdf_path:
             existing_pdf_path = str(self._existing_pdf_path)
 
+        injection_items_hash = None
+        if self._injection_items:
+            injection_items_hash = hashlib.sha256(
+                json.dumps(self._injection_items, sort_keys=True, default=self._serialize_injection_value).encode(
+                    "utf-8"
+                )
+            ).hexdigest()[:16]
+
         return self._create_identifier(
             params={
                 "font_type": self._font_type,
@@ -139,6 +152,11 @@ def _build_identifier(self) -> ComponentIdentifier:
                 "prompt_template_hash": template_hash,
                 "existing_pdf_path": existing_pdf_path,
                 "font_color": list(self._font_color) if self._font_color is not None else None,
+                # Layout and injection settings join the identity only when they are set, so the
+                # default identity keeps its existing hash.
+                "column_width": self._column_width or None,
+                "row_height": self._row_height if self._row_height != _DEFAULT_ROW_HEIGHT else None,
+                "injection_items_hash": injection_items_hash,
             }
         )
 
@@ -461,3 +479,22 @@ async def _serialize_pdf_async(self, pdf_bytes: bytes, content: str) -> DataType
         )
         await pdf_serializer.save_data_async(pdf_bytes, output_filename=output_filename)
         return pdf_serializer
+
+    @staticmethod
+    def _serialize_injection_value(value: object) -> str:
+        """
+        Normalize byte text consistently with ReportLab for identifier serialization.
+
+        Args:
+            value (object): A value not handled by the JSON encoder.
+
+        Returns:
+            str: The decoded UTF-8 text.
+
+        Raises:
+            TypeError: If the value is not bytes.
+            UnicodeDecodeError: If the bytes are not valid UTF-8.
+        """
+        if isinstance(value, bytes):
+            return value.decode("utf-8")
+        raise TypeError(f"Unsupported injection item value type: {type(value).__name__}")

pyrit/converter/qr_code_converter.py

@@ -67,12 +67,22 @@ def _build_identifier(self) -> ComponentIdentifier:
         Returns:
             ComponentIdentifier: The identifier for this converter.
         """
+        # The data, finder, and border colors default to the dark or light color. They join the
+        # identity only when they differ from it, so equivalent configurations, including the
+        # default one, keep their existing hash.
         return self._create_identifier(
             params={
                 "scale": self._scale,
                 "border": self._border,
                 "dark_color": self._dark_color,
                 "light_color": self._light_color,
+                "data_dark_color": self._data_dark_color if self._data_dark_color != self._dark_color else None,
+                "data_light_color": self._data_light_color if self._data_light_color != self._light_color else None,
+                "finder_dark_color": self._finder_dark_color if self._finder_dark_color != self._dark_color else None,
+                "finder_light_color": (
+                    self._finder_light_color if self._finder_light_color != self._light_color else None
+                ),
+                "border_color": self._border_color if self._border_color != self._light_color else None,
             }
         )
 

tests/unit/converter/test_pdf_converter.py

@@ -16,6 +16,7 @@
 from pyrit.converter import ConverterResult, PDFConverter
 from pyrit.memory import DataTypeSerializer
 from pyrit.models import SeedPrompt
+from pyrit.registry import ConverterRegistry
 
 
 @pytest.mark.usefixtures("patch_central_database")
@@ -648,6 +649,84 @@ def test_identifier_distinguishes_font_color() -> None:
     assert len({default.hash, red.hash, white.hash}) == 3
 
 
+@pytest.mark.usefixtures("patch_central_database")
+def test_identifier_distinguishes_layout() -> None:
+    default = PDFConverter().get_identifier()
+    narrow = PDFConverter(column_width=80).get_identifier()
+    tall = PDFConverter(row_height=20).get_identifier()
+    auto_height = PDFConverter(row_height=0).get_identifier()
+
+    assert "column_width" not in default.params
+    assert "row_height" not in default.params
+    assert narrow.params["column_width"] == 80
+    assert tall.params["row_height"] == 20
+    assert auto_height.params["row_height"] == 0
+    assert len({default.hash, narrow.hash, tall.hash, auto_height.hash}) == 4
+
+
+@pytest.mark.usefixtures("patch_central_database")
+def test_identifier_distinguishes_injection_items(mock_pdf_path: Path) -> None:
+    def injecting(text: str) -> PDFConverter:
+        return PDFConverter(existing_pdf=mock_pdf_path, injection_items=[{"page": 0, "x": 50, "y": 700, "text": text}])
+
+    first = injecting("first payload")
+    second = injecting("second payload")
+
+    assert "injection_items_hash" not in PDFConverter(existing_pdf=mock_pdf_path).get_identifier().params
+    assert first.get_identifier().hash != second.get_identifier().hash
+    assert injecting("first payload").get_identifier().hash == first.get_identifier().hash
+
+    # Both configurations get their own default registry name instead of colliding.
+    instances = ConverterRegistry().instances
+    instances.register(first)
+    instances.register(second)
+    assert instances.get(second.get_identifier().unique_name) is second
+
+
+@pytest.mark.usefixtures("patch_central_database")
+@pytest.mark.parametrize("text", ["hello", "caf\u00e9"])
+async def test_identifier_normalizes_injected_byte_text_async(*, mock_pdf_path: Path, text: str) -> None:
+    byte_text = text.encode("utf-8")
+    converters = [
+        PDFConverter(existing_pdf=mock_pdf_path, injection_items=[{"text": value}])
+        for value in (byte_text, text, str(byte_text))
+    ]
+    identifiers = [converter.get_identifier() for converter in converters]
+
+    assert identifiers[0].hash == identifiers[1].hash
+    assert identifiers[0].hash != identifiers[2].hash
+
+    instances = ConverterRegistry().instances
+    instances.register(converters[0])
+    instances.register(converters[2])
+    assert instances.get(identifiers[2].unique_name) is converters[2]
+
+    for converter, expected in zip(converters, (text, text, str(byte_text)), strict=True):
+        result = await converter.convert_async(prompt="")
+        assert PdfReader(result.output_text).pages[0].extract_text() == f"Page 1 content\n{expected}\n"
+
+
+@pytest.mark.usefixtures("patch_central_database")
+def test_identifier_rejects_unsupported_injection_values() -> None:
+    converter = PDFConverter(injection_items=[{"text": object()}])
+
+    with pytest.raises(TypeError, match="Unsupported injection item value type: object"):
+        converter.get_identifier()
+
+
+@pytest.mark.usefixtures("patch_central_database")
+def test_identifier_rejects_invalid_utf8_injection_text() -> None:
+    converter = PDFConverter(injection_items=[{"text": b"\xff"}])
+
+    with pytest.raises(UnicodeDecodeError):
+        converter.get_identifier()
+
+
+@pytest.mark.usefixtures("patch_central_database")
+def test_identifier_keeps_default_hash() -> None:
+    assert PDFConverter().get_identifier().hash == "a05b4cf1482efac3adbc73fd088af40021f1ec3e8f08da575101410595aacf96"
+
+
 @pytest.mark.usefixtures("patch_central_database")
 @pytest.mark.parametrize("font_color", [(256, 0, 0), (0, 0), [0, 0, 0]])
 def test_init_rejects_invalid_font_color(font_color: Any) -> None:

tests/unit/converter/test_qr_code_converter.py

@@ -12,6 +12,7 @@
 
 from pyrit.converter import QRCodeConverter
 from pyrit.memory import DataTypeSerializer
+from pyrit.registry import ConverterRegistry
 
 
 def test_qr_code_converter_initialization():
@@ -98,3 +99,44 @@ def test_text_image_converter_input_supported():
     converter = QRCodeConverter()
     assert converter.input_supported("text") is True
     assert converter.input_supported("image_path") is False
+
+
+@pytest.mark.parametrize(
+    ("color_param", "converter"),
+    [
+        pytest.param("data_dark_color", QRCodeConverter(data_dark_color=(255, 0, 0)), id="data_dark_color"),
+        pytest.param("data_light_color", QRCodeConverter(data_light_color=(255, 0, 0)), id="data_light_color"),
+        pytest.param("finder_dark_color", QRCodeConverter(finder_dark_color=(255, 0, 0)), id="finder_dark_color"),
+        pytest.param("finder_light_color", QRCodeConverter(finder_light_color=(255, 0, 0)), id="finder_light_color"),
+        pytest.param("border_color", QRCodeConverter(border_color=(255, 0, 0)), id="border_color"),
+    ],
+)
+def test_qr_code_identifier_distinguishes_module_colors(color_param: str, converter: QRCodeConverter) -> None:
+    default = QRCodeConverter()
+    assert converter.get_identifier().params[color_param] == [255, 0, 0]
+    assert converter.get_identifier().hash != default.get_identifier().hash
+
+    # Both configurations get their own default registry name instead of colliding.
+    instances = ConverterRegistry().instances
+    instances.register(default)
+    instances.register(converter)
+    assert instances.get(converter.get_identifier().unique_name) is converter
+
+
+def test_qr_code_identifier_ignores_colors_equal_to_their_fallback() -> None:
+    dark, light = (10, 20, 30), (200, 210, 220)
+    implicit = QRCodeConverter(dark_color=dark, light_color=light)
+    explicit = QRCodeConverter(
+        dark_color=dark,
+        light_color=light,
+        data_dark_color=dark,
+        data_light_color=light,
+        finder_dark_color=dark,
+        finder_light_color=light,
+        border_color=light,
+    )
+    assert explicit.get_identifier().hash == implicit.get_identifier().hash
+
+
+def test_qr_code_identifier_keeps_default_hash() -> None:
+    assert QRCodeConverter().get_identifier().hash == "4466678813b329b72a65ac7f1985ffbf60e1f29da83af759b59720eea274bbe4"