feat(assertions): add grader variable allowlists#9939
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9939 +/- ##
=======================================
Coverage 79.47% 79.48%
=======================================
Files 924 924
Lines 74138 74159 +21
Branches 23867 23874 +7
=======================================
+ Hits 58920 58944 +24
+ Misses 15218 15215 -3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![promptfoo-scanner[bot]](https://avatars.githubusercontent.com/in/2172945?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 All Clear
I reviewed the PR’s changes to model-graded assertions, focusing on the new graderVars allowlist and related prompt construction and schema updates. The modifications reduce data sent to LLM graders and do not introduce new tools, execution paths, or permissions. Based on the diff, I did not find any medium-or-higher LLM security vulnerabilities introduced by this PR.
Minimum severity threshold: 🟡 Medium | To re-scan after changes, comment @promptfoo-scanner
Learn more
There was a problem hiding this comment.
👍 All Clear
Reviewed the changes adding grader variable allowlists across multiple assertion handlers and the supporting schema/docs. The update narrows which test variables are sent to model graders and preserves explicit rubric substitutions. I did not find any new LLM security risks introduced by these changes.
Minimum severity threshold: 🟡 Medium | To re-scan after changes, comment @promptfoo-scanner
Learn more
There was a problem hiding this comment.
Pull request overview
Adds per-assertion graderVars allowlists to control which test variables are forwarded to model-based graders, reducing grader prompt/context size when tests include large “ambient” variables.
Changes:
- Extend
AssertionSchemawith optionalgraderVars: string[]. - Introduce
getGraderVars()and apply it across the relevant model-graded assertion handlers (andselect-best) to projecttest.vars/resolved vars before passing them to matchers/graders. - Add/adjust unit tests and documentation/schema to cover validation and end-to-end projection behavior (including ensuring explicit
valuetemplate substitutions happen before projection).
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/types/index.test.ts | Adds schema validation tests for graderVars (valid/invalid cases) and includes graderVars in a representative assertion parse. |
| test/assertions/trajectoryGoalSuccess.test.ts | Updates test to confirm projected grader vars are used while trajectory inputs remain intact. |
| test/assertions/runAssertion.test.ts | Adds end-to-end coverage ensuring explicit value references render before projection and excluded vars are not sent to the grader context/prompt. |
| test/assertions/modelGradedClosedQa.test.ts | Updates expectations to verify grader var projection while keeping closed-QA inputs correct. |
| test/assertions/llmRubric.test.ts | Adds coverage for omitted vs empty vs allowlisted graderVars behavior in llm-rubric. |
| test/assertions/contextRecall.test.ts | Verifies projected grader vars are passed without impacting resolved context handling. |
| test/assertions/contextPropagation.test.ts | Updates context propagation test for select-best to ensure projection occurs while preserving inputs/context. |
| test/assertions/agentRubric.test.ts | Verifies agent-rubric receives projected vars and preserves required rubric inputs. |
| src/types/index.ts | Adds graderVars to AssertionSchema. |
| src/assertions/utils.ts | Introduces getGraderVars() helper for projecting vars based on the assertion allowlist. |
| src/assertions/trajectory.ts | Applies getGraderVars() to the vars passed into the trajectory goal-success grader. |
| src/assertions/searchRubric.ts | Applies getGraderVars() when forwarding vars to matchesSearchRubric. |
| src/assertions/modelGradedClosedQa.ts | Applies getGraderVars() when forwarding vars to matchesClosedQa. |
| src/assertions/llmRubric.ts | Applies getGraderVars() when forwarding vars to matchesLlmRubric. |
| src/assertions/index.ts | Applies getGraderVars() to select-best comparisons and wires the helper import. |
| src/assertions/factuality.ts | Applies getGraderVars() when forwarding vars to matchesFactuality. |
| src/assertions/contextRecall.ts | Applies getGraderVars() when forwarding vars to matchesContextRecall. |
| src/assertions/contextFaithfulness.ts | Applies getGraderVars() when forwarding vars to matchesContextFaithfulness. |
| src/assertions/agentRubric.ts | Applies getGraderVars() when forwarding vars to matchesAgentRubric. |
| site/static/config-schema.json | Updates generated config schema to include graderVars for assertions. |
| site/docs/configuration/expected-outputs/model-graded/index.md | Documents graderVars semantics, examples, and supported assertion types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14321757b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return Object.fromEntries( | ||
| assertion.graderVars | ||
| .filter((name) => Object.hasOwn(vars, name)) | ||
| .map((name) => [name, vars[name]]), |
There was a problem hiding this comment.
When a test uses graderVars to allowlist a variable whose name matches a grader built-in such as output, rubric, criteria, completion, or context, this projection forwards that user variable unchanged; downstream graders merge it after the built-ins (for example { output, rubric, ...vars }), so the user variable replaces the actual model output or rubric in the rendered grading prompt. This contradicts the documented guarantee that built-in grader inputs are still included and can make the judge grade the wrong content; filter or namespace reserved names, or ensure built-ins win when merging.
Useful? React with 👍 / 👎.
mldangelo
left a comment
mldangelo
left a comment
There was a problem hiding this comment.
Review: graderVars allowlists
Good feature, well-tested where it's tested, and I verified it works end-to-end with the local build: a capturing grader provider confirmed that with graderVars set, an excluded large var reaches neither the rendered grading prompt, nor the grader provider's call context, nor (per code inspection) the remote-grading payload; allowlisted-but-absent names are dropped; and a test var named output cannot shadow the grader's real output. The value-rendered-before-projection behavior matches the docs, site/static/config-schema.json is an exact npm run jsonSchema:generate regeneration (no hand-edit drift), all 9 touched test files pass locally, and the hasOwnProperty guard is load-bearing (it keeps graderVars: ['toString'] from projecting prototype members). The earlier Codex bot comment about built-in shadowing is addressed by the GRADER_BUILTIN_VARS commit — but incompletely, which is the headline finding.
Blocking
-
Merge conflict with
main—src/assertions/utils.tsconflicts with main's js-yaml v5 migration (import yaml from 'js-yaml'→import { loadYaml } from '../util/yamlLoad', #9919). Verified withgit merge-tree: this PR cannot merge as-is, and a resolution that keeps the v4 default import will break the build on main (js-yaml v5 has no default export). Please mergeorigin/mainand adoptloadYaml. -
not-prefixed types bypass the built-in protection (inline atutils.ts:70) —GRADER_BUILTIN_VARS[assertion.type]is keyed by base type strings, but every supported type also has a validnot-variant. Reproduced end-to-end:type: not-llm-rubric+graderVars: ['output']+ a test var namedoutputputs<Output>USER_VAR_NAMED_OUTPUT</Output>into the grading prompt instead of the real model output — the exact wrong-text-graded failure the map exists to prevent, and a direct violation of the new doc guarantee ("Names reserved for an assertion's built-in grader inputs are ignored"). The same config with plainllm-rubricis correctly protected. Look up bybaseTypeand add anot-variant test.
Should fix
-
Custom
rubricPromptinteraction is undocumented and silently destructive (inline atllmRubric.ts:33) — the projected vars are what render a customrubricPrompt, so a template referencing an excluded var renders it as an empty string, no error, no warning. The docs describe thevaluecarve-out (rendered before projection) but not this opposite-direction behavior, andsite/docs/guides/llm-as-a-judge.md:546-550still promises "Any testvars" are available inrubricPrompt. -
Silent no-op on unsupported types (inline at
types/index.ts:734) — the schema acceptsgraderVarson every assertion type but only 10 honor it. The sharp edge:conversation-relevancedoes forward rawtest.varsinto its grader's rubric render (src/external/assertions/deepeval.ts:51-57→nunjucks.renderString(..., { messages, ...vars })), so a user who setsgraderVars: []there to keep a sensitive/large var away from the grader gets zero effect and zero warning — the exact exposure the feature exists to prevent. Either wire it in or warn on unsupported types.
Docs audit
The new section is accurate for the happy path, but the feature is essentially undiscoverable and three behaviors are unstated:
- No
graderVarsrow in either assertion-properties table:site/docs/configuration/reference.md:141-152andsite/docs/configuration/expected-outputs/index.md:46-57. Highest-ROI fix. - The new section should state: the custom-
rubricPrompteffect (#3), that unsupported types silently ignore it (#4), and where the per-type reserved names live — "Names reserved for an assertion's built-in grader inputs are ignored" gives users no way to find those names (several types never document them anywhere). - Option enumerations that omit it:
model-graded/index.md:99("you can setthreshold,provider, orrubricPrompt" — trajectory supportsgraderVarstoo),llm-rubric.md(flagship page),agent-rubric.md:74. - Minor: the supported list omits the
model-graded-factualityalias, and the motivation could mention the remote-grading/data-minimization benefit (the projection also governs what's sent to remote graders).
Design notes (non-blocking, worth a maintainer call before this pattern calcifies)
GRADER_BUILTIN_VARSis a hand-maintained copy of matcher-owned knowledge (inline atutils.ts:12). All 10 entries are accurate today (each verified against the matcher spreads), but the authoritative names live in the matchers'{ output, rubric, ...vars }expressions, and the drift test inutils.test.tscompares the map against a hardcoded copy of itself. A deeper shape: have matchers spread built-ins after projected user vars (asmatchesTrajectoryGoalSuccessalready does), which would delete the map and fix the inconsistency at the source.- Assertion-only granularity breaks the sibling pattern:
rubricPrompt/providerlive inGradingConfig(test.options), inherit viadefaultTest, andgetFinalTestfolds assertion-level overrides down.graderVarssupports none of that — a user with 8 model-graded assertions repeats it 8 times. Considertest.options.graderVarswith assertion-level override. - Nothing enforces adoption by the 11th grader — a future model-graded handler that forwards
test.varsraw silently skips projection. Computing the projection once intoAssertionParams(inline atindex.ts:860) shrinks that surface. - Pre-existing, not this PR's fault: without
graderVars, a test var namedoutputstill replaces the real model output in the grading prompt (I reproduced this on the control path). This PR's protection only engages whengraderVarsis set, which makes the same config safe or unsafe depending on an unrelated option. The matcher-side fix above would close it uniformly.
Findings I checked and am not raising
- A workflow finding claimed YAML
graderVars: nullcauses a runtimeTypeError— refuted:validateAssertionshard-fails at config load with a cleanAssertValidationErrorbefore any grading. - A claimed loss of plain-path (no-
graderVars) test coverage from retrofitting existing tests — mostly refuted: siblings preserve the pass-through path in every file exceptmodelGradedClosedQa.test.ts(covered cross-file; minor inline note). - Efficiency and reuse are clean: zero-cost fast path when
graderVarsis unset, projection isn't defeated downstream (callProviderWithContextoverridescontext.varswith the projected set), and no existing pick/projection helper was missed.
| return {}; | ||
| } | ||
|
|
||
| const builtins = GRADER_BUILTIN_VARS[assertion.type]; |
There was a problem hiding this comment.
P1 — not- prefixed assertion types bypass the built-in protection.
The lookup uses raw assertion.type, but every supported type also has a valid not- variant (NotPrefixedAssertionTypesSchema), and those dispatch to the same handlers via baseType. GRADER_BUILTIN_VARS['not-llm-rubric'] is undefined, so !builtins?.has(name) passes every allowlisted name.
Reproduced end-to-end with the local build: type: not-llm-rubric, graderVars: ['issue', 'output'], test var output: USER_VAR_NAMED_OUTPUT → the grading prompt contained <Output>\nUSER_VAR_NAMED_OUTPUT instead of the actual model output, so the (inverted) verdict grades the wrong text. The identical config with plain llm-rubric was correctly protected.
Fix: resolve the base type before the lookup (handlers already receive baseType in AssertionParams, or strip the not- prefix here), and add a not-llm-rubric case to the shadow-protection test.
| import cliState from '../cliState'; | ||
| import { importModule } from '../esm'; | ||
| import { type Assertion, type TestCase } from '../types/index'; | ||
| import { type Assertion, type TestCase, type VarValue } from '../types/index'; |
There was a problem hiding this comment.
Blocking (mechanical) — this file conflicts with origin/main.
Main migrated this import block to import { loadYaml } from '../util/yamlLoad' in the js-yaml v5 upgrade (#9919); js-yaml v5 has no default export, so a conflict resolution that keeps import yaml from 'js-yaml' will compile against the branch but break once merged. Please merge origin/main and switch any yaml.load call in this file to loadYaml.
| outputString, | ||
| test.options, | ||
| test.vars, | ||
| getGraderVars(assertion, test.vars), |
There was a problem hiding this comment.
P2 — excluded vars silently render as empty strings in custom rubricPrompt templates.
The projected vars are exactly what matchesLlmRubric → renderLlmRubricPrompt uses to render a custom rubricPrompt. A template referencing {{expected_answer}} with graderVars: ['question'] renders the reference criteria as an empty string — nunjucks doesn't throw and nothing warns, so the grader confidently grades against a gutted rubric.
This is the mirror image of the documented value behavior (rendered with full vars before projection), and the docs only state the value side; site/docs/guides/llm-as-a-judge.md:546-550 still says any test var is available in rubricPrompt. At minimum document this; ideally warn when an assertion has both a custom rubricPrompt and graderVars (or detect unresolved references).
|
|
||
| // Limit test variables passed to model graders. Built-in grader inputs | ||
| // such as output, rubric, and criteria are always included. | ||
| graderVars: z.array(z.string()).optional(), |
There was a problem hiding this comment.
P2 — accepted on every assertion type, honored by 10, silent everywhere else.
Setting graderVars on g-eval, answer-relevance, context-relevance, pi, or any string-check type validates fine and does nothing. Most of those never forward vars to a grader, so the no-op is harmless-but-confusing — except conversation-relevance, which does forward raw test.vars into its grader's rubric render (src/external/assertions/deepeval.ts:51-57 → matchesConversationRelevance spreads { messages, ...vars } into nunjucks.renderString). A user who sets graderVars: [] there to keep a sensitive or oversized var away from the grader gets zero effect and zero signal — precisely the exposure this feature exists to prevent.
Suggest either wiring conversation-relevance into the projection, or a logger.warn when graderVars is set on an unsupported type (precedent: validateSessionConfig warns on invalid-but-not-fatal config in src/validators/util.ts). The supported-type list already exists in three hand-synced places (this map's keys, the handler wraps, the docs sentence) — a warning would at least make the boundary observable.
| const builtins = GRADER_BUILTIN_VARS[assertion.type]; | ||
| return Object.fromEntries( | ||
| assertion.graderVars | ||
| .filter((name) => !builtins?.has(name) && Object.prototype.hasOwnProperty.call(vars, name)) |
There was a problem hiding this comment.
P3 — allowlisted-but-absent names are dropped with no diagnostic.
graderVars: ['refrenceAnswer'] (typo) is indistinguishable from a correctly filtered var: the rubric's {{referenceAnswer}} renders empty and the grader returns a confident wrong verdict. A one-line logger.debug (or warn) for names that fail the hasOwnProperty check would make this diagnosable.
(The hasOwnProperty guard itself is load-bearing and correct — name in vars would let graderVars: ['toString'] project prototype members. 👍)
|
|
||
| const clone = Clone(); | ||
|
|
||
| const GRADER_BUILTIN_VARS: Readonly<Record<string, ReadonlySet<string>>> = { |
There was a problem hiding this comment.
P3 (design) — a third hand-maintained copy of matcher-owned knowledge.
I verified all 10 entries match the matchers' actual injected vars today (including the context-faithfulness union across its two prompts), so there's no present drift bug. But the authoritative names live in each matcher's spread ({ output, rubric, ...vars } in llmGrading.ts/agent.ts/search.ts/rag.ts/comparison.ts), and nothing ties this map to them — the drift test in utils.test.ts:129 compares the map against a hardcoded copy of the same lists, so it can't catch divergence.
Cheap hardening for this PR: type the map as Partial<Record<AssertionType, ReadonlySet<string>>> so a typo'd/renamed key is a compile error, and share the duplicated Set literals (output/rubric ×3, factuality's set ×2 for the alias). Deeper follow-up worth considering: have matchers spread built-ins after (projected) user vars — matchesTrajectoryGoalSuccess already does — which deletes this map entirely and also fixes the pre-existing footgun where a test var named output replaces the real model output when graderVars is not set (I reproduced that on the control path).
| outputs, | ||
| test.options, | ||
| test.vars, | ||
| getGraderVars(assertion, test.vars), |
There was a problem hiding this comment.
P3 — consider computing the projection once instead of at 9 call sites.
Eight handlers repeat getGraderVars(assertion, test.vars) (plus this one). runAssertion already builds assertionParams with resolvedVars; computing graderVars: getGraderVars(assertion, resolvedVars) there and letting handlers read params.graderVars removes the duplication and — more importantly — shrinks the latent-bug surface where a future model-graded handler forwards test.vars raw and silently skips projection. (Projecting deeper, in getFinalTest or the matchers, doesn't work: handlers legitimately read full test.vars for input extraction like vars.query/vars.context, and 5 matchers don't receive assertion.)
| ['search-rubric', ['output', 'rubric']], | ||
| ['select-best', ['criteria', 'outputs']], | ||
| ['trajectory:goal-success', ['goal', 'output', 'trajectory']], | ||
| ] as const)('does not allow %s vars to shadow grader built-ins', (type, builtins) => { |
There was a problem hiding this comment.
P3 — this drift test is self-referential, and not- variants are uncovered.
The it.each table is a hardcoded copy of GRADER_BUILTIN_VARS, so it asserts the map equals itself — it can't catch the map drifting from what the matchers actually inject. Not easily fixable without exporting builtin names from matchers (see the design note on the map), but at minimum: add not-llm-rubric / not-factuality cases, which currently fail the guarantee this test documents (see the P1 on utils.ts:70).
| }); | ||
|
|
||
| it('should call matchesClosedQa with correct parameters', async () => { | ||
| it('should project grader vars while preserving closed-QA inputs', async () => { |
There was a problem hiding this comment.
Nit — this file lost its only plain-path (no graderVars) matcher-args assertion.
The retrofit converted the sole test asserting what reaches matchesClosedQa into a projection test. The pass-through path is still covered cross-file (contextPropagation.test.ts asserts { testVar: 'value' } with no graderVars), so no net suite gap — but a local one-liner mirroring llmRubric.test.ts's dedicated should preserve all test vars when graderVars is omitted would keep this file self-contained. (llmRubric's split of the three cases — omitted / empty / allowlist — is the right pattern.)
| - criteria | ||
| ``` | ||
|
|
||
| Set `graderVars: []` to pass no test variables. Built-in inputs such as the model output and rubric are still included. Names reserved for an assertion's built-in grader inputs are ignored if they appear in `graderVars`. |
There was a problem hiding this comment.
Docs — three behaviors this section should state, plus discoverability:
- The reserved built-in names are neither listed nor linked — users can't find out what's ignored per type (several types never document their built-ins anywhere). Inline the per-type list or link to it.
- Custom
rubricPrompttemplates render excluded vars as empty strings — the paragraph below covers thevaluedirection but not this opposite one, andguides/llm-as-a-judge.md:546-550still promises all test vars inrubricPrompt. - On unsupported assertion types
graderVarsis silently ignored (notablyconversation-relevance, which does send vars to its grader).
Discoverability: add a graderVars row to the assertion-properties tables in configuration/reference.md:141-152 and expected-outputs/index.md:46-57, and mention it in the option enumerations at index.md:99 (trajectory), llm-rubric.md, and agent-rubric.md:74. Minor: the supported list omits the model-graded-factuality alias.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5c7ec5269
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| assertion.threshold ?? 0, | ||
| test.options, | ||
| test.vars, | ||
| getGraderVars(assertion, test.vars), |
There was a problem hiding this comment.
When a user adds graderVars to a context-faithfulness assertion with a custom first rubricPrompt that references {{context}}, this projection filters context out because it is reserved, and the matcher does not add the resolved context back until rendering the second NLI prompt. Without graderVars the same prompt could receive context from test.vars, so this can make customized faithfulness graders extract claims without the retrieved evidence; pass the resolved context as a built-in for the prompt render phase instead of filtering it with ambient vars.
Useful? React with 👍 / 👎.
mldangelo
left a comment
mldangelo
left a comment
There was a problem hiding this comment.
Reviewed the full diff at f78308d (deep review at 87b6423, then a delta review of the three newer commits). Approving — the design is sound and the issues found during review are all addressed at this tip.
What was verified
- Reserved-name table exactness: every
GRADER_BUILTIN_VARSentry was checked against what its matcher actually injects and the merge order. For 8 of the wired types the matchers spread...varslast ({output, rubric, ...vars}inllmGrading.tsand friends), so the filter is load-bearing, not belt-and-suspenders;trajectory:goal-successspreads builtins last and is safe either way. No missing names, no spurious names. not-variants: at 87b6423 this was a real hole —GRADER_BUILTIN_VARS[assertion.type]missednot-llm-rubric(dispatch strips the prefix only for handler selection), and a probe confirmedgraderVars: ['output']let a test var replace the model's real output in the grading prompt for everynot-grader type. The baseType strip now ingetGraderVars(src/assertions/utils.ts:74) fixes it, theutils.test.tstable includesnot-llm-rubric/not-factualityregression cases, and the docs call out the protection explicitly.- Rendering order: the docs claim that assertion
valuerenders before projection holds for the runAssertion-dispatched types (renderedValue computed with full resolved vars before handler dispatch), and the tworunAssertion.test.tsintegration tests pin it end-to-end through real nunjucks, including that excluded vars stay out of the grader provider context. rubricPromptinteraction: custom rubric prompts render with the projected vars underthrowOnUndefined=false, so a var referenced only inrubricPromptand excluded bygraderVarssilently renders as an empty string (reproduced on both the JSON-message and legacy string paths). The new docs sentence now states this explicitly, which closes the trap; a runtime warning when a projected-away name appears in the template would be a nice follow-up but is not required.conversation-relevancewiring (new in the latest commits): the localgetConversationGraderVarsis correct — themessagesfilter is unconditional (no type-keyed lookup, so thenot-concern cannot recur there), and_conversationis read from the raw test vars before projection, so windowing still works when it is not allowlisted.- Projection itself is prototype-safe (
Object.prototype.hasOwnProperty.call),graderVars: []semantics are pinned, schema validation round-trips, and the docs reserved-name enumeration matches the code table entry-for-entry.
Test basis: 653/653 across the nine touched test files at 87b6423 locally; at f78308d verified by inspection plus the green 39-check CI run (my local node_modules predates the js-yaml v5 merge, so I did not rerun locally).
Non-blocking notes
- Docs: "a rubric containing
{{foo}}still includes the value offooafter variable substitution" is not accurate forselect-best—runCompareAssertionpassesassertion.valueraw tomatchesSelectBest, which inserts it as{{criteria}}in a single render pass; test vars are never substituted into the value. Worth scoping that sentence to the runAssertion-dispatched types. - Coverage: the default no-
graderVarspassthrough is only explicitly pinned forllm-rubric; the in-place rewrites of the agentRubric/contextRecall/modelGradedClosedQa/trajectoryGoalSuccess tests converted their original parameter-passing tests into projection tests. A smallit.eachpassthrough case per handler would restore that. getConversationGraderVarsintentionally duplicates the projection logic to keepsrc/externaloffsrc/assertions(architecture boundary); a one-line cross-reference comment in each would help keep the two in sync.
mldangelo-oai
left a comment
mldangelo-oai
left a comment
There was a problem hiding this comment.
Fresh review at f78308d5443dc0a114a28904b14e000b353853f3
The core allowlist works in the covered paths, but four items still need attention:
- The existing context-faithfulness P2 remains valid.
contextis filtered as reserved, but the first custom faithfulness prompt receives onlyquestion,answer, and projected vars; resolved context is added only to the second NLI prompt. A first-stage{{context}}therefore becomes empty whengraderVarsis enabled. - The docs overstate what omitting a variable can keep out of the grader request (inline).
- The trajectory change is missing the
site/docs/tracing.mdupdate required by the scoped repository instructions (inline). - Three changed projection call sites still lack regression-sensitive handler tests (inline).
Verification on this exact head: 678 tests passed across 12 focused files; npm run tsc passed; npm run build passed; regenerated site/static/config-schema.json matched byte-for-byte after formatting; and a secret-free local eval with an echo subject plus file grader confirmed that an allowlisted sentinel reached the grader while an excluded sentinel did not. All live checks are green.
The branch is one commit behind current main; that upstream commit only changes package-lock.json for Playwright, and a synthetic merge showed no conflict. Strict freshness still means the branch needs an update before merge. No branch changes or pushes were made during this review.
|
|
||
| The same protection applies to `not-` variants such as `not-llm-rubric`. | ||
|
|
||
| `graderVars` does not remove values already inserted into an assertion's `value`. For example, a rubric containing `{{foo}}` still includes the value of `foo` after variable substitution. To keep a variable out of the grader request, omit it from both `graderVars` and `value`. |
There was a problem hiding this comment.
P2 — narrow this data-exclusion guarantee. graderVars only filters the ambient variable map; required built-ins are populated independently. For example, context-recall resolves test.vars.context and passes it separately to the matcher, which includes it in both the prompt and provider context even with graderVars: []. The same pattern applies to faithfulness question/context, conversation messages, factuality input, and trajectory data. A user can follow this sentence and still send the supposedly excluded value to the grader. Please qualify it to say the value must also not be used to construct a built-in input such as the evaluated prompt, output, question, context, messages, goal, or trajectory.
| params.outputString, | ||
| params.test.options, | ||
| params.assertionValueContext.vars, | ||
| getGraderVars(params.assertion, params.assertionValueContext.vars), |
There was a problem hiding this comment.
P2 — update the required trajectory guide. This line adds user-visible graderVars behavior to trajectory:goal-success, but the scoped src/assertions/AGENTS.md rule says: Trace or trajectory changes also need site/docs/tracing.md. The expected-outputs docs are updated, while the canonical trajectory example in site/docs/tracing.md still omits this control. Please add a short example or link to the authoritative graderVars section, including empty-list and reserved-built-in behavior.
| outputString, | ||
| test.options, | ||
| test.vars, | ||
| getGraderVars(assertion, test.vars), |
There was a problem hiding this comment.
P2 — add projection-sensitive tests for the remaining changed handlers. factuality/model-graded-factuality, context-faithfulness, and search-rubric now call getGraderVars, but their handler tests never set graderVars; they cover only the omitted compatibility path. The utility table cannot catch a call site accidentally reverting to raw test.vars, which would silently resend excluded large or sensitive values while the suite stays green. Please add sentinel tests for the three unique handlers with one allowlisted and one excluded variable, and assert the matcher/provider receives only the allowlisted variable while real built-ins remain intact.
mldangelo-oai
left a comment
mldangelo-oai
left a comment
There was a problem hiding this comment.
Re-reviewed exact head ec21783f879a39ba8a9705f481d9a0479c91181a after it changed during the sweep. The new merge from current main is clean, and commit ec21783f8 addresses all four material items from my prior review: resolved context is now available to the first and second faithfulness prompts; the data-exclusion wording now distinguishes ambient variables from built-in grader inputs; site/docs/tracing.md documents trajectory graderVars; and projection-sensitive handler coverage was added for factuality, context-faithfulness, and search-rubric. I did not find a new actionable issue in this fix delta.
Verification on the new exact head: TypeScript passes; 38 focused tests across the four changed assertion/matcher suites pass; git diff --check passes; the head contains current main (e8adf8bf3ccc362801030ec041e4add50c4f329a); and every published check is green. The prior material threads can be resolved. No branch changes or pushes were made during this follow-up.
Adds assertion-level
graderVarsallowlists so model graders receive only selected test variables and avoid oversized grading requests.