33 commits

Author	SHA1	Message	Date
Gergő Magyar	e2da8d90ce	test(eval): run the benchmark offline against a scripted provider (#3235) ... * feat(eval): a scriptable stand-in for Anthropic and OpenAI Every defect this harness shipped last round was invisible to its own tests for one reason: the tests exercised a layer BELOW where the code runs. The usage log was never written because the proxy is a subprocess with a constructed environment. The callback could not be imported because LiteLLM loads it by path, not as a package. Failures went unrecorded because only the async hook was overridden. CI or review caught all three; no unit test could, because each called the function directly instead of driving the path that calls it. This closes that gap without spending money. It speaks the two wire protocols the harness actually depends on - Anthropic Messages, streaming and not, and OpenAI Responses - so a run can go through the real sandbox, the real CLI, the real gateway and the real usage callback with only the model faked. The runner already supports pointing at it: --base-url is the same path the free-model proxy documentation uses. Scripted rather than simulated. A test decides what the model says, which tools it asks for, and exactly what usage it reports. That last part is what makes provider-native accounting testable at all: real cache hits are not reproducible on demand, but a declared cache_read of 44,000 is. One Reply served down both protocols is also the cleanest demonstration that the same billed work is stated as a sum on one side and as a whole on the other. Tool blocks are the mechanism for artifact-producing cells. The CLI runs what it is asked to run, so a scripted Write block makes it write that file inside the sandbox for real - no model deciding anything. The end-to-end test drives the real proxy against the mock and asserts the usage log records the provider's own arithmetic through the Anthropic-shaped translation. It SKIPS here, because litellm's console script is absent in this environment, so it is unverified until CI runs it - the same footing the bubblewrap canary started on, and that one found a real bug on its first CI run. Not yet built: driving a whole sweep against this. That needs a scripted reply sequence that carries a cell to a scored artifact, which is the next step and the point of the exercise. 668 eval tests pass, 17 skipped; the two test_model_gateway.py failures are the pre-existing environmental ones. * test(eval): run a real session against the scripted provider The mock only proves something once the harness runs against it. This adds the stand-in CLI and the first integration tests that use it, so a session goes through the real code with only the model faked. tests/fixtures/fake_claude.py does what the CLI does at the two boundaries the harness depends on: it calls ANTHROPIC_BASE_URL for a turn, EXECUTES the tool blocks that come back, and prints the stream-json sequence the parent parses. Everything between - the session runner, the event-stream parse, the usage extraction, the artifact capture, the scorer - stays real. Four tests, chosen for the layers that have actually broken here: the usage a provider reported survives to the row, a scripted Write produces an artifact parse_review_output accepts, the prompt the harness meant to send is what arrived, and an upstream 529 lands as a failed session rather than a usable measurement. Writing the stand-in found two things worth keeping. The prompt arrives on STDIN under "-p --input-format text"; scanning argv for a non-flag token picks up a flag's value instead, and the prompt-fidelity test is what caught it. And three of these tests had been holding a sandbox they never applied, since no command_prefix is passed - that implied coverage which was not there, so the sandbox is gone from them and stays only in the artifact test, which needs its review directory. What these do NOT cover, checked rather than assumed: making the stand-in write in place instead of atomically still passes. On the host-unsafe backend there is no read-only mount to refuse it, so the atomic-write requirement remains a bubblewrap mount property that only the real-sandbox canary can prove. Dropping cache_read from the recorded usage does fail, so that half is genuinely pinned. 672 eval tests pass, 17 skipped; the two test_model_gateway.py failures are the environmental ones. * fix(eval): the usage adapter read a shape the callback never receives Running the gateway against the scripted provider proved the accounting merged in #3220 does not work, and the same run showed why nothing had caught it. LiteLLM does not hand a logger the upstream body. It normalises usage into its own Chat-Completions-shaped object first, so an OpenAI Responses reply reaches the callback as prompt_tokens / prompt_tokens_details.cached_tokens - never the input_tokens / input_tokens_details the shipped adapter reads. Every field came back unknown. The observed call_type is "anthropic_messages" as well, because Claude Code calls the Anthropic-shaped endpoint, so canonical_provider returned None and normalize_usage would have refused outright. Both were assumptions about a boundary I had only read about. The unit tests agreed with them because their fixture was written in the same wrong shape, so producer and consumer were consistent and both wrong - the exact failure the producer/consumer round trip exists to catch, one layer further out. Adds a LITELLM_NORMALIZED adapter for the object that actually arrives. The arithmetic is still OpenAI's - prompt_tokens is the whole, the details are subsets - so ordinary input is recovered by subtraction. The Responses adapter stays for a raw upstream body, which the mock still serves and tests directly. An unrecognised provider is still refused rather than guessed. The fixtures now carry the measured shape, and the end-to-end test asserts it through a real proxy: 48k prompt tokens with 44k cached is read back as 3k ordinary rather than as silence. 676 eval tests pass, 16 skipped, none failing. * test(eval): run a whole sweep offline, with negative controls The layers between a model turn and a promotion decision had never been exercised together. Unit tests covered each alone, and the paid runs that would have covered the composition kept dying, so the contracts BETWEEN them went unverified - which is where this harness has repeatedly shipped bugs. Drives runner.main() the way the workflow does. Real task selection, hidden oracle capture, sandbox, CLI subprocess, artifact capture, scoring against the oracle, aggregation, health guard and promotion gate. Only the model is scripted. Getting to green meant satisfying nine real contracts nothing had exercised end to end, and each failure was the harness correctly refusing bad evidence: --unsafe-no-bwrap is restricted to the paired review arms; ce_* needs a plugin carrying ce-plan, ce-work and ce-code-review; candidate_* needs an overlay; the clone needs .gitnexus/meta.json with indexedAt and lastCommit; the evidence gate needs a Skill request with a non-error result; review findings need exactly ten fields with severity in critical/high/medium/low; and the hidden labels use a DIFFERENT schema from the review output - line_start/line_end, six fields. That last one only a real run surfaces. Three negative controls, because a scorer that cannot be wrong measures nothing. A finding in the wrong place is tp=0 fp=1 fn=1 and oracle-failed, while its evidence stays VALID - being wrong is a quality result, not a broken measurement. Approving defective code is a miss with no false positive, and precision is None rather than 0, because it is undefined with no predictions. One run cannot promote: the gate says it needs three valid paired runs. A fourth control exists because a mutation demanded it. Forcing skill_was_invoked_events to return True left every other test here passing, so nothing pinned the gate that separates measuring a SKILL from measuring a model. Writing it turned up behaviour worth recording rather than assuming: a skill-not-invoked row still carries its score AND still counts toward the arm median, because aggregate() drops EXCLUDED_ERROR_KINDS and evidence_valid=False and skill-not-invoked is neither. The health guard stops the sweep, so a single-run sweep cannot promote on it, but a mixed run's median would include a cell whose skill never ran. Pinned as-is so it cannot change silently in either direction; changing it is a promotion-semantics decision, not a test fix. Two provisioning steps are stubbed and neither is harness logic: the pinned runtime mounts (no node_modules in a worktree) and the sanitized graph build (needs the gitnexus CLI at a mounted path). Containment is host-unsafe here; bubblewrap stays with the real-sandbox canary. 681 eval tests pass, 16 skipped, none failing. Runs in ~18s. * fix(eval): an uninvoked skill must not move the arm's quality median Found by the offline sweep: a skill-not-invoked row still carried its score into the arm's quality median. aggregate()'s filter dropped EXCLUDED_ERROR_KINDS and evidence_valid=False, and skill-not-invoked is neither, so an arm could be credited for a review it never performed with the skill under test - which is the one thing an arm exists to measure. Excluded from the QUALITY metrics only. Cost and duration still count that row, because the session really ran and really was billed, and the promotion gate still sees it, because it has its own vocabulary for a candidate that never loaded its skill. Two wider fixes were tried and abandoned, both because the tests said so rather than because I reasoned it out first. Reusing the health guard's evidence_failed predicate also excluded transcript-missing rows, but test_aggregate_excludes_session_error_rows_from_medians pins those as counting: that session ran, only its transcript is unverifiable. Excluding the row from `valid` outright turned a candidate whose skill never loaded from keep_incumbent into insufficient_evidence - the safety property held either way, but the decision vocabulary is promotion semantics and not mine to change on a measurement fix. Mutation-checked: putting the rows back into the quality median fails the new test. Both directions asserted, since a filter that excludes everything would also pass - a wrong-but-valid review still moves quality, because being wrong is exactly what a quality median should reflect. 682 eval tests pass, 16 skipped. * test(eval): run the offline sweep unstubbed in the job that can, and probe CLI identity Items 5 and 6 turned out to be one change. The containment (ubuntu) job already installs bubblewrap, the pinned Claude CLI, node_modules and a built GitNexus - everything the sweep's two provisioning stubs stand in for. So the stubs are not a property of the test, only of a machine that lacks those things. GITNEXUS_REQUIRE_FULL_SWEEP=1 makes the sweep run with nothing stubbed: real containment instead of --unsafe-no-bwrap, the real runtime mounts, the real sanitized graph. Set in that job, following the GITNEXUS_REQUIRE_BWRAP_CANARY pattern already there. The gate FAILS on a missing piece rather than degrading to the stubbed path, which is the point - a green tick that silently tested less is what the bubblewrap canary was written to prevent. Verified both states here: default green, and gate-on fails on this machine rather than skipping, since it cannot create user namespaces. Item 7 is an experiment, not an answer. Per-cell attribution needs an identifier that travels WITH the request, because one proxy serves the whole sweep and anything read from its environment is identical for every call. What the real CLI sends is not documented anywhere I can check, and guessing a wire format is exactly how the last three accounting bugs happened. So the probe drives the REAL pinned CLI against the mock and records the identity-bearing headers and body keys that arrive. It asserts only that a request was made; the recorded evidence is the deliverable, and the job log preserves it. Skips without CLAUDE_CANARY_BIN. Two guards caught this rather than review: the repo pins the containment job's env and its exact test list, so both had to be updated deliberately - which is the guard working, not friction. 682 eval tests pass, 17 skipped. * test(eval): make the offline sweep cross-task, so a scheduler change is checkable The sweep fixture had one task, and a single task cannot show the thing a cross-task scheduler changes: waves are per-task, so ordering, packing and a breaker spanning a task boundary are all invisible with one. A second task with its defect in a DIFFERENT file, and its own hidden labels, makes per-task routing observable. The scripted reply is now task-aware, which matters for the same reason: replying with the first task's finding scores the second task wrong. The load-bearing assertion is that each task scored against ITS OWN oracle. That is the dangerous failure mode of interleaving cells from different tasks - a mis-routed context or artifact scores one task against another's labels, and every row still looks green. Mutation-checked: pointing every cell at the first task's oracle snapshot fails it. This is the safety net the packed-scheduler wiring needs. Measured earlier against the real sweep_packed_cells, that change is worth -27% on a cold sweep and -37% weekly, with breaker fidelity holding at three injected failure positions - but it restructures a 125-line loop across ~92 names that also holds graph prefetch, reuse selection, oracle staging and the canary drop. Landing that on top of a one-task fixture would have been unverifiable, which is why this comes first and separately. 682 eval tests pass, 17 skipped. * fix(eval): commit the stand-in CLI's executable bit The file was created and chmod +x'd locally, but committed 100644 - so the mode existed only in my working tree. Any fresh checkout, CI included, gets a non-executable file and every cell dies with "required executable is not an executable regular file". Found by accident: checking out origin/main and back to compare a flaky test restored the file from the index and stripped the bit, which turned 5 green tests into 9 failures. Without that detour this would have failed on the first CI run instead. Same shape as the bugs this branch exists to catch - something that works only because of local state, breaking where the code actually runs. * fix(eval): apply code review findings Seven local reviewers and an independent cross-model pass. The headline is that a fix I added in this branch was worse than the gap it closed. Reverted the aggregate() quality-median filter. Excluding skill-not-invoked rows from the quality metrics left valid_runs and excluded_runs still counting them, so the promotion gate saw N clean runs while the median came from fewer. The dropped rows are systematically an arm's worst, so it biased toward PROMOTING - reproduced: one real run at 0.9 plus two uninvoked rows at 0.0 gave the gate 3 valid runs, zero exclusions and a 0.9 median, flipping keep_incumbent to promote. Three verdict fields compounded it: they are all() reducers still reading the wider set, so one uninvoked cell flipped a whole arm. Five reviewers found the two halves independently. Closing it honestly needs a scored-run count plus a paired-equality check in the gate, which is promotion semantics rather than an aggregation fix. The gap is now pinned by a test that states why the half-fix was reverted. Stopped forging the absence of CI. The runner refuses --unsafe-no-bwrap when CI is set because that mode runs sessions with bypassPermissions behind a boundary its own docstring calls "not a security boundary"; the sweep test deleted CI to get past it, so eval / locked pytest ran an uncontained agent sweep on the runner holding the checkout and credentials. It skips under CI instead - the containment job still runs it for real with GITNEXUS_REQUIRE_FULL_SWEEP=1. The stand-in CLI was lying in three ways. It never set is_error, so a refused write read as a completed one. It had no Skill branch at all, so honoring is_error revealed the evidence gate had been satisfied by a tool the fixture never ran - the gate was measuring the fixture, not a skill. And a reply with no usage became four zero-valued fields plus a fabricated cost, which is exactly the unknown-is-not-zero confusion the accounting it feeds exists to prevent. A provider failure also crashed the subprocess with no terminal result event. The identity probe never ran anywhere. test_mock_provider.py was in no job's file list, and the only job setting CLAUDE_CANARY_BIN runs a fixed list. My commit message claimed the next containment run would produce the answer; it would not have. Now wired in, with the CI-shape test updated to pin it. Also: the regex-miss fallback wrote a predictable name in shared /tmp through a symlink-following stage, now scoped to the test's own directory; and the canonical_provider docstring plus the callback comment still asserted a call_type branch the code no longer has. Deferred as design decisions rather than review fixes: the containment sweep uses the stand-in CLI rather than the pinned real one, the full-sweep path bypasses the gateway so native usage accounting is unexercised there, _normalize_litellm duplicates the Responses algorithm, and OPENAI_RESPONSES is now unreachable from canonical_provider. 682 eval tests pass, 17 skipped, ruff clean. * fix(eval): carry scripted tools over the Responses protocol Review round on #3235. Three real items; five more were already fixed in a20f94e1c and are answered on their threads rather than re-fixed. `_openai_response` emitted only an `output_text` item and never read `reply.tools`, so a reply scripted with a Write or Skill crossed the gateway with the tool silently dropped. Responses is the protocol the gateway is configured for BECAUSE it carries tool use, so the mock was wrong about the wire on the one path that matters most. Function-call items now accompany the message. Mutation-checked: reverting the emit fails the new test on "the scripted tool must cross the Responses path". The artifact session now takes `command_prefix` and `require_pid_namespace` from the sandbox the way `run_arm` does instead of calling `run_claude` bare. On host-unsafe `command_prefix_for` returns `[]` by construction, so this pins the wiring, not the isolation - the comment says so rather than implying more. CodeQL's three unused-variable reports on one line were one finding: a call whose result is entirely discarded. Unpack nothing there. 683 passed, 17 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(eval): forward the usage the provider reported instead of zero-filling it Review round on #3235; all three findings valid. The stand-in CLI defaulted absent cache fields to 0. That fabricated a complete measurement out of an incomplete reply, and the second-order effect was worse than the first: `runner_sessions` requires all four USAGE_FIELDS before it calls a session measured, so a stand-in that always emitted four fields made that guard unfirable from any offline test. It was always satisfied. It now forwards exactly what arrived. `Reply`'s cache fields accept None to script absence, since a consumer that cannot tell "omitted" from "zero" is the bug this harness exists to catch. Mutation-checked: restoring the zero-fill fails the new test. Also corrected a comment claiming aggregate() excludes skill-not-invoked rows from the quality median. It does not - that was the filter reverted in a20f94e1c for inverting a promotion, and the comment survived the revert describing the opposite of what the test pins. Dropped an unused monkeypatch fixture arg. 684 passed, 17 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(eval): keep an omitted cache field omitted on the Responses wire Review round on #3235. The main finding is a miss in my own previous commit: that one taught the Anthropic path to forward absence instead of zero-filling, but `_openai_response` still serialized both `input_tokens_details` keys unconditionally. Collapsing None to 0 is right for the arithmetic - an unreported field adds nothing to the total - and wrong on the wire, because `_int_or_none` reads an absent key as unknown and a present 0 as a measured zero. So a reply scripted with `cache_read_input_tokens=None` was indistinguishable from a provider-reported zero on exactly one of the two protocols. Half-applying the invariant was arguably worse than not applying it: the Anthropic test passing made the pair look covered. Mutation-checked: restoring the unconditional keys fails the new test. Also: the module docstring claimed the stand-in executes the tool blocks that come back, without noting Bash is stubbed; and dropped an unused tmp_path fixture arg. 685 passed, 17 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(eval): validate every usage field the stand-in forwards Review round on #3235. The guard checked input_tokens and output_tokens for type and sign, but the forwarding comprehension passed the cache fields through unchecked whenever present. The parent's well_formed test only asks whether the four keys are PRESENT, so a negative, boolean, or non-integer cache value rode into a `success` result and was recorded as a usable measurement. Same shape as the previous two rounds: the required half of a pair was handled and the optional half was not. A field good enough to report is good enough to check. Mutation-checked: dropping the added clause fails all three parametrized cases (negative, boolean, string). 688 passed, 17 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(eval): say which stand-in tools execute and which are modelled Review round on #3235. The previous commit's docstring fix said "Write and Skill really run" while correcting the Bash claim. Only Write really runs: Skill validates the request and returns a synthetic result. Fourth round of the same shape - the reported half of a pair gets fixed and the sibling keeps the overclaim. Both docstrings now name each of the three branches and what it actually does. 688 passed, 17 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-09-09 12:34:00 +01:00
Gergő Magyar	b1d87c1f33	fix(eval): sweep evidence handling and measurement health, with guarded comparator reuse (#3207) ... * fix(eval): cut skill-evolution wall clock without shrinking the gate Reuse matching incumbent/CE cells, sanitize each SHA once, and default dispatch workers to 3 so weekly review generations finish inside the EventBridge window. Cap the sweep from leftover instance uptime so a Friday dispatch still uploads evidence. Co-authored-by: Cursor <cursoragent@cursor.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(eval): pipeline graph setup and correct the wall-clock cost model The evolution sweep paid `sanitize` + `analyze --pdg --index-only` for every unique task SHA on the critical path, one at a time, with nothing overlapping. `_run_sweep` now starts the next unpaid SHA's clone template and graph snapshot on a prefetch thread as soon as the current task's cells are dispatched, so every SHA but the first hides behind a paid session wave. The thread is joined before that SHA is used and before the trees tempdir is torn down, and a prefetch failure is recorded against the SHA exactly as an inline failure is. Tasks whose cells are all reusable comparator rows are not prefetched: they never build a graph, so priming one would be pure cost. Adds `measure_evolution_cost.py`, the cost model behind these numbers. It reads the review corpus, the evolve defaults, and the workflow's workers default — it does not start a session. Its first version charged `copy_isolated_tree` once per paid cell, serially. `run_cell` clones inside its own pool worker, so the clones in a wave overlap and only one is on the critical path per wave; the model now charges `ceil(cells / workers)` waves. Estimated review generation at workers=3: cold 21570s, weekly 7710s. Wall clock is quantised by `ceil(cells_per_task / workers)`. A cold review task is 9 cells, so workers=4 buys the wall clock of workers=3 and pays host contention for it. Documented in the workflow's rollout checklist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(eval): price the benchmark against measured cell durations The cost model assumed every cell runs the 1140s mean. Cells are not uniform: the 41 rows in Actions run 33912693948's artifact are 826s at the median, 1262s at the mean, 2976s at p90, with two pinned at the 5400s session ceiling. A wave waits for its slowest cell, so a mean understates every concurrent schedule — the previous model called workers=3 cold 5.99h when the same schedule against real durations is 10.33h. session_durations.json carries the sample in submission order with its provenance and its caveat: every cell in that run returned unusable evidence, so the durations are real but a clean run may sit lower. It is the only live artifact; the 2026-07-22 green run's has expired. The model now simulates the schedule cell by cell rather than multiplying a mean by a wave count, averaged over all 41 rotations of the sample so no single alignment between sample order and cell index decides the answer. It prices today's barrier (wave_makespan) against a continuously fed pool (fed_makespan) and reports both, and it charges the proposer session — one per generation, measured at 344.7s — which it had been omitting entirely. Measurement only; no runtime behaviour changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(eval): price arms separately and stop inventing setup constants Two errors in the model, both found by auditing it against the artifact it claims to describe. The arms are not interchangeable. `candidate_review` runs 1416s at the mean against `review`'s 1204s and `ce_review`'s 1176s, and the weekly lane pays the candidate arm and nothing else — reuse skips both incumbents. Pricing weekly from a pooled sample charged it for arms it never runs: weekly is 4.59h, not the 3.65h a pooled sample reported. Cells are also submitted run-major and arm-minor, so at workers=3 every wave holds one cell of each arm and the slowest arm sets the wave; the model now builds cells in that order. The setup constants were invented. GRAPH_ANALYZE_SECONDS=600 and TEMPLATE_SANITIZE_SECONDS=180 charged 3900s of per-SHA setup for a cold run — more than the entire non-session time of the source run, which was 2541s for 41 cells and 5 SHAs. `duration_s` is the sum of a cell's Claude sessions (runner_sessions.py), so that 2541s residual is every clone, graph build, sandbox and teardown the sweep paid. The model now charges the measured residual per cell, 62.0s, and no longer credits clone templates or graph prefetch: both landed after that run and there is no measurement of them yet. The residual bounds what they can be worth. Cold 37452s (10.40h), weekly 16541s (4.59h), against a fed pool at 31683s and 16541s. Measurement only; no runtime behaviour changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(eval): charge sweep overhead where more workers cannot dissolve it Three defects, found by auditing the model against the artifact again. The overhead was charged inside the schedule. session_durations.json claimed the residual was charged "per cell and serially - the pessimistic reading", but task_cells folded it into each cell's duration, where the pool then divided it by the worker count. The residual mixes per-cell work the pool really does divide with per-SHA graph setup it cannot, and the artifact cannot separate them, so it now sits outside the schedule: cold 11.09h, not 10.40h. Alignment averaging weighted the shortest sample twice. The arm samples are 13, 14 and 14 long and the average ran over max()=14 offsets, so candidate_review's first cell was counted twice and its last never. Averaging over lcm()=182 offsets weights every arm's sample evenly. The wall assumed all 54 cells run. Replaying the sample's own error_kind sequence through today's systemic_outage_streak trips the outage breaker at cell 5 of 41. The source run executed all 41, so its runner did not break on that sequence, but the current one would: these numbers price a HEALTHY sweep, and a sweep with the sample's failure profile never reaches them. Stated on generation_seconds and recorded next to the sample it qualifies. Measurement only; no runtime behaviour changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(eval): give the review agent somewhere it can actually write Every review cell in the last recorded generation returned unusable evidence. Not some — all 41, across all three arms and all six tasks, at $3653 for the run. The transcripts say why, 127 times across 35 of 35 sessions: EROFS: read-only file system, open '/workspace/review-output.json.tmp.2.90a76e583b0c' The review arm mounted the artifact as a writable FILE at /workspace/review-output.json while binding /workspace read-only. The Write tool writes atomically: it creates `<target>.tmp.<n>.<hex>` beside the target and renames it. The parent was read-only, so the temp create failed and the artifact was never written. A writable file inside a read-only directory is not writable to anything that writes atomically. Agents tried /proc/self/root/workspace/... and /proc/1/root/workspace/... to get around it; all 41 artifacts came back 0 bytes. The artifact now lives in its own writable directory bound at /review-output, outside the workspace. That is what a rename needs, and it lets the workspace get stricter rather than looser: the review phase may now change nothing there at all (enforce_phase_workspace gained allowed_artifact=None), where before it was entitled to one path inside it. The file is no longer pre-created — the agent writes it, and absence is now meaningful evidence. parse_review_output reported every one of these as "review output is not valid UTF-8 JSON". The file was empty, and its except folded OSError, UnicodeError and JSONDecodeError into that one string, so a sandbox that made writing impossible was indistinguishable from an encoding fault. That is why this read as an agent-quality problem for fifteen consecutive non-green runs. Each cause now names itself: never written, empty, not valid UTF-8, not valid JSON with the decoder's position. run_arm also keeps the FIRST error_detail, as it already did for error_kind, so a phase-boundary violation is no longer buried under the parse failure it causes. The test double conflated sandbox.private_root with the clone, which put the artifact directory inside the workspace and would have hidden the stricter check. Regression tests pin the mount shape in the generated bwrap argv, the contract path in the prompt, the four parse diagnostics, and the untouched-workspace contract. Verified by unit tests only: this container has unprivileged user namespaces disabled, so bwrap cannot run here and the mount was not exercised end to end. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(review): close the artifact path in every layer that gates it Code review of this branch found the relocated review artifact was fixed in the bwrap mount and nowhere else. Four independent layers decide whether the agent can write it, and three still named the old location. Claude Code applies its own filesystem policy to its own tools, and build_claude_settings listed only /workspace, /tmp and /home/agent under allowWrite with denyRead ["/"]. The artifact used to live under /workspace, so this list was correct until it moved. SANDBOX_REVIEW_OUTPUT is now in allowWrite and allowRead; without it the bwrap bind grants a write the CLI then refuses. The task corpus still ran `test -s review-output.json` from the workspace, in a separate sandbox invocation that never sees the artifact mount. Every review cell would have been stamped verify-failed with resolved=False no matter how good the review was, which also made those rows permanently unreusable and so silently disabled this branch's own comparator reuse for review arms. The verify and hidden-oracle commands now read the location from GITNEXUS_BENCH_REVIEW_OUTPUT and get the directory bound read-only, mirroring the mount-plus-env-var shape _run_hidden_oracle already used. host_text and host_path did not translate the new path, so the host-unsafe backend told the agent to write somewhere that exists on neither backend. Adding the mapping exposed a second defect: host_text substituted every occurrence of a target, and "/review-output" appears twice in "/review-output/review-output.json" - once as the directory and once inside the filename. Matching is now anchored to a path boundary. Comparator reuse had three ways to accept evidence it should have rejected. A row with no runtime_digest passed the drift lock because the guard only compared when both sides were bound, and the branch's own test asserted that as correct; absence is now a mismatch and the test states the rule. materialize_reused_row overwrote recorded_at with the copy time while the age check read that field, so a row copied forward each generation refreshed its own clock and never aged out; the first measurement time is now preserved and aged against. A future-dated stamp passed a one-sided bound and is now rejected as corrupt. RUNTIME_DIGEST never reached the runner at all: runner_environment builds a fixed dict and process_control replaces the child environment wholesale, so the digest the workflow exports was dropped and the lock it feeds was inert. The instance-window deadline was also checked only after run_proposer returned, buying a proposal the generation had no room to benchmark. The graph prefetch thread was started without copy_context, so it never saw the cancellation ContextVar the rest of the sweep shares, and the outage breaker returned without setting cancel_event - together, a tripped breaker would block on joining a prefetch that was never told to stop. Both fixed, with outage checked before cancellation at the two exits so an outage keeps exit 1 instead of becoming a Ctrl-C's 130. Both bwrap canaries that actually execute a write still bound the pre-fix shape against a file this branch no longer creates, so they would have errored rather than caught anything. They now bind the directory and write atomically - temp file beside the target, then rename - which is the exact operation that failed with EROFS. A source-text assertion over inspect.getsource(run_arm) was replaced with one that inspects the real mount, and a wall-clock assertion was pinned to a fixed monotonic clock. Not applied, and why: binding task-asset and dependency digests into comparator reuse needs asset snapshots prepared before the reuse decision rather than inside the per-task loop, and shipping the comparison without that would add a guard that silently never fires. Forcing a paid canary cell per incumbent arm and folding reused rows into the outage streak are behaviour decisions, not fixes. Clone-template reuse still has no test. The cost model's per-cell residual still shrinks with arm count, overstating weekly savings by at most the 2541s residual; the docstring now says so rather than inventing a split. 585 eval tests pass, ruff clean, 29 workflow contract tests pass. The two test_model_gateway.py failures are pre-existing and fail on main. * fix(review): bind reuse to its environment and keep the health canary real Applies the five findings the previous review round left open. Comparator reuse ignored the environment a row was measured in. TaskReuseBinding carried the task and oracle identity but not the task-asset or sandbox-dependency digests, and this branch itself changes sandbox_dependencies in the review corpus - so a reused comparator could be measured against one dependency set and compared against a candidate built on another, handing the gate a false comparison. Closing it needed the digests to exist before the reuse decision, so asset snapshots are now prepared for every task up front instead of lazily inside the per-task loop. That also removes the concurrent TaskAssetCache.prepare the prefetch thread could otherwise race, which the file's own "plain dict, read-then-write race" comment warned about. Both digests fail closed on either side, matching the runtime digest. The broken-incumbent canary could not fire when reuse was working. It read `resolved`, which counts reused rows, so an arm whose cells were all reused always looked healthy - in precisely the run where a broken environment would go unnoticed. aggregate now also reports `resolved_fresh` and the canary reads it. That count would be vacuous if an arm were reused end to end, so the sweep keeps one paid cell per incumbent arm and says which one it kept. Reused rows did not participate in the outage streak, so a run of failures could carry across them and trip on stale history. A reused success now resets the streak the way a paid success does. The cost model charged sweep overhead per cell, which credited a weekly generation for shrinking work it still performs: it pays one arm instead of three but builds exactly the same graphs. Overhead is charged per SHA now. Weekly is 5.20h rather than the 4.80h the per-cell rate reported; cold is 10.86h. The residual still cannot be split between per-SHA and per-cell work from one artifact, so session_durations.json records that assumption and the direction it errs in, rather than leaving a number nobody can trace. Clone-template reuse - the branch's core speedup, taken on essentially every multi-cell sweep - now has a test that builds a real sanitized template, asserts the cell runs against the copy with the template's HEAD, and fails if run_cell re-clones. A second test asserting only on a namespace built inside the test was written and deleted: it exercised nothing, which is the failure this review round penalised elsewhere. 589 eval tests pass, ruff clean, 29 workflow contract tests pass. The two test_model_gateway.py failures are pre-existing and fail on main. * refactor(eval): consolidate duplicated harness logic after the review round Simplification pass over the branch. Behavior-preserving throughout; three reviewers, nine findings applied, two skipped. The review-artifact block in _run_hidden_oracle was unreachable. That function runs only in run_arm's non-review branch, while the directory it probes for is created only in the review branch, and each sandbox serves exactly one arm - so `review_artifact.parent.is_dir()` could never be true. It was added an hour earlier to make the hidden oracle resolve the moved artifact; the oracle never runs for review tasks, so the guard was dead on arrival. Deleting it also removes the duplication it had with the verify-command wiring. EXCLUDED_ERROR_KINDS is now one definition. runner.py and comparator_reuse.py each carried the same six-member frozenset, kept in sync by a comment. Only one direction is possible: runner already imports from comparator_reuse, so the reverse import fails at module-init with a circular-import error. That is now stated where the alias lives, so nobody tries it the other way. ensure_task_graph and prefetch_next_graph shared ten keyword parameters, passed through two call sites and forwarded whole between them. They now take a GraphBuildEnv, mirroring TaskCellContext, which already bundles per-cell state in this file. Its ready_keys() replaces an inline four-set union at the call site. Smaller consolidations: _sha256_file's hand-rolled chunk loop becomes hashlib.file_digest (3.11+, already used in runner_artifacts); _copy_owner_only reuses task_assets._write_all and COPY_CHUNK_BYTES instead of repeating the short-write retry; its stat-then-open existence check becomes the O_EXCL failure it was already relying on, which is atomic rather than merely narrow; and runner_environment reads the digest through comparator_reuse.current_runtime_digest instead of re-parsing the environment variable. Three test docstrings summarised the branch's own history ("the branch's core speedup", "the regression that produced fifteen runs") rather than the invariant under test. Rewritten to state the constraint, which is what survives the merge. Repaired the indentation left behind by the outage-streak edit and flattened the prefetch dispatch from three nested conditionals to one. Skipped: consolidating comparator_reuse._real_directory onto proposer_sandbox's same-named helper - they differ, the sandbox one rejects any symlink in the resolved path while this one checks only the leaf, so sharing it would tighten behavior rather than preserve it. That needs a decision about which policy the reuse path wants, not a simplification. 589 eval tests pass, ruff clean, 29 workflow contract tests pass. Unrelated and pre-existing: two test_model_gateway.py failures, and test_process_control.py::test_timeout_kills_term_ignoring_descendants_before_they_write, which is a TERM-to-KILL timing flake (passes 2 of 3 in isolation) in a file this branch does not touch. * refactor(eval): name the reuse directory check for the promise it makes The simplification pass left one finding open: comparator_reuse and proposer_sandbox both defined `_real_directory`, same name and same shape, with different guarantees. The sandbox one rejects every symlink hop in the path; the reuse one checks only the leaf and resolves through parents. Sharing the name invites a consolidation that would silently tighten one of them. They should not be merged, so the name stops claiming they could be. proposer_sandbox guards a mount root, where a symlink hop changes what an untrusted session is handed. comparator_reuse guards a data directory whose contents are already validated one file at a time - reads go through _regular_file, which lstats and rejects symlinks, and writes through O_NOFOLLOW. A symlinked parent therefore grants nothing those guards do not already cover, while refusing one would reject a symlinked artifacts directory or macOS's /var for no gain. Renamed to _resolved_directory, with the reasoning recorded at the definition, and a test that pins both halves: a symlinked parent is accepted and resolved, a symlinked leaf is still refused. Behavior is unchanged. 591 eval tests pass, ruff clean. The two test_model_gateway.py failures are pre-existing and fail on main. * test(eval): measure the sweep scheduler instead of modelling it measure_evolution_cost predicts wall clock from a model of what sweep_task_cells does. This runs the real thing - real threads, the real wave barrier, the real outage breaker - with only the paid agent session replaced by a sleep, and times it. Durations are the measured per-arm samples divided by 5000, so a 1416s cell takes ~0.28s. The shape is kept on purpose: the median cell is 826s against a 5400s ceiling, and that spread is the entire reason a barrier costs anything. Uniform random sleeps would erase the effect under test. All schedulers consume one identical seeded plan, so a comparison cannot be an artifact of one of them drawing luckier cells. The model survives contact: it tracks real execution within about 10%, and workers=1 - which runs without a pool at all - sits at 0.95, so the residual above 1.0 at higher worker counts is per-wave thread overhead rather than a modelling error. Two structural claims that were arithmetic are now observed. Weekly is flat from workers=3: 3.59, 3.59, 3.59, 3.60, 3.59, 3.59 across w=3..8. workers=4 buys nothing over workers=3 on cold, 7.68 against 7.78. Two prototype schedulers are measured beside it, deliberately before any production code exists. A continuously fed pool per task is worth more than the model claimed on cold, -27.3% against a predicted -17.9%, and exactly nothing on weekly, +0.0%, because a weekly task is one wave with nothing to feed. One pool across all tasks beats both: -40.7% weekly and -42.9% cold at workers=3, rising to -65.7% and -63.9% at workers=8. It also subsumes the fed pool, since packing across tasks is a fed pool. That reorders the backlog. Cross-task packing moves from second to first: it dominates on both profiles, and it is the only thing that moves weekly at all. Raising the worker count is worth nothing until it lands - under the barrier weekly does not improve from w=3 to w=8, and speedup against serial is 1.58x for three workers and only 2.40x for eight. The bound on all of it: sleeping threads do not contend. Real sandboxed sessions compete for CPU, page cache and disk, and the duration sample was itself measured at workers=1, so it carries no contention either. These speedups are upper bounds. The ordering is trustworthy because the schedulers were compared under identical conditions; the magnitudes are not. The packed prototype is also a bare ThreadPoolExecutor with no breaker folding, no per-task graph lifecycle and no reuse binding - which is the actual cost of building it, and is not measured here. * test(eval): carry the sweep invariants into the packed prototype The first packed prototype was a bare ThreadPoolExecutor. It reported -43% and none of the invariants the shipped scheduler holds, so it priced an idea nobody could ship. This one carries them: a global submission order continued across task boundaries, in-order folding, the real outage breaker, and per-task graph readiness gating behind a serial builder. The fidelity check first reported the two schedulers tripping on different cells, 17 against 16. That was my instrumentation, not a divergence - sweep_task_cells folds an entire wave before it evaluates the breaker, so the last cell folded is not the cell that tripped. With the harness mirroring the breaker's own evaluation the two agree exactly, across failures starting at cell 0, 4 and 12, with overrun inside the workers-1 bound the wave docstring promises. Two results worth the exercise. Head-of-line blocking, not the barrier, is what a naive in-order design pays. Holding submission to `workers` cells beyond the fold pointer leaves the faithful scheduler at -8.1% cold and -2.7% weekly: one slow cell stalls the pointer, the window cannot slide, and it reproduces the wave almost exactly. That is the number to quote if anyone proposes the obvious implementation. But the overrun bound turns out to be set by the worker count, not the window. Only `workers` cells can be running when the breaker trips; everything queued behind them short-circuits on the halt flag. Overrun is 3 at an unbounded window exactly as at 6, and the trip cell never moves off 16. So H2 does not have to trade breaker fidelity for speed - a wide window takes -42% with the semantics intact. The tension I assumed was there is not, and window=12 already captures 97% of it. Still an upper bound: sleeping threads do not contend, and the sample was measured at workers=1. What this establishes is that the invariants are affordable, which was the thing blocking H2. Not built here: the trees tempdir lifecycle, reuse-row binding, and the cancel_event path. 591 eval tests pass, ruff clean. * test(eval): put the scheduler comparison under real CPU contention Every Phase 2 number so far came from sleeping threads, which contend for nothing, against a duration sample measured at workers=1, which contains no contention either. That was the standing caveat on the whole result, so this measures it. A cell now waits for its API share and then burns a fixed number of sha256 rounds in a subprocess. Work-bounded rather than wall-clock bounded, so it takes longer when cores are busy - that is the effect under test. A subprocess because Python threads burning Python would measure the GIL rather than the machine. Calibrated at 519k rounds/s, stable within 2% across three probes. The first run of this was worthless and is recorded as such: on a 24-core host with 3 to 6 workers nothing ever contends, since cpu_fraction 0.5 at 6 workers is about 3 cores of demand out of 24. It measured an absence. Re-run pinned with taskset to 4 and 2 cores. The packing advantage survives. It holds between -40% and -47% across every host size and CPU fraction tested, including a genuinely oversubscribed 2-core box at cpu_fraction 0.5 with 6 workers. But contention erodes packing more than it erodes waves, for a structural reason: packing is what creates the concurrency. Moving from 24 cores to 2 at cpu 0.5 and 6 workers, the faithful scheduler slows 13% while the wave slows 3.7%, and the gain narrows from 45.0% to 39.8%. Packing and a higher worker count are therefore not independent wins - packing spends the contention headroom first, so raising workers has to be re-argued after it lands rather than added to it. Three things this still does not measure, and they bound the result. The real CPU fraction of a benchmark cell is a guess informed by roughly 180 tool calls per session; nobody has profiled one. The evolution runner's core count decides which column applies and is unknown here. And the burn is sha256, pure CPU, while real cells run vitest and analyze, which are memory and IO heavy - so this is a floor on contention, not a ceiling. 591 eval tests pass, ruff clean. * perf(eval): add a packed sweep scheduler, and correct the bound I claimed for it sweep_task_cells finishes one task before starting the next and drains a wave before refilling it, so a task with fewer cells than workers leaves workers idle and one slow cell stalls its whole wave. sweep_packed_cells feeds every task's cells through a single pool instead. Measured against the review corpus it is worth about 40% of a cold sweep, and it is the only change that moves a seeded weekly run at all - there a task is three cells and a wave is never full. The breaker keeps its exact meaning. Cells carry a total submission order continued across task boundaries, a folder walks results in that order, and consecutive systemic failures are counted there, so a doomed run aborts on the same cell it would have under waves. Verified at three failure positions. This commit also corrects a finding from the Phase 2 prototype. I claimed the overrun bound was set by the worker count rather than the submission window, and that packing therefore cost nothing in breaker fidelity. That was derived from a window sweep that only ever injected failures at one position. Driving the real function at other positions shows the halt flag does not bound overrun at all: the folder walks in order, so a slow early cell lets workers race ahead and the trip is detected after those cells have already paid. An unbounded queue overran by 11 cells where waves overrun by 2. So the window is load-bearing and the trade is real, measured at workers=3 with failures injected at four positions: window 3 -> -8% wall, overrun 2 (the wave scheduler's own bound) window 6 -> -27% wall, overrun 4 window 12 -> -42% wall, overrun 9 window 54 -> -44% wall, overrun 11 Overrun is wasted paid sessions at roughly $70 each. The default multiplier is 2, keeping the worst case within twice the wave bound while taking most of the gain; the curve is in the constant's comment so raising it is an informed decision rather than a guess. Not wired in yet: _run_sweep still calls sweep_task_cells per task. Moving the per-task graph, trees tempdir and reuse binding out of that loop behind await_ready is the larger and riskier half, and it belongs in its own change. 595 eval tests pass, ruff clean. * fix(eval): judge harness health on execution, not on how many tasks resolved broken_incumbent_arms infers "the environment is broken" from an arm resolving zero tasks. That inference does not hold: a reviewer can be wrong about every task in a hard corpus while every process, mount and capture worked perfectly. Actions run 33962002890 is exactly that shape - 51 cells, all resolved=False with error_kind=oracle-failed, median score 0.212, and a healthy harness. Someone already knew this, and patched it by excluding review arms at the call site. That leaves the unsound inference in place for workflow and workflow_direct, and leaves review arms with no health check at all - so the run that genuinely was broken, 33912693948, where the mount made an atomic write impossible and all 41 artifacts came back empty, could not have been caught here either. So this replaces the inference rather than adding another exemption. aggregate now classifies fresh rows into execution failures (the process or its tooling did not complete), evidence failures (it completed but produced nothing trustworthy or scoreable), and admissible measurements. An arm is unhealthy only when it has fresh attempts, zero admissible measurements, and at least one execution or evidence failure. Resolution count is no longer consulted. Arms with only reused rows report current health as UNKNOWN rather than good. With the inference corrected, review arms are checked again, which is what lets the empty-artifact case be caught at all. Deliberately unchanged: comparator reuse eligibility, quality denominators, promotion thresholds, model settings, skill prompts and scheduler behaviour. Failures that stop being called infrastructure failures still surface in the counts and reasons - an agent-originated failure must not vanish from reporting because it was reclassified. broken_incumbent_arms and its tests are left in place; deleting behaviour belongs in its own change. Seven regression tests, built from both runs' shapes and labelled as reconstructed from logged observations, since 33962002890's results.jsonl did not survive the instance shutdown. They pin: a badly-scoring reviewer is healthy; an all-zero score is still a valid negative; empty artifacts are unhealthy; one admissible cell keeps an arm healthy while its failures stay visible; reused rows alone leave health unknown; reused successes do not mask fresh failures; and a parseable artifact does not excuse a failed session. 602 eval tests pass, ruff clean. * fix(eval): pin the health guard below the breaker, and stop calling mixed runs healthy Two corrections to the health-classification patch. The regression I wrote could not have proved what it claimed. A fixture of 41 empty artifacts aborts through the outage breaker long before finalization: review-evidence-invalid is in SYSTEMIC_ERROR_KINDS and the limit is 5, so it trips at cell 5 through the pre-existing path. It demonstrated failure detection, not the new guard. The decisive test now uses ONE fresh unusable cell, asserts the streak stays under the breaker threshold, and only then requires finalization to abort - leaving the new check as the only thing that can catch it. Removing the call makes that test fail; restoring it passes. The accurate defect statement is narrower than the last message claimed. Review arms were excluded from the final incumbent-health check while the consecutive- failure breaker gave them separate, partial coverage. They were not unguarded. Second: "one admissible cell plus two execution failures" was asserted as healthy. That converts "not wholly unusable" into "ran reliably", which is how a partly-broken sweep passes review. Arms now report UNKNOWN, OBSERVED_OK, DEGRADED or UNUSABLE. Only UNUSABLE is fatal, so eligibility and promotion are untouched - this changes what is reported, not what is allowed. The guard is extracted as enforce_measurement_health so it can be driven directly, and it now reports a status line per arm. It names no cause: an empty artifact establishes that evidence is unusable, not that a mount rejected the write, so it prints cause=undetermined rather than guessing EROFS. It still runs after report.md and promotion.json are written, so a failing sweep leaves its evidence behind. ce_review is named explicitly at the call site. It is a comparator rather than a candidate, so it is absent from CANDIDATE_ARMS.values(), and dropping the review exclusion alone would have left it unclassified. The wiring test reads _run_sweep's compiled code object for the referenced global rather than matching source text. It is honest about its limit: it proves the call exists and would catch its removal, but no test here drives _run_sweep end to end, which needs bwrap and a sandbox. broken_incumbent_arms is marked LEGACY and NON-AUTHORITATIVE with removal tracked. It has no production caller. 608 eval tests pass, ruff clean. The two test_model_gateway.py failures are test_locked_litellm_translates_messages_to_offline_responses and test_openai_gateway_never_leaves_proxy_output_on_an_undrained_pipe; both fail identically on origin/main in this environment, checked directly rather than carried forward as an inherited label. * fix(eval): review artifact path, evidence classification, comparator reuse Extracted from the combined skill-evolution branch. This is the runtime change set: everything that alters how a sweep executes and what it records. The packed scheduler and its measurement harness were separated onto perf/skill-evolution-packed-scheduler, which is purely additive. Correctness. The review artifact was mounted as a writable FILE inside a read-only workspace while the agent's Write tool writes atomically - temp file beside the target, then rename - so the temp create failed EROFS and the artifact was never written. Four layers gate that path and three named the old location: the CLI's own allowWrite/allowRead policy, the task corpus verify command run in its own sandbox invocation, and host_text/host_path for the host-unsafe backend. Fixing the translator exposed a second defect, since "/review-output" appears twice in "/review-output/review-output.json"; matching is now anchored to a path boundary. parse_review_output folded OSError, UnicodeError and JSONDecodeError into one message, so an artifact that was never written looked like an encoding fault; each cause now names itself. Health classification. broken_incumbent_arms inferred a broken environment from an arm resolving zero tasks, which a reviewer facing a hard corpus falsifies - Actions run 33962002890 is exactly that shape. Arms are now classified from fresh execution and evidence outcomes as UNKNOWN, OBSERVED_OK, DEGRADED or UNUSABLE, and only UNUSABLE aborts. Resolution count is not consulted. The guard names no cause: an empty artifact establishes unusable evidence, not that a mount rejected the write. Comparator reuse. Reuse accepted evidence it should have rejected: a row without a runtime_digest passed the drift lock, recorded_at was overwritten with the copy time so a row could outlive its own max_age, and the binding ignored task-asset and dependency digests although this change alters sandbox_dependencies in the review corpus. Closing the last one required preparing asset snapshots before the reuse decision, which also removes the concurrent TaskAssetCache.prepare the prefetch thread could race. These three concerns share aggregate() and _run_sweep, which is why they ship together: separating them further would mean hunk-level surgery on a function all three modify, and the risk of a silent omission outweighs the reviewability gain. 592 eval tests pass at this base. The two test_model_gateway.py failures, test_locked_litellm_translates_messages_to_offline_responses and test_openai_gateway_never_leaves_proxy_output_on_an_undrained_pipe, fail identically on origin/main in this environment. Known gap, and the reason this is not ready to merge: no test drives _run_sweep end to end. enforce_measurement_health is unit-tested including the below-breaker unusable case, and the caller wiring is pinned structurally by reading _run_sweep's compiled code object, but interruption semantics, exit precedence and persisted artifacts are not exercised through the real path. * fix(eval): address PR review feedback (#3207) - aggregate: count admissible rows directly instead of subtracting the execution and evidence counters, which double-charged a row that is both a session error and invalid review evidence and could report UNUSABLE for an arm holding real measurements. - run_proposer: bound the session timeout by what is left of --max-runtime-seconds, so clearing the sweep minimum cannot start a full-length session past the instance window. - comparator reuse: hold one O_NOFOLLOW descriptor for the size check, digest and copy, and prove it is the inode that was checked, closing the swap window a concurrent writer of the reuse directory had. - Drive the review-artifact mount assertion through run_arm and the clone-template assertion through run_cell, instead of rebuilding the expected values in the tests (also removes the CodeQL unnecessary lambda). - Assert the workflow invokes run-evolution.sh rather than that its YAML mentions --max-runtime-seconds, which only appears in a comment. - Correct the parse_review_output failure-mode claim: the fold was empty artifacts reported as "not valid UTF-8 JSON"; a never-created file raised FileNotFoundError. - prettier: wrap the over-long readFileSync call flagged by PR autofix. Note: pre-existing failure in tests/test_model_gateway.py::test_locked_litellm_translates_messages_to_offline_responses (local LiteLLM proxy never becomes ready in this environment) not addressed by this PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(eval): address the second round of PR review feedback (#3207) - Refuse a symlinked `transcripts` component on both sides of comparator reuse. O_NOFOLLOW guards the leaf only, so a link there redirected the read or the copy out of the results directory; checked per component as evolution._require_directory_chain does. - Base the paid incumbent canary on the cells this sweep PLANS. Reuse selection accepts any prior run index, so a results directory produced with more runs left extra keys, the equality never held, and the canary stopped firing. Extracted as drop_canary_reuse_key and unit-tested. - Start the runtime clock in main(). --max-runtime-seconds is measured from /proc/uptime before exec, so parsing, task I/O, preflight and gateway setup were being handed back to the sweep out of the upload reserve. - Do not fall back to shutil.copytree when the managed clone copy was cancelled or timed out; that fallback is for a filesystem that cannot reflink, and copytree cannot be cancelled. - Assert the review session's writable mount, not only the verifier's read-only one: the EROFS bug is about the agent's write. - Exercise ref isolation in the copy_isolated_tree test rather than comparing an initial HEAD a shared namespace would also match. - Point the stale-symlink fixture at the sentinel via os.path.relpath, and skip the reuse symlink tests where symlink creation needs privilege. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(eval): close the runtime-cap gap and pin the reuse directory Both were left open on #3207 as approach decisions rather than nits. Runtime cap: run-evolution.sh computed the budget in its own `uv run python -c` and passed a number, so the script's remaining provenance work and the CLI's own startup were spent by nobody and charged to the sweep — out of the upload reserve the cap exists to protect. The script now passes --max-runtime-from-instance-window and evolve reads /proc/uptime itself, on the line after it starts the clock the budget is measured against, so no interval exists to lose. Also removes an interpreter start from the script and lets --dry-run print the real argv. Reuse directory: _real_child_directory lstat-checked `transcripts` and returned its pathname, so a concurrent writer could rename the directory and leave a symlink before the name was used again — O_NOFOLLOW guards only the leaf. Every artifact is now resolved against a held descriptor: _open_real_directory opens with O_DIRECTORY|O_NOFOLLOW (check and open in one syscall), and _open_regular / _copy_owner_only take dir_fd. The reuse path is therefore POSIX-only; _require_openat says so and fails closed, which the runner already treats as "run a paid cell". _resolved_directory still tolerates a symlinked reuse root, unchanged and still tested. evolution._require_directory_chain is still lstat-per-component. It guards a different surface (candidate overlay reads) that neither review raised, so it is left alone rather than widened into here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(eval): sample the proposer budget where it is spent, digest what is copied Four findings against f0cdc9e7, all of them mine. - run_proposer took a precomputed remaining_seconds, but it clones, sanitizes and builds a sandbox before the session starts, so the caller's reading was already stale. It takes started_monotonic now and samples the budget on the last line before run_claude. The caller's earlier reading still decides whether to start at all — it just no longer decides how long to allow. - _copy_transcript_artifact hashed the source and then read it again to copy it. A held descriptor stops the pathname being substituted, not the inode being rewritten, so the row could record the expected digest while the destination held other bytes. _copy_owner_only now digests the same buffers it writes and returns (digest, bytes); a mismatch unlinks the destination and raises. One read instead of two. - Explain the empty except in _open_real_directory: an existing directory is the ordinary case, and the O_DIRECTORY|O_NOFOLLOW open below is what proves what it is (CodeQL). - Drop the unused uptime fixture from the runtime-cap test, and correct the cross-file contract described in the workflow-contract test and the workflow YAML: neither names --max-runtime-from-instance-window, the script does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(eval): close the reuse producer/consumer contract through the real run_cell Every comparator-reuse test built its rows by hand. That proves the predicate's logic and nothing about the producer: a fixture can satisfy eligibility while a row the runner actually emits never does, and a key-name inventory cannot tell the difference. I checked that inventory first - all 25 keys the predicate reads have a producer - which is exactly why it was not sufficient evidence. This carries one record through the production path instead: real run_cell -> production JSONL writer -> load_result_rows -> row_is_reusable_comparator Only the expensive dependencies are replaced: the model session, sandbox launch, repository acquisition, graph preparation, git plumbing. The digest fields the reuse binding compares are assembled by run_cell itself from its TaskCellContext, so they stay real - they are the subject, not scaffolding. Expectations are built from the sweep's own configuration rather than copied out of the emitted row, since copying them back would make producer and consumer agree because the test arranged it. Two cases: a production-emitted row with matching bindings is eligible, and the same evidence with one dependency binding changed is rejected. A test-controlled digest set keeps both deterministic. Mutation-checked. Replacing run_cell's task_asset_manifest_digest assignment with None makes the positive case fail; restoring it passes. That is evidence the inventory audit could not produce. Building it also documented what a real review row must carry, which no fixture had recorded: a scored review with review_weighted_f1 present, and at least one transcript artifact whose source is PARENT_EVENT_STREAM_SOURCE. An empty artifact list is not reusable. Those are production contracts my first double got wrong, and the reader rejected it each time. No production code changed. 607 eval tests pass, ruff clean. * test(eval): drive the real sweep to its finalization decision enforce_measurement_health was unit-tested and its call site pinned by reading _run_sweep's compiled code object. Neither showed the guard running inside a sweep. These drive the real _run_sweep with cell execution scripted and everything downstream left alone: folding, aggregation, the artifact writers, the health guard and the exit selection. The below-breaker case is the decisive one. A fixture of many unusable cells aborts through the pre-existing outage breaker instead - review-evidence-invalid is systemic with a limit of five - and would pass whether or not the finalization guard exists. One fresh unusable cell stays under that threshold, so only the guard can catch it. The test asserts the arm and status the guard reports, and that no systemic-outage line was printed, rather than accepting any SystemExit: an unrelated setup failure must not satisfy it. Mutation-checked at the runtime path. Removing the guard invocation fails the below-breaker test because the expected finalization behaviour disappears, not because a name went missing from a code object. A zero score is covered separately. review_weighted_f1 = 0.0 must stay a present valid negative measurement; a truthiness check would read it as absent and turn a quality result into an execution-health failure. The third test asserts the evidence survives - results.jsonl carries the scored row and report.md exists - so the persisted artifacts tell the same story as the exit. Only expensive setup is replaced: cell execution, graph preparation, asset snapshots, and task-binding resolution, which clones the repository and verifies the ref. Building the fixture also documented that the report renders the whole review metric set, so an incomplete row fails in formatting rather than logic. Still open on this track: interruption semantics and exit-reason precedence (outage 1 before cancellation 130) are not yet exercised, and fold order and the real sandbox mount contract remain on separate tracks. No production code changed. 610 eval tests pass, ruff clean. The two test_model_gateway.py failures reproduce on origin/main in this environment. * fix(eval): an interrupted sweep is interrupted, not aborted Driving the real _run_sweep to its exit revealed that cancellation without an outage exits 1 and writes "Sweep aborted: partial evidence", where the contract is 130 and "Sweep cancelled". sweep_task_cells returns (streak, tripped), and its caller assigns that flag to outage_tripped and turns it into exit 1. Both cancellation paths returned True for it. The breaker's own return is also True, so the two became indistinguishable one frame up and cancellation inherited the outage's exit and wording. The fix is to return False from the cancellation paths: the flag means the breaker tripped, and the caller already tests cancel_event itself for the stop decision, so nothing stops running any later than before. Reproduced before the fix, through the real caller, not from reading. The failing assertion was the report line; the exit code was 1. Two runtime tests cover it. Each begins with one admissible cell so the arm classifies DEGRADED rather than UNUSABLE - otherwise enforce_measurement_health supplies exit 1 first and a precedence test passes without ever reaching exit selection. Cancellation is set from a completed cell rather than a sleep or a real signal, so the interruption point is deterministic. The precedence test is mutation-checked: moving the cancel_event check ahead of the outage check fails it with "assert 130 == 1" - it observes the wrong exit code, not merely some failure. My first attempt at that mutation silently matched nothing and the suite passed; a no-op mutation proves nothing, so the edit now asserts its own anchor. test_process_control read the same flag as "stopped" and asserted it after a cancellation. It now reports the event and the flag separately, which is what it was really asserting: cancelled, and not an outage. The scored-row shape moved into tests/bench_fixtures.py with zero values written out, so the finalization and interruption tests share one definition of what a real review row carries. Impact analysis returned risk UNKNOWN - the index predates this branch and does not carry the eval harness - so the callers were confirmed by text search as the rules require for UNKNOWN: one production caller and three test sites, all updated or verified. detect_changes reports zero for the same reason; that zero is unseen, not unaffected. 612 eval tests pass, ruff clean. The two test_model_gateway.py failures reproduce on origin/main in this environment. * test(eval): prove the review artifact mount under a real sandbox The orchestration tests replace the sandbox, so they say nothing about isolation. This covers the filesystem contract the EROFS defect actually broke, through the production configuration: the same command_prefix_for call run_arm makes for a review cell, with read_only_workspace=True and the review-output directory as the writable mount - not a hand-built mount tuple that merely looks right. A deterministic writer stands in for the agent. It writes a temp file beside the destination and renames it into place, which is the operation that failed: an atomic write needs a WRITABLE PARENT DIRECTORY, and binding the file itself left nowhere to put the temp file. It then attempts a workspace write and must be refused, and the production parse_review_output reads the bytes the sandbox left behind. No model session and no credentials. Placed in test_proposer_sandbox.py, which the "eval / containment (ubuntu)" job already runs with GITNEXUS_REQUIRE_BWRAP_CANARY=1. That gate is the point: where the variable is set, a missing namespace capability FAILS the job instead of skipping into a green tick. Verified here - forcing the variable on this machine fails with "bwrap: No permissions to create new namespace" rather than skipping. What this does not establish: the assertions have never executed. This machine cannot create user namespaces, so the test stops at the preflight. Imports, signatures and the payload were checked statically instead - parse_review_output returns a tuple rather than an object, and it rejects a body without "verdict", so both of my first attempts were wrong and are fixed. The first CI run on a namespace-capable runner is what will actually confirm it. Scope is the filesystem and process contract only. A stand-in writer does not establish that a particular agent CLI's own file-access policy permits the same operation; that is a second, independent gate. 40 passed, 10 skipped locally; ruff clean. * fix(eval): read the review artifact before the sandbox deletes it The containment (ubuntu) job has been red since before the mount canary was added, and both failures have the same cause. This branch moved the review artifact out of the clone and into the session's private root, which is the point of the change: the agent writes atomically, so the artifact needs a writable parent DIRECTORY outside the read-only workspace. prepare_sandbox removes that private root in a finally on scope exit. Two tests read the artifact AFTER the with block, so they were asserting against a directory the sandbox had already deleted - FileNotFoundError, reported as "review output was never written". Production is not affected, and this is the reason: run_arm holds the live session and reads the artifact inside that scope, both to score it and to mount it read-only into the verifier's separate sandbox invocation. The tests were the only readers outside it. Both now read inside the scope. The pre-existing canary (test_read_only_review_workspace_exposes_only_one_writable_artifact) predates this PR and passes on main, where the artifact still lived in the clone and survived teardown; the relocation is what broke it, so the fix belongs here. The new canary found this independently and agreed on the cause, which is what it was written for - though only after CI ran it, since this machine cannot create user namespaces. 40 passed, 10 skipped locally; ruff clean. The bwrap-gated tests still skip here and remain unverified until the ubuntu job runs them. * test(eval): pin what an interrupted sweep persists and refuses to promote The completed-run persistence test could not show either of these: it never interrupts, so it would pass even if the writers only ran on the clean path. Evidence already paid for survives cancellation. The cell that completed before the interruption is still in results.jsonl with its measurement intact, and report.md is still written. Losing those rows would mean paying for evidence the sweep then discards. An interrupted run emits nothing that authorizes promotion. Asserted as the semantic condition rather than the absence of a file: promotion.json IS still written for an aborted run - it is the record of why nothing was promoted - so the test requires run_status "aborted" and every decision reduced to insufficient_evidence with a partial-evidence reason, rather than requiring the artifact to disappear. Mutation-checked. Forcing complete=True at the promotion_evidence call site fails the test with "assert 'complete' == 'aborted'" - it observes an aborted run claiming completeness, not merely some failure. These were the last two unasserted items on this PR's finalization checklist. 614 eval tests pass, ruff clean. The two test_model_gateway.py failures are environmental (litellm[proxy] console script absent) and predate this branch. * Address PR review feedback (#3207) An exhausted runtime cap no longer buys a second. remaining_runtime_seconds floors at 0 and the proposer call site wrapped it in max(1, ...), so a cap fully spent by the clone, the sanitize pass and the sandbox build started a paid session with a one-second allowance instead of stopping before the upload reserve the cap exists to protect. It now returns a not-ok record, which the caller already treats as end-of-run. Pinned by a test driving the real run_proposer with setup that consumes the whole budget; reverting to max(1, ...) fails it. Reuse copies are bounded before their size is validated, not after. The source is a prior sweep directory this module already treats as concurrently writable, so a transcript appended to after its metadata was recorded was streamed to EOF and only then compared against its declared size - filling the destination, or never reaching EOF, long before the drift check could reject it. _copy_owner_only now takes max_bytes and stops one byte past the ceiling, which keeps the drift comparison meaningful. Review and patch artifacts carry no recorded size, but "no recorded size" is not "no limit": they get MAX_TRANSCRIPT_BYTES, the ceiling the capture path already enforces. A reused review row must carry its artifact. The predicate accepted a row on its score and transcript metadata while materialize_reused_row copied the review artifact only when the row named one, so a scored review could be carried forward with nothing for a proposer to read. The shared row fixture was the thing out of step here, not the requirement - production sets review_artifact whenever the review source exists - so it now carries one too. Test fixes, all against code this PR added: unusable_review_row could not be overridden at all. It passed explicit keywords beside **overrides, and Python rejects the duplicate in the call expression before scored_review_row can apply its update, so unusable_review_row(error_kind=...) raised TypeError. Merged into one mapping. The finalization helper monkeypatched runner.prepare_ce_plugin_snapshot with raising=False. No such symbol exists - the real one is staged_ce_plugin_snapshot - so it silently added an attribute nothing reads. Removed. The promotion test named candidate_review in candidate_arms but _run_sweep builds cells only from args.arms, so no candidate cell ran and insufficient_evidence could hold because nothing executed rather than because partial evidence is barred. The candidate arm now runs, cancellation fires after its cell, and the test asserts the candidate actually produced a row so it cannot silently return to being vacuous. The reuse round trip serialized with json.dumps and write_text while claiming to cover the production writer, which applies redact_text over the row's own bytes. It now writes the way the sweep does. Its fixture also writes the review artifact, because run_cell records review_artifact only when the review source exists and the emitted row was otherwise one production never emits. Not addressing the duplicate-match nit on proposer_sandbox.py: the claim is that "/review-output" occurs once in "/review-output/review-output.json". It occurs twice, at offsets 0 and 14 - the separator before the basename forms it again - so the boundary the comment describes is real. Verified with re.finditer. The workspace-snapshot finding is parked for a human: excluding bootstrap noise is a documented deliberate choice, and tightening it is a genuine tradeoff. 634 eval tests pass, ruff clean. The two test_model_gateway.py failures are environmental (litellm[proxy] console script absent) and predate this branch. * Address PR review feedback (#3207), round 2 Carry review_f1 in the scored-row fixture. score_review emits "f1" (review_scoring.py:317), which the runner folds in as review_f1, so a real scored row has it and the fixture did not - the same fidelity gap as the metrics already added there. aggregate now reports 0.5 for it instead of None. The reported failure mode was not real, and the distinction matters for anyone reading the thread later. aggregate filters on record.get(metric) is not None BEFORE indexing record[metric], so a missing key is skipped rather than raising KeyError; the finalization tests were passing throughout. Verified by running aggregate against the old fixture. Fixed because the fixture should match what production emits, not because anything was crashing. Drop the unused row parameter from the round-trip _expectation helper. It never read the argument - deliberately, since the docstring says the bindings must be derived from sweep configuration rather than copied out of the emitted row - but passing the row anyway was dead plumbing that suggested the opposite. The reuse-root TOCTOU finding is parked for a human rather than fixed: the lstat-then-resolve window is real, but _resolved_directory documents the weaker promise as deliberate and says not to merge it with proposer_sandbox's helper "without first deciding which promise the reuse path should make". That decision is the fix, and it is the same class as the reuse TOCTOU already parked on this PR. 634 eval tests pass, ruff clean. test_process_control's TERM-ignoring-descendant test flaked once under full-suite load and passes 3/3 in isolation; it is a timing test and neither file changed here touches it. The two test_model_gateway.py failures remain environmental. * fix(eval): pin the reuse roots to the directory that was checked Settles the reuse TOCTOU that has been parked twice on this PR. The docstring asked to decide which promise the reuse path makes before merging its helper with proposer_sandbox's, and the answer is that two separate things were being conflated. The symlink POLICY stays exactly as it was: parent hops remain allowed, so a symlinked artifacts directory or macOS's /var still works, and a symlinked leaf is still refused. Rejecting hops would break ordinary setups for no gain, which is what the docstring argued and it is still right. What is closed is the other thing - the gap between checking a name and using it. _resolved_directory lstats a name and the later open re-walks that same name, so a prior sweep that renames its results root and leaves something else behind is opened somewhere else entirely, and O_NOFOLLOW cannot see a link that resolve() already followed. _resolved_directory now returns the checked directory's identity alongside its path, and _open_pinned_root fstats the descriptor it opened and refuses a mismatch. The two are separable, so there was no trade to make: comparing identity rejects nothing that holds still, since a stable directory always matches itself. Both cases are pinned - a replacement by a different REAL directory is refused (the leaf-symlink rule does not cover that one), and an ordinary unchanged root opens normally. Why it matters here rather than as a general hardening: the failure is silent. Rows would be copied out of some other directory and folded into a comparator baseline as though they were this sweep's own evidence, which is the one thing the reuse path exists to get right - a corrupted baseline decides promotions. 636 eval tests pass, ruff clean. The two test_model_gateway.py failures remain environmental. * refactor(eval): one prompt digest, and drop two pieces of dead scaffolding Compute task_prompt_digest once. row_is_reusable_comparator compares the value a prior row stored against the value this sweep derives, as exact strings, and the two inline copies of that hash had already drifted apart - the expectation side had picked up a str() cast the row side lacked. They agree today only because tasks validate "prompt" as a string (runner_tasks.required_strings), so nothing was broken; a later divergence on one side would have silently stopped rows matching, with no test to catch it. Verified the extracted helper is byte-identical to what it replaced. Two comments named broken_incumbent_arms as the thing that would read stale health. This branch replaced that call with the measurement-health path, so the reasoning still holds but the name no longer does; they now name arm_health. The function itself stays: origin/main still calls it at runner.py:2098, and retiring it is its own change rather than a cleanup pass. Removed instance_window_budget_from_proc and its one test. It composes two helpers main() deliberately calls separately - there is a comment there explaining why the read and the budget calculation stay decoupled - and nothing outside its own test ever called it. Introduced on this branch, absent from main, so it was never deployed, public, or consumed elsewhere. Dropped an unused tmp_path parameter from a comparator-reuse test that does no filesystem work. Skipped three findings. Hoisting _assert_self_contained_git_objects to a once-per-template check is a real saving - measured at 5.66s over 1551 objects - but it verifies the OUTPUT of each copy operation, and git clone from a local path hardlinks objects by default, which is exactly what its st_nlink check catches. Checking a predecessor instead of the clone each cell runs against thins an isolation guarantee for 0.7% of a median cell. Deleting broken_incumbent_arms outright would remove a symbol main still calls. And a fourth copy of the test-only _git helper extends a pattern that already exists in three other test modules; centralising it means editing conftest.py, outside this scope. 635 eval tests pass, ruff clean. The two test_model_gateway.py failures are the environmental ones. --------- Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-09-08 09:25:45 +00:00
Gergő Magyar	d1463977c8	feat(eval): Add bounded packed-scheduler primitives and offline replay benchmarks (#3206) ... * perf(eval): packed sweep scheduler and the harness that measured it Extracted from the combined skill-evolution branch so it can be reviewed on its own. Purely additive against main: no existing function changes behaviour, and sweep_packed_cells has no production caller yet. sweep_task_cells finishes one task before starting the next and drains a wave before refilling it, so a task with fewer cells than workers leaves workers idle and one slow cell stalls its whole wave. sweep_packed_cells feeds every task's cells through a single pool instead, keeping the breaker's meaning: a total submission order continued across task boundaries, a folder walking results in that order, and consecutive systemic failures counted there, so a doomed run aborts on the same cell it would have under waves. simulate_sweep.py is what produced the numbers. It drives the real schedulers with only the paid agent session stubbed, using the measured per-arm durations in session_durations.json divided by a scale factor. The distribution's shape is kept deliberately - median 826s against a 5400s ceiling - because that spread is the entire reason a barrier costs anything, and uniform sleeps would erase the effect under test. All schedulers consume one identical seeded plan. Measured at workers=3 against the review corpus, packing is worth about 40% of a cold sweep, and it is the only change that moves a seeded weekly run at all - there a task is three cells and a wave is never full. The submission window is a real trade, measured with failures injected at four positions: window 3 -> -8% wall, overrun 2 (the wave scheduler's own bound) window 6 -> -27% wall, overrun 4 window 12 -> -42% wall, overrun 9 window 54 -> -44% wall, overrun 11 Overrun is wasted paid sessions on an aborted sweep. The default multiplier is 2; the curve lives in the constant's comment so raising it is an informed decision. Contention was measured separately by burning real CPU in subprocesses under taskset: the advantage holds between -40% and -47% from 24 cores down to an oversubscribed 2, though packing erodes faster than waves do because packing is what creates the concurrency. measure_evolution_cost.py is the offline cost model, with no runtime caller. It reports workers from the workflow's current default, which on this base is 1. Limits worth stating: sleeping threads do not contend and the duration sample was itself recorded at workers=1, so the speedups are upper bounds; the ordering of the schedulers is trustworthy because they were compared under identical conditions, the magnitudes are not. 562 eval tests pass at this base. The two test_model_gateway.py failures, test_locked_litellm_translates_messages_to_offline_responses and test_openai_gateway_never_leaves_proxy_output_on_an_undrained_pipe, fail identically on origin/main in this environment. * fix(eval): compare the shipped window and bound the overrun by it Address PR review feedback (#3206). run_faithful defaulted its submission window to `workers` while runner.sweep_packed_cells defaults to `max(workers * PACKED_WINDOW_MULTIPLIER, workers)`, so every run that named no window compared a prototype queued twice as tightly as the shipped scheduler and presented it as the production invariant. The faithful default now reads the same constant. Measured at workers=3, faithful and production agreed on nothing before and agree exactly now: breaker overrun 2/1/2 vs 2/4/3 becomes 2/4/3 vs 2/4/3 across the three failure positions. The contention sweep hard-coded `window=12` for faithful only, which the production run never saw - masked at workers=6 where both are 12. Removed, and the production measurement it was already paying for is now reported as `production_s` instead of being discarded. breaker_fidelity checked the overrun against `args.workers`. The bound the producer actually enforces is `window - 1` cells past the fold pointer, which is the wave scheduler's own `workers - 1` when window == workers; against the shipped default of 6 the old predicate reported a failure for an in-bound run. The window is now passed explicitly, reported in each row, and checked against its own bound. --window was parsed and never read. Wired into the schedulers that hold one. Dropped two unused plan constructions CodeQL flagged, and the `skipped` set in sweep_packed_cells that nothing reads - the None appended to `submitted` is the skip representation the fold loop consumes. Verification: 562 passed, 15 skipped, 2 failed (the two test_model_gateway.py failures the PR description documents as reproducing on origin/main), ruff clean. * fix(eval): carry the cancellation scope into packed cells, reject the args that hang Address PR review feedback (#3206). sweep_packed_cells submits from a producer THREAD, and a new thread starts with an empty context, so `copy_context()` there copied the producer's context rather than the one cancellation_scope had just bound _CANCELLATION in. Every packed cell therefore ran with no cancellation event, and run_managed falls back to _CANCELLATION when none is passed - so a cancelled run's subprocesses would never have learned about it. sweep_task_cells gets this right for free by submitting from the thread that entered the scope. Reproduced directly: packed workers observed [False, False], wave workers [True, True]. The caller's context is now captured before the producer starts and copied per submission; the new test fails without the fix. Three CLI arguments were accepted and then wedged the run: --scale 0 ZeroDivisionError before any scheduler starts --graph-seconds -1 hangs: the builder thread dies on a negative sleep, every scheduler waits on a readiness event nobody sets --window 0 (faithful) hangs: submitted - fold_pointer >= 0 holds before the first submission, so the producer and the consumer wait on each other The first two are rejected at the parser, which is the only layer that runs before a thread exists. run_faithful now enforces the same window >= workers rule sweep_packed_cells already had, so the prototype rejects exactly what the shipped function rejects. All three were confirmed to crash or hang first. Verification: 563 passed, 15 skipped, 2 failed (the two test_model_gateway.py failures the PR description documents as reproducing on origin/main), ruff clean. * chore(autofix): apply prettier + eslint fixes via /autofix command * Address PR review feedback (#3206) Preserve settled sibling rows when a packed cell raises. run_cell deliberately lets unexpected harness exceptions propagate, and sweep_task_cells answers that by folding every non-failing sibling before it re-raises - the cells already ran and already spent their budget, so dropping their rows means paying for evidence the sweep then discards. sweep_packed_cells called future.result() bare, so the fold stopped at the failing index and every later cell that had already completed was silently lost. It now folds forward over the settled futures before re-raising. The failing index itself has no row, since execute() assigns only on success, so folding forward cannot duplicate it. Pinned by a regression test that fails without the fix: the later cell is made to finish first, so there is real settled evidence to lose at the moment cell 0 raises. Reject arguments that cannot produce a run, at the boundary rather than deep inside a thread. NaN defeats every comparison it appears in, so the existing "> 0" and ">= 0" checks admitted --scale nan and --graph-seconds nan; the NaN then reached time.sleep in a worker or the graph thread, raised there, and left every scheduler waiting forever on a readiness event nobody would set. Infinity was worse than a crash: it scaled all durations to zero and the run reported a sweep that took no time. Both flags now require a finite value. The count flags are indexed or handed straight to a thread pool, so a zero surfaced as an IndexError on plans[0], a median over an empty sequence, or ThreadPoolExecutor's own error - none naming the flag responsible. --workers, --repeat and --runs now require at least 1. Two flags were not in the review but carry the same invariant and the same one-line treatment, so they are fixed with the class rather than left to resurface: --runs (same empty-plan path as --repeat) and --window, where zero admits no cell at all because the producer waits for a fold pointer to move past a cell it was never allowed to submit. Verified each guard fires with its own message rather than a stack trace. 563 eval tests pass. Note: pre-existing failures in test_model_gateway.py not addressed by this PR - litellm[proxy]'s console script is absent in this environment, and neither test touches the files changed here. * Address PR review feedback (#3206), round 2 Stop charging the fed baseline for overlap the wave scheduler gets free. run_fed is documented as pricing the barrier alone, but it slept graph_seconds serially before every task, while run_wave starts one background builder that prepares task N+1 while task N's cells run. The fed-versus-wave delta therefore mixed the loss of that overlap into what was reported as the price of the barrier. run_fed now uses the same builder, started before the clock, so the barrier is the only remaining difference. This moved the numbers. On the weekly profile fed was 4.203s and is now 3.694s, exactly equal to wave - which is the answer that profile should give. On cold, fed was 5.995s and is now 5.487s, so the measured price of the barrier widens from 1.844s to 2.352s: the old arrangement understated it by about a quarter. No committed results file or PR-body figure quotes these, so there is nothing stale to regenerate. Enforce the window bound the schedulers actually hold. Last round's guard required only >= 1, but run_faithful and sweep_packed_cells both refuse a window below the worker count, so --scheduler faithful --workers 3 --window 1 passed validation and then died on an uncaught ValueError. The check now uses the worker count. It also uses the LARGEST worker count the invocation will really use. --contention-sweep runs its own counts irrespective of --workers, so validating against --workers alone let the three-worker measurements finish and then raised on the six-worker one, losing the run partway through. Those counts are now a named constant the validator can see. Verified: --scheduler faithful --workers 3 --window 1 is rejected naming 3, and --workers 3 --window 3 --contention-sweep is rejected naming 6. No regression test for the graph-overlap fix. Discriminating it from the old behaviour requires cell work to overlap graph work, which makes the assertion a timing comparison, and this project does not take non-deterministic tests. It is verified by the before/after measurement above instead. 564 eval tests pass. Note: pre-existing failures in test_model_gateway.py not addressed by this PR - litellm[proxy]'s console script is absent here. --------- Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>	2026-09-08 08:22:12 +01:00
Claude	de3131fed8	fix(eval): require finite gateway startup budgets	2026-09-05 11:03:23 +00:00
Claude	3598a69188	fix(eval): close CI and remaining review gaps	2026-09-05 10:29:58 +00:00
Gergo Magyar	1054e3e038	fix(eval): make evolution evidence valid and bounded	2026-09-05 10:08:35 +00:00
Gergo Magyar	7fabbb044a	fix(eval): stop hiding review patches from sandboxed git apply ... The oracle-mask overlay covered the same path review setup reads, so every historical cell died with can't-open-patch. Leave the staged copy visible for apply, then fail closed if it is still there when the model starts. Co-authored-by: Cursor <cursoragent@cursor.com>	2026-09-04 19:43:47 +00:00
Gergo Magyar	ac7ae6a8ce	Address PR review feedback (#2785) ... Tighten review-evolution scoring, sandbox lock, and gateway cleanup so historical cells score instead of aborting or leaking host state. Co-authored-by: Cursor <cursoragent@cursor.com>	2026-09-04 18:59:32 +00:00
Gergo Magyar	8491cf4203	fix(eval): make historical review evolution score instead of aborting ... Seed the current gitnexus-review skill into older PR checkouts, force-add historically gitignored skill paths, accept plugin-qualified Skill ids, and lock host-unsafe workspaces to review-output.json so a generation can finish and score. Sandbox cleanup restores owner write bits before delete because a session that copytrees the locked clone otherwise leaves 0555 trees that rmtree cannot remove. Co-authored-by: Cursor <cursoragent@cursor.com>	2026-09-04 18:37:39 +00:00
Gergo Magyar	9047bf00a5	fix(eval): align review metrics and corpus evidence ... Co-authored-by: Cursor <cursoragent@cursor.com>	2026-09-04 05:38:25 +00:00
Gergo Magyar	6925fb344d	feat(eval): evolve review skills against historical PRs ... Co-authored-by: Cursor <cursoragent@cursor.com>	2026-09-04 05:32:31 +00:00
Gergo Magyar	636d45364a	feat(eval): log why a benchmark cell failed ... The sweep printed error_kind=plan-evidence-invalid and nothing else, so the reason a cell failed stayed in results.jsonl — an artifact uploaded after the run, not something a watcher can read while it is still going. Print the cell's error_detail next to its result line, redacted through the same credential list as the artifact and bounded, since a session-error detail carries stdout/stderr tails. Co-authored-by: Cursor <cursoragent@cursor.com>	2026-09-03 17:33:59 +00:00
Gergo Magyar	3fa42e4d83	feat(eval): report live progress for long headless sessions ... A proposer or benchmark session could run for an hour with nothing in the log between "proposing…" and its final result, so a wedged run looked exactly like a working one. The last CI failure spent 66 minutes silently retrying a dead endpoint before saying so. A session's stdout is evidence and is only written out after redaction, so it can never be echoed. Add a stdout_observer hook to run_managed that sees the stream without copying it anywhere, and a SessionProgress reporter that prints only what can be derived safely: turn counts, tool names, API retries, and a heartbeat while the session is quiet. API retries are called out by name because that is the signature of the gateway wedging. Progress goes to stdout so the benchmark sweep's lines reach the log live through the existing echo_stdout passthrough, rather than as a bounded stderr tail after the fact. Co-authored-by: Cursor <cursoragent@cursor.com>	2026-09-03 12:45:29 +00:00
Gergo Magyar	37415cb1ca	feat(eval): route skill evolution through OpenAI ... Co-authored-by: Cursor <cursoragent@cursor.com>	2026-09-03 10:52:37 +00:00
Gergo Magyar	45f97045e0	refactor(eval): simplify sweep internals and needrestart check (#2785) ... Reuse the incumbent skill digest instead of walking the tree twice, and keep the needrestart grep a literal match that actionlint accepts. Co-authored-by: Cursor <cursoragent@cursor.com>	2026-09-03 08:19:28 +00:00
Gergo Magyar	2cc27dcaa8	fix(eval): inspect worker failures without broad catch (#2785) ... Preserve every completed sibling outcome, including worker BaseException cases, without directly catching BaseException. Co-authored-by: Cursor <cursoragent@cursor.com>	2026-09-03 07:54:58 +00:00
Gergo Magyar	ace95d2715	fix(eval): bind gate evidence to selected tasks (#2785) ... Prevent schema-four decisions from substituting fabricated task sets and preserve unmeasured cleanup failures in live progress. Co-authored-by: Cursor <cursoragent@cursor.com>	2026-09-03 07:51:21 +00:00
Gergo Magyar	999b7bbede	fix(eval): close skill evolution review gaps (#2785) ... Keep promotion decisions monotonic and evidence-bound while preserving paid sweep results, redacting live failures, and hardening prior-run seeding. Co-authored-by: Cursor <cursoragent@cursor.com>	2026-09-03 07:38:01 +00:00
Abhinav Pandey	c3eb5991c1	fix(eval): preserve complete evolution evidence	2026-09-03 05:18:46 +05:30
Gergo Magyar	a522a4fbc2	fix(eval): stop failing the benchmark when main's version bumps ... PINNED_GITNEXUS_VERSION did not select a runtime — the sandbox already mounts gitnexus/dist built from the checkout by the workflow's own `npm run build`, so the benchmark has always run main, not a release. The constant only asserted that the checkout's package.json said "1.6.9" and raised SandboxError otherwise. That makes it a tripwire aimed at the wrong thing. main is 1.6.9 today, so it passes; the next release bumps it and every skill-evolution run hard-fails until someone edits this line — discovered on a Saturday, on a lane that runs unattended once a week, after the box has already been started and the proposer session paid for. What the constant was guarding is still guarded, and better: the sandbox canary now checks the version reported from inside the sandbox against the checkout's own package.json, so it still proves the mounted runtime is the one this checkout built, without a literal that has to be maintained in lockstep with releases. The digest bindings in promotion.json (sandbox_dependency_content_digest, graph and oracle digests) remain what actually pins the substrate a candidate was measured against. The remaining check keeps package.json readable and versioned, so a malformed runtime still fails closed.	2026-08-02 15:37:30 +00:00
Gergo Magyar	b5bcdb6c48	fix(eval): redact the token from the one failure line that now reaches CI ... `ManagedProcessError.__str__` embeds up to 1000 raw bytes of stderr_tail (process_control.py:72-73), and run_cell printed it verbatim. That line was inert until this branch: nothing ever printed the sweep subprocess's stdout, on success or failure. `echo_stdout` streams it live into the job log, so the print became a sink — and the only one of its kind here that skipped `redact_text`, which results.jsonl and every transcript already apply to exactly this field, for exactly this reason. GitHub masks the registered secret, but masking only catches that literal value; it is not the guarantee the other sinks have. The test drives a ManagedProcessError carrying the token in stderr_tail and asserts it never reaches stdout — verified to fail without the fix. Also from the same pass: bind `result_indexes[0]` once in run_claude, and give the workflow contract test a `findStep` helper instead of five copies of the same `steps.find` predicate.	2026-08-01 20:11:00 +00:00
Gergo Magyar	8076b98fcc	refactor(ci): address the evidence path directly instead of threading it ... 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.	2026-08-01 19:34:57 +00:00
Gergo Magyar	63d29c384f	feat(eval): run a task's benchmark cells in waves instead of one at a time ... 18 cells at ~48 min each, strictly serial, is 97.4% of a generation's 14.7h. The cells are independent — the wall clock was a scheduling choice, not a measurement requirement. `--workers` (default 1) runs the cells of one task concurrently; tasks stay sequential, so the sanitized graph snapshot each task already builds before its cells stays a single-writer affair. Threads, not processes: cells are subprocess-bound and `run_managed` keeps every piece of ownership state local to its own call, so nothing is shared to race on. Waves, not one fan-out. The outage breaker counts CONSECUTIVE systemic failures, and "consecutive" means nothing in completion order — folding as futures landed would make the trip point flaky between identical runs. Each wave is folded in submission order once complete, and the next wave starts only if the breaker held, so the breaker overruns by at most `workers - 1` cells (the ones already in flight) rather than by a whole task. `--workers 1` calls the cell directly rather than using a pool of one. That is not an optimisation: an async KeyboardInterrupt is delivered only to the main thread, so a cell on a worker thread is outside the reach of the ownership cleanup that kills its sandboxed process tree. The default therefore stays exactly what it is today, Ctrl-C included, and above 1 the flag's help says what is given up. All bookkeeping stays on the main thread — the results.jsonl append, the per-arm accumulation, the progress prints, the streak fold. No lock is needed anywhere, the progress counter cannot race, prints do not interleave, and results.jsonl keeps its canonical order. Every future is read. An exception a cell did not expect stays parked inside its Future until something asks for it; unread, a harness bug would become a silently missing run instead of a crash.	2026-08-01 18:48:19 +00:00
Gergo Magyar	b30f530698	refactor(eval): make a benchmark cell a callable instead of loop-body scope ... The sweep's innermost body — clone, sandbox, sessions, verify, oracle, teardown — was ~260 lines of `main()`'s scope, reachable only by running the whole sweep. Nothing tested it, and it could not run anywhere except that loop. `run_cell(ctx, run_idx, arm)` now owns one (run, arm) cell and returns its row; `TaskCellContext` is a frozen dataclass holding the per-task inputs a cell reads, so a cell depends on named fields rather than on whatever `main()` happens to have in scope. The try/except/finally moves verbatim, exception whitelist unchanged: a harness bug still escapes rather than being recorded as an ordinary infra-error and averaged into the evidence. The task asset snapshot is now prepared once per task, next to the graph snapshot, instead of lazily inside whichever cell reached it first. `TaskAssetCache` is a plain dict (task_assets.py:222-226,340), so the lazy build was a read-then-write race waiting for a caller that is not strictly serial. One behavior delta: when that preparation fails, every cell of the task now reports the same wrapped RuntimeError, where before the first cell reported the original OSError/SandboxError/ValueError. The loop keeps all the bookkeeping — progress counter, prints, the results.jsonl append, per-arm accumulation, the outage streak. Behavior is otherwise unchanged; this is the seam, not the concurrency. Six new tests cover it, the first coverage this body has ever had: the row's task/arm/run/digest bindings, each of the five expected failure kinds still removing the clone, an unexpected KeyError escaping while cleanup still runs, cleanup failure overriding the primary outcome, and a failed per-task snapshot failing every cell closed.	2026-08-01 18:48:18 +00:00
Gergo Magyar	5f51c9ed80	refactor(eval): fold the review cleanups into the evolution fixes ... - `_na` moves next to `measured_cost` in runner_sessions, the function whose None it renders, so the proposer-progress line stops reimplementing it inline. - `evaluate_candidate` derives `ungated_tasks` from the `gated` flag already on each row instead of accumulating a parallel list, and reports the carve-out as one aggregate line rather than one per task: `reasons` is truncated to three entries when it is fed back to the proposer (summarize_gate), and a growing set of unsolvable tasks must not crowd out why the candidate actually won or lost. - run_claude cuts the event window at the result event, so "nothing after the result is evidence" is a property of what the readers below can see rather than an assumption that a `system` event never carries a tool_use block. - The teardown test builds its stream with the existing `event_stream` fixture instead of re-joining the prefix by hand.	2026-08-01 17:18:43 +00:00
Gergo Magyar	2c399a0039	fix(eval): stop letting a task neither arm can solve veto every promotion ... The gate demanded that the candidate resolve every valid run of every selected task, regardless of how the incumbent scored. inv-feature-list- repos-filter fails its hidden oracle on 100% of runs in both arms, so the quality floor could never be met while it stayed in the set — and its cost comparison (which ranks who spent more while failing the same oracle) also fed the per-task regression cap and the median. One task outside both arms' capability was silently vetoing every future promotion. A task both arms measured cleanly — at least min_runs valid runs, zero exclusions — and that neither ever resolved carries no signal about the candidate. It is now reported in the decision (`gated: false`, plus an `ungated_tasks` list and a named reason) and left out of the floor, the regression cap, and the median. It still runs, and its failures still feed the proposer as evidence: an unsolved task is the loop's target, not its veto. The floor is unchanged everywhere it has signal. A candidate that goes 2/3 where the incumbent goes 1/3 is still rejected as unreliable, and a generation where NO task resolved anywhere is now `insufficient_evidence` rather than an efficiency verdict over runs that all failed. promotion.json goes to schema_version 4 — task rows changed meaning, and a stale v3 binding must not be applied under the new rule.	2026-08-01 17:07:14 +00:00
Gergo Magyar	5a017f2722	feat(eval): report evolution progress while the generation is still running ... Run 29907431284 printed its whole 14h45m of output at one timestamp (00:02:59.32) as the process exited: stdout is a pipe, so CPython block-buffered it, and there was no way to tell a live run from a wedged one. Three changes make the lane observable in the Actions log: - PYTHONUNBUFFERED for the driver (workflow step) and for the benchmark subprocess (its env is an explicit minimal dict and inherits nothing), so lines reach the log when they are written. - run_managed grows `echo_stdout`, a passthrough that streams a child's stdout to stderr as it arrives while leaving the bounded tail intact. evolve.py enables it for the benchmark sweep — the multi-hour phase, whose per-run lines previously surfaced only as a tail, and only on failure. It stays off everywhere else: a Claude session's stdout is the evidence stream and is written out only after redaction. - The sweep now announces each cell as it starts (`3/18, 47m elapsed`) and reports `took=` and `error_kind=` when it finishes, so an excluded run — the thing that actually blocks promotion — is visible live instead of only in results.jsonl. evolve.py also reports the proposer's duration, turns, and cost once the proposal lands.	2026-08-01 17:01:03 +00:00
Gergo Magyar	bb09ce28e0	fix(eval): stop discarding completed benchmark sessions as unverifiable ... The evolution loop has not been able to promote anything since it went online. Run 29907431284 (the last green run) reached the gate and threw away 5 of its 18 runs, and the gate requires zero excluded runs in both paired arms — so the generation could never produce a verdict on merit. Two causes, both in the session layer: 1. Claude Code drains background-task bookkeeping after the final result event (`background_tasks_changed`, `task_updated`, `task_notification`, all `type: "system"`). The parent-stream check required the result to be the literal last event, so three sessions that had exited 0 with a complete result and usage payload were recorded as session errors. Trailing `system` events carry no tool_use/tool_result/usage payload and cannot forge skill or cost evidence; anything else after the result still fails closed. 2. The 3600s per-session ceiling killed two `workflow` incumbent runs on inv-bug-pdg-note mid-verification. Successful `workflow` rows in the same run finished in ~1600-2600s across both sessions, so the ceiling moves to 5400s and now lives in one shared constant instead of two argparse defaults that could drift apart. Also marks the activation checklist against reality: the secrets, the Environment, the runner, and the validation dispatch are all in place; the repository variable GITNEXUS_EVOLUTION_ENABLED is the one remaining gap, and until it is set the Saturday cron skips the job in seconds while the EventBridge schedule still starts the runner for the day.	2026-08-01 16:45:48 +00:00
Gergő Magyar	bba25b2103	fix(eval): bind resolved node to a fresh sandbox path (corrects #2607) (#2609) ... * fix(eval): bind the resolved node to a fresh sandbox path, not one under /usr #2607 bound the resolved `node` to /usr/local/bin/node, but that path lives inside the /usr tree that _runtime_mount_args already read-only-binds wholesale. The second real workflow_dispatch run on the self-hosted runner (https://github.com/abhigyanpatwari/GitNexus/actions/runs/29840270554) failed immediately in the bubblewrap preflight: "bwrap: Can't create file at /usr/local/bin/node: Read-only file system" -- bwrap can't create a new mount-point file inside a tree it already bound read-only when the real path doesn't already exist there on the host, which is exactly the self-hosted case this bind exists to fix. Introduces SANDBOX_NODE (/opt/claude/node), a fresh path outside every tree _runtime_mount_args binds, following the same pattern SANDBOX_CLAUDE and SANDBOX_PYTHON3 already use. Updates the two real call sites (sanitized_graph.py, runner_sessions.py) to use the constant instead of the hardcoded literal, so the fix can't drift out of sync with itself again, and re-exports it from runner.py alongside the other SANDBOX_* names for the real-bwrap tests that reference it directly. Adds a real-bwrap test (gated behind GITNEXUS_REQUIRE_BWRAP_CANARY, same as the existing ones) that copies a real node binary to a path outside every bound tree and actually launches bwrap against it -- an argv-construction test alone can't catch a bwrap-level "Read-only file system" error, only a real invocation can, and that's exactly the gap that let #2607's version of this fix through review looking correct. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(eval): don't let the new real-bwrap test's node-mock break bwrap's own resolution CI caught this immediately: the new test_real_bubblewrap_runs_node_from_outside_the_bound_trees monkeypatched shutil.which to return None for anything but "node", but prepare_sandbox's own bwrap/claude resolution (_resolve_executable) goes through shutil.which too -- so the test broke bwrap discovery before the sandbox it's supposed to exercise could even be built ("SandboxError: required executable is unavailable: bwrap"). Delegate to the real shutil.which for every other name instead of blanket-returning None. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-07-21 16:09:45 +01:00
Gergő Magyar	5549403082	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>	2026-07-21 14:40:20 +01:00
Gergő Magyar	52b06b2642	fix(eval): stop using --bare for arms that need Skill or MCP tools (#2584) ... --bare hard-disables the Skill tool and every mcp__* tool by Claude Code design (confirmed against the pinned 2.1.214 binary; --allowedTools cannot restore what --bare removes). Every workflow_bench arm except baseline_nomcp needs Skill and/or GitNexus MCP tools, so every one of those sessions has been silently unable to invoke gitnexus-plan/work/ review or the CE comparator skills -- the last skill-evolution run (gen 0) scored 0/3 resolution on both arms across every task with error_kind "skill-not-invoked", not because the candidate was bad but because the harness could never invoke either arm's skill at all. Only baseline_nomcp keeps --bare (it explicitly wants zero Skill/MCP access anyway). The rest drop --bare and rely on ANTHROPIC_API_KEY alone; the sandboxed HOME has no OAuth/keychain state to conflict with it, and there's no committed .claude/settings.json in this repo for dropping --bare to newly pick up. Outside --bare the built-in toolset defaults to everything (WebFetch, Task, subagents, ...), and --allowedTools only pre-approves within whatever's available -- it doesn't narrow it. Added --tools for non-bare sessions so the intended tool scope is still enforced instead of silently widening. Claude-Session: https://claude.ai/code/session_01Va5uu9Ar3e45QZ5xFsG4AZ Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-07-20 21:26:11 +01:00
Gergő Magyar	becac9a5d3	feat(eval): run the skill-evolution loop online (#2571) ... * feat(eval): run the skill-evolution loop online Add a scheduled + dispatch-gated workflow that runs the offline propose -> benchmark -> gate loop (workflow_bench.evolve) in CI with the pinned Claude canary runtime and bubblewrap containment, uploads the benchmark evidence as an artifact, and on a gate-passed promotion opens a human-reviewed PR via the release App token. The applied overlay is bounded to the canonical skill tree and its shipped mirrors; any escape fails the run instead of reaching a PR. The scheduled lane ships disabled behind GITNEXUS_EVOLUTION_ENABLED and requires the new GITNEXUS_BENCH_AUTH_TOKEN secret (benchmark sessions bill real API usage), mirroring the review agent's staged rollout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Va5uu9Ar3e45QZ5xFsG4AZ * fix(ci): restructure promotion-PR script so no lint suppression is needed Replace the inline single-quoted credential helper with a GIT_ASKPASS file written via a quoted heredoc (the App token still reaches git only through step env at push time), and assemble the PR body from quoted heredocs plus double-quoted printf instead of a backtick-laden single-quoted template. Every run script in the workflow now passes shellcheck with zero findings and zero disables; the body and askpass rendering are smoke-tested. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Va5uu9Ar3e45QZ5xFsG4AZ * fix(ci): apply gate-passing overlays in the evolution loop The loop invoked workflow_bench.evolve without --apply, so apply_promoted_overlay (its only working-tree writer, gated by `if args.apply:`) never ran. git status stayed clean, promoted=false was emitted every run, and the App-token/PR-open steps were unreachable dead code — a gate-passing run went green as "No promotion this run". validate_promotion_for_apply already runs before the apply gate, so adding --apply lets a passing candidate reach the tree without weakening the deterministic gate; the boundary check then confirms it stayed in the skill trees. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Va5uu9Ar3e45QZ5xFsG4AZ * fix(ci): provision ~/GitNexus so the benchmark repo resolves on CI Every scenario in tasks.scenarios.yaml addresses the target repo as ~/GitNexus; runner_tasks.py resolves it with expanduser().resolve() then `git -C <repo> rev-parse`, which raises when the path is missing. On a hosted runner the checkout lands in $GITHUB_WORKSPACE and nothing created ~/GitNexus, so the first real run failed at task-binding. Symlink ~/GitNexus -> $GITHUB_WORKSPACE before the loop. The checkout uses fetch-depth: 0 (full history for the parentless clone), and the benchmark only clones the repo copy-on-write and mounts deps read-only, so the checkout is never mutated. GITNEXUS_BENCH_ORACLE_ROOT stays unset — it defaults to the in-repo oracles dir and is staged by the harness. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Va5uu9Ar3e45QZ5xFsG4AZ * fix(ci): harden promotion summary output and PR branch recovery Three fixes to the promotion-detection and PR-open steps: - GITHUB_OUTPUT summary used a fixed `PROMOTION_EOF` heredoc delimiter; a value containing that marker on its own line could close the block early and inject output keys. Use a per-run random delimiter, matching the pattern already in tree-sitter-upgrade-readiness.yml. - The summary concatenated every generation's promotion.json (including rejected ones), so the PR body could show a losing generation's decisions. The loop returns on the first promotion, so emit only the highest-numbered gen-N/bench/promotion.json — the decision that fired. - The promotion branch name omitted the run attempt. GITHUB_RUN_ID is stable across re-runs, so a re-run after push-succeeds/PR-create-fails could never push. Include ${GITHUB_RUN_ATTEMPT} (the artifact name already does). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Va5uu9Ar3e45QZ5xFsG4AZ * fix(ci): least-privilege the promotion App token and gate on an Environment The Mint-App-Token step passed only app-id + private-key, so the minted token inherited every permission the Release App installation holds (including Workflows: write) — far more than "push a branch, open a PR". Switch to `client-id` (as publish.yml does) and request only permission-contents: write + permission-pull-requests: write. Bind the job to a protected Environment (gitnexus-evolution) so promotion runs can be gated server-side. workflow_dispatch runs the workflow and in-tree evolve.py from the *dispatched ref*, so a code-side ref guard is removable by the dispatched branch itself; an Environment deployment-branch rule (main only) is the boundary that holds. The admin steps to create it and scope the secrets are documented in the activation checklist. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Va5uu9Ar3e45QZ5xFsG4AZ * fix(ci): correct upload-artifact pin comment and add shell strict-mode - The upload-artifact SHA 043fb46d… is v7.0.1 (labeled so in the sibling workflows that pin it); the comment mislabeled it # v6.0.0. Correct the comment; the pin is unchanged. - Add `set -euo pipefail` to the two build steps that lacked it, matching every other run block in the file (GitHub's default shell already sets -eo pipefail; this adds -u and consistency). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Va5uu9Ar3e45QZ5xFsG4AZ * docs(ci): complete the skill-evolution activation checklist - Add RELEASE_APP_ID / RELEASE_APP_PRIVATE_KEY to the required-secrets checklist (the Mint step hard-fails without them on a promotion) and the App-install-scope verification. - Document the protected Environment admin step and why it is the real boundary for the workflow_dispatch ref-secret exposure. - Note that workflow_dispatch runs the billing loop regardless of GITNEXUS_EVOLUTION_ENABLED. - Justify the weekly cron against the README's ~90-day guidance and note the 355-minute timeout ceiling. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Va5uu9Ar3e45QZ5xFsG4AZ * fix(eval): redact API tokens from diagnostic fields before artifact upload results.jsonl (runner.py) and proposer-session.json (evolve.py) serialize session records whose error_detail can carry a stderr_tail that echoed the API key. Transcripts are redacted before persistence, but these two sinks were not, and both land in the 14-day evolution artifact. Run each record's serialized JSON through the existing redact_text with the run's auth token before writing. Scoped to these diagnostic sinks only: the promoted overlay and proposal.md are left untouched (the overlay is the applied artifact and must stay byte-identical for apply and the shipped-skills-sync guard). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Va5uu9Ar3e45QZ5xFsG4AZ * test(ci): add a contract test for the skill-evolution workflow No test exercised this workflow's path, which is why both P1 blockers (missing --apply, unresolvable ~/GitNexus task repo) reached production. Parse the workflow YAML and assert the structural contract: --apply is passed, the task repo is provisioned, the promotion branch carries the run attempt, the App token is permission-scoped and the job is Environment- gated, the output summary uses a random delimiter and a single generation, the artifact pin is labelled correctly, and every multi-line shell step sets strict mode. Follows the review-agent-workflow.test.ts precedent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Va5uu9Ar3e45QZ5xFsG4AZ * feat(ci): run the proposer on its own (stronger) model One `model` input drove both the benchmark arms and the proposer/diagnosis session. Split them: `model` stays the benchmark arms (match the model your skill users run, so a promotion is valid for them and the tasks aren't ceiling-saturated), and a new `proposer_model` input runs the proposer — the harder meta-reasoning task that writes the candidate skill, and only one session per generation, so a stronger model is cheap here. evolve.py already supports --proposer-model; the workflow just didn't expose it. Defaults: arms = claude-sonnet-5, proposer = claude-opus-4-8 (both overridable via workflow_dispatch). The weekly cadence bounds the added spend. Contract test asserts the split stays wired. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Va5uu9Ar3e45QZ5xFsG4AZ --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>	2026-07-20 05:37:07 +01:00
Gergő Magyar	8b5057f325	feat(skills): GitNexus Engineering Tool Kits (#2566) ... * feat(skills): add ce-plan — GitNexus+PDG implementation-planning skill Adds .claude/skills/ce-plan: a planning-only skill that builds implementation-ready plans from GitNexus graph navigation (query/context/ impact/trace), bounded statement-level PDG slices (pdg_query, impact mode:pdg, explain), and targeted source verification, with a context ledger to prevent repeated reads and a machine-readable implementation context pack (stable contract for a future ce-implement). Whitelisted in .gitignore and registered in AGENTS.md and CLAUDE.md outside the auto-managed gitnexus block. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(skills): apply ce-plan validation findings (tool contract, consistency, conventions) Tool contract: impact mode:'pdg' shape now includes the schema-required direction param; CDG branch sense documented as the result 'label' field (reason is cypher/raw-edge only); explain caveats corrected to its real false-negative classes (cross-function TAINT_PATH is modeled). Consistency: PDG slice homed in working memory (ledger keeps one-liners); depth knob defined and category-overrides-baseline ordering stated; call_depth (consumed by nothing) and content-hash bookkeeping dropped; Never section folded into Hard rules; Phase 3 deduplicated to a pointer; allowed-repeat escalations defined; budget/discard accounting clarified; verification-commands gathering added to Phase 4; open_questions added to the context pack. From scenario runs: plans now pin the verified-at HEAD commit and index freshness in a header, tag claims [verified]/[graph]/[inferred]/[assumed], quote load-bearing tool output, prefer pre-hook-carrying npm scripts, and support an out:<path> destination override; output path defined as the Phase 1 target repo root. Conventions: AGENTS.md 1.9.0 / CLAUDE.md 1.4.0 changelog rows + metadata bumps; future ce-implement qualified as future. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(skills): rename ce-plan → gitnexus-plan; add cross-CLI (Codex) entrypoints Renames the skill dir, frontmatter, output filename convention, plan H1 (GitNexus Engineering Plan), the future executor handle (gitnexus-implement), the .gitignore whitelist entry, and all AGENTS.md/CLAUDE.md references. Follows the pr-swarm-review cross-CLI pattern: SKILL.md is the canonical CLI-neutral spec, AGENTS.md § Engineering planning is the Codex/any-agent entrypoint, and the README documents the optional user-level ~/.codex/prompts/gitnexus-plan.md slash command plus an invocation matrix. Skill prose de-branded from Claude Code (agent-neutral verification layer). Also fixes two post-review README contradictions: the anti-reread claim now names the ledger's allowed escalations, and 'read-only by contract' is now 'planning-only' (the skill writes exactly one repo file — the plan); the scope-creep rule and template §12 now agree on where deferred follow-ups land. Drops the stale plugin-collision limitation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(skills): document Codex user-level install path for gitnexus-plan Codex discovers SKILL.md skills from ~/.agents/skills (same path the other gitnexus-* skills install to); README now documents the cp install plus the optional ~/.codex/prompts slash-command file, with the prompt body preferring the repo copy and falling back to the user-level install. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(skills): gitnexus-plan freshness gate + active PDG-layer refresh Freshness is now a Phase 1 gate, not advisory: under the default freshness:strict, a stale index is refreshed once per planning session via node .gitnexus/run.cjs analyze --index-only (appending --pdg when the task will reach the PDG phase), then the context resource is re-read. A missing PDG layer likewise triggers the one permitted --index-only --pdg refresh and re-probe instead of a passive recommendation. freshness:accept (or a failed/impractical refresh) preserves the old behavior: plan on the stale graph, source-weighted, labelled in the plan header. --index-only is the load-bearing flag choice — it suppresses all file generation, so the planning-only contract holds (only the .gitnexus store changes). Ledger gains an index_refresh record; plan header states fresh / refreshed / refresh-skipped-with-reason. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(skills): gitnexus-plan runner build check before freshness refresh When the target repo builds the analyzer from its own source (bin → dist/ mapping, as gitnexus/ does), the Phase 1 freshness gate now verifies dist/ is current before running the analyze refresh — rebuilding via the package's build script when any analyzer source file is newer than the built entrypoint — and prefers that freshly built CLI. Otherwise a stale dist re-indexes with outdated extraction logic and the 'fresh' index lies. Rebuilds are recorded in the ledger's index_refresh; the PDG-phase refresh inherits the same check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(skills): add gitnexus-work executor and gitnexus-lfg pipeline gitnexus-work executes a gitnexus-plan as verified atomic commits: consumes the §11 implementation_context pack, drift-checks the plan's evidence pin against HEAD, re-verifies assumptions before relying on them, runs impact before every symbol edit and detect_changes before every commit (repo mandates), builds tests from the plan's scenarios, and routes structural drift back to gitnexus-plan Deepen mode instead of coding around it. gitnexus-lfg is a thin orchestrator: gitnexus-plan → blocking user gate (deepen / proceed / stop, deepen loops allowed) → gitnexus-work → review via the existing gitnexus-pr-review skill (open PR, else branch diff vs default). One bounded fix cycle for review findings; never pushes or opens a PR on its own. gitnexus-plan gains a Deepen mode (re-run freshness gate, escalate to depth:deep, re-verify graph/inferred/assumed claims toward verified, rewrite the same file); its 'future gitnexus-implement' placeholder is retired in favor of gitnexus-work. Registered via .gitignore whitelists, AGENTS.md 1.10.0 (section renamed to Engineering planning & execution), CLAUDE.md 1.5.0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(skills): apply cross-skill review findings to the gitnexus skill family Two P1s: gitnexus-plan Deepen mode now re-anchors before re-pinning (diffs the old evidence pin over every [verified]-claim file and re-reads or downgrades before the header moves — moving the pin without this laundered stale claims as verified); the index-refresh budget is stated once in Phase 1 (one --index-only refresh plus at most one Phase 3 --pdg upgrade per session, Deepen = its own session) with ledger and pdg-slice deferring to it. Contract fixes: gitnexus-work's drift check now covers every file the pack cites (not just files_to_modify) and parses the full pack incl. primary/related symbols and acceptance_criteria (walked in Phase 4 alongside §13); a pre-completed check skips §7 steps already landed and Deepen gains a reconcile-execution-state step, closing the mid-execution route-back loop; pack assumptions must name what to check and how. lfg: Lane 4 passes the merge-base to detect_changes compare (two-dot diff misattributes upstream commits when default advanced), branch-diff is the stated normal case, oversized review findings route to the plan gate instead of overflowing direct mode, the one-fix-cycle cap is explicit on re-run, and headless runs end at the plan gate with the plan as deliverable. work: blank mode narrowed to *gitnexus-plan*.md with a re-execution guard, direct-mode discipline spelled out, branch meaningfulness defined against the plan slug, and the plan document is committed as the branch's docs commit (review diff includes it). Planning-only contract now names the dist/ rebuild as the second permitted state change; Phase 5.1 names the four claim tags; stale AGENTS.md anchors fixed. Known latent issue left untouched: gitnexus/gitnexus-pr-review pairs a three-dot example with a two-dot detect_changes compare — that skill is also shipped by the plugin, so fixing it here would drift the copies; lfg compensates by passing the merge-base. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(skills): ship the engineering skill family with the gitnexus package npm i -g gitnexus users now get gitnexus-plan / gitnexus-work / gitnexus-lfg: the three skills are added to gitnexus/skills/ in directory form (SKILL.md + references/), which installSkillsTo already enumerates dynamically and copies recursively to every editor target (~/.agents/skills for Codex, Cursor, OpenCode, Qoder, ...) on gitnexus setup — uninstall enumerates the same root, so removal stays clean. The Claude Code plugin channel (gitnexus-claude-plugin/skills/) carries the same copies plus the standard per-skill mcp.json. Global-install support in the skill text: gitnexus-plan Phase 1 now resolves the analyzer runner explicitly — node .gitnexus/run.cjs analyze when the project has a runner, else gitnexus analyze (installed CLI), else npx gitnexus analyze — and all analyze mentions route through it, satisfying the skills-steering policy (#1939/#1945) which sweeps the plugin copies. New drift guard test/unit/shipped-skills-sync.test.ts asserts the npm and plugin copies stay byte-identical to the canonical .claude/skills/ family (plugin = canonical + mcp.json), same discipline as run.cjs ↔ resolve-invocation.ts. skills-steering + shipped-skills-sync: 11/11 green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(eval): workflow_bench — measure the skill workflow's token savings Benchmarks gitnexus-plan → gitnexus-work against a baseline agent (--disallowedTools Skill) on identical tasks, in fresh detached worktrees, using real headless Claude Code sessions; every number comes from the CLI's --output-format json usage report (field names validated against a live 2.1.207 session). Reports per-arm medians (input/cache/output tokens, cost, wall time, turns), a savings row, and resolve status from a per-task verify command — savings on failed tasks are flagged, not celebrated. Per-task setup hook prepares fresh worktrees (deps); --permission-mode bypassPermissions (default) lets sessions run unattended in the throwaway trees. Free-model support: --base-url/--auth-token/--model route headless sessions through any Anthropic-compatible endpoint; free-model.litellm.yaml is a ready litellm-proxy template for OpenRouter :free variants or local Ollama, so benchmarking burns no paid tokens (README documents rate limits and the small-model skill-following caveat). Harness validated end-to-end with a stub CLI (worktree lifecycle, both arms, plan→work chaining, verify, aggregation, report) and 4 pytest units for the pure aggregation/savings/report helpers. AGENTS.md 1.11.0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(eval): record first workflow_bench calibration run Trivial-task calibration (add -V alias): both arms resolved; workflow arm ~4.3x baseline cost — the documented overhead-dominated regime, recorded so the regime boundary is empirical rather than asserted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(eval): workflow_bench scenario matrix — arm variants, task classes, churn Ground-base measurement across scenarios: tasks.scenarios.yaml spans four labeled classes (trivial → investigation-bug → investigation-feature → cross-module) with deterministic verifies (prescribed test files). New arms: workflow_direct (gitnexus-work direct mode — the middle option that locates the routing boundary lfg's gate and work's triage encode) and baseline_nomcp (no skills AND no graph tools — separates workflow-discipline value from GitNexus-tool value; off by default). Records now carry task class and diff churn (files/+ins/−del vs the starting commit) as an over-engineering proxy; the report renders a class column and per-arm savings rows vs baseline. 5 pytest units + stub-CLI e2e of the full three-arm matrix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(eval): record workflow_bench ground base; fix churn measurement bias Ground base (3 classes x 3 arms, n=1/cell): every arm resolved every task — pass/fail quality saturates at this difficulty, making the comparison pure cost. Full plan→work never amortized its ~$9-11 fixed cost on tasks a baseline finishes in ≤35 turns (−211% to −333% cost); workflow_direct sits near baseline (−15% to −55%, once faster wall) with more test coverage. Routing implication recorded: direct mode/plain agent below this scale, full workflow for cross-module / multi-session / plan-as-deliverable work. The cross-module cell and multi-run variance are the next measurements. Churn fix: git add --intent-to-add -A before diffing (arms that never commit no longer undercount new files) and :(exclude)docs/plans (the committed plan doc no longer inflates workflow churn); this run's churn numbers predate the fix and are omitted from the recorded table. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * perf(skills): cost-optimize the workflow from measured ground base Every optimization targets a measured fixed-cost component (eval/workflow_bench ground base: workflow arm −211% to −333% vs baseline, all tasks resolved): - Plan form is category-priced: compact form (core sections w/ § anchors preserved, ≤80 lines excl. pack, mini-pack subset of the context pack) for narrow/default categories; the full 13 sections only for deep work (refactor/security/performance/concurrency/architecture). A compact plan outgrowing its cap reclassifies to full rather than overflowing. - Freshness gate is category-priced: compact categories default to accept (source-weighted, refresh only when a graph claim becomes load-bearing); strict stays the default for full-plan categories — the rebuild+re-index was the largest single fixed cost. - Turn economy: per-category tool-call budgets (~10 to ~45; architecture uncapped); budget exhaustion routes open questions to §12 instead of more digging. - gitnexus-work fast path: HEAD == evidence pin → skip all citation re-reading (the pin's entire point); mini-pack fields tolerated. - lfg Lane 1 boundary triage: tasks below the measured ~35-turn boundary get offered gitnexus-work direct mode before the plan lane is spent. Copies re-synced (npm skills/, plugin, ~/.agents); steering + sync guards green. Re-measurement of the workflow arm follows to verify the numbers actually improve. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(eval): record optimization re-measurement — inv-bug workflow cell −20% cost Same task, same conditions, post-830a0459 skills: $14.56→$11.70 (−20%), 83→72 turns, cache_read −24%; verified in-transcript that the compact form, turn budget, and skipped rebuild/re-index all fired. Wall +15% from a work- session test-debugging tail (n=1 variance). Regime unchanged (~3.5x baseline on this class) — routing rule stands. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(eval): per-arm clone isolation — worktree ref-namespace leak contaminated an arm The cross-module workflow_direct cell reported an impossible 28-turn solve with churn byte-identical to the workflow arm: git worktree add shares the repo's ref namespace, so the workflow arm's slug branch (created by gitnexus-work Phase 2) survived worktree removal and the direct arm found and adopted the completed work. Arms now get isolated git clone --shared copies (object store via alternates, refs clone-local — agent branches and stashes die with the clone; origin/<ref> fallback for non-default refs). Leaked branch deleted; baseline arm verified clean (0 branch references in its transcript); cell marked invalidated pending re-run. Records the valid cross-module cells: workflow $18.32 vs baseline $18.03 (premium −1.6%, vs −211%..−333% on smaller classes) — fixed costs amortize at this scale, with a less destructive diff and a plan artifact as bonus; resolve rate still tied. Churn fingerprinting is what caught the contamination — noted in the README as an integrity check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(eval): complete cross-module cell — direct mode wins 47% cost / 56% wall Clean clone-isolated re-run: workflow_direct resolved the hardest class at $9.53/52 turns/15m vs $18.03/98/34m baseline and $18.32/107/37m full workflow. The measured story across all four classes: the execution discipline (gitnexus-work) is the consistent sweet spot and delivers real token savings on hard tasks; the planning pass buys its artifact, not same-session savings. Resolve rate tied everywhere (n=1/cell caveat). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(eval): add trajectory-gated skill evolution (#2431) - Pair prompt candidates with incumbent workflow arms - Gate promotions on pinned-model quality and efficiency - Expire router evidence and document its lifecycle * fix(eval): allow pr-review skill candidates * feat(skills): rename and generalize GitNexus review * feat(eval): external-comparator and review arms for workflow_bench - ce_workflow / ce_workflow_direct: compound-engineering ce-plan/ce-work arms prompted with the same structure as the gitnexus arms - review / ce_review: gitnexus-review vs ce-code-review on an identical diff applied by the task's setup - plan handoff is snapshot-based: committed example plans in docs/plans/ tie on clone mtimes and broke the name-glob pick (executed a stale plan) - verify output tail is recorded per run and the final working-tree patch is kept, so failed rows are diagnosable after the clone is destroyed Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(skills,eval): address #2431 review — data-safe rename migration, fail-closed bench evidence - setup: never delete a legacy renamed skill dir — the installer cannot prove ownership (users customize or hand-write skills under these names); warn with the path instead, and the test now asserts survival - workflow_bench: fail closed when a session's --output-format json report is empty, malformed, or missing usage fields — an exit-0 shell with no parseable usage no longer counts as measured evidence (5 parametrized regression tests) - workflow_bench: document the trust model prominently (task setup/verify are shell-executed, sessions run bypassPermissions with the parent env, candidate overlays are prompt injection surface) in README + docstring - free-model.litellm.yaml: master_key from LITELLM_MASTER_KEY env instead of a static token; loopback-binding warning - ci: run the eval workflow_bench pytest suite on ubuntu (pytest+pyyaml only — no full eval stack) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(eval): demand observed foreground verification in headless work-arm prompts In a headless -p session there is no later turn: a work arm backgrounded its slow test run, scheduled wakeups that can never fire, and reported done while two of its tests failed. All four work-arm prompts (both skill families, symmetric) now require verification output to be observed inside the session. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(skills): ask plan depth up front instead of offering deepen afterwards gitnexus-plan Phase 0 now asks one blocking question in interactive sessions — quick / standard / deep, mapped onto the existing depth/form/ freshness knobs — when the invocation carries no explicit depth signal. Explicit knobs and headless runs skip the question (category posture unchanged, so benchmarks and automation behave as before). gitnexus-lfg's plan gate slims to proceed/stop: depth was already the user's up-front choice, so deepening is no longer offered by default — an explicit deepen request at the gate and executor route-backs still run Deepen mode, which remains the mechanism for strengthening an existing plan document. All shipped copies resynced (npm skills/, Claude plugin); AGENTS.md 1.13.0 and CLAUDE.md 1.7.0 pointers updated, including the analyzer's regenerated index-stats block at this branch's head. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(skills): taint pass, expert lenses, and post-work index refresh gitnexus-review gains a PDG-backed taint-and-dependence pass (explain + pdg_query, --pdg folded into the stale refresh on trust-boundary diffs) and an Expert lenses section: domain reviewers derived from the graph's clusters plus four cross-cutting lenses (architectural fit, language conformance per the repo's own contract, Definition of Done, simplicity), dispatched once after the evidence-gathering steps and scaled to the diff. gitnexus-work Phase 4 now refreshes the knowledge graph after the DoD walk via the resolved-runner ladder with analyze --index-only, so the lfg review lane and later sessions query the finished work without dirtying the tree. lfg's threshold-governance paragraph moves to its README; eval citations are tagged as measured in the GitNexus repo. All shipped copies re-synced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(cli): remove legacy gitnexus-pr-review on uninstall; cover the rename migration uninstall's removal set now includes LEGACY_SKILL_DIR_NAMES derived from RENAMED_SKILL_DIRS, so a pre-rename install is cleaned up instead of orphaned. The rename warning gains behavioral coverage (fires with a legacy dir present, silent without), and shipped-skills-sync asserts legacy names stay absent from every shipped tree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(eval): metric provenance, error-kind rows, skill-invocation verification, gate noise floor The promotion gate defaults to cost_usd (the only metric that includes subagent spend); token metrics carry an explicit main-loop-only warning in the report and promotion.json. Rows are classified by error_kind (session-error / verify-failed / infra-error), excluded from efficiency medians, and the gate requires equal valid-run counts. Each session's transcript is scanned for the expected Skill invocation and fails closed on a verified miss; a one-run resolution edge no longer promotes (noise floor). Per-run timeouts and setup failures record an infra-error row instead of aborting the sweep. Overlays touching skills no candidate arm exercises are rejected up front. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: fix skill routing paths, version headers, and skill rosters Routing tables point at the tracked direct skill paths (matching the post-#2434 generator output), AGENTS.md/CLAUDE.md headers match their latest changelog rows, the 1.12.0 row describes what the migration actually does, package/cursor READMEs list the full shipped skill roster, and the swarm READMEs describe /gitnexus-review's expert lenses instead of calling it single-agent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: drift-guard workflow for skill copies; pin eval pip deps; track docs/plans ci.yml ignores '**.md', so an md-only skill edit would merge without the shipped-skills-sync test running — skill-sync.yml triggers exactly on the guarded trees. The eval job's pip install is version-pinned, and docs/plans/ is unignored so gitnexus-plan output can be committed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(ci): keep the runner-invocation literal in gitnexus-review; add concurrency block to skill-sync skills-steering requires skills with a stale-index hint to carry the exact 'node .gitnexus/run.cjs analyze' form — restore it with the fallback ladder as a parenthetical instead of replacing it. skill-sync.yml gains the top-level concurrency block the workflow-convention check enforces. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(skills): token-economy guidance for expert lenses Merge lenses that ground in the same material into one reviewer, and use cheaper model/effort tiers for mechanical lenses where the harness offers them, reserving the strongest engine for adversarial judgment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(eval): isolate transcript home on Windows Ensure workflow_bench transcript tests set USERPROFILE alongside HOME so Path.home() resolves to the temporary test home on Windows. * docs(skills): fold PR #2522 execution learnings into review/work/plan Eight incident-backed hardenings from running the full skill cycle (review -> plan -> work, 28-finding fix series) on PR #2522: gitnexus-review: - Expert lenses execute the code under review on candidate failing shapes (empirical probe outranks source reading — every HIGH the language lenses found came from a probe, not a read). - Step 7 re-runs the exact CI check for refreshed baselines/fingerprints (a stale committed artifact is invisible in the diff; caught a red benchmarks arm). - Step 8 treats version/invalidation constants as review surface (INCREMENTAL_SCHEMA_VERSION class recurred verbatim from #2494). gitnexus-work: - Step 4 proves regression tests discriminate against the pre-fix tree. - Step 5 rebuilds executed build output before every verification run (parse workers load dist/; a correct fix 'failed' until rebuilt). - Step 6 makes stage -> detect_changes -> commit one unbroken sequence. gitnexus-plan: - Phase 0 seeded-evidence mode: plan FROM a completed review's verified findings instead of re-running the graph ladder. - Template §7: fingerprint/golden-guarded output rebaselines once, at the series tip. All distribution copies resynced; shipped-skills-sync + skills-steering 24/24 locally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(eval): close the skill-evolution loop with an automated proposer driver workflow_bench.evolve adds the three arrows the README described as manual: a proposer session that turns loser trajectories (results.jsonl rows, transcripts, patches, the learning queue) into ONE bounded candidate overlay, a driver that iterates propose -> paired benchmark -> deterministic gate up to --generations, and an --apply step that copies a promoted overlay onto the canonical skills and shipped mirrors as a working-tree diff. The trust boundary is unchanged: overlays re-validate through candidate_overlay_files before any benchmark or apply consumes them, and committing, CI, and the PR merge stay human. learnings.jsonl is gitignored: it is machine-local evidence, like the session transcripts it complements. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(skills): route live-task friction into the evolution learning queue Each family skill gains a short 'Skill feedback' section: on friction with the skill's own instructions, append one JSON line to eval/workflow_bench/learnings.jsonl (GitNexus repo only) — never self-edit the skill from a live task. The proposer in workflow_bench.evolve consumes the queue as hints; a learning reaches a shipped skill only by beating the incumbent on the paired benchmark. All shipped mirrors re-copied byte- identical. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci(tests): run the evolve helper tests in the eval pytest job test_evolve.py needs only pytest+pyyaml, same as the harness tests the job already runs — without this line the new module had no CI coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(ci): comment-triggered GitNexus review agent for PRs '@gitnexus review' from a maintainer (OWNER/MEMBER/COLLABORATOR; the action re-validates write access) runs the repo's gitnexus-review skill headlessly against the PR and posts the review as a sticky comment — remote triggering with no local setup. Read-only by construction: contents: read token, Write/Edit and web tools disallowed, Bash allowlisted to git reads and the gitnexus CLI; analyze parses PR code with tree-sitter, never executes it. Requires the ANTHROPIC_API_KEY repository secret; activates once the file is on the default branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(ci): dispatch lane + existing OAuth secret for the review agent Align with claude.yml: same action pin and the CLAUDE_CODE_OAUTH_TOKEN secret the repo already carries — no new secret to configure. Add a workflow_dispatch lane (PR number input) so the agent can be triggered from the Actions UI and tested before the issue_comment trigger reaches the default branch. Allowlist gh pr view/diff and gh api, which the review skill uses to pin PR SHAs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(ci): close a fork-PR RCE vector in the review agent's tool allowlist A live headless run of the exact workflow session against PR #2431 (66 turns, full gitnexus-review pass) surfaced a real HIGH-severity confused deputy: .gitnexus/ is gitignored, not blocked — a fork PR can commit its own .gitnexus/run.cjs, issue_comment checks out PR-head content, and the skill's runner ladder tries 'node .gitnexus/run.cjs analyze' first. That would execute fork-controlled JS inside a job holding CLAUDE_CODE_OAUTH_TOKEN and a write-scoped GITHUB_TOKEN — the opposite of the 'PR code is read, never executed' claim in the workflow's own header. Fix: drop the run.cjs allowlist entry so analyze always resolves through npx gitnexus (npm registry, not the checked-out tree); the skill's documented fallback mode covers the resulting graceful degradation. Also drop 'gh api' (not read-only — accepts -X POST/PATCH/DELETE) and downgrade pull-requests: write to read (comment posting only needs issues: write; the prompt already forbids formal review submission). Same session flagged a latent evolve.py bug: select_evidence's cost sort used dict.get's missing-key default, which doesn't cover an explicit JSON null in a foreign --seed-results row and crashes proposer setup with TypeError. Guarded with 'or 0.0' and added a regression test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: harden PR review and evolution trust boundaries * ci: follow workflow concurrency convention * fix(eval): make terminating error paths explicit * fix: unblock hardened review runtime checks * test: make containment canaries deterministic * test: expose Claude canary tool failures * fix: adapt clean shell environment for Claude * fix(eval): accept the runner's transcript source key in evidence preflight The proposer evidence preflight required transcript-artifact metadata to be exactly {path, sha256, bytes}, but the runner stamps a fourth provenance key (source=parent-captured-stream-json). Any --seed-results or generation>=2 run therefore aborted with SandboxError before proposing or promoting. Pin the producer literal as PARENT_EVENT_STREAM_SOURCE and validate it in the metadata check, and round-trip real producer output through sum_sessions into the preflight so the schema can't drift again. * fix(eval): treat an unmeasured session cost as unavailable, not $0 well_formed validated only the nested usage block, so an otherwise-successful session missing total_cost_usd was recorded as cost_usd=0.0 — and cost_usd is the default promotion metric (lower wins), so a cost-less session scored as free and could win promotion it never earned. Extract cost via measured_cost() (None on absent/garbage, a measured 0.0 preserved), propagate None through sum_sessions/aggregate/savings/report, and have the gate refuse to rank on a metric that was not measured on every run in both arms. * fix(eval): warn when ranking on the main-loop-only num_turns metric num_turns comes from the CLI's top-level usage (main-loop session only), like output_tokens, but selecting it emitted no metric_warning — so a subagent-heavy candidate could look artificially efficient. Add num_turns to MAIN_LOOP_ONLY_METRICS and broaden the warning to cover turns. * fix(eval): fail closed when an overlay adds a file with no committed base An overlay adding a new .md under gitnexus-{plan,work} passes the structural overlay checks but has no committed base for committed_destination_base_digests to bind against, so it raised an uncaught ValueError that crashed the evolve driver (and runner --candidate-overlay) mid-run. Catch it at both call sites: evolve reports NOT PROMOTED and exits, runner routes it through parser.error. * feat(eval): circuit-break the runner sweep on a systemic outage A sustained upstream outage used to pay out every remaining --timeout window one session at a time. Track consecutive session/infra/cleanup failures via a pure systemic_outage_streak helper; after --outage-streak (default 5) in a row, stop the sweep, still write report.md/promotion.json from partial evidence, and exit non-zero so evolve.py halts instead of proposing from truncated evidence. A task's own resolved=False never trips the breaker. * fix(cli): report a dirty working tree as stale in gitnexus status status --json (and the human output) computed up-to-date from commit + runner identity + completeness only, so a repo with uncommitted source changes at a matching HEAD was reported up-to-date while analyze would still re-index it. A graph-backed agent gating on that JSON could skip re-analysis on a stale graph. Extract analyze's dirty-tree check into a shared isWorkingTreeDirty() in storage/git and fold it into the status freshness decision. * fix(ci): use single-slash deny globs in the review agent's disallowedTools github.workspace already expands to an absolute path, so Read(/${{ github.workspace }}/**) and Read(//proc/**),(//sys/**),(//dev/**) produced double-slash patterns that a normalizing matcher may not match — silently no-opping the deny layer. Not exploitable (the allowlist is the primary control and never grants those paths), but the globs should be well-formed. Update the pinned test strings. * ci: install gitnexus-shared with npm ci from the committed lockfile The gitnexus-shared build floated its deps via npm install in three workflows (skill-sync, ci-tests, and — most importantly — the release publish.yml) while every other install step uses npm ci. The lockfile is committed and in sync, so switch all three to npm ci for reproducible, locked installs. * test(cli): make the shipped-skills drift guard reject symlinks listFilesRecursive walked with readdirSync and snapshotDir read with readFileSync, both of which follow symlinks — so a mirror file symlinked to the canonical tree passed the byte-compare (and a symlinked mirror dir would be followed too). Reject a symlinked root via lstat and any symlinked entry via Dirent.isSymbolicLink, with negative tests (skipped on Windows). * test(eval): guard the candidate-skill vs mirror-root coverage invariant MIRROR_SKILL_ROOTS omits the Cursor tree, safe only because no candidate skill is cursor-shipped. Pin that invariant: every CANDIDATE_SKILLS entry must exist under canonical + every mirror root and must not ship to Cursor, so adding a cursor-shipped skill to the candidate set (the PR #2488 asymmetric-sync class) fails loudly instead of syncing three of four trees. * docs(ci): describe the review agent's staged post-merge rollout The DoD asked for a dry-run or triggered run before merge, but an issue_comment (or newly added workflow_dispatch) workflow only ever executes the default-branch copy, so it cannot be exercised from the PR that introduces it. Reword the DoD and the activation checklist to a staged rollout: merge registered-but-disabled, validate same-repo and fork execution post-merge, then enable the variable. * fix: pin plugin skill mcp.json to the release version via #2445 tooling The ten plugin skill mcp.json launched `npx -y gitnexus@latest mcp` on every skill connect — non-reproducible and a supply-chain surface, and (unlike the persisted setup config) never pinned. Extend sync-plugin-manifests.mjs with an mcp surface kind that stamps the gitnexus@<version> launch arg, pin all ten to 1.6.9 now, and keep them byte-identical so the drift guard stays green. The release lifecycle + publish.yml --check now re-stamp them like the four manifest surfaces; only READMEs stay on @latest as docs. * test(eval): prove the proposer's built-in file tools are confined The real-Claude canary only exercised Bash + MCP, so it proved process/MCP containment but not that the proposer's built-in file tools stay inside their mounts. Add a canary over the exact PROPOSER_ALLOWED_TOOLS surface and the same read-only /evidence mount as run_proposer (allowlist extracted to a shared constant so it can't drift): Read reaches /evidence, a Write into the read-only evidence mount is denied, and a Write lands in the output tree. * fix(eval): apply the candidate overlay after task setup for fair arms The candidate overlay was applied before the task's untrusted setup ran, so setup could observe candidate prose and the incumbent/candidate arms started from different pre-overlay state. Reorder within the sandbox: capture the base (pre-overlay) skill digest, run setup against the base skills, verify setup did not tamper them, then apply the overlay and capture the post-overlay digest the model must preserve. apply_candidate_overlay stages path-specific overlay files, so setup's uncommitted changes stay out of the baseline and churn is unchanged. Graph freshness for the review arm is handled by the status dirty-tree fix plus the review skill's stale-triggered re-index, not by reordering the cached per-task-sha graph materialization (which is mechanically blocked). * test(eval): end-to-end containment proof of the autonomous proposer Drives the real run_proposer through bubblewrap with a deterministic scripted model (no paid API): it reads the read-only evidence bundle and writes a candidate gitnexus-plan skill edit plus a rationale into the sandbox output tree; run_proposer enforces the trust boundary and copies only the validated overlay + proposal out. This exercises the autonomous-proposal stage of the self-evolution loop end-to-end in the eval/containment CI job (the gate and apply stages are covered by test_workflow_bench_evolution and test_promotion_apply). Env-gated on GITNEXUS_REQUIRE_CLAUDE_CANARY, so it runs only where the pinned Claude binary and user namespaces are available. * fix(eval): let the proposer author its overlay via Bash Running the end-to-end proposer canary in the containment CI job surfaced a real bug: run_proposer starts the session with --bare, which hard-disables the Write/Edit tools ("Write exists but is not enabled in this context"), yet allowlisted Edit/Write and omitted Bash. The proposer therefore had no working way to write its candidate overlay — the self-evolution loop could never produce a candidate. The sandbox settings already pre-authorize Bash (autoAllowBashIfSandboxed) and confine writes to workspace/tmp/home, so switch PROPOSER_ALLOWED_TOOLS to Read/Grep/Glob/Bash and tell the proposer to author files with Bash. The end-to-end test now drives the real run_proposer through bubblewrap and asserts a validated overlay + proposal are produced (this also replaces the earlier file-tool canary, whose Write/Edit premise was moot). * test(eval): author the proposer overlay with newline-free Bash content The nested shell-sandbox prefix mangles embedded newlines, so the multi-line overlay content never landed. Use single-line content for the deterministic proposer canary. * test(eval): drop the unverifiable end-to-end proposer canary The scripted proposer overlay never materialized in the containment job across runs, and the model tool-result content is not visible in CI logs, so the test cannot be finalized without an environment where the sandbox can actually run. Keep the verified production fix (Bash-authoring in run_proposer); the proposer sandbox/containment stays covered by the existing Bash+MCP and process-tree canaries. * test(cli): drop run-analyze.ts from the windowsHide spawn-family list U7 moved run-analyze.ts's only child_process call (the git status --porcelain dirty check) into storage/git.ts (already covered by this test, with windowsHide). run-analyze.ts no longer imports a spawn-family function, so the windowsHide-regression test's 'must have >=1 spawn call' invariant failed for it. Remove it from SRC_FILES. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Zander Raycraft <zanderjraycraft@gmail.com> Co-authored-by: Azizur Rahman <azizur100389@gmail.com>	2026-07-19 15:07:24 +01:00