diff --git a/devops_bench/evalharness/default.py b/devops_bench/evalharness/default.py index 06044dce..33226810 100644 --- a/devops_bench/evalharness/default.py +++ b/devops_bench/evalharness/default.py @@ -54,6 +54,7 @@ ) from devops_bench.tasks import Task from devops_bench.verification import VerificationSpec +from devops_bench.verification.entry import VerificationEntry __all__ = ["DefaultEvalHarness"] @@ -470,6 +471,32 @@ def _build_verification_mapping( return {}, [] errors: list[dict[str, str]] = [] + mapping: dict[str, Any] = {} + + # Typed path: task.verification_spec has been parsed into VerificationEntry + # objects at load time. Placeholders live inside entry.spec (still raw dicts). + if isinstance(raw, list) and raw and isinstance(raw[0], VerificationEntry): + for entry in raw: + try: + spec_resolved = self._resolve_spec_placeholders( + entry.spec, cluster_name, target_deployment, namespace + ) + if entry.name in mapping: + _log.warning( + "duplicate verification entry name %r; overwriting", entry.name + ) + mapping[entry.name] = VerificationSpec(spec_resolved) + except Exception as exc: # noqa: BLE001 - surface every failure + _log.warning( + "verification entry %r failed to validate; skipping: %s", + entry.name, + exc, + ) + errors.append({"name": entry.name, "reason": str(exc)}) + return mapping, errors + + # Legacy path: raw dict / list / JSON-string (pre-migration or direct calls + # that bypass the typed schema). resolved = self._resolve_spec_placeholders(raw, cluster_name, target_deployment, namespace) if isinstance(resolved, str): try: @@ -480,7 +507,6 @@ def _build_verification_mapping( return {}, errors entries = resolved if isinstance(resolved, list) else [resolved] - mapping: dict[str, Any] = {} for index, entry in enumerate(entries): if not isinstance(entry, dict): msg = ( @@ -500,6 +526,8 @@ def _build_verification_mapping( # a ``spec`` key is itself treated as the typed node. node = entry.get("spec") if "spec" in entry else entry try: + if name in mapping: + _log.warning("duplicate verification entry name %r; overwriting", name) mapping[name] = VerificationSpec(node) except Exception as exc: # noqa: BLE001 - surface every failure _log.warning( @@ -944,7 +972,11 @@ def _empty_record(self, task: Task) -> dict[str, Any]: "expected_output_raw": task.expected_output, "retrieval_context": list(task.retrieval_context), "chaos_spec": task.chaos_spec, - "verification_spec": task.verification_spec, + "verification_spec": ( + [e.model_dump() if hasattr(e, "model_dump") else e for e in task.verification_spec] + if task.verification_spec is not None + else None + ), "chaos_report": {}, "perf_report": {}, "documentation": [doc.model_dump() for doc in task.documentation], diff --git a/devops_bench/models/__init__.py b/devops_bench/models/__init__.py index 6f90d965..b30dc217 100644 --- a/devops_bench/models/__init__.py +++ b/devops_bench/models/__init__.py @@ -14,10 +14,10 @@ """LLM provider clients and the provider-selection factory. -Each provider has an adapter module named by model family (``gemini``, -``claude``; ``ollama`` is the runtime exception). A provider's SDK is imported -only when its adapter is constructed, so a missing SDK surfaces as -:class:`MissingDependencyError` at construction time, not on import. +Adapter modules are named by model family and register themselves in +``MODELS`` on import. A provider's SDK is imported only when its adapter is +constructed, so a missing SDK surfaces as :class:`MissingDependencyError` at +construction time, not on import. """ from __future__ import annotations diff --git a/devops_bench/models/base.py b/devops_bench/models/base.py index ba749581..0fbd33bb 100644 --- a/devops_bench/models/base.py +++ b/devops_bench/models/base.py @@ -67,7 +67,7 @@ def format_tools(self, mcp_tools: Any) -> Any: """ @abstractmethod - def extract_function_calls(self, response: Any) -> list[dict]: + def extract_function_calls(self, response: Any) -> list[dict[str, Any]]: """Extract function calls from the model's response. Args: diff --git a/devops_bench/tasks/schema.py b/devops_bench/tasks/schema.py index f5343a1c..404ed139 100644 --- a/devops_bench/tasks/schema.py +++ b/devops_bench/tasks/schema.py @@ -19,6 +19,7 @@ from pydantic import BaseModel, ConfigDict, Field, model_validator from devops_bench.core import get_logger +from devops_bench.verification.entry import VerificationEntry __all__ = ["Task", "DocumentationEntry", "Constraint"] @@ -143,7 +144,7 @@ class Task(BaseModel): expected_output: str = "" retrieval_context: list[str] = Field(default_factory=list) chaos_spec: Any = None - verification_spec: Any = None + verification_spec: list[VerificationEntry] | None = None infrastructure: dict[str, Any] = Field(default_factory=dict) documentation: list[DocumentationEntry] = Field(default_factory=list) validated: bool = False diff --git a/devops_bench/verification/__init__.py b/devops_bench/verification/__init__.py index ebe037c5..ae781be2 100644 --- a/devops_bench/verification/__init__.py +++ b/devops_bench/verification/__init__.py @@ -19,11 +19,15 @@ typed outcome (:class:`VerificationResult`), the registry-driven extension surface (:data:`VERIFIERS`, :func:`parse_node`), and the deadline-based :class:`VerifierAgent` that evaluates them. Importing the package pulls no -heavy SDKs — concrete leaf verifiers register via this package's submodules +heavy SDKs -- concrete leaf verifiers register via this package's submodules only. + +The entry-level schema (:class:`VerificationEntry`, :data:`Role`) is available +via direct submodule imports or from this package. """ -from devops_bench.verification.base import VERIFIERS, BaseVerifier, VerificationResult +from devops_bench.verification.base import VERIFIERS, BaseVerifier, Mode, VerificationResult +from devops_bench.verification.entry import Role, VerificationEntry from devops_bench.verification.runner import VerifierAgent from devops_bench.verification.spec import ( ParallelSpec, @@ -36,9 +40,12 @@ __all__ = [ "BaseVerifier", + "Mode", "ParallelSpec", + "Role", "SequenceSpec", "VERIFIERS", + "VerificationEntry", "VerificationNode", "VerificationResult", "VerificationSpec", diff --git a/devops_bench/verification/base.py b/devops_bench/verification/base.py index 6213c52f..e33f949f 100644 --- a/devops_bench/verification/base.py +++ b/devops_bench/verification/base.py @@ -23,14 +23,23 @@ import time from abc import ABC, abstractmethod from collections.abc import Callable -from typing import Any +from typing import Any, Literal from pydantic import BaseModel, Field from devops_bench.core import Registry from devops_bench.k8s import poll_until -__all__ = ["VERIFIERS", "VerificationResult", "BaseVerifier"] +__all__ = ["Mode", "VERIFIERS", "VerificationResult", "BaseVerifier"] + +# Execution mode for a leaf verifier. +# +# converge -- verify(timeout_sec=N): keep polling until success or deadline. +# assert -- verify(timeout_sec=0): evaluate exactly once, never sleep. +# hold -- sample verify(0) repeatedly; every sample must pass. +# unchanged -- NOT IMPLEMENTED: requires a pre-agent snapshot protocol. +# The first consumer (unchanged_outside) is not in this PR. +Mode = Literal["converge", "assert", "unchanged", "hold"] # Registry keyed by the ``type`` discriminator literal. Entry-point discovery # lets external packages register a verifier without touching this tree. @@ -53,6 +62,14 @@ class VerificationResult(BaseModel): name: Optional label echoed from the spec node, for result rendering. children: Per-member results from compound (sequence/parallel) nodes. raw: Leaf-only kubectl diagnostics or supporting data. + weight: Contribution weight used by the rollup; default 1.0. Echoed + from the originating leaf's :attr:`BaseVerifier.weight` so the + result tree is self-describing for downstream scoring. The runner + (:meth:`~devops_bench.verification.runner.VerifierAgent._run_leaf`) + stamps the leaf's configured weight onto the result after + evaluation, so a custom verifier that builds this result directly + (bypassing :meth:`BaseVerifier._poll_to_result`) does not need to + forward ``weight`` itself — the runner is authoritative. """ success: bool @@ -61,6 +78,7 @@ class VerificationResult(BaseModel): name: str | None = None children: list[VerificationResult] = Field(default_factory=list) raw: dict | None = None + weight: float = 1.0 VerificationResult.model_rebuild() @@ -77,10 +95,26 @@ class BaseVerifier(BaseModel, ABC): kubeconfig: Optional path to a kubeconfig file, forwarded to the ``devops_bench.k8s`` wrappers so a check can target a specific cluster. When ``None`` the wrappers use the ambient kubeconfig. + weight: Contribution weight used by the rollup when summing pass/fail + fractions across leaves. Default 1.0 (all leaves equal). Echoed + onto the :class:`VerificationResult` so the tree is self-describing. + mode: Explicit execution mode. When ``None`` the runner derives the + mode from the parent :class:`~devops_bench.verification.entry.Role`: + ``"objective"`` -> ``"converge"``; + ``"safeguard"`` -> ``"assert"``. An explicit ``mode`` always wins + over the role-derived default. + hold_window_sec: Duration of the hold-mode sampling window; meaningful + only when ``mode="hold"``. Defaults to 30 s. + hold_interval_sec: Pause between hold-mode samples; meaningful only + when ``mode="hold"``. Defaults to 5 s. """ name: str | None = None kubeconfig: str | None = None + weight: float = 1.0 + mode: Mode | None = None + hold_window_sec: float = 30.0 + hold_interval_sec: float = 5.0 @abstractmethod def verify(self, timeout_sec: float) -> VerificationResult: @@ -133,4 +167,5 @@ def predicate() -> bool: reason=last["reason"], name=self.name, raw=last["raw"], + weight=self.weight, ) diff --git a/devops_bench/verification/entry.py b/devops_bench/verification/entry.py new file mode 100644 index 00000000..ac90baa2 --- /dev/null +++ b/devops_bench/verification/entry.py @@ -0,0 +1,102 @@ +# Copyright 2026 The Kubernetes Authors. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Typed outer wrapper for a verification spec entry. + +This module is intentionally kept dependency-light: it must not import from +``devops_bench.tasks`` (that direction would create a cycle) and it must not +import from ``devops_bench.verification.spec`` (that triggers registry +population and verifier imports at task-load time, before placeholder +substitution has run). +""" + +from __future__ import annotations + +from typing import Any, Literal + +from pydantic import BaseModel, model_validator + +__all__ = ["Role", "Severity", "VerificationEntry"] + +# Scoring role of a verification entry. +# +# objective -- something the agent must achieve; feeds the correctness score c. +# safeguard -- a protective invariant that must hold; a violation penalizes +# (recoverable) or hard-gates (catastrophic) the score. +Role = Literal["objective", "safeguard"] + +# Severity of a safeguard violation. Required when role='safeguard', forbidden +# when role='objective'. +# +# recoverable -- a violation penalizes the score (feeds rec_v). +# catastrophic -- a violation hard-gates the score to zero (feeds cat_v). +Severity = Literal["recoverable", "catastrophic"] + + +def _contains_unchanged_mode(value: Any) -> bool: + """Recursively walk a raw spec value looking for ``mode: unchanged``.""" + if isinstance(value, dict): + if value.get("mode") == "unchanged": + return True + return any(_contains_unchanged_mode(v) for v in value.values()) + if isinstance(value, list): + return any(_contains_unchanged_mode(item) for item in value) + return False + + +class VerificationEntry(BaseModel): + """One named verification suite with an associated scoring role. + + Attributes: + name: Cross-reference key resolved by the chaos ``verify:`` field and + used as the key in the name-keyed verification mapping. + role: Scoring category. Defaults to ``"objective"`` so every + existing spec authored without a ``role`` key is backward-compatible + without editing. + severity: Required when ``role == "safeguard"``; must be ``None`` when + ``role == "objective"``. ``"recoverable"`` violations penalize the + score (feed ``rec_v``); ``"catastrophic"`` violations hard-gate the + run (feed ``cat_v``). + spec: Raw (unparsed) verification node dict. Placeholder substitution + (``{{NAMESPACE}}``, etc.) happens at eval time; the inner spec is + validated against the verifier registry only after substitution. + """ + + name: str + role: Role = "objective" + severity: Severity | None = None + spec: Any + + @model_validator(mode="after") + def _validate_severity(self) -> VerificationEntry: + if self.role == "safeguard" and self.severity is None: + raise ValueError( + "severity is required when role='safeguard'; " + "set severity='recoverable' or severity='catastrophic'" + ) + if self.role == "objective" and self.severity is not None: + raise ValueError( + "severity must be None when role='objective'; " + "severity is only meaningful on safeguard entries" + ) + return self + + @model_validator(mode="after") + def _reject_unchanged_mode(self) -> VerificationEntry: + if _contains_unchanged_mode(self.spec): + raise ValueError( + "mode 'unchanged' is designed but not implemented yet: " + "its first consumer (unchanged_outside) is not in this PR" + ) + return self diff --git a/devops_bench/verification/runner.py b/devops_bench/verification/runner.py index c962d6cc..d5832873 100644 --- a/devops_bench/verification/runner.py +++ b/devops_bench/verification/runner.py @@ -119,11 +119,23 @@ def _run_leaf(self, node: Any, deadline: float) -> VerificationResult: Short-circuits when the remaining budget is below :data:`_MIN_LEAF_BUDGET_SECONDS` so we never issue a useless sub-second ``kubectl wait`` at the tail of the deadline. + + The verifier's configured ``weight`` is stamped onto the returned result + here, authoritatively: the leaf is the single source of truth for its + weight, so a custom ``verify()`` that builds a + :class:`VerificationResult` directly (bypassing ``_poll_to_result``) can + never silently drop it. When the node carries no ``weight`` attribute + (e.g. a compound spec reached as a leaf), the result's own weight is + left untouched. """ remaining = deadline - time.monotonic() if remaining < _MIN_LEAF_BUDGET_SECONDS: return _timed_out(node, "deadline exhausted before evaluation") - return node.verify(remaining) + result = node.verify(remaining) + weight = getattr(node, "weight", None) + if weight is not None: + result.weight = weight + return result def _run_sequence(self, node: SequenceSpec, deadline: float) -> VerificationResult: """Run children in order; stop and skip the rest on the first failure.""" diff --git a/docs/migration/pr-plan.md b/docs/migration/pr-plan.md index d3339050..4242783a 100644 --- a/docs/migration/pr-plan.md +++ b/docs/migration/pr-plan.md @@ -16,7 +16,7 @@ To ensure a smooth, error-free upstream review process, every PR must adhere to 3. **Keep the Frontier Documented**: Immediately after an upstream PR merges, uncomment its paths in `migrated.bara.sky` in a small, reviewed `gke-labs` PR (refer to [README.md](./README.md) §4 Step 2.2 for details). This locks the paths and activates the back-sync bot. 4. **All PRs are Disposable**: If a forward PR becomes stale, close it and re-assemble a clean one using `prep-export.sh`. 5. **No Cross-Border Imports**: Banish imports of un-migrated paths in migrated code to maintain a clean boundary. -6. **Smallest reviewable unit**: Prefer the finest PR that still builds and tests green. Once a shared base/registry has landed, migrate **one concrete per PR** — one CLI agent, one verifier, one fault, one deployer engine, one model provider, one task — rather than a whole family at once. This keeps upstream reviews fast and isolates any back-sync issue to a single concrete. The stage groupings in §2 are logical buckets; each typically expands into several granular PRs (enumerated in §3.2). +6. **Smallest reviewable unit**: Prefer the finest PR that still builds and tests green. Once a shared base/registry has landed, migrate **one concrete per PR** — one CLI agent, one verifier, one fault, one deployer engine, one model provider, one task — rather than a whole family at once. This keeps upstream reviews fast and isolates any back-sync issue to a single concrete. The stage groupings in §2 are logical buckets; each typically expands into several granular PRs (enumerated in §3.2). *Exception:* waves **4–5** batch related concretes into **one PR per owner per wave** (10 PRs total) to cap review overhead; a back-sync issue there bisects to a batch rather than a concrete — an accepted trade-off. 7. **Dependencies travel with their code**: `pyproject.toml`/`uv.lock` are `NEVER_SYNC` (managed per-repo), so a package can't be staged upstream ahead of its code. A PR that introduces a new third-party import must add that package to `pyproject.toml` and refresh `uv.lock` **in the same PR** (the `migration-prep` skill does this). `core/` is pure-stdlib and adds none. --- @@ -98,7 +98,7 @@ Every owner completes the [README.md](./README.md) §2 prerequisites independent ### 3.2 PRs by wave -Every PR carries a **wave number**. **All PRs in a wave are mutually independent and ship in parallel**; a wave opens only once *every* PR in the prior wave is merged upstream **and flipped** in `migrated.bara.sky` (so later PRs can import them via the back-sync). The waves are **derived from the real import edges in `devops_bench/`** (verified against the tree), so nothing in a wave imports anything else in the same wave. Load is balanced at **8–9 PRs per owner**. Waves **1–2** are the unavoidable serial bootstrap (toolchain, then `core/`, imported everywhere); **3–5** are the parallel bulk; **6–8** are the serial orchestrator tail + entrypoints; **9** is the benchmark data — each task coupled with the `tf/prebuilt/` stack it provisions (one PR + one `flip-group` per pair) — gated on the full pipeline so tasks are validated upstream before flipping. +Every PR carries a **wave number**. **All PRs in a wave are mutually independent and ship in parallel**; a wave opens only once *every* PR in the prior wave is merged upstream **and flipped** in `migrated.bara.sky` (so later PRs can import them via the back-sync). The waves are **derived from the real import edges in `devops_bench/`** (verified against the tree), so nothing in a wave imports anything else in the same wave. **Waves 4–5 are batched into one PR per owner per wave** (5 + 5 = 10 PRs); each batch is authored by someone who did **not** build that area originally (per the pre-refactor `pkg/` history), with the area's main past contributors as reviewers — fresh eyes write, the experts review. Waves **1–2** are the unavoidable serial bootstrap (toolchain, then `core/`, imported everywhere); **3–5** are the parallel bulk; **6–8** are the serial orchestrator tail + entrypoints; **9** is the benchmark data — each task coupled with the `tf/prebuilt/` stack it provisions (one PR + one `flip-group` per pair) — gated on the full pipeline so tasks are validated upstream before flipping. | Wave | PR | Paths | Imports (basis) | Owner | |:--:|---|---|---|---| @@ -113,31 +113,16 @@ Every PR carries a **wave number**. **All PRs in a wave are mutually independent | **3** | `providers/` | `providers/base.py`, `providers/gcp.py`, `providers/kind.py` | core | **Eugene** | | **3** | Terraform modules | `tf/modules/` *(no code deps — only needs to precede stacks in wave 4)* | — | **Eugene** | | **3** | `results/` model | `results/row.py`, `aggregate.py`, `normalize.py` | core | **Pradeep** | -| **4** | models: gemini client | `models/gemini.py` | models base | **Richard** | -| **4** | models: claude client | `models/claude.py` | models base | **Richard** | -| **4** | models: ollama client | `models/ollama.py` | models base | **Richard** | -| **4** | **gemini CLI agent** | `agents/cli/gemini_cli/` (agent, parsing) | agents base | **Richard** | -| **4** | MCP client | `agents/api/mcp.py` | core | **Richard** | -| **4** | **openclaw CLI agent** | `agents/cli/openclaw/` (agent, parsing) | agents base | **Simran** | -| **4** | agents shared | `agents/shared/cli_capabilities.py`, `shared/skills.py` | agents base | **Simran** | -| **4** | chaos base **(replaces upstream `agents/chaos/`)** | adds `chaos/base.py`, `agent.py`, `schema.py`, `spec.py`; **`git rm` the 3 superseded `agents/chaos/` files** | core, models base | **Simran** | -| **4** | verification base | `verification/base.py`, `spec.py`, `runner.py` | core, k8s | **Jessie** | -| **4** | metrics judge | `metrics/geval.py`, `pipeline.py`, `_skills.py` | core, models base, skills, metrics base | **Jessie** | -| **4** | deployers base | `deployers/base.py`, `factory.py` | core, providers | **Eugene** | -| **4** | default kind stack | `tf/prebuilt/kind/` (the harness's default stack; task-specific stacks ship with their task in wave 9) | tf modules | **Eugene** | -| **4** | `evalharness/reporter` | `evalharness/reporter.py` | core, results | **Simran** | -| **5** | deployers: tofu engine | `deployers/tofu.py` | deployers base | **Eugene** | -| **5** | deployers: noop engine | `deployers/noop.py` | deployers base | **Eugene** | -| **5** | API agent | `agents/api/agent.py`, `api/skills.py` | agents base, models, mcp | **Richard** | -| **5** | chaos: generate-load fault | `chaos/faults/generate_load.py` | chaos base, k8s | **Simran** | -| **5** | chaos: time-delay trigger | `chaos/triggers/time_delay.py` | chaos base | **Simran** | -| **5** | verification: pod-healthy | `verification/verifiers/pod_healthy.py` | verification base | **Jessie** | -| **5** | verification: scaling-complete | `verification/verifiers/scaling_complete.py` | verification base | **Jessie** | -| **5** | metric: grounding | `metrics/grounding.py` | metrics judge | **Pradeep** | -| **5** | metric: tool-invocation | `metrics/tool_invocation.py` | metrics judge | **Pradeep** | -| **5** | metric: checklist | `metrics/checklist.py` | metrics judge | **Pradeep** | -| **5** | metric: outcome-validity | `metrics/outcome_validity.py` | metrics judge | **Jessie** | -| **5** | metric: chaos-metrics | `metrics/chaos_metrics.py` | metrics judge | **Jessie** | +| **4** | models provider clients + **gemini CLI agent** | `models/gemini.py`, `claude.py`, `ollama.py`; `agents/cli/gemini_cli/` (agent, parsing) | models base, agents base | **Richard** *(review: Jessie, Simran)* | +| **4** | **openclaw CLI agent** + agents shared + chaos base **(replaces upstream `agents/chaos/`)** | `agents/cli/openclaw/` (agent, parsing); `agents/shared/cli_capabilities.py`, `shared/skills.py`; adds `chaos/base.py`, `agent.py`, `schema.py`, `spec.py`; **`git rm` the 3 superseded `agents/chaos/` files** | core, agents base, models base | **Pradeep** *(review: Simran, Jessie)* | +| **4** | verification base + metrics judge | `verification/base.py`, `spec.py`, `runner.py`; `metrics/geval.py`, `pipeline.py`, `_skills.py` | core, k8s, models base, skills, metrics base | **Eugene** *(review: Jessie)* | +| **4** | deployers base + default kind stack | `deployers/base.py`, `factory.py`; `tf/prebuilt/kind/` (the harness's default stack; task-specific stacks ship with their task in wave 9) | core, providers, tf modules | **Simran** *(review: Eugene, Jessie)* | +| **4** | `evalharness/reporter` | `evalharness/reporter.py` | core, results | **Jessie** *(review: Pradeep)* | +| **5** | API agent + MCP client | `agents/api/agent.py`, `api/skills.py`, `api/mcp.py` *(`mcp.py` could ship in wave 4 on its basis, but its only importer is the API agent, so it travels here)* | agents base, models clients | **Richard** *(review: Simran)* | +| **5** | chaos concretes | `chaos/faults/generate_load.py`, `chaos/triggers/time_delay.py` | chaos base, k8s | **Jessie** *(review: Simran)* | +| **5** | verification verifiers | `verification/verifiers/pod_healthy.py`, `scaling_complete.py` | verification base | **Simran** *(review: Jessie, Pradeep)* | +| **5** | deployer engines | `deployers/tofu.py`, `noop.py` | deployers base | **Pradeep** *(review: Richard, Eugene)* | +| **5** | concrete metrics | `metrics/grounding.py`, `tool_invocation.py`, `checklist.py`, `outcome_validity.py`, `chaos_metrics.py` *(one PR restores the full `metrics/__init__.py` surface, so the init flips right after — see the wave-3 metrics base row)* | metrics judge, skills | **Eugene** *(review: Jessie, Richard)* | | **6** | harness base + scenario | `evalharness/base.py`, `artifacts.py`, `scenario.py` | agents, chaos, verification, deployers | **Simran** | | **7** | default eval harness | `evalharness/default.py` | harness base, metrics, results, tasks | **Simran** | | **8** | CLI entrypoint | `cli.py`, `__main__.py`, `run.py` | evalharness, metrics, tasks | **Pradeep** | diff --git a/migrated.bara.sky b/migrated.bara.sky index 524d1f05..92e536ea 100644 --- a/migrated.bara.sky +++ b/migrated.bara.sky @@ -3,11 +3,12 @@ # hack/check-migrated-readonly.sh (flip guard) and hack/migration-status.sh (tracker), so there is # exactly one list. Add a path (uncomment a line) when its forward PR merges; remove it to un-migrate. # -# Ordered by wave (pr-plan.md §3.2 code, §3.4 docs/skills). Entries are per-PR: one concrete plus -# its co-located unit tests, matching the "smallest reviewable unit" rule (pr-plan.md §1.6). That -# granularity is deliberate — suggest-flips uncomments a line only once every file it covers exists -# upstream with IDENTICAL content (blob parity), so an upstream placeholder or a same-named-but- -# different file never flips ownership; a per-PR entry flips exactly when its PR lands. Entries +# Ordered by wave (pr-plan.md §3.2 code, §3.4 docs/skills). Entries are per-concrete: one concrete +# plus its co-located unit tests (waves 4–5 batch several concretes into one PR — pr-plan.md §3.2 — +# but the finer entry granularity stays). That granularity is deliberate — suggest-flips uncomments +# a line only once every file it covers exists upstream with IDENTICAL content (blob parity), so an +# upstream placeholder or a same-named-but-different file never flips ownership; an entry flips +# exactly when its files land. Entries # tagged `# flip-group: ` flip atomically — none until all members are content-ready — which # couples each benchmark task with the tf stack it provisions. Wave 1 (toolchain: pyproject.toml, # uv.lock, .python-version, CI) is human-managed in both repos and is intentionally NOT listed here. @@ -32,8 +33,8 @@ MIGRATED = [ # metrics registry base # "devops_bench/metrics/base.py", # __init__.py lags its PR by design: it ships trimmed to the base surface and only reaches - # blob parity once the wave-5 family PRs re-add their imports upstream. Do not flip it by - # hand before then — gke-labs imports the full package root (also NEVER_SYNC'd until parity). + # blob parity once the wave-5 concrete-metrics PR re-adds the family imports upstream. Do not + # flip it by hand before then — gke-labs imports the full package root (NEVER_SYNC'd until parity). # "devops_bench/metrics/__init__.py", # "tests/unit/metrics/test_metrics_base.py", # "tests/unit/metrics/test_metrics_extension_axis.py", @@ -69,31 +70,24 @@ MIGRATED = [ # "devops_bench/results/**", # "tests/unit/results/**", - # --- Wave 4: components on the bases (parallel) --- - # models: provider clients + # --- Wave 4: components on the bases (parallel; one batched PR per owner — pr-plan.md §3.2) --- + # PR: models provider clients + gemini CLI agent # "devops_bench/models/gemini.py", # "tests/unit/models/test_models_gemini.py", # "devops_bench/models/claude.py", # "tests/unit/models/test_models_claude.py", # "devops_bench/models/ollama.py", # "tests/unit/models/test_models_ollama.py", - # gemini CLI agent # "devops_bench/agents/cli/gemini_cli/**", # "devops_bench/agents/cli/__init__.py", # "tests/unit/agents/test_agents_cli_gemini.py", - # openclaw CLI agent + # PR: openclaw CLI agent + agents shared + chaos base + # (chaos base replaces upstream agents/chaos/ — git rm the 3 superseded files in this PR) # "devops_bench/agents/cli/openclaw/**", # "tests/unit/agents/test_agents_cli_openclaw.py", # "tests/unit/agents/test_legacy_openclaw_tokens.py", - # agents shared (cli_capabilities, skills) # "devops_bench/agents/shared/**", # "tests/unit/agents/shared/**", - # MCP client - # "devops_bench/agents/api/mcp.py", - # "devops_bench/agents/api/__init__.py", - # "tests/unit/agents/api/test_agents_api_mcp.py", - # "tests/unit/agents/api/test_agents_api_import_hygiene.py", - # chaos base (replaces upstream agents/chaos/ — git rm the 3 superseded files in this PR) # "devops_bench/chaos/base.py", # "devops_bench/chaos/agent.py", # "devops_bench/chaos/schema.py", @@ -104,7 +98,7 @@ MIGRATED = [ # "tests/unit/chaos/test_spec.py", # "tests/unit/chaos/test_extension_axis.py", # "tests/unit/chaos/test_package_import.py", - # verification base (base, spec, runner) + # PR: verification base + metrics judge # "devops_bench/verification/base.py", # "devops_bench/verification/spec.py", # "devops_bench/verification/runner.py", @@ -114,47 +108,49 @@ MIGRATED = [ # "tests/unit/verification/test_runner.py", # "tests/unit/verification/test_verifier_registry.py", # "tests/unit/verification/test_package_import.py", - # metrics judge (geval, pipeline, _skills) # "devops_bench/metrics/geval.py", # "devops_bench/metrics/pipeline.py", # "devops_bench/metrics/_skills.py", # "tests/unit/metrics/test_metrics_geval.py", # "tests/unit/metrics/test_metrics_pipeline.py", # "tests/unit/metrics/test_metrics_skills.py", - # deployers base (base, factory) + # PR: deployers base + default kind stack (task-specific stacks travel with their task — wave 9) # "devops_bench/deployers/base.py", # "devops_bench/deployers/factory.py", # "devops_bench/deployers/__init__.py", # "tests/unit/deployers/test_deployers_factory.py", - # harness default kind stack (task-specific stacks travel with their task — wave 9) # "tf/prebuilt/kind/**", - # evalharness reporter (consumes results/) + # PR: evalharness reporter (consumes results/) # "devops_bench/evalharness/reporter.py", # "tests/unit/evalharness/test_reporter.py", - # --- Wave 5: concretes on the components (parallel) --- - # deployers engines - # "devops_bench/deployers/tofu.py", - # "tests/unit/deployers/test_deployers_tofu.py", - # "devops_bench/deployers/noop.py", - # "tests/unit/deployers/test_deployers_noop.py", - # API agent (agent, skills) + # --- Wave 5: concretes on the components (parallel; one batched PR per owner — pr-plan.md §3.2) --- + # PR: API agent + MCP client (mcp.py's only importer is the API agent, so it travels here) # "devops_bench/agents/api/agent.py", # "devops_bench/agents/api/skills.py", + # "devops_bench/agents/api/mcp.py", + # "devops_bench/agents/api/__init__.py", # "tests/unit/agents/api/test_agents_api_agent.py", # "tests/unit/agents/api/test_agents_api_skills.py", - # chaos: generate-load fault + # "tests/unit/agents/api/test_agents_api_mcp.py", + # "tests/unit/agents/api/test_agents_api_import_hygiene.py", + # PR: chaos concretes (generate-load fault + time-delay trigger) # "devops_bench/chaos/faults/**", # "tests/unit/chaos/test_generate_load.py", - # chaos: time-delay trigger # "devops_bench/chaos/triggers/**", # "tests/unit/chaos/test_time_trigger.py", - # verification: verifiers + # PR: verification verifiers (pod-healthy + scaling-complete) # "devops_bench/verification/verifiers/pod_healthy.py", # "tests/unit/verification/test_pod_healthy.py", # "devops_bench/verification/verifiers/scaling_complete.py", # "tests/unit/verification/test_scaling_complete.py", - # metric families (each travels with its skills/ guide; see wave 3 skills) + # PR: deployer engines (tofu + noop) + # "devops_bench/deployers/tofu.py", + # "tests/unit/deployers/test_deployers_tofu.py", + # "devops_bench/deployers/noop.py", + # "tests/unit/deployers/test_deployers_noop.py", + # PR: concrete metrics (all five; each travels with its skills/ guide — see wave 3 skills — + # and this PR restores the full metrics/__init__.py surface, so flip the init after it lands) # "devops_bench/metrics/grounding.py", # "tests/unit/metrics/test_metrics_grounding.py", # "devops_bench/metrics/tool_invocation.py", diff --git a/tests/unit/evalharness/test_default_harness.py b/tests/unit/evalharness/test_default_harness.py index 9b78206f..1908f2ee 100644 --- a/tests/unit/evalharness/test_default_harness.py +++ b/tests/unit/evalharness/test_default_harness.py @@ -391,3 +391,35 @@ def test_resolve_deployment_and_namespace_precedence_and_types( dep, ns = harness._resolve_deployment_and_namespace(task_non_str_vars) # noqa: SLF001 assert dep == "123" assert ns == "456" + + +def test_empty_record_does_not_crash_when_verification_spec_holds_raw_dicts( + isolated_env: None, +) -> None: + """``_empty_record`` must not crash when ``verification_spec`` contains raw dicts. + + A test fixture or mock that bypasses ``Task.from_dict`` can produce a + ``Task`` whose ``verification_spec`` holds plain dicts instead of + ``VerificationEntry`` objects. The defensive ``hasattr`` guard in + ``_empty_record`` must absorb this without raising ``AttributeError``. + """ + harness = DefaultEvalHarness(project_id="p", cluster_name="c") + + raw_spec = {"name": "check-a", "spec": {"type": "pod_healthy", "selector": "app=x"}} + task = Task.model_construct( + id="t", + name="demo", + folder="", + prompt="p", + expected_output="", + retrieval_context=[], + chaos_spec=None, + verification_spec=[raw_spec], + infrastructure={}, + documentation=[], + validated=False, + ) + + record = harness._empty_record(task) # noqa: SLF001 + + assert record["verification_spec"] == [raw_spec] diff --git a/tests/unit/evalharness/test_reporter.py b/tests/unit/evalharness/test_reporter.py index bb25d67a..149fe07d 100644 --- a/tests/unit/evalharness/test_reporter.py +++ b/tests/unit/evalharness/test_reporter.py @@ -107,7 +107,9 @@ def _stub_task() -> Task: "expected_output": "exp", "retrieval_context": ["doc-a"], "chaos_spec": {"chaos": "yes"}, - "verification_spec": {"verify": "yes"}, + "verification_spec": [ + {"name": "check", "spec": {"type": "pod_healthy", "selector": "app=demo"}} + ], } ) diff --git a/tests/unit/evalharness/test_spec_parsing.py b/tests/unit/evalharness/test_spec_parsing.py index ca14bf0e..4953e93d 100644 --- a/tests/unit/evalharness/test_spec_parsing.py +++ b/tests/unit/evalharness/test_spec_parsing.py @@ -22,8 +22,12 @@ from __future__ import annotations +import json +import logging from pathlib import Path +from unittest.mock import patch +import pytest import yaml from devops_bench.chaos import ChaosSpec @@ -35,6 +39,7 @@ ParallelSpec, VerificationSpec, ) +from devops_bench.verification.entry import VerificationEntry from devops_bench.verification.verifiers import ( PodHealthyVerifier, ScalingCompleteVerifier, @@ -184,3 +189,79 @@ def test_task_loader_reads_native_yaml_specs_as_python_values() -> None: # Opaque ``Any``: a list-of-mappings flows through verbatim, not as a string. assert isinstance(task.chaos_spec, list) assert isinstance(task.verification_spec, list) + + +def test_verification_mapping_captures_placeholder_resolution_error_as_parse_error( + caplog: pytest.LogCaptureFixture, +) -> None: + """A placeholder-resolution failure is recorded as a parse error, not a crash. + + The typed path (``VerificationEntry`` objects) moves ``_resolve_spec_placeholders`` + inside the per-entry try/except, so any exception it raises is appended to the + errors list rather than propagating out of ``_build_verification_mapping``. + """ + good_entry = VerificationEntry( + name="good", + spec={"type": "pod_healthy", "selector": "app=x", "namespace": "default"}, + ) + bad_entry = VerificationEntry( + name="bad", + spec={"type": "pod_healthy", "selector": "app=y", "namespace": "default"}, + ) + + harness = _harness() + + def _boom(spec, cluster_name, target_deployment=None, namespace=None): + if spec.get("selector") == "app=y": + raise ValueError("placeholder boom") + return spec + + with ( + caplog.at_level(logging.WARNING, logger="devops_bench.evalharness.default"), + patch.object(harness, "_resolve_spec_placeholders", side_effect=_boom), + ): + mapping, errors = harness._build_verification_mapping( # noqa: SLF001 + [good_entry, bad_entry], cluster_name="c" + ) + + assert "good" in mapping + assert "bad" not in mapping + assert len(errors) == 1 + assert errors[0]["name"] == "bad" + assert "placeholder boom" in errors[0]["reason"] + + +def test_verification_mapping_warns_on_duplicate_entry_names( + caplog: pytest.LogCaptureFixture, +) -> None: + """Two entries sharing a name emit a warning and the last one wins. + + The typed path must not silently overwrite without any operator signal. + ``caplog`` pins that a WARNING-level log line naming the duplicate is + emitted, and the final mapping carries the second entry's spec. + """ + entry_first = VerificationEntry( + name="same", + spec={"type": "pod_healthy", "selector": "app=first", "namespace": "default"}, + ) + entry_second = VerificationEntry( + name="same", + spec={"type": "pod_healthy", "selector": "app=second", "namespace": "default"}, + ) + + harness = _harness() + with caplog.at_level(logging.WARNING, logger="devops_bench.evalharness.default"): + mapping, errors = harness._build_verification_mapping( # noqa: SLF001 + [entry_first, entry_second], cluster_name="c" + ) + + assert errors == [] + assert "same" in mapping + # Warning was emitted + assert any( + "duplicate" in record.message and "same" in record.message for record in caplog.records + ) + # Last entry wins: the resolved spec carries the second selector + spec = mapping["same"] + raw_repr = json.dumps(spec.model_dump()) + assert "app=second" in raw_repr diff --git a/tests/unit/models/test_models_base.py b/tests/unit/models/test_models_base.py index 1bf20ace..110e05fe 100644 --- a/tests/unit/models/test_models_base.py +++ b/tests/unit/models/test_models_base.py @@ -16,120 +16,83 @@ from __future__ import annotations -import os +from typing import Any import pytest from devops_bench.core.errors import ConfigError, NotRegisteredError +from devops_bench.core.model_providers import ProviderSpec +from devops_bench.models import base from devops_bench.models.base import MODELS, LLMClient, get_model -def test_registry_has_known_providers(): - # Importing the adapter modules registers them under their canonical keys. - from devops_bench.models import claude, gemini, ollama # noqa: F401 +class _StubClient(LLMClient): + """Minimal concrete adapter recording its constructor arguments.""" - assert MODELS.get("gemini") is gemini.GeminiClientAdapter - assert MODELS.get("claude") is claude.ClaudeClientAdapter - assert MODELS.get("ollama") is ollama.OllamaClientAdapter + def __init__(self, model_name: str | None = None, backend: str | None = None, **kwargs: Any): + self.model_name = model_name + self.backend = backend + self.kwargs = kwargs + async def generate_content(self, contents, tools, system_instruction): + raise NotImplementedError -def test_get_model_returns_gemini_adapter(mocker): - from devops_bench.models import gemini + def format_tools(self, mcp_tools): + raise NotImplementedError - mocker.patch.object(gemini.genai, "Client") - client = get_model(provider="gemini") + def extract_function_calls(self, response): + raise NotImplementedError - assert isinstance(client, gemini.GeminiClientAdapter) - assert isinstance(client, LLMClient) - - -def test_get_model_resolves_google_alias(mocker): - from devops_bench.models import gemini - - mocker.patch.object(gemini.genai, "Client") - client = get_model(provider="google") - - assert isinstance(client, gemini.GeminiClientAdapter) - - -@pytest.mark.parametrize("provider", ["google-vertex", "google_vertex"]) -def test_get_model_resolves_google_vertex_alias(mocker, provider): - from devops_bench.models import gemini - - mocker.patch.object(gemini.genai, "Client") - client = get_model(provider=provider) - - assert isinstance(client, gemini.GeminiClientAdapter) - - -def test_get_model_returns_claude_adapter(mocker): - from devops_bench.models import claude - - mocker.patch.object(claude, "AsyncAnthropicVertex") - client = get_model(provider="claude") - - assert isinstance(client, claude.ClaudeClientAdapter) - - -def test_get_model_resolves_anthropic_alias(mocker): - from devops_bench.models import claude - - mocker.patch.object(claude, "AsyncAnthropicVertex") - client = get_model(provider="anthropic") - - assert isinstance(client, claude.ClaudeClientAdapter) + def get_text_content(self, response): + raise NotImplementedError -def test_get_model_returns_ollama_adapter(mocker): - from devops_bench.models import ollama +@pytest.fixture +def fake_family(monkeypatch): + """Route provider resolution to a fake adapter family backed by ``_StubClient``. - mocker.patch.object(ollama, "AsyncOpenAI") - client = get_model(provider="ollama") + The family has no adapter module on disk, so ``get_model`` skips the import + step and hits the registry directly — no collision with any real adapter, + now or later. + """ - assert isinstance(client, ollama.OllamaClientAdapter) + def install(backend: str | None = None) -> None: + spec = ProviderSpec( + canonical="fake-provider", + adapter_family="fake_family", + oc_provider="fake-provider", + api_key_envs=(), + keyless_ok=True, + backend=backend, + ) + monkeypatch.setattr(base, "resolve_provider", lambda provider: spec) + monkeypatch.setitem(MODELS._items, "fake_family", _StubClient) + return install -def test_get_model_is_case_insensitive(mocker): - from devops_bench.models import claude - mocker.patch.object(claude, "AsyncAnthropicVertex") - client = get_model(provider="CLAUDE") +def test_get_model_constructs_registered_adapter(fake_family): + fake_family() - assert isinstance(client, claude.ClaudeClientAdapter) - - -def test_get_model_defaults_to_gemini(mocker): - from devops_bench.models import gemini - - mocker.patch.object(gemini.genai, "Client") - mocker.patch.dict(os.environ, {}, clear=True) - client = get_model() - - assert isinstance(client, gemini.GeminiClientAdapter) - - -def test_get_model_reads_provider_from_env(mocker): - from devops_bench.models import claude - - mocker.patch.object(claude, "AsyncAnthropicVertex") - mocker.patch.dict(os.environ, {"AGENT_PROVIDER": "claude"}, clear=True) - client = get_model() - - assert isinstance(client, claude.ClaudeClientAdapter) + client = get_model(provider="fake-provider", model_name="fake-model-1", timeout=7) + assert isinstance(client, _StubClient) + assert isinstance(client, LLMClient) + assert client.model_name == "fake-model-1" + assert client.backend is None + assert client.kwargs == {"timeout": 7} -def test_get_model_passes_model_name(mocker): - from devops_bench.models import gemini - mocker.patch.object(gemini.genai, "Client") - client = get_model(provider="gemini", model_name="gemini-custom") +def test_get_model_forwards_backend_hint(fake_family): + fake_family(backend="fake-backend") - assert client.model_name == "gemini-custom" + client = get_model(provider="fake-provider") + assert isinstance(client, _StubClient) + assert client.backend == "fake-backend" -def test_get_model_unknown_provider_raises(mocker): - from devops_bench.models import claude, gemini # noqa: F401 +def test_get_model_unknown_provider_raises(): with pytest.raises(ConfigError): get_model(provider="does-not-exist") @@ -139,28 +102,3 @@ def test_get_model_openai_resolves_but_has_no_adapter(): # so resolution succeeds and the registry lookup raises NotRegisteredError. with pytest.raises(NotRegisteredError): get_model(provider="openai") - - -def test_get_model_passes_vertex_backend_to_gemini(mocker): - from devops_bench.models import gemini - - client_cls = mocker.patch.object(gemini.genai, "Client") - mocker.patch.dict(os.environ, {"GCP_PROJECT_ID": "p"}, clear=True) - get_model(provider="google-vertex") - - # google-vertex -> backend="vertex" -> the adapter builds the Vertex client. - client_cls.assert_called_once_with(vertexai=True, project="p", location="global") - - -def test_get_model_passes_vertex_backend_to_claude(mocker): - from devops_bench.models import claude - - vertex_cls = mocker.patch.object(claude, "AsyncAnthropicVertex") - # An API key is present, but the anthropic-vertex provider must still use - # Vertex (backend hint decouples auth from key presence). - mocker.patch.dict( - os.environ, {"AGENT_API_KEY": "sk-test", "AGENT_MODEL": "claude-x"}, clear=True - ) - get_model(provider="anthropic-vertex") - - vertex_cls.assert_called_once() diff --git a/tests/unit/tasks/test_tasks_schema.py b/tests/unit/tasks/test_tasks_schema.py index eb3cea41..eca00b80 100644 --- a/tests/unit/tasks/test_tasks_schema.py +++ b/tests/unit/tasks/test_tasks_schema.py @@ -28,7 +28,9 @@ def test_from_dict_full(): "expected_output": " done ", "retrieval_context": ["ctx"], "chaos_spec": {"kind": "kill"}, - "verification_spec": {"check": "ok"}, + "verification_spec": [ + {"name": "check-pods", "spec": {"type": "pod_healthy", "selector": "app=web"}} + ], "infrastructure": {"deployer": "tofu"}, } task = Task.from_dict(raw, name_default="dir-name") @@ -39,7 +41,9 @@ def test_from_dict_full(): assert task.expected_output == "done" assert task.retrieval_context == ["ctx"] assert task.chaos_spec == {"kind": "kill"} - assert task.verification_spec == {"check": "ok"} + assert task.verification_spec is not None + assert len(task.verification_spec) == 1 + assert task.verification_spec[0].name == "check-pods" assert task.infrastructure == {"deployer": "tofu"} @@ -183,13 +187,34 @@ def test_constraint_empty_text_coalesces(): assert constraint.critical is False -def test_chaos_and_verification_specs_are_opaque(): - # These specs are parsed downstream, so the schema accepts any shape - # (e.g. a raw JSON string from a YAML literal block, a list, or a mapping). - raw = {"chaos_spec": '[{"name": "spike"}]', "verification_spec": [{"name": "v"}]} +def test_chaos_spec_is_opaque(): + # chaos_spec is still Any; accepts JSON strings, lists, mappings. + raw = {"chaos_spec": '[{"name": "spike"}]'} task = Task.from_dict(raw, name_default="d") assert task.chaos_spec == '[{"name": "spike"}]' - assert task.verification_spec == [{"name": "v"}] + + +def test_verification_spec_parses_typed_entries(): + # verification_spec is now list[VerificationEntry]; the spec field inside + # each entry is kept raw (unparsed) so placeholder substitution can run later. + from devops_bench.verification.entry import VerificationEntry + + raw = { + "verification_spec": [ + { + "name": "check", + "role": "safeguard", + "severity": "recoverable", + "spec": {"type": "pod_healthy", "selector": "app=web"}, + }, + ] + } + task = Task.from_dict(raw, name_default="d") + assert task.verification_spec is not None + assert isinstance(task.verification_spec[0], VerificationEntry) + assert task.verification_spec[0].name == "check" + assert task.verification_spec[0].role == "safeguard" + assert task.verification_spec[0].severity == "recoverable" def test_to_dict_roundtrip_fields(): diff --git a/tests/unit/verification/test_entry_schema.py b/tests/unit/verification/test_entry_schema.py new file mode 100644 index 00000000..c857283c --- /dev/null +++ b/tests/unit/verification/test_entry_schema.py @@ -0,0 +1,219 @@ +# Copyright 2026 The Kubernetes Authors. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Unit tests for VerificationEntry schema and the optimize-scale regression. + +Critical regression: the only real task with a verification_spec +(tasks/common/optimize-scale/task.yaml) must parse under the new typed +list[VerificationEntry] schema and its entries must default to role='objective'. +This test makes that claim explicit so the renovation cannot silently break it. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +import yaml +from pydantic import ValidationError + +from devops_bench.tasks.schema import Task +from devops_bench.verification.entry import VerificationEntry + +# -- VerificationEntry model -------------------------------------------------- + + +def test_entry_defaults_role_to_objective(): + entry = VerificationEntry(name="check-pods", spec={"type": "pod_healthy", "selector": "app=x"}) + + assert entry.role == "objective" + assert entry.severity is None + + +def test_entry_accepts_objective_role(): + entry = VerificationEntry(name="check", role="objective", spec={}) + assert entry.role == "objective" + + +def test_entry_accepts_safeguard_roles_with_severity(): + for severity in ("recoverable", "catastrophic"): + entry = VerificationEntry(name="check", role="safeguard", severity=severity, spec={}) + assert entry.role == "safeguard" + assert entry.severity == severity + + +def test_entry_rejects_unknown_role(): + with pytest.raises(ValidationError): + VerificationEntry(name="check", role="unknown", spec={}) + + +def test_entry_rejects_safeguard_without_severity(): + with pytest.raises(ValidationError, match="severity is required"): + VerificationEntry(name="check", role="safeguard", spec={}) + + +def test_entry_rejects_objective_with_severity(): + with pytest.raises(ValidationError, match="severity must be None"): + VerificationEntry(name="check", role="objective", severity="recoverable", spec={}) + + +def test_entry_rejects_unknown_severity(): + with pytest.raises(ValidationError): + VerificationEntry(name="check", role="safeguard", severity="mild", spec={}) + + +def test_entry_rejects_unchanged_mode(): + with pytest.raises(ValidationError, match="unchanged"): + VerificationEntry( + name="check", + role="objective", + spec={"type": "scaling_complete", "deployment": "web", "mode": "unchanged"}, + ) + + +def test_entry_rejects_unchanged_mode_nested_in_parallel(): + with pytest.raises(ValidationError, match="unchanged"): + VerificationEntry( + name="check", + role="objective", + spec={ + "type": "parallel", + "checks": [ + {"type": "scaling_complete", "deployment": "web", "mode": "unchanged"}, + ], + }, + ) + + +def test_entry_spec_accepts_any_value(): + entry = VerificationEntry(name="raw", spec={"type": "parallel", "checks": []}) + assert entry.spec == {"type": "parallel", "checks": []} + + +def test_entry_spec_can_be_none(): + entry = VerificationEntry(name="raw", spec=None) + assert entry.spec is None + + +def test_entry_name_is_required(): + with pytest.raises(ValidationError): + VerificationEntry(role="objective", spec={}) + + +# -- Role type ---------------------------------------------------------------- + + +def test_role_literal_values(): + entry = VerificationEntry(name="x", role="objective", spec={}) + assert entry.role == "objective" + + for severity in ("recoverable", "catastrophic"): + entry = VerificationEntry(name="x", role="safeguard", severity=severity, spec={}) + assert entry.role == "safeguard" + assert entry.severity == severity + + +# -- Critical regression: optimize-scale task parses under typed schema ------- + + +_TASK_YAML_PATH = Path(__file__).parents[3] / "tasks" / "common" / "optimize-scale" / "task.yaml" + + +@pytest.mark.skipif( + not _TASK_YAML_PATH.exists(), + reason="optimize-scale task.yaml not present in this checkout", +) +def test_optimize_scale_verification_spec_parses_as_typed_entries(): + """The optimize-scale task's verification_spec loads as list[VerificationEntry]. + + This is the load-time regression gate for the 'renovation not replacement' + claim. The spec must round-trip through Task.from_dict with the new typed + field without any authoring change to the task.yaml. + """ + raw = yaml.safe_load(_TASK_YAML_PATH.read_text()) + task = Task.from_dict(raw, name_default="optimize-scale", folder="optimize-scale") + + assert task.verification_spec is not None + assert isinstance(task.verification_spec, list) + assert len(task.verification_spec) == 1 + + entry = task.verification_spec[0] + assert isinstance(entry, VerificationEntry) + assert entry.name == "Planned Load Spike Verification" + assert entry.role == "objective" + + # The inner spec is kept raw (unsubstituted, unparsed) at task-load time. + assert isinstance(entry.spec, dict) + assert entry.spec.get("type") == "parallel" + + +@pytest.mark.skipif( + not _TASK_YAML_PATH.exists(), + reason="optimize-scale task.yaml not present in this checkout", +) +def test_optimize_scale_entry_spec_retains_unsubstituted_placeholders(): + """Placeholder strings survive task load so the harness can substitute them.""" + raw = yaml.safe_load(_TASK_YAML_PATH.read_text()) + task = Task.from_dict(raw, name_default="optimize-scale", folder="optimize-scale") + + entry = task.verification_spec[0] + spec_str = str(entry.spec) + assert "{{TARGET_DEPLOYMENT_NAME}}" in spec_str or "{{NAMESPACE}}" in spec_str + + +# -- Task.from_dict with typed verification_spec ------------------------------ + + +def test_task_from_dict_with_no_verification_spec_is_none(): + raw = {"id": "t1", "name": "t", "prompt": "p"} + task = Task.from_dict(raw) + + assert task.verification_spec is None + + +def test_task_from_dict_with_list_of_entries(): + raw = { + "id": "t2", + "name": "t", + "prompt": "p", + "verification_spec": [ + { + "name": "check-pods", + "role": "safeguard", + "severity": "recoverable", + "spec": {"type": "pod_healthy", "selector": "app=x"}, + }, + ], + } + task = Task.from_dict(raw) + + assert task.verification_spec is not None + assert len(task.verification_spec) == 1 + assert task.verification_spec[0].name == "check-pods" + assert task.verification_spec[0].role == "safeguard" + assert task.verification_spec[0].severity == "recoverable" + + +def test_task_from_dict_with_entries_defaults_role(): + raw = { + "id": "t3", + "name": "t", + "prompt": "p", + "verification_spec": [ + {"name": "check", "spec": {"type": "pod_healthy", "selector": "app=x"}}, + ], + } + task = Task.from_dict(raw) + + assert task.verification_spec[0].role == "objective" diff --git a/tests/unit/verification/test_runner.py b/tests/unit/verification/test_runner.py index 5cc439c2..ac0f8b2e 100644 --- a/tests/unit/verification/test_runner.py +++ b/tests/unit/verification/test_runner.py @@ -59,6 +59,40 @@ def fake_verify(self: Any, timeout_sec: float) -> VerificationResult: assert 25.0 < calls[0] <= 30.0 +def test_runner_stamps_configured_weight_onto_leaf_result(): + """The runner is authoritative for weight, even if ``verify`` forgets it. + + A custom verifier that builds its ``VerificationResult`` directly (bypassing + ``_poll_to_result``) and omits ``weight`` would otherwise leave the default + 1.0 on the result. The runner re-stamps the leaf's configured weight so the + scoring rollup cannot silently lose it. + """ + + def forgetful_verify(self: Any, timeout_sec: float) -> VerificationResult: + # Deliberately omit ``weight`` — the default 1.0 would be wrong. + return VerificationResult(success=True, elapsed_time=0.0, reason="ok") + + spec = VerificationSpec({"type": "pod_healthy", "selector": "app=web", "weight": 3.0}) + with patch.object(PodHealthyVerifier, "verify", forgetful_verify): + result = VerifierAgent().wait_for_condition(spec, timeout_sec=30) + + assert result.success is True + assert result.weight == 3.0 + + +def test_runner_preserves_default_weight_when_unset(): + """A leaf with no configured weight keeps the default 1.0.""" + + def fake_verify(self: Any, timeout_sec: float) -> VerificationResult: + return VerificationResult(success=True, elapsed_time=0.0, reason="ok") + + spec = VerificationSpec({"type": "pod_healthy", "selector": "app=web"}) + with patch.object(PodHealthyVerifier, "verify", fake_verify): + result = VerifierAgent().wait_for_condition(spec, timeout_sec=30) + + assert result.weight == 1.0 + + def test_sequence_runs_children_in_order_and_aggregates(): order: list[str] = [] verify_calls: dict[str, float] = {}