From 8076b98fccd1d4839f13eebe971a7b94f38b4c5b Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Sat, 1 Aug 2026 19:34:57 +0000 Subject: [PATCH] refactor(ci): address the evidence path directly instead of threading it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The upload step needs a path that does not depend on the sweep step surviving. It did not need shared state to get one: `runner.temp` is available in a step, only not in a job-level `env:`, so each of the three consumers can name `${RUNNER_TEMP}/wfevolve` itself. That deletes the env var and the step that published it — the previous fix swapped one threading channel for a sturdier one where no channel was required. Also from the same review pass: - `announce`/`keep` drop their default-argument capture of `task["id"]` and `per_arm`. Late binding only bites a closure invoked after the loop moves on; these are called synchronously inside `sweep_task_cells`, which blocks until every wave completes. The trick was guarding against a race that cannot happen here, while implying to the next reader that it can. - `_stub_cell_dependencies` returns the list its teardown appends to rather than taking it as an out-parameter, dropping the boilerplate from every call site. - The workflow's `WORKERS` comment points at the `--workers` help text instead of restating it, so the rationale has one home. --- .../workflows/gitnexus-skill-evolution.yml | 25 ++++++------------- eval/tests/test_runner_hardening.py | 22 ++++++++-------- eval/workflow_bench/runner.py | 16 ++++-------- .../unit/skill-evolution-workflow.test.ts | 10 ++------ 4 files changed, 27 insertions(+), 46 deletions(-) diff --git a/.github/workflows/gitnexus-skill-evolution.yml b/.github/workflows/gitnexus-skill-evolution.yml index c1b20965c..64bfbb4ae 100644 --- a/.github/workflows/gitnexus-skill-evolution.yml +++ b/.github/workflows/gitnexus-skill-evolution.yml @@ -134,25 +134,13 @@ jobs: env: GENERATIONS: ${{ inputs.generations || '1' }} RUNS: ${{ inputs.runs || '3' }} - # Serial by default. A cell that loses CPU to its siblings takes longer, - # and a session that reaches its timeout is an excluded run the promotion - # gate refuses to work with — so this only goes up when the runner has the - # vCPUs to back it (the box is sized for one cell at a time today). + # Serial by default — see workflow_bench.runner --workers for why, and + # raise it only to match the runner's vCPUs. WORKERS: ${{ inputs.workers || '1' }} MODEL: ${{ inputs.model || 'claude-sonnet-5' }} PROPOSER_MODEL: ${{ inputs.proposer_model || 'claude-opus-4-8' }} INCLUDE_EXPENSIVE: ${{ inputs.include_expensive && '1' || '' }} steps: - - name: Pin the evidence path before anything can run - # The upload step must not take its path from the sweep step's outputs - # — that is the step whose death is the reason the upload matters. A - # job-level `env:` cannot hold it either (the `runner` context does not - # exist there), so publish it to GITHUB_ENV first: every later step - # sees it, including the `if: always()` upload after a killed sweep. - run: | - set -euo pipefail - echo "OUT_ROOT=${RUNNER_TEMP}/wfevolve" >> "${GITHUB_ENV}" - - name: Require the benchmark auth secret env: HAS_TOKEN: ${{ secrets.GITNEXUS_BENCH_AUTH_TOKEN != '' }} @@ -322,7 +310,7 @@ jobs: --runs "${RUNS}" \ --workers "${WORKERS}" \ --claude-bin "${RUNNER_TEMP}/claude-canary/node_modules/@anthropic-ai/claude-code-linux-x64/claude" \ - --out-root "${OUT_ROOT}" \ + --out-root "${RUNNER_TEMP}/wfevolve" \ --apply \ "${extra[@]}" working-directory: eval @@ -335,7 +323,10 @@ jobs: uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: gitnexus-evolution-${{ github.run_id }}-${{ github.run_attempt }} - path: ${{ env.OUT_ROOT }} + # Addressed directly rather than carried from the sweep step: that is + # the step whose death is the reason this upload matters, and a value + # threaded from it would not be there when it counts. + path: ${{ runner.temp }}/wfevolve retention-days: 14 if-no-files-found: warn @@ -369,7 +360,7 @@ jobs: # generation's decisions could surface in the PR body. The heredoc # uses a per-run random delimiter so a summary value that ever # contains the marker cannot close the block early and inject keys. - promotion_file="$(find "${OUT_ROOT}" -name promotion.json | sort -V | tail -1)" + promotion_file="$(find "${RUNNER_TEMP}/wfevolve" -name promotion.json | sort -V | tail -1)" delim="PROMOTION_EOF_$(openssl rand -hex 16)" { echo "summary<<${delim}" diff --git a/eval/tests/test_runner_hardening.py b/eval/tests/test_runner_hardening.py index f72c4b797..407bcbe0d 100644 --- a/eval/tests/test_runner_hardening.py +++ b/eval/tests/test_runner_hardening.py @@ -3,6 +3,7 @@ import hashlib import json from contextlib import nullcontext +from pathlib import Path from types import SimpleNamespace import pytest @@ -433,8 +434,12 @@ def _cell_context(tmp_path, **overrides): return runner.TaskCellContext(**fields) -def _stub_cell_dependencies(monkeypatch, tmp_path, removed): - """Replace everything a cell shells out to, so only its own logic runs.""" +def _stub_cell_dependencies(monkeypatch, tmp_path): + """Replace everything a cell shells out to, so only its own logic runs. + + Returns the clone it will hand out and the list its teardown appends to. + """ + removed: list[Path] = [] worktree = tmp_path / "clone" worktree.mkdir() monkeypatch.setattr(runner, "make_worktree", lambda *_a, **_k: worktree) @@ -454,12 +459,11 @@ def _stub_cell_dependencies(monkeypatch, tmp_path, removed): monkeypatch.setattr(runner, "capture_patch", lambda *_a, **_k: b"diff") monkeypatch.setattr(runner, "run_arm", lambda *_a, **_k: {"resolved": True, "ok": True, "error_kind": None}) monkeypatch.setattr(runner, "remove_clone", lambda path: removed.append(path)) - return worktree + return worktree, removed def test_run_cell_returns_a_row_bound_to_its_task_and_snapshots(monkeypatch, tmp_path): - removed: list = [] - _stub_cell_dependencies(monkeypatch, tmp_path, removed) + _, removed = _stub_cell_dependencies(monkeypatch, tmp_path) record = runner.run_cell(_cell_context(tmp_path), 2, "workflow") @@ -497,8 +501,7 @@ def test_run_cell_returns_a_row_bound_to_its_task_and_snapshots(monkeypatch, tmp ids=["managed-process", "sandbox", "os", "runtime", "value"], ) def test_run_cell_records_an_expected_failure_and_still_removes_its_clone(monkeypatch, tmp_path, failure): - removed: list = [] - _stub_cell_dependencies(monkeypatch, tmp_path, removed) + _, removed = _stub_cell_dependencies(monkeypatch, tmp_path) def explode(*_args, **_kwargs): raise failure @@ -514,8 +517,7 @@ def test_run_cell_records_an_expected_failure_and_still_removes_its_clone(monkey def test_run_cell_lets_an_unexpected_failure_escape_rather_than_scoring_it(monkeypatch, tmp_path): - removed: list = [] - _stub_cell_dependencies(monkeypatch, tmp_path, removed) + _, removed = _stub_cell_dependencies(monkeypatch, tmp_path) def explode(*_args, **_kwargs): raise KeyError("harness bug") @@ -529,7 +531,7 @@ def test_run_cell_lets_an_unexpected_failure_escape_rather_than_scoring_it(monke def test_run_cell_reports_a_cleanup_failure_over_its_primary_outcome(monkeypatch, tmp_path): - _stub_cell_dependencies(monkeypatch, tmp_path, []) + _stub_cell_dependencies(monkeypatch, tmp_path) def refuse(_path): raise OSError("clone is busy") diff --git a/eval/workflow_bench/runner.py b/eval/workflow_bench/runner.py index b86a1a132..71c8eedc0 100644 --- a/eval/workflow_bench/runner.py +++ b/eval/workflow_bench/runner.py @@ -1425,22 +1425,16 @@ def main() -> None: per_arm: dict[str, list[dict[str, Any]]] = {a: [] for a in args.arms} cells = [(run_idx, arm) for run_idx in range(args.runs) for arm in args.arms] - def announce(run_idx: int, arm: str, task_id: str = task["id"]) -> None: + def announce(run_idx: int, arm: str) -> None: nonlocal started_cells started_cells += 1 print( - f"[{task_id}][{arm}][run {run_idx}] starting " + f"[{task['id']}][{arm}][run {run_idx}] starting " f"({started_cells}/{total_cells}, {(time.monotonic() - sweep_started) / 60:.0f}m elapsed)" ) - def keep( - run_idx: int, - arm: str, - record: dict[str, Any], - task_id: str = task["id"], - rows: dict[str, list[dict[str, Any]]] = per_arm, - ) -> None: - rows[arm].append(record) + def keep(run_idx: int, arm: str, record: dict[str, Any]) -> None: + per_arm[arm].append(record) with results_path.open("a") as fh: # Redact any API token a session-error stderr_tail echoed # into error_detail before it enters the uploaded @@ -1448,7 +1442,7 @@ def main() -> None: # sink was not). fh.write(redact_text(json.dumps(record), [args.auth_token or ""]) + "\n") print( - f"[{task_id}][{arm}][run {run_idx}] resolved={record['resolved']} " + f"[{task['id']}][{arm}][run {run_idx}] resolved={record['resolved']} " f"in={record['input_tokens']} out={record['output_tokens']} " f"cost=${_na(record['cost_usd'])} " f"took={_na(record.get('duration_s'))}s " diff --git a/gitnexus/test/unit/skill-evolution-workflow.test.ts b/gitnexus/test/unit/skill-evolution-workflow.test.ts index 777aee4a2..58b8a2b69 100644 --- a/gitnexus/test/unit/skill-evolution-workflow.test.ts +++ b/gitnexus/test/unit/skill-evolution-workflow.test.ts @@ -151,19 +151,13 @@ describe('gitnexus skill-evolution workflow contract', () => { expect(jobBudget as number).toBeLessThanOrEqual(21 * 60); }); - it('uploads benchmark evidence unconditionally, on a path fixed before the sweep runs', () => { - // OUT_ROOT is published to GITHUB_ENV by the first step, not held in a - // job-level `env:` — the `runner` context does not exist there, so - // `${{ runner.temp }}/wfevolve` would silently resolve to `/wfevolve`. - const first = evolveJob?.steps?.[0]; - expect(first?.name).toBe('Pin the evidence path before anything can run'); - expect(first?.run).toContain('echo "OUT_ROOT=${RUNNER_TEMP}/wfevolve" >> "${GITHUB_ENV}"'); + it('uploads benchmark evidence unconditionally, on a path it addresses itself', () => { const upload = evolveJob?.steps?.find(({ name }) => name === 'Upload benchmark evidence'); // The sweep appends results.jsonl and transcripts as it goes, so a killed // generation still holds the evidence explaining why — and a path taken // from the killed step's outputs is exactly what would not be there. expect(upload?.if).toBe('always()'); - expect(upload?.with?.path).toBe('${{ env.OUT_ROOT }}'); + expect(upload?.with?.path).toBe('${{ runner.temp }}/wfevolve'); }); it('documents the App secrets and protected Environment on the activation checklist', () => {