diff --git a/CHANGELOG.md b/CHANGELOG.md index 02d795c5..3bc18bae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ Versions follow [SemVer](https://semver.org). ## [Unreleased] +- Startup validation of `OUTERLOOP_AUTHOR_OVERRIDES` (the tick and `outerloop start`) uses the + image sessions actually run with, the default image when `OUTERLOOP_IMAGE` is unset. Before, a + codex override on a deployment without `OUTERLOOP_IMAGE` failed validation and stopped the tick. + - `OUTERLOOP_AUTHOR_OVERRIDES` accepts a list of entries per target, so different slots of one target can use different authors (each listed entry names its slots; a slot may appear only once). The single-object form is unchanged. diff --git a/src/outerloop/cli.py b/src/outerloop/cli.py index 0fe20f31..ed875e7f 100644 --- a/src/outerloop/cli.py +++ b/src/outerloop/cli.py @@ -417,6 +417,21 @@ def _setting_of(key: str, values: Mapping[str, str], environ: Mapping[str, str]) return (environ[key] if key in environ else values.get(key, "")).strip() +def _tick_image(values: Mapping[str, str], environ: Mapping[str, str], mode: str) -> str: + """The image the launched tick runs with, resolved the way its launch mode passes + settings on: the resident chain's deploy step lets a settings-file line win (even + empty, meaning no image); a local or login loop keeps the shell's value and fills + only what is missing from the file. Absent everywhere: the default image.""" + from outerloop.tick import _default_image + + key = "OUTERLOOP_IMAGE" + first, second = (values, environ) if mode == "slurm" else (environ, values) + for source in (first, second): + if key in source: + return source[key].strip() + return _default_image() + + def missing_harness_binary(values: Mapping[str, str], environ: Mapping[str, str]) -> str: """Check all configured authors' host CLIs using the harness's lookup.""" from outerloop.author_overrides import override_entries @@ -609,14 +624,6 @@ def start(args: argparse.Namespace) -> int: values = env_file_values( operator_env_file(ENV_FILE), START_KEYS + TICK_ENV_KEYS ) # one read for everything - from outerloop.author_overrides import validate_overrides - - try: - validate_overrides( - {**values, **os.environ}, _setting_of("OUTERLOOP_IMAGE", values, os.environ) - ) - except ValueError as exc: - raise StartError(str(exc)) from exc problem = "" if args.dry_run else missing_harness_binary(values, os.environ) try: author_model_setting( @@ -643,6 +650,13 @@ def start(args: argparse.Namespace) -> int: sbatch_on_path=shutil.which("sbatch") is not None, cwd=Path.cwd(), ) + from outerloop.author_overrides import validate_overrides + + try: + # validate with the image the tick this start launches will actually run with + validate_overrides({**values, **os.environ}, _tick_image(values, os.environ, plan.mode)) + except ValueError as exc: + raise StartError(str(exc)) from exc except (StartError, ValueError, OSError) as e: print(f"outerloop start: {e}", file=sys.stderr) return 2 diff --git a/src/outerloop/tick.py b/src/outerloop/tick.py index f22de6bd..b407544c 100644 --- a/src/outerloop/tick.py +++ b/src/outerloop/tick.py @@ -3440,6 +3440,12 @@ def _default_image() -> str: return os.path.expanduser("~/outerloop-images/agent-py312.sif") +def startup_image() -> str: + """The image sessions run with: OUTERLOOP_IMAGE, else the default. Startup + validation uses the same value, so it can never refuse what the tick would run.""" + return os.environ.get("OUTERLOOP_IMAGE", _default_image()) + + def _service_spec_from_env( root: Path, *, gpu_lanes: dict[str, GpuLane] | None = None ) -> tuple[Any, ServiceSpec | None]: @@ -3452,7 +3458,7 @@ def _service_spec_from_env( account = os.environ.get("OUTERLOOP_ACCOUNT", "") partition = os.environ.get("OUTERLOOP_PARTITION", "") qos = os.environ.get("OUTERLOOP_QOS", "") - image = os.environ.get("OUTERLOOP_IMAGE", _default_image()) + image = startup_image() home = os.environ.get("OUTERLOOP_HOME", "") # Account and partition are optional on Slurm: empty ones leave the billing # association and the partition to Slurm's defaults, as `start` already @@ -3588,7 +3594,7 @@ def main() -> int: try: gpu_lanes = gpu_lanes_from_env() - validate_overrides(os.environ, os.environ.get("OUTERLOOP_IMAGE", "")) + validate_overrides(os.environ, startup_image()) except ValueError as exc: parser.error(str(exc)) logging.basicConfig(level=logging.INFO, format="%(asctime)s %(message)s") diff --git a/tests/test_author_overrides.py b/tests/test_author_overrides.py index b00b89c6..08921383 100644 --- a/tests/test_author_overrides.py +++ b/tests/test_author_overrides.py @@ -433,3 +433,91 @@ def test_a_target_may_list_overrides_for_different_slots(monkeypatch): def test_listed_overrides_are_validated(entries): with pytest.raises(ValueError, match=r"^OUTERLOOP_AUTHOR_OVERRIDES:"): parse_overrides(json.dumps({"owner/repo": entries})) + + +def test_startup_validation_uses_the_image_the_tick_runs(monkeypatch): + # A deployment that leaves OUTERLOOP_IMAGE unset runs codex sessions on the default + # image; startup validation of a codex override must use that same image, or the + # tick refuses to start at all. + import os + + from outerloop import tick + from outerloop.author_overrides import validate_overrides + + monkeypatch.delenv("OUTERLOOP_IMAGE", raising=False) + monkeypatch.setenv( + "OUTERLOOP_AUTHOR_OVERRIDES", + json.dumps( + {"owner/repo": {"backend": "codex", "model": "some-codex-model", "slots": ["agent-03"]}} + ), + ) + assert tick.startup_image() == tick._default_image() + with pytest.raises(ValueError, match="requires --image"): + validate_overrides(os.environ, "") # the old startup behaviour + try: + validate_overrides(os.environ, tick.startup_image()) + except ValueError as exc: + assert "requires --image" not in str(exc) + + +def test_tick_startup_validates_with_startup_image(): + # The tick's entry point must validate overrides with the image sessions run with. + import inspect + + from outerloop import tick + + src = inspect.getsource(tick.main) + assert "validate_overrides(os.environ, startup_image())" in src + + +@pytest.mark.parametrize( + ("mode", "env_value", "file_value", "expected"), + [ + ("local", None, None, "DEFAULT"), + ("local", "", None, ""), + ("local", "/img.sif", None, "/img.sif"), + ("local", "/inherited.sif", "", "/inherited.sif"), # a local loop keeps the shell's value + ("local", None, "/file.sif", "/file.sif"), + ("slurm", None, None, "DEFAULT"), + ("slurm", "/inherited.sif", "", ""), # the deploy step lets the settings file win + ("slurm", "/inherited.sif", "/file.sif", "/file.sif"), + ("slurm", "/inherited.sif", None, "/inherited.sif"), + ], +) +def test_start_validates_with_the_launched_tick_image( + monkeypatch, tmp_path, mode, env_value, file_value, expected +): + # outerloop start must validate overrides with exactly the image the tick it launches uses. + from outerloop import cli, tick + + seen = [] + monkeypatch.setattr( + "outerloop.author_overrides.validate_overrides", lambda env, image: seen.append(image) + ) + monkeypatch.setattr(tick, "_default_image", lambda: "DEFAULT") + if env_value is None: + monkeypatch.delenv("OUTERLOOP_IMAGE", raising=False) + else: + monkeypatch.setenv("OUTERLOOP_IMAGE", env_value) + env_file = tmp_path / "settings.env" + env_file.write_text("" if file_value is None else f"OUTERLOOP_IMAGE={file_value}\n") + env_file.chmod(0o600) + monkeypatch.setattr(cli, "ENV_FILE", env_file) + monkeypatch.setenv("OUTERLOOP_ENV_FILE", str(env_file)) + monkeypatch.delenv("OUTERLOOP_COMPUTE", raising=False) + monkeypatch.delenv("OUTERLOOP_TICK_HOST", raising=False) + real_which = cli.shutil.which + monkeypatch.setattr( + cli.shutil, + "which", + lambda name, *a, **k: ( + ("/usr/bin/sbatch" if mode == "slurm" else None) + if name == "sbatch" + else real_which(name, *a, **k) + ), + ) + import contextlib + + with contextlib.suppress(SystemExit): + cli.main(["start", "--dry-run", "--root", str(tmp_path / "state")]) + assert seen and seen[0] == expected