Fix: port build_env to modular expert_harmonizer (restores main broken by #8)

[cmungall]

Why

main is currently broken. PR #8 (modular refactor) deleted
src/folds/ablate_monolith.py, but build_env.py (merged in #6) still did
from src.folds.ablate_monolith import .... Result on main:

ModuleNotFoundError: No module named 'src.folds.ablate_monolith'

build_env won't import and tests/test_build_env.py errors on collection — so
step 2 of the eval pipeline is non-functional. (This is the merge-order hazard
flagged in the #8 review: the refactor branch predated #6 and didn't update
build_env.)

Fix

Port build_env from the old monolith/ablate model to the modular harmonizer:

• Import kept_module_paths / kept_indices / block_indices / resolve_holdout
  from expert_harmonizer instead of the deleted ablate_monolith.
• The env now contains the held-out-free modular code: common.py + the kept
  dataset_NN.py modules, copied to data/gold/expert_code/harmonize_sm/ — the
  path the refactored harmonizer skill now reads (Refactor expert harmonizer from monolith to modular package #8 changed its
  context_dependencies). Held-out modules are simply not copied (ablation by
  omission). This replaces writing a single ablated notebooks/…py.
• Filtered exemplar mapping, skills copy, metadata symlink, MANIFEST.json
  (held-out dataset_identifiers for trace audit), and AGENT_INSTRUCTIONS.md
  are unchanged in spirit; instructions text updated to the new code path.

Verification

• Full suite green: 36 passed + 2 doctests (build_env + expert_harmonizer).
• Real smoke: --holdout 1,2,3,6,16,27 → env harmonize_sm/ has common.py +
  the 13 kept dataset modules; held-out modules and mapping entries absent.

Unblocks the pipeline so step 3 (runner) can build on it.

[claude]

Claude finished @cmungall's task —— View job

---

PR Review: Fix build_env port to modular expert_harmonizer

• Gather context and read changed files
• Review code quality and best practices
• Check for potential bugs or issues
• Review test coverage
• Post final feedback

---

Summary

This PR correctly fixes the broken main by porting build_env.py from the deleted ablate_monolith model to the new modular expert_harmonizer. The change is straightforward, well-scoped, and the approach (ablation by omission rather than text-splicing) is cleaner than the old approach. No blocking issues found.

---

Code Quality & Best Practices

src/folds/build_env.py

✅ Clean import of kept_module_paths, kept_indices, block_indices, and resolve_holdout from expert_harmonizer — exactly the right decomposition.

✅ Fail-fast pattern at build_env.py:97: kept_module_paths() is called before any filesystem writes, which correctly prevents partial env creation when a holdout index is invalid. Good defensive design.

✅ The docstring at the top is thorough and accurate about the new ablation model.

Minor nit — build_env.py:177: block_indices(env / HARMONIZER_REL) is called on the already-copied destination directory just to report a count. This works, but since kept (line 98) already holds the exemplar indices, len(kept) would be more direct and avoids an unnecessary disk scan:

# current (rescans copied dir)
f"{len(block_indices(env / HARMONIZER_REL))} exemplar modules remain"

# simpler
f"{len(kept)} exemplar modules remain"

---

Potential Bugs / Issues

common.py existence is assumed, not validated (build_env.py:109). kept_module_paths() always prepends common.py (expert_harmonizer.py:78), but if the package dir is missing common.py, shutil.copy will raise a FileNotFoundError mid-copy — after the env directory has already been created and the old env has been deleted (build_env.py:103-104). This is an unlikely edge case in practice, but it's a silent partial-state risk. A pre-check or at least a descriptive error would be safer.

held_ids can contain None (build_env.py:122-123): idx_to_dsid.get(i) returns None if the index doesn't exist in the mapping (e.g. holdout indices not present in the mapping). The MANIFEST.json will then include null entries in holdout_identifiers. The _instructions function does filter these out with if i (build_env.py:141), but the manifest silently contains nulls. A warning or explicit None filtering in the manifest would improve traceability.

---

Performance

No concerns. The shutil.rmtree + shutil.copytree pattern is fine for the scale of this pipeline. No hot paths.

---

Security

No concerns. No secrets handled. Symlink uses .resolve() which is correct. The sandbox escape caveat is honestly documented in the module docstring.

---

Test Coverage

✅ Tests are well-structured and cover the main scenarios:

• test_filter_mapping_drops_holdout_keeps_index — unit test for mapping filter
• test_default_name_is_sorted — stability of sort
• test_build_env_layout_and_content — full layout check including holdout omission, manifest, instructions
• test_build_env_metadata_symlink — symlink path
• test_build_env_rejects_holdout_without_module — the key guard (holdout without module → ValueError)
• test_build_env_overwrites_existing — idempotency / clean rebuild
• test_cli_builds_env — CLI integration

Gap: No test for the held_ids contains None case (holdout index absent from mapping). The manifest would contain null in holdout_identifiers without any error or warning, which could confuse downstream trace audits.

needs_pkg skip guards are appropriate for CI environments without the real data.

---

Verdict

✅ Approved in spirit — this is a correct, clean fix that unblocks the pipeline. The two minor issues (unnecessary block_indices rescan in the echo, and silent null in manifest) are non-blocking. The test suite is solid.