[Hrushikesh Ramilla (Hrushikesh-ramilla)]

Summary

Extend the task output confinement established in #264 and #309 to the ALFWorld benchmark rollout (skillopt/envs/alfworld/rollout.py).

Previously, run_alfworld_batch() joined unvalidated task_id values under predictions/ (conv_dir = os.path.join(pred_dir, task_id)), leaving ALFWorld as the only benchmark rollout in the repository that did not confine its persistent task outputs using confined_task_output_dir(). An unconfined or path-traversing task identifier (e.g. ../../escape) would write outside predictions/ and run without early validation.

Key Changes

1. Rollout confinement (skillopt/envs/alfworld/rollout.py):

   • Pre-validate all IDs in result_ids against confined_task_output_dir(out_root, str(rid)) upfront when out_root is provided, failing fast before environment reset, thread allocation, or target model calls.
   • Use confined_task_output_dir(out_root, task_id) when constructing conv_dir to ensure the conversation log is strictly confined within out_root/predictions/.
   • Specify encoding="utf-8" when writing conversation.json.

2. Unit Tests (tests/test_alfworld_task_output_confinement.py):

   • Added unit test coverage verifying that unconfined IDs (e.g. ../../escape) are rejected before environment reset or target model execution.
   • Verified that conversation outputs are saved to the expected confined predictions directory for custom and default (env_000) identifiers.
   • Verified symlink escape rejection and no-op behavior when out_root is unset.

Verification

• pytest tests/test_alfworld_task_output_confinement.py tests/test_alfworld_paths.py tests/test_spreadsheetbench_task_id_confinement.py -v: all 31 tests passed.
• ruff check skillopt/envs/alfworld/rollout.py tests/test_alfworld_task_output_confinement.py: all checks passed.
• mkdocs build --strict: passed with 0 warnings/errors.

[Yifan Yang (Yif-Yang)]

Thanks for extending the #264/#309 confinement to ALFWorld — the goal is right, but this cannot merge as is because it breaks real ALFWorld runs.

The ids shipped in data/alfworld_path_split are train:0000, val:…, test:… (all 191 contain a colon), and ALFWorldDataLoader._metadata_for_items passes them through as result_ids. confined_task_output_dir(out_root, rid) without map_unsafe=True rejects them, so with this change every run on the shipped split raises ValueError: unsafe task id: 'train:0000' before the environment resets. I reproduced this with the real shipped ids and dataloader metadata (only the simulator and model call substituted): it passes on current main and fails on this branch. The existing tests use task_001, which is why they do not catch it.

Suggested change, matching LiveMathematicianBench: rebase onto current main (which now includes #341), then call confined_task_output_dir(out_root, task_id, map_unsafe=True) for both the pre-validation and the write. The rebase matters: on this branch's base, reflect.py still reads only predictions/<raw id>, so mapped ALFWorld outputs would be invisible to the analysts, which is the #332 failure. With map_unsafe=True, an id such as ../../escape is mapped to a _mapped-<sha256> directory inside predictions/ rather than rejected, so test_alfworld_rollout_rejects_unconfined_task_ids_before_running needs to assert that confined write instead of a ValueError (the symlink-escape test still raises as before). Please also add a regression test that feeds ids from data/alfworld_path_split/train/items.json through the dataloader metadata into run_alfworld_batch and checks that the conversation is readable via fmt_minibatch_trajectories and slow_update._read_trajectory. As a side benefit, mapped segments avoid colon directory names, which are not valid on Windows. Happy to re-review once updated.

Reviewed head: d99708684beb against main bc0a392ef0a7 (current main is 51ae50161aa6).