[WIP]patcheval env#16
Conversation
📝 WalkthroughWalkthroughChangesThe pull request adds a PatchEval environment with CVE datasets, Docker agent configuration, an LLM patch runner, rule-based scoring, evaluation and training launch scripts, and documentation. It also updates Docker runner installation paths and Geo3K environment settings. PatchEval execution flow
Environment configuration updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Launcher
participant PatchEvalRunner
participant Gateway
participant TargetContainer
Launcher->>PatchEvalRunner: Send evaluation request
PatchEvalRunner->>TargetContainer: Read referenced source context
PatchEvalRunner->>Gateway: Request patch from chat completions
Gateway-->>PatchEvalRunner: Return unified diff
PatchEvalRunner->>TargetContainer: Apply patch and run validation
TargetContainer-->>PatchEvalRunner: Return validation metrics
PatchEvalRunner-->>Launcher: Emit structured result
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
rl/examples/geo3k_vl/env.sh (2)
71-72: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the gateway host configurable.
The fixed
100.99.221.45address will break when this script runs on another training node. Preserve an override, for example${AIEVOBOX_GATEWAY_HOST:-100.99.221.45}, and verify that the gateway binds to an interface reachable from Docker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rl/examples/geo3k_vl/env.sh` around lines 71 - 72, Update the AIEVOBOX_GATEWAY_HOST assignment in env.sh to allow an externally provided value while retaining 100.99.221.45 as the default, and ensure the gateway binding uses an interface reachable from Docker.
4-4: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the repository root overrideable.
This hardcodes all derived DB, agent, and evaluation paths to
/mnt/shared-storage-user/leishanzhe/...; launches from another node or account will fail if that mount differs. Use an environment override with this value as the default, and validate the root exists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rl/examples/geo3k_vl/env.sh` at line 4, Update the AIEVOBOX_ROOT assignment in env.sh to honor an existing environment value while defaulting to the current repository path, then validate that the resolved root exists before continuing. Keep all derived paths based on the validated AIEVOBOX_ROOT value.rl/examples/patcheval/env.sh (1)
35-36: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo validation that
SLIME_GLOBAL_BATCH_SIZEis divisible byRL_GROUP_SIZE.
SLIME_ROLLOUT_BATCH_SIZE=$((SLIME_GLOBAL_BATCH_SIZE / RL_GROUP_SIZE))(Line 36) silently truncates on integer division if the two values don't divide evenly, which would quietly change training batch semantics when either override is customized. A guard (e.g. fail if remainder != 0) would surface misconfiguration early instead of silently truncating.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rl/examples/patcheval/env.sh` around lines 35 - 36, Validate that SLIME_GLOBAL_BATCH_SIZE is evenly divisible by RL_GROUP_SIZE before calculating SLIME_ROLLOUT_BATCH_SIZE. Add a shell guard that detects a nonzero remainder and exits with a clear configuration error; only perform the existing integer division when validation succeeds.env/patcheval/runner.py (4)
52-53: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPolicy-rejected patches and truly unextracted patches share the same
failure_stage, losing training signal.When
_patch_is_allowedrejects a diff (Line 52-53),patchis cleared before_evaluate_patchruns, sometrics.patch_extractedbecomesFalseandfailure_stageis reported as"patch_extraction"(Line 112) — identical to the case where the model produced no diff at all. These are different failure modes (policy violation vs. no output) and conflating them makes it harder to diagnose/shape reward during RL training.Consider a distinct stage, e.g.
"patch_rejected", when the diff was extracted but blocked by_patch_is_allowed.Also applies to: 104-117
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@env/patcheval/runner.py` around lines 52 - 53, Update the patch handling in the runner flow around _patch_is_allowed and _evaluate_patch to preserve whether a non-empty diff was extracted before clearing policy-rejected patches. Report a distinct failure_stage such as "patch_rejected" for disallowed diffs, while retaining "patch_extraction" for cases where no diff was produced.
41-50: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo retry/backoff around the single gateway call.
_call_gatewaymakes oneurlopenattempt with no retry; any transient network hiccup during training will surface as aninfrastructure_errorepisode (Line 84-101) rather than being retried, reducing rollout throughput. Worth a small retry-with-backoff wrapper given this runs inside an RL training loop where transient failures are expected at scale.Also applies to: 248-264
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@env/patcheval/runner.py` around lines 41 - 50, Wrap the gateway invocation in the runner flow around _call_gateway with a small bounded retry mechanism and backoff for transient network failures. Preserve the existing request parameters, timeout, and infrastructure-error behavior after retries are exhausted; apply the same retry handling to the additional _call_gateway invocation noted in the comment.
1-346: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftNo test coverage for the reward-determining helpers.
_extract_patch,_patch_is_allowed, and_evaluate_patchdirectly determine the RL reward signal but ship with no unit tests in this PR. Given how central these are to training correctness, some coverage (e.g. malformed diffs, forbidden-path patches, PoC/unit-test transitions) would help catch regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@env/patcheval/runner.py` around lines 1 - 346, Add unit tests covering the reward-determining helpers _extract_patch, _patch_is_allowed, and _evaluate_patch. Include malformed or fenced diff extraction, forbidden-path rejection, and _evaluate_patch transitions for patch application, PoC, and unit-test outcomes; place tests in the existing test suite without modifying validation scripts.
226-245: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueReject
..path components in patch targets inenv/patcheval/runner.py:226-245.git apply --checkalready blocks path escapes, so this is only defense in depth, but adding a".." in Path(path).partsguard would keep malformed targets from reaching/workspace/fix-run.shlater.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@env/patcheval/runner.py` around lines 226 - 245, Update _patch_is_allowed to reject any patch target whose Path(path).parts contains the parent-directory component "..". Preserve the existing forbidden-name, test-path, and valid-path handling while adding this defense-in-depth guard before allowing the patch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@clusters/docker_clusters.py`:
- Around line 530-532: The runner installation fallback is evaluated before the
environment-specific runner path is resolved, causing custom paths with omitted
install_runner_script to skip copying. Update the relevant installation flow
around runner_container_path and the fallback check near the runner-copy helper
so the per-environment path is resolved first and used when deciding whether
installation is required; preserve explicit install_runner_script decisions.
In `@env/patcheval/PatchEval`:
- Line 1: Add the missing .gitmodules entry for the env/patcheval/PatchEval
gitlink, including its submodule path and repository URL, so fresh clones and
git submodule status initialize it correctly; alternatively remove the gitlink
and track the directory as regular files if it is not intended to remain a
submodule.
In `@env/patcheval/patcheval_config.yaml`:
- Line 3: Replace the :latest tag in each env_image entry, including the listed
repeated entries, with the tested immutable release tag or image digest used by
PatchEval. Ensure every configured image reference remains pinned to a fixed
version for reproducible evaluations.
In `@env/patcheval/runner.py`:
- Around line 22-101: Move _read_request() and session_id extraction inside
main()’s safety-net try block so parsing failures also produce a structured
failed result. Initialize session_id before the try with a safe fallback value,
then replace it when parsing succeeds, ensuring the exception handler can always
include session_id without triggering another error. Preserve the existing
failed-result fields and return behavior.
In `@manager/simulation_config.py`:
- Around line 468-476: Update the mount validation around normalized_mounts and
runner_mount so that, when install_runner_script is enabled, any normalized
mount targeting runner_entrypoint.target is rejected before Docker receives it;
preserve the existing _append_mount_if_missing behavior for non-conflicting
mounts.
In `@rl/examples/patcheval/env.sh`:
- Line 3: Update the AIEVOBOX_ROOT assignment to use the same ${VAR:-default}
pattern as the other environment variables, preserving an existing
caller-provided value while supplying a portable repository-relative or
documented default instead of the hardcoded personal path.
In `@rl/examples/patcheval/run_external_eval.sh`:
- Line 8: Remove the hardcoded external IP fallback from PATCH_EVAL_API_BASE in
run_external_eval.sh and require callers to set this endpoint explicitly,
matching the existing PATCH_EVAL_API_KEY validation behavior; preserve the
variable’s use by the evaluation commands once validated.
---
Nitpick comments:
In `@env/patcheval/runner.py`:
- Around line 52-53: Update the patch handling in the runner flow around
_patch_is_allowed and _evaluate_patch to preserve whether a non-empty diff was
extracted before clearing policy-rejected patches. Report a distinct
failure_stage such as "patch_rejected" for disallowed diffs, while retaining
"patch_extraction" for cases where no diff was produced.
- Around line 41-50: Wrap the gateway invocation in the runner flow around
_call_gateway with a small bounded retry mechanism and backoff for transient
network failures. Preserve the existing request parameters, timeout, and
infrastructure-error behavior after retries are exhausted; apply the same retry
handling to the additional _call_gateway invocation noted in the comment.
- Around line 1-346: Add unit tests covering the reward-determining helpers
_extract_patch, _patch_is_allowed, and _evaluate_patch. Include malformed or
fenced diff extraction, forbidden-path rejection, and _evaluate_patch
transitions for patch application, PoC, and unit-test outcomes; place tests in
the existing test suite without modifying validation scripts.
- Around line 226-245: Update _patch_is_allowed to reject any patch target whose
Path(path).parts contains the parent-directory component "..". Preserve the
existing forbidden-name, test-path, and valid-path handling while adding this
defense-in-depth guard before allowing the patch.
In `@rl/examples/geo3k_vl/env.sh`:
- Around line 71-72: Update the AIEVOBOX_GATEWAY_HOST assignment in env.sh to
allow an externally provided value while retaining 100.99.221.45 as the default,
and ensure the gateway binding uses an interface reachable from Docker.
- Line 4: Update the AIEVOBOX_ROOT assignment in env.sh to honor an existing
environment value while defaulting to the current repository path, then validate
that the resolved root exists before continuing. Keep all derived paths based on
the validated AIEVOBOX_ROOT value.
In `@rl/examples/patcheval/env.sh`:
- Around line 35-36: Validate that SLIME_GLOBAL_BATCH_SIZE is evenly divisible
by RL_GROUP_SIZE before calculating SLIME_ROLLOUT_BATCH_SIZE. Add a shell guard
that detects a nonzero remainder and exits with a clear configuration error;
only perform the existing integer division when validation succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 794b67ce-37cf-472f-ab8e-98a6348bc7c7
📒 Files selected for processing (25)
.gitignore=10.1clusters/docker_clusters.pyenv/geo3k/geo3k_config.yamlenv/patcheval/PatchEvalenv/patcheval/datasets/cve-2020-25459.jsonlenv/patcheval/datasets/cve-2020-26215.jsonlenv/patcheval/datasets/cve-2021-23376.jsonlenv/patcheval/datasets/cve-2022-1986.jsonlenv/patcheval/datasets/cve-2024-25620.jsonlenv/patcheval/patcheval_config.yamlenv/patcheval/patcheval_start.yamlenv/patcheval/rule_evaluator.pyenv/patcheval/runner.pyevaluator/configs/patcheval_rule_eval.yamlmanager/simulation_config.pyrl/examples/geo3k_vl/=10.1rl/examples/geo3k_vl/env.shrl/examples/geo3k_vl/run_slime_generator.shrl/examples/patcheval/README.mdrl/examples/patcheval/env.shrl/examples/patcheval/reward_1_fix_patches.txtrl/examples/patcheval/run_buffer_server.shrl/examples/patcheval/run_external_eval.shrl/examples/patcheval/run_slime_generator.sh
| runner_container_path = str( | ||
| docker_cfg.get("runner_container_path", self._runner_container_path) or self._runner_container_path | ||
| ).strip() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the per-environment path before deciding whether to install.
The fallback at Lines 521-524 checks self._runner_container_path before this custom path is resolved. For a custom runner path with install_runner_script omitted, Lines 576-581 enable installation, but this helper returns early and never copies the runner. Resolve runner_container_path before the fallback check, or pass the resolved install decision into this method.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@clusters/docker_clusters.py` around lines 530 - 532, The runner installation
fallback is evaluated before the environment-specific runner path is resolved,
causing custom paths with omitted install_runner_script to skip copying. Update
the relevant installation flow around runner_container_path and the fallback
check near the runner-copy helper so the per-environment path is resolved first
and used when deciding whether installation is required; preserve explicit
install_runner_script decisions.
| @@ -0,0 +1 @@ | |||
| Subproject commit ae9b3a7233e9e2fe86dcba8baba24beff9bcbba8 | |||
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git config -f .gitmodules --get-regexp '^submodule\..*\.path$'
git submodule status -- env/patcheval/PatchEval
git ls-tree HEAD env/patcheval/PatchEval
if [ -d env/patcheval/PatchEval/.git ] || [ -f env/patcheval/PatchEval/.git ]; then
git -C env/patcheval/PatchEval cat-file -e ae9b3a7233e9e2fe86dcba8baba24beff9bcbba8^{commit}
fiRepository: AI45Lab/SAfactory
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== top-level files ==\n'
git ls-files | sed -n '1,200p'
printf '\n== .gitmodules ==\n'
if [ -f .gitmodules ]; then
cat -n .gitmodules
else
echo '(missing)'
fi
printf '\n== path object ==\n'
git ls-tree HEAD env/patcheval/PatchEval || true
printf '\n== submodule status ==\n'
git submodule status -- env/patcheval/PatchEval || true
printf '\n== file type ==\n'
if [ -e env/patcheval/PatchEval ]; then
stat -c '%F %n' env/patcheval/PatchEval
else
echo 'path missing'
fiRepository: AI45Lab/SAfactory
Length of output: 6012
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os, subprocess, sys
def run(cmd):
p = subprocess.run(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True)
print(f"$ {' '.join(cmd)}")
print(p.stdout)
print(f"exit={p.returncode}\n")
run(["git", "rev-parse", "--is-inside-work-tree"])
run(["git", "ls-tree", "HEAD", "env/patcheval/PatchEval"])
run(["git", "submodule", "status", "--", "env/patcheval/PatchEval"])
print("exists", os.path.exists("env/patcheval/PatchEval"))
print("isdir", os.path.isdir("env/patcheval/PatchEval"))
print("isfile", os.path.isfile("env/patcheval/PatchEval"))
PYRepository: AI45Lab/SAfactory
Length of output: 519
🏁 Script executed:
#!/bin/bash
set -u
echo '== .gitmodules =='
if [ -f .gitmodules ]; then
cat -n .gitmodules
else
echo '(missing)'
fi
echo '== git ls-tree =='
git ls-tree HEAD env/patcheval/PatchEval || true
echo '== git submodule status =='
git submodule status -- env/patcheval/PatchEval || true
echo '== path info =='
if [ -e env/patcheval/PatchEval ]; then
stat -c '%F %n' env/patcheval/PatchEval
else
echo 'missing'
fiRepository: AI45Lab/SAfactory
Length of output: 450
Add the missing submodule mapping for env/patcheval/PatchEval.
env/patcheval/PatchEval is tracked as a gitlink, but .gitmodules has no entry for it, so git submodule status fails and fresh clones or CI can’t initialize the path. Add the submodule registration or stop tracking it as a submodule.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@env/patcheval/PatchEval` at line 1, Add the missing .gitmodules entry for the
env/patcheval/PatchEval gitlink, including its submodule path and repository
URL, so fresh clones and git submodule status initialize it correctly;
alternatively remove the gitlink and track the directory as regular files if it
is not intended to remain a submodule.
| @@ -0,0 +1,40 @@ | |||
| environments: | |||
| - env_name: patcheval_cve_2021_23376 | |||
| env_image: ghcr.io/anonymous2578-data/cve-2021-23376:latest | |||
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Pin PatchEval images to immutable versions.
Using :latest lets the image revision drift from the dataset’s fixed source locations and checks, making evaluation and training rewards non-reproducible. Pin each image to a tested release tag or digest.
Also applies to: 11-11, 19-19, 27-27, 35-35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@env/patcheval/patcheval_config.yaml` at line 3, Replace the :latest tag in
each env_image entry, including the listed repeated entries, with the tested
immutable release tag or image digest used by PatchEval. Ensure every configured
image reference remains pinned to a fixed version for reproducible evaluations.
| def main() -> int: | ||
| started_at = time.perf_counter() | ||
| request = _read_request() | ||
| session_id = _required_text(request.get("session_id"), "session_id") | ||
|
|
||
| try: | ||
| env_params = request.get("env_params") if isinstance(request.get("env_params"), dict) else {} | ||
| dataset = env_params.get("dataset") if isinstance(env_params.get("dataset"), dict) else {} | ||
| cve_id = _required_text(dataset.get("cve_id") or env_params.get("cve_id"), "cve_id") | ||
| work_dir = Path(_required_text(dataset.get("work_dir") or env_params.get("work_dir"), "work_dir")) | ||
| problem = _required_text(dataset.get("problem_statement"), "problem_statement") | ||
| timeout_s = _float(request.get("agent_start_timeout_s"), DEFAULT_TIMEOUT_S) | ||
|
|
||
| if not work_dir.is_dir(): | ||
| raise RuntimeError(f"PatchEval work directory does not exist: {work_dir}") | ||
|
|
||
| _remove_answer_leaks() | ||
| source_context = _collect_source_context(problem, work_dir) | ||
| prompt = _build_prompt(cve_id, problem, source_context) | ||
| response_text = _call_gateway( | ||
| base_url=_resolve_base_url(request, session_id), | ||
| model=_required_text( | ||
| request.get("model") or env_params.get("route_model") or os.environ.get("SAFACTORY_ROUTE_MODEL"), | ||
| "model", | ||
| ), | ||
| prompt=prompt, | ||
| temperature=_float(request.get("temperature"), 1.0), | ||
| timeout_s=timeout_s, | ||
| ) | ||
| patch = _extract_patch(response_text) | ||
| if patch and not _patch_is_allowed(patch): | ||
| patch = "" | ||
| result = _evaluate_patch(patch, work_dir, timeout_s) | ||
|
|
||
| _write_result( | ||
| { | ||
| "session_id": session_id, | ||
| "status": "succeeded", | ||
| "total_reward": result["score"], | ||
| "step_count": 1, | ||
| "terminated": True, | ||
| "truncated": False, | ||
| "error_text": None, | ||
| "metrics": { | ||
| "bench": "patcheval", | ||
| "cve_id": cve_id, | ||
| "score": result["score"], | ||
| "strict_success": result["strict_success"], | ||
| "patch_extracted": bool(patch), | ||
| "patch_applied": result["patch_applied"], | ||
| "poc_passed": result["poc_passed"], | ||
| "unit_test_present": result["unit_test_present"], | ||
| "unit_tests_passed": result["unit_tests_passed"], | ||
| "failure_stage": result["failure_stage"], | ||
| "patch": patch, | ||
| "poc_log": result["poc_log"], | ||
| "unit_test_log": result["unit_test_log"], | ||
| "duration_ms": round((time.perf_counter() - started_at) * 1000, 3), | ||
| }, | ||
| } | ||
| ) | ||
| return 0 | ||
| except Exception as exc: # runtime must always emit a result object | ||
| _write_result( | ||
| { | ||
| "session_id": session_id, | ||
| "status": "failed", | ||
| "total_reward": 0.0, | ||
| "step_count": 0, | ||
| "terminated": True, | ||
| "truncated": isinstance(exc, subprocess.TimeoutExpired), | ||
| "error_text": str(exc), | ||
| "metrics": { | ||
| "bench": "patcheval", | ||
| "infrastructure_error": True, | ||
| "duration_ms": round((time.perf_counter() - started_at) * 1000, 3), | ||
| }, | ||
| } | ||
| ) | ||
| return 0 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Request parsing runs outside the safety-net try, breaking the "always emit a result" contract.
_read_request() and the session_id extraction (lines 24-25) execute before the try block whose except is explicitly commented as "runtime must always emit a result object" (Line 84). If stdin/env JSON is missing/invalid, or session_id is absent, _required_text/_read_request raise RuntimeError outside the try, so the process crashes with an unhandled traceback instead of writing a structured failed result — exactly the failure mode this handler is supposed to prevent.
🐛 Proposed fix: move request parsing inside the try, with a safe session_id fallback
def main() -> int:
started_at = time.perf_counter()
- request = _read_request()
- session_id = _required_text(request.get("session_id"), "session_id")
+ session_id = "unknown"
try:
+ request = _read_request()
+ session_id = _required_text(request.get("session_id"), "session_id")
env_params = request.get("env_params") if isinstance(request.get("env_params"), dict) else {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def main() -> int: | |
| started_at = time.perf_counter() | |
| request = _read_request() | |
| session_id = _required_text(request.get("session_id"), "session_id") | |
| try: | |
| env_params = request.get("env_params") if isinstance(request.get("env_params"), dict) else {} | |
| dataset = env_params.get("dataset") if isinstance(env_params.get("dataset"), dict) else {} | |
| cve_id = _required_text(dataset.get("cve_id") or env_params.get("cve_id"), "cve_id") | |
| work_dir = Path(_required_text(dataset.get("work_dir") or env_params.get("work_dir"), "work_dir")) | |
| problem = _required_text(dataset.get("problem_statement"), "problem_statement") | |
| timeout_s = _float(request.get("agent_start_timeout_s"), DEFAULT_TIMEOUT_S) | |
| if not work_dir.is_dir(): | |
| raise RuntimeError(f"PatchEval work directory does not exist: {work_dir}") | |
| _remove_answer_leaks() | |
| source_context = _collect_source_context(problem, work_dir) | |
| prompt = _build_prompt(cve_id, problem, source_context) | |
| response_text = _call_gateway( | |
| base_url=_resolve_base_url(request, session_id), | |
| model=_required_text( | |
| request.get("model") or env_params.get("route_model") or os.environ.get("SAFACTORY_ROUTE_MODEL"), | |
| "model", | |
| ), | |
| prompt=prompt, | |
| temperature=_float(request.get("temperature"), 1.0), | |
| timeout_s=timeout_s, | |
| ) | |
| patch = _extract_patch(response_text) | |
| if patch and not _patch_is_allowed(patch): | |
| patch = "" | |
| result = _evaluate_patch(patch, work_dir, timeout_s) | |
| _write_result( | |
| { | |
| "session_id": session_id, | |
| "status": "succeeded", | |
| "total_reward": result["score"], | |
| "step_count": 1, | |
| "terminated": True, | |
| "truncated": False, | |
| "error_text": None, | |
| "metrics": { | |
| "bench": "patcheval", | |
| "cve_id": cve_id, | |
| "score": result["score"], | |
| "strict_success": result["strict_success"], | |
| "patch_extracted": bool(patch), | |
| "patch_applied": result["patch_applied"], | |
| "poc_passed": result["poc_passed"], | |
| "unit_test_present": result["unit_test_present"], | |
| "unit_tests_passed": result["unit_tests_passed"], | |
| "failure_stage": result["failure_stage"], | |
| "patch": patch, | |
| "poc_log": result["poc_log"], | |
| "unit_test_log": result["unit_test_log"], | |
| "duration_ms": round((time.perf_counter() - started_at) * 1000, 3), | |
| }, | |
| } | |
| ) | |
| return 0 | |
| except Exception as exc: # runtime must always emit a result object | |
| _write_result( | |
| { | |
| "session_id": session_id, | |
| "status": "failed", | |
| "total_reward": 0.0, | |
| "step_count": 0, | |
| "terminated": True, | |
| "truncated": isinstance(exc, subprocess.TimeoutExpired), | |
| "error_text": str(exc), | |
| "metrics": { | |
| "bench": "patcheval", | |
| "infrastructure_error": True, | |
| "duration_ms": round((time.perf_counter() - started_at) * 1000, 3), | |
| }, | |
| } | |
| ) | |
| return 0 | |
| def main() -> int: | |
| started_at = time.perf_counter() | |
| session_id = "unknown" | |
| try: | |
| request = _read_request() | |
| session_id = _required_text(request.get("session_id"), "session_id") | |
| env_params = request.get("env_params") if isinstance(request.get("env_params"), dict) else {} | |
| dataset = env_params.get("dataset") if isinstance(env_params.get("dataset"), dict) else {} | |
| cve_id = _required_text(dataset.get("cve_id") or env_params.get("cve_id"), "cve_id") | |
| work_dir = Path(_required_text(dataset.get("work_dir") or env_params.get("work_dir"), "work_dir")) | |
| problem = _required_text(dataset.get("problem_statement"), "problem_statement") | |
| timeout_s = _float(request.get("agent_start_timeout_s"), DEFAULT_TIMEOUT_S) | |
| if not work_dir.is_dir(): | |
| raise RuntimeError(f"PatchEval work directory does not exist: {work_dir}") | |
| _remove_answer_leaks() | |
| source_context = _collect_source_context(problem, work_dir) | |
| prompt = _build_prompt(cve_id, problem, source_context) | |
| response_text = _call_gateway( | |
| base_url=_resolve_base_url(request, session_id), | |
| model=_required_text( | |
| request.get("model") or env_params.get("route_model") or os.environ.get("SAFACTORY_ROUTE_MODEL"), | |
| "model", | |
| ), | |
| prompt=prompt, | |
| temperature=_float(request.get("temperature"), 1.0), | |
| timeout_s=timeout_s, | |
| ) | |
| patch = _extract_patch(response_text) | |
| if patch and not _patch_is_allowed(patch): | |
| patch = "" | |
| result = _evaluate_patch(patch, work_dir, timeout_s) | |
| _write_result( | |
| { | |
| "session_id": session_id, | |
| "status": "succeeded", | |
| "total_reward": result["score"], | |
| "step_count": 1, | |
| "terminated": True, | |
| "truncated": False, | |
| "error_text": None, | |
| "metrics": { | |
| "bench": "patcheval", | |
| "cve_id": cve_id, | |
| "score": result["score"], | |
| "strict_success": result["strict_success"], | |
| "patch_extracted": bool(patch), | |
| "patch_applied": result["patch_applied"], | |
| "poc_passed": result["poc_passed"], | |
| "unit_test_present": result["unit_test_present"], | |
| "unit_tests_passed": result["unit_tests_passed"], | |
| "failure_stage": result["failure_stage"], | |
| "patch": patch, | |
| "poc_log": result["poc_log"], | |
| "unit_test_log": result["unit_test_log"], | |
| "duration_ms": round((time.perf_counter() - started_at) * 1000, 3), | |
| }, | |
| } | |
| ) | |
| return 0 | |
| except Exception as exc: # runtime must always emit a result object | |
| _write_result( | |
| { | |
| "session_id": session_id, | |
| "status": "failed", | |
| "total_reward": 0.0, | |
| "step_count": 0, | |
| "terminated": True, | |
| "truncated": isinstance(exc, subprocess.TimeoutExpired), | |
| "error_text": str(exc), | |
| "metrics": { | |
| "bench": "patcheval", | |
| "infrastructure_error": True, | |
| "duration_ms": round((time.perf_counter() - started_at) * 1000, 3), | |
| }, | |
| } | |
| ) | |
| return 0 |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 84-84: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@env/patcheval/runner.py` around lines 22 - 101, Move _read_request() and
session_id extraction inside main()’s safety-net try block so parsing failures
also produce a structured failed result. Initialize session_id before the try
with a safe fallback value, then replace it when parsing succeeds, ensuring the
exception handler can always include session_id without triggering another
error. Preserve the existing failed-result fields and return behavior.
| install_runner_script = bool(container.get("install_runner_script", False)) | ||
| mounts = container.get("mounts", container.get("volumes", [])) or [] | ||
| if isinstance(mounts, (str, dict)): | ||
| mounts = [mounts] | ||
| if not isinstance(mounts, list): | ||
| raise ValueError(f"container.mounts for {agent_name!r} must be a list in {cfg_path}") | ||
| normalized_mounts = [_normalize_mount(mount, cfg_path) for mount in mounts] | ||
| runner_mount = _mount_from_runner_entrypoint(runner_entrypoint) | ||
| if runner_mount: | ||
| if runner_mount and not install_runner_script: |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject mounts that shadow the installed runner target.
When install_runner_script is enabled, Line 476 bypasses _append_mount_if_missing, so an existing mount at runner_entrypoint.target is never rejected. Validate that no normalized mount targets the runner destination before passing it to Docker.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@manager/simulation_config.py` around lines 468 - 476, Update the mount
validation around normalized_mounts and runner_mount so that, when
install_runner_script is enabled, any normalized mount targeting
runner_entrypoint.target is rejected before Docker receives it; preserve the
existing _append_mount_if_missing behavior for non-conflicting mounts.
| @@ -0,0 +1,36 @@ | |||
| #!/usr/bin/env bash | |||
|
|
|||
| export AIEVOBOX_ROOT=/mnt/shared-storage-user/leishanzhe/repo/SAfactory | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
AIEVOBOX_ROOT is hardcoded and not overridable, unlike every other variable in this file.
Every other variable in this file (Lines 12-26, 29-30) uses the ${VAR:-default} pattern so callers can override via environment. AIEVOBOX_ROOT (Line 3) is assigned unconditionally, so exporting it before sourcing env.sh is silently clobbered. It's also a personal, single-user absolute path (/mnt/shared-storage-user/leishanzhe/...) baked into a checked-in script, which breaks for any other engineer running this launch script as-is.
♻️ Proposed fix
-export AIEVOBOX_ROOT=/mnt/shared-storage-user/leishanzhe/repo/SAfactory
+export AIEVOBOX_ROOT=${AIEVOBOX_ROOT:-/mnt/shared-storage-user/leishanzhe/repo/SAfactory}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export AIEVOBOX_ROOT=/mnt/shared-storage-user/leishanzhe/repo/SAfactory | |
| export AIEVOBOX_ROOT=${AIEVOBOX_ROOT:-/mnt/shared-storage-user/leishanzhe/repo/SAfactory} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rl/examples/patcheval/env.sh` at line 3, Update the AIEVOBOX_ROOT assignment
to use the same ${VAR:-default} pattern as the other environment variables,
preserving an existing caller-provided value while supplying a portable
repository-relative or documented default instead of the hardcoded personal
path.
| AIEVOBOX_ROOT="$(cd -- "${SCRIPT_DIR}/../../.." &>/dev/null && pwd)" | ||
|
|
||
| PYTHON_BIN=${PYTHON_BIN:-python3} | ||
| PATCH_EVAL_API_BASE=${PATCH_EVAL_API_BASE:-http://35.220.164.252:3888/v1} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Hardcoded external IP address as default API endpoint.
PATCH_EVAL_API_BASE defaults to a raw external IP (http://35.220.164.252:3888/v1) baked into a checked-in script. Baking a specific external endpoint IP into source control is fragile (breaks silently if the IP changes) and exposes internal/partner infrastructure details in the repository. Consider requiring this to be explicitly set (like PATCH_EVAL_API_KEY already is at Line 17-21) rather than defaulting to a real address.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rl/examples/patcheval/run_external_eval.sh` at line 8, Remove the hardcoded
external IP fallback from PATCH_EVAL_API_BASE in run_external_eval.sh and
require callers to set this endpoint explicitly, matching the existing
PATCH_EVAL_API_KEY validation behavior; preserve the variable’s use by the
evaluation commands once validated.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation