mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-09-19 05:58:01 +08:00
fix(eval): self-hosted skill-evolution runner + sandbox Python 3 trust fix (#2600)
* fix(eval): move skill-evolution to a self-hosted runner and fix the sandbox's Python 3 trust gap GitHub-hosted runners hard-cap job execution at 6 hours, which is too short once a benchmark session actually invokes Skill/MCP tools for real (the --bare fix in #2584 means sessions no longer no-op). Move the job onto a self-hosted runner (5-day cap instead) and document the activation step in the workflow's own checklist. Validating the self-hosted run surfaced a real bug: gitnexus-plan sessions inside the bwrap sandbox failed with "planning must create or modify exactly one plan artifact; observed 0". Root cause: evidence-provenance.mjs's atomic plan-writer only trusts a Python 3 binary owned by root or by the current process. Inside this --unshare-user sandbox only the calling uid is mapped (root isn't), so the real, root-owned /usr/bin/python3 surfaces as the kernel's overflow uid and gets correctly refused as untrusted. Fix: provision a small, self-owned wrapper script (same pattern already used for shell-prefix) that execs the real interpreter, so the sandbox has a Python 3 candidate the existing trust check can actually accept -- without touching that security-sensitive validation logic at all. Also add visibility so this class of failure isn't quiet next time: report.md now shows why each row failed (error_kinds), not just resolved 0/1, and the benchmark now exits non-zero when an incumbent arm -- the currently-shipped skill -- resolves zero across every task, since that reads as a broken harness rather than a normal candidate miss. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(eval): close the broken_incumbent_arms zero-valid-runs gap; document runner exposure tradeoff Addresses the two MEDIUM findings from the gitnexus-review-agent on this PR (https://github.com/abhigyanpatwari/GitNexus/pull/2600#issuecomment-5033363096). broken_incumbent_arms required valid_runs > 0 before flagging an incumbent, so an incumbent that fails every run with an excluded-but-non-systemic error_kind (e.g. evidence-unverified, which the outage-streak breaker explicitly resets on rather than accumulates) never accumulated a single valid run and sailed through silently -- the exact quiet no-promotion outcome this guard exists to catch, and arguably worse than the some-runs-resolved-zero case since here nothing completed at all. aggregate() never marks an excluded/unverifiable row resolved=True, so dropping the valid_runs requirement and checking resolved == 0 alone correctly covers both cases. Added a test for exactly this all-excluded scenario, which none of the existing three did. Updated the workflow's own activation checklist to reflect what's actually true now (the gitnexus-evolution environment's branch policy and the self-hosted runner are both live, codified in infra/gitnexus-evolution/ in a companion PR) and documented the exposure-window tradeoff the review flagged: the runner is stopped between runs but not destroyed/recreated per run, so it isn't fully ephemeral. Stopping already bounds the exposure window to the job's own runtime on one day out of seven; full per-job ephemeral provisioning is a deliberate non-goal for a job that runs at most weekly, revisit if that changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(eval): remove public infra/ pointers from the activation checklist PR #2603 (the Terraform codification this checklist pointed to) got closed -- publishing the exact IAM roles, security group rules, and self-hosted runner topology for a real, live AWS account isn't safe to do in a public repo, even with no literal secrets or resource IDs in the diff. The underlying AWS/GitHub setup is unaffected and still documented privately; this just removes the now-dangling references to a directory that won't exist in this repo. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ci(actionlint): register the gitnexus-evolution self-hosted runner label actionlint rejected `runs-on: [self-hosted, linux, x64, gitnexus-evolution]` in gitnexus-skill-evolution.yml because it can't discover custom runner labels. Register it in .github/actionlint.yaml so the Workflow Lint check passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,5 @@
|
||||
# Custom self-hosted runner labels actionlint can't discover on its own.
|
||||
# gitnexus-evolution: the skill-evolution EC2 runner (infra/gitnexus-evolution/).
|
||||
self-hosted-runner:
|
||||
labels:
|
||||
- gitnexus-evolution
|
||||
@@ -12,12 +12,34 @@
|
||||
# App that opens the promotion PR). The Mint-App-Token step hard-fails
|
||||
# without them once a promotion is detected. Verify the App installation
|
||||
# is scoped to this repo with only Contents: RW + Pull requests: RW.
|
||||
# [ ] Create the protected Environment `gitnexus-evolution` with a
|
||||
# [x] Create the protected Environment `gitnexus-evolution` with a
|
||||
# deployment-branch rule restricting it to `main`, and ideally scope the
|
||||
# three secrets above to that Environment. workflow_dispatch runs this
|
||||
# workflow (and eval/workflow_bench/evolve.py) from the *dispatched ref*,
|
||||
# so this server-side rule — not a code-side guard the branch could edit
|
||||
# away — is what stops a non-main branch from running with the secrets.
|
||||
# [x] Register a self-hosted runner labeled `gitnexus-evolution` (a dedicated
|
||||
# EC2 box works well). GitHub-hosted runners hard-cap job execution at 6
|
||||
# hours, non-configurable — too short once a benchmark session actually
|
||||
# invokes Skill/MCP tools for real. Self-hosted runners cap at 5 days
|
||||
# instead. This job only ever runs on schedule/workflow_dispatch, never
|
||||
# on fork-PR content, so the usual public-repo self-hosted-runner risk
|
||||
# doesn't apply — still keep the box dedicated to this workflow, with
|
||||
# outbound-only network access, and prefer on-demand over Spot (a Spot
|
||||
# reclaim mid-run loses the same way a 6-hour timeout does). Instance,
|
||||
# security group, and IAM setup are documented privately, not in this
|
||||
# repo — publishing the exact topology of a real, live AWS account
|
||||
# isn't safe to do in a public repo even without literal secrets.
|
||||
# Accepted tradeoff: the box is stopped between runs (an EventBridge
|
||||
# schedule starts it ~15min before the Saturday cron and stops it 24h
|
||||
# later) but is not destroyed/recreated per run, so it isn't fully
|
||||
# ephemeral — a compromise between the review-flagged ideal (re-image
|
||||
# between runs, bounding how long the injected model API key could
|
||||
# matter if the box were ever compromised some other way) and the added
|
||||
# complexity of per-job ephemeral provisioning for a job that runs at
|
||||
# most weekly. Revisit if run frequency increases or the threat model
|
||||
# changes; stopping already bounds the exposure window to the job's own
|
||||
# runtime on 1 day out of 7.
|
||||
# [ ] Run workflow_dispatch once and confirm: containment preflight passes,
|
||||
# the benchmark completes inside the job timeout, the results artifact
|
||||
# uploads, and a promotion (if any) opens a well-formed PR.
|
||||
@@ -77,13 +99,13 @@ jobs:
|
||||
github.event_name == 'workflow_dispatch' ||
|
||||
vars.GITNEXUS_EVOLUTION_ENABLED == 'true'
|
||||
)
|
||||
runs-on: ubuntu-latest
|
||||
runs-on: [self-hosted, linux, x64, gitnexus-evolution]
|
||||
# Gate promotion runs on a protected Environment. An admin must attach a
|
||||
# deployment-branch rule (main only) and ideally scope the three secrets to
|
||||
# it — server-side enforcement a dispatched non-main ref cannot bypass by
|
||||
# editing its own workflow copy. See the activation checklist above.
|
||||
environment: gitnexus-evolution
|
||||
timeout-minutes: 355 # ceiling just under GitHub's 360-minute hard cap
|
||||
timeout-minutes: 1440 # self-hosted ceiling is 5 days (7200min); 24h is a generous margin over a single-generation serial run
|
||||
permissions:
|
||||
contents: read # The promotion PR uses a short-lived App token minted below.
|
||||
env:
|
||||
|
||||
@@ -19,6 +19,7 @@ from workflow_bench.process_control import ManagedProcessResult, run_managed
|
||||
from workflow_bench.proposer_sandbox import (
|
||||
MAX_BUNDLE_BYTES,
|
||||
MAX_EVIDENCE_FILE_BYTES,
|
||||
SANDBOX_PYTHON3,
|
||||
SANDBOX_SHELL_PREFIX,
|
||||
SANDBOX_USER_SKILLS,
|
||||
ReadOnlyMount,
|
||||
@@ -162,6 +163,27 @@ def test_sandbox_command_has_minimal_mounts_and_no_host_root_bind(tmp_path: Path
|
||||
)
|
||||
assert probe.returncode == 0, probe.stderr
|
||||
assert probe.stdout == "/home/agent|/opt/claude:/usr/local/bin:/usr/bin:/bin"
|
||||
|
||||
# The evidence-provenance.mjs plan-writer's PATH-scan trusts a Python 3
|
||||
# candidate only if it (and its directory) is owned by root or by the
|
||||
# current process — real /usr/bin/python3 is root-owned on the host,
|
||||
# which surfaces as the kernel's overflow uid inside this
|
||||
# --unshare-user sandbox (root itself is never mapped in). This wrapper
|
||||
# is freshly created by the host process instead, so it's trusted, and
|
||||
# it must still exec through to a real, working Python 3.
|
||||
python3_index = argv.index(SANDBOX_PYTHON3)
|
||||
assert argv[python3_index - 2] == "--ro-bind"
|
||||
python3_wrapper = Path(argv[python3_index - 1])
|
||||
assert stat.S_IMODE(python3_wrapper.stat().st_mode) == 0o500
|
||||
version = subprocess.run(
|
||||
[str(python3_wrapper), "-I", "-S", "-c", "import sys; print(sys.version_info[0])"],
|
||||
text=True,
|
||||
capture_output=True,
|
||||
check=False,
|
||||
)
|
||||
assert version.returncode == 0, version.stderr
|
||||
assert version.stdout.strip() == "3"
|
||||
|
||||
assert SANDBOX_USER_SKILLS in argv
|
||||
user_skills_index = argv.index(SANDBOX_USER_SKILLS)
|
||||
assert argv[user_skills_index - 2] == "--ro-bind"
|
||||
|
||||
@@ -10,6 +10,7 @@ import yaml
|
||||
|
||||
from workflow_bench.runner import (
|
||||
aggregate,
|
||||
broken_incumbent_arms,
|
||||
build_parser,
|
||||
infra_error_record,
|
||||
normalized_model_identifier,
|
||||
@@ -64,6 +65,7 @@ def test_aggregate_takes_medians_and_counts_resolved():
|
||||
"valid_runs": 3,
|
||||
"excluded_runs": 0,
|
||||
"transcripts_missing": 0,
|
||||
"error_kinds": {},
|
||||
}
|
||||
|
||||
|
||||
@@ -333,6 +335,61 @@ def test_render_report_surfaces_excluded_and_unverified_runs():
|
||||
assert "no locatable session transcript" in report
|
||||
|
||||
|
||||
def test_render_report_surfaces_why_each_row_failed():
|
||||
results = {
|
||||
"t": {
|
||||
"workflow": aggregate(
|
||||
[record(resolved=False, error_kind="plan-evidence-invalid")],
|
||||
),
|
||||
}
|
||||
}
|
||||
report = render_report(results)
|
||||
assert "plan-evidence-invalid×1" in report
|
||||
|
||||
|
||||
def test_broken_incumbent_arms_flags_an_incumbent_that_resolved_nothing():
|
||||
results = {
|
||||
"t1": {"workflow": aggregate([record(resolved=False, error_kind="plan-evidence-invalid")])},
|
||||
"t2": {"workflow": aggregate([record(resolved=False, error_kind="plan-evidence-invalid")])},
|
||||
}
|
||||
assert broken_incumbent_arms(results, {"workflow"}) == ["workflow"]
|
||||
|
||||
|
||||
def test_broken_incumbent_arms_ignores_a_merely_underperforming_candidate():
|
||||
# The incumbent works fine; only the candidate arm fails. That's a normal,
|
||||
# expected "bad candidate" outcome and must not read as a broken harness.
|
||||
results = {
|
||||
"t1": {
|
||||
"workflow": aggregate([record(resolved=True)]),
|
||||
"candidate_workflow": aggregate([record(resolved=False, error_kind="verify-failed")]),
|
||||
},
|
||||
}
|
||||
assert broken_incumbent_arms(results, {"workflow"}) == []
|
||||
|
||||
|
||||
def test_broken_incumbent_arms_flags_an_incumbent_with_zero_valid_runs():
|
||||
# Every run excluded via an excluded-but-non-systemic error_kind
|
||||
# ("evidence-unverified"): valid_runs == 0 for every task, which the old
|
||||
# `valid_runs > 0` guard let sail through silently, and which the outage
|
||||
# streak breaker also doesn't catch (it resets rather than accumulates
|
||||
# on this exact error_kind -- see test_systemic_outage_streak_resets_on_non_outage).
|
||||
results = {
|
||||
"t1": {"workflow": aggregate([record(resolved=False, error_kind="evidence-unverified")])},
|
||||
"t2": {"workflow": aggregate([record(resolved=False, error_kind="evidence-unverified")])},
|
||||
}
|
||||
assert results["t1"]["workflow"]["valid_runs"] == 0
|
||||
assert broken_incumbent_arms(results, {"workflow"}) == ["workflow"]
|
||||
|
||||
|
||||
def test_broken_incumbent_arms_ignores_partial_incumbent_failure():
|
||||
# Resolved in at least one task — struggling, not broken.
|
||||
results = {
|
||||
"t1": {"workflow": aggregate([record(resolved=False, error_kind="verify-failed")])},
|
||||
"t2": {"workflow": aggregate([record(resolved=True)])},
|
||||
}
|
||||
assert broken_incumbent_arms(results, {"workflow"}) == []
|
||||
|
||||
|
||||
def test_infra_error_record_captures_the_failure_and_is_excluded():
|
||||
exc = subprocess.TimeoutExpired(cmd="claude -p", timeout=5)
|
||||
rec = infra_error_record(exc)
|
||||
|
||||
@@ -26,6 +26,7 @@ SANDBOX_HOME = "/home/agent"
|
||||
SANDBOX_TMP = "/tmp"
|
||||
SANDBOX_CLAUDE = "/opt/claude/claude"
|
||||
SANDBOX_SHELL_PREFIX = "/opt/claude/shell-prefix"
|
||||
SANDBOX_PYTHON3 = "/opt/claude/python3"
|
||||
SANDBOX_PATH = "/opt/claude:/usr/local/bin:/usr/bin:/bin"
|
||||
SANDBOX_GITNEXUS = "/opt/gitnexus"
|
||||
SANDBOX_GITNEXUS_SHARED = "/opt/gitnexus-shared"
|
||||
@@ -384,6 +385,24 @@ def _create_shell_prefix_wrapper(private_root: Path) -> Path:
|
||||
return wrapper
|
||||
|
||||
|
||||
def _create_python3_wrapper(private_root: Path) -> Path:
|
||||
"""A trusted, self-owned Python 3 launcher for evidence-provenance.mjs's atomic mover.
|
||||
|
||||
/usr/bin/python3 is a real system binary, but it's root-owned on the host.
|
||||
Inside this --unshare-user sandbox only the calling uid is mapped (root is
|
||||
not), so root-owned files surface as the kernel's overflow uid — which
|
||||
evidence-provenance.mjs's PATH-scan correctly refuses to trust. This
|
||||
wrapper is freshly created by the same host process that owns
|
||||
home/temp/shell-prefix, so it maps to the sandbox's own trusted uid
|
||||
instead, and simply execs the real interpreter through to do the work.
|
||||
"""
|
||||
|
||||
wrapper = private_root / "python3"
|
||||
wrapper.write_text("#!/bin/bash\nset -eu\nexec /usr/bin/python3 \"$@\"\n")
|
||||
wrapper.chmod(0o500)
|
||||
return wrapper
|
||||
|
||||
|
||||
def _resolve_executable(executable: Path | str | None, default: str) -> Path:
|
||||
raw = os.fspath(executable) if executable is not None else shutil.which(default)
|
||||
if not raw:
|
||||
@@ -641,6 +660,7 @@ def prepare_sandbox(
|
||||
directory.mkdir(mode=0o700)
|
||||
directory.chmod(0o700)
|
||||
shell_prefix = _create_shell_prefix_wrapper(private_root)
|
||||
python3_wrapper = _create_python3_wrapper(private_root)
|
||||
# Claude may discover user-level skills below HOME. Keep the rest of HOME
|
||||
# writable for normal CLI state, but overlay an immutable empty skills root
|
||||
# so a model cannot shadow the evaluated repository/plugin skill by name.
|
||||
@@ -651,6 +671,7 @@ def prepare_sandbox(
|
||||
*read_only_mounts,
|
||||
ReadOnlyMount(source=user_skills, target=SANDBOX_USER_SKILLS),
|
||||
ReadOnlyMount(source=shell_prefix, target=SANDBOX_SHELL_PREFIX),
|
||||
ReadOnlyMount(source=python3_wrapper, target=SANDBOX_PYTHON3),
|
||||
)
|
||||
primary: BaseException | None = None
|
||||
try:
|
||||
|
||||
@@ -679,13 +679,21 @@ def aggregate(records: list[dict[str, Any]]) -> dict[str, Any]:
|
||||
# unmeasured run makes the whole median unavailable so the gate won't rank
|
||||
# a candidate on a cost that was never actually captured.
|
||||
valid_costs = [r.get("cost_usd") for r in valid]
|
||||
out["cost_usd"] = None if (not valid or any(cost is None for cost in valid_costs)) else statistics.median(valid_costs)
|
||||
out["cost_usd"] = (
|
||||
None if (not valid or any(cost is None for cost in valid_costs)) else statistics.median(valid_costs)
|
||||
)
|
||||
out["resolved"] = sum(1 for r in records if r["resolved"])
|
||||
out["runs"] = len(records)
|
||||
out["valid_runs"] = len(valid)
|
||||
out["excluded_runs"] = len(records) - len(valid)
|
||||
out["transcripts_missing"] = sum(1 for r in records if r.get("transcript_missing"))
|
||||
out["class"] = records[0].get("class", "")
|
||||
error_kinds: dict[str, int] = {}
|
||||
for r in records:
|
||||
kind = r.get("error_kind")
|
||||
if kind:
|
||||
error_kinds[kind] = error_kinds.get(kind, 0) + 1
|
||||
out["error_kinds"] = error_kinds
|
||||
return out
|
||||
|
||||
|
||||
@@ -702,6 +710,33 @@ def savings(baseline: dict[str, Any], workflow: dict[str, Any]) -> dict[str, Any
|
||||
return out
|
||||
|
||||
|
||||
def broken_incumbent_arms(
|
||||
results: dict[str, dict[str, dict[str, Any]]],
|
||||
incumbent_arms: set[str],
|
||||
) -> list[str]:
|
||||
"""Incumbent arms that resolved nothing across every task they ran.
|
||||
|
||||
An incumbent arm is the currently-shipped, presumably-working skill: if it
|
||||
resolves NOTHING across every task it ran, that reads as an environment or
|
||||
harness failure (missing trusted interpreter, stale skill fingerprint,
|
||||
sandbox misconfiguration), not a skill regression. A candidate merely
|
||||
underperforming is a normal, expected outcome and must not trip this —
|
||||
only checking incumbents keeps that distinction.
|
||||
|
||||
Deliberately does NOT require valid_runs > 0 per task: an incumbent that
|
||||
fails every run with an excluded-but-non-systemic error_kind (e.g.
|
||||
"evidence-unverified", which the outage-streak breaker explicitly resets
|
||||
on rather than accumulates) would otherwise never accumulate a single
|
||||
valid run and sail through silently — the exact "quiet no-promotion"
|
||||
outcome this guard exists to catch, and arguably worse than the
|
||||
some-runs-resolved-zero case since here nothing completed at all.
|
||||
aggregate() never marks an excluded/unverifiable row resolved=True, so
|
||||
resolved == 0 alone already covers both cases.
|
||||
"""
|
||||
present = incumbent_arms & {arm for arms in results.values() for arm in arms}
|
||||
return sorted(arm for arm in present if all(arms[arm]["resolved"] == 0 for arms in results.values() if arm in arms))
|
||||
|
||||
|
||||
def _na(value: Any) -> Any:
|
||||
"""Render an unmeasured metric as ``n/a`` instead of a misleading number."""
|
||||
return "n/a" if value is None else value
|
||||
@@ -726,8 +761,8 @@ def render_report(results: dict[str, dict[str, dict[str, Any]]]) -> str:
|
||||
"efficiency, sum usage from the session transcripts instead",
|
||||
"(dedup events sharing one message.id).",
|
||||
"",
|
||||
"| task | class | arm | resolved | input | cache_create | cache_read | output | cost $ | wall s | turns | churn |",
|
||||
"| --- | --- | --- | --- | --- | --- | --- | --- | --- | --- | --- | --- |",
|
||||
"| task | class | arm | resolved | input | cache_create | cache_read | output | cost $ | wall s | turns | churn | errors |",
|
||||
"| --- | --- | --- | --- | --- | --- | --- | --- | --- | --- | --- | --- | --- |",
|
||||
]
|
||||
for task_id, arms in results.items():
|
||||
for arm, agg in arms.items():
|
||||
@@ -735,12 +770,14 @@ def render_report(results: dict[str, dict[str, dict[str, Any]]]) -> str:
|
||||
resolved_cell = f"{agg['resolved']}/{agg.get('valid_runs', agg['runs'])}"
|
||||
if excluded:
|
||||
resolved_cell += f" ({excluded} excluded)"
|
||||
error_cell = ", ".join(f"{kind}×{count}" for kind, count in sorted(agg.get("error_kinds", {}).items()))
|
||||
lines.append(
|
||||
f"| {task_id} | {agg['class']} | {arm} | {resolved_cell} "
|
||||
f"| {agg['input_tokens']:.0f} | {agg['cache_creation_input_tokens']:.0f} "
|
||||
f"| {agg['cache_read_input_tokens']:.0f} | {agg['output_tokens']:.0f} "
|
||||
f"| {_cost_cell(agg['cost_usd'])} | {agg['duration_s']:.0f} | {agg['num_turns']:.0f} "
|
||||
f"| {agg['diff_files']:.0f}/+{agg['diff_insertions']:.0f}/−{agg['diff_deletions']:.0f} |"
|
||||
f"| {agg['diff_files']:.0f}/+{agg['diff_insertions']:.0f}/−{agg['diff_deletions']:.0f} "
|
||||
f"| {error_cell} |"
|
||||
)
|
||||
for arm in arms:
|
||||
if arm != "baseline" and "baseline" in arms:
|
||||
@@ -749,7 +786,7 @@ def render_report(results: dict[str, dict[str, dict[str, Any]]]) -> str:
|
||||
f"| {task_id} | {arms[arm]['class']} | **{arm} savings %** | — "
|
||||
f"| {s['input_tokens']} | {s['cache_creation_input_tokens']} "
|
||||
f"| {s['cache_read_input_tokens']} | {s['output_tokens']} "
|
||||
f"| {_na(s['cost_usd'])} | {s['duration_s']} | — | — |"
|
||||
f"| {_na(s['cost_usd'])} | {s['duration_s']} | — | — | — |"
|
||||
)
|
||||
lines.append("")
|
||||
all_aggs = [agg for arms in results.values() for agg in arms.values()]
|
||||
@@ -1333,6 +1370,17 @@ def main() -> None:
|
||||
}
|
||||
(out_dir / "promotion.json").write_text(json.dumps(promotion, indent=2) + "\n")
|
||||
print(f"\n{report}\n\nWritten to {out_dir}/")
|
||||
broken_incumbents = broken_incumbent_arms(results, set(CANDIDATE_ARMS.values()))
|
||||
if broken_incumbents:
|
||||
# Fail loudly rather than let a broken environment read as a quiet
|
||||
# "no promotion, incumbent stands."
|
||||
print(
|
||||
f"[harness-health] incumbent arm(s) {', '.join(broken_incumbents)} resolved zero "
|
||||
"tasks across every valid run — this looks like an environment/harness failure, "
|
||||
"not a normal candidate miss. See the errors column in report.md and error_detail "
|
||||
"in results.jsonl. Exiting non-zero rather than reporting a quiet no-promotion."
|
||||
)
|
||||
raise SystemExit(1)
|
||||
if outage_tripped:
|
||||
# Non-zero exit so a driver (evolve.py) treats the partial benchmark as a
|
||||
# failed run and halts instead of proposing from outage-truncated evidence.
|
||||
|
||||
Reference in New Issue
Block a user