Add leave-one-cluster-out evaluation workflow

[cmungall]

Summary

This PR adds a new GitHub Actions workflow for running leave-one-cluster-out (LOCO) harmonization evaluations. The workflow enables manual evaluation of the harmonization agent on held-out datasets while excluding them from exemplar mappings and reference code.

Key Changes

• New workflow file (.github/workflows/run-eval.yml): Implements a manually-triggered evaluation pipeline that:
  • Accepts flexible hold-out specifications (dataset indices, identifiers, comma-separated lists, or cluster names from config/cv_folds.yaml)
  • Resolves cluster definitions to dataset indices for evaluation
  • Builds isolated run environments with held-out data removed from exemplar mappings and reference code
  • Optionally stages raw input data from public sources (Google Drive/ESS-DIVE)
  • Executes the curator+harmonizer agent in the prepared environment using Claude Code Action
  • Uploads complete run outputs (harmonized CSVs, transformation code, mappings, notes) as build artifacts

Notable Implementation Details

• Flexible hold-out targeting: Supports multiple input formats (e.g., 5, dataset_id, 1,2,3,6, cluster_1, sg_ph_micromet) with automatic resolution via YAML cluster configuration
• Customizable run naming: Auto-generates environment folder names based on cluster ID or hold-out specification, with optional override
• Best-effort staging: Raw data staging continues even if it fails, allowing evaluation to proceed
• Isolated evaluation environment: Uses src.folds.build_env to create self-contained run folders that exclude held-out data from training materials
• MCP tool integration: Includes OLS (Ontology Lookup Service) and sequential-thinking MCP servers for enhanced agent capabilities
• Comprehensive artifact capture: Uploads entire run folder to preserve all agent outputs for analysis

https://claude.ai/code/session_01PwfJQSWjmeiVqk2d65c6xJ

[claude]

Claude finished @cmungall's task —— View job

---

PR Review: Add leave-one-cluster-out evaluation workflow

Tasks

• Gather context and understand the request
• Read the workflow file and supporting code
• Review for code quality, bugs, security, performance
• Post comprehensive feedback

---

Overall this is a well-structured, thoughtfully documented workflow. Below are specific observations across the requested dimensions.

---

Security Concerns

[High] Unquoted expression in shell command (code injection risk)

.github/workflows/run-eval.yml line 148:

cd ${{ steps.resolve.outputs.env_path }}

The env_path value is constructed from env_name, which is derived from user input (inputs.name or inputs.holdout). Although the regex in line 90 sanitizes holdout for the holdout-<ids> case, inputs.name is passed through entirely unfiltered. A malicious name input like foo; rm -rf / could execute arbitrary shell commands inside the direct_prompt. It's inside Claude's prompt so exploitability is limited, but it is a bad pattern.

Similarly, lines 105–106 interpolate steps.resolve.outputs.holdout and steps.resolve.outputs.env_name directly into shell run: blocks:

--holdout "${{ steps.resolve.outputs.holdout }}" \
--name "${{ steps.resolve.outputs.env_name }}"

These are less risky because they're quoted, but any step output originating from user input should ideally be passed via environment variables, not direct expression interpolation.

Recommendation: Sanitize or validate inputs.name (reject characters outside [A-Za-z0-9._-]) and pass step outputs via env: rather than inline expressions in run: blocks.

[Medium] contents: write permission is broader than necessary

Line 44: contents: write is declared but the workflow only reads the repo and uploads artifacts. Artifact upload uses actions: write implicitly but doesn't require contents: write. Consider using contents: read unless there's a requirement to commit outputs.

---

Potential Bugs

[Medium] stage_raw output path is fixed, not relative to the run environment

Lines 111–114:

uv run python -m src.folds.stage_raw_data \
  --indices "${{ steps.resolve.outputs.stage_indices }}" \
  --drive-method public

There is no --output-dir or equivalent pointing into .runs/<name>/. If stage_raw_data.py writes to a fixed location (e.g., ~/ess-dive_wfsfa_soil_datasets/), that's fine by design (the agent prompt references the home path). But if it writes anywhere relative to the repo root, it won't be captured in the artifact upload at line 173 (path: ${{ steps.resolve.outputs.env_path }}/). Worth verifying or documenting.

[Low] Inline Python print() for GITHUB_OUTPUT is fragile with multiline values

Lines 95–98 use print(f"holdout={holdout}") etc. If holdout or env_name ever contained a newline (unlikely given current logic, but worth noting), the output would be malformed. Using the multiline delimiter syntax is safer for production workflows:

with open(os.environ["GITHUB_OUTPUT"], "a") as f:
    f.write(f"holdout={holdout}\n")

(Also: writing to "$GITHUB_OUTPUT" via >> "$GITHUB_OUTPUT" redirect is correct, but the print() approach relies on Python's stdout going there, which works only because of the heredoc redirect on line 66.)

[Low] cv_folds.yaml inconsistency is silently ignored

The inline Python at lines 79–83 matches on cluster name using str(c.get("name", "")).strip(). In config/cv_folds.yaml, cluster_4 (global_exclude) contains datasets like index 0 and 11–22 that the code in build_env.py explicitly rejects (raises). If a user passes global_exclude or cluster_4, the workflow will fail at the "Build run environment" step with a cryptic error rather than a friendly one. Consider adding a validation step or a clearer error message.

---

Code Quality & Best Practices

[Medium] Duplicate prompt vs. AGENT_INSTRUCTIONS.md

The direct_prompt at lines 139–167 partially duplicates the instructions in AGENT_INSTRUCTIONS.md (generated by build_env.py). The workflow prompt is more prescriptive (explicit cd, numbered steps) while the file is briefer. This is intentional for redundancy, but any future changes need to be kept in sync in two places. A comment noting this relationship would help future maintainers.

[Low] Action version pinned to @beta

Line 117: uses: anthropics/claude-code-action@beta — using a mutable tag like @beta means the action can change behavior between runs without a code change. For a reproducible evaluation pipeline, pinning to a specific commit SHA or release tag (e.g., @v1.2.3) is strongly recommended.

[Low] uv sync without --frozen

Line 58: uv sync without --frozen will update the lockfile if it's out of date, potentially changing package versions mid-run. Use uv sync --frozen for reproducibility in CI.

[Low] No timeout on the agent step

The anthropics/claude-code-action@beta step at lines 116–167 has no explicit timeout-minutes. If the agent hangs or enters a long loop, the job will run until the default GitHub Actions limit (6 hours), consuming runner minutes unnecessarily. Adding timeout-minutes: 60 (or similar) to the job or step would cap runaway evals.

---

Performance

• The workflow does a shallow clone (fetch-depth: 1) which is appropriate for eval.
• No caching of uv artifacts (.venv, uv cache). For large dependency sets, adding actions/cache for ~/.cache/uv would speed up repeated runs noticeably.

---

Test Coverage

There are no automated tests for the workflow logic itself (expected for a workflow file). The inline Python at lines 66–99 is the most logic-dense untested piece — the resolve_holdout / cluster lookup. The existing unit tests in src/folds/ cover build_env.py well; adding a small test for the name sanitization regex (re.sub(r"[^A-Za-z0-9._-]+", "-", raw).strip("-")) and the cluster resolution logic (currently duplicated from Python into the workflow YAML) would catch regressions if one is updated but not the other.

---

Summary

Category	Severity	Item
Security	High	inputs.name not sanitized before shell interpolation in direct_prompt
Security	Medium	contents: write broader than needed
Bug	Medium	Raw data staging output path may not land in artifact
Bug	Low	Multiline output values could corrupt GITHUB_OUTPUT
Bug	Low	global_exclude cluster gives cryptic failure
Quality	Medium	direct_prompt and AGENT_INSTRUCTIONS.md are unsynchronized duplicates
Quality	Low	@beta action tag is not pinned
Quality	Low	uv sync should use --frozen
Quality	Low	No timeout-minutes on agent step
Performance	Low	No uv cache in CI

The security item around inputs.name is the most actionable before merge. The rest are relatively low-risk given that this is a manually-triggered internal eval workflow.

---

[claude]

Claude finished @cmungall's task —— View job

---

Tasks

• Gather context and understand the request
• Read the workflow file and supporting code
• Review for code quality, bugs, security, performance
• Post comprehensive feedback

---

PR Review: Add leave-one-cluster-out evaluation workflow (updated)

This review covers the current state of the file, which already addresses several items from the previous review. The diff from 6da6fd8 → b401fb1 fixed:

• ✅ contents: write → contents: read
• ✅ inputs.name and all other user inputs now sanitized through the same re.sub call before writing to GITHUB_OUTPUT
• ✅ Inline expressions in run: blocks replaced with env: variables
• ✅ GITHUB_OUTPUT written properly (file append, not print() → stdout redirect)
• ✅ timeout-minutes: 60 added to agent step
• ✅ Comment added linking direct_prompt to AGENT_INSTRUCTIONS.md

The diff is clean and targeted. The remaining items from the previous review (mostly low-priority quality nits) are addressed below.

---

Remaining Issues

[Low] uv sync without --frozen (line 58)

run: uv sync

In CI, uv sync without --frozen will silently update the lockfile if it's out of date, potentially altering package versions mid-run. For reproducible evaluations:

run: uv sync --frozen

[Low] @beta action tag is not pinned (line 122)

uses: anthropics/claude-code-action@beta

@beta is a mutable tag — the action can change behavior between runs without any code change in this repo. For reproducible eval results, pinning to a commit SHA or explicit version tag (e.g., @v1) is preferable. That said, for an internal eval pipeline this is acceptable if you want to track latest beta behavior.

[Low] No uv cache

There's no caching of ~/.cache/uv or .venv. For a CI job that runs uv sync on every dispatch, adding actions/cache would speed up repeated runs:

- uses: actions/cache@v4
  with:
    path: ~/.cache/uv
    key: uv-${{ runner.os }}-${{ hashFiles('uv.lock') }}

[Low] global_exclude cluster gives cryptic failure

If a user passes cluster_4 (or global_exclude), the workflow resolves holdout indices [0, 11, 12, 13, 14, 19, 20, 21, 22] and the "Build run environment" step fails with an exception from build_env.py / kept_module_paths() because those indices have no corresponding expert modules. The error message won't be user-friendly. A validation guard in the resolve step would help:

if cluster_id and c.get("description", "").lower().find("exclude") >= 0:
    raise SystemExit(f"ERROR: cluster '{cluster_id}' is a global-exclude cluster and cannot be held out.")

---

Observations on New Code

The injection-safety fix is solid: env_name = re.sub(r"[^A-Za-z0-9._-]+", "-", env_name).strip("-") or "run" now applies uniformly to all three name sources (name_in, cluster_id, and the raw holdout input). The or "run" fallback is a nice touch.

The GITHUB_OUTPUT write approach (file append) is correct and multiline-safe.

The comment on line 144 (# This prompt intentionally restates...) appropriately documents the intentional duplication between direct_prompt and AGENT_INSTRUCTIONS.md.

---

Summary

Category	Severity	Item	Status
Security	High	inputs.name not sanitized	✅ Fixed
Security	Medium	contents: write broader than needed	✅ Fixed
Bug	Low	GITHUB_OUTPUT multiline fragility	✅ Fixed
Bug	Low	global_exclude cluster gives cryptic failure	Still present
Quality	Medium	direct_prompt / AGENT_INSTRUCTIONS.md sync note	✅ Fixed
Quality	Low	@beta action tag not pinned	Still present (acceptable)
Quality	Low	uv sync should use --frozen	Still present
Quality	Low	No timeout-minutes on agent step	✅ Fixed
Performance	Low	No uv cache in CI	Still present

The high-priority security concern is resolved. The remaining items are all low-priority quality/performance nits — this is ready to merge with those understood as follow-up candidates.