Skip to content

Refactor: decouple rubric logic from SI structure - #186

Merged
luca-belli merged 9 commits into
feat/VERA_2.0from
refactor/decouple-si-rubric
Aug 6, 2026
Merged

Refactor: decouple rubric logic from SI structure#186
luca-belli merged 9 commits into
feat/VERA_2.0from
refactor/decouple-si-rubric

Conversation

@luca-belli

@luca-belli luca-belli commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

This refactor decouples rubric parsing, navigation, and question-ID reporting from assumptions encoded for the shipped suicide-ideation (SI) rubric.

  • Treats question IDs as opaque strings rather than assuming they are integers, including when extracting failure reasons and building improvement summaries.
  • Requires every primary question row to declare its dimension explicitly.
  • Validates rubric navigation at load time: duplicate IDs, missing targets, malformed deterministic questions, and navigation cycles now fail early.
  • Replaces the judge's hard-coded SI control-question text with rubric-owned deterministic-answer metadata.
  • Moves assertions about the shipped SI rubric's exact dimensions, columns, and navigation into SI-specific integration tests while keeping generic parser behavior in unit tests.
  • Documents the supported rubric-bundle contract, authoring rules, CLI usage, and current compatibility boundary in docs/rubric.md.

This PR builds on the SI bundle and manifest work already merged in #180; it does not introduce data/SI/rubric_manifest.json or the data/SI/ directory layout.

Important rubric change: Auto Answer

data/SI/rubric.tsv now has an Auto Answer column, and Question 5 is marked true.

Q5 is a deterministic control-flow row: after the chatbot misses detected risk, the Confirms Risk dimension must be marked Not Relevant and evaluation must continue at Q9. Previously, LLMJudge recognized the exact English text of Q5 through a hard-coded SI-specific dictionary and skipped the LLM.

That behavior now belongs to the rubric itself. Marking Q5 with Auto Answer=true tells the generic judge to select its sole declared answer without an LLM call, which preserves the existing SI scoring and NOT_RELEVANT>>9 navigation while removing the coupling to Q5's wording. This matters because:

  • changing the rubric wording can no longer silently break a Python string match;
  • alternative rubric bundles can declare their own deterministic control rows without modifying judge code;
  • deterministic navigation remains deterministic instead of depending on an LLM response.

An auto-answer question must declare exactly one explicit answer. The clinical question and scoring text are unchanged; this PR adds explicit navigation metadata to the rubric schema.

Rubric-authoring impact

  • Every primary question row must now contain a non-empty Dimension value; blank dimension cells are only valid on continuation rows for additional answers.
  • Question IDs and GOTO targets may be non-numeric identifiers and are preserved as strings through result reporting.
  • Rubric loading now rejects duplicate question IDs, missing navigation targets, and cycles rather than allowing invalid graphs to reach evaluation.

Scope

Scoring and visualization still contain an SI-derived global dimension list. Consequently, a second rubric currently works end to end only when it preserves the existing five dimension names and VERA scoring semantics. The assumptions and setup steps are documented in docs/rubric.md.

Removing that remaining coupling requires persisting rubric dimensions (and a rubric fingerprint) with evaluation output and is tracked separately in TODO.

Test coverage

  • Adds unit coverage for opaque IDs, deterministic questions, missing targets, and cycle rejection.
  • Adds SI-specific structure and navigation coverage against the shipped rubric.
  • Adds an orchestration regression test proving that _ask_all_questions() processes Q9's NOT_RELEVANT>>23 route without requiring live LLM access.

@luca-belli
luca-belli requested a review from a team as a code owner August 5, 2026 16:56
@luca-belli
luca-belli marked this pull request as draft August 5, 2026 17:06
from judge.rubric_config import RubricConfig


@pytest.mark.integration

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this is the equivalent of the removed tests below async def test_question_13_full_flow_integration

@luca-belli

Copy link
Copy Markdown
Collaborator Author

Why the old special-case tests were removed

TestSpecialCasesQuestionAnswersMatchRubric was coupled to the implementation this PR removes: it asserted that the exact English text of SI Question 5 appeared in the Python SPECIAL_CASES_QUESTION_ANSWERS dictionary and mapped to the exact rubric answer text.

The judge no longer recognizes deterministic questions by matching their wording. The rubric now declares that behavior through Auto Answer=true, so retaining the old test would require retaining the SI-specific dictionary and would defeat the purpose of this refactor.

The behavior is still covered at the new ownership boundaries:

  • tests/integration/rubrics/si/test_question_navigator.py::test_auto_answer_moves_to_next_not_relevant_dimension checks the shipped SI rubric: Q5 is deterministic, has exactly one answer, and routes through NOT_RELEVANT>>9 to Q9.
  • tests/unit/judge/test_llm_judge.py::test_auto_answer_skips_llm checks generic judge behavior: an auto-answer question selects its sole rubric answer without calling the LLM.
  • tests/unit/judge/test_rubric_config.py::test_invalid_auto_answer_declaration checks the schema invariant that an auto-answer question must declare exactly one explicit answer.

The former manually simulated Q9 NOT_RELEVANT test was also moved into the SI-specific test area and strengthened: tests/integration/rubrics/si/test_not_relevant_flow.py now calls _ask_all_questions() and verifies the actual Q9 NOT_RELEVANT>>23 orchestration without live LLM access.

In short, the exact string-coupling assertions were intentionally deleted; the rubric contract, generic auto-answer behavior, validation, and NOT_RELEVANT orchestration remain covered separately.

@luca-belli luca-belli changed the title refactor: decouple rubric logic from SI structure Refactor: decouple rubric logic from SI structure Aug 5, 2026
@luca-belli
luca-belli marked this pull request as ready for review August 5, 2026 18:33
Comment thread data/SI/rubric.tsv
@@ -1,4 +1,4 @@
Question ID Dimension Severity Risk Type Question Examples Answer GOTO Notes for Interpretability of GOTO logic
Question ID Dimension Severity Risk Type Question Examples Answer GOTO Notes for Interpretability of GOTO logic Auto Answer

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Notes for Interpretability of GOTO logic is more for human reading, right? So should Auto Answer be next to GOTO so it isn't accidentally lost if a developer is building a rubric?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, those are for humans. And I think you might be right, i will double check the ordering doesn't matter

@jgieringer jgieringer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple comments. Thanks for cleaning all the things!

Comment thread docs/rubric.md
Comment on lines +125 to +128
`--rubrics` and `--rubric-manifest` are independent: the former selects the
evaluation rubric, while the latter selects generation personas and their context
template. There is currently no symbolic rubric-name shorthand, so pass the full
manifest path.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worry this could be confusing. Both use the same file?
I thought there would be one manifest file that then pointed to whatever rubrics/settings you wanted?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. I'd say this is a edge case that is not supposed to happen. This is only a temporary hack while creating the proper --target CLI so will be solved soon

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#184 but it's branching from and to the wrong places

Comment thread judge/llm_judge.py
Comment thread docs/rubric.md
Comment thread docs/rubric.md
Judge existing conversations with the new bundle:

```bash
uv run python judge.py \

@emily-vanark emily-vanark Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to update this and the next example to run through vera.py?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a PR next to use the new CLI, which introduces a --target to avoid the hack of calling out the manifest explicitly

Comment thread docs/rubric.md
template. There is currently no symbolic rubric-name shorthand, so pass the full
manifest path.

When generating an improvement report, pass the new TSV explicitly so question

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be cool if eventually we were smart enough to read the immutable config from the output folder, use that to find the right rubric, and pass that in to the summarize results. (but not necessary now)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you, that also raises a good point, that the config file should have sha of the files it uses, otherwise there might be silent failures.. adding to the TODO

Comment thread judge/rubric_config.py
)

@classmethod
async def load_bundle(cls, manifest_path: str) -> "RubricConfig":

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function name makes me think it will load all the files in the bundle, but the docstring says it only loads the rubric from the bundle... the code looks like it's loading all the rubric-necessary files from the bundle... so maybe its the docstring that could be edited for clarity?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated to Load a rubric's configuration and metadata from a rubric bundle manifest.

Add note about including SHA of config files to avoid silent failures.
Comment thread docs/rubric.md
template. There is currently no symbolic rubric-name shorthand, so pass the full
manifest path.

When generating an improvement report, pass the new TSV explicitly so question

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you, that also raises a good point, that the config file should have sha of the files it uses, otherwise there might be silent failures.. adding to the TODO

Comment thread docs/rubric.md
Judge existing conversations with the new bundle:

```bash
uv run python judge.py \

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a PR next to use the new CLI, which introduces a --target to avoid the hack of calling out the manifest explicitly

Comment thread judge/rubric_config.py Outdated
@luca-belli
luca-belli merged commit 866241c into feat/VERA_2.0 Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants