fix(batch_runner): propagate fatal and validation errors as non-zero exit codes
Python Fire serializes the return value of a wrapped function but does not use that value as the process exit code. Error paths in main() that used or therefore caused the process to exit 0, swallowing fatal errors and argument-validation failures. Raise SystemExit(1) on every error path so batch_runner returns a non-zero exit code when it cannot run. Success paths (e.g. --list_distributions) are left unchanged. Closes NousResearch/hermes-agent#86524.
This commit is contained in:
@@ -1246,21 +1246,21 @@ def main(
|
||||
# Validate required arguments
|
||||
if not dataset_file:
|
||||
print("❌ Error: --dataset_file is required")
|
||||
return
|
||||
|
||||
raise SystemExit(1)
|
||||
|
||||
if not batch_size or batch_size < 1:
|
||||
print("❌ Error: --batch_size must be a positive integer")
|
||||
return
|
||||
|
||||
raise SystemExit(1)
|
||||
|
||||
if not run_name:
|
||||
print("❌ Error: --run_name is required")
|
||||
return
|
||||
|
||||
raise SystemExit(1)
|
||||
|
||||
# Parse provider preferences (comma-separated strings to lists)
|
||||
providers_allowed_list = [p.strip() for p in providers_allowed.split(",")] if providers_allowed else None
|
||||
providers_ignored_list = [p.strip() for p in providers_ignored.split(",")] if providers_ignored else None
|
||||
providers_order_list = [p.strip() for p in providers_order.split(",")] if providers_order else None
|
||||
|
||||
|
||||
# Build reasoning_config from CLI flags
|
||||
# --reasoning_disabled takes priority, then --reasoning_effort, then default (medium)
|
||||
reasoning_config = None
|
||||
@@ -1273,10 +1273,10 @@ def main(
|
||||
valid_efforts = ["none", "minimal", "low", "medium", "high", "xhigh", "max", "ultra"]
|
||||
if reasoning_effort not in valid_efforts:
|
||||
print(f"❌ Error: --reasoning_effort must be one of: {', '.join(valid_efforts)}")
|
||||
return
|
||||
raise SystemExit(1)
|
||||
reasoning_config = {"enabled": True, "effort": reasoning_effort}
|
||||
print(f"🧠 Reasoning effort: {reasoning_effort}")
|
||||
|
||||
|
||||
# Load prefill messages from JSON file if provided
|
||||
prefill_messages = None
|
||||
if prefill_messages_file:
|
||||
@@ -1285,12 +1285,12 @@ def main(
|
||||
prefill_messages = json.load(f)
|
||||
if not isinstance(prefill_messages, list):
|
||||
print("❌ Error: prefill_messages_file must contain a JSON array of messages")
|
||||
return
|
||||
raise SystemExit(1)
|
||||
print(f"💬 Loaded {len(prefill_messages)} prefill messages from {prefill_messages_file}")
|
||||
except Exception as e:
|
||||
print(f"❌ Error loading prefill messages: {e}")
|
||||
return
|
||||
|
||||
raise SystemExit(1)
|
||||
|
||||
# Initialize and run batch runner
|
||||
try:
|
||||
runner = BatchRunner(
|
||||
@@ -1317,12 +1317,12 @@ def main(
|
||||
)
|
||||
|
||||
runner.run(resume=resume)
|
||||
|
||||
|
||||
except Exception as e:
|
||||
print(f"\n❌ Fatal error: {e}")
|
||||
if verbose:
|
||||
traceback.print_exc()
|
||||
return 1
|
||||
raise SystemExit(1)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
|
||||
64
tests/test_batch_runner_exit_code.py
Normal file
64
tests/test_batch_runner_exit_code.py
Normal file
@@ -0,0 +1,64 @@
|
||||
"""Regression tests for batch_runner process exit codes.
|
||||
|
||||
Python Fire serializes the return value of the wrapped function but does not
|
||||
use it as the process exit code. Before the fix, all of ``main``'s error paths
|
||||
returned ``0`` because a bare ``return`` or ``return 1`` was treated as the
|
||||
function result, not a non-zero exit status.
|
||||
"""
|
||||
|
||||
import subprocess
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
REPO_ROOT = Path(__file__).parent.parent
|
||||
PYTHON = sys.executable
|
||||
|
||||
|
||||
def _run(*args):
|
||||
return subprocess.run(
|
||||
[PYTHON, "batch_runner.py", *args],
|
||||
cwd=REPO_ROOT,
|
||||
text=True,
|
||||
capture_output=True,
|
||||
)
|
||||
|
||||
|
||||
def test_missing_dataset_file_exits_nonzero():
|
||||
result = _run("--run_name=test")
|
||||
assert result.returncode == 1
|
||||
assert "--dataset_file is required" in result.stdout
|
||||
|
||||
|
||||
def test_invalid_batch_size_exits_nonzero():
|
||||
result = _run("--dataset_file=/tmp/data.jsonl", "--batch_size=-1", "--run_name=test")
|
||||
assert result.returncode == 1
|
||||
assert "--batch_size must be a positive integer" in result.stdout
|
||||
|
||||
|
||||
def test_invalid_reasoning_effort_exits_nonzero():
|
||||
result = _run(
|
||||
"--dataset_file=/tmp/data.jsonl",
|
||||
"--batch_size=1",
|
||||
"--run_name=test",
|
||||
"--reasoning_effort=invalid",
|
||||
)
|
||||
assert result.returncode == 1
|
||||
assert "--reasoning_effort must be one of" in result.stdout
|
||||
|
||||
|
||||
def test_invalid_prefill_messages_file_exits_nonzero(tmp_path):
|
||||
bad_prefill = tmp_path / "prefill.json"
|
||||
bad_prefill.write_text("not json", encoding="utf-8")
|
||||
result = _run(
|
||||
"--dataset_file=/tmp/data.jsonl",
|
||||
"--batch_size=1",
|
||||
"--run_name=test",
|
||||
f"--prefill_messages_file={bad_prefill}",
|
||||
)
|
||||
assert result.returncode == 1
|
||||
assert "Error loading prefill messages" in result.stdout
|
||||
|
||||
|
||||
Reference in New Issue
Block a user