[bcarmeli]

Summary

Adds an optional common pre-rollout helper gate to the SpreadsheetBench adapter. Off by default; behaviour is unchanged unless CAPEVOLVE_HELPERS_SPEC is set.

Hill-climb never runs cap-evolve's helper checks (the static lint_candidate and the run_smoke import test); only agent-optimize's pregate does. Other optimizers that share this adapter's rollout layer get no check at all. When CAPEVOLVE_HELPERS_SPEC points at a run's capevolve.yaml, the adapter now runs both checks itself, so every optimizer using the adapter is held to the same rule.

How it works

• When: before any LLM call, in Adapter.run_target, right after the existing task-template check.
• How often: once per distinct helper tree. The verdict is cached by a content hash of skills/*/scripts/*.py, so a candidate's many rollouts pay for one check.
• What it runs: cap-evolve's own lint_candidate (task ids, answer literals, branching on task identity, network use) and run_smoke (each script imports cleanly in a subprocess that cannot read deny_roots). Smoke only runs if lint passes.
• On failure: every rollout returns error_kind=candidate with a CANDIDATE_HELPER_GATE: ... message listing the findings. That scores 0 under errored_trial_policy: fail, and the harness still debits the evaluation.
• On a broken config (unreadable spec): it raises, since that is the run's fault, not the candidate's.

Why

Used in the issue #895 comparison, where nine non-CapEvolve optimizers run through this adapter. Without it, only CapEvolve's agent-optimize path would have its helper scripts checked. Results are on the spreadsheet-history branch.

Testing

• core/tests/test_spreadsheetbench_helper_gate.py (new, 7 tests): off when the env var is unset; a clean helper passes; a candidate with no scripts passes; a task-id literal is refused by lint; an import crash is refused by the smoke test; the verdict is cached per tree; run_target returns a candidate fault without an LLM call. The broken-spec path (it raises) is not covered by a test.
• On a compute node: the new test plus all test_spreadsheetbench*.py and test_helper*.py: 373 passed, 6 skipped, 1 failed.
• The one failure, test_spreadsheetbench_recalc_parallel.py::test_headless_and_no_leftover_profile_dirs, also fails on plain main on the same host. Three stale /tmp/capevolve_sb_libre_* dirs from earlier runs were already on that host, and the test asserts none exist. It is unrelated to this change.

🤖 Generated with Claude Code

[skillberry-bot]

❌ Automatic Labeling Failed

An error occurred while trying to automatically label this pull request. Please check the workflow logs for details and add labels manually.

[OsherElhadad]

Summary

Adds an opt-in "common pre-rollout helper gate" to templates/adapters/spreadsheetbench/adapter.py: when CAPEVOLVE_HELPERS_SPEC points at a capevolve.yaml (and helpers.enabled resolves true), Adapter.run_target now runs cap_evolve.helpers.lint_candidate and cap_evolve.helper_smoke.run_smoke on the candidate's skills/*/scripts tree before issuing any LLM call. A failure is cached per content-hash of the tree and returned as a Rollout with error_kind=candidate on every rollout for that candidate, so it is scored 0 under errored_trial_policy: fail without spending LLM budget. Motivation (per the PR body): hill-climb and other non-agent-optimize optimizers never run agent-optimize's pregate (helper_lint/helper_smoke), so for the issue #895 nine-optimizer comparison, only CapEvolve's own path was getting its helper scripts checked. This closes that gap by moving the check into the adapter's shared rollout layer.

Verification performed

• gh pr diff 954 read in full; both commits reviewed (the second is a CI-only fix: fakes litellm in sys.modules since CI doesn't install it).
• Checked out the PR via git worktree add /tmp/pr954-review on pr-954-review (tracking refs/pull/954/head), no disruption to other local work.
• Ran the new test file on the PR branch: core/tests/test_spreadsheetbench_helper_gate.py → 7 passed.
• Ran the same path on main (no worktree, in the primary clone): pytest tests/test_spreadsheetbench_helper_gate.py → file not found (confirms the tests genuinely exercise new behavior, not something already present).
• Ran the broader suite the PR body cites: pytest tests/ -k "spreadsheetbench or helper" on the PR branch → 395 passed, 4 skipped (lower numbers than the PR's reported "373 passed, 6 skipped, 1 failed" because that run also matched other files/paths on a compute node; no failures here, and I did not try to reproduce the one failure the PR already explained — test_headless_and_no_leftover_profile_dirs, attributed to stale /tmp dirs from earlier runs on that host and said to fail on main too, which I have no reason to doubt and did not independently verify).
• Manually exercised _helper_gate_error directly (not just via the new test's fixtures) in a throwaway Python session:
  • Off path: CAPEVOLVE_HELPERS_SPEC unset → None, confirmed no behavior change (matches test_gate_is_off_without_the_spec_env).
  • Gate does not activate on helpers.enabled: true in the spec alone — see Issue 1 below; confirmed by direct call.
  • Broken-spec path (CAPEVOLVE_HELPERS_SPEC pointing at a nonexistent file): confirmed _helper_gate_error itself raises RuntimeError, but confirmed run_target does not propagate it — see Issue 2 below.

Confirmed issues

1. The gate is dead unless CAPEVOLVE_HELPERS=1 is also set — helpers.enabled: true in the spec alone does not turn it on.
_helper_gate_config guards with not _helpers.enabled() (adapter.py:1215), which calls _helpers.enabled(spec=None) → resolve(None). resolve() only reads helpers.enabled from the spec dict passed to it; called with no argument, it only sees CAPEVOLVE_HELPERS from the environment (helpers.py:90-93, _env_enabled). The actual spec at CAPEVOLVE_HELPERS_SPEC is read two lines later, by _helper_gate_config itself, and is never passed into this enabled() check — only into _helpers.resolve(read_yaml(...)) after the gate already decided to proceed. I verified this directly: with CAPEVOLVE_HELPERS unset and a spec file containing helpers: {enabled: true, lint: true, smoke: true} pointed to by CAPEVOLVE_HELPERS_SPEC, _helper_gate_error returns None even for an obviously-bad helper script. The PR's own docstring for _helper_gate_config says "Unset means the gate is off, as before" about CAPEVOLVE_HELPERS_SPEC, but doesn't flag that the spec's own helpers.enabled: true is silently ignored unless the env var is also set — which is exactly the footgun cli.py:762-765 exists to prevent for every other call site (it explicitly sets os.environ["CAPEVOLVE_HELPERS"] = "1" whenever _helpers.enabled(spec) is true, precisely because adapters run in a subprocess and only see the env). The exp895 launcher this PR is for presumably sets both CAPEVOLVE_HELPERS_SPEC and CAPEVOLVE_HELPERS=1, so the PR's intended use case may be unaffected in practice — but the function reads as if the spec's helpers.enabled plus CAPEVOLVE_HELPERS_SPEC alone is sufficient to turn the gate on, and it is not. This should at minimum be called out in the docstring/comment, or (better) _helper_gate_config should resolve enabled from the loaded spec dict, not from the empty-arg global check, so the config file is self-sufficient without a second env var.

2. The "fails loudly on a broken config" contract is broken by run_target's outer catch-all.
The PR body and the inline comment at adapter.py:1245 ("a broken config is the run's fault: fail loudly, not per candidate") promise that an unreadable/malformed CAPEVOLVE_HELPERS_SPEC raises rather than silently scoring the candidate 0. _helper_gate_error does raise a RuntimeError in that case. But the call site (adapter.py:1694, inside run_target's big try) is covered by the existing outer except Exception as e: return Rollout(task_id=task.id, error=f"run_target failed: {e}") at the end of run_target (adapter.py:1941). I reproduced this directly: pointing CAPEVOLVE_HELPERS_SPEC at a nonexistent file and calling Adapter().run_target(...) returns a normal Rollout with error="run_target failed: helper gate: could not load CAPEVOLVE_HELPERS_SPEC: ..." and metadata={} — no error_kind, no raised exception out of run_target. Under errored_trial_policy: fail this becomes a missing-data trial (dropped/retried), and under the default drop policy it's silently absorbed per-rollout, exactly the "silently run broken" outcome the comment says it's avoiding. A broken config will therefore manifest as every rollout across the whole run quietly erroring with an infra-looking message, not as a loud, run-stopping failure — the harness-level live() contextmanager (which prints loudly but also deliberately swallows, by the adapter's own existing design for _task_template_error) is not even reached here because the failure happens inside run_target, not live(). This is a real gap between the stated intent and the actual behavior; worth either letting the exception propagate (e.g. raise it before/outside run_target's try, or re-raise past the catch-all specifically for this case) or, if per-rollout attribution is accepted as "good enough," updating the comment/PR description to stop claiming it "fails loudly."

3. Parent/seed baseline is not passed to lint_candidate or run_smoke, unlike the existing agent-optimize pregate path.
skills/algorithms/agent-optimize/scripts/pregate.py:429-438's helper_checks calls helpers.lint_candidate(cand, cfg, parent) and helper_smoke.run_smoke(cand, cfg, parent=parent, fixtures=rows), passing the candidate's parent so that (a) lint only flags new or changed findings, not violations the parent/seed already had (helpers.py:343-355, explicit comment: "a seed tools.py is not this edit's fault"), and (b) smoke only re-runs changed files (helper_smoke.py:178: changed_files(cand, parent, ...); with no files changed, run_smoke returns ok: True, skipped: True immediately). The new _helper_gate_error calls both with no parent (adapter.py:1257, 1261), so changed_files degrades to treating every helper file as "changed" (helpers.py:178-181: if parent is None ...: return list(files)). Two consequences: (a) if the seed capability itself ever ships a helper script that would fail lint/smoke (none currently does — seed_capability_v2/skills/spreadsheet-helpers/scripts/ is empty except .gitkeep), every candidate inheriting it would be gated out through no fault of its own edit; (b) every rollout's first gate check always does a full lint + full smoke run of the entire tree rather than only the diff from the candidate's parent, which is more CPU/subprocess work than necessary (mitigated by the content-hash cache, but the cache key is per distinct tree, not per edit, so a large unchanged seed tree is smoke-tested in full on first sight of every new candidate). This is a smaller issue than #1/#2, but it's a real behavioral difference from the existing, tested pregate path that the PR doesn't mention.

4. No test for the broken-config path, which is exactly the "fails loudly" guarantee that issue #2 shows is not held.
The PR body says plainly: "The broken-spec path (it raises) is not covered by a test." Given issue #2 above, this gap is not cosmetic — a test driving it through run_target() (not just _helper_gate_error() directly) would likely have caught the swallowed exception before merge.

5. Minor: the print(..., file=sys.stderr) on every first-seen tree is unconditional, unlike the rest of the adapter which is fairly quiet; harmless, but worth confirming it doesn't spam logs across a large population of distinct candidate trees in a long optimize run (it's one line per distinct tree thanks to the cache, so probably fine, just flagging for the author's awareness since the file otherwise reserves stderr logging for genuine live()-level problems).

Other observations (no action required)

• Docs: no README/capevolve.yaml schema entry documents CAPEVOLVE_HELPERS_SPEC anywhere (grepped docs/, SEED_V2.md, CHANGELOG.md) — it's only mentioned in the adapter docstring and the new test. Given this is explicitly described as a mechanism for one experiment (exp895's launcher), that may be intentional, but future readers of capevolve.yaml's helpers: block (documented in helpers.py's module docstring) won't learn this adapter-level env var exists from there.
• supports_error_kind = True is not set on this Adapter class (unlike the tau2 adapters), so errored_trial_policy defaults to "drop" rather than "fail" unless the run's spec or env explicitly sets it (evaluator.py:182-185). That's pre-existing behavior, not something this PR changes, but it means the "scores 0 under errored_trial_policy: fail" framing in the PR body is conditional on the run opting into that policy — worth the launcher double-checking it sets errored_trial_policy: fail for the comparison to actually attribute these failures as candidate faults rather than dropped/retried trials (which ties back to issue fix: add core/README.md and fix readme path in pyproject.toml #2: under drop, a broken-config failure and a real candidate-helper failure become indistinguishable from ordinary missing data).
• Off-by-default behavior is correctly preserved for existing (non-exp895) SpreadsheetBench runs: CAPEVOLVE_HELPERS_SPEC unset → _helper_gate_config returns None immediately → _helper_gate_error is a no-op → zero behavior change, confirmed both by test and by manual check.
• Code quality / style is consistent with the surrounding file (docstring conventions, # noqa: BLE001 usage, Rollout/metadata conventions match _task_template_error's established pattern). The caching strategy (content-hash, once per distinct tree, thread-safe via _HELPER_GATE_LOCK) is a reasonable, simple design given run_trials_pool uses a ThreadPoolExecutor (not multiprocessing), so the in-process module-level cache dict is valid across concurrent rollouts of the same process.
• CI is green (gh pr checks 954): Build Dashboard, CodeQL, DCO, Docs link check, Install smoke, Skill authoring lint, Spellcheck, Test Python (core + dashboard backend), Test benchmark CI helpers, Toy example — all passing.

Merge recommendation

Do not merge as-is. The test suite is solid and the off-by-default contract holds, but two of the findings go to the heart of what the PR claims to deliver:

• Issue Executing: pip install ./core #1 means the feature, as written, requires an undocumented second env var (CAPEVOLVE_HELPERS=1) alongside CAPEVOLVE_HELPERS_SPEC to actually activate — if the exp895 launcher doesn't already set both (I could not find the launcher script in this repo to confirm), the gate silently never fires for any arm, which is the exact failure mode this PR exists to close.
• Issue fix: add core/README.md and fix readme path in pyproject.toml #2 directly contradicts the PR's own stated safety property ("a broken config... fail[s] loudly, not per candidate") — I reproduced the swallowed exception end-to-end through run_target.

Recommend: fix _helper_gate_config to resolve enabled from the loaded spec (not a second env-only check), make the broken-config RuntimeError actually escape run_target (or explicitly accept and document that it's attributed as error_kind rather than raised, and add the missing test for that path), and consider passing parent/seed into lint_candidate/run_smoke to match the existing pregate's semantics. These are contained, mechanical fixes to the two new helper functions — not a design rework — so a quick follow-up push should be enough to get this to mergeable shape.