fix(deepseek-math): stop symbolic_equal executing the answer it grades

sieval/community/CLAUDE.md

@@ -2,7 +2,7 @@
 
 ## Purpose
 
-This directory contains local adaptations of third-party evaluation tools (e.g. livecodebench, instruction_following_eval, simple_evals). These are wrappers around upstream implementations, not original code.
+This directory contains local adaptations of third-party evaluation tools (e.g. livecodebench, instruction_following_eval, simple_evals). These are wrappers around upstream implementations, not original code — with the narrow exception below.
 
 ## Requirements
 
@@ -15,3 +15,18 @@ This directory contains local adaptations of third-party evaluation tools (e.g.
 * No mandatory test coverage.
 * No mandatory internal code style enforcement (but keep it readable).
 * License attribution must be preserved where required by upstream.
+
+## First-Party Modules
+
+`_sympy_guards.py` is original code, not a wrapper: the execution guards
+`deepseek_math` and `ugmathbench` share. Holding it *outside* both serves
+upstream alignment rather than working against it — the vendored files keep only
+a small annotated divergence each, instead of carrying a copy of the guards
+inline where it would swamp a diff against upstream. Do not add more original
+code here without the same argument; a helper with one caller belongs in that
+caller's module.
+
+The package-wide `ruff` / `mypy` / `pre-commit` exclusions exist to keep vendored
+code byte-identical and cover this file too, which is the wrong default for a
+security boundary. Until they are narrowed, lint it by hand:
+`ruff check --config 'exclude=["vendor"]' sieval/community/_sympy_guards.py`.

sieval/community/_sympy_guards.py

@@ -0,0 +1,156 @@
+"""Guards for handing model output to sympy.
+
+Both math graders in this package parse a model's answer with sympy, and sympy
+parsing *is* an execution path. The three guards here are the same in both, and
+have to stay the same: a new escape route closes in one place or the other
+grader is left open. That shared contract is why they live here rather than in
+either module.
+
+The threat has three legs, and closing one without the others buys nothing:
+
+1. ``parse_expr``'s default global namespace is built by
+   ``exec("from sympy import *", ...)``, which also injects ``__builtins__`` --
+   so model output gets ``__import__`` and ``open``. :func:`sympy_globals`
+   removes them.
+2. Sympy re-sympifies a *string* argument with its own default namespace, which
+   has the builtins back, so a call carrying a string literal escapes leg 1.
+   :func:`quotes_free` refuses the quote instead of the callee.
+3. ``parse_expr`` evaluates as it parses, so ``9**9**9`` never returns.
+   :func:`evaluable` screens it out with an unevaluated pre-parse.
+
+**None of this makes a string safe to hand to ``sympify``, ``simplify``, ``N``
+or ``S``.** Those take the default namespace, not the caller's, so they defeat
+legs 1 and 2 on their own -- a payload needs no quote at all once ``__import__``
+resolves (``__import__(chr(111)+chr(115))``). A caller must ensure a *parsed
+sympy object* reaches them, never the raw text; refusing at the parse step and
+then falling through to ``simplify(text)`` reopens exactly what was closed.
+
+Callers are responsible for their own time bound. Leg 3 only screens the two
+shapes common enough to be worth not spending a worker on; an eagerly-evaluating
+sympy callable needs no exponent to be expensive (``primepi(10**12)`` takes 48 s,
+``factorial(1000000)`` 3.8 s), and enumerating those is the same losing game as
+allowlisting callees in leg 2. The bound that actually holds is
+:data:`~sieval.core.utils.offload.GRADE_TIMEOUT` in the worker process.
+
+AI-Generated Code - Claude Opus 5 (1M context) (Anthropic)
+"""
+
+#: Largest integer exponent :func:`evaluable` will admit. Sympy computes
+#: ``a**b`` eagerly, so a boxed ``9^9^9^9`` asks for a 370-million-digit integer
+#: and never returns. Nothing either benchmark asks for comes close — the
+#: largest exponent in UGMathBench's pinned references is three digits — so the
+#: cap costs no reachable comparison.
+MAX_EXPONENT = 10_000
+
+
+def sympy_globals() -> dict:
+    """Namespace for :func:`parse_expr`, with the builtins removed.
+
+    ``parse_expr`` evaluates its input, and its *default* global namespace is
+    built by ``exec("from sympy import *", ...)`` — which also injects
+    ``__builtins__``. Since the string being parsed is model output, that hands
+    a model ``__import__`` and ``open``: a boxed
+    ``__import__('os').system(...)`` runs, and the grader still reports the slot
+    wrong, so nothing in the run looks unusual.
+
+    Clearing ``__builtins__`` closes that without narrowing the dialect. The
+    sympy names have to stay: ``auto_symbol`` rewrites an unknown callable into
+    ``Function('sin')``, so a namespace holding *only* the answer aliases fails
+    every legitimate ``sin(pi*x/5)`` with ``NameError: name 'Function' is not
+    defined``.
+
+    This is a namespace restriction, not a sandbox — attribute access on sympy
+    objects still resolves, and it only covers the namespace *this* parse runs
+    in. It does not survive a nested parse, which is why :func:`quotes_free`
+    exists.
+    """
+    namespace: dict = {}
+    exec("from sympy import *", namespace)  # noqa: S102 - fixed literal, not input
+    namespace["__builtins__"] = {}
+    return namespace
+
+
+def quotes_free(text: str) -> bool:
+    """Is *text* free of the string literals that reopen the interpreter?
+
+    :func:`sympy_globals` sanitizes the namespace the top-level parse runs in,
+    and that is not enough on its own. Sympy re-sympifies a *string* argument
+    with its own default namespace, which has the builtins back, so a call
+    carrying a string literal escapes the restriction and runs::
+
+        eval("__import__('os').system(...)")
+
+    ``sympify``, ``S`` and ``N`` do the same thing, and ``auto_symbol`` turns
+    any unrecognized name into a ``Function``, so the callee cannot be
+    allowlisted — every function call is a potential carrier. What can be
+    refused is the payload: without a quote there is no string literal for the
+    nested parse to read, and the argument comes back as a sympy object
+    (``eval(chr(112))`` evaluates ``chr`` symbolically and does nothing).
+
+    This holds only for text reaching ``parse_expr`` with a cleared namespace.
+    It is **not** sufficient for text handed straight to ``sympify`` / ``N`` /
+    ``simplify``, where ``__import__`` resolves without any quote at all — see
+    the module docstring.
+
+    Nothing legitimate is lost, on evidence from both dialects: not one of
+    UGMathBench's 42,064 gold slots on the pinned revision contains a quote
+    (sympy source, where quotes have no meaning), and for deepseek's LaTeX it is
+    the 6,319-sample replay in that module's deviations note. A refused
+    prediction only loses this one reading, with the LaTeX and literal-equality
+    paths still offered to the comparison.
+    """
+    return "'" not in text and '"' not in text
+
+
+def evaluable(cleaned: str, local: dict | None = None, transformations=None) -> bool:
+    """Is *cleaned* free of the two exponent shapes that never finish?
+
+    Deliberately narrower than "would this terminate". ``parse_expr`` evaluates
+    as it parses, so the check cannot run afterwards — by then the process is
+    already computing. Parsing with ``evaluate=False`` first builds the tree
+    without doing the arithmetic (microseconds even for the pathological cases),
+    which is cheap enough to screen on.
+
+    Rejected: a power whose exponent is itself a power (``9**9**9``, the tower
+    shape), and an integer exponent above :data:`MAX_EXPONENT`. A left-nested
+    ``(x**2)**3`` is fine and stays — only the right-nested tower explodes.
+
+    What it does **not** screen is covered in the module docstring: an eagerly
+    evaluating sympy callable needs no exponent to be expensive, and the bound
+    that holds is the caller's worker timeout. This screen only buys back the
+    two shapes common enough to be worth not spending a worker on.
+
+    A rejected answer grades wrong rather than hanging the run. That is the
+    correct trade for a grader: this pass only ever *upgrades* a verdict (it is
+    reached after every other strategy said "not equal"), so refusing to run it
+    can lose a point but can never invent one.
+    """
+    import sympy
+    from sympy.parsing.sympy_parser import parse_expr
+    from sympy.parsing.sympy_parser import (
+        standard_transformations as _standard,
+    )
+
+    if "**" not in cleaned:
+        return True
+    try:
+        tree = parse_expr(
+            cleaned,
+            local_dict=local,
+            global_dict=sympy_globals(),
+            transformations=_standard if transformations is None else transformations,
+            evaluate=False,
+        )
+    except Exception:
+        # Unparseable under this reading; the real parse will fail the same way
+        # and is harmless. Screening is not the place to decide that.
+        return True
+    for node in sympy.preorder_traversal(tree):
+        if not isinstance(node, sympy.Pow):
+            continue
+        exponent = node.exp
+        if exponent.has(sympy.Pow):
+            return False
+        if exponent.is_Integer and abs(int(exponent)) > MAX_EXPONENT:
+            return False
+    return True

sieval/community/deepseek_math.py

@@ -27,6 +27,30 @@
   (`few_shot_prompts/cot_minerva_math_4_shot.py` + the `math-cot-test` path).
 
 Deviations from upstream:
+- **`symbolic_equal` does not execute model output.** Upstream parses a
+  prediction with a bare `parse_expr`, whose default namespace carries
+  `__builtins__`, and — when both parsers fail — returns the *raw string*, which
+  then reaches `N`, and `N` sympifies it with sympy's own default namespace.
+  (Only `N`: `simplify(a - b)` raises `TypeError` first, since sympy's
+  arithmetic dunders sympify strictly.) Either route runs
+  `__import__('os').system(...)` supplied as an answer, and the grader still
+  reports the sample wrong, so nothing in the run looks unusual. Two changes
+  close it: `parse_expr` runs under `_sympy_guards` (cleared namespace, quote
+  screen, unevaluated exponent pre-parse), and an unparseable answer becomes
+  `None` and refuses the comparison rather than reaching `N` as text. The second
+  matters more than the first — once `__import__` resolves through the default
+  namespace, a payload needs no quote at all, so guarding only the parse would
+  have moved the hole rather than closed it.
+  MEASURED DIVERGENCE: ZERO. Replayed over two full stored runs — GSM8K 1319
+  samples (deepseek-llm-7b-chat) and MATH 5000 (Qwen2.5-72B) — under both a
+  working and a deliberately disabled `parse_latex`, the latter forcing every
+  comparison down the guarded path (1622 of 5000 fall through on MATH). All
+  four cells agree with upstream on every sample: GSM8K 63.3813 / 63.3055 and
+  MATH 61.2600 / 60.0200, upstream and guarded alike.
+  The exponent pre-parse also declines a right-nested `**` tower and an exponent
+  above `MAX_EXPONENT`, which upstream evaluates; `parse_latex` reads those
+  spellings first, so the zero covers them too. See `sieval/tasks/CLAUDE.md` on
+  why this ships under the unqualified task names.
 - `math_equal` is only ever called with the default `timeout=False` (via
   `eval_math` / `is_correct` and the GSM8K path), so the
   `symbolic_equal_process` / `call_with_timeout` multiprocessing path is unused
@@ -54,6 +78,8 @@
 from sympy.parsing.latex import parse_latex
 from sympy.parsing.sympy_parser import parse_expr
 
+from ._sympy_guards import evaluable, quotes_free, sympy_globals
+
 
 def _fix_fracs(string):
     substrs = string.split("\\frac")
@@ -312,16 +338,36 @@ def is_digit(num):
     # paired with parse_digits
     return parse_digits(num) is not None
 
+def _guarded_parse_expr(s):
+    """`parse_expr` with the three guards in `_sympy_guards` applied.
+
+    SIEVAL DIVERGENCE (execution safety). Upstream calls bare `parse_expr(s)`
+    on model output, whose default namespace carries `__builtins__`, so a
+    prediction of `__import__('os').system(...)` runs. See `_sympy_guards`.
+    """
+    if not quotes_free(s) or not evaluable(s):
+        raise ValueError("refused by sieval: unsafe to hand to sympy")
+    return parse_expr(s, global_dict=sympy_globals())
+
+
 def symbolic_equal(a, b):
     def _parse(s):
-        for f in [parse_latex, parse_expr]:
+        for f in [parse_latex, _guarded_parse_expr]:
             try:
                 return f(s)
             except:
                 pass
-        return s
+        # SIEVAL DIVERGENCE (execution safety). Upstream returns `s` here, the
+        # raw model output, which reaches `N` below -- and `N` sympifies it with
+        # sympy's own default namespace, not the caller's. (Not `simplify(a-b)`:
+        # the subtraction raises TypeError first.) That alone defeats the guards
+        # above -- with `__import__` resolvable a payload needs no quote -- so an
+        # unparseable answer becomes None and the comparison is refused.
+        return None
     a = _parse(a)
     b = _parse(b)
+    if a is None or b is None:
+        return False
 
     try:
         if simplify(a-b) == 0: