Add targeted grouped cross-validation folds

[hmworsham]

Summary

• define one target fold for each included dataset
• retain full-cluster reference ablation for clustered targets while staging and harmonizing only one target
• make target-only instructions unambiguous and hide reference-holdout identities from the agent
• update README and manuscript methods for targeted grouped CV
• strengthen cross-validation configuration invariants
• extract and test GitHub workflow fold resolution, including safe dispatch-output handling

Testing

• PYENV_VERSION=bio-3.11 python -m pytest tests/test_resolve_fold.py tests/test_cv_folds.py tests/test_build_env.py tests/test_invigilator.py -o addopts="" (37 passed)
• workflow YAML parsed successfully

[claude]

Claude finished @hmworsham's task in 2m 55s —— View job

---

Review: Add targeted grouped cross-validation folds

Nice restructuring overall. Deriving the exemplar pool from reference_holdout_datasets instead of hand-maintaining exemplar_pool lists removes a whole class of silent errors — and the old config had them: fold_id: 11 held out 23 but listed 23 in its own exemplar pool, and folds 11–13 all dropped 18 from their pools by accident. That failure mode is now structurally impossible. The target ⊆ holdout invariant in build_env is the right guard, and the new --target option is backward compatible by defaulting to holdout.

Below, roughly in order of impact.

---

1. AGENT_INSTRUCTIONS.md prose still tells the agent to harmonize all held-out datasets

src/folds/build_env.py:195-227 adds the target/reference-holdout split, but the surrounding prose was not updated:

• L199: "# Run environment — leave-one-cluster-out"
• L203: "Harmonize the held-out dataset(s) using ONLY:"
• L214: "Do NOT look up the held-out dataset's existing harmonized output..."
• L220: "For each held-out dataset, use its existing index from MANIFEST.json"

For a cluster-1 fold the agent now reads "Harmonize the held-out dataset(s)", then a Reference holdout datasets list of six identifiers, and only one sentence buried in the last section says otherwise. The contract is self-contradictory in the exact place the PR calls "the authoritative contract for this fold evaluation". The likely failure is an agent attempting six harmonizations, finding raw data for only one (since stage_indices now stages only the target), and burning the 60-minute cap writing "missing inputs" notes for the other five — or worse, emitting partial outputs for datasets that are then scored.

Allowed inputs and Required outputs should say target dataset(s) throughout. Fix this →

2. The reference-holdout identifiers are now disclosed to the agent

build_env.py:191 passes held_ids into the instructions and MANIFEST.json carries holdout_identifiers for every cluster member. Before this PR that was harmless — holdout was the target set, so the agent needed those names. Now, for a cluster-1 fold, the agent is handed the identifiers of five datasets that were deliberately ablated from its exemplar pool, and it has no task-related use for them.

That is a small but real widening of the isolation surface the rest of this repo works hard to keep narrow (the module docstring calls the design "isolation by absence"). Identifiers like ess-dive_<site>_<instrument> carry structural hints, and naming them invites the agent to reason about — or try to reach for — data that was removed on purpose. Consider listing only the count, or dropping the section from AGENT_INSTRUCTIONS.md and keeping the identifiers in MANIFEST.json only if the scorer needs them. If they're kept, at minimum they shouldn't be presented as a list the agent is invited to act on.

3. Test-coverage regression unrelated to this PR

tests/test_build_env.py:82-85 deletes three assertions:

assert claude_settings["sandbox"]["failIfUnavailable"] is True
assert claude_settings["sandbox"]["allowUnsandboxedCommands"] is False
assert claude_settings["sandbox"]["filesystem"]["denyRead"] == ["/"]

What remains is assert claude_settings == FOLD_CLAUDE_SETTINGS — but FOLD_CLAUDE_SETTINGS is imported from the module under test (L11), so that comparison only proves the file was serialized from the constant. It asserts nothing about the constant's values. The deleted lines were the only regression guard on the sandbox posture; after this change, flipping allowUnsandboxedCommands to True in build_env.py:84 passes the suite green. Nothing in this PR touches sandbox settings, so I'd restore them. Fix this →

4. Docs still describe the 13-fold design

The CV plan changed shape but the prose describing it did not:

• README.md:~200 — "Supply a dataset index, dataset identifier, comma-separated indices, or a cluster ID/name." Still accurate on syntax, but omits the new and non-obvious behaviour: a bare index now silently expands to a cluster-wide reference holdout. The local example (--holdout 15,26 --name fold-02-holdout-15-26) is now a legacy multi-target run that no longer corresponds to any configured fold (15 and 26 are folds 7 and 8).
• docs/manuscript/03_methods.md:114-120 — "datasets sharing a lab, site, or instrument family are held out together ... For each fold, the held-out dataset(s) are harmonized". Half of that is still true (they're still ablated together) and half is now wrong (only one is harmonized).
• src/folds/build_env.py:1-30 module docstring — same, describes holdout as the thing being evaluated.

For a repo whose docs/manuscript/ is the paper, the methods section is worth fixing in this PR rather than at results time.

5. Statistical note: the 19 folds are not independent replicates

Worth stating explicitly somewhere, because the config no longer makes it visible: folds 1–6 share an identical exemplar pool (all of cluster 1 ablated), as do folds 7–8. So six of the nineteen scores are drawn from one ablation condition on datasets that were grouped precisely because they're correlated. That's a defensible design — it's more evaluation points at no leakage cost — but any aggregate CI or significance test that treats n=19 as independent will be overconfident. Cluster-level aggregation, or a mixed model with cluster as a random effect, is the safer default. Flagging now since the config change is what makes this tempting.

6. tests/test_cv_folds.py gaps

Good addition, and the coverage invariant is the right thing to assert. Three things it doesn't catch:

• cluster_3 folds are unchecked. The loop only validates reference_holdout_datasets for cluster_1/cluster_2. For an independent dataset the invariant is reference_holdout_datasets == [target_dataset]; without that, a typo widening fold 12 to [8, 9] passes.
• No check that the target belongs to its declared cluster. A fold saying target_dataset: 15, held_out_cluster: "cluster_1" would pass today.
• len(folds) == len(DATASET_INDICES) == 19 — the literal 19 fails the moment a dataset is added, even though sorted(targets) == sorted(DATASET_INDICES) (which already implies the length and uniqueness checks) still holds. I'd drop the magic number and the now-redundant len(set(targets)) line.

Fix this →

7. The workflow's resolve step is now the most logic-dense part of the change, and is untested

.github/workflows/run-eval.yml:69-117 grew a fold lookup, a cluster fallback, and a naming branch — none of it reachable from pytest. Extracting it into e.g. src/folds/resolve_fold.py (returning holdout, target, env_name) would let you test the mapping from "1" to holdout=1,2,3,6,16,27 / target=1 directly, and shrink the YAML heredoc to a single call. Two smaller things in there:

• if fold_id (L103) is a falsy test on an integer. Fold ids start at 1 today so it works, but fold_id: 0 would silently fall through to the cluster name. if fold_id is not None is what's meant.
• L88 uses configured["reference_holdout_datasets"] while neighbouring reads use .get(...). Fine given the config is repo-controlled and now test-covered, but the inconsistency reads as accidental.
• Unrelated to this PR but adjacent: holdout and the new target are written to $GITHUB_OUTPUT unsanitized when the input doesn't match a fold or cluster, so an input containing a newline can inject additional outputs (env_path=, build_root=). The env_name line right above it is carefully sanitized "in EVERY branch ... to avoid any injection." Low severity — workflow_dispatch requires write access — but the asymmetry is worth closing while you're in here.

I can't push changes to .github/workflows/ (GitHub App permissions don't allow workflow modifications), so those would need to be applied by hand.

8. Nits

• default_name() (build_env.py:108) still names envs after the holdout, so a local --holdout 1,2,3,6,16,27 --target 1 run without --name produces holdout-1-2-3-6-16-27 — indistinguishable across all six cluster-1 folds, and each overwrites the last (L146-147 rmtree). Defaulting to the target set, or including it, would be safer.
• The name input still silently overrides the new fold-NN-target-N naming; probably intended, but it means an operator supplying name loses the fold id in the artifact and eval/* branch.
• tests/test_cv_folds.py reads Path("config/cv_folds.yaml") relative to CWD. Consistent with test_build_env.py's existing Path("data/gold/expert_code"), so fine — just noting it ties the suite to being run from the repo root.

---

Verification: I could not execute the test suite here — pytest was blocked by command approval in this environment, so I have not independently confirmed the PR's stated test run. All findings above are from reading the code.

Nothing here is a blocker except (1), which I'd want fixed before running folds 1–8, since it directly affects what the agent under evaluation is told to produce.
· codex/targeted-grouped-cv-folds

[claude]

Claude finished @hmworsham's task in 3m 30s —— View job

---

Re-review: Add targeted grouped cross-validation folds

Re-reviewed at 0f58931. Finding #1 from my previous pass is fixed — _instructions() now says "Harmonize only the target dataset(s)", "a target dataset's existing harmonized output", and "For each target dataset", and test_build_env_separates_target_from_reference_holdout pins all three with negative assertions. That was the blocker; it's closed.

But the same contradiction survives one level down, in files that are copied verbatim into every fold environment. That's the lead item below. The rest is prior findings, with status.

---

1. The skills shipped into the env still direct the agent from the holdout fields

build_env.py copies skills/ verbatim into the run environment (L163), and both skill files still describe the pre-PR contract:

skills/essdive_sm_curator/SKILL.md:93-94:

Outside fold evaluation mode, normalize inputs to package identifiers using the ESS-DIVE API. In fold evaluation mode, use the dataset identifiers in MANIFEST.json directly.

skills/essdive_sm_harmonizer/SKILL.md:77:

The held-out dataset's canonical index is listed in MANIFEST.json and must be retained in its mapping entry

MANIFEST.json now carries four identity fields — holdout_indices, holdout_identifiers, target_indices, target_identifiers. For a cluster-1 fold the first two contain six entries and the last two contain one. Neither skill names a field, so "the dataset identifiers in MANIFEST.json" is ambiguous at exactly the point where the ambiguity costs six harmonizations instead of one.

The curator is the worse of the two, because it's the entry point that decides which datasets enter the pipeline. Both skills do defer to AGENT_INSTRUCTIONS.md ("it is authoritative and overrides this skill's generic workflow"), so this is weaker than the original #1 — but the deferral is about the workflow, and an agent resolving "which identifiers" naturally reaches for the field whose name matches the sentence it just read. The failure mode is the one I described last time: six curation passes, raw data staged for one (stage_indices is target-only now), five sets of "missing inputs" notes against the 60-minute cap.

Fix is small — name the fields explicitly: target_identifiers in the curator, target_indices in the harmonizer. Fix this →

2. A dataset_identifier input silently skips the cluster ablation

.github/workflows/run-eval.yml:84 gates the fold lookup on raw.isdigit():

if raw.isdigit():
    configured = next((f for f in folds if f.get("target_dataset") == int(raw)), None)

So 1 resolves to fold 1 and holds out all of cluster 1, but the dataset identifier for the same dataset falls through to holdout = target = raw — a single-dataset holdout with datasets 2, 3, 6, 16 and 27 left in the exemplar pool. Two ways of naming one dataset now produce materially different ablation, and the run that leaks looks structurally identical to the one that doesn't: same env layout, one target, one staged raw input, clean invigilator report. Nothing downstream would catch it.

This is not a regression — pre-PR, a bare index didn't expand to its cluster either, so both paths were equally leaky and the operator was expected to type cluster_1. The PR fixes the index path and leaves the identifier path behind, which is what makes the asymmetry newly dangerous. The input description (L20-23) doesn't mention identifiers at all now, but the holdout option still accepts them.

Resolving the identifier to an index via resolve_holdout before the fold lookup would close it. Fix this →

3. Sandbox regression assertions are still removed (unresolved from prior review)

tests/test_build_env.py:84 — the three deleted lines have not come back:

assert claude_settings["sandbox"]["failIfUnavailable"] is True
assert claude_settings["sandbox"]["allowUnsandboxedCommands"] is False
assert claude_settings["sandbox"]["filesystem"]["denyRead"] == ["/"]

What remains is assert claude_settings == FOLD_CLAUDE_SETTINGS, and FOLD_CLAUDE_SETTINGS is imported from the module under test (L11), so it only proves the file was serialized from the constant — it asserts nothing about the constant's values. Flipping allowUnsandboxedCommands to True at build_env.py:84 still passes the suite green. Nothing in this PR touches sandbox settings, so this is a coverage loss the PR doesn't need to take. Fix this →

4. Docs still describe the 13-fold, harmonize-the-whole-cluster design (unresolved)

Now with one addition I missed last time — README.md:84-92 is a copy-pasteable operator prompt that directly contradicts the generated AGENT_INSTRUCTIONS.md:

You are running a leave-one-cluster-out harmonization evaluation. […] Read AGENT_INSTRUCTIONS.md and MANIFEST.json. Harmonize the held-out dataset(s) using only skills/…

Plus the ones already flagged:

• README.md:96-98 — "AGENT_INSTRUCTIONS.md supplies the exact held-out identifiers"; README.md:190-200 — the invigilator example still uses fold-02-holdout-15-26, which no longer corresponds to any configured fold (15 and 26 are folds 7 and 8, and env names are now fold-NN-target-N).
• docs/manuscript/03_methods.md:112-116 — "datasets sharing a lab, site, or instrument family are held out together […] For each fold, the held-out dataset(s) are harmonized". First half still true, second half now wrong.
• src/folds/build_env.py:1-30 module docstring — describes the holdout as the thing being evaluated.
• .github/workflows/run-eval.yml:183 direct_prompt — "grouped leave-one-cluster-out". Weakest of these since it defers to AGENT_INSTRUCTIONS.md and names no datasets.

The methods section is the paper; worth fixing here rather than at results time. Fix this →

5. tests/test_cv_folds.py gaps (unresolved)

The two invariants it does assert are the right ones. Still uncaught:

• cluster_3 folds are unvalidated. The if cluster in {"cluster_1", "cluster_2"} branch has no else; for an independent dataset the invariant is reference_holdout_datasets == [target_dataset], and without it a typo widening fold 12 to [8, 9] passes.
• No check that a target belongs to its declared cluster. target_dataset: 15, held_out_cluster: "cluster_1" passes today.
• No check that reference_holdout_datasets ⊆ DATASET_INDICES. A stray cluster_4 index (0, 11–14, 19–22) would only surface as a kept_module_paths exception at build time, in CI, after uv sync.
• len(folds) == len(DATASET_INDICES) == 19 — the literal 19 breaks the moment a dataset is added, and sorted(targets) == sorted(DATASET_INDICES) already implies both the length and the uniqueness that len(set(targets)) re-checks on the next line.

Fix this →

6. Workflow resolve step: still the most logic-dense untested code (unresolved)

.github/workflows/run-eval.yml:69-117. Extracting it to src/folds/resolve_fold.py returning (holdout, target, env_name) would make the "1" → holdout=1,2,3,6,16,27 / target=1 mapping testable and shrink the heredoc to one call — and it's the natural home for the identifier fix in #2. Smaller points, all still present:

• L103 if fold_id is a falsy test on an integer; fold_id: 0 would fall through to cluster_id. if fold_id is not None is what's meant.
• L88 configured["reference_holdout_datasets"] vs .get(...) on the neighbouring lines — reads as accidental.
• L110-111 holdout and target go to $GITHUB_OUTPUT unsanitized when the input matches neither a fold nor a cluster, so a newline in the input can inject env_path= / build_root=. env_name right below is deliberately sanitized "in EVERY branch […] to avoid any injection"; the asymmetry is worth closing. Low severity — workflow_dispatch needs write access.

I can't push to .github/workflows/ (GitHub App permissions), so #2 and this one need to be applied by hand.

7. Reference-holdout identifiers are disclosed to the agent (unresolved — judgment call)

build_env.py:191 still passes held_ids into AGENT_INSTRUCTIONS.md. The new prose does the best available job of it ("The following datasets are excluded […] Only the target dataset(s) above should be harmonized"), so this is much softer than before. Still, for a cluster-1 fold the agent is handed five identifiers it has no task-related use for, in a design whose docstring calls isolation "by absence". ess-dive_<site>_<instrument> names carry structural hints.

Worth noting the field is not dead weight in MANIFEST.json — invigilator.py:378 reads holdout_identifiers and _reason() (L164-165) uses it to annotate violations as [references held-out <id>]. So the manifest should keep it; the question is only whether AGENT_INSTRUCTIONS.md needs the list rather than a count. Your call — flagging, not blocking.

8. Statistical non-independence (unresolved — note, not a code issue)

Folds 1–6 share an identical exemplar pool, as do 7–8. Six of nineteen scores come from one ablation condition, on datasets grouped precisely because they correlate. Defensible design — more evaluation points at no leakage cost — but an aggregate CI treating n=19 as independent will be overconfident. Cluster-level aggregation or a mixed model with cluster as a random effect is the safer default. The old config made this visible in the exemplar_pool lists; the new one doesn't, which is why it's worth writing down somewhere.

9. Nits (unresolved)

• default_name() (build_env.py:108, used at L145) still names from the holdout, so a local --holdout 1,2,3,6,16,27 --target 1 without --name yields holdout-1-2-3-6-16-27 for all six cluster-1 folds — and L146-147 rmtrees the previous one. CI always passes --name, so this only bites local runs. Including the target would fix it.
• The name input still overrides fold-NN-target-N, so an operator supplying it loses the fold id from the artifact and eval/* branch name. Probably intended.
• tests/test_cv_folds.py reads Path("config/cv_folds.yaml") relative to CWD — consistent with test_build_env.py's Path("data/gold/expert_code"), so fine, just ties the suite to the repo root.

---

Verification: I could not execute the test suite — pytest (both uv run and direct) was blocked by command approval in this environment, so the PR's stated test run remains unconfirmed by me. I did verify the config's coverage invariant by hand: config/cv_folds.yaml targets {1,2,3,4,5,6,7,8,9,10,15,16,17,18,23,24,25,26,27} exactly equals DATASET_INDICES (expert_harmonizer.py:33), 19 folds, no duplicates, and each cluster's datasets list matches the folds that reference it. All other findings are from reading the code.

Deriving the exemplar pool from reference_holdout_datasets remains the right call — it structurally eliminates the hand-maintenance bugs the old config had (fold 11 held out 23 while listing 23 in its own pool; folds 11–13 dropped 18 by accident).

Blocking-ish: #1, for the same reason the old #1 was — it changes what the agent under evaluation is told to produce on folds 1–8. #2 is the one I'd want fixed before anyone runs a fold by identifier.
· codex/targeted-grouped-cv-folds

[claude]

Claude finished @hmworsham's task in 2m 49s —— View job

---

Re-review after 0f58931 + 08870ff

• Read PR diff and the two new commits
• Verify prior findings (1) instruction prose and (2) identifier disclosure
• Re-check remaining findings (tests, docs, workflow)
• Attempt test run — blocked (pytest and python -c both require command approval in this environment; findings below are from reading the code, plus a manual read-through of config/cv_folds.yaml against DATASET_INDICES)
• Post updated review

---

Both blocking items from my last pass are resolved, and cleanly.

(1) instruction prose — _instructions() now says "Harmonize only the target dataset(s)", "a target dataset's existing harmonized output", "For each target dataset", and the header reads "targeted grouped leave-one-out". The contract is internally consistent. Nice touch that tests/test_build_env.py:97-99 asserts the negative forms are gone, not just that the new forms are present — that's the assertion that actually holds the line.

(2) identifier disclosure — AGENT_INSTRUCTIONS.md no longer lists the reference holdout, MANIFEST.json carries only reference_holdout_count, and tests/test_build_env.py:101 ("ess-dive_b" not in instructions) pins it. grep confirms nothing else in the repo read holdout_indices / holdout_identifiers from the manifest, so dropping those fields is safe.

I also read config/cv_folds.yaml against DATASET_INDICES (src/folds/expert_harmonizer.py:33) by hand: 19 targets, exactly matching the 19 indices, every target a member of its declared cluster, cluster_1/cluster_2 holdouts equal to the full cluster, and each cluster_3 holdout a singleton. The config is correct as written — the gaps in §3 below are about what the tests would catch, not about a live defect.

---

1. invigilator.py loses the ability to name a reach for an ablated sibling — and the info is now unrecoverable

src/folds/invigilator.py:382-384 now reads target_identifiers with holdout_identifiers as fallback. Detection is unaffected: check() sets report.clean = False before _reason() is ever called (L313-316), and the cd branch does the same (L350-354). The identifiers only feed the [references held-out <id>] suffix in _reason() (L164-165).

But that suffix is exactly the signal you'd most want on a cluster fold. If the agent on fold 1 reaches out of bounds for dataset 27's expert module, the audit will now report a bare "repo file outside env" instead of naming it as a reach for an ablated sibling. And because the manifest keeps only a count, nothing downstream can reconstruct the list.

The audit runs in the trusted workflow, outside the environment, so it doesn't have to learn this from a file the agent can read. An --holdout-identifier option on invigilator.main (populated in run-eval.yml from reference_holdout_datasets + the gold mapping) would restore full labelling with zero disclosure. Worth doing before folds 1-8 run, since those are the six where the distinction matters. Fix this →

Two smaller things in the same area:

• tests/test_invigilator.py:36 still writes a legacy {"holdout_identifiers": [...]} manifest, so the suite exercises only the fallback branch. The new primary key target_identifiers has no invigilator-level coverage — a typo there would be caught by test_build_env.py but not by the consumer.
• The local holdout_ids and the audit(holdout_identifiers=...) parameter now carry targets, not holdouts. The comment at L379-381 explains it, but if you add the CLI option above the parameter genuinely becomes "identifiers whose appearance is noteworthy" and could be renamed to match.

2. reference_holdout_count has no consumer

grep finds it only in build_env.py:179 and three test assertions. It's readable by the agent (MANIFEST.json is one of the two files the prompt tells it to open) and tells it "six datasets were removed". That's a much smaller surface than the old identifier list, and if the scorer will use it, keep it. If nothing will, dropping it costs nothing — the count is derivable from cv_folds.yaml on the trusted side.

3. Still open from the previous review

These are unchanged by the two new commits; recapping briefly rather than restating in full.

• Sandbox assertions still deleted (tests/test_build_env.py:82-83). claude_settings == FOLD_CLAUDE_SETTINGS is tautological — the constant is imported from the module under test (L12), so flipping allowUnsandboxedCommands to True in build_env.py:84 still passes green. This is the one I'd most like restored; it's unrelated to the PR's purpose and it's the only guard on the sandbox posture. Fix this →
• tests/test_cv_folds.py gaps — cluster_3 folds unvalidated, no check that a target belongs to its declared cluster, and the literal 19 on L17 is redundant with sorted(targets) == sorted(DATASET_INDICES) (as is len(set(targets)) on L19). Fix this →
• default_name() (build_env.py:108-114) still derives from the holdout, so all six cluster-1 folds collide on holdout-1-2-3-6-16-27 and each rmtrees the last (L146-147). Only bites local runs without --name, but it's a silent overwrite. Fix this →
• .github/workflows/run-eval.yml — the resolve step remains untested and unreachable from pytest; if fold_id (L103) is a falsy test on an integer; holdout/target reach $GITHUB_OUTPUT unsanitized while env_name right above them is carefully sanitized. I'm unable to modify files under .github/workflows/ — the GitHub App permissions don't allow workflow modifications (FAQ) — so these need a hand edit.
• n=19 is not 19 independent replicates — folds 1-6 share an identical exemplar pool, as do 7-8. Cluster-level aggregation or cluster-as-random-effect is the safer default for any aggregate CI.

4. Docs drifted further, not less

08870ff made one README claim actively false:

• README.md:95 — "AGENT_INSTRUCTIONS.md supplies the exact held-out identifiers and allowed paths for each fold." It no longer supplies identifiers for anything but the target; that was the point of the commit.
• README.md:88-89 — the quoted operator prompt still reads "Harmonize the held-out dataset(s) using only skills/, the filtered mapping, the non-held-out expert-code patterns" — verbatim the wording 0f58931 just removed from the real contract. Since this block is presented as the prompt to paste, it would re-introduce the exact ambiguity the commit fixed.
• README.md:170-193 — the worked example --holdout 15,26 --name fold-02-holdout-15-26 is now a legacy multi-target run matching no configured fold (15 and 26 are folds 7 and 8). A --holdout 1,2,3,6,16,27 --target 1 example would show the new shape.
• docs/manuscript/03_methods.md:114-120 — "For each fold, the held-out dataset(s) are harmonized" is now wrong for folds 1-8; the grouping half of the sentence is still right. This is the paper's methods section, so worth correcting in this PR.
• src/folds/build_env.py:1-30 module docstring and build_env's own docstring (L127) still describe the design as leave-one-cluster-out with the holdout as the evaluated set.

Fix this →

---

Summary. The core change is in good shape and the two follow-up commits fixed the right things in the right way. Nothing here blocks merge on its own; if I were sequencing, I'd take the invigilator labelling (§1) and the sandbox assertions (§3) before running folds 1-8, and the README/methods corrections (§4) whenever convenient — §4 is now a documented-behaviour mismatch rather than just staleness.

I made no code changes — this was a review pass only. I was not able to execute the test suite (command approval), so I have not independently confirmed the PR's stated test run.
· codex/targeted-grouped-cv-folds

[claude]

Claude finished @hmworsham's task in 3m 6s —— View job

---

Re-review: Add targeted grouped cross-validation folds

Three new commits since my last pass. Findings 1, 2, and 4 are resolved — and resolved well. Findings 3, 6, 7 are still open. Summary of the delta, then what's left.

---

Resolved

(1) Instructions prose — fixed. _instructions() (src/folds/build_env.py:194-223) is now internally consistent: "Harmonize only the target dataset(s)", "Do NOT look up a target dataset's existing harmonized output", "For each target dataset, use its existing index". The closing sentence ("Additional datasets may be absent from the exemplar references; they are not targets") pre-empts the failure mode I was worried about — an agent finding gaps in the exemplar pool and inferring extra work. And tests/test_build_env.py:96-99 locks it in with negative assertions on the old strings, which is exactly the right shape for a prose contract.

(2) Reference-holdout identifiers — fixed, and more thoroughly than I suggested. _instructions() no longer takes held_ids at all; MANIFEST.json carries reference_holdout_count: len(holdout) (build_env.py:180) instead of the identifier list. Cluster mates are now unnamed anywhere in the environment. tests/test_build_env.py:95,101 assert both the section and the identifier are absent. Two follow-on notes below.

(4) Docs — fixed. docs/manuscript/03_methods.md:113-121 now correctly separates ablation ("removed from the exemplar pool together") from the task ("stages and harmonizes only its target dataset"), the module docstring at build_env.py:1-31 was rewritten, and the README example moved to --holdout 15,26 --target 15 --name fold-07-target-15, which is now a real configured fold. README.md:196-200 also documents the non-obvious index→cluster-holdout expansion that I flagged.

I also hand-verified the config: cluster_1 ∪ cluster_2 ∪ cluster_3 = the 19 entries of DATASET_INDICES, each is the target of exactly one fold, every target is a member of its declared held_out_cluster, and cluster_3 folds hold out only themselves. The invariant holds.

---

Still open

3. Sandbox assertions still deleted _(repeat)

tests/test_build_env.py:82-83 is now just:

claude_settings = json.loads((env / CLAUDE_SETTINGS_REL).read_text())
assert claude_settings == FOLD_CLAUDE_SETTINGS

FOLD_CLAUDE_SETTINGS is imported from the module under test (L11), so this proves serialization round-trips and nothing about the values. Flipping allowUnsandboxedCommands to True at build_env.py:85 still passes green. Nothing in this PR touches sandbox posture, so this remains a gratuitous coverage loss. Fix this →

6. tests/test_cv_folds.py gaps _(repeat)

Unchanged since my last read. The three gaps still stand: no else branch validating cluster_3 folds (reference_holdout_datasets == [target_dataset]), no check that target_dataset ∈ clusters[held_out_cluster]["datasets"], and the hardcoded 19 at L17 plus the redundant len(set(targets)) at L19 (both implied by sorted(targets) == sorted(DATASET_INDICES)). All three invariants currently hold — I checked by hand — but nothing keeps them holding. Fix this →

7. Workflow resolve step _{(repeat — I can't push to .github/workflows/)}

if fold_id (L103) is still a falsy integer test; configured["reference_holdout_datasets"] (L88) is still the lone non-.get read; holdout and target are still written to $GITHUB_OUTPUT unsanitized (L110-111) while env_name one line up is carefully sanitized. One new ordering note now that both branches exist: the cluster loop (L93-98) runs after the fold lookup and unconditionally does target = holdout, without clearing fold_id. Unreachable today — a digit raw can't equal cluster_N or any configured cluster name — but it's an implicit invariant nothing enforces. The extraction into a testable src/folds/resolve_fold.py would make all four of these disappear.

---

New, from this round of changes

A. The invigilator can no longer attribute a violation to an ablated cluster mate

invigilator.py:382-384 now reads target_identifiers with holdout_identifiers as fallback. That's the correct adaptation — but note what it costs. Those identifiers only feed _reason() (invigilator.py:164-165), which appends [references held-out <id>] to an already-detected violation. Detection is path-based and unaffected, so this is not a leak-detection gap. It is an attribution gap: in a cluster-1 fold, an agent reaching outside the env for dataset 16's expert module is still caught, but the report says "outside allowed roots" rather than naming the ablated sibling — which is precisely the case a reviewer most wants labelled.

The identifiers genuinely can't live in MANIFEST.json anymore (that's finding 2's whole point), but the invigilator runs outside the agent boundary, from $RUNNER_TEMP, after the checkout is destroyed. An optional --holdout-id repeatable flag on invigilator main(), populated from the resolve step's holdout output, would restore the annotation without re-exposing anything to the agent. Worth a follow-up issue if not this PR.

Related nit: the local holdout_ids at invigilator.py:375 and the holdout_identifiers= parameter name are now misnomers — they hold target identifiers. The comment explains it, but renaming to answer_identifiers would say it directly.

B. Ablated indices remain inferable from exemplar_indices

MANIFEST.json still publishes exemplar_indices (build_env.py:181), and the filtered mapping carries each surviving entry's canonical index, so the gaps in the sequence disclose which indices were removed. Identity-hiding here is by identifier, not by index. That's a real improvement — an index carries none of the site/instrument structure that ess-dive_<site>_<instrument> does — but it's worth stating explicitly rather than leaving the impression the holdout is fully opaque. exemplar_indices is load-bearing (the agent must retain canonical indices), so I wouldn't remove it; just don't over-claim.

C. Skill text still says "held-out dataset's canonical index"

skills/essdive_sm_harmonizer/SKILL.md:77 — "The held-out dataset's canonical index is listed in MANIFEST.json" — and skills/essdive_sm_curator/SKILL.md:94, "use the dataset identifiers in MANIFEST.json directly". The skills are copied verbatim into the env, so the agent reads "held-out" in the skill and "target" in AGENT_INSTRUCTIONS.md for the same thing. AGENT_INSTRUCTIONS.md is declared authoritative and the keys it points at do exist (target_indices, target_identifiers), so this resolves correctly — but it's the same terminology drift finding 1 fixed, one file over. SKILL.md:475-476 has it too.

Also stale, cosmetic: build_env.py:128 ("one leave-one-cluster-out config"), build_env.py:226 (Typer help), and the workflow step names Build leave-one-cluster-out run environment / Stage raw data for held-out dataset(s) — the latter now stages only the target. Fix this →

D. A target missing from the mapping produces an empty target list, silently

build_env.py:174-175: target_ids = [idx_to_dsid.get(i) for i in sorted(target)], and _instructions() (L195) filters falsy ids out of the bullet list. Validation upstream (kept_module_paths → _validate_holdout) checks that hold-out indices have a module on disk, not a mapping entry. So if expert_code/ and the gold mapping ever diverge for one index, the fold builds cleanly, MANIFEST.json records "target_identifiers": [null], and AGENT_INSTRUCTIONS.md ships a "## Target datasets" heading with nothing under it — the agent is told to harmonize only the targets listed, and none are. Cheap guard:

if any(i is None for i in target_ids):
    missing = [i for i, d in zip(sorted(target), target_ids) if d is None]
    raise ValueError(f"target indices absent from mapping: {missing}")

Low likelihood, but the instructions are now the sole channel telling the agent what to do, so a silent empty list is worse than it was before this PR. Fix this →

E. default_name() still collides across the six cluster-1 folds _{(repeat nit)}

build_env.py:109-115 names by holdout, so --holdout 1,2,3,6,16,27 --target 1 and --target 27 both produce holdout-1-2-3-6-16-27, and L147-148 rmtrees the previous one. CI always passes --name, so this only bites local runs — but the README's own workflow is local, and it now has six folds sharing a holdout. Including the target set in the default would close it.

F. Statistical note _{(repeat, informational)}

Folds 1–6 share an identical exemplar pool, as do 7–8. Six of nineteen scores come from one ablation condition, on datasets grouped precisely because they correlate. Defensible design — more evaluation points at no leakage cost — but n=19 independent replicates would be overconfident. Cluster-level aggregation or a mixed model with cluster as a random effect is the safer default when results are written up.

---

Verification: pytest was again blocked by command approval in this environment, so I have not run the suite; the PR's stated test run is unconfirmed by me. I did verify the config/cv_folds.yaml invariants by hand against DATASET_INDICES (src/folds/expert_harmonizer.py:33) — 19 folds, 19 unique targets, exact cluster coverage, every target inside its declared cluster, cluster_3 folds self-holding. All other findings are from reading the code.

Bottom line: the blocker from my last review (finding 1) is fixed, and finding 2 was addressed better than proposed. Nothing remaining blocks running folds 1–8. I'd take (3) before merge since it's a two-line restoration of a security regression guard, and file (A) as a follow-up.
· codex/targeted-grouped-cv-folds

[claude]

Claude finished @hmworsham's task in 2m 45s —— View job

---

Re-review: Add targeted grouped cross-validation folds

• Read repo guidelines and PR diff
• Verify status of prior findings (1) instructions prose and (2) holdout identifier disclosure
• Re-check remaining findings (tests, docs, workflow)
• Attempt test suite (blocked — see Verification)
• Post updated review

Four commits landed since my last pass. The blocker is resolved and the fixes are cleaner than what I suggested. Status of every prior finding, then what's new.

---

Resolved

(1) Instructions prose — fixed, and better than asked. _instructions() now reads "Harmonize only the target dataset(s) listed below", "Do NOT look up a target dataset's existing harmonized output", "For each target dataset, use its existing index", under a ## Target datasets heading, plus an explicit disambiguating sentence: "Additional datasets may be absent from the exemplar references; they are not targets." That last line is the one that actually closes the failure mode — an agent noticing gaps in the exemplar pool now has a stated reason not to treat them as work. tests/test_build_env.py:96-99 asserts the negatives ("Harmonize the held-out dataset(s)" not in instructions), so the prose can't silently regress.

I also confirmed .github/workflows/run-eval.yml:183-192 — the CI direct_prompt enumerates nothing and defers entirely to AGENT_INSTRUCTIONS.md, so there's no second contradictory contract on the CI path. Good.

(2) Identifier disclosure — fixed. MANIFEST.json now carries target_indices / target_identifiers / reference_holdout_count; cluster-mate identifiers are gone from both the manifest and the instructions. test_build_env_separates_target_from_reference_holdout pins it with assert "ess-dive_b" not in instructions. The invigilator.py:380-384 compatibility fallback for previously-staged runs, with a comment explaining why, is the right call given eval/* branches persist old manifests.

(4) Docs — fixed across all three. 03_methods.md:113-121 now distinguishes ablation from targeting correctly ("removed from the exemplar pool together … stages and harmonizes only its target dataset"); the build_env.py module docstring is rewritten; README.md swaps the legacy --holdout 15,26 example for --holdout 15 --target 15 --name fold-07-target-15 and documents the bare-index expansion.

(6) test_cv_folds.py — all three gaps closed. cluster_3 else-branch, target-membership-in-cluster assertion, and the literal 19 is gone. I verified DATASET_INDICES (expert_harmonizer.py:33) is exactly the 19 targets in the config, so sorted(targets) == sorted(DATASET_INDICES) holds.

---

Still open

(3) Sandbox assertions still missing. tests/test_build_env.py:83 is now the only check: assert claude_settings == FOLD_CLAUDE_SETTINGS, where the constant is imported from the module under test (L11). Flipping allowUnsandboxedCommands to True in build_env.py still passes green. This is the only thing I'd call a regression rather than a nit — it's unrelated to the PR's purpose and removes the only guard on the isolation posture the whole design rests on. Fix this →

(7) Workflow resolve step. Unchanged, and I can't edit .github/workflows/ (GitHub App permissions), so these need a hand-applied patch:

• run-eval.yml:103 — if fold_id is a falsy test on an int; if fold_id is not None is what's meant.
• run-eval.yml:110-111 — holdout and target go to $GITHUB_OUTPUT unsanitized, while env_name on L104 is carefully scrubbed "in EVERY branch … to avoid any injection". A newline in a non-matching input injects extra outputs. Low severity (workflow_dispatch needs write access), but the asymmetry is the kind of thing that looks deliberate later.
• L88 configured["reference_holdout_datasets"] vs .get() on the neighbouring lines — now test-covered by test_cv_folds.py, so it's fine, just inconsistent.
• The whole block is still unreachable from pytest. Extracting src/folds/resolve_fold.py returning (holdout, target, env_name) would make "1" → holdout=1,2,3,6,16,27 / target=1 / fold-01-target-1 directly testable and shrink the heredoc to one call. Optional, but this is now the densest untested logic in the change.

(8) default_name() (build_env.py:108) still derives from the holdout, so a local --holdout 1,2,3,6,16,27 --target 1 with no --name yields holdout-1-2-3-6-16-27 for all six cluster-1 folds — each rmtrees the previous. CI always passes --name, so this is local-only, but the README now teaches the --target flag without mentioning that --name is effectively mandatory for cluster folds.

(5) Statistical note (advisory, unchanged): folds 1–6 share one ablation condition and folds 7–8 share another, so n=19 is not 19 independent replicates. Cluster-level aggregation or cluster-as-random-effect is the safe default at analysis time.

---

New this round

(a) The invigilator's leakage attribution is now weaker for the 8 cluster folds. invigilator.py:380-384 feeds only target_identifiers into audit(holdout_identifiers=...), which flows to _reason() (invigilator.py:155-165). Detection is unaffected — it's root-based, so an agent reaching for a cluster mate's data outside the env is still flagged — but the violation loses its [references held-out <id>] annotation. For a cluster-1 fold that's five of six ablated datasets whose leakage now reports as a bare "repo file outside env".

The fix can't be "put them back in MANIFEST.json" — the manifest lives inside the sandbox, which would undo finding (2). The invigilator runs outside the boundary, so it can take them from the trusted side: a --holdout-ids option, or resolving config/cv_folds.yaml + the full mapping directly. Forensic quality only, not a correctness bug. Fix this →

(b) The new target_identifiers manifest path is untested. tests/test_invigilator.py:36 still writes only the legacy holdout_identifiers key, so the fallback is covered but the preferred branch isn't. One extra fixture would cover both.

(c) test_cv_folds.py:26 special-cases the string "cluster_3". A future second independent-dataset cluster would land in the else branch and be required to hold out its whole membership. Keying on len(clusters[cluster]["datasets"]) == 1 — or asserting the general invariant (target ∈ reference_holdout ⊆ cluster.datasets) — would survive a config reshuffle.

(d) Residual inference channel, noted not blocking. MANIFEST.json still exposes exemplar_indices, so an agent can subtract to learn which indices were ablated even though it can't resolve them to identifiers. That's a real reduction from before (identifiers carry site/instrument semantics; bare indices don't), so I'd leave it — just recording that reference_holdout_count narrows the channel rather than closing it.

(e) Stale terminology, cosmetic. run-eval.yml:3,5,9,119,129,183, README.md:6,84, and build_env.py:128,226 still say "leave-one-cluster-out" / "held-out dataset(s)" where the design is now targeted grouped LOO. None of it reaches the agent as instructions — the CI prompt defers to AGENT_INSTRUCTIONS.md, which is correct — so this is naming hygiene, worth a sweep whenever .github/ is next touched by hand.

---

Verification: pytest is still blocked by command approval in this environment, so I have not executed the suite; findings are from reading the code, plus a manual check that the 19 configured targets match DATASET_INDICES exactly.

Assessment: The blocker from my last review is fixed properly. Nothing remaining blocks running folds 1–8. Of what's left, only (3) is a regression introduced by this PR rather than a pre-existing gap or a nit, and it's a two-line restore.
· codex/targeted-grouped-cv-folds

[claude]

Claude finished @hmworsham's task in 2m 49s —— View job

---

Re-review: Add targeted grouped cross-validation folds

Five commits since my last pass. Most of what I raised is addressed, and the workflow extraction in particular came out better than I sketched — resolve_fold.py closes the injection asymmetry at the input boundary (_SAFE_INPUT.fullmatch rejects the newline before anything reaches $GITHUB_OUTPUT), which is stronger than sanitizing the outputs individually. test_fold_id_zero_still_selects_fold_name is a nice touch.

---

Status of prior findings

#	Finding	Status
1	Instructions prose said "harmonize the held-out dataset(s)"	Fixed (build_env.py:212-222), with negative assertions in test_build_env.py:96-99
2	Reference-holdout identifiers disclosed to agent	Fixed — manifest now carries reference_holdout_count only; assert "ess-dive_b" not in instructions
3	Sandbox assertions deleted	Not addressed — see below
4	Docs described the 13-fold design	Fixed — README, 03_methods.md, module docstring
5	Statistical non-independence	Withdraw. 03_methods.md:206-216 already prescribes cluster-bootstrap CIs and (1|dataset) + (1|cluster). I missed that section last time; the config change doesn't undermine it
6	test_cv_folds.py gaps	Fixed — cluster_3 branch, target-in-cluster membership, magic 19 dropped
7	Untested workflow resolve logic	Fixed — extracted with tests; fold_id is not None; .get() consistency; injection closed
8	default_name() names envs after holdout	Not addressed

---

Remaining and new

1. A dataset_identifier input silently bypasses cluster ablation (new, medium)

resolve_fold.py:44 gates the fold lookup on raw.isdigit(). The workflow input still advertises identifiers as a supported "legacy" form, and build_env --holdout <identifier> still accepts them. So dispatching ess-dive_<a cluster-1 member> resolves to holdout = target = <that identifier> — cluster mates 2, 3, 6, 16, 27 stay in the exemplar pool. That is exactly the near-duplicate leak config/cv_folds.yaml exists to prevent, and it produces a plausible-looking fold with no warning.

It's partly self-limiting: stage_indices drops non-digit tokens, so no raw data stages and the run fails on missing inputs (assuming stage_raw). But that's an accident of the digit filter, not a guard.

The cheap fix is to resolve identifiers to indices via the mapping before the fold lookup, so an identifier for a configured target gets the same treatment as its index. The cheaper one is to reject a non-digit, non-cluster input outright unless it contains a comma. Either way test_resolve_fold.py should cover the identifier path and the non-configured-index path ("11", "30"), neither of which is exercised today. Fix this →

2. Sandbox assertions still missing (carried over)

tests/test_build_env.py:83 is now just assert claude_settings == FOLD_CLAUDE_SETTINGS, and FOLD_CLAUDE_SETTINGS is imported from the module under test — the comparison proves serialization, not posture. Flipping allowUnsandboxedCommands to True in build_env.py still passes green. Nothing in this PR touches sandbox settings; the three deleted lines were the only regression guard on the isolation boundary. Fix this →

3. default_name() still keys on the holdout (carried over)

build_env.py:108-115 returns holdout-<ids>, so all six cluster-1 folds run locally without --name collapse to holdout-1-2-3-6-16-27 and each rmtrees the previous one (L146-147). CI is safe — resolve_fold always supplies fold-NN-target-N — but the local path in README §"Run a fold locally" now passes --target without any hint that --name is load-bearing. Including the target set in the default would close it. Fix this →

4. The cluster loop can still clobber a resolved fold (new, nit)

resolve_fold.py:53-58 runs unconditionally after the digit branch, resetting fold_id = None and widening target back to the whole cluster. Unreachable today — no cluster id or name is all-digits — but the two branches are mutually exclusive by intent and don't say so. elif or an early return would make that structural rather than incidental.

5. The resolve step no longer logs its resolution (new, nit)

The old heredoc ended with print(f"Resolved holdout={holdout!r} env_name={env_name!r}"). main() now sends every line to stdout, which the workflow redirects wholesale into $GITHUB_OUTPUT, so the step produces no visible log and a future diagnostic print would corrupt the output file. Writing a one-line summary to stderr restores it safely.

6. Invigilator annotation narrowed (new, low)

invigilator.py:381-384 now feeds only target_identifiers into audit(). That list is used solely by _reason() (L164) to annotate an already-detected violation, so a cluster mate's expert file at repo root is still flagged — just as "repo file outside env" rather than "[references held-out X]". Detection is unchanged; only forensic legibility drops, on exactly the datasets whose leakage the clustering is designed to prevent. Reasonable trade for finding 2, but the comment there explains the fallback and not this narrowing, which is the part a future reader will wonder about.

7. Stale "leave-one-cluster-out" phrasing

The docs pass caught the substantive prose but left the labels: build_env.py:128 (build_env docstring) and :226 (typer.Typer(help=...)), README.md:6-7 and :84, and run-eval.yml:3-9, :75, :85, :139. Cosmetic, and the workflow ones I can't touch — but "leave-one-cluster-out" is now the name of a thing this repo doesn't do.

---

Verification: I still could not execute the suite — pytest and python -c are both blocked by command approval in this environment, so the PR's stated test run remains unconfirmed by me. All findings are from reading the code. (If you want me running tests on future passes, Bash(uv run pytest*) in --allowedTools would do it.)

Nothing here blocks. Finding 1 is the only one I'd want resolved before anyone dispatches a fold by identifier, since it produces a silently leaky exemplar pool rather than a loud failure; findings 2 and 3 are cheap and worth taking while you're in the file.

• Read repo guidelines and full PR diff
• Verify status of prior findings 1–8
• Review new src/folds/resolve_fold.py and its tests
• Run test suite — blocked by command approval
• Post updated review
  · codex/targeted-grouped-cv-folds