Cvefactory - #21
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a LiveCVEBench/PatchEval dataset, container and environment configuration, a Terminal-Bench execution runner with result artifacts, and a rule evaluator that normalizes benchmark scores. ChangesLiveCVEBench adapter
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SimulationRequest
participant runner.py
participant tb_run
participant results.json
SimulationRequest->>runner.py: provide task and suite request
runner.py->>tb_run: execute selected task
tb_run->>results.json: write metadata and trial results
runner.py->>results.json: read resolution and score data
runner.py->>SimulationRequest: emit SAFACTORY_RESULT_JSON result
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (9)
env/livecvebench/rule_evaluator.py-30-30 (1)
30-30: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject non-finite scores before clamping.
float("nan")reaches this expression and normalizes to1.0, producing a perfect reward from invalid metrics. Reject non-finite values withmath.isfinite(score)before normalization.Proposed fix
+import math + - score = max(0.0, min(1.0, score)) + if not math.isfinite(score): + return EvalResult.failed( + session_id=request.session_id, + eval_id=spec.eval_id, + method=spec.method.value, + reason="LiveCVEBench score must be finite", + artifacts={"bench": "livecvebench", "metrics": metrics}, + ) + score = max(0.0, min(1.0, score))🤖 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/livecvebench/rule_evaluator.py` at line 30, Update the score normalization in the rule evaluator to validate score with math.isfinite(score) before applying the existing max/min clamp. Reject non-finite values, including NaN and infinities, rather than allowing them to produce a normalized reward.rl/examples/deepeyes/env.sh-55-60 (1)
55-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winForward
RL_OFF_BY_Nto the Slime runtime.
run_slime_generator.shexportsSLIME_OFF_BY_N, but this config only definesRL_OFF_BY_N; changing the latter has no effect on the submitted Ray job.Proposed fix
export SLIME_ROLLBUF_RESTART_TRAINING=True export SLIME_N_SAMPLES_PER_PROMPT=$RL_GROUP_SIZE +export SLIME_OFF_BY_N=$RL_OFF_BY_N # train batch size🤖 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/deepeyes/env.sh` around lines 55 - 60, Update the environment configuration near SLIME_ROLLOUT_BATCH_SIZE to forward RL_OFF_BY_N to the Slime runtime by exporting SLIME_OFF_BY_N from its value. Preserve the existing rollout batch-size configuration and ensure changes to RL_OFF_BY_N are reflected in the submitted Ray job.rl/examples/deepeyes/run_buffer_server.sh-15-16 (1)
15-16: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the configured Buffer Server port.
Overriding
BUFFER_SERVER_PORTchanges the generator’s target but leaves this server onROLLBUF_PORT’s hard-coded default, so rollout requests fail.Proposed fix
export ROLLBUF_HOST="${ROLLBUF_HOST:-0.0.0.0}" -export ROLLBUF_PORT="${ROLLBUF_PORT:-18889}" +export ROLLBUF_PORT="${ROLLBUF_PORT:-${BUFFER_SERVER_PORT}}"🤖 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/deepeyes/run_buffer_server.sh` around lines 15 - 16, Update the port configuration in run_buffer_server.sh so the Buffer Server uses the configured BUFFER_SERVER_PORT value, including its existing default when unset, instead of relying on ROLLBUF_PORT. Keep the ROLLBUF_HOST configuration unchanged.rl/examples/math500/run_slime_generator_opd_sglang.sh-201-216 (1)
201-216: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve argument boundaries in the Ray submit command. Unquoted
${...[@]}expansions can split values with spaces or glob characters; quote each array expansion here.🤖 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/math500/run_slime_generator_opd_sglang.sh` around lines 201 - 216, Update the Ray job submit command in the train_async invocation to quote every array expansion, including MODEL_ARGS, CKPT_ARGS, ROLLOUT_ARGS, OPTIMIZER_ARGS, GRPO_ARGS, WANDB_ARGS, PERF_ARGS, SGLANG_ARGS, MISC_ARGS, and TEACHER_ARGS, preserving each argument’s boundaries and preventing glob expansion.Source: Linters/SAST tools
rl/examples/geo3k_vl/env.sh-37-39 (1)
37-39: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winRemove the hard-coded credential-shaped placeholder.
RL_API_KEY=openai_api_keyis committed in an executable environment file. Even if currently unused, remove it or read it from injected runtime configuration so future code cannot accidentally treat the literal as a credential.🤖 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 37 - 39, Remove the hard-coded RL_API_KEY assignment from the environment configuration, or replace it with a runtime-injected value without any literal placeholder. Keep the existing RL_MODEL setting unchanged.Source: Linters/SAST tools
rl/examples/geo3k_vl/run_slime_generator.sh-25-25 (1)
25-25: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace the typoed buffering env var
PYTHONBUFFERED=16has no effect for Python. UsePYTHONUNBUFFERED=1if unbuffered output is intended, or remove the export.🤖 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/run_slime_generator.sh` at line 25, Replace the ineffective PYTHONBUFFERED export in run_slime_generator.sh with PYTHONUNBUFFERED=1 to enable unbuffered Python output, or remove the export if buffering is intended.rl/examples/geo3k_vl/run_slime_generator.sh-38-55 (1)
38-55: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winQuote the paths and array expansions passed to Ray. Unquoted
${HF_CKPT_DIR},${SAVE_DIR},${MASTER_ADDR}, and${ARRAY[@]}expansions are still subject to word splitting and globbing, so paths containing spaces or*can be misparsed. Use quoted scalar expansions and"${...[@]}"throughout.🤖 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/run_slime_generator.sh` around lines 38 - 55, Quote all scalar path and address expansions passed through the script, including HF_CKPT_DIR, SAVE_DIR, and MASTER_ADDR, to prevent word splitting and globbing; when expanding argument arrays such as CKPT_ARGS and ROLLOUT_ARGS, use the quoted `"${ARRAY[@]}"` form throughout the Ray invocation.Source: Linters/SAST tools
rl/examples/search/run_slime_generator.sh-25-25 (1)
25-25: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
PYTHONUNBUFFEREDherePYTHONBUFFEREDisn’t used elsewhere in the repo, and the existing runner code setsPYTHONUNBUFFERED=1; this should likely beexport PYTHONUNBUFFERED=1or removed if buffering isn’t needed.🤖 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/search/run_slime_generator.sh` at line 25, Update the environment variable export in run_slime_generator.sh from PYTHONBUFFERED to PYTHONUNBUFFERED, using the existing value of 1 so Python output buffering is configured consistently with the runner code.rl/examples/search/run_slime_generator.sh-33-50 (1)
33-50: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve shell argument boundaries. Quote the variable expansions in
CKPT_ARGSandROLLOUT_ARGS, expand every array as"${...[@]}", and quotepython3 "${SLIME_HOME}"/train.pyin the submit call too. Also apply the same fix at 124-142.🤖 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/search/run_slime_generator.sh` around lines 33 - 50, Preserve shell argument boundaries in the CKPT_ARGS and ROLLOUT_ARGS definitions by quoting all variable expansions, expand both arrays with "${...[@]}" wherever they are passed, and quote the train script invocation as python3 "${SLIME_HOME}"/train.py in the submit call. Apply the same quoting changes to the corresponding block around the later referenced section.Source: Linters/SAST tools
🧹 Nitpick comments (2)
rl/examples/deepeyes/run_slime_generator.sh (1)
33-50: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winQuote dynamic array values and expansions.
Unquoted values can split or glob when paths/configuration contain whitespace or glob characters; unquoted
"${ARRAY[@]}"expansions re-split submitted arguments.Proposed fix
- --hf-checkpoint ${HF_CKPT_DIR} - --load ${HF_CKPT_DIR} - --save ${SAVE_DIR} + --hf-checkpoint "${HF_CKPT_DIR}" + --load "${HF_CKPT_DIR}" + --save "${SAVE_DIR}" ... - --rollout-buffer-url ${ROLLOUT_BUFFER_URL} + --rollout-buffer-url "${ROLLOUT_BUFFER_URL}" ... - ${MODEL_ARGS[@]} \ - ${MEGATRON_ARGS[@]} \ - ${CKPT_ARGS[@]} \ - ${ROLLOUT_ARGS[@]} \ - ${OPTIMIZER_ARGS[@]} \ - ${GRPO_ARGS[@]} \ - ${WANDB_ARGS[@]} \ - ${TRAIN_ARGS[@]} \ - ${SGLANG_ARGS[@]} + "${MODEL_ARGS[@]}" \ + "${MEGATRON_ARGS[@]}" \ + "${CKPT_ARGS[@]}" \ + "${ROLLOUT_ARGS[@]}" \ + "${OPTIMIZER_ARGS[@]}" \ + "${GRPO_ARGS[@]}" \ + "${WANDB_ARGS[@]}" \ + "${TRAIN_ARGS[@]}" \ + "${SGLANG_ARGS[@]}"Also applies to: 129-143
🤖 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/deepeyes/run_slime_generator.sh` around lines 33 - 50, Quote all variable expansions in the CKPT_ARGS and ROLLOUT_ARGS arrays, including HF_CKPT_DIR, SAVE_DIR, ROLLOUT_BUFFER_URL, SLIME_ROLLOUT_BATCH_SIZE, SLIME_N_SAMPLES_PER_PROMPT, LLM_MAX_LENGTH, LLM_TEMPERATURE, and SLIME_GLOBAL_BATCH_SIZE. Preserve each option/value pair while ensuring paths and configuration values remain single arguments without word splitting or glob expansion; apply the same change to the corresponding array block around the additionally referenced section.Source: Linters/SAST tools
rl/examples/geo3k_vl/run_slime_generator.sh (1)
26-26: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep
NUM_GPUSconsistent with the submitted resource request.
NUM_GPUSis configurable forray start, but the job always requests one actor GPU plus three rollout GPUs. Overrides below four can fail at submission, while larger values are silently underused. Either validate the fixed four-GPU requirement or derive the job resources fromNUM_GPUS.Also applies to: 139-142
🤖 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/run_slime_generator.sh` at line 26, Keep NUM_GPUS consistent with the fixed resource allocation in the job submission: validate that it is exactly four before invoking ray start and submitting the actor and rollout resources, or derive those resource values from NUM_GPUS. Ensure invalid overrides cannot submit mismatched resource requests.
🤖 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/livecvebench/livecvebench_start.yaml`:
- Around line 15-17: Replace the developer-specific LiveCVEBench paths with
documented configurable variables: update
env/livecvebench/livecvebench_start.yaml lines 15-17 to source the runs mount
from the configured host path; update env/livecvebench/README_CN.md lines 8-12
to use the configurable LiveCVEBench repository root, line 24 to reference the
caller’s Safactory checkout, and lines 41-43 to describe artifacts via the
configured runs-directory variable.
In `@rl/examples/deepeyes/run_slime_generator.sh`:
- Around line 6-12: Remove the broad “pkill -9 python” command from the process
cleanup sequence in run_slime_generator.sh, or replace it with a narrowly
targeted cleanup that only terminates processes owned by this script while
preserving the Buffer Server started by run_buffer_server.sh.
In `@rl/examples/geo3k_vl/env.sh`:
- Around line 33-35: The off-by-N setting is defined as RL_OFF_BY_N but is not
propagated to the Slime runtime. In rl/examples/geo3k_vl/env.sh lines 33-35,
export the launcher-consumed variable or add an explicit
RL_OFF_BY_N-to-SLIME_OFF_BY_N mapping; in
rl/examples/geo3k_vl/run_slime_generator.sh lines 126-133, use RL_OFF_BY_N when
constructing RUNTIME_ENV_JSON so the configured value reaches the runtime.
- Around line 4-6: Update the environment setup around AIEVOBOX_ROOT so it
preserves an existing value and otherwise derives the repository root
dynamically instead of assigning /root/Safactory. Keep AIEVOBOX_DB_URL based on
the resolved AIEVOBOX_ROOT so buffer-server, database, and evaluation paths
remain inside the checkout.
In `@rl/examples/geo3k_vl/run_buffer_server.sh`:
- Line 12: Update the PYTHONPATH assignment in run_buffer_server.sh to append
AIEVOBOX_ROOT without introducing a leading empty element when PYTHONPATH is
unset. Preserve existing PYTHONPATH entries when it is already defined.
In `@rl/examples/geo3k_vl/run_slime_generator.sh`:
- Around line 6-13: Update the startup cleanup block in run_slime_generator.sh
to avoid globally terminating unrelated Ray and SGLang workloads. Scope process
cleanup to instances owned by this run using identifiable process or launch
metadata, or gate the destructive cleanup behind an explicit opt-in while
preserving the required Buffer Server process.
In `@rl/examples/math500/env.sh`:
- Around line 15-22: Update the shim-directory setup around AIEVOBOX_SHIM_BIN to
create a unique per-run directory with mktemp -d and enforce mode 0700, instead
of using the predictable /tmp/aievobox-bin-shim path. Keep the symlink creation
and PATH-prepending behavior unchanged while ensuring subsequent python3 and ray
calls use only this private directory.
In `@rl/examples/math500/run_buffer_server.sh`:
- Line 22: Update the PYTHONPATH assignment in run_buffer_server.sh so it
appends AIEVOBOX_ROOT without introducing a leading colon when PYTHONPATH is
unset or empty. Preserve existing PYTHONPATH entries when present, while
ensuring the current working directory is never added implicitly.
In `@rl/examples/math500/run_slime_generator_opd_sglang.sh`:
- Around line 34-36: Remove the hard-coded TEACHER_URL assignment near the OPD
setup, preserving the value sourced from env.sh so caller overrides remain
effective when line 117 passes it as --rm-url.
In `@rl/examples/search/env.sh`:
- Around line 24-25: Remove the RL_API_KEY export from the environment setup,
leaving RL_MODEL unchanged; do not retain a hardcoded credential-shaped value,
and only reintroduce the variable later through an injected runtime secret
mechanism.
In `@rl/examples/search/run_buffer_server.sh`:
- Around line 15-21: Update the run_buffer_server.sh configuration and startup
logging to use BUFFER_SERVER_PORT, matching the variable consumed by
buffer_server.py, and remove the configurable ROLLBUF_HOST setting and host
advertisement because the server always binds 0.0.0.0. Preserve the existing
default port and database URL behavior.
In `@rl/examples/search/run_slime_generator.sh`:
- Around line 29-30: Update the HF_CKPT_DIR and SAVE_DIR configuration in the
launcher to read both values from the environment rather than assigning machine-
and user-specific defaults. Require each variable to be set before execution and
fail clearly when either is missing.
- Line 26: Update the Ray startup arguments to use the configurable NUM_GPUS
value instead of a hard-coded 8, and ensure the actor/rollout GPU allocation
validation uses that same configured total. Preserve the existing split behavior
while supporting hosts with different GPU counts.
- Around line 6-13: Replace the global pkill calls in the launcher cleanup block
with scoped process management: track processes started by this script and
terminate only those processes or their dedicated process groups. Preserve
cleanup of the launcher’s sglang, ray, and Python children without affecting the
Buffer Server or unrelated host jobs.
- Around line 118-125: The RUNTIME_ENV_JSON environment block must forward the
variables expected by rl/slime_generator.py. Add LLM_PROXY_PORT and RL_OFF_BY_N
using their existing shell values/defaults, and replace SLIME_OFF_BY_N with
RL_OFF_BY_N so rollout workers receive the configured off-by-N behavior.
In `@rl/README.md`:
- Around line 48-53: Align the documented environment-variable names with Buffer
Server’s implementation by updating the configuration sections in rl/README.md
lines 48-53 and rl/README_CN.md lines 47-52 to use AIEVOBOX_AGENT_CONFIG and
AIEVOBOX_AGENT_ROOT, matching the variables read by rl/buffer_server.py; make no
code change to Buffer Server.
- Around line 5-10: Align the documented configuration workflow with the Geo3K
launchers: update rl/README.md lines 5-10 and rl/README_CN.md lines 7-10 to
document the actual environment source, and update
rl/examples/geo3k_vl/run_buffer_server.sh lines 8-10 plus
rl/examples/geo3k_vl/run_slime_generator.sh lines 17-19 to load .env with
explicit precedence, or remove claims that editing .env configures these
launchers.
- Around line 12-46: The startup instructions in rl/README.md lines 12-46 and
rl/README_CN.md lines 12-45 must reflect the launcher dependency: document
starting Buffer Server before Slime training, or add an explicit readiness wait
before launching Slime. Apply the same corrected ordering and guidance in both
English and Chinese sections.
- Around line 64-70: Standardize the Buffer Server endpoint variables across all
affected sites: update rl/README.md lines 64-70 and rl/README_CN.md lines 63-68
to document the variables used consistently by clients and the launcher; update
rl/examples/geo3k_vl/env.sh lines 49-50 to preserve configurable host overrides
and map the client port to the server bind port; update
rl/examples/geo3k_vl/run_buffer_server.sh lines 15-16 to derive ROLLBUF_HOST and
ROLLBURF_PORT from the standardized Buffer Server variables, avoiding hard-coded
loopback values.
---
Minor comments:
In `@env/livecvebench/rule_evaluator.py`:
- Line 30: Update the score normalization in the rule evaluator to validate
score with math.isfinite(score) before applying the existing max/min clamp.
Reject non-finite values, including NaN and infinities, rather than allowing
them to produce a normalized reward.
In `@rl/examples/deepeyes/env.sh`:
- Around line 55-60: Update the environment configuration near
SLIME_ROLLOUT_BATCH_SIZE to forward RL_OFF_BY_N to the Slime runtime by
exporting SLIME_OFF_BY_N from its value. Preserve the existing rollout
batch-size configuration and ensure changes to RL_OFF_BY_N are reflected in the
submitted Ray job.
In `@rl/examples/deepeyes/run_buffer_server.sh`:
- Around line 15-16: Update the port configuration in run_buffer_server.sh so
the Buffer Server uses the configured BUFFER_SERVER_PORT value, including its
existing default when unset, instead of relying on ROLLBUF_PORT. Keep the
ROLLBUF_HOST configuration unchanged.
In `@rl/examples/geo3k_vl/env.sh`:
- Around line 37-39: Remove the hard-coded RL_API_KEY assignment from the
environment configuration, or replace it with a runtime-injected value without
any literal placeholder. Keep the existing RL_MODEL setting unchanged.
In `@rl/examples/geo3k_vl/run_slime_generator.sh`:
- Line 25: Replace the ineffective PYTHONBUFFERED export in
run_slime_generator.sh with PYTHONUNBUFFERED=1 to enable unbuffered Python
output, or remove the export if buffering is intended.
- Around line 38-55: Quote all scalar path and address expansions passed through
the script, including HF_CKPT_DIR, SAVE_DIR, and MASTER_ADDR, to prevent word
splitting and globbing; when expanding argument arrays such as CKPT_ARGS and
ROLLOUT_ARGS, use the quoted `"${ARRAY[@]}"` form throughout the Ray invocation.
In `@rl/examples/math500/run_slime_generator_opd_sglang.sh`:
- Around line 201-216: Update the Ray job submit command in the train_async
invocation to quote every array expansion, including MODEL_ARGS, CKPT_ARGS,
ROLLOUT_ARGS, OPTIMIZER_ARGS, GRPO_ARGS, WANDB_ARGS, PERF_ARGS, SGLANG_ARGS,
MISC_ARGS, and TEACHER_ARGS, preserving each argument’s boundaries and
preventing glob expansion.
In `@rl/examples/search/run_slime_generator.sh`:
- Line 25: Update the environment variable export in run_slime_generator.sh from
PYTHONBUFFERED to PYTHONUNBUFFERED, using the existing value of 1 so Python
output buffering is configured consistently with the runner code.
- Around line 33-50: Preserve shell argument boundaries in the CKPT_ARGS and
ROLLOUT_ARGS definitions by quoting all variable expansions, expand both arrays
with "${...[@]}" wherever they are passed, and quote the train script invocation
as python3 "${SLIME_HOME}"/train.py in the submit call. Apply the same quoting
changes to the corresponding block around the later referenced section.
---
Nitpick comments:
In `@rl/examples/deepeyes/run_slime_generator.sh`:
- Around line 33-50: Quote all variable expansions in the CKPT_ARGS and
ROLLOUT_ARGS arrays, including HF_CKPT_DIR, SAVE_DIR, ROLLOUT_BUFFER_URL,
SLIME_ROLLOUT_BATCH_SIZE, SLIME_N_SAMPLES_PER_PROMPT, LLM_MAX_LENGTH,
LLM_TEMPERATURE, and SLIME_GLOBAL_BATCH_SIZE. Preserve each option/value pair
while ensuring paths and configuration values remain single arguments without
word splitting or glob expansion; apply the same change to the corresponding
array block around the additionally referenced section.
In `@rl/examples/geo3k_vl/run_slime_generator.sh`:
- Line 26: Keep NUM_GPUS consistent with the fixed resource allocation in the
job submission: validate that it is exactly four before invoking ray start and
submitting the actor and rollout resources, or derive those resource values from
NUM_GPUS. Ensure invalid overrides cannot submit mismatched resource requests.
🪄 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: 7347370a-9ca2-43eb-9d9c-929ee8f22450
📒 Files selected for processing (26)
env/geo3k/geo3k_rule_eval.yamlenv/livecvebench/README_CN.mdenv/livecvebench/datasets/livecvebench_and_patcheval_verified.jsonlenv/livecvebench/livecvebench_config.yamlenv/livecvebench/livecvebench_start.yamlenv/livecvebench/rule_evaluator.pyenv/livecvebench/runner.pyresults/.gitkeeprl/README.mdrl/README_CN.mdrl/buffer_server.pyrl/examples/deepeyes/env.shrl/examples/deepeyes/run_buffer_server.shrl/examples/deepeyes/run_slime_generator.shrl/examples/geo3k_vl/env.shrl/examples/geo3k_vl/run_buffer_server.shrl/examples/geo3k_vl/run_slime_generator.shrl/examples/math500/__init__.pyrl/examples/math500/env.shrl/examples/math500/run_buffer_server.shrl/examples/math500/run_slime_generator_opd_sglang.shrl/examples/search/env.shrl/examples/search/run_buffer_server.shrl/examples/search/run_slime_generator.shrl/run_buffer_server.shrl/run_slime_generator.sh
💤 Files with no reviewable changes (2)
- rl/run_buffer_server.sh
- rl/run_slime_generator.sh
| # Kill existing processes | ||
| pkill -9 sglang || true | ||
| sleep 2 | ||
| ray stop --force || true | ||
| pkill -9 ray || true | ||
| # Don't kill all python processes to preserve buffer server | ||
| pkill -9 python || true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not kill the Buffer Server.
pkill -9 python also matches the python3 Buffer Server started by run_buffer_server.sh, despite the preceding comment. The generator then cannot start rollouts and retries requests indefinitely. Remove this command or target only processes this script owns.
🤖 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/deepeyes/run_slime_generator.sh` around lines 6 - 12, Remove the
broad “pkill -9 python” command from the process cleanup sequence in
run_slime_generator.sh, or replace it with a narrowly targeted cleanup that only
terminates processes owned by this script while preserving the Buffer Server
started by run_buffer_server.sh.
| export RL_GROUP_SIZE=8 | ||
| export RL_EPOCH=1000 | ||
| export RL_OFF_BY_N=0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The RL off-by-N configuration is silently discarded. The environment defines RL_OFF_BY_N, but the Slime runtime receives SLIME_OFF_BY_N.
rl/examples/geo3k_vl/env.sh#L33-L35: export the variable consumed by the launcher, or define an explicit mapping.rl/examples/geo3k_vl/run_slime_generator.sh#L126-L133: readRL_OFF_BY_Nwhen constructingRUNTIME_ENV_JSON.
📍 Affects 2 files
rl/examples/geo3k_vl/env.sh#L33-L35(this comment)rl/examples/geo3k_vl/run_slime_generator.sh#L126-L133
🤖 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 33 - 35, The off-by-N setting is
defined as RL_OFF_BY_N but is not propagated to the Slime runtime. In
rl/examples/geo3k_vl/env.sh lines 33-35, export the launcher-consumed variable
or add an explicit RL_OFF_BY_N-to-SLIME_OFF_BY_N mapping; in
rl/examples/geo3k_vl/run_slime_generator.sh lines 126-133, use RL_OFF_BY_N when
constructing RUNTIME_ENV_JSON so the configured value reaches the runtime.
| SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" &>/dev/null && pwd)" | ||
| source "${SCRIPT_DIR}/env.sh" | ||
|
|
||
| export PYTHONPATH="${PYTHONPATH:-}:${AIEVOBOX_ROOT}" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid an empty PYTHONPATH entry.
When PYTHONPATH is unset, this expands to :${AIEVOBOX_ROOT}; the empty element can add the current working directory to Python’s import path, allowing unintended modules to shadow application imports. Build the value without a leading empty element.
Proposed fix
-export PYTHONPATH="${PYTHONPATH:-}:${AIEVOBOX_ROOT}"
+export PYTHONPATH="${AIEVOBOX_ROOT}${PYTHONPATH:+:${PYTHONPATH}}"📝 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 PYTHONPATH="${PYTHONPATH:-}:${AIEVOBOX_ROOT}" | |
| export PYTHONPATH="${AIEVOBOX_ROOT}${PYTHONPATH:+:${PYTHONPATH}}" |
🤖 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/run_buffer_server.sh` at line 12, Update the PYTHONPATH
assignment in run_buffer_server.sh to append AIEVOBOX_ROOT without introducing a
leading empty element when PYTHONPATH is unset. Preserve existing PYTHONPATH
entries when it is already defined.
| RUNTIME_ENV_JSON="{\ | ||
| \"env_vars\": {\ | ||
| \"PYTHONPATH\": \"${SLIME_HOME}:${AIEVOBOX_ROOT}/rl:${AIEVOBOX_ROOT}:/root/Megatron-LM\",\ | ||
| \"CUDA_DEVICE_MAX_CONNECTIONS\": \"1\",\ | ||
| \"LLM_PROXY_URL\": \"${LLM_PROXY_URL}\",\ | ||
| \"ROLLOUT_BUFFER_URL\": \"${ROLLOUT_BUFFER_URL}\",\ | ||
| \"SLIME_OFF_BY_N\": \"${SLIME_OFF_BY_N:-0}\"\ | ||
| }\ |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Forward the environment variables consumed by rollout workers.
The supplied rl/slime_generator.py contract reads LLM_PROXY_PORT at import time and RL_OFF_BY_N during rollout. This runtime environment forwards neither and sends SLIME_OFF_BY_N instead, so Ray workers may fail during import or ignore the configured off-by-N behavior.
Proposed fix
"LLM_PROXY_URL": "${LLM_PROXY_URL}",\
+ "LLM_PROXY_PORT": "${LLM_PROXY_PORT}",\
"ROLLOUT_BUFFER_URL": "${ROLLOUT_BUFFER_URL}",\
- "SLIME_OFF_BY_N": "${SLIME_OFF_BY_N:-0}"\
+ "RL_OFF_BY_N": "${RL_OFF_BY_N}"\📝 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.
| RUNTIME_ENV_JSON="{\ | |
| \"env_vars\": {\ | |
| \"PYTHONPATH\": \"${SLIME_HOME}:${AIEVOBOX_ROOT}/rl:${AIEVOBOX_ROOT}:/root/Megatron-LM\",\ | |
| \"CUDA_DEVICE_MAX_CONNECTIONS\": \"1\",\ | |
| \"LLM_PROXY_URL\": \"${LLM_PROXY_URL}\",\ | |
| \"ROLLOUT_BUFFER_URL\": \"${ROLLOUT_BUFFER_URL}\",\ | |
| \"SLIME_OFF_BY_N\": \"${SLIME_OFF_BY_N:-0}\"\ | |
| }\ | |
| RUNTIME_ENV_JSON="{\ | |
| \"env_vars\": {\ | |
| \"PYTHONPATH\": \"${SLIME_HOME}:${AIEVOBOX_ROOT}/rl:${AIEVOBOX_ROOT}:/root/Megatron-LM\",\ | |
| \"CUDA_DEVICE_MAX_CONNECTIONS\": \"1\",\ | |
| \"LLM_PROXY_URL\": \"${LLM_PROXY_URL}\",\ | |
| \"LLM_PROXY_PORT\": \"${LLM_PROXY_PORT}\",\ | |
| \"ROLLOUT_BUFFER_URL\": \"${ROLLOUT_BUFFER_URL}\",\ | |
| \"RL_OFF_BY_N\": \"${RL_OFF_BY_N}\"\ | |
| }\ |
🤖 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/search/run_slime_generator.sh` around lines 118 - 125, The
RUNTIME_ENV_JSON environment block must forward the variables expected by
rl/slime_generator.py. Add LLM_PROXY_PORT and RL_OFF_BY_N using their existing
shell values/defaults, and replace SLIME_OFF_BY_N with RL_OFF_BY_N so rollout
workers receive the configured off-by-N behavior.
| ### 1. Configure environment variables | ||
|
|
||
| | Component | Role | | ||
| | --- | --- | | ||
| | `rl/examples/<env>/env.sh` | Environment and experiment configuration. Switch environments by sourcing or passing a different file. | | ||
| | `rl/run_buffer_server.sh` | Starts Buffer Server and the Safactory rollout runner. | | ||
| | `rl/run_slime_generator.sh` | Starts Ray, Slime training, SGLang rollout engines, and the Safactory rollout function. | | ||
| | `rl/buffer_server.py` | Starts rollout collection, reads completed trajectories from storage, groups samples, and serves them to Slime. | | ||
| | `rl/slime_generator.py` | Slime rollout function. It starts the LLM proxy, fetches trajectory groups, builds masks/rewards, and returns training samples. | | ||
| ```bash | ||
| cp .env.example .env | ||
| # Edit .env and fill in the real configuration | ||
| ``` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The documented .env workflow is disconnected from the provided Geo3K launchers. Both launchers source env.sh directly, so .env edits are ignored unless an omitted wrapper loads it.
rl/README.md#L5-L10: document the actual configuration source or add.envloading.rl/README_CN.md#L7-L10: apply the same correction to the Chinese instructions.rl/examples/geo3k_vl/run_buffer_server.sh#L8-L10: load.envwith explicit precedence or stop claiming it configures this launcher.rl/examples/geo3k_vl/run_slime_generator.sh#L17-L19: apply the same configuration-loading behavior.
📍 Affects 4 files
rl/README.md#L5-L10(this comment)rl/README_CN.md#L7-L10rl/examples/geo3k_vl/run_buffer_server.sh#L8-L10rl/examples/geo3k_vl/run_slime_generator.sh#L17-L19
🤖 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/README.md` around lines 5 - 10, Align the documented configuration
workflow with the Geo3K launchers: update rl/README.md lines 5-10 and
rl/README_CN.md lines 7-10 to document the actual environment source, and update
rl/examples/geo3k_vl/run_buffer_server.sh lines 8-10 plus
rl/examples/geo3k_vl/run_slime_generator.sh lines 17-19 to load .env with
explicit precedence, or remove claims that editing .env configures these
launchers.
| ### Service ports | ||
|
|
||
| and the example-specific configuration is: | ||
| | Service | Default port | Environment variable | | ||
| |---------|--------------|----------------------| | ||
| | Buffer Server | 18889 | `BUFFER_SERVER_PORT` | | ||
| | LLM Proxy | 18890 | `LLM_PROXY_PORT` | | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The Buffer Server endpoint contract uses two incompatible variable pairs. Clients use BUFFER_SERVER_HOST/BUFFER_SERVER_PORT, while the server launcher binds ROLLBUF_HOST/ROLLBUF_PORT; the environment also hard-codes loopback, defeating the documented cross-machine setup.
rl/README.md#L64-L70: document the variables actually used by the server, or standardize the contract.rl/README_CN.md#L63-L68: mirror the corrected endpoint configuration.rl/examples/geo3k_vl/env.sh#L49-L50: preserve host overrides and map the client port to the server bind port.rl/examples/geo3k_vl/run_buffer_server.sh#L15-L16: deriveROLLBUF_*from the standardized Buffer Server variables.
📍 Affects 4 files
rl/README.md#L64-L70(this comment)rl/README_CN.md#L63-L68rl/examples/geo3k_vl/env.sh#L49-L50rl/examples/geo3k_vl/run_buffer_server.sh#L15-L16
🤖 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/README.md` around lines 64 - 70, Standardize the Buffer Server endpoint
variables across all affected sites: update rl/README.md lines 64-70 and
rl/README_CN.md lines 63-68 to document the variables used consistently by
clients and the launcher; update rl/examples/geo3k_vl/env.sh lines 49-50 to
preserve configurable host overrides and map the client port to the server bind
port; update rl/examples/geo3k_vl/run_buffer_server.sh lines 15-16 to derive
ROLLBUF_HOST and ROLLBURF_PORT from the standardized Buffer Server variables,
avoiding hard-coded loopback values.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/livecvebench/runner.py`:
- Around line 32-39: Validate the request-derived task_id immediately after
_required_text returns and before using it in dataset_path / task_id or the tb
command. Reject path-traversal values, including absolute paths and any value
whose normalized path escapes a single task-name component, while preserving
valid task IDs; use the validated value throughout the runner’s filesystem and
subprocess flow.
- Around line 96-101: Update _read_json to stop swallowing broad exceptions and
returning {} for unreadable or malformed results.json files; propagate the
underlying read/parse error so the runner surfaces a failure instead of scoring
the trial as unresolved. Preserve the existing valid JSON handling and metadata
fallback behavior.
- Around line 260-261: Update _safe_name to reject the exact reserved path
components "." and ".." by returning a safe non-traversal name instead. Preserve
the existing character sanitization and length limit for all other values used
as job_id or session_id.
- Around line 24-30: Move request parsing and session_id validation in main,
including _read_request and _required_text, inside the existing try/except so
malformed input or a missing session_id follows the same failure handling as
other errors. Preserve the always-emit behavior by ensuring the exception path
still produces SAFACTORY_RESULT_JSON and writes the RESULT_PATH_ENV artifact,
using a safe fallback session identifier if needed.
🪄 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: c3998a90-17e9-43de-9ad7-bc5a6d960b3b
📒 Files selected for processing (6)
env/livecvebench/README_CN.mdenv/livecvebench/datasets/livecvebench_and_patcheval_verified.jsonlenv/livecvebench/livecvebench_config.yamlenv/livecvebench/livecvebench_start.yamlenv/livecvebench/rule_evaluator.pyenv/livecvebench/runner.py
🚧 Files skipped from review as they are similar to previous changes (5)
- env/livecvebench/livecvebench_start.yaml
- env/livecvebench/livecvebench_config.yaml
- env/livecvebench/rule_evaluator.py
- env/livecvebench/README_CN.md
- env/livecvebench/datasets/livecvebench_and_patcheval_verified.jsonl
| task_id = _required_text( | ||
| dataset.get("task_id") or dataset.get("id") or env_params.get("task_id"), | ||
| "env_params.dataset.task_id", | ||
| ) | ||
| suite = _first_text(dataset.get("suite"), env_params.get("suite"), "livecvebench").lower() | ||
| dataset_path = _resolve_dataset_path(dataset, env_params, suite) | ||
| if not (dataset_path / task_id).is_dir(): | ||
| raise RuntimeError(f"{suite} task does not exist: {dataset_path / task_id}") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
task_id isn't validated against path traversal before being used to build a filesystem path and CLI arg.
task_id comes straight from the request (dataset.get("task_id")/env_params.get("task_id")) with only a non-empty check, then is joined onto dataset_path (Line 38) and forwarded verbatim to tb run --task-id (Lines 65-66). A value like "../other-suite-task" could resolve to a directory outside the intended dataset root and still pass the .is_dir() check, letting the runner execute tb against an unintended task directory. Note: since subprocess.run here uses a list (no shell=True), classic shell/OS command injection isn't feasible — the real risk from the flagged data flow is this path-traversal gap, not shell injection.
Proposed fix
- dataset_path = _resolve_dataset_path(dataset, env_params, suite)
- if not (dataset_path / task_id).is_dir():
- raise RuntimeError(f"{suite} task does not exist: {dataset_path / task_id}")
+ dataset_path = _resolve_dataset_path(dataset, env_params, suite)
+ task_dir = (dataset_path / task_id).resolve()
+ try:
+ task_dir.relative_to(dataset_path.resolve())
+ except ValueError:
+ raise RuntimeError(f"invalid task_id {task_id!r}: outside dataset path") from None
+ if not task_dir.is_dir():
+ raise RuntimeError(f"{suite} task does not exist: {task_dir}")As per static analysis hints, the ast-grep os-system-unsanitized-data/subprocess-from-request findings on Lines 80-89 point at this same unsanitized request-derived data flowing into the subprocess command.
Also applies to: 60-90
🤖 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/livecvebench/runner.py` around lines 32 - 39, Validate the
request-derived task_id immediately after _required_text returns and before
using it in dataset_path / task_id or the tb command. Reject path-traversal
values, including absolute paths and any value whose normalized path escapes a
single task-name component, while preserving valid task IDs; use the validated
value throughout the runner’s filesystem and subprocess flow.
Source: Linters/SAST tools
| resolved = [bool(item.get("is_resolved")) for item in trial_results] | ||
| score = ( | ||
| sum(1.0 for value in resolved if value) / len(resolved) | ||
| if resolved | ||
| else _float_or_default(metadata.get("accuracy"), 0.0) | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Unreadable/malformed results.json is silently treated as "not resolved" instead of a runner failure.
_read_json swallows any exception and returns {} (Lines 246-250); bool(item.get("is_resolved")) on that empty dict is False (Line 96). Since status is only marked "failed" when result_paths is empty (Line 111-113), a results.json that exists but fails to parse (corruption, permissions, partial write) is silently counted as a failed trial rather than surfaced as a read error, hiding a potential harness/tooling bug behind a benchmark score.
Suggested improvement
def _read_json(path: Path) -> dict[str, Any]:
try:
value = json.loads(path.read_text(encoding="utf-8"))
- except Exception:
+ except Exception as exc:
+ print(f"warning: failed to read {path}: {exc}", file=sys.stderr)
return {}
return value if isinstance(value, dict) else {}As per static analysis hints, Ruff's BLE001 on Line 248 flags this same blind except Exception.
Also applies to: 245-250
🤖 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/livecvebench/runner.py` around lines 96 - 101, Update _read_json to stop
swallowing broad exceptions and returning {} for unreadable or malformed
results.json files; propagate the underlying read/parse error so the runner
surfaces a failure instead of scoring the trial as unresolved. Preserve the
existing valid JSON handling and metadata fallback behavior.
Source: Linters/SAST tools
| def _safe_name(value: str) -> str: | ||
| return "".join(char if char.isalnum() or char in "._-" else "-" for char in value)[:100] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
_safe_name doesn't reject "."/"..", allowing directory traversal via job_id/session_id.
The sanitizer only replaces characters outside [A-Za-z0-9._-]; dots are preserved. A session_id/job_id of exactly ".." passes through unchanged, so output_dir = output_root / "safactory" / job_id / _safe_name(session_id) (Line 45) can escape the intended run directory.
Proposed fix
def _safe_name(value: str) -> str:
- return "".join(char if char.isalnum() or char in "._-" else "-" for char in value)[:100]
+ cleaned = "".join(char if char.isalnum() or char in "._-" else "-" for char in value)[:100]
+ if not cleaned or cleaned.strip(".") == "":
+ raise RuntimeError(f"unsafe identifier: {value!r}")
+ return cleaned📝 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 _safe_name(value: str) -> str: | |
| return "".join(char if char.isalnum() or char in "._-" else "-" for char in value)[:100] | |
| def _safe_name(value: str) -> str: | |
| cleaned = "".join(char if char.isalnum() or char in "._-" else "-" for char in value)[:100] | |
| if not cleaned or cleaned.strip(".") == "": | |
| raise RuntimeError(f"unsafe identifier: {value!r}") | |
| return cleaned |
🤖 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/livecvebench/runner.py` around lines 260 - 261, Update _safe_name to
reject the exact reserved path components "." and ".." by returning a safe
non-traversal name instead. Preserve the existing character sanitization and
length limit for all other values used as job_id or session_id.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
env/livecvebench/runner.py (1)
36-49: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
task_idstill not validated against path traversal before filesystem/CLI use.
task_idcomes straight from the request and is only checked for non-emptiness, then joined ontodataset_path(line 48) and forwarded verbatim totb run --task-id(line 69). A value like"../other-suite-task"can still resolve outside the intended dataset root while passing.is_dir().Proposed fix
dataset_path = _resolve_dataset_path(dataset, env_params, suite) - if not (dataset_path / task_id).is_dir(): - raise RuntimeError(f"{suite} task does not exist: {dataset_path / task_id}") + task_dir = (dataset_path / task_id).resolve() + try: + task_dir.relative_to(dataset_path.resolve()) + except ValueError: + raise RuntimeError(f"invalid task_id {task_id!r}: outside dataset path") from None + if not task_dir.is_dir(): + raise RuntimeError(f"{suite} task does not exist: {task_dir}")🤖 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/livecvebench/runner.py` around lines 36 - 49, Validate task_id in the runner flow before joining it with dataset_path or passing it to the CLI. Reject path-traversal or non-single-component values such as "." and "..", while preserving valid task identifiers, then use the validated value for the existing is_dir check and tb run invocation.
♻️ Duplicate comments (1)
env/livecvebench/runner.py (1)
26-34: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPartial fix only — request parsing still crashes uncaught before the try block.
Lines 31-33 now default
task_id/suite/output_dirso theexceptblocks won'tNameError, but the actual root cause flagged previously is untouched:_read_request()(line 27) and_required_text(request.get("session_id"), "session_id")(line 28) still run beforetry:(line 35). Malformed stdin JSON or a missingsession_idstill propagates out ofmain()uncaught — noSAFACTORY_RESULT_JSONprinted, no artifact written — exactly the scenario every other branch in this file is designed to avoid.Proposed fix
def main() -> int: started_at = time.perf_counter() - 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 {} task_id = "" suite = "" output_dir: Path | None = None - + 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 {} + task_id = _required_text(🤖 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/livecvebench/runner.py` around lines 26 - 34, Move request parsing in main, including _read_request() and _required_text(...), inside the existing try block so malformed JSON or missing session_id follows the established exception handling. Preserve the existing task_id, suite, and output_dir defaults so the except paths can emit SAFACTORY_RESULT_JSON and write artifacts without NameError.
🤖 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/livecvebench/runner.py`:
- Around line 147-172: Update _write_summary to handle failures from writing the
summary internally, preventing exceptions from propagating to its callers. Make
summary writing best-effort so every existing caller proceeds to
_write_result(result) and preserves the SAFACTORY_RESULT_JSON emission across
success, timeout, and generic-exception paths.
---
Outside diff comments:
In `@env/livecvebench/runner.py`:
- Around line 36-49: Validate task_id in the runner flow before joining it with
dataset_path or passing it to the CLI. Reject path-traversal or
non-single-component values such as "." and "..", while preserving valid task
identifiers, then use the validated value for the existing is_dir check and tb
run invocation.
---
Duplicate comments:
In `@env/livecvebench/runner.py`:
- Around line 26-34: Move request parsing in main, including _read_request() and
_required_text(...), inside the existing try block so malformed JSON or missing
session_id follows the established exception handling. Preserve the existing
task_id, suite, and output_dir defaults so the except paths can emit
SAFACTORY_RESULT_JSON and write artifacts without NameError.
🪄 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: b5b1a9bb-6fd0-42a9-8664-bc604b41bd60
📒 Files selected for processing (2)
env/livecvebench/README_CN.mdenv/livecvebench/runner.py
🚧 Files skipped from review as they are similar to previous changes (1)
- env/livecvebench/README_CN.md
| _write_summary(output_dir, result) | ||
| _write_result(result) | ||
| return 0 | ||
| except subprocess.TimeoutExpired as exc: | ||
| result = _failure_result( | ||
| session_id, | ||
| f"tb run timed out after {float(exc.timeout or 0):.1f}s", | ||
| started_at, | ||
| suite=suite, | ||
| task_id=task_id, | ||
| truncated=True, | ||
| ) | ||
| _write_summary(output_dir, result) | ||
| _write_result(result) | ||
| return 0 | ||
| except Exception as exc: | ||
| result = _failure_result( | ||
| session_id, | ||
| str(exc), | ||
| started_at, | ||
| suite=suite, | ||
| task_id=task_id, | ||
| ) | ||
| _write_summary(output_dir, result) | ||
| _write_result(result) | ||
| return 0 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
_write_summary failure inside the except-handlers defeats the "always emit a result" guarantee.
All three exit paths (147-149 success, 159-161 timeout, 170-172 generic exception) call _write_summary(output_dir, result) before _write_result(result). _write_summary has no internal error handling — if path.write_text(...) raises (disk full, permission error, etc.) while already inside the except subprocess.TimeoutExpired or except Exception handler, there's no further handler to catch it. The exception propagates out of main() uncaught, and the process exits without ever printing SAFACTORY_RESULT_JSON — the exact failure mode this file's exception handling is otherwise built to prevent.
Proposed fix
def _write_summary(output_dir: Path | None, result: dict[str, Any]) -> None:
if output_dir is None:
return
- path = output_dir / SUMMARY_FILENAME
- metrics = result.get("metrics")
- if isinstance(metrics, dict):
- metrics.setdefault("safactory_result_path", str(path))
- path.write_text(
- json.dumps(result, ensure_ascii=False, indent=2) + "\n",
- encoding="utf-8",
- )
+ try:
+ path = output_dir / SUMMARY_FILENAME
+ metrics = result.get("metrics")
+ if isinstance(metrics, dict):
+ metrics.setdefault("safactory_result_path", str(path))
+ path.write_text(
+ json.dumps(result, ensure_ascii=False, indent=2) + "\n",
+ encoding="utf-8",
+ )
+ except OSError as exc:
+ print(f"warning: failed to write summary: {exc}", file=sys.stderr)This localizes the fix to one place instead of guarding each of the three call sites.
Also applies to: 304-315
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 162-162: 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/livecvebench/runner.py` around lines 147 - 172, Update _write_summary to
handle failures from writing the summary internally, preventing exceptions from
propagating to its callers. Make summary writing best-effort so every existing
caller proceeds to _write_result(result) and preserves the SAFACTORY_RESULT_JSON
emission across success, timeout, and generic-exception paths.
| - livecvebench-runner-docker:/var/lib/docker | ||
|
|
||
| # Keep native Terminal-Bench artifacts on the host. | ||
| - source: /home/qiupanjia/code/LiveCVEBench-Preview-master/container-runs |
There was a problem hiding this comment.
should not use customized resources/pathes/dirs
There was a problem hiding this comment.
change the private path to some hints like or relative pathes
| @@ -0,0 +1,71 @@ | |||
| # LiveCVEBench adapter | |||
There was a problem hiding this comment.
replace this readme to a dockerfile which you used to build the bench docker images
| livecvebench: /benchmark/tasks/LiveCVEBench-verified | ||
| patcheval: /benchmark/tasks/PatchEval-verified | ||
| output_root: /benchmark/runs | ||
| agent: oracle |
There was a problem hiding this comment.
need claude code + opencode, bulit in the docker image
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
env/livecvebench/runner.py (2)
36-49: 🔒 Security & Privacy | 🔴 CriticalValidate
task_idbefore filesystem and CLI use.A request value such as
../other-taskor an absolute path can escapedataset_pathand still pass the directory check. Resolve the candidate task directory and require it to remain beneath the resolved dataset root before usingtask_id.Proposed fix
dataset_path = _resolve_dataset_path(dataset, env_params, suite) + dataset_root = dataset_path.resolve() + task_dir = (dataset_root / task_id).resolve() + try: + task_dir.relative_to(dataset_root) + except ValueError: + raise RuntimeError(f"invalid task_id {task_id!r}") from None output_root = Path( ... - if not (dataset_path / task_id).is_dir(): - raise RuntimeError(f"{suite} task does not exist: {dataset_path / task_id}") + if not task_dir.is_dir(): + raise RuntimeError(f"{suite} task does not exist: {task_dir}")Also applies to: 63-69
🤖 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/livecvebench/runner.py` around lines 36 - 49, Update the task-directory validation in the runner flow around _required_text and _resolve_dataset_path: resolve the dataset root and candidate task directory, then require the candidate to remain beneath the resolved dataset root before any filesystem or CLI use of task_id. Reject absolute paths and traversal such as ../other-task while preserving the existing missing-task RuntimeError behavior.
45-49: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUse a fresh attempt directory for each run.
output_diris deterministic and never cleaned, while_find_metadata()/_result_paths()scan the whole tree for the newestrun_metadata.json/results.json. A rerun can therefore score artifacts from a previous attempt if the new invocation doesn’t leave fresher files.🤖 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/livecvebench/runner.py` around lines 45 - 49, Update the output directory construction in the runner flow around _safe_name, output_dir, and _find_metadata()/_result_paths() so every invocation creates a unique fresh attempt directory beneath the job/session path. Preserve the existing job and session grouping, but add a per-run uniqueness component and ensure subsequent artifact scans are scoped to or correctly resolve the current attempt.
♻️ Duplicate comments (3)
env/livecvebench/runner.py (3)
27-30: 🩺 Stability & Availability | 🟠 MajorKeep request parsing inside the protected failure path.
_read_request()andsession_idvalidation still run beforetry, so malformed input or a missingsession_idcan terminatemain()without emittingSAFACTORY_RESULT_JSONor writing the result artifact. Move these operations insidetrywith a safe fallback session identifier.🤖 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/livecvebench/runner.py` around lines 27 - 30, Move _read_request(), session_id validation, and env_params/dataset extraction into main()’s protected try block, initializing a safe fallback session identifier before parsing begins. Ensure malformed input or missing session_id still reaches the existing failure handling, emits SAFACTORY_RESULT_JSON, and writes the result artifact.
148-172: 🩺 Stability & Availability | 🟡 MinorKeep summary writing best-effort in exception paths.
If
_write_summary()fails inside either exception handler, the exception propagates before_write_result()runs, breaking the always-emit contract. Catch write errors inside_write_summary()so failure reporting can continue.🤖 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/livecvebench/runner.py` around lines 148 - 172, Update both exception handlers in the runner flow around _failure_result, _write_summary, and _write_result so summary-writing failures are caught and suppressed, allowing _write_result(result) and the existing return path to execute. Keep summary output best-effort without changing the failure result contents or normal success-path behavior.
96-117: 🎯 Functional Correctness | 🟡 MinorTreat unreadable trial artifacts as runner failures.
A malformed or unreadable
results.jsonis converted to{}and can leavestatusas"succeeded"because the path exists. Distinguish a legitimate unresolved trial from a corrupted artifact and surface the read/parse failure.🤖 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/livecvebench/runner.py` around lines 96 - 117, Update the trial-results loading around `_read_json` so malformed or unreadable `results.json` artifacts are detected separately from valid unresolved trials. Mark the run as failed and populate `error_text` with the read or parse failure, while preserving successful handling of legitimate results whose `is_resolved` value is false; use the existing status/error flow in the runner.
🧹 Nitpick comments (1)
env/livecvebench/Dockerfile (1)
19-24: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin the build-time toolchain inputs.
Lines [19-24] use a moving installer script and
@latestnpm packages, so identical builds can silently receive different agent versions or fail later. Pin tested versions and verify the installer or base-image digest.🤖 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/livecvebench/Dockerfile` around lines 19 - 24, Pin the build-time inputs in the Dockerfile’s Node.js installation block: replace the moving NodeSource setup script with a verified version or checksum, pin the base image by digest if applicable, and replace both npm `@latest` dependencies with tested explicit versions. Preserve the existing installation flow while ensuring identical builds resolve the same toolchain.
🤖 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/livecvebench/Dockerfile`:
- Line 1: Update env/livecvebench/Dockerfile lines 1-46 to use a non-root or
rootless setup where feasible, preserving required benchmark functionality.
Update env/livecvebench/livecvebench_start.yaml lines 16-18 to document that the
privileged Docker-in-Docker runner must execute only on disposable VMs or
equivalently isolated workers.
In `@env/livecvebench/runner.py`:
- Around line 244-267: Update _docker_client_env and the surrounding tb
lifecycle so proxy credentials are written only to a private temporary Docker
config directory with restrictive permissions, not output_dir/.docker. Ensure
DOCKER_CONFIG points to that directory while tb runs, and remove the temporary
directory after tb exits, including failure paths.
---
Outside diff comments:
In `@env/livecvebench/runner.py`:
- Around line 36-49: Update the task-directory validation in the runner flow
around _required_text and _resolve_dataset_path: resolve the dataset root and
candidate task directory, then require the candidate to remain beneath the
resolved dataset root before any filesystem or CLI use of task_id. Reject
absolute paths and traversal such as ../other-task while preserving the existing
missing-task RuntimeError behavior.
- Around line 45-49: Update the output directory construction in the runner flow
around _safe_name, output_dir, and _find_metadata()/_result_paths() so every
invocation creates a unique fresh attempt directory beneath the job/session
path. Preserve the existing job and session grouping, but add a per-run
uniqueness component and ensure subsequent artifact scans are scoped to or
correctly resolve the current attempt.
---
Duplicate comments:
In `@env/livecvebench/runner.py`:
- Around line 27-30: Move _read_request(), session_id validation, and
env_params/dataset extraction into main()’s protected try block, initializing a
safe fallback session identifier before parsing begins. Ensure malformed input
or missing session_id still reaches the existing failure handling, emits
SAFACTORY_RESULT_JSON, and writes the result artifact.
- Around line 148-172: Update both exception handlers in the runner flow around
_failure_result, _write_summary, and _write_result so summary-writing failures
are caught and suppressed, allowing _write_result(result) and the existing
return path to execute. Keep summary output best-effort without changing the
failure result contents or normal success-path behavior.
- Around line 96-117: Update the trial-results loading around `_read_json` so
malformed or unreadable `results.json` artifacts are detected separately from
valid unresolved trials. Mark the run as failed and populate `error_text` with
the read or parse failure, while preserving successful handling of legitimate
results whose `is_resolved` value is false; use the existing status/error flow
in the runner.
---
Nitpick comments:
In `@env/livecvebench/Dockerfile`:
- Around line 19-24: Pin the build-time inputs in the Dockerfile’s Node.js
installation block: replace the moving NodeSource setup script with a verified
version or checksum, pin the base image by digest if applicable, and replace
both npm `@latest` dependencies with tested explicit versions. Preserve the
existing installation flow while ensuring identical builds resolve the same
toolchain.
🪄 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: e3c61d8a-c7bb-4cb5-bfeb-8a6937043981
📒 Files selected for processing (3)
env/livecvebench/Dockerfileenv/livecvebench/livecvebench_start.yamlenv/livecvebench/runner.py
| @@ -0,0 +1,50 @@ | |||
| FROM ubuntu:22.04 | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files env/livecvebench
printf '\n== Dockerfile ==\n'
cat -n env/livecvebench/Dockerfile
printf '\n== livecvebench_start.yaml ==\n'
cat -n env/livecvebench/livecvebench_start.yaml
printf '\n== Search for related isolation/docs ==\n'
rg -n --hidden -S "livecvebench|Docker-in-Docker|privileged|rootless|/var/lib/docker|runner image|Safactory|disposable" env README.md .github 2>/dev/null || trueRepository: AI45Lab/SAfactory
Length of output: 32471
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map nearby symbols/structure if files are larger than expected.
ast-grep outline env/livecvebench/Dockerfile --view expanded || true
ast-grep outline env/livecvebench/livecvebench_start.yaml --view expanded || trueRepository: AI45Lab/SAfactory
Length of output: 224
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read a bit more context around the startup config and any runner docs.
sed -n '1,220p' env/livecvebench/livecvebench_start.yaml
printf '\n==\n'
sed -n '1,120p' env/livecvebench/Dockerfile
printf '\n== Additional matches ==\n'
rg -n -S "Docker-in-Docker|/var/lib/docker|privileged|rootless|non-root|livecvebench_start" env .github README.md 2>/dev/null || trueRepository: AI45Lab/SAfactory
Length of output: 2834
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== Dockerfile =='
cat -n env/livecvebench/Dockerfile
echo
echo '== livecvebench_start.yaml =='
cat -n env/livecvebench/livecvebench_start.yaml
echo
echo '== Related references =='
rg -n -S "Docker-in-Docker|/var/lib/docker|privileged|rootless|non-root|livecvebench_start|Safactory|runner image" env .github README.md 2>/dev/null || trueRepository: AI45Lab/SAfactory
Length of output: 8494
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for p in [Path('env/livecvebench/Dockerfile'), Path('env/livecvebench/livecvebench_start.yaml')]:
print(f'== {p} ==')
try:
print(p.read_text())
except Exception as e:
print(e)
PYRepository: AI45Lab/SAfactory
Length of output: 2420
Keep this runner on isolated workers. env/livecvebench/Dockerfile runs as root and env/livecvebench/livecvebench_start.yaml launches with --privileged for Docker-in-Docker, so benchmark code should only run on disposable VMs or similarly isolated hosts.
env/livecvebench/Dockerfile#L1-L46: switch to a non-root/rootless setup if possible.env/livecvebench/livecvebench_start.yaml#L16-L20: call out the isolation requirement next to the privileged launch.
🧰 Tools
🪛 Trivy (0.72.0)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
📍 Affects 2 files
env/livecvebench/Dockerfile#L1-L1(this comment)env/livecvebench/livecvebench_start.yaml#L16-L18
🤖 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/livecvebench/Dockerfile` at line 1, Update env/livecvebench/Dockerfile
lines 1-46 to use a non-root or rootless setup where feasible, preserving
required benchmark functionality. Update
env/livecvebench/livecvebench_start.yaml lines 16-18 to document that the
privileged Docker-in-Docker runner must execute only on disposable VMs or
equivalently isolated workers.
Source: Linters/SAST tools
| def _docker_client_env(output_dir: Path) -> dict[str, str]: | ||
| env = dict(os.environ) | ||
| http_proxy = _first_text(env.get("HTTP_PROXY"), env.get("http_proxy")) | ||
| https_proxy = _first_text(env.get("HTTPS_PROXY"), env.get("https_proxy"), http_proxy) | ||
| if not http_proxy and not https_proxy: | ||
| return env | ||
|
|
||
| no_proxy = _first_text(env.get("NO_PROXY"), env.get("no_proxy")) | ||
| docker_config_dir = output_dir / ".docker" | ||
| docker_config_dir.mkdir(parents=True, exist_ok=True) | ||
| config = { | ||
| "proxies": { | ||
| "default": { | ||
| "httpProxy": http_proxy, | ||
| "httpsProxy": https_proxy, | ||
| "noProxy": no_proxy, | ||
| } | ||
| } | ||
| } | ||
| (docker_config_dir / "config.json").write_text( | ||
| json.dumps(config, ensure_ascii=False), | ||
| encoding="utf-8", | ||
| ) | ||
| env["DOCKER_CONFIG"] = str(docker_config_dir) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and nearby symbols first
ast-grep outline env/livecvebench/runner.py --view expanded || true
# Read the relevant section with line numbers
sed -n '200,340p' env/livecvebench/runner.py | cat -n
# Search for output_dir usage and any cleanup of .docker/config.json
rg -n "output_dir|DOCKER_CONFIG|config\.json|\.docker" env/livecvebench/runner.py env/livecvebench -SRepository: AI45Lab/SAfactory
Length of output: 9969
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,190p' env/livecvebench/runner.py | cat -n
# Look for any cleanup or temp-dir handling related to Docker config or output_dir
rg -n "TemporaryDirectory|mkdtemp|shutil\.rmtree|cleanup\(|DOCKER_CONFIG|config\.json|\.docker" env/livecvebench/runner.py -SRepository: AI45Lab/SAfactory
Length of output: 8618
Don't persist proxy credentials under the run artifacts. HTTP_PROXY/HTTPS_PROXY can include credentials, and this writes them to output_dir/.docker/config.json inside the persisted benchmark output tree. Use a private temp Docker config with restrictive permissions and remove it after tb exits.
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 263-263: use jsonify instead of json.dumps for JSON output
Context: json.dumps(config, ensure_ascii=False)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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/livecvebench/runner.py` around lines 244 - 267, Update _docker_client_env
and the surrounding tb lifecycle so proxy credentials are written only to a
private temporary Docker config directory with restrictive permissions, not
output_dir/.docker. Ensure DOCKER_CONFIG points to that directory while tb runs,
and remove the temporary directory after tb exits, including failure paths.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
env/livecvebench/livecvebench_start.rjob.yaml (2)
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an immutable image reference for reproducible benchmark runs.
IfNotPresentcan execute a stale cached image even when the datedenv_imagetag is repointed. Prefer a digest-pinned image, or useAlwayswhen mutable tags are intentional.🤖 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/livecvebench/livecvebench_start.rjob.yaml` at line 18, Update the image configuration in livecvebench_start to use an immutable digest-pinned image reference for reproducible runs; if the existing env_image tag must remain mutable, change image_pull_policy from IfNotPresent to Always.
21-22: 🩺 Stability & Availability | 🔵 TrivialDefine a retention and cleanup policy for completed jobs.
With
cleanup_on_finish: falseandkeep_failed_jobs: true, every run can leave job resources behind indefinitely. Preserve result artifacts on the mounted output path, but clean up finished jobs or enforce bounded retention to prevent resource accumulation.🤖 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/livecvebench/livecvebench_start.rjob.yaml` around lines 21 - 22, Update the job retention settings near cleanup_on_finish and keep_failed_jobs to define bounded cleanup for completed jobs, while preserving result artifacts on the mounted output path. Enable cleanup after completion or configure an equivalent finite retention policy, and avoid retaining failed jobs indefinitely.env/livecvebench/runner.py (1)
379-386: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
_with_log_tailloads the entire log file into memory just to keep the lastmax_chars.
log_path.read_text(...)reads the wholetb-run.log/dockerd.logbefore slicingtext[-max_chars:]. For a long-running/verbosetb run, this is an unbounded read relative to log size for a feature that only needs the tail.Suggested improvement
def _with_log_tail(message: str, log_path: Path, *, max_chars: int = 6000) -> str: try: - text = log_path.read_text(encoding="utf-8", errors="replace").strip() + with log_path.open("rb") as fh: + fh.seek(0, 2) + size = fh.tell() + fh.seek(max(0, size - max_chars * 4), 0) + text = fh.read().decode("utf-8", errors="replace").strip() except OSError: return message🤖 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/livecvebench/runner.py` around lines 379 - 386, Update _with_log_tail to retrieve only the final max_chars from log_path instead of calling read_text and loading the entire file; use bounded file seeking/reading while preserving UTF-8 replacement, whitespace stripping, empty-file handling, and the existing fallback message on OSError.
🤖 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/livecvebench/runner.py`:
- Around line 316-334: Update the timeout cleanup in _ensure_docker_daemon so
the process is reaped after proc.terminate(): wait for proc to exit before
raising the timeout RuntimeError, while preserving the existing log-tail message
and error behavior.
- Around line 337-345: Guard the post-kill wait in _stop_docker_daemon so a
second subprocess.TimeoutExpired does not escape during main() cleanup. Preserve
the existing terminate, timeout, kill, and successful wait behavior, but handle
a process that remains alive after the final wait without propagating an
exception.
---
Nitpick comments:
In `@env/livecvebench/livecvebench_start.rjob.yaml`:
- Line 18: Update the image configuration in livecvebench_start to use an
immutable digest-pinned image reference for reproducible runs; if the existing
env_image tag must remain mutable, change image_pull_policy from IfNotPresent to
Always.
- Around line 21-22: Update the job retention settings near cleanup_on_finish
and keep_failed_jobs to define bounded cleanup for completed jobs, while
preserving result artifacts on the mounted output path. Enable cleanup after
completion or configure an equivalent finite retention policy, and avoid
retaining failed jobs indefinitely.
In `@env/livecvebench/runner.py`:
- Around line 379-386: Update _with_log_tail to retrieve only the final
max_chars from log_path instead of calling read_text and loading the entire
file; use bounded file seeking/reading while preserving UTF-8 replacement,
whitespace stripping, empty-file handling, and the existing fallback message on
OSError.
🪄 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: 84282143-7bfc-4529-97a3-6ef087091f2d
📒 Files selected for processing (4)
env/livecvebench/Dockerfileenv/livecvebench/livecvebench_config.rjob.yamlenv/livecvebench/livecvebench_start.rjob.yamlenv/livecvebench/runner.py
🚧 Files skipped from review as they are similar to previous changes (1)
- env/livecvebench/Dockerfile
| for _ in range(60): | ||
| if proc.poll() is not None: | ||
| raise RuntimeError( | ||
| _with_log_tail("the inner Docker daemon failed to start", log_path) | ||
| ) | ||
| if subprocess.run( | ||
| ["docker", "info"], | ||
| env=env, | ||
| stdout=subprocess.DEVNULL, | ||
| stderr=subprocess.DEVNULL, | ||
| check=False, | ||
| ).returncode == 0: | ||
| return proc | ||
| time.sleep(1) | ||
|
|
||
| proc.terminate() | ||
| raise RuntimeError( | ||
| _with_log_tail("timed out waiting for the inner Docker daemon", log_path) | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Terminated dockerd process is never reaped on readiness timeout, leaving a zombie child.
When the 60s readiness loop times out, proc.terminate() (Line 331) is called but the process is never wait()-ed. Since _ensure_docker_daemon raises immediately afterward, the caller's dockerd_proc in main() is never assigned (the exception occurs before the return value reaches Line 83), so _stop_docker_daemon in the finally block also never reaps it — the terminated child lingers as a zombie until this process itself exits.
Proposed fix
proc.terminate()
+ try:
+ proc.wait(timeout=5)
+ except subprocess.TimeoutExpired:
+ proc.kill()
+ proc.wait(timeout=5)
raise RuntimeError(
_with_log_tail("timed out waiting for the inner Docker daemon", log_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.
| for _ in range(60): | |
| if proc.poll() is not None: | |
| raise RuntimeError( | |
| _with_log_tail("the inner Docker daemon failed to start", log_path) | |
| ) | |
| if subprocess.run( | |
| ["docker", "info"], | |
| env=env, | |
| stdout=subprocess.DEVNULL, | |
| stderr=subprocess.DEVNULL, | |
| check=False, | |
| ).returncode == 0: | |
| return proc | |
| time.sleep(1) | |
| proc.terminate() | |
| raise RuntimeError( | |
| _with_log_tail("timed out waiting for the inner Docker daemon", log_path) | |
| ) | |
| for _ in range(60): | |
| if proc.poll() is not None: | |
| raise RuntimeError( | |
| _with_log_tail("the inner Docker daemon failed to start", log_path) | |
| ) | |
| if subprocess.run( | |
| ["docker", "info"], | |
| env=env, | |
| stdout=subprocess.DEVNULL, | |
| stderr=subprocess.DEVNULL, | |
| check=False, | |
| ).returncode == 0: | |
| return proc | |
| time.sleep(1) | |
| proc.terminate() | |
| try: | |
| proc.wait(timeout=5) | |
| except subprocess.TimeoutExpired: | |
| proc.kill() | |
| proc.wait(timeout=5) | |
| raise RuntimeError( | |
| _with_log_tail("timed out waiting for the inner Docker daemon", log_path) | |
| ) |
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 320-326: Command coming from incoming request
Context: subprocess.run(
["docker", "info"],
env=env,
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.16.0)
[error] 322-322: Starting a process with a partial executable path
(S607)
🤖 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/livecvebench/runner.py` around lines 316 - 334, Update the timeout
cleanup in _ensure_docker_daemon so the process is reaped after
proc.terminate(): wait for proc to exit before raising the timeout RuntimeError,
while preserving the existing log-tail message and error behavior.
| def _stop_docker_daemon(proc: subprocess.Popen[str] | None) -> None: | ||
| if proc is None or proc.poll() is not None: | ||
| return | ||
| proc.terminate() | ||
| try: | ||
| proc.wait(timeout=15) | ||
| except subprocess.TimeoutExpired: | ||
| proc.kill() | ||
| proc.wait(timeout=5) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Second proc.wait() after kill() is not guarded, so it can re-raise TimeoutExpired uncaught out of main()'s finally.
If the process still hasn't exited 5s after SIGKILL (rare, e.g. stuck in uninterruptible I/O), proc.wait(timeout=5) at Line 344 raises subprocess.TimeoutExpired with no handler. Since this runs inside the finally at Lines 179-180, the exception overrides the already-computed return 0, crashing main() with an unhandled exception after the result JSON was already emitted — undermining the exit-code guarantee the rest of this file is built around.
Proposed fix
proc.terminate()
try:
proc.wait(timeout=15)
except subprocess.TimeoutExpired:
proc.kill()
- proc.wait(timeout=5)
+ try:
+ proc.wait(timeout=5)
+ except subprocess.TimeoutExpired:
+ pass📝 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 _stop_docker_daemon(proc: subprocess.Popen[str] | None) -> None: | |
| if proc is None or proc.poll() is not None: | |
| return | |
| proc.terminate() | |
| try: | |
| proc.wait(timeout=15) | |
| except subprocess.TimeoutExpired: | |
| proc.kill() | |
| proc.wait(timeout=5) | |
| def _stop_docker_daemon(proc: subprocess.Popen[str] | None) -> None: | |
| if proc is None or proc.poll() is not None: | |
| return | |
| proc.terminate() | |
| try: | |
| proc.wait(timeout=15) | |
| except subprocess.TimeoutExpired: | |
| proc.kill() | |
| try: | |
| proc.wait(timeout=5) | |
| except subprocess.TimeoutExpired: | |
| pass |
🤖 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/livecvebench/runner.py` around lines 337 - 345, Guard the post-kill wait
in _stop_docker_daemon so a second subprocess.TimeoutExpired does not escape
during main() cleanup. Preserve the existing terminate, timeout, kill, and
successful wait behavior, but handle a process that remains alive after the
final wait without propagating an exception.
Development branch for CVE-Factory benchmark and PatchEval tasks.
Summary by CodeRabbit