Skip to content

[Harbor 3/4] Mode B (nested harbor run) + the vero harbor build compiler#5

Open
varunursekar wants to merge 1 commit into
harbor-2-sidecarfrom
harbor-3-compiler
Open

[Harbor 3/4] Mode B (nested harbor run) + the vero harbor build compiler#5
varunursekar wants to merge 1 commit into
harbor-2-sidecarfrom
harbor-3-compiler

Conversation

@varunursekar

@varunursekar varunursekar commented Jun 24, 2026

Copy link
Copy Markdown

Draft · Stack 3 of 4 — targets harbor-2-sidecar.

  • Mode B (runner.py): HarborRunner, an EvalStrategy that — per candidate — runs a nested harbor run of the agent over the selected Harbor tasks (e.g. on Modal) and collates the verifier rewards into vero SampleResults. One Harbor task = one sample; inference is delegated, scoring comes from Harbor.
  • The compiler (build/): vero harbor build renders a BuildConfig into a runnable Harbor task — a Docker Compose env (optimizer workbench + eval sidecar + 3 volumes), two Dockerfiles, instruction.md, tests/test.sh, seed/solve scripts — baking the dataset/scorer/baseline repo and the sidecar's ServeConfig. Mode A and Mode B.

Review focus: the nested-run + collation, and the trust-boundary plumbing baked into the generated compose (volumes, root:600 token, secrets → sidecar only). Also un-ignores src/vero/harbor/build/ (the repo's build/ rule was hiding the package).

Stack: [1/4] core → [2/4] sidecar → this → [4/4] docs.

🤖 Generated with Claude Code

Greptile Summary

This PR adds Mode B nested-harbor-run evaluation (HarborRunner) and the vero harbor build compiler that renders a BuildConfig into a self-contained Harbor task directory (Docker Compose workbench + eval sidecar, Dockerfiles, scripts, and baked ServeConfig). The trust-boundary design is sound — secrets are sidecar-only, the admin token is root:600, and the optimizer runs as a de-privileged agent user.

  • runner.py: HarborRunner.produce_sample_results shells out to a nested harbor run, then collates per-trial result.json files into SampleResults; resume logic skips already-persisted samples.
  • compiler.py: compile_task materialises the baseline repo via git archive, registers the dataset into a baked VERO_HOME, emits a ServeConfig, and Jinja2-renders all environment files in one pass.
  • config.py: BuildConfig cleanly separates Mode A (dataset + scorer baked in) from Mode B (partition + inner Harbor task baked sidecar-only).

Confidence Score: 3/5

The trust-boundary plumbing (secrets sidecar-only, admin token root:600, agent de-privileged) is correctly wired. Three bugs need fixing before this should run in production: two in the runner and one in the compiler.

The runner silently ignores n_attempts/max_retries from HarborConfig, so retry behaviour is always the harbor default regardless of what the caller configures — directly affecting budget usage on live runs. _load_trials overwrites earlier results for the same task_name in filesystem-iteration order, meaning a successful retry can be replaced by a failed one, producing incorrect scores. In the compiler, git archive exit code is discarded: a failed archive populates an empty baseline directory whose SHA is recorded in the ServeConfig and baked into the image without any error signal.

Both runner.py (retry flag forwarding and collation deduplication) and compiler.py (git archive error handling and mkdtemp cleanup) need attention before this ships.

Important Files Changed

Filename Overview
vero/src/vero/harbor/runner.py New HarborRunner EvalStrategy for Mode B: silently drops n_attempts/max_retries from harbor run, and _load_trials has non-deterministic last-write-wins when harbor retries produce multiple result.json files for the same task_name.
vero/src/vero/harbor/build/compiler.py New Harbor build compiler: git archive exit code is silently ignored in _prepare_baseline_repo (corrupt baseline possible), and mkdtemp() scratch dir is never deleted.
vero/src/vero/harbor/build/config.py BuildConfig Pydantic model cleanly captures Mode A/B differences; relative-path resolution in from_file() is correct and portable.
vero/src/vero/harbor/build/templates/docker-compose.yaml.j2 Compose template correctly isolates secrets to eval-sidecar only; admin/token volumes never mounted to main; healthcheck properly gates depends_on.
vero/tests/test_harbor_build.py Comprehensive build compiler tests covering structure, ServeConfig validation, file parsing, path rewriting, and shared baseline SHA.
vero/tests/test_harbor_runner.py Good coverage for command building, reward extraction, collation, and resume logic; correctly tests that missing trials produce error samples.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant V as vero (HarborRunner)
    participant H as harbor run (nested)
    participant S as eval-sidecar (vero harbor serve)
    participant FS as jobs_dir (filesystem)

    V->>V: "_task_names_for(params) -> [(sample_id, task_name)]"
    V->>V: filter pending (skip existing SampleResults)
    V->>H: uv run --project worktree harbor run -i task0 -i task1 --jobs-dir ...
    H->>S: POST /eval (agent commit)
    S-->>H: reward JSON
    H->>FS: write jobs/ts/trial/result.json
    H-->>V: exit code (non-zero tolerated)
    V->>FS: "rglob result.json -> task_name to result_dict"
    V->>V: _sample_result() per (sample_id, task_name)
    V->>V: "save_sample_result() -> VERO_HOME/sessions/"
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant V as vero (HarborRunner)
    participant H as harbor run (nested)
    participant S as eval-sidecar (vero harbor serve)
    participant FS as jobs_dir (filesystem)

    V->>V: "_task_names_for(params) -> [(sample_id, task_name)]"
    V->>V: filter pending (skip existing SampleResults)
    V->>H: uv run --project worktree harbor run -i task0 -i task1 --jobs-dir ...
    H->>S: POST /eval (agent commit)
    S-->>H: reward JSON
    H->>FS: write jobs/ts/trial/result.json
    H-->>V: exit code (non-zero tolerated)
    V->>FS: "rglob result.json -> task_name to result_dict"
    V->>V: _sample_result() per (sample_id, task_name)
    V->>V: "save_sample_result() -> VERO_HOME/sessions/"
Loading

Comments Outside Diff (2)

  1. vero/src/vero/harbor/runner.py, line 739-752 (link)

    P1 n_attempts and max_retries silently dropped from harbor run

    HarborConfig exposes n_attempts and max_retries as typed fields (not extra_args pass-throughs), signalling that callers expect to configure retry behavior. Neither field is translated to a CLI flag in _build_command, so every harbor run invocation uses whatever the harbor defaults are regardless of what the caller configured. A user who sets max_retries=0 to disable retries on a budget-sensitive run will get the default retry behavior and may blow their budget.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: vero/src/vero/harbor/runner.py
    Line: 739-752
    
    Comment:
    **`n_attempts` and `max_retries` silently dropped from harbor run**
    
    `HarborConfig` exposes `n_attempts` and `max_retries` as typed fields (not `extra_args` pass-throughs), signalling that callers expect to configure retry behavior. Neither field is translated to a CLI flag in `_build_command`, so every `harbor run` invocation uses whatever the harbor defaults are regardless of what the caller configured. A user who sets `max_retries=0` to disable retries on a budget-sensitive run will get the default retry behavior and may blow their budget.
    
    How can I resolve this? If you propose a fix, please make it concise.

    Fix in Cursor Fix in Claude Code Fix in Codex

  2. vero/src/vero/harbor/runner.py, line 799-813 (link)

    P1 Last-write-wins on duplicate task_name when harbor retries a task

    rglob("result.json") visits all result files across all timestamp directories under jobs_dir. If harbor's n_attempts or max_retries causes the same task to produce multiple trial entries with the same task_name, trials[task_name] = data simply overwrites with whichever file rglob happens to yield last — filesystem iteration order is not defined. This can cause a passing retry to be silently replaced by a failing one (or vice versa), yielding an incorrect score for that sample.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: vero/src/vero/harbor/runner.py
    Line: 799-813
    
    Comment:
    **Last-write-wins on duplicate `task_name` when harbor retries a task**
    
    `rglob("result.json")` visits all result files across all timestamp directories under `jobs_dir`. If harbor's `n_attempts` or `max_retries` causes the same task to produce multiple trial entries with the same `task_name`, `trials[task_name] = data` simply overwrites with whichever file `rglob` happens to yield last — filesystem iteration order is not defined. This can cause a passing retry to be silently replaced by a failing one (or vice versa), yielding an incorrect score for that sample.
    
    How can I resolve this? If you propose a fix, please make it concise.

    Fix in Cursor Fix in Claude Code Fix in Codex

Fix All in Cursor Fix All in Claude Code Fix All in Codex

Prompt To Fix All With AI
Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 4
vero/src/vero/harbor/runner.py:739-752
**`n_attempts` and `max_retries` silently dropped from harbor run**

`HarborConfig` exposes `n_attempts` and `max_retries` as typed fields (not `extra_args` pass-throughs), signalling that callers expect to configure retry behavior. Neither field is translated to a CLI flag in `_build_command`, so every `harbor run` invocation uses whatever the harbor defaults are regardless of what the caller configured. A user who sets `max_retries=0` to disable retries on a budget-sensitive run will get the default retry behavior and may blow their budget.

### Issue 2 of 4
vero/src/vero/harbor/build/compiler.py:120-130
**`git archive` exit code is silently discarded**

`archive.wait()` is called but its return value is not checked. If `git archive` fails (e.g., the `rel` subpath doesn't exist at `HEAD`, or the ref is invalid), `tar` receives an empty or truncated stream. Depending on the tar implementation, it may exit 0 with partial content, causing `_prepare_baseline_repo` to return a commit SHA from a near-empty directory and bake that corrupt baseline into both containers. Additionally, if `subprocess.run(tar, ..., check=True)` raises before reaching `archive.wait()`, the `git archive` subprocess is never reaped until GC collects the `Popen` object.

### Issue 3 of 4
vero/src/vero/harbor/runner.py:799-813
**Last-write-wins on duplicate `task_name` when harbor retries a task**

`rglob("result.json")` visits all result files across all timestamp directories under `jobs_dir`. If harbor's `n_attempts` or `max_retries` causes the same task to produce multiple trial entries with the same `task_name`, `trials[task_name] = data` simply overwrites with whichever file `rglob` happens to yield last — filesystem iteration order is not defined. This can cause a passing retry to be silently replaced by a failing one (or vice versa), yielding an incorrect score for that sample.

### Issue 4 of 4
vero/src/vero/harbor/build/compiler.py:237-239
**Temp directory created by `mkdtemp()` is never cleaned up**

The directory created by `tempfile.mkdtemp()` on line 239 is used only to stage the dataset before registration, but is never deleted — not on success and not on exception. Over repeated builds this accumulates large scratch directories (the dataset can be gigabytes). Wrapping `tmp` creation with `tempfile.TemporaryDirectory()` as a context manager would clean it up automatically.

Reviews (1): Last reviewed commit: "Harbor: Mode B (nested harbor run) + the..." | Re-trigger Greptile

Greptile also left 2 inline comments on this PR.

- Mode B (runner.py): `HarborRunner`, an `EvalStrategy` that — for each candidate —
  runs a *nested* `harbor run` of the agent over the selected Harbor tasks (e.g. on
  Modal) and collates the verifier rewards into vero `SampleResult`s. One Harbor task
  = one sample; inference is fully delegated, scoring comes from Harbor's verifier.
- The compiler (build/): `vero harbor build` renders a `BuildConfig` into a runnable
  Harbor task directory — a Docker Compose environment (optimizer workbench `main` +
  the eval sidecar + three volumes), two Dockerfiles, instruction.md, tests/test.sh,
  and the seed/solve scripts — baking the dataset/scorer/baseline repo and the
  sidecar's ServeConfig. Supports Mode A (local dataset/scorer) and Mode B (a registry
  or local Harbor benchmark, passed through to the HarborConfig).
- `.gitignore`: un-ignore src/vero/harbor/build/ (the repo's `build/` rule was hiding
  the compiler package).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@varunursekar varunursekar requested a review from a team June 24, 2026 18:15
@varunursekar varunursekar marked this pull request as ready for review June 24, 2026 18:22
Comment on lines +120 to +130

sessions = vero_home / "sessions"
datasets = vero_home / "datasets"
(sessions / SESSION_ID).mkdir(parents=True, exist_ok=True)
datasets.mkdir(parents=True, exist_ok=True)
if not isinstance(dataset, str): # a DatasetDict -> save_to_disk first
path = tmp / "ds"
dataset.save_to_disk(str(path))
dataset = str(path)
return resolve_and_save_dataset(dataset, sessions, datasets, SESSION_ID)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 git archive exit code is silently discarded

archive.wait() is called but its return value is not checked. If git archive fails (e.g., the rel subpath doesn't exist at HEAD, or the ref is invalid), tar receives an empty or truncated stream. Depending on the tar implementation, it may exit 0 with partial content, causing _prepare_baseline_repo to return a commit SHA from a near-empty directory and bake that corrupt baseline into both containers. Additionally, if subprocess.run(tar, ..., check=True) raises before reaching archive.wait(), the git archive subprocess is never reaped until GC collects the Popen object.

Prompt To Fix With AI
This is a comment left during a code review.
Path: vero/src/vero/harbor/build/compiler.py
Line: 120-130

Comment:
**`git archive` exit code is silently discarded**

`archive.wait()` is called but its return value is not checked. If `git archive` fails (e.g., the `rel` subpath doesn't exist at `HEAD`, or the ref is invalid), `tar` receives an empty or truncated stream. Depending on the tar implementation, it may exit 0 with partial content, causing `_prepare_baseline_repo` to return a commit SHA from a near-empty directory and bake that corrupt baseline into both containers. Additionally, if `subprocess.run(tar, ..., check=True)` raises before reaching `archive.wait()`, the `git archive` subprocess is never reaped until GC collects the `Popen` object.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Cursor Fix in Claude Code Fix in Codex

Comment on lines +237 to +239
name=config.name,
description=config.description,
mode=config.mode,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Temp directory created by mkdtemp() is never cleaned up

The directory created by tempfile.mkdtemp() on line 239 is used only to stage the dataset before registration, but is never deleted — not on success and not on exception. Over repeated builds this accumulates large scratch directories (the dataset can be gigabytes). Wrapping tmp creation with tempfile.TemporaryDirectory() as a context manager would clean it up automatically.

Prompt To Fix With AI
This is a comment left during a code review.
Path: vero/src/vero/harbor/build/compiler.py
Line: 237-239

Comment:
**Temp directory created by `mkdtemp()` is never cleaned up**

The directory created by `tempfile.mkdtemp()` on line 239 is used only to stage the dataset before registration, but is never deleted — not on success and not on exception. Over repeated builds this accumulates large scratch directories (the dataset can be gigabytes). Wrapping `tmp` creation with `tempfile.TemporaryDirectory()` as a context manager would clean it up automatically.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Cursor Fix in Claude Code Fix in Codex

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant