Stop HTML-escaping scorer values in the ragas prompts - #227
Open
fei (feiiiiii5) wants to merge 1 commit into
Open
fei (feiiiiii5) wants to merge 1 commit into
fei (feiiiiii5) wants to merge 1 commit into
Conversation
`js/render-messages.ts` renders values verbatim and pins that with a test
("should never HTML-escape values, regardless of mustache syntax"), but
`js/ragas.ts` still calls `mustache.render` with the default escaper at all
eight prompt sites. The same value therefore reaches the judge model
untouched through the LLMClassifier scorers and entity-encoded through the
ragas ones:
text: Is 3 < 5 & 2 > 1? "q"
For anything scraped from HTML, code or maths that is a corrupted prompt, and
the recorded trace no longer reproduces the input it came from.
Export the existing `escapeValue` and pass it at the eight call sites, which
is what render-messages already does.
One behaviour change beyond escaping: `{{statements}}` is a `string[]`, so
with the verbatim escaper it renders as JSON rather than comma-joined. That
moves ragas towards the Python implementation, which renders the list
directly; if you would rather keep the old comma-joined form, that is the one
site to special-case.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The ragas scorers HTML-escape the values they interpolate, so the judge reads a corrupted prompt:
js/render-messages.tsrenders values verbatim and pins that with a test — "should never HTML-escape values, regardless of mustache syntax" — butjs/ragas.tsstill callsmustache.renderwith the default escaper at all eight prompt sites. So the same value reaches the judge untouched through theLLMClassifierscorers (Factuality,ClosedQA, …) and entity-encoded through the ragas ones. For anything containing<,>,&,",'or/— scraped HTML, source code, maths — the prompt no longer matches the input, and a recorded trace no longer reproduces what was evaluated. Tag names read as literal text can also shift entity extraction.The fix exports the
escapeValuehelper that already exists inrender-messages.tsand passes it at the eight call sites, so both paths render the same way.Test:
pnpm run test --run js/ragas.test.ts -t "Ragas prompt rendering"fails on9546b28for the three scorers above and passes here (4 passed / 6 skipped); the new tests capture the outgoing user message and assert on it. Suite: 69 passed before, 73 after, with the same 8 pre-existing failures either way — all of themMissing credentials … set the OPENAI_API_KEY, which CI supplies from secrets.pnpm run build(tsup, CJS + ESM + DTS) passes, and pre-commit on the three changed files passes. There is nolintortypecheckscript in this repo;npx tsc --noEmitreports one pre-existing error injs/render-messages.test.tsthat this change does not touch.Two things to push back on if you disagree:
py/autoevals/ragas.pyrenders withchevron, which HTML-escapes by default (verified on chevron 0.14.0, the pinned version), so today both languages escape and the bug is symmetric. The argument for changing JS is the repository's own test and the escape objects to stringified in autoevals #110 commit that fixedrender-messages, not cross-language parity. If you would rather have parity, the alternative is to bypass chevron's escaping in Python ({{&text}}or an escape hook) — happy to do that as a follow-up.{{statements}}changes shape. It is astring[], so with the verbatim escaper it renders as JSON instead of comma-joined. That moves ragas towards Python (which renders the list directly), but it is a real prompt change at that one site; say the word and I will special-case it.