[Bug]: theoremqa evaluates model output with bare eval(); the sandbox is escapable, and closing it is a `_fixed`-shaped change

[ethan-scitix]

Your current environment

• Python version: 3.13
• SiEval version: 0.7.0 (main @ 8aa09ae3)
• Relevant libraries and their versions: n/a — the defect is in sieval/tasks/theoremqa_kshot_base_gen.py, not a dependency
• Other information: found while sweeping for the defect class fixed in feat(tasks): task-name variants, UGMathBench as _fixed, and grading off the shared event loop #68 (parse_expr evaluating model output)

🐛 Describe the bug

theoremqa_kshot_base_gen evaluates model output with bare eval() at three sites — :266, :313, :366 — guarded by _EVAL_GLOBALS, which sets "__builtins__": {}.

That guard is not a sandbox. Clearing __builtins__ does not prevent attribute traversal, and the standard catch_warnings route reaches a live open:

from sieval.tasks.theoremqa_kshot_base_gen import _EVAL_GLOBALS

payload = (
    "[c for c in ().__class__.__base__.__subclasses__() "
    "if c.__name__=='catch_warnings'][0]()._module.__builtins__['open']"
    "('/tmp/pwned','w')"
)
eval(payload, _EVAL_GLOBALS)

Observed: /tmp/pwned is created. The three call sites reach this from an extracted model answer, so any model being evaluated can write files as the eval process.

Two smaller notes on the same guard:

• factorial is exposed, so eval("factorial(2000000)", _EVAL_GLOBALS) runs for 16 s on this machine (measured) with no upper bound. Grading is synchronous on the shared event loop — MultiTaskRunner.arun starts every TaskRunner in one anyio.run — so that stalls every other task in the session, not just this one.
• The task also still grades inline. feat(tasks): task-name variants, UGMathBench as _fixed, and grading off the shared event loop #68 moved every math-verify grader into a worker process and the pure-sympy ones onto anyio.to_thread.run_sync; theoremqa was left alone because its fix is not mechanical (see below).

This is not just a hardening change — it changes grading semantics

This is the part worth deciding before writing any code. The module already says so at :78:

# Upstream uses bare eval(num). SiEval keeps eval sandboxed for task runtime
# safety, so builtins such as abs/round/pow are intentionally unavailable.

So a model answering abs(-5) or round(1.6) scores differently here than under upstream's evaluator. That divergence is real, deliberate, and unquantified — and reference_impl.notes currently claims the opposite:

"answer_clean and numeric matching mirror official utils.py/number_utils.py."

Meanwhile the task ships under the unqualified name theoremqa_kshot_base_gen.

Under the variant convention introduced in #68 (.claude/rules/tasks.md, sieval/tasks/CLAUDE.md), the unqualified name means "what upstream measures, bugs included", and a local correction takes a _fixed variant owing (a) every divergence enumerated in reference_impl.notes and (b) a quantified score impact. By that rule:

1. the existing sandboxing is already a convention violation — an unquantified, score-affecting divergence under an unqualified name, with notes asserting fidelity;
2. and closing the RCE properly makes the divergence larger, so it lands in the same place: a _fixed variant, not a patch to the unqualified task.

Put plainly: hardening the evaluator is itself a _fixed. That is why this is an issue and not a quick PR.

Proposed sequencing

1. Now, cheap and semantics-neutral: correct reference_impl.notes so it stops claiming to mirror upstream, and record the known divergence (missing builtins) as text. Regenerate sieval/meta/index.json.
2. Gate on feat(tasks): task-name variants, UGMathBench as _fixed, and grading off the shared event loop #68 landing and the _fixed pattern being exercised — ugmathbench_0shot_gen_fixed is its first user and hasn't merged yet. Migrating a second benchmark onto a convention that has not yet been proven end-to-end is the wrong order.
3. Then: theoremqa_kshot_base_gen_fixed with a real evaluator (a tokenising parser rather than eval, the way sieval/community/ugmathbench.py::_parse_sympy_source does it — it was tested against these same payloads and does not execute them, because parse_expr rewrites name tokens into symbols), plus the offload, plus the quantified delta against the current task on a stored run.

Open decision: the convention needs an explicit safety carve-out

This outlives the theoremqa fix — do not close it with the RCE.

Step 3 above leaves the unqualified theoremqa_kshot_base_gen in an awkward place. The convention says the unqualified name tracks upstream including its bugs, and upstream's bug here is an unsandboxed eval on model output. Read literally, the rule asks us to ship a remote-code-execution path under the plain name.

That cannot be the intent, but right now nothing says so. The rule is silent on the case, which means the next person to hit it has to re-derive the answer — and the two readings ("be faithful" vs "don't ship an RCE") point opposite ways.

Proposed: state the exception in .claude/rules/tasks.md and sieval/tasks/CLAUDE.md, next to the existing "the unqualified name tracks upstream" paragraph. Rough wording:

Fidelity stops at execution safety. Tracking upstream never extends to reproducing a path that executes model output, writes outside the run directory, or cannot be bounded. Where upstream's own harness does that, the unqualified task carries the hardened behaviour, and the divergence is recorded in reference_impl.notes with its score impact quantified like any other. This is the one divergence that does not require a _fixed variant — a variant exists so two readings can be compared, and "the unsafe reading" is not one we intend to run.

Note this cuts against the rest of the convention on purpose, so it should be argued rather than slipped in — hence flagging it here instead of folding it into #68. Two things to settle:

1. does the carve-out cover only execution safety (eval/exec/subprocess), or also unbounded computation (the factorial(2000000) case above, and the power towers guarded in feat(tasks): task-name variants, UGMathBench as _fixed, and grading off the shared event loop #68)?
2. if a task takes the hardened behaviour under its unqualified name, does the score impact still have to be quantified before it may ship as stable? (I would say yes — it is the same obligation _fixed carries, and it is the only thing keeping "hardened" from becoming an excuse.)

Known sites this would already apply to: theoremqa_kshot_base_gen (this issue), sieval/community/deepseek_math.py:317, and — already hardened under #68 — sieval/community/ugmathbench.py.

Related

• feat(tasks): task-name variants, UGMathBench as _fixed, and grading off the shared event loop #68 — introduces the variant convention, and fixes the same defect class (parse_expr on model output) in sieval/community/ugmathbench.py
• sieval/community/deepseek_math.py:317 — passes model output to parse_expr as well; vendored GPL upstream, so fidelity is the constraint there and it needs its own decision

Before submitting a new issue...

• Make sure you already searched for relevant issues and documentation.

[ethan-scitix]

Fixed in #83. Leaving this open for the convention decision, as the issue asks.

Reproduced

All three defects confirmed on main @ 1c15c00c. The escape fires through number_it — i.e. from an extracted model answer, not just from a hand-called _EVAL_GLOBALS — and also through answer_clean and compare_answer_with_groundtruth. factorial is unbounded. Both closed; factorial(2000000) now returns in 0.03 s.

The evaluator is an AST allowlist walk, not the parse_expr route the issue proposed. parse_expr is still an execution path — that is why ugmathbench needs a cleared namespace plus a quote screen plus a power-tower pre-parse. These three sites want a number out of a Python-ish expression, so a tree walk answers it directly and has no guard surface to get wrong.

Two premises did not survive measurement

Replayed a stored 800-sample run (Qwen2.5-72B, 5-shot greedy, full test set) through three readings: upstream's bare eval, the cleared-builtins eval shipping today, and the walk.

1. "The existing sandboxing is already a convention violation — an unquantified, score-affecting divergence." Now quantified: 0 samples. Upstream and the current task agree on 800/800, accuracy 44.625 both. The missing abs/round/pow are never reached, because latex2sympy runs first and its str() output never contains a Python builtin call. The sandbox was worth removing because it was escapable, not because it was scoring differently — and the notes claiming fidelity were wrong about the reason, not about the result.

2. "Closing the RCE properly makes the divergence larger." It does not — also 0, if you separate safety from repair:

	count
expressions reached across the 3 sites	706
both evaluate, identical value	586
both evaluate, different value	0
both fail (NameError / SyntaxError alike)	120
refused here but evaluated by upstream	0
bounds actually hit	0

Accuracy 44.625 under all three readings, and the patched task reproduces the stored report.json exactly over a real 8-worker pool.

That only holds because fidelity was kept wherever safety did not object. The temptation was to refuse set displays: latex2sympy folds a list answer like [0, 0] into {0}, and evaluating that set cements the corruption, so refusing it scores +2/800 (44.625 → 44.875, nothing moving right-to-wrong). That is a grader repair, not a safety measure — so it is deliberately not taken, the defect stays reproduced, and the +2/800 is recorded in the notes for whoever wants the _fixed. Same reasoning restored the bitwise operators.

This is what makes the carve-out cheap to argue: hardening cost nothing, because it was not allowed to smuggle a repair in with it.

The open decision

#68 has landed, so the step-2 gate is satisfied. Proposed wording is in #83 (sieval/tasks/CLAUDE.md + .claude/rules/tasks.md), close to the issue's draft, with the two sub-questions answered:

Q1 — execution only, or unbounded computation too? Both, on the same footing. Grading is synchronous and every runner shares one event loop, so an answer that grades forever is a session-wide stall rather than a slow sample. factorial(2000000) reached from model output is the same class of defect as open() reached from model output. Added obligation: every bound must be shown not to bind on the pinned data — otherwise a bound is a scoring change wearing a safety label. (Here: 0 of 706.)

Q2 — is the score impact still owed before shipping stable? Yes, agreed, and it is the load-bearing part. Alongside it I would add the constraint that actually did the work above: preserve upstream everywhere safety does not require otherwise, including where upstream is wrong. A grader defect noticed while hardening is still a _fixed and still owes its own number. Done properly the safety delta is near zero, so a large one is evidence a repair got smuggled in — which makes the rule self-checking rather than a matter of good faith.

Remaining known site: sieval/community/deepseek_math.py:317, still passing model output to parse_expr, vendored GPL upstream, needs its own decision — not touched here.

[ethan-scitix]

The third site listed here — sieval/community/deepseek_math.py — is fixed in #84 (stacked on #83). Two corrections to this issue's text, both of which changed what the fix had to be.

It is MIT, not GPL

vendored GPL upstream, so fidelity is the constraint there

DeepSeek-Math's code is MIT (LICENSE-CODE); GPL-3.0 is UGMathBench's judge_rule.py. Nothing here was license-constrained — the only constraint was the module's own byte-faithfulness claim, which is exactly what the carve-out says yields to safety.

The unbounded half is already handled; the RCE half was worse than described

Both consuming tasks (gsm8k_0shot_gen, hendrycks_math_kshot_base_gen) already route grading through run_cpu_bound with GRADE_TIMEOUT from #68, and deepseek's own call_with_timeout path is dead code — math_equal is only ever reached with timeout=False. So it is not a session stall. Executing in a worker process is still an RCE.

And there are two execution paths, not one. parse_expr is the one this issue names. The other is the raw-string fallback: when both parsers fail, upstream returns the answer verbatim, and it then reaches simplify(a - b) and N(a) — both of which sympify a string using sympy's own default namespace, not the caller's.

That second path defeats a fix to the first. Clearing the namespace doesn't reach it, and neither does the quote screen, because once __import__ resolves directly a payload needs no quote at all (__import__(chr(111)+chr(115))). Guarding only the parse would have moved the hole and looked like a fix. Both are closed: the parse is guarded, and an unparseable answer now refuses the comparison instead of being handed to sympify under another name.

Measured at zero, in the adversarial environment too

Full stored runs, both benchmarks, upstream reading vs guarded:

corpus	environment	upstream	guarded	differ
GSM8K 1319	parse_latex on	63.3813	63.3813	0
GSM8K 1319	parse_latex off	63.3055	63.3055	0
MATH 5000	parse_latex on	61.2600	61.2600	0
MATH 5000	parse_latex off	60.0200	60.0200	0

The off rows are the adversarial case: with the LaTeX parser dead, every comparison falls through to the guarded path — 1622 of 5000 on MATH hit the raw-string refusal specifically, and none changes a verdict.

The three guards are now shared in sieval/community/_sympy_guards.py rather than duplicated. That extraction is licensed by coupling — both graders hand model output to the same library under the same threat model, so a new escape route has to close in both — not by call count. The ugmathbench side is a pure move, its 55 tests unchanged.

Incidental: both stored runs encode a missing dependency

Worth flagging separately. Both runs were produced without antlr4-python3-runtime, so parse_latex raised into upstream's bare except: and the symbolic path silently never ran. Disabling it reproduces each stored report.json byte-exactly (MATH 60.0200, GSM8K 63.3055), which is how the cause was confirmed rather than guessed. Worth 1.24 pp on MATH, with nothing in any log to say so.

Already pinned in the [math] extra since #31 — main is correct, those runs just predate it — so no change was needed, but it is now recorded in both tasks' reference_impl.notes and pinned by tests, since an old low number is otherwise unexplainable.

---

Still open here after #84: only the convention carve-out itself, per this issue's own instruction not to close it with the RCE.