APR-262 Add Harbor verifiers support for v1#1401
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a91f41ae52
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if verifier_mode == VERIFIER_MODE_SEPARATE and verifier_environment is None: | ||
| verifier_sandbox = {**sandbox, "command_timeout": int(test_timeout)} |
There was a problem hiding this comment.
When a task sets [verifier] environment_mode = "separate" but does not provide [verifier.environment], this branch advertises support by creating a fresh verifier sandbox from the agent sandbox config. That fresh sandbox never receives the task's tests/ directory: run_separate_harbor_verifier only creates an empty /tests directory and transfers /logs/artifacts plus configured artifacts before running bash /tests/test.sh. In this supported mode the script is absent unless the base agent image happens to contain it, so otherwise-valid Harbor tasks always get reward 0.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Harbor PR #1655 does support environment_mode = "separate" without [verifier.environment]; upstream resolves it to a fresh copy of the top-level environment. The difference is that Harbor can build the verifier image from tests/, while this v1 sandbox path cannot. I fixed the fallback by marking that case with verifier_upload_tests and uploading the task tests into the fresh verifier sandbox before running /tests/test.sh, while explicit [verifier.environment] continues to expect a pre-baked verifier image.
ApprovabilityVerdict: Needs human review This PR introduces a substantial new feature (Harbor verifier support) with 350+ lines of new runtime logic including sandbox management, artifact transfer, and changed reward file precedence. Multiple unresolved P1 review comments identify potential bugs affecting evaluation correctness. You can customize Macroscope's approvability policy. Learn more. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f02768b6d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
xeophon
force-pushed
the
apr-262-harbor-v1-verifier-modes
branch
from
8ae240a to
05d4739
Compare
xeophon
force-pushed
the
apr-262-harbor-v1-verifier-modes
branch
2 times, most recently
from
a953660 to
f23a006
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f23a0066c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "if [ -s /logs/verifier/reward.json ]; then " | ||
| "cat /logs/verifier/reward.json; " | ||
| "elif [ -s /logs/verifier/reward.txt ]; then " | ||
| "cat /logs/verifier/reward.txt; fi" |
There was a problem hiding this comment.
When both /logs/verifier/reward.txt and /logs/verifier/reward.json are present, this command now reads JSON first, which can score from stale/diagnostic JSON or return 0 for invalid JSON even if reward.txt contains the valid final score. That changes previously working Harbor tasks and diverges from the existing Harbor implementations in this repo that prioritize reward.txt first. Fresh evidence in this commit: HARBOR_REWARD_COMMAND checks reward.json before reward.txt, and the new test test_harbor_reward_command_prefers_json_reward explicitly locks in that order.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f23a006. Configure here.
xeophon
force-pushed
the
apr-262-harbor-v1-verifier-modes
branch
from
f23a006 to
9daf4b2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9daf4b2aee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| lease.id, | ||
| state, | ||
| command="bash /tests/test.sh", | ||
| working_dir=None, |
There was a problem hiding this comment.
In separate verifier mode, the test runner now invokes bash /tests/test.sh with working_dir=None, which changes execution semantics versus the existing Harbor path (bash test.sh in /tests). Any verifier script that uses relative paths (for example ./fixtures/... or python helper.py) will resolve against the sandbox default workdir (often /app or image WORKDIR) instead of /tests, causing false failures and reward 0 in otherwise valid tasks. This is specific to verifier_mode = "separate" and is introduced by the new separate-sandbox path.
Useful? React with 👍 / 👎.
xeophon
force-pushed
the
apr-262-harbor-v1-verifier-modes
branch
from
9daf4b2 to
a063a69
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a063a69bff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "if [ -s /logs/verifier/reward.json ]; then " | ||
| "cat /logs/verifier/reward.json; " | ||
| "elif [ -s /logs/verifier/reward.txt ]; then " | ||
| "cat /logs/verifier/reward.txt; fi" |
There was a problem hiding this comment.
HARBOR_REWARD_COMMAND now reads /logs/verifier/reward.json before /logs/verifier/reward.txt, which can mis-score rollouts when both files exist (for example, a stale/diagnostic JSON alongside the final scalar reward in reward.txt). In that case valid tasks can return the wrong reward or 0 if JSON is malformed, despite a correct text reward being present, so this ordering can silently corrupt evaluation results.
Useful? React with 👍 / 👎.
1 participant
Summary
[verifier].environment_modeand[verifier.environment], including shared/separate validation and separate-mode sandbox configenvironment_mode = "separate"fallback without[verifier.environment]/logs/artifactsplus configured Harbor artifacts into verifier sandboxes, including explicit/logs/artifactsentries withexcludepatterns and source-path handling for artifactdestinationmappingsreward.jsonbeforereward.txtand documenting the v1 Harbor verifier lifecycleVerification
uv run --frozen pytest tests/test_v1_harbor_cli.py tests/test_opencode_harbor.py -quv run --frozen ruff format .uv run --frozen ruff check --fix .uv run --frozen ty check verifiersuv run --frozen --group policy semgrep --metrics=off --disable-version-check --config .semgrep/verifiers.yml --error --quietNote
Medium Risk
Adds a new execution path that spins up and tears down separate verifier sandboxes, including cross-sandbox file transfer and reward parsing changes, which could affect Harbor scoring and sandbox lifecycle behavior. Risk is mitigated by added unit tests covering mode resolution and separate-sandbox execution.
Overview
Adds v1 Harbor support for running verifiers in shared vs separate sandbox modes based on
task.toml([verifier].environment_mode/[verifier.environment]), including validation and per-mode sandbox configuration inHarborTaskset.Updates
harbor_rewardto optionally create a fresh verifier sandbox, transfer/logs/artifactsplus configuredartifactsinto it (with exclude handling), optionally upload tasktests/, run/tests/test.sh, and then delete the verifier sandbox. Reward parsing is aligned to prefer/logs/verifier/reward.jsonoverreward.txt, and docs/tests are updated accordingly.Reviewed by Cursor Bugbot for commit a063a69. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add Harbor verifier support for separate sandbox execution in v1 taskset
verifier_modesupport toHarborTaskset.task_row: defaults toshared; switches toseparatewhen[verifier.environment]is present in task config; raisesValueErrorfor incompatible combinations.run_separate_harbor_verifierto provision a fresh isolated sandbox, transfer task artifacts and inputs viatransfer_harbor_verifier_inputs, run tests, and delete the sandbox on completion.transfer_harbor_verifier_inputswhich packages specified artifacts plus/logs/artifactsfrom the agent sandbox into a gzipped tarball and extracts it inside the verifier sandbox.HARBOR_REWARD_COMMANDto prefer/logs/verifier/reward.jsonoverreward.txtwhen both exist.harbor_rewardnow routes execution throughrun_separate_harbor_verifierwhenverifier_modeisseparate, storesharbor_verifier_sandbox_idin state, and passes[verifier].envto the test process.Macroscope summarized a063a69.