Skip to content

Commit b1e24ac

Browse files
marcusgreenmarcusgreen
authored
Merge pull request #56 from bycs-lp/MBS-10533-Escape_backslashes_in_response_for_properly_JSON_parsing
MBS-10533: Escape backslashes in response for properly json parsing
2 parents b661867 + 5641796 commit b1e24ac

File tree

2 files changed

+68
-16
lines changed

2 files changed

+68
-16
lines changed

‎question.php‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -383,7 +383,7 @@ public function process_feedback(string $feedback) {
383383
$contentobject->marks = null;
384384
}
385385
$disclaimer = get_config('qtype_aitext', 'disclaimer');
386-
$contentobject->feedback = format_text($contentobject->feedback, FORMAT_MARKDOWN);
386+
$contentobject->feedback = format_text($contentobject->feedback, FORMAT_MARKDOWN, ['para' => false]);
387387
$contentobject->feedback .= ' ' . $this->llm_translate($disclaimer);
388388

389389
return $contentobject;
@@ -417,6 +417,23 @@ private function extract_single_json_object(string $text): ?stdClass {
417417
}
418418
}
419419
if ($json) {
420+
// Sometimes, the external LLM will return bad JSON (with not properly escaped backslashes, for example in a LaTeX
421+
// formula). The returned JSON then contains something like "... \( K_\alpha \) ...", which is invalid JSON.
422+
// However, we cannot just blindly escape all backslashes, because that would also mess up valid JSON sequences like
423+
// "\n" or "\t" and we cannot know if these are intended well escaped backslashes or the LLM just messed up and forgot
424+
// to escape.
425+
// So we are trying to parse LaTeX-like sequences explicitely and only inside them add an extra backslash to each
426+
// backslash.
427+
$json = preg_replace_callback(
428+
'/\\\\\(.*?\\\\\)|\\\\\[.*?\\\\\]|\$\$.*?\$\$|\$[^$]+\$/s',
429+
function ($matches) {
430+
if (empty($matches[0])) {
431+
return $matches[0] ?? '';
432+
}
433+
return str_replace('\\', '\\\\', $matches[0]);
434+
},
435+
$json
436+
);
420437
$decoded = json_decode($json);
421438
if (json_last_error() === JSON_ERROR_NONE) {
422439
return $decoded;

‎tests/question_test.php‎

Lines changed: 50 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,7 @@ public function test_expert_mode(): void {
247247
* @covers ::process_feedback()
248248
* @dataProvider process_feedback_provider
249249
* @param string $json The JSON string generated by the LLM.
250-
* @param bool $exceptionexpected Whether the called function is expected to throw an exception.
250+
* @param bool $exceptionexpected If there is an exception expected during parsing.
251251
* @param string $expectedfeedback The expected feedback extracted from the JSON.
252252
* @param float $expectedmarks The expected marks extracted from the JSON.
253253
*/
@@ -266,15 +266,20 @@ public function test_process_feedback(
266266

267267
try {
268268
$processedfeedback = $aitext->process_feedback($json);
269-
$this->assertTrue($exceptionexpected);
270-
} catch (Exception) {
271-
$this->assertFalse($exceptionexpected);
272-
return;
269+
} catch (Exception $e) {
270+
if ($exceptionexpected) {
271+
$this->assertTrue(true);
272+
return;
273+
} else {
274+
$this->fail('Unexpected exception thrown: ' . $e->getMessage());
275+
}
273276
}
274277
$this->assertIsObject($processedfeedback);
278+
// The process_feedback function is implemented in a way that in case of a parsing failure,
279+
// it as a fallback returns the original JSON string as feedback and 0 marks.
275280
$this->assertEquals(
276-
$processedfeedback->feedback,
277-
format_text($expectedfeedback, FORMAT_MARKDOWN) . ' (example disclaimer)'
281+
format_text($expectedfeedback, FORMAT_MARKDOWN) . ' (example disclaimer)',
282+
$processedfeedback->feedback
278283
);
279284
$this->assertEquals($processedfeedback->marks, $expectedmarks);
280285
}
@@ -288,57 +293,87 @@ public function test_process_feedback(
288293
*/
289294
public static function process_feedback_provider(): array {
290295
return [
296+
// If parsing works or not can be seen if the 'expectedfeedback' value is equal to the 'json' value.
297+
// If that's the case the parsing failed. Otherwise, the feedback and marks could be extracted properly.
291298
'valid_json' => [
292299
'json' => '{"feedback": "Good job", "marks": 0}',
293-
'exceptionexpected' => true,
300+
'exceptionexpected' => false,
294301
'expectedfeedback' => 'Good job',
295302
'expectedmarks' => 0,
296303
],
297304
'broken_json' => [
298305
'json' => '{"feedback": "Good job", "marks": 0',
299306
'exceptionexpected' => false,
300-
'expectedfeedback' => 'Good job',
307+
'expectedfeedback' => '{"feedback": "Good job", "marks": 0',
301308
'expectedmarks' => 0,
302309
],
303310
'valid_json_markdown_formatted' => [
304311
// @codingStandardsIgnoreLine moodle.Strings.ForbiddenStrings.Found
305312
'json' => '```json{"feedback": "Good job", "marks": 1}```',
306-
'exceptionexpected' => true,
313+
'exceptionexpected' => false,
307314
'expectedfeedback' => 'Good job',
308315
'expectedmarks' => 1,
309316
],
310317
'valid_json_with_text_around' => [
311318
'json' => 'Here is the feedback: {"feedback": "Well done", "marks": 0.5} Thank you!',
312-
'exceptionexpected' => true,
319+
'exceptionexpected' => false,
313320
'expectedfeedback' => 'Well done',
314321
'expectedmarks' => 0.5,
315322
],
316323
'valid_json_with_wrapped_html_tags' => [
317324
'json' => '<p>{"feedback": "Well done", "marks": 0.5} Thank you!</p>',
318-
'exceptionexpected' => true,
325+
'exceptionexpected' => false,
319326
'expectedfeedback' => 'Well done',
320327
'expectedmarks' => 0.5,
321328
],
322329
'valid_json_with_code' => [
323330
'json' => '{"feedback": "The code has a syntax error: the opening brace '
324331
. '\'{\' after the function signature is missing.", "marks": 0.5}',
325-
'exceptionexpected' => true,
332+
'exceptionexpected' => false,
326333
'expectedfeedback' => 'The code has a syntax error: the opening brace '
327334
. '\'{\' after the function signature is missing.',
328335
'expectedmarks' => 0.5,
329336
],
330337
'not_a_json_at_all' => [
331338
'json' => 'Not a json string',
332-
'exceptionexpected' => true,
339+
'exceptionexpected' => false,
333340
'expectedfeedback' => 'Not a json string',
334341
'expectedmarks' => 0,
335342
],
336343
'empty_json' => [
337344
'json' => '',
338-
'exceptionexpected' => false,
345+
'exceptionexpected' => true,
339346
'expectedfeedback' => '',
340347
'expectedmarks' => 0,
341348
],
349+
'json_with_backslashes' => [
350+
// This JSON is real answer from an LLM.
351+
// @codingStandardsIgnoreLine moodle.Strings.ForbiddenStrings.Found
352+
'json' => '```json { "feedback": "Die Antwort des Schülers zeigt ein grundlegendes Verständnis für das ' .
353+
'Röntgenspektrum und die Entstehung des Bremsspektrums sowie des charakteristischen Spektrums. Es wird ' .
354+
'korrekt beschrieben, dass das Bremsspektrum durch das Bremsen der Elektronen entsteht und von der ' .
355+
'Beschleunigungsspannung abhängt. Auch die materialabhängige Entstehung des charakteristischen Spektrums ' .
356+
'wird erwähnt und teilweise richtig interpretiert. Allerdings fehlen wesentliche physikalische Inhalte: 1. ' .
357+
'Es wird nicht erklärt, dass die Bremsstrahlung ein kontinuierliches Spektrum darstellt. ' .
358+
'2. Der Unterschied zwischen \(K_\alpha\)- und \(K_\beta\)-Linien des Röntgenspektrums wird nicht ' .
359+
'angesprochen. 3. Der Zusammenhang zwischen der Kernladungszahl und den Frequenzen \(K_\alpha\)-Linien ' .
360+
'(Moseleys Gesetz) ist ungenau erläutert und könnte präziser beschrieben werden. Insgesamt ist die ' .
361+
'Antwort gut, aber es sind noch einige physikalische Details notwendig, um die volle Punktzahl zu ' .
362+
'erreichen. Gegeben: 7/10 Punkte.", "marks": 7 } ```',
363+
'exceptionexpected' => false,
364+
'expectedfeedback' => 'Die Antwort des Schülers zeigt ein grundlegendes Verständnis für das ' .
365+
'Röntgenspektrum und die Entstehung des Bremsspektrums sowie des charakteristischen Spektrums. Es wird ' .
366+
'korrekt beschrieben, dass das Bremsspektrum durch das Bremsen der Elektronen entsteht und von der ' .
367+
'Beschleunigungsspannung abhängt. Auch die materialabhängige Entstehung des charakteristischen Spektrums ' .
368+
'wird erwähnt und teilweise richtig interpretiert. Allerdings fehlen wesentliche physikalische Inhalte: 1. ' .
369+
'Es wird nicht erklärt, dass die Bremsstrahlung ein kontinuierliches Spektrum darstellt. ' .
370+
'2. Der Unterschied zwischen \(K_\alpha\)- und \(K_\beta\)-Linien des Röntgenspektrums wird nicht ' .
371+
'angesprochen. 3. Der Zusammenhang zwischen der Kernladungszahl und den Frequenzen \(K_\alpha\)-Linien ' .
372+
'(Moseleys Gesetz) ist ungenau erläutert und könnte präziser beschrieben werden. Insgesamt ist die ' .
373+
'Antwort gut, aber es sind noch einige physikalische Details notwendig, um die volle Punktzahl zu ' .
374+
'erreichen. Gegeben: 7/10 Punkte.',
375+
'expectedmarks' => 7,
376+
],
342377
];
343378
}
344379

0 commit comments

Comments
 (0)