[WIP]patcheval env#16
Conversation
📝 WalkthroughWalkthroughThis PR adds Docker image archive loading and cleanup, per-environment runner installation, official PatchEval evaluation, strict patch execution, generated configurations, launch scripts, training orchestration, and related Geo3K runtime updates. ChangesPatchEval execution flow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 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.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
manager/simulation_config.py (1)
472-480: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject conflicting runner mounts when
install_runner_scriptis enabledmanager/simulation_config.py:479-503
_append_mount_if_missingis skipped in this branch, so an existing mount onrunner_entrypoint.targetstill slips through. Run the same target-conflict check before settingrunner_container_path.🤖 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 472 - 480, Update the runner-mount handling near _mount_from_runner_entrypoint and runner_container_path so that, when install_runner_script is enabled, it still checks normalized_mounts for an existing mount targeting runner_entrypoint.target and rejects conflicts before setting runner_container_path. Preserve the existing _append_mount_if_missing behavior for non-conflicting mounts.
♻️ Duplicate comments (1)
clusters/docker_clusters.py (1)
531-568: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRunner-install auto-detection still uses the backend default path, not the per-environment path.
_install_runner_script_if_neededderives its auto-detect fallback withself._runner_container_path in record.run_command(Line 538) before it resolves the per-environmentrunner_container_pathat Lines 545-547._start_containeritself resolves the correct per-env path at Lines 687-692 for logging, but never passes that resolved value into_install_runner_script_if_needed. So for an agent config with a customrunner_container_pathandinstall_runner_scriptomitted,record.run_commanduses the custom path while the fallback check tests against the backend default — the substring test fails,install_runnerevaluates falsy, and the method returns early without ever copying the runner script into the container.This is the same gap flagged in a prior review on this file.
🛠️ Proposed fix
async def _install_runner_script_if_needed( self, record: DockerContainerRecord, docker_cfg: Dict[str, Any], ) -> None: + runner_container_path = str( + docker_cfg.get("runner_container_path", self._runner_container_path) or self._runner_container_path + ).strip() install_runner = docker_cfg.get("install_runner_script") if install_runner is None: - install_runner = self._runner_container_path in record.run_command + install_runner = runner_container_path in record.run_command if not install_runner: return runner_host_path = self._resolve_runner_host_path(docker_cfg.get("runner_script_host_path")) if runner_host_path is None: runner_host_path = self._runner_host_path - runner_container_path = str( - docker_cfg.get("runner_container_path", self._runner_container_path) or self._runner_container_path - ).strip()Also applies to: 687-692
🤖 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 531 - 568, Update _install_runner_script_if_needed and its caller _start_container so auto-detection checks record.run_command against the resolved per-environment runner_container_path, not self._runner_container_path. Resolve the configured path before evaluating the omitted install_runner_script fallback, pass or reuse that value consistently for installation and logging, and preserve explicit install_runner_script behavior.
🧹 Nitpick comments (1)
env/patcheval/docker_archive_adapter/sitecustomize.py (1)
16-19: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMissing env var crashes at import time with a raw
KeyError.
ARCHIVE_DIRis built fromos.environ["PATCHEVAL_IMAGE_ARCHIVE_DIR"]at module scope, which runs whenever this directory is placed onPYTHONPATH(per the module docstring). If the variable is unset, this raises an unguardedKeyErrorbefore_install()'s own explicit "archive directory does not exist" check ever gets a chance to produce a clearer message. Sincesitecustomizemodules are auto-imported by every Python process that inherits thisPYTHONPATH, an unset var turns into an opaque traceback rather than the intended actionable error.🛠️ Proposed fix
-ARCHIVE_DIR = Path(os.environ["PATCHEVAL_IMAGE_ARCHIVE_DIR"]).expanduser() +_archive_dir_env = os.environ.get("PATCHEVAL_IMAGE_ARCHIVE_DIR", "").strip() +if not _archive_dir_env: + raise RuntimeError("PATCHEVAL_IMAGE_ARCHIVE_DIR must be set when this sitecustomize adapter is on PYTHONPATH") +ARCHIVE_DIR = Path(_archive_dir_env).expanduser()🤖 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/docker_archive_adapter/sitecustomize.py` around lines 16 - 19, Update the module-level ARCHIVE_DIR initialization in sitecustomize so a missing PATCHEVAL_IMAGE_ARCHIVE_DIR does not raise an unguarded KeyError during import; use the existing _install() validation path to emit its intended actionable archive-directory error, while preserving the current behavior when the variable is set.
🤖 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 570-671: Update _start_container to hold the per-image lock across
image preparation and the actual docker run, preventing
_cleanup_archive_loaded_image from removing the image before creation. Refactor
_ensure_image_available so it does not reacquire the same lock when called from
this locked startup path, while preserving serialization for other callers.
Apply the same protection to pull-policy handling where the image is used.
In `@env/patcheval/generate_full_config.py`:
- Around line 96-102: Update archive_name to match the loader’s stem derivation
for untagged images: preserve the repository name and return its .tar archive
without appending “-latest”; continue using the repository-tag stem for
explicitly tagged images.
In `@env/patcheval/rule_evaluator.py`:
- Around line 26-34: Update the EvalResult construction in the strict-success
evaluation flow so the reason is conditional on strict_success: retain the
existing success message when true, and provide an accurate failure reason when
false. Keep the score calculation and other result fields unchanged.
In `@rl/examples/patcheval/run_external_eval.sh`:
- Line 253: Update the --max-steps argument in run_external_eval.sh so the
official run_official_s1.sh path honors the five-step S1.4 contract instead of
forcing a single step. Preserve the configured value from env.sh and ensure the
formal evaluation runs with five feedback steps.
---
Outside diff comments:
In `@manager/simulation_config.py`:
- Around line 472-480: Update the runner-mount handling near
_mount_from_runner_entrypoint and runner_container_path so that, when
install_runner_script is enabled, it still checks normalized_mounts for an
existing mount targeting runner_entrypoint.target and rejects conflicts before
setting runner_container_path. Preserve the existing _append_mount_if_missing
behavior for non-conflicting mounts.
---
Duplicate comments:
In `@clusters/docker_clusters.py`:
- Around line 531-568: Update _install_runner_script_if_needed and its caller
_start_container so auto-detection checks record.run_command against the
resolved per-environment runner_container_path, not self._runner_container_path.
Resolve the configured path before evaluating the omitted install_runner_script
fallback, pass or reuse that value consistently for installation and logging,
and preserve explicit install_runner_script behavior.
---
Nitpick comments:
In `@env/patcheval/docker_archive_adapter/sitecustomize.py`:
- Around line 16-19: Update the module-level ARCHIVE_DIR initialization in
sitecustomize so a missing PATCHEVAL_IMAGE_ARCHIVE_DIR does not raise an
unguarded KeyError during import; use the existing _install() validation path to
emit its intended actionable archive-directory error, while preserving the
current behavior when the variable is set.
🪄 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: 3fe45ff5-8c6b-4e7b-a0fe-bed1576e4ae6
📒 Files selected for processing (20)
.dockerignoreargs.pyclusters/docker_clusters.pyenv/patcheval/docker_archive_adapter/sitecustomize.pyenv/patcheval/generate_full_config.pyenv/patcheval/official_bridge.pyenv/patcheval/rule_evaluator.pyenv/patcheval/strict_runner.pymanager/simulation_config.pymanager/types.pyrequirements.txtrl/buffer_server.pyrl/examples/patcheval/.gitignorerl/examples/patcheval/README.mdrl/examples/patcheval/env.shrl/examples/patcheval/run_buffer_server.shrl/examples/patcheval/run_external_eval.shrl/examples/patcheval/run_official_s1.shtests/test_patcheval_official_bridge.pytests/test_patcheval_strict_protocol.py
| async def _ensure_image_available(self, image: str) -> None: | ||
| image_lock = self._image_locks.setdefault(image, asyncio.Lock()) | ||
| async with image_lock: | ||
| inspect_result = await self._run_command( | ||
| [self.docker_bin, "image", "inspect", image], | ||
| check=False, | ||
| timeout_s=self._inspect_timeout_s, | ||
| ) | ||
| if inspect_result.returncode == 0: | ||
| return | ||
|
|
||
| archive_path = self._archive_path_for_image(image) | ||
| if archive_path is None: | ||
| raise RuntimeError( | ||
| f"Required Docker image {image!r} is unavailable and no matching archive was found" | ||
| ) | ||
|
|
||
| log.info("loading Docker image %s from archive %s", image, archive_path) | ||
| await self._run_required( | ||
| [self.docker_bin, "load", "-i", str(archive_path)], | ||
| action=f"load image archive for {image}", | ||
| timeout_s=self._start_timeout_s, | ||
| ) | ||
| verify_result = await self._run_command( | ||
| [self.docker_bin, "image", "inspect", image], | ||
| check=False, | ||
| timeout_s=self._inspect_timeout_s, | ||
| ) | ||
| if verify_result.returncode != 0: | ||
| raise RuntimeError( | ||
| f"Docker archive {archive_path} loaded but expected image tag {image!r} is unavailable" | ||
| ) | ||
| self._archive_loaded_images.add(image) | ||
|
|
||
| async def _cleanup_archive_loaded_image(self, image: str) -> None: | ||
| if not self._cleanup_image_on_finish or image not in self._archive_loaded_images: | ||
| return | ||
|
|
||
| image_lock = self._image_locks.setdefault(image, asyncio.Lock()) | ||
| async with image_lock: | ||
| try: | ||
| containers = await self._run_command( | ||
| [self.docker_bin, "ps", "-aq", "--filter", f"ancestor={image}"], | ||
| check=False, | ||
| timeout_s=self._inspect_timeout_s, | ||
| ) | ||
| except subprocess.TimeoutExpired: | ||
| log.warning( | ||
| "timed out after %.1fs checking containers for archive-loaded image %s; defer image cleanup", | ||
| self._inspect_timeout_s, | ||
| image, | ||
| ) | ||
| return | ||
| if containers.returncode != 0: | ||
| log.warning( | ||
| "failed to check containers using archive-loaded image %s: %s", | ||
| image, | ||
| self._tail(containers.stderr), | ||
| ) | ||
| return | ||
| if (containers.stdout or "").strip(): | ||
| return | ||
|
|
||
| result = await self._run_command( | ||
| [self.docker_bin, "image", "rm", image], | ||
| check=False, | ||
| timeout_s=self._remove_timeout_s, | ||
| ) | ||
| if result.returncode == 0 or self._is_missing_image_error(result.stderr): | ||
| self._archive_loaded_images.discard(image) | ||
| log.info("removed archive-loaded Docker image %s", image) | ||
| return | ||
| log.warning("failed to remove archive-loaded Docker image %s: %s", image, self._tail(result.stderr)) | ||
|
|
||
| def _archive_path_for_image(self, image: str) -> Optional[Path]: | ||
| if self._image_archive_dir is None: | ||
| return None | ||
|
|
||
| image_name = str(image or "").strip().rsplit("/", 1)[-1] | ||
| if "@" in image_name: | ||
| repository, digest = image_name.split("@", 1) | ||
| archive_stem = f"{repository}-{digest.replace(':', '-')}" | ||
| elif ":" in image_name: | ||
| repository, tag = image_name.rsplit(":", 1) | ||
| archive_stem = f"{repository}-{tag}" | ||
| else: | ||
| archive_stem = f"{image_name}-latest" | ||
|
|
||
| for suffix in (".tar", ".tar.gz", ".tgz"): | ||
| candidate = self._image_archive_dir / f"{archive_stem}{suffix}" | ||
| if candidate.is_file() and candidate.stat().st_size > 0: | ||
| return candidate | ||
| return None | ||
|
|
||
| async def _start_container(self, *, env_name: str, image: str) -> DockerContainerRecord: | ||
| docker_cfg = self._docker_cfg_for_env(env_name) | ||
| pull_policy = self._resolve_pull_policy(docker_cfg) | ||
| if pull_policy == "always": | ||
| await self._run_required([self.docker_bin, "pull", image], action=f"pull image {image}") | ||
| else: | ||
| await self._ensure_image_available(image) | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Image can be removed by a concurrent cleanup between "ensure available" and the actual docker run.
_ensure_image_available holds the per-image lock only through loading/verification (Line 570-602), then releases it. _start_container subsequently issues the real docker run (Line 743) outside that lock. If another task is concurrently finishing the last container that used the same image, _cleanup_archive_loaded_image (Line 604) can acquire the now-free per-image lock, see zero matching containers via docker ps --filter ancestor=... (the new container hasn't been created yet), and remove the image right before/while the new docker run executes — causing a spurious "no such image" failure. This only manifests when cleanup_image_on_finish is enabled (the intended PatchEval use case) with concurrent container starts, which this backend explicitly supports via _startup_sem.
See consolidated comment for the cross-file fix (this pattern also exists in env/patcheval/docker_archive_adapter/sitecustomize.py).
🤖 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 570 - 671, Update _start_container
to hold the per-image lock across image preparation and the actual docker run,
preventing _cleanup_archive_loaded_image from removing the image before
creation. Refactor _ensure_image_available so it does not reacquire the same
lock when called from this locked startup path, while preserving serialization
for other callers. Apply the same protection to pull-policy handling where the
image is used.
| def archive_name(image: str) -> str: | ||
| image_name = image.rsplit("/", 1)[-1] | ||
| if ":" in image_name: | ||
| repository, tag = image_name.rsplit(":", 1) | ||
| else: | ||
| repository, tag = image_name, "latest" | ||
| return f"{repository}-{tag}.tar" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the loader's stem derivation to validate the naming contract.
fd -a sitecustomize.py env/patcheval/docker_archive_adapter --exec sed -n '50,80p' {}Repository: AI45Lab/SAfactory
Length of output: 1412
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant functions and nearby callers.
sed -n '1,240p' env/patcheval/generate_full_config.py
echo '---'
# Find all references to archive_name and image sources.
rg -n "archive_name\(|image\b|latest|tag" env/patcheval/generate_full_config.py env/patcheval -g '!**/__pycache__/**'Repository: AI45Lab/SAfactory
Length of output: 14394
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import json
dataset = Path("env/patcheval/PatchEval/patcheval/exp_agent/sweagent/dataset.jsonl")
official = Path("env/patcheval/PatchEval/patcheval/datasets/input.json")
def scan_jsonl(path):
total = 0
no_tag = 0
samples = []
with path.open("r", encoding="utf-8") as f:
for line in f:
if not line.strip():
continue
total += 1
rec = json.loads(line)
img = str(rec.get("image_name", "")).strip()
last = img.rsplit("/", 1)[-1]
if ":" not in last:
no_tag += 1
if len(samples) < 10:
samples.append(img)
return total, no_tag, samples
total, no_tag, samples = scan_jsonl(dataset)
print(f"dataset: {dataset}")
print(f"records={total} no_tag={no_tag}")
for s in samples:
print(s)
with official.open("r", encoding="utf-8") as f:
records = json.load(f)
count = 0
samples = []
for rec in records:
img = rec.get("image_name")
if isinstance(img, str) and img.strip():
count += 1
last = img.rsplit("/", 1)[-1]
if ":" not in last and len(samples) < 10:
samples.append(img)
print(f"official records with image_name={count}")
print("official no-tag samples:")
for s in samples:
print(s)
PYRepository: AI45Lab/SAfactory
Length of output: 593
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the dataset files used by env/patcheval/generate_full_config.py.
fd -a 'dataset.jsonl' env/patcheval
echo '---'
fd -a 'input.json' env/patcheval
echo '---'
# Print every image_name pattern from the likely dataset files if present.
python3 - <<'PY'
from pathlib import Path
import json
candidates = list(Path("env/patcheval").rglob("dataset.jsonl")) + list(Path("env/patcheval").rglob("input.json"))
seen = set()
for path in candidates:
if path in seen:
continue
seen.add(path)
print(f"FILE: {path}")
try:
if path.name == "dataset.jsonl":
total = 0
no_tag = 0
samples = []
with path.open("r", encoding="utf-8") as f:
for line in f:
if not line.strip():
continue
total += 1
rec = json.loads(line)
img = str(rec.get("image_name", "")).strip()
last = img.rsplit("/", 1)[-1]
if ":" not in last:
no_tag += 1
if len(samples) < 10:
samples.append(img)
print(f"records={total} no_tag={no_tag}")
for s in samples:
print(f" {s}")
elif path.name == "input.json":
with path.open("r", encoding="utf-8") as f:
records = json.load(f)
total = 0
no_tag = 0
samples = []
for rec in records if isinstance(records, list) else []:
img = str(rec.get("image_name", "")).strip()
if not img:
continue
total += 1
last = img.rsplit("/", 1)[-1]
if ":" not in last:
no_tag += 1
if len(samples) < 10:
samples.append(img)
print(f"records_with_image_name={total} no_tag={no_tag}")
for s in samples:
print(f" {s}")
except Exception as e:
print(f"ERROR: {e}")
PYRepository: AI45Lab/SAfactory
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where image_name values come from and whether examples are tagged or untagged.
rg -n '"image_name"|env_image|archive_name\(' . -g '!**/__pycache__/**' -g '!**/.git/**'
echo '---'
# Show files in env/patcheval that mention the dataset or archive conventions.
rg -n 'dataset.jsonl|input.json|archive|docker_archive_adapter|latest|image_name' env/patcheval -g '!**/__pycache__/**'Repository: AI45Lab/SAfactory
Length of output: 9623
Align archive_name with the loader’s stem rule.
generate_full_config.py writes bar-latest.tar for any image without a tag, but env/patcheval/docker_archive_adapter/sitecustomize.py looks for bar.tar. That leaves colonless images undeployable; use the same stem derivation in both places.
🔧 Align the stem derivation
def archive_name(image: str) -> str:
- image_name = image.rsplit("/", 1)[-1]
- if ":" in image_name:
- repository, tag = image_name.rsplit(":", 1)
- else:
- repository, tag = image_name, "latest"
- return f"{repository}-{tag}.tar"
+ stem = image.rsplit("/", 1)[-1].replace(":", "-")
+ return f"{stem}.tar"📝 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 archive_name(image: str) -> str: | |
| image_name = image.rsplit("/", 1)[-1] | |
| if ":" in image_name: | |
| repository, tag = image_name.rsplit(":", 1) | |
| else: | |
| repository, tag = image_name, "latest" | |
| return f"{repository}-{tag}.tar" | |
| def archive_name(image: str) -> str: | |
| stem = image.rsplit("/", 1)[-1].replace(":", "-") | |
| return f"{stem}.tar" |
🤖 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/generate_full_config.py` around lines 96 - 102, Update
archive_name to match the loader’s stem derivation for untagged images: preserve
the repository name and return its .tar archive without appending “-latest”;
continue using the repository-tag stem for explicitly tagged images.
| --pool-size "${PATCH_EVAL_POOL_SIZE}" \ | ||
| --max-workers "${PATCH_EVAL_POOL_SIZE}" \ | ||
| --no-circuit-breaker \ | ||
| --max-steps 1 \ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Honor the five-step S1.4 contract.
Line 253 forces every setting to one step. This makes the formal run_official_s1.sh path run S1.4 without the five feedback steps configured in env.sh.
Proposed fix
+if [[ "${PATCH_EVAL_SETTING}" == "s1.4" ]]; then
+ PATCH_EVAL_MAX_STEPS=${PATCH_EVAL_MAX_STEPS:-5}
+else
+ PATCH_EVAL_MAX_STEPS=${PATCH_EVAL_MAX_STEPS:-1}
+fi
+
...
- --max-steps 1 \
+ --max-steps "${PATCH_EVAL_MAX_STEPS}" \📝 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.
| --max-steps 1 \ | |
| if [[ "${PATCH_EVAL_SETTING}" == "s1.4" ]]; then | |
| PATCH_EVAL_MAX_STEPS=${PATCH_EVAL_MAX_STEPS:-5} | |
| else | |
| PATCH_EVAL_MAX_STEPS=${PATCH_EVAL_MAX_STEPS:-1} | |
| fi | |
| --max-steps "${PATCH_EVAL_MAX_STEPS}" \ |
🤖 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 253, Update the
--max-steps argument in run_external_eval.sh so the official run_official_s1.sh
path honors the five-step S1.4 contract instead of forcing a single step.
Preserve the configured value from env.sh and ensure the formal evaluation runs
with five feedback steps.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/test_patcheval_rule_evaluator.py (1)
35-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the failure branches of
evaluate_rule.Current tests only cover the "Repair Success" and generic validation-failure happy paths via a monkeypatched
_load_official_evaluation. Consider adding cases for: missingcve_id/patch/language(earlyEvalResult.failed),poc_passed is None(container failed to start), and an exception raised fromrun_evaluation. Also assert onresult.reasonin the failure test to guard against regressions of the "reason always claims success" bug fixed in this file.🤖 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 `@tests/test_patcheval_rule_evaluator.py` around lines 35 - 73, Extend the tests around evaluate_rule to cover missing cve_id, patch, or language returning EvalResult.failed, poc_passed=None representing container startup failure, and run_evaluation raising an exception. Assert each failure’s status and reason, including the existing validation-failure case, to ensure reasons do not incorrectly report success.
🤖 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 `@env/patcheval/generate_full_config.py`:
- Around line 36-38: Remove the machine-specific default from
DEFAULT_OFFICIAL_RUNTIME and make --official-runtime-dir an explicit required
CLI argument, ensuring the generator cannot fall back to a personal path and
still passes the provided directory through the existing required_runtime_files
validation.
In `@env/patcheval/rule_evaluator.py`:
- Around line 53-75: Apply request.env_params["rule_evaluator_timeout_s"] to the
asyncio.to_thread call that runs evaluation.run_evaluation, using an async
timeout mechanism and preserving the existing EvalResult.failed response for
timeout failures. Ensure timeout handling reports that the official PatchEval
evaluation exceeded the configured limit, while retaining the current exception
handling for other evaluation errors.
In `@rl/examples/patcheval/run_eval.sh`:
- Line 11: Update the default PATCH_EVAL_API_BASE configuration so Gateway never
sends PATCH_EVAL_API_KEY to a cleartext endpoint by default. Use the HTTPS base
URL, or require an explicit opt-in before permitting an http:// value, while
preserving the existing API key forwarding behavior for secure endpoints.
- Around line 24-25: Stop defaulting GATEWAY_HOST to the first address returned
by hostname -I; require it explicitly or derive it using a known Docker-routable
address. Update the related guidance at rl/examples/patcheval/run_eval.sh lines
24-25 and 145, and rl/examples/patcheval/README.md line 165 so all documented
and runtime behavior uses the same safe configuration.
---
Nitpick comments:
In `@tests/test_patcheval_rule_evaluator.py`:
- Around line 35-73: Extend the tests around evaluate_rule to cover missing
cve_id, patch, or language returning EvalResult.failed, poc_passed=None
representing container startup failure, and run_evaluation raising an exception.
Assert each failure’s status and reason, including the existing
validation-failure case, to ensure reasons do not incorrectly report success.
🪄 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: 88a7c8b1-ad0d-49dd-bdc8-b645afa8604d
📒 Files selected for processing (7)
env/patcheval/generate_full_config.pyenv/patcheval/rule_evaluator.pyenv/patcheval/strict_runner.pyrl/examples/patcheval/README.mdrl/examples/patcheval/env.shrl/examples/patcheval/run_eval.shtests/test_patcheval_rule_evaluator.py
💤 Files with no reviewable changes (1)
- rl/examples/patcheval/env.sh
| DEFAULT_OFFICIAL_RUNTIME = Path( | ||
| "/mnt/shared-storage-user/evobox-share/leishanzhe/dataset/patcheval-runtime" | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hardcoded personal path as a CLI default.
DEFAULT_OFFICIAL_RUNTIME bakes in what looks like a specific engineer's home-mount path (.../leishanzhe/dataset/patcheval-runtime). Anyone running this generator without explicitly passing --official-runtime-dir will hit the required_runtime_files failure (Line 251-259) on any machine other than the original author's.
🔧 Suggested fix
-DEFAULT_OFFICIAL_RUNTIME = Path(
- "/mnt/shared-storage-user/evobox-share/leishanzhe/dataset/patcheval-runtime"
-)
+DEFAULT_OFFICIAL_RUNTIME = Path(
+ os.environ.get("PATCHEVAL_RUNTIME_DIR", "/opt/patcheval-runtime")
+)Or simply make --official-runtime-dir a required argument instead of defaulting to a machine-specific path.
📝 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.
| DEFAULT_OFFICIAL_RUNTIME = Path( | |
| "/mnt/shared-storage-user/evobox-share/leishanzhe/dataset/patcheval-runtime" | |
| ) | |
| DEFAULT_OFFICIAL_RUNTIME = Path( | |
| os.environ.get("PATCHEVAL_RUNTIME_DIR", "/opt/patcheval-runtime") | |
| ) |
🤖 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/generate_full_config.py` around lines 36 - 38, Remove the
machine-specific default from DEFAULT_OFFICIAL_RUNTIME and make
--official-runtime-dir an explicit required CLI argument, ensuring the generator
cannot fall back to a personal path and still passes the provided directory
through the existing required_runtime_files validation.
| try: | ||
| evaluation_class = _load_official_evaluation(request.env_params) | ||
| evaluation = evaluation_class( | ||
| logger=logging.getLogger(f"safactory.patcheval.official.{cve_id.lower()}"), | ||
| cve=cve_id, | ||
| ) | ||
| poc_passed, poc_log, unit_tests_passed, unit_test_log, validation_type = await asyncio.to_thread( | ||
| evaluation.run_evaluation, | ||
| cve_id, | ||
| patch, | ||
| language, | ||
| f"safactory_{request.session_id.replace('-', '_')}", | ||
| [], | ||
| ) | ||
| except Exception as exc: | ||
| return EvalResult.failed( | ||
| session_id=request.session_id, | ||
| eval_id=spec.eval_id, | ||
| method=spec.method.value, | ||
| reason="official PatchEval evaluation raised an exception", | ||
| error_text=str(exc), | ||
| artifacts={"bench": "patcheval", "cve_id": cve_id, "patch": patch}, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
rule_evaluator_timeout_s is generated but never applied — the blocking evaluation call has no timeout.
generate_full_config.py sets env_params["rule_evaluator_timeout_s"] specifically for this evaluator (default 3600s, CLI-configurable), but evaluate_rule never reads that key. await asyncio.to_thread(evaluation.run_evaluation, ...) has no timeout at all — a hung/blocked official Docker evaluation will hang this coroutine indefinitely, and even if an outer caller later cancels it, the underlying thread can't be forcibly stopped (Python threads aren't killable), so the run continues consuming resources in the background.
🔧 Suggested fix: enforce the generated timeout
try:
evaluation_class = _load_official_evaluation(request.env_params)
evaluation = evaluation_class(
logger=logging.getLogger(f"safactory.patcheval.official.{cve_id.lower()}"),
cve=cve_id,
)
- poc_passed, poc_log, unit_tests_passed, unit_test_log, validation_type = await asyncio.to_thread(
- evaluation.run_evaluation,
- cve_id,
- patch,
- language,
- f"safactory_{request.session_id.replace('-', '_')}",
- [],
- )
+ timeout_s = float(request.env_params.get("rule_evaluator_timeout_s") or spec.timeout_s)
+ poc_passed, poc_log, unit_tests_passed, unit_test_log, validation_type = await asyncio.wait_for(
+ asyncio.to_thread(
+ evaluation.run_evaluation,
+ cve_id,
+ patch,
+ language,
+ f"safactory_{request.session_id.replace('-', '_')}",
+ [],
+ ),
+ timeout=timeout_s,
+ )
+ except asyncio.TimeoutError as exc:
+ return EvalResult.failed(
+ session_id=request.session_id,
+ eval_id=spec.eval_id,
+ method=spec.method.value,
+ reason="official PatchEval evaluation timed out",
+ error_text=str(exc),
+ artifacts={"bench": "patcheval", "cve_id": cve_id, "patch": patch},
+ )
except Exception as exc:📝 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.
| try: | |
| evaluation_class = _load_official_evaluation(request.env_params) | |
| evaluation = evaluation_class( | |
| logger=logging.getLogger(f"safactory.patcheval.official.{cve_id.lower()}"), | |
| cve=cve_id, | |
| ) | |
| poc_passed, poc_log, unit_tests_passed, unit_test_log, validation_type = await asyncio.to_thread( | |
| evaluation.run_evaluation, | |
| cve_id, | |
| patch, | |
| language, | |
| f"safactory_{request.session_id.replace('-', '_')}", | |
| [], | |
| ) | |
| except Exception as exc: | |
| return EvalResult.failed( | |
| session_id=request.session_id, | |
| eval_id=spec.eval_id, | |
| method=spec.method.value, | |
| reason="official PatchEval evaluation raised an exception", | |
| error_text=str(exc), | |
| artifacts={"bench": "patcheval", "cve_id": cve_id, "patch": patch}, | |
| ) | |
| try: | |
| evaluation_class = _load_official_evaluation(request.env_params) | |
| evaluation = evaluation_class( | |
| logger=logging.getLogger(f"safactory.patcheval.official.{cve_id.lower()}"), | |
| cve=cve_id, | |
| ) | |
| timeout_s = float(request.env_params.get("rule_evaluator_timeout_s") or spec.timeout_s) | |
| poc_passed, poc_log, unit_tests_passed, unit_test_log, validation_type = await asyncio.wait_for( | |
| asyncio.to_thread( | |
| evaluation.run_evaluation, | |
| cve_id, | |
| patch, | |
| language, | |
| f"safactory_{request.session_id.replace('-', '_')}", | |
| [], | |
| ), | |
| timeout=timeout_s, | |
| ) | |
| except asyncio.TimeoutError as exc: | |
| return EvalResult.failed( | |
| session_id=request.session_id, | |
| eval_id=spec.eval_id, | |
| method=spec.method.value, | |
| reason="official PatchEval evaluation timed out", | |
| error_text=str(exc), | |
| artifacts={"bench": "patcheval", "cve_id": cve_id, "patch": patch}, | |
| ) | |
| except Exception as exc: | |
| return EvalResult.failed( | |
| session_id=request.session_id, | |
| eval_id=spec.eval_id, | |
| method=spec.method.value, | |
| reason="official PatchEval evaluation raised an exception", | |
| error_text=str(exc), | |
| artifacts={"bench": "patcheval", "cve_id": cve_id, "patch": patch}, | |
| ) |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 67-67: 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/rule_evaluator.py` around lines 53 - 75, Apply
request.env_params["rule_evaluator_timeout_s"] to the asyncio.to_thread call
that runs evaluation.run_evaluation, using an async timeout mechanism and
preserving the existing EvalResult.failed response for timeout failures. Ensure
timeout handling reports that the official PatchEval evaluation exceeded the
configured limit, while retaining the current exception handling for other
evaluation errors.
| : "${DOCKER_HOST:?Set DOCKER_HOST before running}" | ||
|
|
||
| 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 | 🟠 Major | ⚡ Quick win
Do not send the API key to a cleartext endpoint.
The default PATCH_EVAL_API_BASE uses http://, and lines 76-77 configure Gateway to send PATCH_EVAL_API_KEY to it. Use HTTPS by default, or require an explicit insecure opt-in.
🤖 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_eval.sh` at line 11, Update the default
PATCH_EVAL_API_BASE configuration so Gateway never sends PATCH_EVAL_API_KEY to a
cleartext endpoint by default. Use the HTTPS base URL, or require an explicit
opt-in before permitting an http:// value, while preserving the existing API key
forwarding behavior for secure endpoints.
| GATEWAY_HOST=${GATEWAY_HOST:-$(hostname -I | awk '{print $1}')} | ||
| PATCH_EVAL_NO_PROXY=${PATCH_EVAL_NO_PROXY:-host.docker.internal,localhost,127.0.0.1,::1,${GATEWAY_HOST},10.0.0.0/8,100.96.0.0/12,.pjlab.org.cn} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== run_eval.sh (relevant sections) ==\n'
sed -n '1,220p' rl/examples/patcheval/run_eval.sh | nl -ba | sed -n '1,220p'
printf '\n== README.md (relevant sections) ==\n'
sed -n '130,190p' rl/examples/patcheval/README.md | nl -ba | sed -n '1,120p'
printf '\n== Search for gateway URL usage ==\n'
rg -n "gateway-base-url|GATEWAY_HOST|host.docker.internal|127\.0\.0\.1:8000/v1/sessions|PATCH_EVAL_NO_PROXY" rl/examples/patcheval -SRepository: AI45Lab/SAfactory
Length of output: 234
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' rl/examples/patcheval/run_eval.sh | nl -ba | sed -n '1,220p'
sed -n '130,190p' rl/examples/patcheval/README.md | nl -ba | sed -n '1,120p'
rg -n "gateway-base-url|GATEWAY_HOST|host.docker.internal|127\.0\.0\.1:8000/v1/sessions|PATCH_EVAL_NO_PROXY" rl/examples/patcheval -SRepository: AI45Lab/SAfactory
Length of output: 195
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
def show(path, start, end):
print(f"\n== {path} {start}-{end} ==")
lines = Path(path).read_text().splitlines()
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4d}: {lines[i-1]}")
show("rl/examples/patcheval/run_eval.sh", 1, 220)
show("rl/examples/patcheval/README.md", 130, 190)
print("\n== Search ==\n")
import subprocess, shlex
cmd = r"rg -n \"gateway-base-url|GATEWAY_HOST|host.docker.internal|127\.0\.0\.1:8000/v1/sessions|PATCH_EVAL_NO_PROXY\" rl/examples/patcheval -S"
subprocess.run(cmd, shell=True, check=False)
PYRepository: AI45Lab/SAfactory
Length of output: 10198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
def find_files(root, needle):
for p in Path(root).rglob("*"):
if p.is_file():
try:
text = p.read_text(errors="ignore")
except Exception:
continue
if needle in text:
print(p)
print("== matches for gateway-base-url ==")
find_files(".", "gateway-base-url")
print("\n== matches for GATEWAY_HOST ==")
find_files(".", "GATEWAY_HOST")
PYRepository: AI45Lab/SAfactory
Length of output: 790
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== args.py: gateway-base-url definition and related args ==\n'
python3 - <<'PY'
from pathlib import Path
path = Path("args.py")
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if "gateway-base-url" in line or "gateway" in line.lower() and "base" in line.lower():
start = max(1, i-12)
end = min(len(lines), i+20)
print(f"\n-- {path} {start}-{end} --")
for j in range(start, end + 1):
print(f"{j:4d}: {lines[j-1]}")
PY
printf '\n== docs/gateway.md relevant sections ==\n'
python3 - <<'PY'
from pathlib import Path
path = Path("docs/gateway.md")
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if "gateway-base-url" in line or "127.0.0.1" in line or "docker" in line.lower():
start = max(1, i-10)
end = min(len(lines), i+25)
print(f"\n-- {path} {start}-{end} --")
for j in range(start, end + 1):
print(f"{j:4d}: {lines[j-1]}")
PY
printf '\n== rl/gateway_autostart.py relevant sections ==\n'
python3 - <<'PY'
from pathlib import Path
path = Path("rl/gateway_autostart.py")
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if "GATEWAY_HOST" in line or "gateway" in line.lower() and "url" in line.lower():
start = max(1, i-12)
end = min(len(lines), i+20)
print(f"\n-- {path} {start}-{end} --")
for j in range(start, end + 1):
print(f"{j:4d}: {lines[j-1]}")
PYRepository: AI45Lab/SAfactory
Length of output: 12176
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== definitions/usages of gateway-base-url ==\n'
rg -n "gateway-base-url|gateway_base_url|GATEWAY_BASE_URL" args.py launcher.py rl docs README.md README_CN.md rl -S
printf '\n== launcher.py around gateway handling ==\n'
python3 - <<'PY'
from pathlib import Path
path = Path("launcher.py")
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if "gateway" in line.lower():
start = max(1, i-12)
end = min(len(lines), i+20)
print(f"\n-- {path} {start}-{end} --")
for j in range(start, end + 1):
print(f"{j:4d}: {lines[j-1]}")
PY
printf '\n== args.py around gateway options ==\n'
python3 - <<'PY'
from pathlib import Path
path = Path("args.py")
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if "gateway" in line.lower():
start = max(1, i-12)
end = min(len(lines), i+20)
print(f"\n-- {path} {start}-{end} --")
for j in range(start, end + 1):
print(f"{j:4d}: {lines[j-1]}")
PYRepository: AI45Lab/SAfactory
Length of output: 19580
Avoid auto-selecting the first local IP. hostname -I | awk '{print $1}' can pick an address the Docker containers can’t reach on multi-homed hosts or some DOCKER_HOST setups; make GATEWAY_HOST explicit or derive a known routable host address.
📍 Affects 2 files
rl/examples/patcheval/run_eval.sh#L24-L25(this comment)rl/examples/patcheval/run_eval.sh#L145-L145rl/examples/patcheval/README.md#L165-L165
🤖 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_eval.sh` around lines 24 - 25, Stop defaulting
GATEWAY_HOST to the first address returned by hostname -I; require it explicitly
or derive it using a known Docker-routable address. Update the related guidance
at rl/examples/patcheval/run_eval.sh lines 24-25 and 145, and
rl/examples/patcheval/README.md line 165 so all documented and runtime behavior
uses the same safe configuration.
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests