Claude commited on
Commit
91c3a3b
·
unverified ·
1 Parent(s): 20af117

refactor(sprint-S9): simplifier les défenses anti-régression prompt — -77 lignes

Browse files

Revert partiel du commit 20af117 sur questionnement légitime :
"c'était pas plus simple avant ?". Les 3 défenses ajoutées hier
étaient du bricolage défensif après-coup — heuristiques ad-hoc,
checks redondants, contrats cassés.

Garde-fous gardés
-----------------

1. **OCRLLMPipelineConfig.__post_init__** : check minimal —
``prompt_template`` non-vide sans aucune accolade → ``ValueError``.
Une ligne, sémantique claire : un prompt LLM substituable a
forcément au moins une accolade. Capture le cas
``correction_*.txt`` injecté tel quel.

2. **Test d'intégration end-to-end**
(``test_s9_prompt_loading_defenses.py``) : mock du LLM qui
capture le prompt envoyé via le flux factory web → pipeline
→ LLM. Le seul filet qui prouve réellement que ``thou hast``
et ``Early Modern English`` arrivent au LLM (pas le filename).
C'est ce filet qui aurait pris le bug initialement.

Bricolage retiré
----------------

1. ``OCRLLMPipelineConfig`` : suppression de l'heuristique
"ressemble à un filename" (``.endswith(".txt")`` + pas de
newline + < 256 chars). Ad-hoc, n'apporte rien — le check
``"{" not in template`` couvre déjà le cas.

2. ``_substitute_prompt_variables`` : suppression du check
"pas de placeholder → raise ValueError". Redondant avec
le check au niveau ``OCRLLMPipelineConfig``, et a cassé
un test légitime du contrat strict ``str.format`` (qui
doit lever ``KeyError`` sur variable inconnue, pas
``ValueError`` sur absence de placeholder connu).

3. ``BaseLLMAdapter.complete`` : retour à ``if param_prompt is
not None:`` (au lieu de ``if param_prompt:``). Le cas
``param_prompt = ""`` est marginal et géré ailleurs ; le
contrat original "None signifie absent" est respecté.

Tests
-----

- ``test_rewrite_format_unknown_variable_raises`` : revert à
``KeyError`` attendu (contrat strict format d'origine).
- Classe ``TestSubstituteRejectsTemplateWithoutPlaceholder``
supprimée (défense retirée).
- ``test_filename_passed_as_template_rejected`` : message
attendu simplifié (``accolade|filename`` au lieu du diagnostic
multilignes avec heuristique).
- Test ``test_string_without_brace_rejected`` ajouté pour
vérifier que la défense minimale couvre aussi les strings
non-filename sans placeholder.

Le bug initial reste impossible à reproduire grâce à :
- ``_load_prompt_content`` côté factory web (fix racine, commit
``f7f7ea8``).
- Validation minimale au constructeur ``OCRLLMPipelineConfig``.
- Test d'intégration qui capture le prompt réel envoyé au LLM.

Régression : 4578 passed, 0 failed, ruff clean.

picarones/adapters/llm/base.py CHANGED
@@ -162,21 +162,6 @@ def _substitute_prompt_variables(
162
  .replace("{ocr_output}", text)
163
  .replace("{image_b64}", image_b64 or "")
164
  )
165
- # Convention rewrite : ``{text}`` est l'unique placeholder.
166
- # Défense en profondeur (Sprint S9) : si la string n'a aucun
167
- # placeholder de substitution, ``template.format(text=text)``
168
- # retournerait la string inchangée sans erreur — ce qui faisait
169
- # passer un filename (``correction_*.txt``) au LLM en prod.
170
- # On lève maintenant explicitement : un template sans
171
- # placeholder est sémantiquement vide (le LLM ignorerait l'OCR).
172
- if "{text}" not in template:
173
- raise ValueError(
174
- "Prompt template invalide : aucun placeholder "
175
- "``{ocr_output}``, ``{text}`` ou ``{image_b64}`` "
176
- "trouvé. Le LLM recevrait une string fixe. "
177
- "Probable cause : un filename a été injecté au "
178
- "lieu du contenu du fichier prompt.",
179
- )
180
  return template.format(text=text)
181
 
182
 
@@ -449,13 +434,8 @@ class BaseLLMAdapter(ABC):
449
  # de l'adapter — pattern historique).
450
  # 3. Prompt par langue selon ``self.config["lang"]``.
451
  # 4. Fallback FR.
452
- # ``""`` est traité comme "pas fourni" (au même titre que
453
- # ``None``) — on tombe sur le défaut de l'adapter. Avant
454
- # Sprint S9, ``""`` était propagé jusqu'à
455
- # ``_substitute_prompt_variables`` qui retournait ``""``
456
- # silencieusement, laissant le LLM voir une string vide.
457
  param_prompt = params.get("prompt_template") if params else None
458
- if param_prompt:
459
  prompt_template = param_prompt
460
  else:
461
  custom_prompt = self.config.get("correction_prompt")
 
162
  .replace("{ocr_output}", text)
163
  .replace("{image_b64}", image_b64 or "")
164
  )
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
165
  return template.format(text=text)
166
 
167
 
 
434
  # de l'adapter — pattern historique).
435
  # 3. Prompt par langue selon ``self.config["lang"]``.
436
  # 4. Fallback FR.
 
 
 
 
 
437
  param_prompt = params.get("prompt_template") if params else None
438
+ if param_prompt is not None:
439
  prompt_template = param_prompt
440
  else:
441
  custom_prompt = self.config.get("correction_prompt")
picarones/pipeline/llm_pipeline_config.py CHANGED
@@ -90,41 +90,22 @@ class OCRLLMPipelineConfig:
90
  "OCRLLMPipelineConfig : mode 'zero_shot' ne doit pas "
91
  "avoir d'``ocr_adapter`` (le VLM lit l'image directement).",
92
  )
93
- # Garde-fou Sprint S9 — anti-régression du bug "filename passé
94
- # à la place du contenu" (cf. ``tests/web/test_s9_prompt_loading.py``).
95
- # ``prompt_template`` est censé être le contenu BRUT du prompt
96
- # (déjà lu depuis disque par le caller), pas un identifiant de
97
- # ressource. Un template sans aucun placeholder de substitution
98
- # est sémantiquement invalide : le LLM recevrait une string fixe
99
- # qui ignore le texte OCR et halluciinerait une réponse plausible.
100
- # Le filename ``correction_*.txt`` était précisément ce cas.
101
- if self.prompt_template:
102
- has_placeholder = any(
103
- marker in self.prompt_template
104
- for marker in ("{ocr_output}", "{text}", "{image_b64}")
 
 
 
105
  )
106
- if not has_placeholder:
107
- # Heuristique pour aider au diagnostic : si la string
108
- # ressemble à un filename, on le dit explicitement.
109
- looks_like_filename = (
110
- self.prompt_template.endswith(".txt")
111
- and "\n" not in self.prompt_template
112
- and len(self.prompt_template) < 256
113
- )
114
- hint = (
115
- " (la string ressemble à un nom de fichier — "
116
- "as-tu oublié de charger le contenu via "
117
- "``Path(prompts_dir / filename).read_text()`` "
118
- "avant de l'injecter ?)"
119
- if looks_like_filename else ""
120
- )
121
- raise ValueError(
122
- "OCRLLMPipelineConfig : ``prompt_template`` ne "
123
- "contient aucun placeholder substituable "
124
- "(``{ocr_output}``, ``{text}`` ou ``{image_b64}``). "
125
- "Le LLM recevrait une string fixe et ignorerait "
126
- f"le texte OCR.{hint}",
127
- )
128
 
129
  @property
130
  def name(self) -> str:
 
90
  "OCRLLMPipelineConfig : mode 'zero_shot' ne doit pas "
91
  "avoir d'``ocr_adapter`` (le VLM lit l'image directement).",
92
  )
93
+ # Sprint S9 — garde-fou minimal contre l'oubli de chargement
94
+ # du contenu : un template non-vide sans aucune accolade ne
95
+ # peut pas être un prompt LLM substituable. Capture le cas
96
+ # ``correction_*.txt`` passé tel quel comme template (cf.
97
+ # tests/integration/test_s9_prompt_loading_defenses.py pour
98
+ # le contexte du bug).
99
+ if self.prompt_template and "{" not in self.prompt_template:
100
+ raise ValueError(
101
+ "OCRLLMPipelineConfig : ``prompt_template`` ne contient "
102
+ "aucune accolade — un prompt LLM substituable a au "
103
+ "moins un placeholder ``{ocr_output}``, ``{text}`` ou "
104
+ "``{image_b64}``. Probable cause : un filename a été "
105
+ "injecté au lieu du contenu (charge via "
106
+ "``Path(prompts_dir / filename).read_text()`` avant "
107
+ "d'instancier).",
108
  )
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
109
 
110
  @property
111
  def name(self) -> str:
tests/adapters/llm/test_s8_base_utilities.py CHANGED
@@ -114,14 +114,13 @@ class TestSubstitutePromptVariables:
114
  )
115
  assert result == "x|"
116
 
117
- def test_template_without_known_placeholder_raises(self) -> None:
118
- """Sprint S9 la défense au niveau substitution refuse
119
- tout template qui n'a aucun placeholder connu
120
- (``{ocr_output}``, ``{text}``, ``{image_b64}``). Avant
121
- S9, ce test attendait un ``KeyError`` de ``str.format`` ;
122
- le contrat est maintenant strict en amont : pas de
123
- placeholder = template invalide."""
124
- with pytest.raises(ValueError, match="placeholder|filename"):
125
  _substitute_prompt_variables(
126
  "{unknown_var}", text="x", image_b64=None,
127
  )
 
114
  )
115
  assert result == "x|"
116
 
117
+ def test_rewrite_format_unknown_variable_raises(self) -> None:
118
+ """Le mode rewrite passe par ``str.format`` variable
119
+ inconnue lève ``KeyError`` (comportement strict d'origine
120
+ documenté). Le filtrage "filename → contenu" se fait au
121
+ niveau ``OCRLLMPipelineConfig.__post_init__`` (Sprint S9),
122
+ pas ici."""
123
+ with pytest.raises(KeyError):
 
124
  _substitute_prompt_variables(
125
  "{unknown_var}", text="x", image_b64=None,
126
  )
tests/integration/test_s9_prompt_loading_defenses.py CHANGED
@@ -1,24 +1,20 @@
1
- """Sprint S9 — défenses anti-régression pour la classe de bugs
2
- "filename passé à la place du contenu".
3
 
4
- Trois niveaux de garde-fou indépendants :
5
 
6
  1. **Contrat ``OCRLLMPipelineConfig.__post_init__``** : refuse un
7
- ``prompt_template`` non-vide sans placeholder substituable.
8
- Catch le bug au point de violation du contrat (instanciation).
9
-
10
- 2. **Substitution ``_substitute_prompt_variables``** : refuse de
11
- "substituer" un template sans placeholder. Defense en
12
- profondeur si quelqu'un court-circuite le constructeur (par ex.
13
- via ``object.__setattr__`` ou un fichier frozen dataclass
14
- patché).
15
-
16
- 3. **Test d'intégration** : mock du LLM qui capture le prompt
17
- réellement envoyé, exécution d'un pipeline réel, assertion
18
- que le prompt capturé est le **contenu** du fichier prompt
19
- (pas le filename). C'est le filet que la suite de tests
20
- pré-S9 n'avait pas — chaque couche était testée en isolation,
21
- personne ne vérifiait le bout-en-bout du flux post-correction.
22
  """
23
 
24
  from __future__ import annotations
@@ -45,33 +41,30 @@ class TestPipelineConfigRefusesInvalidTemplate:
45
  OCRLLMPipelineConfig,
46
  )
47
 
48
- with pytest.raises(ValueError) as exc_info:
49
  OCRLLMPipelineConfig(
50
  ocr_adapter=TesseractAdapter(lang="fra"),
51
  llm_adapter=OpenAIAdapter(model="gpt-4o"),
52
  mode="text_only",
53
  prompt_template="correction_early_modern_english.txt",
54
  )
55
- msg = str(exc_info.value)
56
- assert "placeholder" in msg.lower()
57
- # Le message doit pointer le diagnostic : ressemble à un filename.
58
- assert "fichier" in msg.lower() or "filename" in msg.lower()
59
-
60
- def test_random_string_without_placeholder_rejected(self) -> None:
61
- """Pas qu'un filename — toute string sans placeholder est
62
- refusée car sémantiquement vide pour la post-correction."""
63
  from picarones.adapters.llm.openai_adapter import OpenAIAdapter
64
  from picarones.adapters.ocr.tesseract import TesseractAdapter
65
  from picarones.pipeline.llm_pipeline_config import (
66
  OCRLLMPipelineConfig,
67
  )
68
 
69
- with pytest.raises(ValueError, match="placeholder"):
70
  OCRLLMPipelineConfig(
71
  ocr_adapter=TesseractAdapter(lang="fra"),
72
  llm_adapter=OpenAIAdapter(model="gpt-4o"),
73
  mode="text_only",
74
- prompt_template="Corrige ce texte.", # pas de placeholder
75
  )
76
 
77
  def test_empty_template_still_allowed(self) -> None:
@@ -119,49 +112,19 @@ class TestPipelineConfigRefusesInvalidTemplate:
119
 
120
 
121
  # ──────────────────────────────────────────────────────────────────────
122
- # Niveau 2 — substitution helper
123
- # ──────────────────────────────────────────────────────────────────────
124
-
125
-
126
- class TestSubstituteRejectsTemplateWithoutPlaceholder:
127
- """``_substitute_prompt_variables`` est la dernière ligne de
128
- défense avant que le template atteigne le LLM. S'il arrive
129
- sans placeholder, on lève."""
130
-
131
- def test_string_without_placeholder_raises(self) -> None:
132
- from picarones.adapters.llm.base import _substitute_prompt_variables
133
-
134
- with pytest.raises(ValueError, match="placeholder|filename"):
135
- _substitute_prompt_variables(
136
- "correction_medieval_french.txt",
137
- text="hello",
138
- image_b64=None,
139
- )
140
-
141
- def test_empty_string_returns_empty(self) -> None:
142
- """Edge case : empty template → format() retourne ""
143
- (pas un placeholder mais pas un bug non plus)."""
144
- from picarones.adapters.llm.base import _substitute_prompt_variables
145
-
146
- # ``""`` n'a pas ``{text}`` non plus → lève selon la nouvelle
147
- # défense. Documenté comme contrat : un template sans
148
- # placeholder n'est jamais valide pour la substitution.
149
- with pytest.raises(ValueError):
150
- _substitute_prompt_variables("", text="x", image_b64=None)
151
-
152
-
153
- # ──────────────────────────────────────────────────────────────────────
154
- # Niveau 3 — intégration LLM end-to-end (filet manquant pré-S9)
155
  # ──────────────────────────────────────────────────────────────────────
156
 
157
 
158
  class TestEndToEndPromptReachesLLM:
159
- """Le filet manquant : un test qui capture le prompt réel
160
- envoyé au LLM lors d'une post-correction, et vérifie qu'il
161
- contient bien le contenu chargé depuis disque (pas un
162
  filename, pas une string fixe).
163
 
164
- C'est exactement le test qui aurait pris le bug initialement.
 
 
165
  """
166
 
167
  def test_llm_receives_loaded_prompt_content(self, monkeypatch) -> None:
 
1
+ """Sprint S9 — garde-fous anti-régression pour le bug
2
+ "filename passé à la place du contenu" (post-correction LLM).
3
 
4
+ Deux niveaux de garde-fou :
5
 
6
  1. **Contrat ``OCRLLMPipelineConfig.__post_init__``** : refuse un
7
+ ``prompt_template`` non-vide sans aucune accolade. Check
8
+ minimal qui capture le cas ``correction_*.txt`` injecté tel
9
+ quel comme template.
10
+
11
+ 2. **Test d'intégration** : mock du LLM qui capture le prompt
12
+ réellement envoyé, exécution du factory web pipeline → LLM,
13
+ assertion que le prompt capturé est le **contenu** du fichier
14
+ prompt (pas le filename). C'est le filet manquant pré-S9
15
+ qui aurait pris le bug en amont — chaque couche était testée
16
+ en isolation, personne ne vérifiait le bout-en-bout du flux
17
+ post-correction.
 
 
 
 
18
  """
19
 
20
  from __future__ import annotations
 
41
  OCRLLMPipelineConfig,
42
  )
43
 
44
+ with pytest.raises(ValueError, match="accolade|filename"):
45
  OCRLLMPipelineConfig(
46
  ocr_adapter=TesseractAdapter(lang="fra"),
47
  llm_adapter=OpenAIAdapter(model="gpt-4o"),
48
  mode="text_only",
49
  prompt_template="correction_early_modern_english.txt",
50
  )
51
+
52
+ def test_string_without_brace_rejected(self) -> None:
53
+ """Toute string non-vide sans accolade est refusée pas
54
+ seulement les filenames. Le LLM recevrait une string fixe
55
+ qui ignore l'OCR."""
 
 
 
56
  from picarones.adapters.llm.openai_adapter import OpenAIAdapter
57
  from picarones.adapters.ocr.tesseract import TesseractAdapter
58
  from picarones.pipeline.llm_pipeline_config import (
59
  OCRLLMPipelineConfig,
60
  )
61
 
62
+ with pytest.raises(ValueError, match="accolade"):
63
  OCRLLMPipelineConfig(
64
  ocr_adapter=TesseractAdapter(lang="fra"),
65
  llm_adapter=OpenAIAdapter(model="gpt-4o"),
66
  mode="text_only",
67
+ prompt_template="Corrige ce texte.",
68
  )
69
 
70
  def test_empty_template_still_allowed(self) -> None:
 
112
 
113
 
114
  # ──────────────────────────────────────────────────────────────────────
115
+ # Niveau 2 — intégration LLM end-to-end (filet manquant pré-S9)
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
116
  # ──────────────────────────────────────────────────────────────────────
117
 
118
 
119
  class TestEndToEndPromptReachesLLM:
120
+ """Le filet manquant pré-S9 : un test qui capture le prompt
121
+ réel envoyé au LLM lors d'une post-correction, et vérifie
122
+ qu'il contient bien le contenu chargé depuis disque (pas un
123
  filename, pas une string fixe).
124
 
125
+ C'est exactement le test qui aurait pris le bug initialement
126
+ le restant des défenses est superflu tant que ce filet tourne
127
+ en CI.
128
  """
129
 
130
  def test_llm_receives_loaded_prompt_content(self, monkeypatch) -> None: