Follow-up from #264 (now merged). That PR validated the SpreadsheetBench task identifier and confined its persistent output directory. The same unconfined pattern remains in four other benchmark rollouts:
skillopt/envs/docvqa/rollout.py:173 and :232
skillopt/envs/livemathematicianbench/rollout.py:144
skillopt/envs/searchqa/rollout.py:204
skillopt/envs/officeqa/rollout.py:530
Each is os.path.join(out_root, "predictions", item_id), where item_id comes straight from the dataset item — no validation of the identifier and no containment check on the destination. An id such as ../../x therefore derives a destination outside out_root and processing continues into the model / code-execution path. skillopt/envs/spreadsheetbench/rollout.py now has _is_safe_task_id() and _confined_task_out_dir(), which could be lifted into a shared helper.
Two caveats are why this is filed rather than sent as a direct port:
-
A charset validator is not sufficient for livemathematicianbench. Its ids are colon-shaped (202602:12), so all 177 ids across the shipped splits would be rejected by the SpreadsheetBench rule. Those runs need confinement by construction (map an id to a safe directory name) rather than validate-and-reject. Separately, an id containing : cannot be a Windows directory name at all, so those runs are already broken on Windows for an unrelated reason.
-
Renaming the directory is not free. Readers look the task up by its raw id — skillopt/optimizer/slow_update.py:113 reads predictions/<task_id>/conversation.json — so any id-to-dirname mapping has to be applied on the read side too.
docvqa (63180), searchqa (hex) and officeqa (UID0003) use ids that are already safe shapes, so for those three the SpreadsheetBench approach can be applied as-is.
Filed separately because it is outside #264's reviewed scope.
Follow-up from #264 (now merged). That PR validated the SpreadsheetBench task identifier and confined its persistent output directory. The same unconfined pattern remains in four other benchmark rollouts:
skillopt/envs/docvqa/rollout.py:173and:232skillopt/envs/livemathematicianbench/rollout.py:144skillopt/envs/searchqa/rollout.py:204skillopt/envs/officeqa/rollout.py:530Each is
os.path.join(out_root, "predictions", item_id), whereitem_idcomes straight from the dataset item — no validation of the identifier and no containment check on the destination. An id such as../../xtherefore derives a destination outsideout_rootand processing continues into the model / code-execution path.skillopt/envs/spreadsheetbench/rollout.pynow has_is_safe_task_id()and_confined_task_out_dir(), which could be lifted into a shared helper.Two caveats are why this is filed rather than sent as a direct port:
A charset validator is not sufficient for
livemathematicianbench. Its ids are colon-shaped (202602:12), so all 177 ids across the shipped splits would be rejected by the SpreadsheetBench rule. Those runs need confinement by construction (map an id to a safe directory name) rather than validate-and-reject. Separately, an id containing:cannot be a Windows directory name at all, so those runs are already broken on Windows for an unrelated reason.Renaming the directory is not free. Readers look the task up by its raw id —
skillopt/optimizer/slow_update.py:113readspredictions/<task_id>/conversation.json— so any id-to-dirname mapping has to be applied on the read side too.docvqa(63180),searchqa(hex) andofficeqa(UID0003) use ids that are already safe shapes, so for those three the SpreadsheetBench approach can be applied as-is.Filed separately because it is outside #264's reviewed scope.