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

[varunursekar]

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 (HarborRunner nested harbor run strategy) and the vero harbor build compiler that renders a BuildConfig into a Docker-Compose-based Harbor task directory. The compiler bakes the dataset, baseline repo, sidecar ServeConfig, and all secrets-plumbing into the generated environment.

• HarborRunner (runner.py): correctly builds the harbor run command (including --n-attempts/--max-retries), uses _trial_rank to deterministically pick the best trial per task, and gates resume on is_error() so transient failures are re-run.
• vero harbor build compiler (build/): renders Dockerfiles, compose, seed/test/solve scripts from Jinja2 templates; trust-boundary plumbing (secrets → sidecar only, root:600 admin token, advisory read_only_paths) looks correct. However, _prepare_baseline_repo has a broken stderr-capture pattern: communicate() is called immediately after archive.stdout.close(), which raises ValueError: I/O operation on closed file on CPython for every git-backed agent repo.
• Tests: test_harbor_runner.py has thorough coverage including a TestReviewFixes class that directly verifies the prior-round fixes; test_harbor_build.py covers the compiler but its built-fixture tests will fail due to the communicate() bug.

Confidence Score: 4/5

Hold until the communicate() / stdout.close() ordering in _prepare_baseline_repo is corrected — as written, compile_task always raises ValueError for any git-backed agent repo, which is the typical case.

The runner logic and trust-boundary wiring are sound, and the prior-round fixes (returncode check, TemporaryDirectory, retry ranking, --n-attempts/--max-retries) are all present. The single blocking issue is in _prepare_baseline_repo: calling archive.communicate() after archive.stdout.close() causes CPython to raise ValueError when it tries to register the closed file descriptor with a selector inside communicate(). This makes compile_task broken for all git repos, which would be immediately visible if the test suite is run.

vero/src/vero/harbor/build/compiler.py — the _prepare_baseline_repo finally block needs archive.stderr.read() + archive.wait() instead of archive.communicate().

Important Files Changed

Filename	Overview
vero/src/vero/harbor/build/compiler.py	New compiler: _prepare_baseline_repo has a P1 bug — communicate() after stdout.close() raises ValueError on CPython, breaking compile_task for all git repos. Temp-dir and git archive returncode fixes from prior review are correctly applied, but the stderr-capture approach is wrong.
vero/src/vero/harbor/runner.py	New HarborRunner Mode-B strategy: _build_command now correctly includes --n-attempts/--max-retries; _load_trials uses _trial_rank to avoid last-write-wins on retries. No new issues found.
vero/src/vero/harbor/build/config.py	New BuildConfig: well-structured Pydantic model; relative-path resolution in from_file handles monorepo layout correctly. No issues.
vero/src/vero/harbor/build/templates/docker-compose.yaml.j2	Compose template correctly restricts secrets to the sidecar via ${VAR:?msg} fail-fast interpolation; admin token volume mounted read-only to main; trust boundary wiring looks correct.
vero/tests/test_harbor_build.py	Comprehensive unit tests for the compiler; test_structure and other built-fixture tests will fail due to the P1 communicate() bug in _prepare_baseline_repo. The archive-failure and missing-secret tests are correct.
vero/tests/test_harbor_runner.py	Solid test coverage for HarborRunner: command construction, reward extraction, collation, resume, and retry ranking; TestReviewFixes directly validates the fixes from the previous review round.
vero/src/vero/harbor/build/templates/seed.sh.j2	Seed script correctly documents that read_only_paths chmod is advisory only and that authoritative enforcement is sidecar-side. No issues.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Caller
    participant HarborRunner
    participant harbor_CLI as harbor run (nested)
    participant jobs_dir as jobs/timestamp/trial
    participant Sidecar as eval-sidecar (vero harbor serve)

    Caller->>HarborRunner: produce_sample_results(workspace, params, result_dir)
    HarborRunner->>HarborRunner: _task_names_for(params) → [(sample_id, task_name)]
    HarborRunner->>HarborRunner: filter pending (skip _is_done samples)
    HarborRunner->>harbor_CLI: uv run harbor run -i task0 -i task1 --jobs-dir jobs/
    harbor_CLI->>Sidecar: eval requests per task
    Sidecar-->>harbor_CLI: verifier rewards
    harbor_CLI->>jobs_dir: result.json per trial
    harbor_CLI-->>HarborRunner: exit (non-zero tolerated)
    HarborRunner->>HarborRunner: _load_trials(jobs_dir) → best trial per task
    HarborRunner->>HarborRunner: _collate → _sample_result per pair
    HarborRunner->>Caller: save_sample_result (SampleResult) for each sample

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 Caller
    participant HarborRunner
    participant harbor_CLI as harbor run (nested)
    participant jobs_dir as jobs/timestamp/trial
    participant Sidecar as eval-sidecar (vero harbor serve)

    Caller->>HarborRunner: produce_sample_results(workspace, params, result_dir)
    HarborRunner->>HarborRunner: _task_names_for(params) → [(sample_id, task_name)]
    HarborRunner->>HarborRunner: filter pending (skip _is_done samples)
    HarborRunner->>harbor_CLI: uv run harbor run -i task0 -i task1 --jobs-dir jobs/
    harbor_CLI->>Sidecar: eval requests per task
    Sidecar-->>harbor_CLI: verifier rewards
    harbor_CLI->>jobs_dir: result.json per trial
    harbor_CLI-->>HarborRunner: exit (non-zero tolerated)
    HarborRunner->>HarborRunner: _load_trials(jobs_dir) → best trial per task
    HarborRunner->>HarborRunner: _collate → _sample_result per pair
    HarborRunner->>Caller: save_sample_result (SampleResult) for each sample

Loading

Comments Outside Diff (2)

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

   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.

   

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

   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.

   

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
vero/src/vero/harbor/build/compiler.py:100-104
**`communicate()` after `stdout.close()` raises `ValueError` — breaks git-repo builds**

`archive.stdout.close()` (line 103) closes the underlying OS file descriptor and marks the Python file object as closed. The very next line calls `archive.communicate()`, which internally calls `selector.register(self.stdout, EVENT_READ)` → `self.stdout.fileno()` → `ValueError: I/O operation on closed file` (CPython's `_fileobj_lookup` re-raises `ValueError` for non-int file objects). This means `_prepare_baseline_repo` always raises `ValueError` for any git-tracked agent repo, so `compile_task` is entirely broken for the normal case and all of the `built`-fixture tests would fail.

The intended fix (get stderr for the error message + reap the child) should instead read `archive.stderr` directly and call `archive.wait()`:
```python
finally:
    if archive.stdout is not None:
        archive.stdout.close()
    archive_err = (archive.stderr.read() if archive.stderr else b"").decode(errors="replace")
    archive.wait()
```

_{Reviews (3): Last reviewed commit: "fix(harbor): nested-run flags/dedup/resu..." | Re-trigger Greptile}

Greptile also left 1 inline comment on this PR.

[shehabyasser-scale]

HarborConfig.n_attempts / max_retries are typed fields, which signals they are honored, but _build_command never emits flags for them — so every nested run uses harbor defaults. On a budget-graded run that is a real cost/correctness bug. Either map them to the real harbor run flags here, or drop the fields. (Confirms Greptile's P1.)

[shehabyasser-scale]

trials[task_name] = data is last-write-wins over an unordered rglob; with retries or a reused jobs_dir, a failing attempt can clobber a passing one, feeding a wrong score into auto_best. Collect all matches and pick deterministically (latest mtime / highest attempt index), and/or namespace jobs_dir per attempt. (Confirms Greptile's P1.)

[shehabyasser-scale]

_existing treats a persisted error sample as 'done', so a sample whose nested run failed once is permanently skipped on resume and silently degrades the candidate's score. Either don't persist error samples, or make the pending/skip predicate treat error results as not-done.

[shehabyasser-scale]

archive.wait()'s return code is discarded; if git archive fails, tar can still exit 0 on a truncated stream and you commit a near-empty baseline whose SHA gets baked into both images. Assert archive.wait() == 0 (and reap the Popen on the tar-failure path). (Confirms Greptile.)

[shehabyasser-scale]

This renders SECRET: "${SECRET}", resolved from the run host's env, not anything the compiler validates. An unset var interpolates to empty and the sidecar comes up credential-less, failing every eval opaquely. Consider failing the build / emitting a required-secrets manifest when a declared secret is missing. Note test_rendered_files_parse only asserts the literal ${...} string, which hides this.

[shehabyasser-scale]

This chown root:root + chmod -R a-w applies to the working tree under /work/agent, but the verifier never executes the working tree: _transfer_commit fetches git blobs and the evaluator checks the commit out into a fresh temp copy. The agent can git add -f a modified scorer and commit it regardless of working-tree perms. So read_only_paths is not a tamper control — please document it as advisory-only and move scorer-provenance enforcement into the sidecar (see the Mode A scorer-provenance comment on #4).

[greptile-apps]

communicate() after stdout.close() raises ValueError — breaks git-repo builds

archive.stdout.close() (line 103) closes the underlying OS file descriptor and marks the Python file object as closed. The very next line calls archive.communicate(), which internally calls selector.register(self.stdout, EVENT_READ) → self.stdout.fileno() → ValueError: I/O operation on closed file (CPython's _fileobj_lookup re-raises ValueError for non-int file objects). This means _prepare_baseline_repo always raises ValueError for any git-tracked agent repo, so compile_task is entirely broken for the normal case and all of the built-fixture tests would fail.

The intended fix (get stderr for the error message + reap the child) should instead read archive.stderr directly and call archive.wait():

finally:
    if archive.stdout is not None:
        archive.stdout.close()
    archive_err = (archive.stderr.read() if archive.stderr else b"").decode(errors="replace")
    archive.wait()

Prompt To Fix With AI

This is a comment left during a code review.
Path: vero/src/vero/harbor/build/compiler.py
Line: 100-104

Comment:
**`communicate()` after `stdout.close()` raises `ValueError` — breaks git-repo builds**

`archive.stdout.close()` (line 103) closes the underlying OS file descriptor and marks the Python file object as closed. The very next line calls `archive.communicate()`, which internally calls `selector.register(self.stdout, EVENT_READ)` → `self.stdout.fileno()` → `ValueError: I/O operation on closed file` (CPython's `_fileobj_lookup` re-raises `ValueError` for non-int file objects). This means `_prepare_baseline_repo` always raises `ValueError` for any git-tracked agent repo, so `compile_task` is entirely broken for the normal case and all of the `built`-fixture tests would fail.

The intended fix (get stderr for the error message + reap the child) should instead read `archive.stderr` directly and call `archive.wait()`:
```python
finally:
    if archive.stdout is not None:
        archive.stdout.close()
    archive_err = (archive.stderr.read() if archive.stderr else b"").decode(errors="replace")
    archive.wait()
```

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