From 27a8a49ca2011510bdf8daf307f86b0e3c11b62b Mon Sep 17 00:00:00 2001 From: Nick Masluk Date: Mon, 13 Jul 2026 18:06:30 +0000 Subject: [PATCH 1/2] fix(validate): auto-populate omitted locked_parameters instead of hard-failing (#298) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The DESIGN gate required the design agent to echo every `campaign.locked_parameters` key verbatim into `bundle.experiment_spec.verified_parameters`. A single omitted key hard-failed the whole iteration before any experiment ran — pure friction, since the campaign already declares the value. In one recent 6-iteration campaign this killed 5 of 6 iterations. `_validate_locked_parameters` now distinguishes: - Omitted key — auto-populate `verified_parameters[k]` from `campaign.locked_parameters[k]` (mutating the bundle in place) and continue. The campaign is the unambiguous source of truth. - Deviating value — the bundle set a *different* value than the campaign locked. Still a hard-fail, listing every deviation. The #246 anti-mirage protection is fully preserved. `validate_design` snapshots the bundle before validation and persists the auto-filled values back to `bundle.yaml` (via atomic_write) so downstream phases read the campaign's canonical values. `compute_campaign_spec_diff` (the soft-audit mirror) now skips omitted keys so it doesn't re-flag auto-filled values as violations. A present-but-wrong-TYPE `experiment_spec` / `verified_parameters` (not a mapping) still hard-fails — we auto-fill omissions, not structurally broken bundles. Tests: reworked the two `test_f1_*` missing-key tests (which encoded the old hard-fail behavior) into `test_298_*` covering auto-fill, the missing-block and missing-experiment_spec cases, the mixed omit+deviate case, malformed-type still-fails, end-to-end validate_design pass + on-disk persistence, still-fail-on-deviation, and compute_campaign_spec_diff ignoring omitted keys. Docs: CLAUDE.md spec-fidelity section, campaign-authoring-guide.md, and friction-245-resolution.md updated for the omission-vs-deviation distinction. Closes #298 Co-Authored-By: Claude Opus 4.8 (1M context) --- CLAUDE.md | 9 +- docs/campaign-authoring-guide.md | 16 ++++ docs/friction-245-resolution.md | 2 +- orchestrator/validate.py | 64 +++++++++++--- tests/test_friction_245.py | 144 +++++++++++++++++++++++++++---- 5 files changed, 208 insertions(+), 27 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 9ad0ad2..baf5335 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -153,11 +153,18 @@ for the schema. Every campaign should declare ``locked_parameters`` (and, when applicable, ``locked_workload``) for every knob whose deviation would invalidate the experiment. The validator hard-fails any -bundle whose ``experiment_spec.verified_parameters`` deviates from +bundle whose ``experiment_spec.verified_parameters`` *deviates from* ``locked_parameters`` — regardless of ``--auto-approve``. This closes the spec-fidelity gap that allowed paper-memorytime-mirage iter-1 to silently rewrite four locked workload parameters. +Issue #298 refines the "missing key" case: a locked key that the +bundle simply *omits* is not a violation (the campaign is the source +of truth), so the validator **auto-populates** it into +``verified_parameters`` and persists the bundle, rather than +hard-failing. Only a value the bundle sets *differently* from the +campaign fails. + Authoring discipline lives in ``docs/campaign-authoring-guide.md`` (the "what to lock" inventory + the rehearsal-as-instrument worked example). The full friction-report resolution map is in diff --git a/docs/campaign-authoring-guide.md b/docs/campaign-authoring-guide.md index 7e9ff96..8643fca 100644 --- a/docs/campaign-authoring-guide.md +++ b/docs/campaign-authoring-guide.md @@ -125,6 +125,22 @@ campaign's intent, regardless of ``--auto-approve``. Use them liberally — they are the cheapest defense against silent design- agent rewrites. +**Omission vs deviation (#298).** The design gate distinguishes two +cases for a ``locked_parameters`` key: + +- **Omitted** — the bundle's ``experiment_spec.verified_parameters`` + simply doesn't echo the key. No information is lost (the campaign + already declares it), so the gate **auto-populates** the value from + ``locked_parameters`` and continues, persisting it back to + ``bundle.yaml``. Agents no longer have to copy every locked key + verbatim just to clear the gate. +- **Deviating** — the bundle sets a *different* value than the + campaign locked. This is the real spec-fidelity violation and still + hard-fails, listing every deviation in one shot. + +So the campaign is always the source of truth for a locked value; the +only way to fail is to actively contradict it. + ## Reproducibility (#262 / F17) nous auto-captures ``reproducibility_metadata`` at INIT (target diff --git a/docs/friction-245-resolution.md b/docs/friction-245-resolution.md index 189dd1a..58ad731 100644 --- a/docs/friction-245-resolution.md +++ b/docs/friction-245-resolution.md @@ -8,7 +8,7 @@ implementation in one hop. | F | Issue | Severity | Resolution | Tests | |---|---|---|---|---| -| F1 | [#246](https://github.com/AI-native-Systems-Research/agentic-strategy-evolution/issues/246) | HIGH | `campaign.locked_parameters` schema field + `_validate_locked_parameters` in `validate.py` (hard-fails regardless of `--auto-approve`) | `tests/test_friction_245.py::test_f1_*` | +| F1 | [#246](https://github.com/AI-native-Systems-Research/agentic-strategy-evolution/issues/246) | HIGH | `campaign.locked_parameters` schema field + `_validate_locked_parameters` in `validate.py` (hard-fails on *deviation* regardless of `--auto-approve`; #298 auto-populates *omitted* keys from the campaign and persists them, rather than failing) | `tests/test_friction_245.py::test_f1_*`, `::test_298_*` | | F2 | [#247](https://github.com/AI-native-Systems-Research/agentic-strategy-evolution/issues/247) | MED-HIGH | "Source-of-truth hierarchy" section added to `prompts/methodology/design.md` with worked example | (prompt-text — no behavioral test) | | F3 | [#248](https://github.com/AI-native-Systems-Research/agentic-strategy-evolution/issues/248) | MED | `rehearsal_subset.depth_overrides` + `invalidates_checks` schema; `_validate_depth_overrides` enforces non-empty `invalidates_checks` whenever any depth payload is set; methodology prompt explains the breadth-vs-depth distinction | `tests/test_friction_245.py::test_f3_*` | | F4 | [#249](https://github.com/AI-native-Systems-Research/agentic-strategy-evolution/issues/249) | HIGH | `compute_campaign_spec_diff` in `validate.py` + `_augment_summary_with_spec_diff` in `iteration.py` writes `campaign_spec_diff` block into every `gate_summary_design.json`, regardless of `--auto-approve`. `nous status` surfaces it | `tests/test_friction_245.py::test_f4_*` | diff --git a/orchestrator/validate.py b/orchestrator/validate.py index 1bcdd8d..13e6acc 100644 --- a/orchestrator/validate.py +++ b/orchestrator/validate.py @@ -13,6 +13,8 @@ import jsonschema import yaml +from orchestrator.util import atomic_write + SCHEMAS_DIR = Path(__file__).resolve().parent / "schemas" @@ -211,7 +213,7 @@ def validate_principles_have_empirical_content( def _validate_locked_parameters( bundle: dict, campaign: dict | None, ) -> list[str]: - """Issue #246 (F1): hard-fail when bundle deviates from campaign.locked_parameters. + """Issue #246 (F1): hard-fail when bundle *deviates* from campaign.locked_parameters. Closes the spec-fidelity gap left by HUMAN_DESIGN_GATE bypass under --auto-approve. The bundle's ``experiment_spec.verified_parameters`` @@ -222,6 +224,19 @@ def _validate_locked_parameters( guess (paper-memorytime-mirage iter-1: ``model``, ``concurrency``, ``duration``, ``warmup`` all overwritten). + #298: distinguish two very different cases that #246 originally + conflated — + + * **Omitted key** — the agent simply didn't echo a value the + campaign already declares. No information is lost, so this is not a + spec-fidelity violation. The campaign is the unambiguous source of + truth, so we **auto-populate** ``verified_parameters[k]`` from + ``campaign.locked_parameters[k]`` (mutating ``bundle`` in place so + the caller can persist it) and continue. + * **Deviating value** — the agent wrote a *different* value than the + campaign locked. This is the real anti-mirage violation and still + hard-fails. + The error message lists EVERY deviation in one shot, not just the first — so a single re-run of the gate sees the full diff. """ @@ -230,20 +245,35 @@ def _validate_locked_parameters( locked = campaign.get("locked_parameters") if not isinstance(locked, dict) or not locked: return [] - spec = bundle.get("experiment_spec") or {} - verified = spec.get("verified_parameters") or {} - if not isinstance(verified, dict): + + # #298: ensure a real experiment_spec / verified_parameters dict is + # attached to the bundle so auto-filled values survive in the object + # the caller holds (and can write back to disk). A present-but-wrong + # TYPE is still a hard-fail — we auto-fill omissions, not structurally + # broken bundles. + spec = bundle.get("experiment_spec") + if spec is None: + spec = bundle["experiment_spec"] = {} + elif not isinstance(spec, dict): + return [ + "campaign.locked_parameters is set but " + "bundle.experiment_spec is malformed (not a mapping); " + "cannot verify spec-fidelity (#246)." + ] + verified = spec.get("verified_parameters") + if verified is None: + verified = spec["verified_parameters"] = {} + elif not isinstance(verified, dict): return [ "campaign.locked_parameters is set but " - "bundle.experiment_spec.verified_parameters is missing or " - "malformed; cannot verify spec-fidelity (#246)." + "bundle.experiment_spec.verified_parameters is malformed " + "(not a mapping); cannot verify spec-fidelity (#246)." ] + deviations: list[str] = [] for key, expected in locked.items(): if key not in verified: - deviations.append( - f" - {key}: campaign={expected!r}, bundle=" - ) + verified[key] = expected # #298: auto-populate; campaign wins continue actual = verified[key] if actual != expected: @@ -501,7 +531,13 @@ def compute_campaign_spec_diff( locked = (campaign or {}).get("locked_parameters") or {} if isinstance(locked, dict) and isinstance(verified, dict): for k, expected in locked.items(): - actual = verified.get(k, "") + # #298: an omitted key is auto-populated from the campaign by + # the hard-fail layer (_validate_locked_parameters), so it is + # NOT a spec-fidelity violation. Only a value that is PRESENT + # and DIFFERENT is a real deviation. + if k not in verified: + continue + actual = verified[k] if actual != expected: diff["locked_parameters_violations"].append( {"param": k, "campaign": expected, "bundle": actual} @@ -555,9 +591,17 @@ def validate_design(iter_dir: Path, campaign: dict | None = None) -> dict: schema = _load_yaml_schema("bundle.schema.yaml") jsonschema.validate(bundle, schema) errors.extend(_validate_typed_arm_fields(bundle)) + # #298: _validate_locked_parameters may auto-populate omitted + # locked values into the bundle in place. Snapshot first so we + # can detect the mutation and persist it back to disk below. + bundle_before = json.dumps(bundle, sort_keys=True, default=str) # #246 (F1): locked_parameters spec-fidelity. Hard-fail under # auto-approve too — that's the whole point. errors.extend(_validate_locked_parameters(bundle, campaign)) + # #298: persist auto-filled verified_parameters so downstream + # phases read the campaign's canonical values from bundle.yaml. + if json.dumps(bundle, sort_keys=True, default=str) != bundle_before: + atomic_write(bundle_path, yaml.safe_dump(bundle, sort_keys=False)) # #265 (F20): locked_workload diff against bundle.inputs/*.yaml. errors.extend(_validate_locked_workload(iter_dir, bundle, campaign)) # #248 (F3): depth_overrides without invalidates_checks. diff --git a/tests/test_friction_245.py b/tests/test_friction_245.py index fbf9e48..0099ca6 100644 --- a/tests/test_friction_245.py +++ b/tests/test_friction_245.py @@ -64,30 +64,74 @@ def test_f1_locked_parameters_fail_lists_all_deviations(): assert "duration_seconds" not in msg.split("\n")[-1] -def test_f1_locked_parameters_missing_verified_parameters_fails(): - """When the locked parameter has no entry in verified_parameters, the - validator reports it as a deviation (with bundle=) — same - path as a value mismatch, so the user sees one consistent message - format regardless of which side is responsible.""" +def test_298_missing_key_autofills_from_campaign(): + """#298: a locked parameter with no entry in verified_parameters is + NOT a spec-fidelity violation — the campaign is the source of truth. + The validator auto-populates the omitted value into the bundle and + passes, instead of hard-failing the whole iteration on pure friction. + """ campaign = {"locked_parameters": {"model": "llama"}} bundle = {"experiment_spec": {"verified_parameters": {}}} errors = _validate_locked_parameters(bundle, campaign) + assert errors == [] + # The omitted value is filled in from the campaign, in place. + assert bundle["experiment_spec"]["verified_parameters"]["model"] == "llama" + + +def test_298_missing_verified_parameters_block_autofills(): + """#298: when experiment_spec lacks a verified_parameters block + entirely, the validator creates one and auto-populates every locked + key rather than hard-failing.""" + campaign = {"locked_parameters": {"model": "llama", "concurrency": 32}} + bundle = {"experiment_spec": {}} + errors = _validate_locked_parameters(bundle, campaign) + assert errors == [] + assert bundle["experiment_spec"]["verified_parameters"] == { + "model": "llama", "concurrency": 32, + } + + +def test_298_missing_experiment_spec_block_autofills(): + """#298: even when the bundle has no experiment_spec at all, the + locked values are auto-populated (campaign is source of truth).""" + campaign = {"locked_parameters": {"model": "llama"}} + bundle = {} + errors = _validate_locked_parameters(bundle, campaign) + assert errors == [] + assert bundle["experiment_spec"]["verified_parameters"]["model"] == "llama" + + +def test_298_mixed_missing_and_mismatch_fails_only_on_mismatch(): + """#298: an omitted key auto-fills while a *deviating* value still + hard-fails. The error lists only the real deviation, not the + auto-filled key.""" + campaign = {"locked_parameters": { + "model": "llama", "concurrency": 32, "duration_seconds": 600, + }} + bundle = {"experiment_spec": {"verified_parameters": { + "model": "qwen", # deviation → hard-fail + "duration_seconds": 600, # match → fine + # concurrency omitted → auto-fill + }}} + errors = _validate_locked_parameters(bundle, campaign) assert len(errors) == 1 - assert "" in errors[0] - assert "model" in errors[0] + msg = errors[0] + assert "model" in msg and "qwen" in msg + # The auto-filled key must NOT be reported as a deviation. + assert "concurrency" not in msg + # ...and it was populated from the campaign. + assert bundle["experiment_spec"]["verified_parameters"]["concurrency"] == 32 -def test_f1_locked_parameters_no_verified_parameters_block_fails_with_clear_message(): - """When experiment_spec lacks verified_parameters entirely (vs an - empty dict), surface a structured error pointing the user at the - bundle field they must populate.""" +def test_298_malformed_verified_parameters_still_fails(): + """#298: a verified_parameters that is present but the wrong TYPE + (not a dict) is still a hard-fail — we only auto-fill omissions, not + structurally broken bundles.""" campaign = {"locked_parameters": {"model": "llama"}} - bundle = {"experiment_spec": {}} + bundle = {"experiment_spec": {"verified_parameters": ["not", "a", "dict"]}} errors = _validate_locked_parameters(bundle, campaign) assert len(errors) == 1 - # Either form is acceptable — the missing dict path or the - # listed-as-deviation path. Both surface enough for the user to act. - assert "verified_parameters" in errors[0] or "" in errors[0] + assert "verified_parameters" in errors[0] def test_f1_locked_parameters_no_campaign_block_skips(): @@ -118,6 +162,56 @@ def test_f1_validate_design_hard_fails_under_locked_parameters_deviation(tmp_pat assert any("locked_parameters" in e for e in result["errors"]) +def test_298_validate_design_passes_and_persists_autofilled_params(tmp_path: Path): + """#298 end-to-end: a bundle that OMITS some locked_parameters keys + passes DESIGN, and the auto-filled values are written back to + bundle.yaml on disk so downstream phases read the campaign's values. + """ + iter_dir = tmp_path / "iter-1" + (iter_dir / "inputs").mkdir(parents=True) + (iter_dir / "results").mkdir() + (iter_dir / "patches").mkdir() + (iter_dir / "problem.md").write_text("test") + (iter_dir / "handoff_snapshot.md").write_text("test") + bundle = { + "metadata": {"iteration": 1, "family": "f", "research_question": "q"}, + "arms": [{"type": "h-main", "prediction": "p", "mechanism": "m", "diagnostic": "d"}], + # verified_parameters omits both locked keys entirely. + "experiment_spec": {"verified_parameters": {}}, + } + (iter_dir / "bundle.yaml").write_text(yaml.safe_dump(bundle)) + campaign = {"locked_parameters": {"model": "llama", "concurrency": 32}} + result = validate_design(iter_dir, campaign=campaign) + assert result["status"] == "pass", result + # Auto-filled values are persisted to disk. + on_disk = yaml.safe_load((iter_dir / "bundle.yaml").read_text()) + assert on_disk["experiment_spec"]["verified_parameters"] == { + "model": "llama", "concurrency": 32, + } + + +def test_298_validate_design_still_fails_on_deviation(tmp_path: Path): + """#298: the anti-mirage protection (#246) is preserved — a bundle + that SETS a value different from the campaign still hard-fails, even + though sibling keys may be auto-filled.""" + iter_dir = tmp_path / "iter-1" + (iter_dir / "inputs").mkdir(parents=True) + (iter_dir / "results").mkdir() + (iter_dir / "patches").mkdir() + (iter_dir / "problem.md").write_text("test") + (iter_dir / "handoff_snapshot.md").write_text("test") + bundle = { + "metadata": {"iteration": 1, "family": "f", "research_question": "q"}, + "arms": [{"type": "h-main", "prediction": "p", "mechanism": "m", "diagnostic": "d"}], + "experiment_spec": {"verified_parameters": {"model": "qwen"}}, # deviates + } + (iter_dir / "bundle.yaml").write_text(yaml.safe_dump(bundle)) + campaign = {"locked_parameters": {"model": "llama", "concurrency": 32}} + result = validate_design(iter_dir, campaign=campaign) + assert result["status"] == "fail" + assert any("locked_parameters" in e for e in result["errors"]) + + # ─── F3 / #248: depth_overrides + invalidates_checks ─────────────────────── @@ -187,6 +281,26 @@ def test_f4_compute_campaign_spec_diff_clean_when_match(tmp_path: Path): assert diff["workload_changes_from_canonical_declared"] is False +def test_298_compute_campaign_spec_diff_ignores_omitted_keys(tmp_path: Path): + """#298: the soft-audit mirror must not re-flag auto-filled (omitted) + keys as violations. Only a value that is PRESENT and DIFFERENT counts. + """ + iter_dir = tmp_path / "iter-1" + iter_dir.mkdir() + (iter_dir / "bundle.yaml").write_text(yaml.safe_dump({ + "metadata": {"iteration": 1, "family": "f", "research_question": "q"}, + "arms": [{"type": "h-main", "prediction": "p", "mechanism": "m", "diagnostic": "d"}], + "experiment_spec": {"verified_parameters": { + "model": "qwen", # present + different → violation + # concurrency omitted → auto-filled, must NOT be a violation + }}, + })) + campaign = {"locked_parameters": {"model": "llama", "concurrency": 32}} + diff = compute_campaign_spec_diff(iter_dir, campaign) + violated = {v["param"] for v in diff["locked_parameters_violations"]} + assert violated == {"model"} + + # ─── F15 / #260: physical_realism_check soft warning ─────────────────────── From 87760c5627c598badd35ffedc9d9e1886795e87e Mon Sep 17 00:00:00 2001 From: Nick Masluk Date: Mon, 13 Jul 2026 20:12:23 +0000 Subject: [PATCH 2/2] =?UTF-8?q?fix(validate):=20address=20#299=20review=20?= =?UTF-8?q?=E2=80=94=20gate-pass-only=20persist,=20WARN=20on=20auto-fill,?= =?UTF-8?q?=20structured=20OSError=20(#298)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to PR #299 review (susiejojo). The auto-fill change turned a previously pure validator into one with a disk side-effect; this makes that side-effect safe and observable. 1. Persist only on PASS (review pt.1). `atomic_write` ran immediately after `_validate_locked_parameters`, before the remaining design validators. A run that ultimately returned status:"fail" (including the mixed omit+deviate case) could still leave a gate-rewritten bundle.yaml on disk. The write now happens once, just before the final return, gated on `not errors and bundle_mutated`. A failing gate leaves the file exactly as the agent authored it. 2. Auto-fill is no longer silent (review pt.2). _validate_locked_parameters now returns a WARN:-prefixed advisory naming every injected key (same convention as _validate_physical_realism / #85). validate_design routes it to `warnings`, so the human gate has a record of which verified_parameters were gate-injected vs agent-authored. compute_campaign_spec_diff still reports clean. 3. Structured OSError (review pt.3). The persist write is wrapped so a read-only FS / ENOSPC / permission error returns {"status":"fail","errors":[...]} instead of escaping as a raw traceback from a function whose contract is {"status","errors"}. 4. Mutation detection uses copy.deepcopy + structural `!=` instead of json.dumps(..., default=str) string-rendering compare (review minor) — exact and future-proof. Tests: WARN-contract assertions on the unit tests; new test_298_malformed_experiment_spec_still_fails (review pt.4), test_298_no_persist_when_gate_fails, test_298_no_spurious_write_when_all_present, test_298_persist_is_idempotent, and test_298_compute_campaign_spec_diff_no_experiment_spec_is_all_omitted. compute_campaign_spec_diff comment reworded to credit the local guard, not call-ordering. Full suite: 1344 passed. Refs #298 Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/campaign-authoring-guide.md | 7 +- orchestrator/validate.py | 103 +++++++++++++++----- tests/test_friction_245.py | 161 +++++++++++++++++++++++++++---- 3 files changed, 223 insertions(+), 48 deletions(-) diff --git a/docs/campaign-authoring-guide.md b/docs/campaign-authoring-guide.md index 8643fca..b158cd6 100644 --- a/docs/campaign-authoring-guide.md +++ b/docs/campaign-authoring-guide.md @@ -131,8 +131,11 @@ cases for a ``locked_parameters`` key: - **Omitted** — the bundle's ``experiment_spec.verified_parameters`` simply doesn't echo the key. No information is lost (the campaign already declares it), so the gate **auto-populates** the value from - ``locked_parameters`` and continues, persisting it back to - ``bundle.yaml``. Agents no longer have to copy every locked key + ``locked_parameters`` and continues, surfacing a ``WARN`` at the + design gate that names each injected key (so the fill is recorded, + not silent). The filled bundle is persisted back to ``bundle.yaml`` + only when the gate passes — a failed gate leaves the file exactly as + the agent authored it. Agents no longer have to copy every locked key verbatim just to clear the gate. - **Deviating** — the bundle sets a *different* value than the campaign locked. This is the real spec-fidelity violation and still diff --git a/orchestrator/validate.py b/orchestrator/validate.py index 13e6acc..e15c6ff 100644 --- a/orchestrator/validate.py +++ b/orchestrator/validate.py @@ -6,6 +6,7 @@ python -m orchestrator.validate meta-findings --dir / """ import argparse +import copy import json import sys from pathlib import Path @@ -232,13 +233,18 @@ def _validate_locked_parameters( spec-fidelity violation. The campaign is the unambiguous source of truth, so we **auto-populate** ``verified_parameters[k]`` from ``campaign.locked_parameters[k]`` (mutating ``bundle`` in place so - the caller can persist it) and continue. + the caller can persist it) and emit a ``WARN:``-prefixed advisory + naming the injected keys — so the auto-fill is recorded at the gate, + not silent. * **Deviating value** — the agent wrote a *different* value than the campaign locked. This is the real anti-mirage violation and still hard-fails. - The error message lists EVERY deviation in one shot, not just the - first — so a single re-run of the gate sees the full diff. + Returns a mixed list of hard errors and ``WARN:``-prefixed advisories; + the caller (``validate_design``) splits them the same way it splits + the physical-realism / ground-truth warnings. The hard-error message + lists EVERY deviation in one shot, not just the first — so a single + re-run of the gate sees the full diff. """ if not campaign: return [] @@ -271,22 +277,36 @@ def _validate_locked_parameters( ] deviations: list[str] = [] + autofilled: list[str] = [] for key, expected in locked.items(): if key not in verified: verified[key] = expected # #298: auto-populate; campaign wins + autofilled.append(f"{key}={expected!r}") continue actual = verified[key] if actual != expected: deviations.append( f" - {key}: campaign={expected!r}, bundle={actual!r}" ) - if not deviations: - return [] - return [ - "bundle.experiment_spec.verified_parameters deviates from " - "campaign.locked_parameters (#246/F1). Each entry must match " - "exactly:\n" + "\n".join(deviations) - ] + out: list[str] = [] + if deviations: + out.append( + "bundle.experiment_spec.verified_parameters deviates from " + "campaign.locked_parameters (#246/F1). Each entry must match " + "exactly:\n" + "\n".join(deviations) + ) + if autofilled: + # #298: auto-fill is defensible (campaign is source of truth) but + # must not be silent — surface it as a WARN so the human gate has + # a record of which verified_parameters were gate-injected rather + # than agent-authored. WARN: prefix routes it to warnings, not + # errors (same convention as _validate_physical_realism / #85). + out.append( + "WARN: auto-populated verified_parameters from " + "campaign.locked_parameters (#298; campaign is source of " + "truth for omitted keys): " + ", ".join(autofilled) + ) + return out def _validate_locked_workload( @@ -531,10 +551,12 @@ def compute_campaign_spec_diff( locked = (campaign or {}).get("locked_parameters") or {} if isinstance(locked, dict) and isinstance(verified, dict): for k, expected in locked.items(): - # #298: an omitted key is auto-populated from the campaign by - # the hard-fail layer (_validate_locked_parameters), so it is - # NOT a spec-fidelity violation. Only a value that is PRESENT - # and DIFFERENT is a real deviation. + # #298: an omitted locked key is not a spec-fidelity violation + # (the campaign is the source of truth; the hard-fail layer + # auto-populates it). Only a value that is PRESENT and DIFFERENT + # is a real deviation. This guard is self-contained — it does + # not rely on _validate_locked_parameters having run first, so + # it stays correct for a direct or out-of-order caller. if k not in verified: continue actual = verified[k] @@ -583,6 +605,13 @@ def validate_design(iter_dir: Path, campaign: dict | None = None) -> dict: # bundle.yaml bundle_path = iter_dir / "bundle.yaml" + # #298: _validate_locked_parameters may auto-populate omitted locked + # values into the bundle in place. We defer persisting that mutation + # until we know the gate PASSES — a failed gate must not leave a + # gate-rewritten artifact on disk. These carry the bundle + snapshot + # from inside the try to the persist step after the final verdict. + bundle_to_persist: dict | None = None + bundle_snapshot: dict | None = None if not bundle_path.exists(): errors.append("bundle.yaml not found") else: @@ -591,17 +620,19 @@ def validate_design(iter_dir: Path, campaign: dict | None = None) -> dict: schema = _load_yaml_schema("bundle.schema.yaml") jsonschema.validate(bundle, schema) errors.extend(_validate_typed_arm_fields(bundle)) - # #298: _validate_locked_parameters may auto-populate omitted - # locked values into the bundle in place. Snapshot first so we - # can detect the mutation and persist it back to disk below. - bundle_before = json.dumps(bundle, sort_keys=True, default=str) - # #246 (F1): locked_parameters spec-fidelity. Hard-fail under - # auto-approve too — that's the whole point. - errors.extend(_validate_locked_parameters(bundle, campaign)) - # #298: persist auto-filled verified_parameters so downstream - # phases read the campaign's canonical values from bundle.yaml. - if json.dumps(bundle, sort_keys=True, default=str) != bundle_before: - atomic_write(bundle_path, yaml.safe_dump(bundle, sort_keys=False)) + # #298: deepcopy snapshot so we can detect the in-place + # auto-fill by exact structural compare (not string rendering, + # which would conflate 1 and "1"). + bundle_snapshot = copy.deepcopy(bundle) + bundle_to_persist = bundle + # #246 (F1): locked_parameters spec-fidelity. Hard-fail on + # DEVIATION under auto-approve too — that's the whole point. + # #298: omitted keys auto-fill and come back as WARN advisories. + for entry in _validate_locked_parameters(bundle, campaign): + if entry.startswith("WARN:"): + warnings.append(entry) + else: + errors.append(entry) # #265 (F20): locked_workload diff against bundle.inputs/*.yaml. errors.extend(_validate_locked_workload(iter_dir, bundle, campaign)) # #248 (F3): depth_overrides without invalidates_checks. @@ -639,6 +670,28 @@ def validate_design(iter_dir: Path, campaign: dict | None = None) -> dict: required = _campaign_required_iter_root(campaign) errors.extend(_check_unexpected_files(iter_dir, extensions | required)) + # #298: persist the auto-filled bundle ONLY when the whole gate passes, + # and only if it was actually mutated (no spurious rewrite when every + # locked key was already present). A failing gate leaves the on-disk + # bundle exactly as the agent authored it — no gate-injected values. + if ( + not errors + and bundle_to_persist is not None + and bundle_to_persist != bundle_snapshot + ): + try: + atomic_write( + bundle_path, yaml.safe_dump(bundle_to_persist, sort_keys=False) + ) + except OSError as exc: + # Keep the {"status", "errors"} contract instead of letting a + # raw disk error escape from a validation function. + return { + "status": "fail", + "errors": [f"failed to persist auto-filled bundle.yaml: {exc}"], + "warnings": warnings, + } + if errors: return {"status": "fail", "errors": errors, "warnings": warnings} return {"status": "pass", "warnings": warnings} diff --git a/tests/test_friction_245.py b/tests/test_friction_245.py index 0099ca6..b70ad72 100644 --- a/tests/test_friction_245.py +++ b/tests/test_friction_245.py @@ -64,16 +64,28 @@ def test_f1_locked_parameters_fail_lists_all_deviations(): assert "duration_seconds" not in msg.split("\n")[-1] -def test_298_missing_key_autofills_from_campaign(): +def _split_warns(entries): + """Partition _validate_locked_parameters output into (hard errors, + WARN advisories) — the same WARN:-prefix convention validate_design + uses to route entries to the design gate.""" + warns = [e for e in entries if e.startswith("WARN:")] + errs = [e for e in entries if not e.startswith("WARN:")] + return errs, warns + + +def test_298_missing_key_autofills_and_warns(): """#298: a locked parameter with no entry in verified_parameters is NOT a spec-fidelity violation — the campaign is the source of truth. - The validator auto-populates the omitted value into the bundle and - passes, instead of hard-failing the whole iteration on pure friction. + The validator auto-populates the omitted value into the bundle (no + hard-fail) and records a WARN so the human gate sees the injection. """ campaign = {"locked_parameters": {"model": "llama"}} bundle = {"experiment_spec": {"verified_parameters": {}}} - errors = _validate_locked_parameters(bundle, campaign) - assert errors == [] + errs, warns = _split_warns(_validate_locked_parameters(bundle, campaign)) + assert errs == [] + # The auto-fill is surfaced, not silent, and names the key + value. + assert len(warns) == 1 + assert "model" in warns[0] and "llama" in warns[0] # The omitted value is filled in from the campaign, in place. assert bundle["experiment_spec"]["verified_parameters"]["model"] == "llama" @@ -81,11 +93,15 @@ def test_298_missing_key_autofills_from_campaign(): def test_298_missing_verified_parameters_block_autofills(): """#298: when experiment_spec lacks a verified_parameters block entirely, the validator creates one and auto-populates every locked - key rather than hard-failing.""" + key (one WARN each) rather than hard-failing.""" campaign = {"locked_parameters": {"model": "llama", "concurrency": 32}} bundle = {"experiment_spec": {}} - errors = _validate_locked_parameters(bundle, campaign) - assert errors == [] + errs, warns = _split_warns(_validate_locked_parameters(bundle, campaign)) + assert errs == [] + # One consolidated advisory that names every auto-filled key (same + # one-message-lists-all convention as the deviation error). + assert len(warns) == 1 + assert "model" in warns[0] and "concurrency" in warns[0] assert bundle["experiment_spec"]["verified_parameters"] == { "model": "llama", "concurrency": 32, } @@ -96,30 +112,32 @@ def test_298_missing_experiment_spec_block_autofills(): locked values are auto-populated (campaign is source of truth).""" campaign = {"locked_parameters": {"model": "llama"}} bundle = {} - errors = _validate_locked_parameters(bundle, campaign) - assert errors == [] + errs, warns = _split_warns(_validate_locked_parameters(bundle, campaign)) + assert errs == [] + assert len(warns) == 1 assert bundle["experiment_spec"]["verified_parameters"]["model"] == "llama" def test_298_mixed_missing_and_mismatch_fails_only_on_mismatch(): - """#298: an omitted key auto-fills while a *deviating* value still - hard-fails. The error lists only the real deviation, not the - auto-filled key.""" + """#298: an omitted key auto-fills (WARN) while a *deviating* value + still hard-fails. The hard error lists only the real deviation, not + the auto-filled key.""" campaign = {"locked_parameters": { "model": "llama", "concurrency": 32, "duration_seconds": 600, }} bundle = {"experiment_spec": {"verified_parameters": { "model": "qwen", # deviation → hard-fail "duration_seconds": 600, # match → fine - # concurrency omitted → auto-fill + # concurrency omitted → auto-fill (WARN) }}} - errors = _validate_locked_parameters(bundle, campaign) - assert len(errors) == 1 - msg = errors[0] + errs, warns = _split_warns(_validate_locked_parameters(bundle, campaign)) + assert len(errs) == 1 + msg = errs[0] assert "model" in msg and "qwen" in msg # The auto-filled key must NOT be reported as a deviation. assert "concurrency" not in msg - # ...and it was populated from the campaign. + # ...it is instead surfaced as a WARN, and populated from the campaign. + assert any("concurrency" in w for w in warns) assert bundle["experiment_spec"]["verified_parameters"]["concurrency"] == 32 @@ -129,9 +147,22 @@ def test_298_malformed_verified_parameters_still_fails(): structurally broken bundles.""" campaign = {"locked_parameters": {"model": "llama"}} bundle = {"experiment_spec": {"verified_parameters": ["not", "a", "dict"]}} - errors = _validate_locked_parameters(bundle, campaign) - assert len(errors) == 1 - assert "verified_parameters" in errors[0] + errs, warns = _split_warns(_validate_locked_parameters(bundle, campaign)) + assert len(errs) == 1 + assert "verified_parameters" in errs[0] + assert warns == [] + + +def test_298_malformed_experiment_spec_still_fails(): + """#298: an experiment_spec that is present but the wrong TYPE (not a + mapping) is a hard-fail — guards against a regression that coerces a + non-dict spec to {} and silently drops the check.""" + campaign = {"locked_parameters": {"model": "llama"}} + bundle = {"experiment_spec": ["not", "a", "dict"]} + errs, warns = _split_warns(_validate_locked_parameters(bundle, campaign)) + assert len(errs) == 1 + assert "experiment_spec" in errs[0] + assert warns == [] def test_f1_locked_parameters_no_campaign_block_skips(): @@ -183,6 +214,10 @@ def test_298_validate_design_passes_and_persists_autofilled_params(tmp_path: Pat campaign = {"locked_parameters": {"model": "llama", "concurrency": 32}} result = validate_design(iter_dir, campaign=campaign) assert result["status"] == "pass", result + # The auto-fill is surfaced to the design gate as a warning, not silent. + assert any( + "model" in w or "concurrency" in w for w in result.get("warnings", []) + ) # Auto-filled values are persisted to disk. on_disk = yaml.safe_load((iter_dir / "bundle.yaml").read_text()) assert on_disk["experiment_spec"]["verified_parameters"] == { @@ -212,6 +247,75 @@ def test_298_validate_design_still_fails_on_deviation(tmp_path: Path): assert any("locked_parameters" in e for e in result["errors"]) +def _write_iter(tmp_path: Path, bundle: dict) -> Path: + """Scaffold a minimal valid iter-dir with the given bundle.""" + iter_dir = tmp_path / "iter-1" + (iter_dir / "inputs").mkdir(parents=True) + (iter_dir / "results").mkdir() + (iter_dir / "patches").mkdir() + (iter_dir / "problem.md").write_text("test") + (iter_dir / "handoff_snapshot.md").write_text("test") + (iter_dir / "bundle.yaml").write_text(yaml.safe_dump(bundle)) + return iter_dir + + +def test_298_no_persist_when_gate_fails(tmp_path: Path): + """#298 review pt.1: a failing gate must NOT leave a rewritten + bundle.yaml behind. Here an omitted key (would auto-fill) coexists + with a deviation (hard-fail); the on-disk bundle must be byte-for-byte + what the agent authored, with no gate-injected values. + """ + bundle = { + "metadata": {"iteration": 1, "family": "f", "research_question": "q"}, + "arms": [{"type": "h-main", "prediction": "p", "mechanism": "m", "diagnostic": "d"}], + "experiment_spec": {"verified_parameters": {"model": "qwen"}}, # deviates + } + iter_dir = _write_iter(tmp_path, bundle) + original = (iter_dir / "bundle.yaml").read_text() + campaign = {"locked_parameters": {"model": "llama", "concurrency": 32}} + result = validate_design(iter_dir, campaign=campaign) + assert result["status"] == "fail" + # Not persisted: no auto-filled "concurrency", file unchanged. + assert (iter_dir / "bundle.yaml").read_text() == original + on_disk = yaml.safe_load((iter_dir / "bundle.yaml").read_text()) + assert "concurrency" not in on_disk["experiment_spec"]["verified_parameters"] + + +def test_298_no_spurious_write_when_all_present(tmp_path: Path): + """#298 review minor: when every locked key is already present and + matching, nothing is auto-filled, so bundle.yaml must be left + untouched (no needless rewrite / reformat).""" + bundle = { + "metadata": {"iteration": 1, "family": "f", "research_question": "q"}, + "arms": [{"type": "h-main", "prediction": "p", "mechanism": "m", "diagnostic": "d"}], + "experiment_spec": {"verified_parameters": {"model": "llama", "concurrency": 32}}, + } + iter_dir = _write_iter(tmp_path, bundle) + original = (iter_dir / "bundle.yaml").read_text() + campaign = {"locked_parameters": {"model": "llama", "concurrency": 32}} + result = validate_design(iter_dir, campaign=campaign) + assert result["status"] == "pass", result + assert (iter_dir / "bundle.yaml").read_text() == original + + +def test_298_persist_is_idempotent(tmp_path: Path): + """#298 review minor: running the gate twice on an auto-filled bundle + yields identical bytes the second time — the first run persisted the + fill, the second finds nothing to change.""" + bundle = { + "metadata": {"iteration": 1, "family": "f", "research_question": "q"}, + "arms": [{"type": "h-main", "prediction": "p", "mechanism": "m", "diagnostic": "d"}], + "experiment_spec": {"verified_parameters": {}}, + } + iter_dir = _write_iter(tmp_path, bundle) + campaign = {"locked_parameters": {"model": "llama", "concurrency": 32}} + assert validate_design(iter_dir, campaign=campaign)["status"] == "pass" + after_first = (iter_dir / "bundle.yaml").read_text() + assert validate_design(iter_dir, campaign=campaign)["status"] == "pass" + after_second = (iter_dir / "bundle.yaml").read_text() + assert after_first == after_second + + # ─── F3 / #248: depth_overrides + invalidates_checks ─────────────────────── @@ -301,6 +405,21 @@ def test_298_compute_campaign_spec_diff_ignores_omitted_keys(tmp_path: Path): assert violated == {"model"} +def test_298_compute_campaign_spec_diff_no_experiment_spec_is_all_omitted(tmp_path: Path): + """#298: a bundle with no experiment_spec at all has every locked key + omitted — none should be flagged as a violation (all auto-fill).""" + iter_dir = tmp_path / "iter-1" + iter_dir.mkdir() + (iter_dir / "bundle.yaml").write_text(yaml.safe_dump({ + "metadata": {"iteration": 1, "family": "f", "research_question": "q"}, + "arms": [{"type": "h-main", "prediction": "p", "mechanism": "m", "diagnostic": "d"}], + # no experiment_spec key + })) + campaign = {"locked_parameters": {"model": "llama", "concurrency": 32}} + diff = compute_campaign_spec_diff(iter_dir, campaign) + assert diff["locked_parameters_violations"] == [] + + # ─── F15 / #260: physical_realism_check soft warning ───────────────────────