fix(judge-builder): labeling session lookup can match the wrong judge - #931
Open
mittalpk wants to merge 1 commit into
Open
fix(judge-builder): labeling session lookup can match the wrong judge#931mittalpk wants to merge 1 commit into
mittalpk wants to merge 1 commit into
Conversation
LabelingService._get_labeling_session() looks up a judge's labeling
session by checking `short_id in session_name` (a raw substring test),
even though its own comment says "ends with short ID" and
create_session_name() always places the short ID as the segment
immediately before the trailing "_labeling" suffix
(f'{safe_name}_{short_id}_labeling').
Because judge names are user-supplied and only lowercased/underscored
(not otherwise restricted), a judge's sanitized name can coincidentally
contain another judge's 8-hex-char short ID anywhere in it. When that
happens, the substring check returns that unrelated judge's session
instead of the correct one (or instead of None), and every operation
built on top of it -- get_labeling_session, add_examples,
delete_labeling_session, get_labeling_progress -- silently operates on
the wrong judge's session.
Fix: anchor the match to the exact position the short ID actually
occupies, `session_name.endswith(f'_{short_id}_labeling')`, matching
create_session_name()'s real format and the comment's original intent.
Added two tests reproducing the exact collision (a decoy session whose
sanitized name embeds another judge's short ID) confirming both that
the correct session is picked when both are present, and that the
decoy is rejected when it's the only session. Verified both fail
against the pre-fix code via `git stash` isolation. Full judge-builder
suite: 148 passed / 60 failed / 4 errors, identical baseline to
upstream/main (146 passed) aside from the 2 new tests -- no
regressions; the pre-existing failures are unrelated (dspy/schema
tests requiring live model/Databricks credentials not available here).
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.
Summary
LabelingService._get_labeling_session()(judge-builder/server/services/labeling_service.py) looks up a judge's labeling session with a raw substring check:The comment says "ends with short ID," and
create_session_name()does always place the short ID as the segment immediately before the trailing_labelingsuffix (f'{safe_name}_{short_id}_labeling') — but the check itself isin, not an anchored match.Impact
Judge names are user-supplied and only lowercased/underscored by
sanitize_judge_name(not otherwise restricted), so a judge's sanitized name can coincidentally contain another judge's 8-hex-char short ID anywhere inside it. When that happens,short_id in session_namereturns the wrong judge's session — and every operation built on top of this lookup (get_labeling_session,add_examples,delete_labeling_session,get_labeling_progress) silently operates on that unrelated judge's session instead of raising a not-found error.Example: judge A is named
"Check for a1b2c3d4 keyword"→ session namecheck_for_a1b2c3d4_keyword_e5f6a7b8_labeling. Judge B's short ID happens to bea1b2c3d4. Looking up judge B's session returns judge A's session instead.Fix
Anchor the match to the position the short ID actually occupies:
This matches
create_session_name()'s real output format and the comment's original intent, and is collision-safe (a false match now requires the exact short ID to appear at that exact position, not merely as a substring anywhere).Tests
Added two tests to
tests/services/test_labeling_service.pyreproducing the exact collision:test_get_labeling_session_helper_ignores_short_id_appearing_in_another_judges_name— a decoy session (whose sanitized name embeds the target judge's short ID) and the real session are both present; asserts the real one is returned.test_get_labeling_session_helper_no_false_positive_when_only_decoy_present— only the decoy is present; assertsNoneis returned rather than a false match.Both confirmed to fail against the pre-fix code (verified via
git stashisolating justlabeling_service.py, rerunning, then restoring).Verification
judge-builder/tests/services/test_labeling_service.py: 23 passed (up from 21 onupstream/main, exactly the 2 new tests), same 6 pre-existing failures as the unmodified baseline (unrelated dspy/schema tests requiring live model access).judge-buildersuite (tests/): 148 passed / 60 failed / 4 errors — identical to theupstream/mainbaseline (146 passed / 60 failed / 4 errors) aside from the 2 new tests. The pre-existing failures require live Databricks/model credentials not available in this environment, and are unrelated to this change — stated honestly rather than glossed over.ruff check/ruff format --check: no new issues introduced by this change (the file has a handful of pre-existingE501line-length warnings on unrelated lines, left untouched, out of scope for this fix).git diff upstream/mainon both touched files confirmed minimal and scoped to just this fix.