fix(grading): compare numerically-equal state values as equal#532
Merged
Conversation
State grading compared DB field values as raw strings/JSON, so a Decimal column that round-trips as "130.00" on one side and "130.0" (or 72 vs "72.00") on the other was graded a state mismatch — a false-fail on otherwise-correct trials (e.g. OTS custom_refund_amount / payroll amount). Canonicalize numeric values before comparison in both grading hash paths — core/hash.py:compute_stable_hash (the frozen-mcp-core / OTS path) and core/grading/state_checks.py:to_hashable (the native / tau path) — and in the human-readable runner/grading.py diff. bool and leading-zero id strings are left untouched so distinct ids/flags are never collapsed; genuinely different numbers stay different. compute_stable_hash gains a canonicalize_numbers flag (default True) so a caller needing byte-exact mcp_core.calculate_database_hash() output can opt out. Grading is engine-vs-engine (expected and actual are both hashed live at grade time), so the change is symmetric and invalidates no stored production hash; the three tau_retail_mini test fixtures pin a stored hash and are re-pinned to the canonicalized value.
Contributor
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL flagged `_DECIMAL_LITERAL_RE` as a polynomial-backtracking regex on uncontrolled data. Replace it with a regex-free, linear `_looks_like_plain_decimal` check plus a length cap, preserving the exact accept/reject behavior (leading-zero ids and scientific notation still rejected; "130.00"=="130.0"=="130" etc. still collapse). No backtracking; oversized input short-circuits on the cap.
Contributor
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
…t, docs
Adversarial review follow-ups on the numeric-canonicalization fix:
- Escape a genuine string that begins with the reserved NUL prefix so a crafted
value like "\x00tf-num:130" can never be byte-identical to a numeric token
(closes the one false-pass channel in the design).
- Fold -0 to 0 in the canonical token so numerically-equal negative-zero
representations ("-0.00" vs "0.0") collapse.
- Type-stable set sort in to_hashable so a set mixing bool with tagged
numeric/str tokens no longer raises TypeError.
- Correct the mcp_core-parity docstrings: byte-for-byte parity with
calculate_database_hash() now holds only under canonicalize_numbers=False.
Contributor
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
…lback - `_records_might_match` now compares via `_make_hashable`, so a numeric id "123" pairs with 123 (consistent with the record hashing; the verdict already came from the hash, so this only tidies the human-readable diff pairing). - The json_db_service standalone import-fallback `compute_stable_hash` mirrors the numeric canonicalization and the `canonicalize_numbers` kwarg (byte-parity with core.hash verified), so degraded standalone mode no longer diverges from the installed engine.
Contributor
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
Contributor
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
Review feedback (task/project-level opt-in, not a global default):
- canonical_number / compute_stable_hash split into two tiers. Numeric TYPES
(int/float/Decimal) always fold (72 == 72.0 == Decimal("72.00")) — the type
declares number-ness, generic and safe. Numeric-looking STRINGS
("130.00" == "130.0") fold ONLY with the new normalize_numeric_strings
opt-in — exact string representation can carry meaning (versions "1.10" vs
"1.1", codes), so folding it is NOT a generic improvement.
- New per-task grading key state_checks.numeric_string_normalization, threaded
task config -> runner StateChecksConfig -> GradeTrial -> db_client
get_stable_hash -> json_db_service /state/hash -> compute_stable_hash. The
native adapter maps it from grading.yaml; the frozen-mcp-core adapter
follow-up lives in tolokaforge-tools.
- json_db_service standalone fallback mirrors the two-tier split.
- Tests updated: default tier (types fold, strings do not) + flag tier
(strings fold, guards hold: leading-zero ids, bool, genuine differences,
NUL-tag collision). tau_retail_mini fixture hashes unchanged (no numeric
strings in that state).
Contributor
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
Contributor
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
…l switch
The opt-in that folds numeric-looking STRINGS ("130.00" == "130.0") was a
single per-task boolean. That is inconsistent with the danger it guards
against: a task can hold both a money field (safe to fold) and a version /
code field (must NOT fold) in the same record, so one global switch is wrong;
it only happened to be safe in the current domains by luck (their id fields
carry letter prefixes).
Replace the boolean with a per-field allow-list. Numeric TYPES still always
fold (generic, safe); numeric-looking STRINGS fold ONLY for values under a
record key named in state_checks.numeric_string_fields.
- core/hash.py: compute_stable_hash gains numeric_string_fields; the recursive
canonicalizer is now key-aware (a value folds strings only when its enclosing
record key is listed; a nested dict re-decides per its own keys, so a version
field is never folded because a sibling money field is listed).
- runner/models.py: StateChecksConfig.numeric_string_normalization ->
numeric_string_fields: list[str].
- runner/service.py, runner/db_client.py, env/json_db_service/app.py
(endpoint + Trial method + standalone fallback), adapters/native.py: thread
the field list through the whole grading hash path.
- tests: per-field API, plus a selectivity test proving an unlisted sibling
field (version "1.10" vs "1.1") is preserved while the listed money field folds.
Contributor
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
…ncy, docs, parity) Independent review (APPROVE-WITH-NITS) surfaced 5 items; all fixed: - [MEDIUM] numeric_string_fields was a silent no-op on the core GradingEngine path. The runner gRPC path folded per-field strings, but Adapter.grade -> combine.py -> to_hashable did not: core StateChecksConfig lacked the field (pydantic dropped it) and to_hashable had no per-field param, so the same task could grade differently by substrate. Add numeric_string_fields to core StateChecksConfig; make to_hashable key-aware (mirrors _canonicalize_numbers); thread it through check_hash / grade / grade_tau_style / compute_tau_style_expected_hash / combine.py / environment_evaluator. Default (empty) preserves current behavior exactly. Add a core-path selectivity test. - [MEDIUM] the unconditional to_hashable change invalidates any externally stored expected_state_hash. Scanned all task-pack clones: 0 grading configs store a hash literal (nothing to migrate; the 3 tau_retail_mini fixtures were re-pinned). Documented the recompute caveat in GRADING.md + GRADING_VERIFICATION.md. - [LOW] runner/grading.py comment claimed a numeric-string id "123" pairs with 123; it does not (string tier is off on that reason-only path). Comment fixed. - [LOW] stale docs: GRADING.md / GRADING_VERIFICATION.md embedded the pre-canon algorithms and did not mention numeric_string_fields. Updated both, with a per-field config example and the "don't list ids" guidance. - [LOW] the json_db_service standalone fallback had no sync guard beyond a comment. Added a parity test that force-loads the fallback (poisoned import, guarded so it can't silently compare the real impl to itself) and checks it byte-matches core compute_stable_hash across tiers/escape/opt-out.
Contributor
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
bberkes-toloka
marked this pull request as ready for review
July 21, 2026 12:47
…tring_fields Adding numeric_string_fields to the core StateChecksConfig (previous commit) adds the key to GradingConfig.model_dump(), so the 4 native-adapter canonical grading_config.json snapshots that carry a state_checks block drift. Repin them (field only; tbench_echo_hello has state_checks=null and is unaffected). Caught by CI test-smoke (tests/canonical), which the local grading sweep skips.
Contributor
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
azorej
approved these changes
Jul 21, 2026
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
3 participants
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.
Problem
State grading hashes the trial's final DB state and the golden state and passes
iff the two hashes match. Field values were compared as raw strings / JSON,
so a
Decimalcolumn that round-trips through the DB as"130.00"on one sideand
"130.0"(or72vs"72.00") on the other was graded a statemismatch — a false-fail on an otherwise-correct trial.
This is not cosmetic. On a full arena eval it false-failed 258 trials in one
OTS domain alone (
custom_refund_amount), dragging its raw pass@1 from a true~40% down to ~2%, plus a payroll
amountfield in a second domain. Because theleaderboard ranks by
pass^5, the raw number badly mis-ranks any model whoseonly "error" is a trailing-zero formatting difference.
Root cause
Two independent hashing paths compared values literally:
core/hash.py:compute_stable_hash— JSON-serializes the state then SHA-256.Used by the frozen-mcp-core adapter (OTS / arena domains).
core/grading/state_checks.py:to_hashable— folds state into a hashed tuple.Used by the native / tau adapter + env-evaluator.
"130.00" != "130.0"as strings → different hash → mismatch → fail.Fix
A shared
canonical_number()maps every representation of a number to one tokenbefore hashing, applied on both paths and in the human-readable
runner/grading.pydiff. Two tiers:int/float/Decimal) always fold(
72 == 72.0 == Decimal("72.00")). Generic and safe — the type declaresnumber-ness. On by default.
"130.00" == "130.0") fold ONLY for values undera record key listed in
state_checks.numeric_string_fields(a per-fieldallow-list), because a string that merely looks numeric can carry meaning in
its exact form (versions
"1.10"vs"1.1", codes, zero-padded ids). Thecanonicalizer is key-aware: a value folds strings only when its immediate
record key is listed, and a nested dict re-decides per its own keys.
Guards in both tiers:
boolis left untouched (True == 1is undesirable forgrading); leading-zero identifier strings (
"00123") are not collapsed;genuinely different numbers stay different; a genuine string that begins with the
reserved numeric-token prefix is escaped so it cannot masquerade as a token; the
string parser is a linear (ReDoS-safe) scan with a length cap.
compute_stable_hashgains acanonicalize_numbersflag (default True) so acaller needing byte-exact
mcp_core.calculate_database_hash()output can opt outwith
canonicalize_numbers=False. The string tier is off unless a caller passesnumeric_string_fields.numeric_string_fieldsis honored identically on both grading substrates —the core
GradingEngine(to_hashable) path and the runner gRPC(
compute_stable_hash) path — so a task grades the same regardless of how it isrun.
Why this is safe (no stored hash invalidated)
Grading is engine-vs-engine: both the expected hash (replay golden actions on
fresh state) and the actual hash are computed at grade time with the same
function, so the change is symmetric — it can only flip a decimal-format
false-fail to a correct pass, never break a real pass.
No production task stores a precomputed hash: a scan of the task-pack clones
found 0 grading configs with an
expected_state_hashliteral (arena OTS +tau all replay golden actions live). The only stored hashes in the tree are 3
tau_retail_minitest fixtures, re-pinned here.to_hashablehas no byte-paritycontract to preserve, but note that any hash literal precomputed before this
change must be recomputed (documented in
docs/GRADING.md).Testing
Grading / hash / canonical suites pass on the frozen engine venv (
283intests/unit/grading,444intests/canonical, plus json_db_service / nativeadapter / grader / golden-replay). New coverage:
listed field; a selectivity test proves an unlisted sibling (
version"1.10"vs"1.1") is preserved while the listed money field folds — on boththe
compute_stable_hashandto_hashablepaths.-0, tag-collision escape, opt-out.json_db_servicefallback isbyte-identical to core
compute_stable_hash(force-loads the fallback via aguarded poisoned import).
Independent review
An independent adversarial review (~120 executed probes) returned
APPROVE-WITH-NITS with no primary-path grading bug. All findings addressed:
the per-field key is now threaded through the core
GradingEnginepath (was asilent no-op there); the stored-hash scan (0 hits) is recorded; the
json_db_servicefallback has a parity test; a misleading diff-path comment andthe stale
docs/GRADING*.mdare fixed, andnumeric_string_fieldsis documented.Configuration
Companion: tolokaforge-tasks#38 enables it for the two d365 money domains
(
[custom_refund_amount]for travel_marketplace,[amount]for logistics). Aone-line frozen-mcp-core adapter mapping (tolokaforge-tools) carries the key from
grading.yaml into the runner
StateChecksConfig.Follow-up (not in this PR)
Cut an engine release, repin it in
tolokaforge-tasks, land the adapter mapping,and regrade the affected saved trajectories (the fix changes grades, so
existing results must be re-graded, not just re-run).
Regrade impact (measured on saved results)
Offline reclassification from each trial's saved final state +
state_diff— nomodel re-run. A trial flips to pass iff its only state differences are
numerically-equal but differently-formatted values on the configured fields.
Every measured flip had a single-field signature on exactly
custom_refund_amount(travel_marketplace) oramount(logistics), so theper-field lists preserve 100% of the benefit.
Muse 1.1 (surfaced the bug) — materially affected:
Other existing models — the defect is model-agnostic (false-failed 9 models
across 8 vendors on the same two money fields), but retroactively small: 86
flips across 90,450 trials (0.16% of failures).
No pair of models crosses — the leaderboard order is unchanged. Regrading the
fleet is a correctness / consistency exercise, not a board-mover; Muse is the
only model whose standing materially depends on the fix.