2/3 Add the redesigned VeRO system (vero/)#45
Open
varunursekar wants to merge 53 commits into
Open
Conversation
Introduce the v0.5 codebase: vero/ (core + harbor eval sidecar, build compiler, runtime/observability), vero-tasks/, and runs/. Root README points to legacy/ for the original-paper code.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…back #1) Addresses the 'baseline floor fails open' regression: - baseline_floor now defaults to false. The floor gates a ship on a validation comparison while the reward is on the (possibly differently-distributed) target, so it is opt-in. - Pin the fixed seed's score to avoid re-scoring it every run (reproducibility + one fewer eval): VerificationTarget.baseline_reward (per-target, post scale/offset) and VerificationSelection .baseline_selection_score (for the floor). Plumbed through the build config + compiler. - Fail safe when the floor is on, unpinned, and the seed re-score can't be measured: ship nothing (NoCandidateError -> shipped=false + infra error) instead of shipping the best candidate unverified (the fail-open bug). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
#1) Add an admin-only way to generate the number to pin: CanonicalVerifier .measure_baseline(replicates) admin-scores the fixed seed N times on the selection partition and each target and returns per-key mean/stddev. Exposed as POST /score/baseline and 'vero harbor score-baseline --replicates N'. Reuses the existing admin scoring machinery. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…atten The benchmark-config tests read harness-engineering-bench/ (a separate branch in the stacked split), so skip them when it isn't checked out. Update the paths for the candidates/ -> top-level flatten (all benchmarks are now top-level siblings of gaia/). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
min_aggregate_cases defaulted to 1 — a vacuous floor that let an agent read held-out validation labels one case at a time via single-case aggregate evals. Default to 5 in both the sidecar policy and the build AgentAccessSpec so a build that omits it is safe rather than unfloored (shipped builds already set 5 explicitly). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Re-enabling harness isolation broke the eval: 'uv run' as the unprivileged harness got a fresh empty uv cache and couldn't resolve the candidate package from it (No module named 'gaia_agent'), with no network to download at eval time. Pre-warm /home/harness/.cache/uv from the build (root) cache in the sidecar image, matching the UV_CACHE_DIR the backend sets for the harness user. Also fixes a test regression from feedback #2: with the k-anonymity floor defaulting to 5, the instruction now reads 'must include at least 5 cases' rather than 'arbitrary subsets'. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…ack #8) The mean-of-k recorded only score/n_attempts/n_scored, so an outage-diluted cell (dead attempts zero-filled) was indistinguishable from a genuinely clean low cell. Add n_dead_infra (dead-to-infrastructure attempts, classified via the existing error taxonomy) and n_clean (the rest), so the split is visible in the live metrics rather than only reconstructable from raw trial records. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
- optimizer.run: validate self.max_proposals >= 0 before the proposal_limit guard, so the specific 'must be non-negative' error is reachable (was a dead branch that always tripped the generic message first). - compiler._safe_extract_tar: extractall(filter='data') to strip device files / setuid bits and neutralize unsafe links, matching extract_harbor_session_archive (also silences the py3.14 deprecation). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
A cold-start walkthrough of the harbor path: the three-container trust boundary, a run end-to-end, the evaluation core, disclosure/selection, the five security mechanisms, and a suggested outside-in review order keyed to the source files. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Update harbor_requirement pin from harbor[modal]==0.18.0 to ==0.20.0 across the README and harbor tests. Remove the PYTHONPATH injection in the harness eval launch: it was a no-op (uv strips PYTHONPATH from the child env) and did not fix the "No module named <agent>" failure under the dropped uid, so it and its test assertion are gone. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
The candidate is checked out at <mktemp>/repository; `mktemp -d` makes that parent 0700 root. The eval launch chowned the repository (and the staging tree) to the harness user but not the mktemp parent, so once the process dropped to harness it could not traverse into its own workspace to resolve the editable candidate package's absolute path — every eval (baseline included) failed with "No module named <agent>" while harbor itself still loaded from the harness-owned cache. Grant the harness user traversal on that parent (chmod o+x) alongside the existing chowns. o+x suffices because run_as drops to the user's own uid+gid, so it is "other" relative to the root-owned parent. The dir holds only candidate code, so widening traversal leaks no trusted data. Reproduced and fixed against the real compiled sidecar image with a faithful user+group+extra_groups privilege drop: 0700 -> "No module named gaia_agent"; 0701 -> import resolves. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Add a reachability probe: after chowning/granting traversal, run `test -r <project_path>` as the dropped user before launching harbor. Readability requires traversing every ancestor, so any provisioning gap (a missing chown, a non-traversable parent) is caught here with a clear message instead of resurfacing as a cryptic "No module named <agent>" several retries downstream — which is exactly how the checkout-parent bug hid. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Extract the harness provisioning commands and reachability probe into vero.harbor.isolation as a single source of truth, and have the eval launch use them, so the runtime and the test can't drift. Add tests/test_v05_harbor_isolation_container.py: inside a throwaway Linux container it loads the real sandbox.py + isolation.py standalone (stdlib-only, no install) and asserts, against the real uid/gid drop and the real provisioning commands, that (1) a checkout under a 0700 mktemp parent is unreachable to the dropped user when only the leaf is chowned (the regression sentinel for the shipped bug), (2) it becomes reachable after harness_grant_commands and the probe agrees, and (3) trusted root-only state stays unreadable to the dropped user. Skips when no docker daemon is available. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Agents don't know what "vero" is; every name they meet should say what it holds. The workspace context directory is now .evals/ with results/ (was evaluations/), tasks/ (was cases/), and plan.json (was evaluations.json). Host-side paths (~/.vero, ./.vero/session) are operator-facing and unchanged. Co-Authored-By: Claude Fable 5 <[email protected]>
One well-named entry point over the .evals/ context — the new avatar of the legacy VeroAgent's three structured tools. Runner subcommands (run/status/ result/submit) delegate to the sidecar client lazily; the viewers (list/show/ cases/trace/diff/plan/tasks) are unprivileged stdlib readers of the disclosure- projected tree, with bounded, paginated, metadata-first output. The compiler bakes skills/evals/SKILL.md into the optimizer workspace by default (include_evals_skill), and the instruction template now teaches `evals`. Co-Authored-By: Claude Fable 5 <[email protected]>
min_aggregate_cases moves onto the core EvaluationAccessPolicy (omitted resolves to 5 under aggregate disclosure, 1 otherwise) and the engine refuses agent-chosen aggregate subsets below the floor after cost resolution, so the protection holds on every path — programmatic API and local runs included — not just compiled harbor deployments. The sidecar keeps its earlier check as defense-in-depth and now passes the floor through its explicit authorization. AgentAccessSpec.to_access_policy() is the single typed translation from build spec to runtime policy; the compiler emits it nested in serve.json and SidecarEvaluationPolicy holds it as `access`, replacing the hand-rolled flat keys (disclosure/agent_evaluable/min_aggregate_cases/expose_case_resources) that had to be kept in sync across three representations. Co-Authored-By: Claude Fable 5 <[email protected]>
varunursekar
force-pushed
the
pr2-add-vero
branch
from
July 24, 2026 03:08
71860e0 to
3fdcf90
Compare
The sidecar's context writer predates the .evals rename and had its own literals: exposed task resources landed in .evals/cases/ (README, skill, and instruction docs all say tasks/) and plan.json was never written in the harbor topology, breaking the documented 'evals plan' first step. Caught live in the first gaia run of the new stack.
AgentContextDirectory now owns write_case_resources and write_evaluation_plan; both WorkspaceContextManager and the sidecar feed it ContextPlanEntry rows (policy, evaluation set, resolved budget), so the tree layout can no longer drift between topologies. Top-level names (results/, tasks/, candidates/, plan.json) are module constants. Behavior note: case-resource exposure now keys on expose_case_resources alone (which model validation already ties to agent_visible); the sidecar previously also required agent_can_evaluate.
…ecar The gateway now keeps a size-rotated JSONL log of every request it proxies or denies (scope, attribution, model, status, latency, tokens, and head+tail-truncated request/response bodies; SSE responses are captured from the chunks already tapped for usage). On by default in compiled tasks at /state/inference/requests with a 16KB per-body cap (inference_gateway.log_requests / request_log_body_bytes). The trusted sidecar - the only credential holder - mirrors the gateway state into the existing W&B run: a poller logs per-scope usage series (live optimizer producer-scope burn included) every 30s and ships rotated log files as artifacts, sharing the sink step counter. At finalize the gateway ledger and request log are copied into the session's artifacts so /session/export preserves every request-response, W&B or not.
The loader resolves an existing local task_source to an absolute path, while the committed partition manifest records it relative to itself; compare resolved locations before rejecting. Registry sources still compare literally.
shehabyasser-scale
approved these changes
Jul 24, 2026
Collaborator
|
Really nice split. Breaking #41 into relocate-legacy / add-system / add-benchmarks makes it reviewable again (Greptile passes each now), keeps the old stack intact under Two things I'd flagged on the earlier v0.5 snapshot are already handled here, thanks for that:
One integrity concern I'd want resolved before this is relied on for competitive selection: Infra-classified cases are excluded from the aggregate rather than scored at If a candidate-attributable failure can reach that path (a candidate that raises Could you confirm whether the routing prevents a candidate-raised exception from being classified transient-infra? If it doesn't, I think the fix is:
One more (high), in Smaller, non-blocking follow-ups:
Happy to pair on the aggregate/retry piece if that's useful. |
Addresses greptile P1s on #45. In BudgetLedger.reserve/refund a failing durable write (transient OSError) escaped at the shield-drain await and swallowed the run's CancelledError, breaking structured cancellation and leaving a reservation charged on disk. Both drains now catch the write failure and re-raise the originating cancellation, chaining the write error for diagnosis. In the engine, the EvaluationExecutionError handler and the infrastructure-failure refund now wrap record+refund in asyncio.shield like the EvaluationCancelledError handler, so a cancellation racing cleanup cannot interrupt the refund and leak budget.
Addresses the competitive-selection integrity concern on #45. The transient-infra category is detected from the candidate process's own exception type/message, and excluded cases drop out of the aggregate denominator — so a candidate emitting a timeout/connection error on a hard case could inflate its mean, with whole-sub-run retry (default 3) amplifying it under aggregate_attempts: best. For competitive (agent) evaluations a within-trial transient-infra result is now scored at the failure value (the zero-fill invariant), and whole-sub-run infrastructure retry is disabled. Trusted finalization re-scores — run by the operator, not gameable — keep the exclusion and the configured retries. Genuinely harness-produced deadness stays excluded for both: coverage gaps (no trial produced) and gateway-ledger budget/auth (terminating, out of band). Default infrastructure_max_ attempts lowered to 1.
Both specs govern the fair-play boundary but neither said so. Give each a docstring naming its role and fields: AgentAccessSpec is what the optimizer may see and spend while searching a partition, VerificationTargetSpec is how the trusted verifier finally scores the candidate that search picked. Note that min_aggregate_cases is only consulted when disclosure is AGGREGATE; under FULL the agent already sees every per-case result, so the floor is inert there whatever it is set to. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
EvaluationModel was a BaseModel with extra="forbid" and nothing else, but its name claimed an evaluation-contract scope it never had: WorkspaceOverlaySpec and WandbSpec have nothing to do with evaluation. Worse, the build and runtime layers imported it from vero.evaluation purely to inherit a config flag, which made the evaluation package a dependency of code that otherwise has no reason to touch it. Move the base to vero/models.py as StrictModel, a dependency leaf importing only pydantic, and rebase all 79 subclasses onto it. Fold in the 13 models that hand-rolled the same ConfigDict (Candidate, SessionManifest, RuntimeEvent, the session specs, and others), and drop it from WorkspaceOverlaySpec and HarborBuildConfig, which were setting it redundantly on an already-strict base. Strictness is the point and is unchanged: a typo'd key in a build YAML must fail the build rather than be dropped in silence, leaving a default that quietly widens the optimizer's access. EvaluationModel is removed rather than aliased; it had no references outside src/vero. Test results are identical to before the change. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Give the five undocumented models in the build schema the same treatment as AgentAccessSpec and VerificationTargetSpec: a summary, the context needed to use them, and an Attributes block. HarborBuildConfig is the payload, since it is the whole grammar of a benchmark's build.yaml and had a one-line docstring for 59 fields. Its header also records the two cross-field rules validate_references enforces, which were previously only discoverable by reading the validator. The Inference*Spec docstrings note two things that were only visible in the gateway: a denied model returns 403 model_denied, and the token budget is checked before a request starts, so one request can overshoot it. The InferenceGatewaySpec docstring also states plainly that the per-scope allow-list confines the target only against a non-adversarial optimizer, since that spec is where someone would otherwise assume the guarantee is airtight. Inline comments that would have duplicated an Attributes entry are folded in rather than left alongside, so the two cannot drift apart. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
A build could name a model that no gateway scope allows. Nothing caught it, so the mismatch surfaced as a 403 model_denied at run time — and for a verification target, only after search had already spent its whole budget, which is the most expensive moment to learn about a typo. Check each model that will actually be requested against the scope that will actually serve it: the task model against evaluation, and each target's scoring model (its own override, else the task model) against finalization. Verification runs the target agent too, which is why finalization has to allow the target's model and not just a judge's. This stays narrow on purpose. It only objects when a model that will genuinely be requested is unreachable, so a finalization scope that allows extra models still builds. All five current benchmarks pass unchanged. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The compiled path assumed both ends were Harbor: the outer optimizer and the inner evaluation. That left no config-driven route for a target that is not an agent — a solver, an index build, a data pipeline — where there is nothing for a nested `harbor run` to drive. ALE-Bench is that case today and handles it by bypassing the compiler entirely, hand-writing a sidecar factory that re-implements build_harbor_components with one backend swapped out. Make the inner backend a choice instead. HarborDeploymentConfig.backends is now a union discriminated on a new `type` tag, build_harbor_components dispatches on it, and HarborBuildConfig grows evaluation_backend plus a command_backend spec. The compiler emits the matching backend, bakes the scoring harness into the trusted sidecar at /opt/harness, and tells it where the case files are; a command backend's case ids name whatever its harness understands, so they are no longer resolved as local Harbor task directories. Everything above the backend seam is untouched. Disclosure, budgets, the gateway, and verification are backend-agnostic already, which is the point: a task on this route gets the metered gateway and the build-time validators that a hand-written factory does not. Compatibility is preserved in both directions. The tag defaults to "harbor", a serve.json written before the union still loads, and all five benchmarks parse unchanged. agent_import_path becomes optional and is required for a harbor backend; Harbor-only fields set on a command build are reported rather than silently ignored. Nesting each backend's fields under its own sub-spec would make that structural rather than validated, but it would rewrite every benchmark's build.yaml, so it is left as a TODO. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
A command build had to name a task_source it never used, so the only way to satisfy the schema was to point it at an empty directory. Nothing on that path reads it: it names Harbor task definitions, and a command backend has none. Make it optional and require it for a harbor backend alongside agent_import_path, reporting both together when either is missing. Setting it on a command build is now rejected as Harbor-only, which also means a case id is resolved as a local Harbor task directory exactly when a task_source names one. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
CommandBackendSpec documented harness_source as resolved relative to the build YAML, matching agent_repo, task_source, task_manifest, and overlay sources, but the loader never resolved it. A relative path was therefore interpreted against the working directory, so the existence check failed from anywhere except the YAML's own directory. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The TODO said what to do and why to defer, but not what it would cost, so the question would be re-litigated from scratch. Note the measurement and the migration trick that avoids a breaking change. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Every container path in a compiled task was written down two or more times: as a Python constant in the compiler and again as a literal in the Jinja templates. /work/agent appeared 19 times across 10 files. Changing a constant therefore did not propagate to the templates, and three paths (/opt/seed.sh, /opt/inference.json, /opt/overlay) had no constant at all. I had already created one instance of the hazard: HARNESS_DIR in the compiler and /opt/harness hardcoded in the sidecar Dockerfile. Add vero.harbor.layout.TaskLayout, a frozen dataclass holding every container path, service name, and port, with child paths derived as properties so a base and its children cannot drift. Every template now receives it, so no template spells out a path of its own. It lives outside build/ because harbor.inference also needs two of these as field defaults. Path values are unchanged and deliberately so: they are a contract with the benchmarks' read_only_paths and with compiled task directories that are checked in. Renaming an attribute is now cheap; repointing one is still breaking. Measured: literal path occurrences outside the definition drop from 93 across 13 files to 0, with the only remaining copies being layout.py itself and one pinning test. That test exists because every other test now references the layout, which would otherwise make them tautological — it is the single place the values are written twice on purpose. Verified by rendering four tasks covering every template branch (gateway on/off, overlay, wandb, budget-blind, harness isolation off, harbor and command backends) before and after: the generated output is byte-identical. Jinja runs with StrictUndefined, so a mistyped layout attribute fails the build rather than rendering an empty path. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The compose template named the factory as a string literal, duplicating a value that also exists in Python — the last instance of exactly what layout.py was created to remove, and the worst-behaved one. A stale dotted path here fails when the sidecar container starts, so neither the type checker nor the suite would catch a rename of the module or the function. Derive FACTORY_PATH in deployment.py from the function object itself and render it into the template. This does not belong in layout.py: that module is dependency-free by design, and a hand-maintained literal there would reproduce the same problem one level over, when the referent is an object we can introspect. The compiler imports it inside compile_harbor_task rather than at module scope. A top-level import does work today, but only because importing a submodule skips the parent package's partially-run __init__, which imports this package before deployment — an ordering accident, not a property anyone would notice breaking. It would also pull the whole runtime stack into `vero harbor build`. The test asserts on the rendered compose file rather than on the constant, so it still holds if a literal is put back, and it imports the path to prove it resolves. Verified both ways: renaming the function needs no template edit and stays green, and a stale literal fails. Left alone: agent_import_path in each build.yaml is the same species of string, but it points into the target repo, whose dependencies are not installed on the build host, so it genuinely cannot be resolved at compile time. Dotted paths pointing inside vero get derived and resolution-tested; ones pointing outside cannot be. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The proxy route was declared in inference.py and then independently rebuilt by string concatenation in four places that knew nothing about it: launch.json, the compose file's OPENAI_BASE_URL, the seed script, and the trusted backend. Five copies of three conventions — the scope segment, the attribution segment, and /v1. Change the route and four sites keep producing URLs the gateway 404s or 403s on, inside a container, at run time. Put the route shape in the layout as one string and derive everything from it: scope_route for the gateway to register, scope_url for callers using this layout's gateway, and scope_path for the trusted backend, which reads its base URL from the deployment config rather than assuming ours. The optimizer's attribution segment was duplicated three times and is now a field too. The templates receive a precomputed producer_base_url instead of concatenating. The test asserts on the compiled artifacts — compose, launch.json, and the seed script — rather than on the constant, so it still holds if a template goes back to building the URL by hand. Verified by doing exactly that with a wrong shape: it fails. Also folded in, both hygiene rather than risk: VERO_EVAL_URL was a producer/consumer pair split between the compose template and the CLI that reads it, and the gateway's upstream env var names were the last constants still local to the compiler. The OPENAI_* scrub set is now a named constant with the invariant written down. Worth being precise about what that invariant is, since it looks alarming: the blanking loop is default-deny, so a new credential added to `secrets` is blanked for the optimizer automatically. The exclusion list is the explicit-allow exception for the two names the template sets deliberately, and forgetting one emits a duplicate compose key rather than leaking anything. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
task.toml.j2 hardcoded build_timeout_sec = 1800 while the timeout_sec two lines above it was already parametrized, so one timeout in the same generated file was tunable and the other was not. The outer build is not trivial — for a source build it copies the vero tree, installs it plus harbor into the sidecar, syncs the baseline, and pre-warms a uv cache — and there was no way to raise the limit without editing the template. Typed as an int to match the neighbouring verifier timeout, so the default still renders as 1800 and compiled output is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
harbor/ held fourteen flat modules that are not peers: they run in three different processes on either side of a trust boundary, and nothing in the layout said so. Most of them were not Harbor code at all. The sidecar stack -- sidecar, app, serve, verifier, transport, auth, isolation, session -- imports only itself, never backend, build, deployment or cli, and inference.py imports only layout. deployment.py is the single module binding the two, which is exactly its job. The nested-backend work sharpened the point: a compiled task can now use a command backend and no HarborBackend at all, while every module implementing it still lived under harbor/. Split those out to vero/sidecar/ and vero/gateway/, hoist layout.py to the top level so gateway/ needs no backwards edge, and leave harbor/ holding what is genuinely Harbor: build/, backend, generation, deployment, cli. Regroup evaluation/ along its own dependency layers into backends/, scoring/ and store/. A reviewer can now read the trust architecture off the tree. Package docstrings carry what the tree cannot: that nothing in sidecar/ is Harbor-specific, and that its co-location is load-bearing -- HTTP carries the API, but candidate code, the .evals context and the admin token all move through volumes shared with main, so it is not deployable away from the task it serves. That is also why it keeps the name. evaluation/ is an internal regroup: every name is still re-exported from vero.evaluation, so only deep importers moved. harbor/ -> sidecar//gateway/ is a real move, so those exports were relocated rather than aliased back; leaving compatibility shims in harbor/__init__ would preserve the false impression this commit exists to remove. Pure moves and import rewrites. No logic changes: 399 passed / 9 skipped before and after. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
circle_factory built SidecarEvaluationPolicy with flat disclosure and expose_case_resources keyword arguments, but those moved under a nested access=EvaluationAccessPolicy some time ago. The sidecar container therefore died at startup with three validation errors before serving anything. Predates the package restructure -- the same call fails identically on pr2-add-vero -- and no test covers it, because nothing exercises the example factories. Found by actually booting the container: it now starts and serves /health and /status, reporting full disclosure on development, aggregate on validation, and the budget ledger. ale_factory.py in harness-engineering-bench has the same stale construction and is broken the same way. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The example only had the hand-written path: circle_factory.py wiring a CommandBackend directly, with the agent handed raw OPENAI_* credentials. That is the same shape as ale-bench, and it means the one worked example of a non-agent target is also an example of an unmetered optimizer. The nested-backend refactor makes the compiled path available to such a target, so add a build.yaml beside the hand-written task: same packing.py, same evaluate.py, but `evaluation_backend: command` and an inference gateway. The optimizer now reaches its model only through a scoped, allow-listed, metered token; the raw upstream stays in the gateway container and is scrubbed to "" in main. Two wirings of one problem, directly comparable. Verified end to end with codex/gpt-5.4: reward 2.6324 against a 0.9598 baseline and a ~2.635 known optimum, 0 exceptions in 20m46s. The gateway metered every optimizer call -- 57 requests, 3.6M tokens, no denials -- and the evaluation scope stayed at zero, since a command harness makes no model calls of its own. passthrough_environment is set and commented because it is easy to get wrong: CommandBackend deliberately runs the harness with PATH=os.defpath, so python (in /usr/local/bin on the base image) is not found unless PATH is forwarded. The failure surfaces only when the harness first runs, after the agent has already spent its budget. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
test_v05_harbor_build.py carried two tests that read build YAMLs out of harness-engineering-bench, guarded by a skipif on that directory existing. It does not exist on this branch, so they always skipped here and only ran one branch up -- where they had been failing for a while against stale expectations nobody saw. A test that can silently skip on the branch that owns it is a test that rots. Drop them and the BENCHMARK_ROOT/_requires_benchmarks harness; they move to pr3-harness-bench, where the benchmarks are always present and the guard can go away entirely. Suite is unchanged at 399 passed, with skips down from 9 to 5. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
report.py was the largest module in the codebase at 43K, but 57% of that was one raw string: a complete 25K HTML document with its own CSS and JavaScript, pasted into _REPORT_HTML. Inside a string literal it gets no highlighting, no formatter, no linter, and no diff worth reading -- and it made the module look like a 43K Python problem when the Python is 19K. Extract it verbatim to templates/report.html, loaded through importlib.resources the way the packaged evals skill already is. Non-Python artifacts get their own directory rather than sitting beside the modules, matching harbor/build/templates and skills/evals. Substitution is unchanged: still one replace of __VERO_REPORT_DATA__. The extracted text is byte-identical to the old literal, so generated reports are unchanged by construction rather than by inspection. The wheel ships the file alongside the existing Jinja templates and skills/evals/SKILL.md, checked by building one. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The main container blanks every declared secret except the two the compose template assigns itself -- the optimizer's scoped token and its gateway URL. GATEWAY_ROUTED_CREDENTIALS named that exemption in Python while the template spelled the same names out again, five copies of OPENAI_API_KEY and four of OPENAI_BASE_URL between them. The duplication is one-directional and quiet. Blanking is default-deny, so a newly declared secret is scrubbed automatically and adding one is safe. The failure is the other way: a name added to the exemption but not assigned by the template would be neither blanked nor set, leaving whatever the host exported in the optimizer's container -- the one outcome the scrub exists to prevent. Move both names onto LAYOUT beside the other env-name fields, derive the exemption from LAYOUT.routed_credential_envs, and have the compose file and codex's provider config reference them. Nine literals become one definition. Add a test that renders a compiled task and asserts every exempt name is positively set with a scoped value, and that the raw upstream is blanked in the same container. Verified it fails as intended by adding a third name to the exemption: "STRAY_KEY is skipped by the scrub but never set". Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
config.py held three unrelated things in 953 lines: the leaf models a
build.yaml composes, HarborBuildConfig itself, and the ${NAME:-default}
substitution language plus the YAML reader. Split into specs.py (313),
config.py (635), and loader.py (155). Only two places imported deeply;
build/__init__.py re-exports the same eleven names.
validate_references was 136 lines running six unrelated checks, including
45 lines of task_manifest JSON parsing. It is now six named validators in
the original order, so error precedence is unchanged and a rejected build
points at the rule it broke. The manifest check reads and parses the file
once instead of twice.
The fields are grouped into six private base classes, each with its own
docstring and the validators that concern only itself. Grouping by
inheritance rather than nesting keeps the wire format flat, so every
checked-in build.yaml parses identically -- verified by loading all five
benchmark configs and by comparing the old and new field sets (62 each,
same annotations and defaults).
That retires a duplicate list: the Harbor-only fields were enumerated once
as declarations and once in a hand-maintained frozenset, and
_HARBOR_ONLY_FIELDS is now derived from _HarborEvaluationFields. The two
had already drifted -- reward_key is Harbor-only in fact, since a command
harness reports its own reward, but was missing from the list, so setting
it on a command build passed validation and did nothing. It is now in the
group and rejected.
Two tests. One sweeps the derived set, so a field added to the group is
covered the day it lands; the other asserts the groups stay flat on the
wire and that every field belongs to one.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The package restructure moved the sidecar stack, the gateway, and the evaluation store without updating this doc, leaving fourteen paths that resolve to nothing: harbor/sidecar.py, harbor/verifier.py, harbor/session.py, harbor/app.py, harbor/inference.py, and evaluation/budget.py. Since the doc's whole purpose is a suggested reading order, a dead path is worse here than in a code comment. Also names the four modules the build pipeline now passes through. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Each of reserve's charge write, reserve's rollback write, and refund's write hand-rolled the same shield-and-drain loop, and each got the same detail wrong at least once: a write failure replacing the CancelledError of the run being torn down. Two were patched individually in review; the third, reserve's charge write, still called write.result() unguarded, so an OSError there discarded the cancellation and the caller saw the wrong exception. _drain_write now owns the loop and returns both outcomes rather than raising one, which is what makes the invariant checkable: no call site can propagate a write error in place of a cancellation, because it has to decide about both. Draining to completion is preserved -- an abandoned write could overwrite a later one once the ledger lock is released. Behaviour is unchanged except in the case that was broken. Reserve still rolls its charge back on cancellation, and now skips the rollback when the charge never landed. Refund still commits, since returning budget is safe even for a run that is going away; that asymmetry is now stated rather than implied. The new test fails on the old code with OSError: disk gone. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
varunursekar
added a commit
that referenced
this pull request
Jul 26, 2026
Six findings from the reviews on #45/#46 that survived checking against the current tree. Each was verified against the live litellm endpoint the gateway proxies to, not inferred from a commit message. gaia moves to gpt-5.4-mini. It is the one multimodal benchmark and deepseek-v4-flash is text-only: 5 of the 66 held-out tasks send images and get 400 "This model does not support image inputs", which caps achievable reward near 0.92 and reads as ordinary agent failure. gpt-5.4-mini returns 200 for every request shape the gaia agent sends -- image input, hosted web_search, reasoning.effort, parallel_tool_calls -- confirmed through the SDK path the agent actually uses. The name is unprefixed on purpose: every agent sends model_name.removeprefix("openai/"), so an openai/-prefixed name would be allow-listed in one form and requested in another and the gateway would deny it. The gaia held-out baseline now needs re-measuring. tau3 executes every tool call the model returns instead of only the first. deepseek-v4-flash does return two in a turn, and the usual guard is not available: litellm rejects parallel_tool_calls for fireworks_ai with UnsupportedParamsError, so the reviewer's suggested one-liner would have 400'd every tau3 call. Dropping calls[1:] silently skipped actions -- for a customer-service agent, a verified identity with no message sent. officeqa and swe-atlas-qna run their tool dispatch inside the try rather than an else, and catch KeyError/TypeError/ValueError alongside JSONDecodeError, matching browsecomp-plus. Neither tool schema is strict, so the model can return valid JSON missing a required key; that ended the trial instead of feeding the model an error. Widening the except alone would have caught nothing, since the lookups were in the else. gaia's client gets max_retries=8, the only one of the five without it. A within-trial transient failure scores at the failure value for competitive evaluations, so an unretried 429 cost a candidate a 0.0. tau3 quotes the MCP session id before interpolating it into a shell command, for the same reason the payload beside it is base64-encoded. per_trial_tokens buckets unattributed records under a name rather than None, which crashed sorted() as soon as one appeared beside an attributed record. Not actioned: the browsecomp-plus grader was reported as vulnerable to format-string injection from curly braces in the agent's response. str.format does not rescan substituted values, so there is nothing to fix. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
_execute_record shields _record and refund in both exception handlers but left the success-path _record bare, so a cancellation arriving after the evaluation finished could abandon it part-way -- the budget charged for a run the engine never finished publishing. The mechanism is narrower than "the database write is lost": that write is an asyncio.to_thread, and a cancelled await does not stop the thread, so the file lands either way. What is actually skipped is everything after it -- the listener loop, where the trusted side mirrors an evaluation to W&B and the session archive -- plus the whole of _record when the cancellation arrives before its lock is acquired. Shield does not wait for the shielded coroutine, only protect it: the caller still unwinds at once while _record completes in the background. That is already the contract of the four shielded calls beside it, so this makes the path consistent rather than introducing a new guarantee. The test cancels during a listener, the one genuinely interruptible step, and times out on the unshielded code. Reported by greptile on #45. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
varunursekar
added a commit
that referenced
this pull request
Jul 26, 2026
Six findings from the reviews on #45/#46 that survived checking against the current tree. Each was verified against the live litellm endpoint the gateway proxies to, not inferred from a commit message. gaia moves to gpt-5.4-mini. It is the one multimodal benchmark and deepseek-v4-flash is text-only: 5 of the 66 held-out tasks send images and get 400 "This model does not support image inputs", which caps achievable reward near 0.92 and reads as ordinary agent failure. gpt-5.4-mini returns 200 for every request shape the gaia agent sends -- image input, hosted web_search, reasoning.effort, parallel_tool_calls -- confirmed through the SDK path the agent actually uses. The name is unprefixed on purpose: every agent sends model_name.removeprefix("openai/"), so an openai/-prefixed name would be allow-listed in one form and requested in another and the gateway would deny it. The gaia held-out baseline now needs re-measuring. tau3 executes every tool call the model returns instead of only the first. deepseek-v4-flash does return two in a turn, and the usual guard is not available: litellm rejects parallel_tool_calls for fireworks_ai with UnsupportedParamsError, so the reviewer's suggested one-liner would have 400'd every tau3 call. Dropping calls[1:] silently skipped actions -- for a customer-service agent, a verified identity with no message sent. officeqa and swe-atlas-qna run their tool dispatch inside the try rather than an else, and catch KeyError/TypeError/ValueError alongside JSONDecodeError, matching browsecomp-plus. Neither tool schema is strict, so the model can return valid JSON missing a required key; that ended the trial instead of feeding the model an error. Widening the except alone would have caught nothing, since the lookups were in the else. gaia's client gets max_retries=8, the only one of the five without it. A within-trial transient failure scores at the failure value for competitive evaluations, so an unretried 429 cost a candidate a 0.0. tau3 quotes the MCP session id before interpolating it into a shell command, for the same reason the payload beside it is base64-encoded. per_trial_tokens buckets unattributed records under a name rather than None, which crashed sorted() as soon as one appeared beside an attributed record. Not actioned: the browsecomp-plus grader was reported as vulnerable to format-string injection from curly braces in the agent's response. str.format does not rescan substituted values, so there is nothing to fix. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
A held-out target can now set n_attempts/aggregate_attempts to score each case several times and combine them (e.g. 3x mean to average a noisy final eval), while search and selection keep the global attempts. Backends are per-partition, so the override is injected into the target partition's backend at compile time; a config validator rejects overriding an agent-evaluable partition (shared with search) or a command backend. Co-Authored-By: Claude Opus 4.8 <[email protected]>
…uction The instruction's insights paragraph was gated on overlay_present, which is true whenever the always-baked evals skill is added -- so every optimizer was told to dispatch an insights-generator subagent and read skills/insights that no benchmark ships. The insights skill is retired; drop the paragraph. Co-Authored-By: Claude Opus 4.8 <[email protected]>
varunursekar
added a commit
that referenced
this pull request
Jul 27, 2026
Six findings from the reviews on #45/#46 that survived checking against the current tree. Each was verified against the live litellm endpoint the gateway proxies to, not inferred from a commit message. gaia moves to gpt-5.4-mini. It is the one multimodal benchmark and deepseek-v4-flash is text-only: 5 of the 66 held-out tasks send images and get 400 "This model does not support image inputs", which caps achievable reward near 0.92 and reads as ordinary agent failure. gpt-5.4-mini returns 200 for every request shape the gaia agent sends -- image input, hosted web_search, reasoning.effort, parallel_tool_calls -- confirmed through the SDK path the agent actually uses. The name is unprefixed on purpose: every agent sends model_name.removeprefix("openai/"), so an openai/-prefixed name would be allow-listed in one form and requested in another and the gateway would deny it. The gaia held-out baseline now needs re-measuring. tau3 executes every tool call the model returns instead of only the first. deepseek-v4-flash does return two in a turn, and the usual guard is not available: litellm rejects parallel_tool_calls for fireworks_ai with UnsupportedParamsError, so the reviewer's suggested one-liner would have 400'd every tau3 call. Dropping calls[1:] silently skipped actions -- for a customer-service agent, a verified identity with no message sent. officeqa and swe-atlas-qna run their tool dispatch inside the try rather than an else, and catch KeyError/TypeError/ValueError alongside JSONDecodeError, matching browsecomp-plus. Neither tool schema is strict, so the model can return valid JSON missing a required key; that ended the trial instead of feeding the model an error. Widening the except alone would have caught nothing, since the lookups were in the else. gaia's client gets max_retries=8, the only one of the five without it. A within-trial transient failure scores at the failure value for competitive evaluations, so an unretried 429 cost a candidate a 0.0. tau3 quotes the MCP session id before interpolating it into a shell command, for the same reason the payload beside it is base64-encoded. per_trial_tokens buckets unattributed records under a name rather than None, which crashed sorted() as soon as one appeared beside an attributed record. Not actioned: the browsecomp-plus grader was reported as vulnerable to format-string injection from curly braces in the agent's response. str.format does not rescan substituted values, so there is nothing to fix. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second of three stacked PRs splitting #41. Base:
pr1-legacy-relocate.Adds the v0.5 system at the repo root:
vero/(core + harbor eval sidecar, build compiler, runtime/observability),vero-tasks/,runs/. Root README points tolegacy/. Pure adds (197 files) + README/.gitignore.Review after 1/3. 3/3 adds
harness-engineering-bench/on top.🤖 Generated with Claude Code
Greptile Summary
This PR introduces the VeRO v0.5 system as a pure-add set of 197 files under
vero/,vero-tasks/, andruns/. It ships the complete evaluation engine, budget ledger, Harbor eval sidecar, inference gateway, build compiler, optimizer, W&B observability sinks, and avero-taskslibrary.EvaluationCancelledError,EvaluationExecutionError, infrastructure failure) now correctly wrap both_recordandrefundinasyncio.shield; a new_drain_writehelper centralises the cancellation-safe write-drain loop that was previously hand-rolled inconsistently acrossreserveandrefund.secrets.compare_digestwith atomic file writes (fchmod+fsync+os.replace);_safe_extract_taradoptsfilter="data"(matchingextract_harbor_session_archive) to strip device files and setuid bits; secret sanitisation is applied longest-first to prevent partial-match leakage.validate_extra_argsinHarborBackendConfighas an unreachable duplicatereturn valuefollowing the real return, likely a merge/edit artifact.Confidence Score: 5/5
Safe to merge; the only finding is a dead unreachable
return valuethat has no effect on runtime behaviour.This is a large pure-add PR. The most security-sensitive paths (admin-token auth, tar extraction, budget atomicity, cancellation shielding) are all handled correctly. The sole finding is an unreachable duplicate return statement in a Pydantic validator — a cosmetic artifact with no runtime impact.
Files Needing Attention: vero/src/vero/harbor/backend.py — contains the dead-code duplicate return in
validate_extra_args.Important Files Changed
return valueinvalidate_extra_args(dead code, no runtime impact). All other validation, secret-sanitization, and isolation logic looks correct._recordandrefundfrom mid-flight cancellation, addressing the prior review's concerns._drain_writehelper that correctly pairs forward write errors with savedCancelledErrors; the asymmetry betweenreserverollback andrefundforward-write is now resolved._safe_extract_tarnow usesfilter="data"to strip device files and setuid bits, aligning withextract_harbor_session_archive; path-traversal and unsafe-link validation also present.fchmod+fsync+os.replace, constant-time comparison viasecrets.compare_digest, and restricted directory mode to block unprivileged agent traversal.asyncio.Lock; baseline-floor logic correctly refuses to ship the seed unverified when infrastructure fails.self.max_proposals < 0guard now correctly precedes theproposal_limitcheck, fixing the previously unreachable negative-limit message.SidecarWandbSinkcorrectly persistsrun_idvia UUID for restart-safe resumption, whileWandbEventSinkderives a stable ID fromsession_id.BackgroundTaskafter the response stream completes.Sequence Diagram
sequenceDiagram participant Agent participant Sidecar as EvaluationSidecar (app.py) participant Engine as EvaluationEngine participant Budget as BudgetLedger participant Backend as HarborBackend participant Verifier as CanonicalVerifier Agent->>Sidecar: POST /eval Sidecar->>Engine: evaluate(request, AGENT) Engine->>Budget: reserve(backend_id, set, cost) Budget-->>Engine: budget snapshot Engine->>Backend: evaluate(candidate) Backend-->>Engine: EvaluationRecord Engine->>Engine: asyncio.shield(_record(record)) Engine-->>Sidecar: projected disclosure Sidecar-->>Agent: EvaluationReceipt Note over Sidecar,Verifier: Finalization (admin-token gated) Verifier->>Engine: quiesce_agent_evaluations() Engine->>Engine: cancel remaining agent tasks Verifier->>Verifier: _select_candidate() Verifier->>Engine: evaluate_record(candidate, ADMIN) Engine->>Backend: evaluate(selected candidate) Backend-->>Engine: VerificationResult Verifier->>Verifier: _atomic_write_json(finalize.json)Reviews (24): Last reviewed commit: "Stop advertising the retired insights su..." | Re-trigger Greptile