Conversation
Port the v1 geo3k_vl_test (core.env.BaseEnv, multi-turn VL geometry) onto
the v2 external-runtime contract. The runner now owns the multi-turn loop:
it calls the gateway session repeatedly, tracks messages, handles the
calc_score self-check tool, scores with the sympy grader, and prints one
result JSON.
- env/geo3k/runner.py: multi-turn loop, tool handling, turn-cap fallback,
<think> stripping, boxed extraction, image injection (ported from v1 step)
- env/geo3k/math_utils.py: sympy answer grader, verbatim from v1
- env/geo3k/rule_evaluator.py: metrics.score -> 0..10 reward
- env/geo3k/{geo3k_config,geo3k_start}.yaml + sample dataset
- evaluator/configs/geo3k_rule_eval.yaml
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The v2 launcher has no in-process local mode; every rollout runs the env in a docker/rjob container built from an agent start config. buffer_server was spawning launcher.py without --agent-start-config, so docker mode had no container startup definition (env_types) and the env runner never ran. - buffer_server: pass --agent-start-config, reading AIEVOBOX_AGENT_START_CONFIG or deriving it from AIEVOBOX_AGENT_CONFIG (_config.yaml -> _start.yaml); make --mode configurable via AIEVOBOX_MODE (docker/rjob). - examples/geo3k_vl/env.sh: migrate to v2 vars (AIEVOBOX_AGENT_CONFIG / AIEVOBOX_AGENT_START_CONFIG) pointing at env/geo3k; the old AIEVOBOX_ENV_CONFIG pointed at the deleted v1 geo3k_vl_test. Lower pool_size to a docker-appropriate default. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
For the two-layer RL topology docker -> gateway -> llm_proxy -> sglang, the llm_proxy needs the Safactory session id to bind training trajectories, but the gateway forwarded a plain /chat/completions and dropped it. Propagate the session id out-of-band via an X-Safactory-Session-Id header: - inference_forwarder.build_upstream_headers now injects the header when a session id is present; plain OpenAI upstreams ignore the unknown header, so no per-route flag is needed. - app.py passes ctx.session_id at the forward call site. - llm_proxy accepts the session id from the URL path (direct single-layer call) or the header (gateway-forwarded two-layer call), path taking precedence, so both topologies work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The gateway is now the mandatory entry point for RL rollouts
(docker -> gateway -> llm_proxy -> sglang). Remove the direct single-layer
path route so there is exactly one topology.
- llm_proxy: drop /v1/{session_id}/chat/completions; keep only
/v1/chat/completions reading the session id from X-Safactory-Session-Id.
Missing header -> 400 (gateway must front the proxy).
- examples/geo3k_vl/env.sh: add AIEVOBOX_GATEWAY_BASE_URL so the runner targets
the gateway session root; the gateway routes to the llm_proxy.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
In the two-layer RL topology the gateway must run on the launcher/DB machine, which buffer_server owns. Start it there instead of requiring a manual process. - rl/gateway_autostart.py: generate a gateway config from RL env vars (one route keyed by RL_MODEL -> llm_proxy /v1, storage sharing AIEVOBOX_DB_URL so it matches launcher --db-path, max_steps=-1), launch `python -m gateway`, wait for /readyz. Idempotent; disabled via AIEVOBOX_GATEWAY_AUTOSTART=0. - buffer_server: ensure the gateway is up before spawning the launcher (which validates gateway /readyz), and stop it on exit via atexit. - examples/geo3k_vl/env.sh: document the autostart toggle. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Do not submit test files; instead, merge the relevant test files for the geo3k environment into rl/example/geo3k_vl and include a README.
|
AIEVOBOX_GATEWAY_HOST 可不可以自动获取 |
There was a problem hiding this comment.
This file should not appeared in this folder.
remove this to env/geo3k folder
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds Geo3K runtime and evaluation support, parquet column projection with lazy row materialization, and session-aware RL gateway startup, forwarding, telemetry, and operational configuration. ChangesDataset projection and Geo3K runtime
Session-aware RL gateway integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BufferServer
participant Gateway
participant LLMProxy
participant Evaluator
BufferServer->>Gateway: start rollout with session configuration
Gateway->>LLMProxy: forward chat request with session header
LLMProxy-->>Gateway: return response and metadata
Gateway->>Evaluator: provide recorded trajectory data
Evaluator-->>BufferServer: produce rewarded training rows
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
rl/examples/geo3k_vl/env.sh (1)
71-73: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep Gateway settings overridable for external deployments.
Because
AIEVOBOX_GATEWAY_AUTOSTART=0supports a manually hosted Gateway, these unconditional exports overwrite caller-provided host/port values and hard-code the base URL to loopback. Host discovery is not reliable across containers or hosts; preserve explicit overrides and derive the default URL only when one was not supplied.Proposed fix
-export AIEVOBOX_GATEWAY_HOST=127.0.0.1 -export AIEVOBOX_GATEWAY_PORT=8000 -export AIEVOBOX_GATEWAY_BASE_URL=http://${AIEVOBOX_GATEWAY_HOST}:${AIEVOBOX_GATEWAY_PORT}/v1/sessions +export AIEVOBOX_GATEWAY_HOST="${AIEVOBOX_GATEWAY_HOST:-127.0.0.1}" +export AIEVOBOX_GATEWAY_PORT="${AIEVOBOX_GATEWAY_PORT:-8000}" +export AIEVOBOX_GATEWAY_BASE_URL="${AIEVOBOX_GATEWAY_BASE_URL:-http://${AIEVOBOX_GATEWAY_HOST}:${AIEVOBOX_GATEWAY_PORT}/v1/sessions}"🤖 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 - 73, Update the AIEVOBOX_GATEWAY_HOST, AIEVOBOX_GATEWAY_PORT, and AIEVOBOX_GATEWAY_BASE_URL exports in env.sh to preserve caller-provided values. Set host and port only when unset, and derive the default base URL from the resulting host and port only when AIEVOBOX_GATEWAY_BASE_URL is unset, allowing manually hosted external Gateways to override all settings.rl/examples/geo3k_vl/run_slime_generator.sh (1)
119-119: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winQuote the environment-controlled GPU count.
ShellCheck reports that the unquoted expansion can undergo word splitting or glob expansion. Use
--num-gpus "${NUM_GPUS}".🤖 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 119, Quote the NUM_GPUS expansion in the ray command’s --num-gpus argument to prevent shell word splitting and glob expansion, while preserving the existing command behavior.Source: Linters/SAST tools
env/geo3k/geo3k_config.yaml (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded absolute dataset path may break portability.
The dataset path points to a specific machine's shared storage (
/mnt/shared-storage-user/yinzhenyun/...). This will fail on any other host. Consider using a relative path or an environment variable substitution if this config is intended to be shared across environments.🤖 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/geo3k/geo3k_config.yaml` at line 6, Replace the machine-specific absolute value in the dataset configuration with a portable relative path or supported environment-variable substitution, while preserving the reference to the intended train.parquet dataset.
🤖 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 `@core/data_manager/load_yaml.py`:
- Line 208: Update the ImportError raised in the parquet-loading exception
handler to chain the caught exception with raise-from syntax. Preserve the
existing user-facing message while referencing the handler’s exception symbol so
the original traceback is retained.
In `@env/geo3k/Dockerfile`:
- Around line 1-10: Update the Geo3K Dockerfile to create and switch to a
dedicated non-root runtime user with UID 10001 after installing dependencies.
Ensure the required application or bind-mounted paths are writable by UID 10001
while preserving the existing dependency installation and runtime behavior.
In `@env/geo3k/rule_evaluator.py`:
- Line 3: Validate metric values with an explicit finite-number check before
clamping or awarding rewards in the relevant scoring paths around the module’s
metric evaluation logic. Reject NaN and positive or negative infinity rather
than allowing them through the clamp to produce a full reward, while preserving
existing handling for valid finite scores.
In `@env/geo3k/runner.py`:
- Around line 42-48: Move request parsing, including _read_request() and
_required_text() in main(), inside the existing try boundary so malformed input
or a missing session_id reaches the failure-result handling. Preserve the
current successful env_params processing and ensure every request path emits the
runtime’s result JSON.
In `@requirements.txt`:
- Line 13: Update the pyarrow dependency constraint from 23.0.0 to 23.0.1 or a
newer patched release, ensuring the requirements entry no longer permits the
vulnerable version.
In `@rl/buffer_server.py`:
- Around line 360-363: Update the derived start-config lookup in the agent
configuration fallback to resolve relative paths against aievobox_root, matching
the launcher’s path semantics. Use the root-resolved path for the os.path.exists
check and assign that resolved path to agent_start_config, while preserving
absolute paths and the existing derivation logic.
In `@rl/examples/geo3k_vl/run_buffer_server.sh`:
- Around line 12-13: Update the PYTHONPATH assignment in the run buffer server
script to conditionally append the existing PYTHONPATH only when it is set,
using the requested parameter-expansion pattern before AIEVOBOX_ROOT. Preserve
AIEVOBOX_ROOT as the fallback root while ensuring an unset PYTHONPATH does not
produce a leading empty entry.
In `@rl/examples/geo3k_vl/run_slime_generator.sh`:
- Line 26: Update the GPU allocation in run_slime_generator.sh so the Ray job
derives rollout GPU usage from NUM_GPUS instead of hardcoded 1 + 3 values.
Validate that NUM_GPUS leaves at least one GPU for rollout, and preserve the
configured total by allocating the remaining GPUs appropriately.
In `@rl/gateway_autostart.py`:
- Around line 92-113: Serialize the check-and-spawn sequence in ensure_started
with a shared lock covering the existing _gateway_process liveness check through
configuration generation and subprocess.Popen assignment. Preserve the current
disabled and already-running behavior, ensuring concurrent callers observe the
same process handle and cannot launch duplicate gateways.
- Around line 115-125: Update the timeout branch after _wait_ready in the
gateway startup flow to terminate the failed _gateway_process child, then raise
RuntimeError instead of returning normally. Preserve the existing readiness
success logging and return behavior when the gateway becomes ready.
---
Nitpick comments:
In `@env/geo3k/geo3k_config.yaml`:
- Line 6: Replace the machine-specific absolute value in the dataset
configuration with a portable relative path or supported environment-variable
substitution, while preserving the reference to the intended train.parquet
dataset.
In `@rl/examples/geo3k_vl/env.sh`:
- Around line 71-73: Update the AIEVOBOX_GATEWAY_HOST, AIEVOBOX_GATEWAY_PORT,
and AIEVOBOX_GATEWAY_BASE_URL exports in env.sh to preserve caller-provided
values. Set host and port only when unset, and derive the default base URL from
the resulting host and port only when AIEVOBOX_GATEWAY_BASE_URL is unset,
allowing manually hosted external Gateways to override all settings.
In `@rl/examples/geo3k_vl/run_slime_generator.sh`:
- Line 119: Quote the NUM_GPUS expansion in the ray command’s --num-gpus
argument to prevent shell word splitting and glob expansion, while preserving
the existing command behavior.
🪄 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: c033f13e-3417-405f-a466-71285bb1080d
📒 Files selected for processing (26)
.gitignorecore/data_manager/load_yaml.pydocs/configuration.mddocs/configuration_CN.mddocs/rl-training.mddocs/rl-training_CN.mdenv/geo3k/Dockerfileenv/geo3k/datasets/geo3k_sample.jsonlenv/geo3k/geo3k_config.yamlenv/geo3k/geo3k_rule_eval.yamlenv/geo3k/geo3k_start.yamlenv/geo3k/math_utils.pyenv/geo3k/rule_evaluator.pyenv/geo3k/runner.pygateway/app.pygateway/inference_forwarder.pygateway/storage.pymanager/simulation_worker.pyrequirements.txtrl/buffer_server.pyrl/examples/geo3k_vl/env.shrl/examples/geo3k_vl/run_buffer_server.shrl/examples/geo3k_vl/run_slime_generator.shrl/gateway_autostart.pyrl/llm_proxy.pytests/test_geo3k_v2_runtime.py
| try: | ||
| import pandas as pd | ||
| except ImportError: | ||
| raise ImportError("加载parquet文件需要安装pandas: pip install pandas pyarrow") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use raise ... from err in the except ImportError clause.
Ruff B904 flags this line: the raise ImportError inside an except ImportError block should chain the original exception to preserve the traceback.
🔧 Proposed fix
except ImportError:
- raise ImportError("加载parquet文件需要安装pandas: pip install pandas pyarrow")
+ raise ImportError("加载parquet文件需要安装pandas: pip install pandas pyarrow") from None📝 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.
| raise ImportError("加载parquet文件需要安装pandas: pip install pandas pyarrow") | |
| raise ImportError("加载parquet文件需要安装pandas: pip install pandas pyarrow") from None |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 208-208: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 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 `@core/data_manager/load_yaml.py` at line 208, Update the ImportError raised in
the parquet-loading exception handler to chain the caught exception with
raise-from syntax. Preserve the existing user-facing message while referencing
the handler’s exception symbol so the original traceback is retained.
Source: Linters/SAST tools
| FROM python:3.11-slim | ||
|
|
||
| # Geo3K runner dependencies are installed at image build time so runtime | ||
| # containers do not need outbound network access. | ||
| RUN python -m pip install \ | ||
| --no-cache-dir \ | ||
| --disable-pip-version-check \ | ||
| requests==2.32.5 \ | ||
| sympy==1.14.0 \ | ||
| pylatexenc==2.10 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Run the Geo3K runtime as a non-root user.
The runner processes model-generated content and has access to a read-write bind mount. Leaving the image at Docker’s root default unnecessarily increases the impact of a parser or dependency compromise.
Proposed fix
RUN python -m pip install \
--no-cache-dir \
--disable-pip-version-check \
requests==2.32.5 \
sympy==1.14.0 \
pylatexenc==2.10
+
+RUN useradd --create-home --uid 10001 geo3k
+USER geo3kEnsure required bind mounts are writable by UID 10001.
📝 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.
| FROM python:3.11-slim | |
| # Geo3K runner dependencies are installed at image build time so runtime | |
| # containers do not need outbound network access. | |
| RUN python -m pip install \ | |
| --no-cache-dir \ | |
| --disable-pip-version-check \ | |
| requests==2.32.5 \ | |
| sympy==1.14.0 \ | |
| pylatexenc==2.10 | |
| FROM python:3.11-slim | |
| # Geo3K runner dependencies are installed at image build time so runtime | |
| # containers do not need outbound network access. | |
| RUN python -m pip install \ | |
| --no-cache-dir \ | |
| --disable-pip-version-check \ | |
| requests==2.32.5 \ | |
| sympy==1.14.0 \ | |
| pylatexenc==2.10 | |
| RUN useradd --create-home --uid 10001 geo3k | |
| USER geo3k |
🧰 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)
🤖 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/geo3k/Dockerfile` around lines 1 - 10, Update the Geo3K Dockerfile to
create and switch to a dedicated non-root runtime user with UID 10001 after
installing dependencies. Ensure the required application or bind-mounted paths
are writable by UID 10001 while preserving the existing dependency installation
and runtime behavior.
Source: Linters/SAST tools
| @@ -0,0 +1,75 @@ | |||
| from __future__ import annotations | |||
|
|
|||
| from typing import Any | |||
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject non-finite metrics before awarding rewards.
float("nan") is accepted as a score and the current clamp resolves it to 10.0, producing a successful full reward with a NaN raw score.
Proposed fix
+import math
from typing import Any def _float_or_none(value: Any) -> float | None:
try:
if value is None:
return None
- return float(value)
+ number = float(value)
+ return number if math.isfinite(number) else None
except (TypeError, ValueError):
return NoneAlso applies to: 38-45, 65-71
🤖 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/geo3k/rule_evaluator.py` at line 3, Validate metric values with an
explicit finite-number check before clamping or awarding rewards in the relevant
scoring paths around the module’s metric evaluation logic. Reject NaN and
positive or negative infinity rather than allowing them through the clamp to
produce a full reward, while preserving existing handling for valid finite
scores.
| 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 {} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Include request parsing in the failure-result boundary.
Line 44 and Line 45 can raise before the try, so malformed input or a missing session ID produces no result JSON despite the runtime’s “always emit a result” contract.
Proposed fix
def main() -> int:
started_at = time.perf_counter()
- request = _read_request()
- session_id = _required_text(request.get("session_id"), "session_id")
-
+ session_id = ""
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 {}Also applies to: 112-114
🤖 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/geo3k/runner.py` around lines 42 - 48, Move request parsing, including
_read_request() and _required_text() in main(), inside the existing try boundary
so malformed input or a missing session_id reaches the failure-result handling.
Preserve the current successful env_params processing and ensure every request
path emits the runtime’s result JSON.
| openai==1.108.0 | ||
| pandas==2.3.3 | ||
| pillow==11.3.0 | ||
| pyarrow==23.0.0 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for the latest pyarrow version and security advisories.
curl -s https://pypi.org/pypi/pyarrow/json | jq '.info.version'
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: PIP, package: "pyarrow") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Repository: AI45Lab/SAfactory
Length of output: 1167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate pyarrow usage and the relevant files.
git ls-files | rg '(^|/)(requirements\.txt|.*\.py)$' || true
rg -n --hidden --glob '!**/.git/**' 'pyarrow|ParquetFile|read_row_group|read_parquet|read_table' .Repository: AI45Lab/SAfactory
Length of output: 4647
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the parquet materialization code that imports pyarrow.
sed -n '1,120p' core/data_manager/load_yaml.pyRepository: AI45Lab/SAfactory
Length of output: 4208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the parquet materialization code that imports pyarrow.
sed -n '1,140p' core/data_manager/load_yaml.pyRepository: AI45Lab/SAfactory
Length of output: 5275
Upgrade pyarrow to 23.0.1 or newer. pyarrow==23.0.0 falls in the vulnerable range for GHSA-rgxp-2hwp-jwgg, and 23.0.1 is the first patched release.
🧰 Tools
🪛 OSV Scanner (2.4.0)
[HIGH] 13-13: pyarrow 23.0.0: undefined
(PYSEC-2026-113)
[HIGH] 13-13: pyarrow 23.0.0: Apache Arrow: Potential use-after-free when reading IPC file with pre-buffering
🤖 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 `@requirements.txt` at line 13, Update the pyarrow dependency constraint from
23.0.0 to 23.0.1 or a newer patched release, ensuring the requirements entry no
longer permits the vulnerable version.
| if not agent_start_config and agent_config: | ||
| derived = re.sub(r"_config\.ya?ml$", "_start.yaml", agent_config) | ||
| if derived != agent_config and os.path.exists(derived): | ||
| agent_start_config = derived |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Resolve the derived start config relative to aievobox_root.
os.path.exists(derived) uses the buffer server’s current directory, while the launcher resolves the same relative path from aievobox_root. This can silently omit the required container startup config.
Proposed fix
if not agent_start_config and agent_config:
derived = re.sub(r"_config\.ya?ml$", "_start.yaml", agent_config)
- if derived != agent_config and os.path.exists(derived):
+ candidate = derived if os.path.isabs(derived) else os.path.join(aievobox_root, derived)
+ if derived != agent_config and os.path.exists(candidate):
agent_start_config = derived📝 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.
| if not agent_start_config and agent_config: | |
| derived = re.sub(r"_config\.ya?ml$", "_start.yaml", agent_config) | |
| if derived != agent_config and os.path.exists(derived): | |
| agent_start_config = derived | |
| if not agent_start_config and agent_config: | |
| derived = re.sub(r"_config\.ya?ml$", "_start.yaml", agent_config) | |
| candidate = derived if os.path.isabs(derived) else os.path.join(aievobox_root, derived) | |
| if derived != agent_config and os.path.exists(candidate): | |
| agent_start_config = derived |
🤖 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/buffer_server.py` around lines 360 - 363, Update the derived start-config
lookup in the agent configuration fallback to resolve relative paths against
aievobox_root, matching the launcher’s path semantics. Use the root-resolved
path for the os.path.exists check and assign that resolved path to
agent_start_config, while preserving absolute paths and the existing derivation
logic.
| export PYTHONPATH="${PYTHONPATH:-}:${AIEVOBOX_ROOT}" | ||
| export AIEVOBOX_ROOT="${AIEVOBOX_ROOT:-/root/AIEvoBox}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target script with line numbers.
git ls-files 'rl/examples/geo3k_vl/run_buffer_server.sh'
echo '---'
cat -n rl/examples/geo3k_vl/run_buffer_server.shRepository: AI45Lab/SAfactory
Length of output: 1150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check how the two exports behave for an unset or empty AIEVOBOX_ROOT.
python3 - <<'PY'
import os
def expand(py, root):
env = os.environ.copy()
if py is None:
env.pop('PYTHONPATH', None)
else:
env['PYTHONPATH'] = py
if root is None:
env.pop('AIEVOBOX_ROOT', None)
else:
env['AIEVOBOX_ROOT'] = root
# Shell-like expansion modeled in Python for the two lines as written.
py1 = f"{env.get('PYTHONPATH', '')}:{env.get('AIEVOBOX_ROOT', '')}"
root2 = env.get('AIEVOBOX_ROOT', '/root/AIEvoBox')
return py1, root2
cases = [
(None, None),
("", None),
("/tmp/x", None),
(None, ""),
("/tmp/x", ""),
(None, "/work"),
]
for py, root in cases:
py1, root2 = expand(py, root)
print(f"PYTHONPATH={py!r} AIEVOBOX_ROOT={root!r} => line12={py1!r} line13={root2!r}")
PYRepository: AI45Lab/SAfactory
Length of output: 578
🏁 Script executed:
#!/bin/bash
set -euo pipefail
bash -lc '
for root_state in unset empty set; do
for py_state in unset empty set; do
unset PYTHONPATH AIEVOBOX_ROOT
[ "$py_state" = empty ] && export PYTHONPATH=""
[ "$py_state" = set ] && export PYTHONPATH="/tmp/x"
[ "$root_state" = empty ] && export AIEVOBOX_ROOT=""
[ "$root_state" = set ] && export AIEVOBOX_ROOT="/work"
line12="${PYTHONPATH:-}:${AIEVOBOX_ROOT}"
line13="${AIEVOBOX_ROOT:-/root/AIEvoBox}"
printf "PYTHONPATH=%s AIEVOBOX_ROOT=%s => line12=%q line13=%q\n" "${PYTHONPATH-<unset>}" "${AIEVOBOX_ROOT-<unset>}" "$line12" "$line13"
done
done
'Repository: AI45Lab/SAfactory
Length of output: 785
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the sourced environment for AIEVOBOX_ROOT.
git ls-files 'rl/examples/geo3k_vl/env.sh'
echo '---'
cat -n rl/examples/geo3k_vl/env.shRepository: AI45Lab/SAfactory
Length of output: 4234
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for AIEVOBOX_ROOT definitions across the example directory.
rg -n --hidden --no-messages 'AIEVOBOX_ROOT' rl/examples/geo3k_vlRepository: AI45Lab/SAfactory
Length of output: 1178
Avoid prepending an empty PYTHONPATH entry in rl/examples/geo3k_vl/run_buffer_server.sh:12.
export PYTHONPATH="${PYTHONPATH:-}:${AIEVOBOX_ROOT}" emits a leading : when PYTHONPATH is unset, which adds the current directory to the import path. Use ${PYTHONPATH:+${PYTHONPATH}:}${AIEVOBOX_ROOT} instead.
🤖 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` around lines 12 - 13, Update the
PYTHONPATH assignment in the run buffer server script to conditionally append
the existing PYTHONPATH only when it is set, using the requested
parameter-expansion pattern before AIEVOBOX_ROOT. Preserve AIEVOBOX_ROOT as the
fallback root while ensuring an unset PYTHONPATH does not produce a leading
empty entry.
|
|
||
| export PYTHONBUFFERED=16 | ||
| NUM_GPUS=${NUM_GPUS:-8} | ||
| NUM_GPUS=${NUM_GPUS:-4} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep the Ray GPU request consistent with NUM_GPUS.
The cluster is started with configurable NUM_GPUS, but the job always requests 1 + 3 = 4 GPUs. Setting NUM_GPUS=2 or 3 leaves the job unschedulable; larger values are underutilized. Derive the rollout allocation from the configured total and validate that at least one GPU remains for rollout.
Also applies to: 141-141
🤖 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, Update the GPU
allocation in run_slime_generator.sh so the Ray job derives rollout GPU usage
from NUM_GPUS instead of hardcoded 1 + 3 values. Validate that NUM_GPUS leaves
at least one GPU for rollout, and preserve the configured total by allocating
the remaining GPUs appropriately.
| def ensure_started(*, aievobox_root: str, config_dir: str) -> Optional[subprocess.Popen]: | ||
| """Start the gateway once (idempotent). Returns the process, or None if disabled.""" | ||
| global _gateway_process | ||
| if not autostart_enabled(): | ||
| logger.info( | ||
| "gateway autostart disabled (AIEVOBOX_GATEWAY_AUTOSTART=0); " | ||
| "assuming an external gateway at %s", | ||
| os.environ.get("AIEVOBOX_GATEWAY_BASE_URL", "<unset>"), | ||
| ) | ||
| return None | ||
| if _gateway_process is not None and _gateway_process.poll() is None: | ||
| return _gateway_process | ||
|
|
||
| cfg = build_gateway_config(aievobox_root=aievobox_root) | ||
| os.makedirs(config_dir, exist_ok=True) | ||
| cfg_path = os.path.join(config_dir, "gateway.rl.generated.yaml") | ||
| with open(cfg_path, "w", encoding="utf-8") as f: | ||
| yaml.safe_dump(cfg, f, sort_keys=False, allow_unicode=True) | ||
|
|
||
| cmd = ["python3", "-m", "gateway", "--config", cfg_path] | ||
| logger.info("Starting gateway: %s (config=%s)", " ".join(cmd), cfg_path) | ||
| _gateway_process = subprocess.Popen(cmd, cwd=aievobox_root) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Serialize gateway lifecycle operations.
The check-and-spawn sequence is not thread-safe despite being documented as idempotent. Concurrent rollout requests can start multiple gateway processes and orphan the overwritten process handle.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 112-112: Command coming from incoming request
Context: subprocess.Popen(cmd, cwd=aievobox_root)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 112-112: Use of unsanitized data to create processes
Context: subprocess.Popen(cmd, cwd=aievobox_root)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(os-system-unsanitized-data)
[warning] 107-107: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(cfg_path, "w", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🪛 Ruff (0.15.21)
[error] 113-113: subprocess call: check for execution of untrusted input
(S603)
🤖 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/gateway_autostart.py` around lines 92 - 113, Serialize the check-and-spawn
sequence in ensure_started with a shared lock covering the existing
_gateway_process liveness check through configuration generation and
subprocess.Popen assignment. Preserve the current disabled and already-running
behavior, ensuring concurrent callers observe the same process handle and cannot
launch duplicate gateways.
| ready_url = _readyz_url() | ||
| timeout_s = float(os.environ.get("AIEVOBOX_GATEWAY_READY_TIMEOUT_S", "60")) | ||
| if _wait_ready(ready_url, timeout_s=timeout_s): | ||
| logger.info("gateway ready at %s (pid=%s)", ready_url, _gateway_process.pid) | ||
| else: | ||
| logger.error( | ||
| "gateway did not become ready at %s within %.0fs; check logs/gateway.log", | ||
| ready_url, | ||
| timeout_s, | ||
| ) | ||
| return _gateway_process |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail startup when the gateway never becomes ready.
After the readiness timeout, this returns normally, so buffer_server launches the rollout against an unavailable gateway. Terminate the failed child and raise RuntimeError so startup fails synchronously.
Proposed fix
if _wait_ready(ready_url, timeout_s=timeout_s):
logger.info("gateway ready at %s (pid=%s)", ready_url, _gateway_process.pid)
else:
- logger.error(
+ message = (
"gateway did not become ready at %s within %.0fs; check logs/gateway.log",
- ready_url,
- timeout_s,
)
+ logger.error(message, ready_url, timeout_s)
+ stop()
+ raise RuntimeError(message % (ready_url, timeout_s))
return _gateway_process🤖 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/gateway_autostart.py` around lines 115 - 125, Update the timeout branch
after _wait_ready in the gateway startup flow to terminate the failed
_gateway_process child, then raise RuntimeError instead of returning normally.
Preserve the existing readiness success logging and return behavior when the
gateway becomes ready.
Summary
Summary by CodeRabbit
New Features
Documentation
Bug Fixes