Skip to content

Commit 697caad

Browse files
committed
fix(eval_optimize_loop): address AI review round 14 — 1 critical + 6 warnings
Critical: - run_pipeline.py: replace remove-then-create lock pattern with atomic os.replace() from temp file, followed by ownership verification. Eliminates the TOCTOU window where a concurrent process could steal the lock between os.remove() and O_CREAT|O_EXCL. Warning: - baseline.py: simplify sys.path finally block — unconditional try/except ValueError remove, no flag variable needed - call_agent.py: add sys.path.remove() cleanup for plate_agent_root insert (was permanent pollution) - auditor.py: total_cost now directly from validation.summary instead of sum(e.cost_candidate) which multiplied total by candidate count - gate.py: unknown strategy now raises ValueError instead of silently falling back to all_must_pass behavior - validator.py: merge duplicate warning logic for unknown failure_category into single if-block - baseline.py: remove dead image_key variable in real mode fallback; keep case_id traceable via image_id rather than generating garbage 99 tests pass, pipeline 6 phases run end-to-end.
1 parent 790f3ab commit 697caad

6 files changed

Lines changed: 34 additions & 21 deletions

File tree

examples/optimization/eval_optimize_loop/run_pipeline.py

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -120,13 +120,22 @@ def _pid_alive(pid):
120120
sys.exit(75)
121121
if not args.quiet:
122122
print(f"Cleaning stale lock from dead PID {old_pid}", file=sys.stderr)
123-
_os.remove(LOCK_FILE)
124-
fd = _os.open(LOCK_FILE, _os.O_CREAT | _os.O_EXCL | _os.O_WRONLY, 0o644)
125-
with _os.fdopen(fd, "w", encoding="utf-8") as lf:
126-
lf.write(f"{my_pid} {started_at}")
127-
lf.flush()
128-
_os.fsync(lf.fileno())
129-
acquired = True
123+
# Atomic takeover: write PID to temp file, then os.replace()
124+
# is atomic (rename) on both POSIX and Windows -- no TOCTOU window.
125+
tmp = LOCK_FILE + ".tmp"
126+
with open(tmp, "w", encoding="utf-8") as tf:
127+
tf.write(f"{my_pid} {started_at}")
128+
tf.flush()
129+
_os.fsync(tf.fileno())
130+
_os.replace(tmp, LOCK_FILE)
131+
# Verify ownership: if another process raced us, the file contains
132+
# their PID, not ours. In that case, back off.
133+
with open(LOCK_FILE, "r", encoding="utf-8") as vf:
134+
lock_owner = int(vf.read().strip().split()[0])
135+
if lock_owner == my_pid:
136+
acquired = True
137+
# else: someone else replaced the lock between our replace and read;
138+
# fall through to acquired=False -> exit(75)
130139
except (FileNotFoundError, ValueError, FileExistsError):
131140
pass
132141

examples/optimization/eval_optimize_loop/src/auditor.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ def build_trail(self, pipeline_name, mode, random_seed, optimization, baseline_v
8888
mode=mode,
8989
random_seed=random_seed,
9090
entries=entries,
91-
total_cost=sum(e.cost_candidate for e in entries),
91+
total_cost=validation.summary.total_cost_candidate if validation else 0.0,
9292
avg_latency_ms=baseline_val.summary.avg_latency_ms if baseline_val else 0.0,
9393
)
9494

examples/optimization/eval_optimize_loop/src/baseline.py

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -324,20 +324,19 @@ async def _run_real_split(
324324
import sys
325325
plate_root_str = str(Path(plate_agent_root))
326326
sys.path.insert(0, plate_root_str)
327-
path_restored = False
328327
try:
329328
from agent.session_manager import create_session_service, create_memory_service
330329
from eval.evaluator import PlateEvaluator
331330
except ImportError as e:
332-
sys.path.remove(plate_root_str) # restore before raising
333-
path_restored = True
334331
raise ImportError(
335332
f"Cannot import PlateAgent modules from {plate_agent_root}. "
336333
f"Ensure trpc_agent_sdk is installed. Error: {e}"
337334
)
338335
finally:
339-
if not path_restored:
340-
sys.path.remove(plate_root_str) # restore on success path too
336+
try:
337+
sys.path.remove(plate_root_str)
338+
except ValueError:
339+
pass # already removed or never inserted
341340
# 构建 ground_truth.json 格式(临时文件)
342341
# Build ground_truth items with sequential IDs for stable reverse mapping.
343342
# Uses enumerate(start=1) instead of SHA256 hash so that the id->case_id
@@ -374,7 +373,9 @@ async def _run_real_split(
374373
for r in report.details:
375374
case_id = id_to_case.get(r.image_id)
376375
if case_id is None:
377-
image_key = Path(r.image_path).name if r.image_path else ""
376+
# Defence-in-depth: should not happen with sequential IDs.
377+
# Use image_id (which IS the sequential id from gt_items) so
378+
# the case_id remains traceable rather than generating garbage.
378379
case_id = f"case_{r.image_id}"
379380
case_result = BaselineCaseResult(
380381
case_id=case_id,

examples/optimization/eval_optimize_loop/src/call_agent.py

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,10 @@ def create_plate_call_agent(
3737
to ensure evaluation isolation.
3838
"""
3939
import sys
40-
sys.path.insert(0, str(Path(plate_agent_root)))
41-
42-
async def _call_agent(query: str) -> str:
40+
_plate_root = str(Path(plate_agent_root))
41+
sys.path.insert(0, _plate_root)
42+
try:
43+
async def _call_agent(query: str) -> str:
4344
try:
4445
from trpc_agent_sdk.runners import Runner
4546
from trpc_agent_sdk.sessions import InMemorySessionService
@@ -107,6 +108,11 @@ async def _call_agent(query: str) -> str:
107108

108109
return final_text.strip() or "recognition failed"
109110

111+
finally:
112+
try:
113+
sys.path.remove(_plate_root)
114+
except ValueError:
115+
pass
110116
return _call_agent
111117

112118

examples/optimization/eval_optimize_loop/src/gate.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ def decide(
116116
# Strict majority: more than half must pass. Ties (exactly half) = reject.
117117
accepted = sum(1 for c in checks if c.passed) > len(checks) / 2
118118
else:
119-
accepted = all(c.passed for c in checks)
119+
raise ValueError(f"Unknown gate strategy: {self.strategy!r}. Expected 'all_must_pass' or 'majority'.")
120120

121121
reason = self._build_reason(accepted, checks)
122122
return GateDecision(

examples/optimization/eval_optimize_loop/src/validator.py

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,9 +68,6 @@ def run(self, val_baseline, optimization_result, simulate_regression=False):
6868
return self._run_real(val_baseline, candidate)
6969

7070
def _run_fake(self, val_baseline, candidate, simulate_regression=False):
71-
if candidate.failure_category not in CANDIDATE_PREDICTIONS:
72-
import warnings
73-
warnings.warn(f"Unknown failure_category '{candidate.failure_category}', falling back to final_answer_mismatch")
7471
pred_map = REGRESSION_PREDICTIONS if simulate_regression else CANDIDATE_PREDICTIONS.get(
7572
candidate.failure_category)
7673
if pred_map is None:

0 commit comments

Comments
 (0)