From 8c3151bdfb975218a502edebb13e26150fd21f80 Mon Sep 17 00:00:00 2001 From: okorzh Date: Mon, 12 Oct 2026 20:26:08 -0500 Subject: [PATCH 01/11] fix(integrate_patch): end a patch set the tree already holds as already_present A patch set whose every patch reverse-applies cleanly was benchmarked as a new candidate. The apply helpers now return a named PatchApplyResult that says when a patch was already present, the apply loop moves into apply_patch_set with a PatchSetOutcome, and the executor ends such a set as already_present without a benchmark, writing OUTCOME_ALREADY_PRESENT through the framework-KB writeback. A reverse check counts only at a strip level where a file the patch names resolves, so a deletion whose target drifted is not taken as present. An empty checkout-head diff is not proof on its own, because a reverted PR keeps its head in the history: with a diff_url the reverse check decides, and without one the tree must match the head on every changed file. Critic priors and the phase summary count the new status, an upstream-PR result keeps its task id so the enablement lane treats it like every other upstream-PR result, the dead semantic_status mapping is removed, and the operator docs describe the outcome. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/conceptual/optimization-loop.md | 6 +- src/hyperloom/inference_optimizer/SKILL.md | 10 +- .../actions/integrate_patch.md | 7 +- .../tests/test_framework_agent_authoring.py | 11 +- .../test_framework_agent_selection_audit.py | 71 +-- .../tests/test_patch_source_pr.py | 96 +++- .../actions/executors/_apply_feedback.py | 30 +- .../actions/executors/_nogit_patch.py | 95 ++-- .../actions/executors/_patch_set_apply.py | 220 ++++++++ .../actions/executors/_patch_source_pr.py | 122 +++-- .../actions/executors/baseline.py | 12 +- .../actions/executors/integrate_patch.py | 277 ++++------ .../tests/test_integrate_patch_executor.py | 12 +- .../test_integrate_patch_helpers_unit.py | 11 +- .../executors/tests/test_nogit_patch_unit.py | 241 +++++---- .../tests/test_patch_already_applied_unit.py | 476 ++++++++++++++++-- .../delivery/tests/test_delivery_channels.py | 4 +- .../orchestrator/framework/artifacts.py | 4 +- .../orchestrator/knowledge/kb_writeback.py | 2 +- .../orchestrator/phases/framework.py | 21 +- .../orchestrator/specialists/patch_safety.py | 42 +- 21 files changed, 1240 insertions(+), 530 deletions(-) create mode 100644 src/hyperloom/orchestrator/actions/executors/_patch_set_apply.py diff --git a/docs/conceptual/optimization-loop.md b/docs/conceptual/optimization-loop.md index 5ff404eed5..674697ef90 100644 --- a/docs/conceptual/optimization-loop.md +++ b/docs/conceptual/optimization-loop.md @@ -369,7 +369,11 @@ One phase, two levers worked in parallel: a specialist wrote, `upstream_pr` for a diff fetched from an upstream PR that a `candidate_discovery_specialist` found, ranked and judged. Both serialise on the `workspace_mutation` lane: one landing at a time, - independent of how fast proposals arrive. + independent of how fast proposals arrive. A candidate the tree already + holds — every patch reverse-applies cleanly, or the PR head is in the live + history and the tree still has its copy of every file the PR changes — and + that changes nothing else is not benchmarked: it ends `already_present` and + is recorded so in the framework KB. `specialist` serves both levers — investigation, patch authoring, and candidate discovery are all dispatches of the one specialist action. An diff --git a/src/hyperloom/inference_optimizer/SKILL.md b/src/hyperloom/inference_optimizer/SKILL.md index a39d889e89..ee22c73d5c 100644 --- a/src/hyperloom/inference_optimizer/SKILL.md +++ b/src/hyperloom/inference_optimizer/SKILL.md @@ -444,8 +444,14 @@ with that off too, the source arm reports itself dry. Every diff lands through `integrate_patch`, with `patch_source` naming where it came from. KEEP commits to the live tree so the next candidate stacks on -top; REVERT does `git reset --hard`. Resume skips completed candidates by -idempotency key. +top; REVERT does `git reset --hard`. A candidate the tree already holds is not +benchmarked: when every patch in the set reverse-applies cleanly and the round +changes nothing else, it ends `already_present`, with that evidence on the +result and an `already_present` row in the framework KB. An upstream PR whose +head is already in the live history ends the same way only when the tree still +has the head's copy of every file the PR changes; with a `diff_url`, the +fetched diff's reverse check decides instead, so a reverted PR is benchmarked. +Resume skips completed candidates by idempotency key. The phase exits when both arms are dry, when its budget is spent, or at the absolute per-phase cap. diff --git a/src/hyperloom/inference_optimizer/actions/integrate_patch.md b/src/hyperloom/inference_optimizer/actions/integrate_patch.md index ddab8dfe83..478cc59244 100644 --- a/src/hyperloom/inference_optimizer/actions/integrate_patch.md +++ b/src/hyperloom/inference_optimizer/actions/integrate_patch.md @@ -66,7 +66,9 @@ delegate{ cleanly. A patch that does not apply, and is not already in the tree, is REVERT-immediate (record `apply_failed`); a failed apply leaves the tree untouched, and the revert resets it to the HEAD recorded before - the attempt. + the attempt. When every patch is already in the tree and the task + changes nothing else, the action ends `already_present` here: no + restart, no benchmark. 4. Layer `config_changes` onto the server-launch env. 5. Stop the server this session launched, by the pid and process group it recorded; wait for VRAM to drain. No pattern-based kill runs. @@ -88,7 +90,7 @@ delegate{ ``` { - "status": "kept" | "reverted" | "apply_failed", + "status": "kept" | "reverted" | "apply_failed" | "already_present", "output_throughput": , "delta_pct": , "accuracy_pass": , @@ -112,6 +114,7 @@ hash and `optimization_stack` is the authoritative ledger. | Symptom | Cause | Mitigation | |---|---|---| | `git apply` rejects | base HEAD drifted under specialist | reject patch, record `apply_failed`, REVERT path | +| every patch is already in the tree | the change landed upstream or in an earlier round | `already_present`, with the reverse check per patch under `already_present`; framework KB row `already_present` | | server fails to start | broken patch | `git checkout` + restart prior config, mark REVERT | | accuracy drops >0.05 absolute | semantic regression | REVERT, record into pitfalls KB | | throughput unchanged within keep_threshold | no-op patch | REVERT, log low-confidence specialist for the gap | diff --git a/src/hyperloom/inference_optimizer/tests/test_framework_agent_authoring.py b/src/hyperloom/inference_optimizer/tests/test_framework_agent_authoring.py index f39c71921d..d82bd79246 100644 --- a/src/hyperloom/inference_optimizer/tests/test_framework_agent_authoring.py +++ b/src/hyperloom/inference_optimizer/tests/test_framework_agent_authoring.py @@ -654,7 +654,6 @@ def test_empty_outcome_fires_when_patch_dropped_by_vetting(tmp_path: Path): "framework_agent_authoring": True, "framework_agent_candidate_id": cand, "framework_batch_id": "b1", - "framework_audit": {"semantic_status": "not_present"}, }, ) done_payload = { @@ -671,7 +670,7 @@ def test_empty_outcome_fires_when_patch_dropped_by_vetting(tmp_path: Path): rows = stub.shared_state.framework_agent_phase_progress assert len(rows) == 1 assert rows[0]["candidate_id"] == cand - assert rows[0]["status"] == "not_applicable" + assert rows[0]["status"] == "author_empty" assert rows[0]["kept"] is False @@ -773,7 +772,6 @@ def test_empty_outcome_skips_when_config_levers_present(tmp_path: Path): "framework_agent_authoring": True, "framework_agent_candidate_id": "https://github.com/ROCm/vllm/pull/1014", "framework_batch_id": "b1", - "framework_audit": {"semantic_status": "not_present"}, }, ) done_payload = { @@ -818,7 +816,6 @@ def test_empty_outcome_skips_when_artifacts_written_routable(tmp_path: Path): "framework_agent_authoring": True, "framework_agent_candidate_id": "https://github.com/ROCm/aiter/pull/4130", "framework_batch_id": "b1", - "framework_audit": {"semantic_status": "not_present"}, }, ) done_payload = { @@ -853,7 +850,6 @@ def test_empty_outcome_stamps_when_artifacts_source_missing(tmp_path: Path): "framework_agent_authoring": True, "framework_agent_candidate_id": "https://github.com/ROCm/aiter/pull/9999", "framework_batch_id": "b1", - "framework_audit": {"semantic_status": "not_present"}, }, ) done_payload = { @@ -875,7 +871,7 @@ def test_empty_outcome_stamps_when_artifacts_source_missing(tmp_path: Path): ) rows = stub.shared_state.framework_agent_phase_progress assert len(rows) == 1 - assert rows[0]["status"] == "not_applicable" + assert rows[0]["status"] == "author_empty" assert rows[0]["kept"] is False @@ -898,7 +894,6 @@ def test_empty_outcome_stamps_when_artifacts_source_outside_sandbox(tmp_path: Pa "framework_agent_authoring": True, "framework_agent_candidate_id": "https://github.com/ROCm/aiter/pull/8888", "framework_batch_id": "b1", - "framework_audit": {"semantic_status": "not_present"}, }, ) done_payload = { @@ -919,7 +914,7 @@ def test_empty_outcome_stamps_when_artifacts_source_outside_sandbox(tmp_path: Pa ) rows = stub.shared_state.framework_agent_phase_progress assert len(rows) == 1 - assert rows[0]["status"] == "not_applicable" + assert rows[0]["status"] == "author_empty" assert rows[0]["kept"] is False diff --git a/src/hyperloom/inference_optimizer/tests/test_framework_agent_selection_audit.py b/src/hyperloom/inference_optimizer/tests/test_framework_agent_selection_audit.py index be1c06bd3d..07ddb91b71 100644 --- a/src/hyperloom/inference_optimizer/tests/test_framework_agent_selection_audit.py +++ b/src/hyperloom/inference_optimizer/tests/test_framework_agent_selection_audit.py @@ -121,15 +121,25 @@ def test_collect_framework_agent_candidate_priors(coord: Coordinator) -> None: {"candidate_id": "c1", "status": "kept", "gain_pct": 3.2}, {"candidate_id": "c2", "status": "in_flight"}, # non-terminal -> excluded {"candidate_id": "c3", "status": "critic_denied", "rationale": "off the bottleneck"}, + {"candidate_id": "c4", "status": "already_present"}, ] priors = coord.phase_framework._collect_framework_agent_candidate_priors() statuses = {o["status"] for o in priors["recent_outcomes"]} - assert statuses == {"kept", "critic_denied"} + assert statuses == {"kept", "critic_denied", "already_present"} # The denial reason has to reach the Critic, or the priors carry the verdict without the argument behind it. denied = next(o for o in priors["recent_outcomes"] if o["status"] == "critic_denied") assert denied["rationale"] == "off the bottleneck" +def test_an_already_present_candidate_counts_as_tested() -> None: + """It reached the apply stage, so the phase summary does not read it as a discovery that found nothing.""" + from hyperloom.orchestrator.framework.artifacts import summarize_candidate_outcomes + + summary = summarize_candidate_outcomes([{"candidate_id": "c1", "status": "already_present", "kept": False}]) + assert summary["tested"] == 1 + assert summary["outcome_class"] == "tested_no_keep" + + # _match_framework_agent_candidate @@ -221,51 +231,51 @@ def _fake(args, timeout=None): def test_materialize_pr_diff_success(monkeypatch, tmp_path) -> None: monkeypatch.setattr(fpr_mod, "_run_git", _scripted_run_git()) dest = tmp_path / "out" / "cand.patch" - ok, err = fpr_mod._materialize_pr_diff_from_head(tmp_path / "root", {"pr_number": 1015}, dest, timeout_sec=30.0) - assert ok is True and err == "" + out = fpr_mod._materialize_pr_diff_from_head(tmp_path / "root", {"pr_number": 1015}, dest, timeout_sec=30.0) + assert out.failure is None and out.patches == [dest.resolve()] assert dest.read_text().startswith("diff --git") def test_materialize_pr_diff_fetch_fails(monkeypatch, tmp_path) -> None: monkeypatch.setattr(fpr_mod, "_run_git", _scripted_run_git(fetch_ok=False)) - ok, err = fpr_mod._materialize_pr_diff_from_head( + out = fpr_mod._materialize_pr_diff_from_head( tmp_path / "root", {"ref": "refs/pull/1/head"}, tmp_path / "c.patch", timeout_sec=30.0 ) - assert ok is False and "git fetch" in err + assert out.failure is not None and "git fetch" in out.failure["error"] def test_materialize_pr_diff_empty_diff(monkeypatch, tmp_path) -> None: monkeypatch.setattr(fpr_mod, "_run_git", _scripted_run_git(diff_text=" \n")) - ok, err = fpr_mod._materialize_pr_diff_from_head( + out = fpr_mod._materialize_pr_diff_from_head( tmp_path / "root", {"head_sha": "deadbeef"}, tmp_path / "c.patch", timeout_sec=30.0 ) - assert ok is False and "empty diff" in err + assert out.failure is not None and "empty diff" in out.failure["error"] def test_materialize_pr_diff_no_head_resolvable(monkeypatch, tmp_path) -> None: monkeypatch.setattr(fpr_mod, "_run_git", _scripted_run_git()) - ok, err = fpr_mod._materialize_pr_diff_from_head(tmp_path / "root", {}, tmp_path / "c.patch", timeout_sec=30.0) - assert ok is False and "cannot resolve PR head" in err + out = fpr_mod._materialize_pr_diff_from_head(tmp_path / "root", {}, tmp_path / "c.patch", timeout_sec=30.0) + assert out.failure is not None and "cannot resolve PR head" in out.failure["error"] def test_materialize_pr_diff_checks_out_nothing(monkeypatch, tmp_path) -> None: """Both ends of the diff range are shas, so no tree has to be materialized.""" seen: list[str] = [] monkeypatch.setattr(fpr_mod, "_run_git", _scripted_run_git(seen=seen)) - ok, _err = fpr_mod._materialize_pr_diff_from_head( + out = fpr_mod._materialize_pr_diff_from_head( tmp_path / "root", {"head_sha": "deadbeef"}, tmp_path / "c.patch", timeout_sec=30.0 ) - assert ok is True + assert out.failure is None assert "worktree" not in seen def test_materialize_pr_diff_ignores_an_unusable_pr_number(monkeypatch, tmp_path) -> None: """The row reaches us from the KB and from LLM-authored proposals alike.""" monkeypatch.setattr(fpr_mod, "_run_git", _scripted_run_git()) - ok, err = fpr_mod._materialize_pr_diff_from_head( + out = fpr_mod._materialize_pr_diff_from_head( tmp_path / "root", {"pr_number": "not-a-number"}, tmp_path / "c.patch", timeout_sec=30.0 ) - assert ok is False and "cannot resolve PR head" in err + assert out.failure is not None and "cannot resolve PR head" in out.failure["error"] # Dispatch pause on a spent phase budget @@ -540,7 +550,7 @@ def test_record_authoring_empty_guards(coord: Coordinator) -> None: assert not (coord.shared_state.framework_agent_phase_progress or []) -def test_record_authoring_empty_already_present(coord: Coordinator) -> None: +def test_record_authoring_empty_stamps_author_empty_once(coord: Coordinator) -> None: _enter_fpr(coord) task = types.SimpleNamespace( task_id="spec-2", @@ -548,15 +558,14 @@ def test_record_authoring_empty_already_present(coord: Coordinator) -> None: "framework_agent_authoring": True, "framework_agent_candidate_id": "cand-2", "framework_batch_id": "b1", - "framework_audit": {"semantic_status": "already_equivalent"}, }, ) coord.phase_framework._record_framework_agent_authoring_empty_outcome( - task=task, done_payload={"payload": {"patches_written": [], "summary": "already there"}} + task=task, done_payload={"payload": {"patches_written": [], "summary": "nothing to write"}} ) row = coord.shared_state.framework_agent_phase_progress[-1] assert row["candidate_id"] == "cand-2" - assert row["status"] == "already_present" + assert row["status"] == "author_empty" # Idempotent: a second call does not append a duplicate. coord.phase_framework._record_framework_agent_authoring_empty_outcome( task=task, done_payload={"payload": {"patches_written": [], "summary": "again"}} @@ -564,32 +573,4 @@ def test_record_authoring_empty_already_present(coord: Coordinator) -> None: assert sum(1 for p in coord.shared_state.framework_agent_phase_progress if p["candidate_id"] == "cand-2") == 1 -def test_record_authoring_empty_status_variants(coord: Coordinator) -> None: - _enter_fpr(coord) - coord.shared_state.framework_agent_phase_progress = [] - # not_present -> not_applicable. - coord.phase_framework._record_framework_agent_authoring_empty_outcome( - task=types.SimpleNamespace( - task_id="s-a", - params={ - "framework_agent_authoring": True, - "framework_agent_candidate_id": "na-1", - "framework_audit": {"semantic_status": "not_present"}, - }, - ), - done_payload={"patches_written": [], "summary": "missing"}, - ) - # no audit -> author_empty. - coord.phase_framework._record_framework_agent_authoring_empty_outcome( - task=types.SimpleNamespace( - task_id="s-b", - params={"framework_agent_authoring": True, "framework_agent_candidate_id": "ae-1"}, - ), - done_payload={"patches_written": []}, - ) - statuses = {p["candidate_id"]: p["status"] for p in coord.shared_state.framework_agent_phase_progress} - assert statuses["na-1"] == "not_applicable" - assert statuses["ae-1"] == "author_empty" - - # Lenient ranker: an "applicable: false" reply never vetoes the phase diff --git a/src/hyperloom/inference_optimizer/tests/test_patch_source_pr.py b/src/hyperloom/inference_optimizer/tests/test_patch_source_pr.py index a82230276d..b5dc7129b7 100644 --- a/src/hyperloom/inference_optimizer/tests/test_patch_source_pr.py +++ b/src/hyperloom/inference_optimizer/tests/test_patch_source_pr.py @@ -10,6 +10,7 @@ from pathlib import Path from hyperloom.orchestrator.actions.executors._patch_source_pr import ( + PrMaterialization, _fetch_diff_to_path, _materialize_pr_diff_from_head, materialize_candidate_patches, @@ -92,8 +93,8 @@ def test_materialize_pr_diff_from_head_extracts_net_diff(tmp_path: Path): head_sha = _init_repo_with_pr_branch(repo, pr_ref="pr-head") dest = tmp_path / "out" / "pr.patch" cand = {"repo": "x/y", "pr_number": 7, "ref": "pr-head", "head_sha": head_sha} - ok, err = _materialize_pr_diff_from_head(repo, cand, dest, timeout_sec=60.0) - assert ok, err + out = _materialize_pr_diff_from_head(repo, cand, dest, timeout_sec=60.0) + assert out.failure is None, out.failure text = dest.read_text() assert "src.py" in text assert "return 2" in text @@ -105,9 +106,94 @@ def test_materialize_pr_diff_empty_when_no_ref(tmp_path: Path): repo = tmp_path / "framework" _init_repo_with_pr_branch(repo) dest = tmp_path / "pr.patch" - ok, err = _materialize_pr_diff_from_head(repo, {"repo": "x/y"}, dest, timeout_sec=30.0) - assert not ok - assert "cannot resolve PR head" in err or "head" in err.lower() + out = _materialize_pr_diff_from_head(repo, {"repo": "x/y"}, dest, timeout_sec=30.0) + assert out.failure is not None + assert "cannot resolve PR head" in out.failure["error"] + + +def _git(repo: Path, *args: str) -> None: + subprocess.run(["git", "-C", str(repo), *args], check=True, capture_output=True, env=_git_env()) + + +def _merge_pr_head(repo: Path, *, revert: bool) -> None: + """Merge ``pr-head`` into ``main`` with a merge commit, and optionally revert that merge.""" + (repo / "other.py").write_text("x = 1\n", encoding="utf-8") + _git(repo, "add", "other.py") + _git(repo, "commit", "-qm", "unrelated") + _git(repo, "merge", "-q", "--no-ff", "-m", "merge pr", "pr-head") + if revert: + _git(repo, "revert", "--no-edit", "-m", "1", "HEAD") + + +def _materialize_head(repo: Path, tmp_path: Path, head_sha: str, **extra: object) -> PrMaterialization: + return materialize_candidate_patches( + candidate={"repo": "x/y", "ref": "pr-head", "head_sha": head_sha, "prefer_checkout": True, **extra}, + params={}, + framework_root=repo, + output_root=tmp_path / "out", + slug="s", + diff_fetch_timeout_sec=30.0, + ) + + +def test_materialize_reports_a_merged_pr_head_as_already_present(tmp_path: Path): + """The tree has the head's copy of every file the PR changes: nothing to apply, nothing failed.""" + repo = tmp_path / "framework" + head_sha = _init_repo_with_pr_branch(repo) + _merge_pr_head(repo, revert=False) + + out = _materialize_head(repo, tmp_path, head_sha, changed_files=["src.py"]) + + assert out.failure is None + assert out.patches == [] + assert out.already_present == [{"diff_range": f"{head_sha}..{head_sha}", "check": "pr_head_files_in_tree"}] + + +def test_materialize_does_not_report_a_reverted_pr_head_as_already_present(tmp_path: Path): + """A reverted merge leaves the head in the history but not its change in the tree.""" + repo = tmp_path / "framework" + head_sha = _init_repo_with_pr_branch(repo) + _merge_pr_head(repo, revert=True) + + out = _materialize_head(repo, tmp_path, head_sha, changed_files=["src.py"]) + + assert out.already_present == [] + assert out.failure is not None and out.failure["status"] == "fetch_failed" + assert "empty diff" in out.failure["error"] + + +def test_materialize_needs_changed_files_that_exist_at_the_head(tmp_path: Path): + """A path that names nothing at the head would compare equal in any tree, so it proves nothing.""" + repo = tmp_path / "framework" + head_sha = _init_repo_with_pr_branch(repo) + _merge_pr_head(repo, revert=False) + + out = _materialize_head(repo, tmp_path, head_sha, changed_files=["python/src.py"]) + + assert out.already_present == [] + assert out.failure is not None + + +def test_materialize_falls_back_to_the_diff_url_when_the_head_diff_is_empty(tmp_path: Path, monkeypatch): + """With a diff_url the fetched diff goes to the apply, whose reverse check tells merged from reverted.""" + repo = tmp_path / "framework" + head_sha = _init_repo_with_pr_branch(repo) + _merge_pr_head(repo, revert=False) + + def fetch(_url, dest, *, timeout_sec): + dest.write_text(_VALID_PATCH, encoding="utf-8") + return True, "" + + monkeypatch.setattr("hyperloom.orchestrator.actions.executors._patch_source_pr._fetch_diff_to_path", fetch) + + out = _materialize_head( + repo, tmp_path, head_sha, changed_files=["src.py"], diff_url="https://example.invalid/pr.diff" + ) + + assert out.already_present == [] + assert out.failure is None + assert out.mode == "diff_url_fallback" + assert out.patches == [(tmp_path / "out" / "s.patch").resolve()] def test_materialize_explicit_patches_are_used_verbatim(tmp_path: Path): diff --git a/src/hyperloom/orchestrator/actions/executors/_apply_feedback.py b/src/hyperloom/orchestrator/actions/executors/_apply_feedback.py index 5d27bc08ce..85e135e177 100644 --- a/src/hyperloom/orchestrator/actions/executors/_apply_feedback.py +++ b/src/hyperloom/orchestrator/actions/executors/_apply_feedback.py @@ -1,12 +1,13 @@ # SPDX-FileCopyrightText: 2026 Advanced Micro Devices, Inc. # SPDX-License-Identifier: MIT -"""Structured apply-failure feedback for patch reauthoring.""" +"""Per-patch apply results, and the structured failure feedback used for reauthoring.""" from __future__ import annotations import logging from dataclasses import dataclass, field +from enum import Enum from pathlib import Path from typing import Any @@ -15,6 +16,17 @@ log = logging.getLogger(__name__) +class PresenceCheck(Enum): + """How a change was found already in the tree, so applying it would change nothing.""" + + #: ``git apply -R --check`` passes: every hunk's post-image is in the checkout. + GIT_REVERSE_APPLY = "git_reverse_apply_check" + #: ``patch -R --fuzz=0 --dry-run`` passes with no offset: the same proof without git. + PATCH_REVERSE_DRY_RUN = "patch_reverse_dry_run" + #: The PR head is in the live history, and the tree still has the head's copy of every file the PR changes. + PR_HEAD_FILES = "pr_head_files_in_tree" + + @dataclass class ApplyFeedback: """Structured feedback from a failed patch apply attempt.""" @@ -64,6 +76,20 @@ def format_for_mandate(self) -> str: return "\n".join(parts) +@dataclass(frozen=True) +class PatchApplyResult: + """What applying one patch did: changed the tree, found it already there, or failed.""" + + ok: bool + error: str = "" + #: Backup records the plain-``patch`` channel wrote before changing files; a git apply keeps none. + backups: list[dict[str, Any]] = field(default_factory=list) + #: Reauthoring feedback for a failure. + feedback: ApplyFeedback | None = None + #: The check that found every hunk already in the tree, when the apply changed nothing. + present_by: PresenceCheck | None = None + + def read_patch_source_context( patch_text: str, framework_root: Path, @@ -194,6 +220,8 @@ def build_apply_feedback( __all__ = [ "ApplyFeedback", + "PatchApplyResult", + "PresenceCheck", "build_apply_feedback", "read_patch_source_context", "source_context_for_file", diff --git a/src/hyperloom/orchestrator/actions/executors/_nogit_patch.py b/src/hyperloom/orchestrator/actions/executors/_nogit_patch.py index 5c88c89606..317212522f 100644 --- a/src/hyperloom/orchestrator/actions/executors/_nogit_patch.py +++ b/src/hyperloom/orchestrator/actions/executors/_nogit_patch.py @@ -17,6 +17,8 @@ from ...delivery.ledger import append_record, mark_prepared from ...delivery import file_digest as _file_digest +from ...specialists.patch_safety import path_resolves_at +from ._apply_feedback import ApplyFeedback, PatchApplyResult, PresenceCheck, read_patch_source_context log = logging.getLogger(__name__) @@ -26,6 +28,9 @@ _PATCH_DEV_NULL = "/dev/null" +# Patch and reject files are read leniently: one undecodable byte must not hide the rest. +_DECODE_ERRORS = "replace" + # Characters unsafe in filenames (replaced with ``_`` in rel_flat). _UNSAFE_NAME_RE = re.compile(r"[/\\:<>\"?*|]") @@ -84,6 +89,32 @@ def _is_git_tree(path: Path) -> bool: return False +def _reverse_probe_levels(framework_root: Path, patch_path: Path) -> tuple[int, ...]: + """The ``-p`` levels at which a clean reverse check shows ``patch_path`` is already applied. + + Undoing a file deletion creates the file, which succeeds at any level where + the stripped path happens to be absent, so a level counts only when some + file the patch names resolves at it. A deletion already carried out leaves + nothing to resolve, and then every level counts. + + Args: + framework_root: The source-tree root the patch targets. + patch_path: The unified-diff patch file to probe. + + Returns: + The levels to probe, in priority order: every level when the manifest + cannot be parsed, and none when the patch cannot be read. + """ + try: + manifest = parse_patch_manifest(patch_path.read_text(encoding="utf-8", errors=_DECODE_ERRORS)) + except OSError: + return () + except ValueError: + return _P_LEVELS + anchored = tuple(lvl for lvl in _P_LEVELS if any(path_resolves_at(framework_root, t.path, lvl) for t in manifest)) + return anchored or _P_LEVELS + + def _reverse_applies_cleanly(framework_root: Path, patch_path: Path) -> bool: """True when ``patch_path`` is already fully applied in ``framework_root``. @@ -97,7 +128,7 @@ def _reverse_applies_cleanly(framework_root: Path, patch_path: Path) -> bool: the post-image even though every hunk found a home. Strictly a probe: ``--dry-run`` is passed at every level, so the tree is - never mutated. + never mutated. Only the levels :func:`_reverse_probe_levels` names are tried. Args: framework_root: The source-tree root the patch targets. @@ -106,7 +137,7 @@ def _reverse_applies_cleanly(framework_root: Path, patch_path: Path) -> bool: Returns: ``True`` when the tree exactly holds the patch's post-state. """ - for lvl in _P_LEVELS: + for lvl in _reverse_probe_levels(framework_root, patch_path): try: cp = subprocess.run( # ``--fuzz=0``: an approximate context match would answer yes @@ -144,7 +175,7 @@ def _rejected_hunks(reject_path: Path) -> str: """The hunk bodies ``patch`` could not apply; empty when it failed before rejecting any.""" if not reject_path.exists(): return "" - return reject_path.read_text(encoding="utf-8", errors="replace").strip() + return reject_path.read_text(encoding="utf-8", errors=_DECODE_ERRORS).strip() def _apply_patch_no_git( @@ -153,19 +184,16 @@ def _apply_patch_no_git( backup_root: Path, *, seq_offset: int = 0, -) -> "tuple[bool, str, list[dict[str, Any]], Any]": +) -> PatchApplyResult: """Apply ``patch_path`` into ``framework_root`` without git, backing up targets.""" - from ._apply_feedback import ApplyFeedback, read_patch_source_context - try: - patch_text = patch_path.read_text(encoding="utf-8", errors="replace") + patch_text = patch_path.read_text(encoding="utf-8", errors=_DECODE_ERRORS) except OSError as exc: err_msg = f"cannot read patch file: {exc}" - return ( - False, - err_msg, - [], - ApplyFeedback(patch=str(patch_path), channel="nogit", tried_levels=[], stderr=err_msg), + return PatchApplyResult( + ok=False, + error=err_msg, + feedback=ApplyFeedback(patch=str(patch_path), channel="nogit", tried_levels=[], stderr=err_msg), ) # Feed the CLI a copy with contradicting index headers removed; keep the original path in feedback so advisories @@ -208,7 +236,7 @@ def _apply_patch_no_git( tried_levels=tried_levels, stderr=err_msg, ) - return False, err_msg, [], feedback + return PatchApplyResult(ok=False, error=err_msg, feedback=feedback) dry_run_stderrs.append(f"-p{lvl}: {cp.stderr.strip()}" if cp.stderr.strip() else f"-p{lvl}: (no stderr)") if cp.returncode == 0: detected_level = lvl @@ -227,23 +255,24 @@ def _apply_patch_no_git( # A clean *reverse* dry-run is the unambiguous already-applied probe: it # succeeds only when every hunk's post-state is already present in the # tree, which is exactly the state a forward apply would produce. In that - # case the apply is a satisfied no-op -- report success with no backups - # (the patch that really made those edits owns the backups needed for a - # correct revert). + # case the apply is a satisfied no-op -- report it as present, with no + # backups (the patch that really made those edits owns the backups needed + # for a correct revert). if _reverse_applies_cleanly(framework_root, patch_input): if not mark_prepared(backup_root): err_msg = "backup prepare record could not be persisted" - return ( - False, - err_msg, - [], - ApplyFeedback(patch=str(patch_path), channel="nogit", tried_levels=tried_levels, stderr=err_msg), + return PatchApplyResult( + ok=False, + error=err_msg, + feedback=ApplyFeedback( + patch=str(patch_path), channel="nogit", tried_levels=tried_levels, stderr=err_msg + ), ) log.info( "nogit patch: %s is already fully applied (clean reverse dry-run); treating as a no-op", patch_path.name, ) - return True, "", [], None + return PatchApplyResult(ok=True, present_by=PresenceCheck.PATCH_REVERSE_DRY_RUN) combined_stderr = "\n".join(dry_run_stderrs) err_msg = f"patch --dry-run failed at all strip levels for {patch_path.name}" source_ctx = read_patch_source_context(patch_text, framework_root, radius=50) @@ -254,15 +283,15 @@ def _apply_patch_no_git( stderr=combined_stderr, source_context=source_ctx, ) - return False, err_msg, [], feedback - - def _fail(err_message: str, recs: list[dict[str, Any]]) -> "tuple[bool, str, list[dict[str, Any]], Any]": - """Return the canonical 4-tuple failure result with structured feedback.""" - return ( - False, - err_message, - recs, - ApplyFeedback( + return PatchApplyResult(ok=False, error=err_msg, feedback=feedback) + + def _fail(err_message: str, recs: list[dict[str, Any]]) -> PatchApplyResult: + """Return the failure result with structured feedback.""" + return PatchApplyResult( + ok=False, + error=err_message, + backups=recs, + feedback=ApplyFeedback( patch=str(patch_path), channel="nogit", tried_levels=tried_levels, @@ -364,8 +393,8 @@ def _backup_existing(abs_path: Path, rel: Path) -> tuple[dict[str, Any] | None, rejected_hunks=rejected_hunks, source_context=source_ctx, ) - return False, apply_stderr, backups, feedback - return True, "", backups, None + return PatchApplyResult(ok=False, error=apply_stderr, backups=backups, feedback=feedback) + return PatchApplyResult(ok=True, backups=backups) __all__ = [ diff --git a/src/hyperloom/orchestrator/actions/executors/_patch_set_apply.py b/src/hyperloom/orchestrator/actions/executors/_patch_set_apply.py new file mode 100644 index 0000000000..2d06e1eef9 --- /dev/null +++ b/src/hyperloom/orchestrator/actions/executors/_patch_set_apply.py @@ -0,0 +1,220 @@ +# SPDX-FileCopyrightText: 2026 Advanced Micro Devices, Inc. +# SPDX-License-Identifier: MIT + +"""Apply an attempt's ordered patch set to its framework tree, and say what that changed. + +A git checkout takes each patch through ``git apply`` here; any other tree goes +through :mod:`._nogit_patch`. Both channels report a patch whose every hunk the +tree already holds as present rather than failed. +""" + +from __future__ import annotations + +import logging +from dataclasses import dataclass, field +from pathlib import Path +from typing import TYPE_CHECKING, Any + +from ...specialists.patch_safety import is_unified_diff, patch_escapes_tree +from ._apply_feedback import ApplyFeedback, PatchApplyResult, PresenceCheck, build_apply_feedback +from ._git import _run_git_cp +from ._nogit_patch import _P_LEVELS, _apply_patch_no_git, _reverse_probe_levels + +if TYPE_CHECKING: + from ._integrate_attempt import IntegrateAttempt + +log = logging.getLogger(__name__) + + +def _run_git_apply( + framework_root: Path, + patch_path: Path, + *, + p_level: int, + check_only: bool, +) -> tuple[bool, str]: + """Single ``git apply`` invocation at an explicit strip level. + + Args: + framework_root: The git checkout to apply into. + patch_path: The patch file to apply. + p_level: The ``-p`` strip level. + check_only: Whether to pass ``--check`` (dry run, no mutation). + + Returns: + A ``(ok, stderr)`` tuple; ``ok`` is True on a zero return code. + """ + args = ["-C", str(framework_root), "apply", f"-p{p_level}"] + if check_only: + args.append("--check") + args.append(str(patch_path)) + cp = _run_git_cp(args, timeout=120.0) + if cp is None: + return False, "git apply spawn failed" + return cp.returncode == 0, cp.stderr.strip() + + +def _git_reverse_applies_cleanly(framework_root: Path, patch_path: Path) -> bool: + """True when ``patch_path`` is already fully applied in ``framework_root``. + + The git-channel twin of + :func:`._nogit_patch._reverse_applies_cleanly`. ``git apply -R --check`` + succeeds only when every hunk's *post*-state is already present, i.e. the + tree already equals what a forward apply would produce. Read-only: + ``--check`` never mutates the tree. + + Args: + framework_root: The git checkout the patch targets. + patch_path: The patch file to probe. + + Returns: + ``True`` when some level :func:`._nogit_patch._reverse_probe_levels` + names reverse-checks cleanly. + """ + for lvl in _reverse_probe_levels(framework_root, patch_path): + cp = _run_git_cp( + ["-C", str(framework_root), "apply", "-R", f"-p{lvl}", "--check", str(patch_path)], + timeout=120.0, + ) + if cp is not None and cp.returncode == 0: + return True + return False + + +def _git_apply_failure( + framework_root: Path, patch_path: Path, tried_levels: list[int], stderr: str +) -> PatchApplyResult: + """A failed git apply, with the feedback a reauthoring round needs.""" + feedback = build_apply_feedback( + patch_path, + channel="git", + tried_levels=tried_levels, + stderr=stderr, + framework_root=framework_root, + ) + return PatchApplyResult(ok=False, error=stderr, feedback=feedback) + + +def _git_apply_collect_feedback(framework_root: Path, patch_path: Path) -> PatchApplyResult: + """Apply ``patch_path`` at the first ``-p`` level whose ``git apply --check`` passes. + + With no level passing, a patch the tree already holds in full is reported + present. A failure carries the stderr of every tried ``-p`` level and a + source-context snippet. + + Args: + framework_root: The git checkout to apply into. + patch_path: The patch file to apply. + + Returns: + The apply's result. + """ + tried_levels: list[int] = [] + level_stderrs: list[str] = [] + for lvl in _P_LEVELS: + ok_check, stderr_check = _run_git_apply(framework_root, patch_path, p_level=lvl, check_only=True) + tried_levels.append(lvl) + if stderr_check: + level_stderrs.append(f"-p{lvl}: {stderr_check}") + if ok_check: + ok_apply, stderr_apply = _run_git_apply(framework_root, patch_path, p_level=lvl, check_only=False) + if ok_apply: + return PatchApplyResult(ok=True) + return _git_apply_failure(framework_root, patch_path, tried_levels, stderr_apply) + + # A subset of a patch already applied is a no-op that ``--check`` still + # rejects; a clean reverse check proves every hunk is in the tree. + if _git_reverse_applies_cleanly(framework_root, patch_path): + log.info( + "integrate_patch: %s is already fully applied (clean git apply -R --check); treating as a no-op", + patch_path.name, + ) + return PatchApplyResult(ok=True, present_by=PresenceCheck.GIT_REVERSE_APPLY) + return _git_apply_failure(framework_root, patch_path, tried_levels, "\n".join(level_stderrs)) + + +@dataclass +class PatchSetOutcome: + """What applying an attempt's patch set did to its framework tree.""" + + #: One row per patch the tree already held in full: the patch, and the check that showed it. + already_present: list[dict[str, str]] = field(default_factory=list) + #: The patch that stopped the set, and why; empty when every patch landed. + errors: list[dict[str, str]] = field(default_factory=list) + #: Reauthoring feedback for that failure. + feedbacks: list[ApplyFeedback] = field(default_factory=list) + #: Every patch landed, and the tree already held each one, so the tree is unchanged. + changed_nothing: bool = False + + +def _unvetted_reason(patch: Path) -> str: + """Why ``patch`` may not reach an apply channel, or ``""`` when it may. + + ``vet_patches`` runs at authoring time inside the specialist runner, so a + patch from anywhere else -- ``params.patches``, and every remotely-fetched + ``upstream_pr`` diff -- arrives unvetted. + """ + try: + patch_text = patch.read_text(encoding="utf-8", errors="replace") + except OSError as exc: + return f"unreadable: {exc!r}" + if not is_unified_diff(patch_text): + return "not a unified diff" + escaping = patch_escapes_tree(patch_text) + if escaping is not None: + return f"path escapes tree: {escaping!r}" + return "" + + +def apply_patch_set( + attempt: IntegrateAttempt, + patch_paths: list[Path], + *, + git_tree: bool, + backup_root: Path, +) -> PatchSetOutcome: + """Apply ``patch_paths`` in order into ``attempt.framework_root``, stopping at the first that fails. + + A patch the tree already holds still counts as landed, so one subset patch + inside a larger set does not fail the set. Each landed patch is appended to + ``attempt.applied`` as it lands, so a stage that never returns still + reports what reached the tree. + + Args: + attempt: The attempt whose tree the set lands in. + patch_paths: The patches, in apply order. + git_tree: Whether the tree is a git checkout, which goes back through + its HEAD; any other tree goes through ``patch`` with backups. + backup_root: Where the ``patch`` channel keeps its per-file backups. + + Returns: + What the set did to the tree. + """ + framework_root = attempt.framework_root + outcome = PatchSetOutcome() + backups: list[dict[str, Any]] = [] + for patch in patch_paths: + refusal = _unvetted_reason(patch) + if refusal: + outcome.errors.append({"patch": str(patch), "stderr": refusal}) + break + if git_tree: + result = _git_apply_collect_feedback(framework_root, patch) + else: + result = _apply_patch_no_git(framework_root, patch, backup_root, seq_offset=len(backups)) + backups.extend(result.backups) + if not result.ok: + outcome.errors.append({"patch": str(patch), "stderr": result.error}) + if result.feedback is not None: + outcome.feedbacks.append(result.feedback) + break + if result.present_by is not None: + outcome.already_present.append({"patch": str(patch), "check": result.present_by.value}) + attempt.applied.append(patch) + outcome.changed_nothing = ( + bool(patch_paths) and not outcome.errors and len(outcome.already_present) == len(patch_paths) + ) + return outcome + + +__all__ = ["PatchSetOutcome", "apply_patch_set"] diff --git a/src/hyperloom/orchestrator/actions/executors/_patch_source_pr.py b/src/hyperloom/orchestrator/actions/executors/_patch_source_pr.py index 661f575403..98b0d2b573 100644 --- a/src/hyperloom/orchestrator/actions/executors/_patch_source_pr.py +++ b/src/hyperloom/orchestrator/actions/executors/_patch_source_pr.py @@ -14,6 +14,7 @@ from hyperloom.common.url_safety import require_http_url +from ._apply_feedback import PresenceCheck from ._git import _git_head_sha, _run_git log = logging.getLogger(__name__) @@ -23,6 +24,9 @@ DEFAULT_DIFF_FETCH_TIMEOUT_SEC: float = 30.0 #: Cap on a downloaded unified-diff body (bytes). Larger payloads are refused. DEFAULT_DIFF_MAX_BYTES: int = 32 * 1024 * 1024 +#: Timeout for a local git query against the live tree (merge base, tree listing, file comparison). +_GIT_QUERY_TIMEOUT_SEC: float = 60.0 +_CHECKOUT_HEAD: str = "checkout_head" def _pr_number_of(candidate: dict[str, Any]) -> int: @@ -136,7 +140,7 @@ def _materialize_pr_diff_from_head( dest: Path, *, timeout_sec: float, -) -> tuple[bool, str]: +) -> PrMaterialization: """checkout-head (diff source) mode.""" dest.parent.mkdir(parents=True, exist_ok=True) root = str(framework_root) @@ -159,7 +163,7 @@ def _materialize_pr_diff_from_head( timeout=timeout_sec, ) if not ok: - return False, f"git fetch {fetch_ref!r} failed: {err}" + return _checkout_head_failed(_CHECKOUT_HEAD, f"git fetch {fetch_ref!r} failed: {err}") if not head_sha: # FETCH_HEAD now points at the fetched head. ok2, out2, err2 = _run_git( @@ -167,46 +171,106 @@ def _materialize_pr_diff_from_head( timeout=30.0, ) if not ok2 or not out2.strip(): - return False, f"could not resolve FETCH_HEAD: {err2}" + return _checkout_head_failed(_CHECKOUT_HEAD, f"could not resolve FETCH_HEAD: {err2}") head_sha = out2.strip() if not head_sha: - return False, ("checkout-head: no head_sha / ref / pr_number on candidate; cannot resolve PR head") + return _checkout_head_failed( + _CHECKOUT_HEAD, "checkout-head: no head_sha / ref / pr_number on candidate; cannot resolve PR head" + ) # Diff against the merge-base so applying introduces only the PR's own commits, not the full divergence from the # live HEAD. live_head = _git_head_sha(framework_root) merge_base = "" if live_head: - ok_mb, mb_out, _mb_e = _run_git(["-C", root, "merge-base", live_head, head_sha], timeout=60.0) + ok_mb, mb_out, _mb_e = _run_git(["-C", root, "merge-base", live_head, head_sha], timeout=_GIT_QUERY_TIMEOUT_SEC) if ok_mb and mb_out.strip(): merge_base = mb_out.strip() diff_range = f"{merge_base}..{head_sha}" if merge_base else head_sha ok_d, diff_out, err_d = _run_git(["-C", root, "diff", "--binary", diff_range], timeout=timeout_sec) if not ok_d: - return False, f"git diff {diff_range!r} failed: {err_d}" + return _checkout_head_failed(_CHECKOUT_HEAD, f"git diff {diff_range!r} failed: {err_d}") if not diff_out.strip(): - return False, ( - f"checkout-head produced an empty diff for range {diff_range!r} (PR head already merged into live tree?)" - ) + return _empty_head_diff(framework_root, candidate, head_sha, diff_range) try: dest.write_text(diff_out, encoding="utf-8") except OSError as exc: - return False, f"could not write diff to {dest}: {exc!r}" - return True, "" + return _checkout_head_failed(_CHECKOUT_HEAD, f"could not write diff to {dest}: {exc!r}") + return PrMaterialization(patches=[dest.resolve()], mode=_CHECKOUT_HEAD) @dataclass class PrMaterialization: """What an upstream-PR candidate resolved to on disk.""" - #: The patch files to apply; empty when :attr:`failure` is set. + #: The patch files to apply; empty when :attr:`failure` or :attr:`already_present` is set. patches: list[Path] = field(default_factory=list) #: Which source produced them: ``explicit`` / ``checkout_head`` / #: ``diff_url_fallback`` / ``diff_url``. mode: str = "" - #: A terminal result dict when nothing applyable could be produced. The + #: A terminal result dict when no patch to apply could be produced. The #: caller returns it as-is rather than benching an unpatched tree. failure: dict[str, Any] | None = None + #: Evidence that the live tree already holds every change the candidate + #: makes, so there is nothing to apply. + already_present: list[dict[str, str]] = field(default_factory=list) + + +def _checkout_head_failed(mode: str, err: str) -> PrMaterialization: + """The terminal result of a checkout-head extraction that produced no diff to apply.""" + return PrMaterialization( + mode=mode, + failure={ + "status": "fetch_failed", + "error_class": "checkout_head_failed", + "error": err, + "reason": f"checkout-head diff extraction failed: {err}", + }, + ) + + +def _empty_head_diff( + framework_root: Path, candidate: dict[str, Any], head_sha: str, diff_range: str +) -> PrMaterialization: + """Map a PR head that diffs empty against the live tree's merge base. + + That shows only that the head is in the live history, which stays true + after a revert. A candidate with a ``diff_url`` therefore falls back to it, + and the apply's reverse check decides. Without one, the tree holds the PR + only when it still has the head's copy of every file the PR changes. + """ + has_diff_url = bool(str(candidate.get("diff_url") or "").strip()) + if has_diff_url or not _tree_has_head_files(framework_root, head_sha, candidate.get("changed_files")): + return _checkout_head_failed( + _CHECKOUT_HEAD, + f"checkout-head produced an empty diff for range {diff_range!r} (PR head already merged into live tree?)", + ) + log.info("upstream_pr: %s diffs empty and the tree has the head's files; the PR is already in it", diff_range) + return PrMaterialization( + mode=_CHECKOUT_HEAD, + already_present=[{"diff_range": diff_range, "check": PresenceCheck.PR_HEAD_FILES.value}], + ) + + +def _tree_has_head_files(framework_root: Path, head_sha: str, changed_files: Any) -> bool: + """True when the working tree matches ``head_sha`` on every path in ``changed_files``. + + Every path must exist at the head, so one that names nothing in either tree + cannot pass as unchanged. + """ + files = {str(f).strip() for f in changed_files if str(f).strip()} if isinstance(changed_files, list) else set() + if not files: + return False + root = str(framework_root) + ok_ls, listed, _err = _run_git( + ["-C", root, "ls-tree", "-z", "--name-only", head_sha, "--", *files], timeout=_GIT_QUERY_TIMEOUT_SEC + ) + if not ok_ls or set(filter(None, listed.split("\0"))) != files: + return False + ok_diff, _out, _err = _run_git( + ["-C", root, "diff", "--quiet", head_sha, "--", *files], timeout=_GIT_QUERY_TIMEOUT_SEC + ) + return ok_diff def materialize_candidate_patches( @@ -248,7 +312,7 @@ def materialize_candidate_patches( or str(candidate.get("ref") or "").strip() or bool(_pr_number_of(candidate)) ) - explicit_checkout = apply_mode in {"checkout_head", "checkout-head", "checkout"} or prefer_checkout + explicit_checkout = apply_mode in {_CHECKOUT_HEAD, "checkout-head", "checkout"} or prefer_checkout use_checkout_head = explicit_checkout or (not diff_url and has_checkout_ref) # Same-repo guard: checkout-head fetches from the live origin, so a cross-repo candidate would fetch the wrong # ref. @@ -269,29 +333,15 @@ def materialize_candidate_patches( dest = output_root / f"{slug}.patch" if use_checkout_head: - mode = "checkout_head" - ok, err = _materialize_pr_diff_from_head( - framework_root, - candidate, - dest, - timeout_sec=diff_fetch_timeout_sec * 4.0, - ) - if not ok and diff_url: - # A worktree/fetch hiccup must not strand an applyable candidate. - log.warning("upstream_pr: checkout-head failed (%s); falling back to diff_url", err) - mode = "diff_url_fallback" - ok, err = _fetch_diff_to_path(diff_url, dest, timeout_sec=diff_fetch_timeout_sec) + head = _materialize_pr_diff_from_head(framework_root, candidate, dest, timeout_sec=diff_fetch_timeout_sec * 4.0) + if head.failure is None or not diff_url: + return head + # A worktree/fetch hiccup must not strand a candidate that would apply. + log.warning("upstream_pr: checkout-head failed (%s); falling back to diff_url", head.failure["error"]) + ok, err = _fetch_diff_to_path(diff_url, dest, timeout_sec=diff_fetch_timeout_sec) if not ok: - return PrMaterialization( - mode=mode, - failure={ - "status": "fetch_failed", - "error_class": "checkout_head_failed", - "error": err, - "reason": f"checkout-head diff extraction failed: {err}", - }, - ) - return PrMaterialization(patches=[dest.resolve()], mode=mode) + return _checkout_head_failed("diff_url_fallback", err) + return PrMaterialization(patches=[dest.resolve()], mode="diff_url_fallback") ok, err = _fetch_diff_to_path(diff_url, dest, timeout_sec=diff_fetch_timeout_sec) if not ok: diff --git a/src/hyperloom/orchestrator/actions/executors/baseline.py b/src/hyperloom/orchestrator/actions/executors/baseline.py index c1b28c5dad..baf20a8ea2 100644 --- a/src/hyperloom/orchestrator/actions/executors/baseline.py +++ b/src/hyperloom/orchestrator/actions/executors/baseline.py @@ -1359,18 +1359,16 @@ def _apply_warm_patches( try: if use_nogit: backup_root = patch_log_dir / "patch_backups" - ok, err, backups, _feedback = _apply_patch_no_git( + nogit_apply = _apply_patch_no_git( target_path, patch_path, backup_root, seq_offset=len(nogit_backups), ) - if not ok: - raise RuntimeError(err or "nogit patch apply failed") - nogit_backups.extend(backups) - # A real apply backs up every file it writes, so an empty set is the - # applier reporting an overlay the tree already carried. - method = "applied_nogit" if backups else "already_present" + if not nogit_apply.ok: + raise RuntimeError(nogit_apply.error or "nogit patch apply failed") + nogit_backups.extend(nogit_apply.backups) + method = "already_present" if nogit_apply.present_by else "applied_nogit" else: checked = subprocess.run( ["git", "apply", "--check", str(patch_path)], diff --git a/src/hyperloom/orchestrator/actions/executors/integrate_patch.py b/src/hyperloom/orchestrator/actions/executors/integrate_patch.py index a88f9f44a8..1aed7352ec 100644 --- a/src/hyperloom/orchestrator/actions/executors/integrate_patch.py +++ b/src/hyperloom/orchestrator/actions/executors/integrate_patch.py @@ -41,8 +41,6 @@ resolve_session_framework_root, ) from ...specialists.patch_safety import ( - is_unified_diff, - patch_escapes_tree, patch_targets_missing, resolve_patch_apply_root, ) @@ -88,7 +86,6 @@ parse_eval_results, read_eval_probe, ) -from ._apply_feedback import ApplyFeedback, build_apply_feedback from ._git import _git_head_sha, _run_git_cp from ._integrate_attempt import IntegrateAttempt from ._setup_replay import replay_setup_commands @@ -100,11 +97,11 @@ ) from ._nogit_patch import ( _P_LEVELS, - _apply_patch_no_git, _is_git_tree, _is_within, ) from ...enablement.lane import bind_integrate_to_lane +from ._patch_set_apply import _run_git_apply, apply_patch_set from ...enablement.recipe.credentials import detect_credential_channels from ...enablement.recipe.projections import project_launch_evidence from ._patch_snapshot import _git_commit_kept, _patch_touched_paths @@ -646,34 +643,6 @@ def _resolve_framework_root( return tree.root if tree is not None else None -def _run_git_apply( - framework_root: Path, - patch_path: Path, - *, - p_level: int, - check_only: bool, -) -> tuple[bool, str]: - """Single ``git apply`` invocation at an explicit strip level. - - Args: - framework_root: The git checkout to apply into. - patch_path: The patch file to apply. - p_level: The ``-p`` strip level. - check_only: Whether to pass ``--check`` (dry run, no mutation). - - Returns: - A ``(ok, stderr)`` tuple; ``ok`` is True on a zero return code. - """ - args = ["-C", str(framework_root), "apply", f"-p{p_level}"] - if check_only: - args.append("--check") - args.append(str(patch_path)) - cp = _run_git_cp(args, timeout=120.0) - if cp is None: - return False, "git apply spawn failed" - return cp.returncode == 0, cp.stderr.strip() - - def _derive_lane(params: dict[str, Any]) -> str: """Derive the retry lane name from integrate_patch params. @@ -805,94 +774,6 @@ def _git_apply( return _run_git_apply(framework_root, patch_path, p_level=lvl, check_only=False) -def _git_reverse_applies_cleanly(framework_root: Path, patch_path: Path) -> bool: - """True when ``patch_path`` is already fully applied in ``framework_root``. - - The git-channel twin of - :func:`._nogit_patch._reverse_applies_cleanly`. ``git apply -R --check`` - succeeds only when every hunk's *post*-state is already present, i.e. the - tree already equals what a forward apply would produce. Read-only: - ``--check`` never mutates the tree. - - Args: - framework_root: The git checkout the patch targets. - patch_path: The patch file to probe. - - Returns: - ``True`` when some strip level reverse-checks cleanly. - """ - from ._nogit_patch import _P_LEVELS - - for lvl in _P_LEVELS: - cp = _run_git_cp( - ["-C", str(framework_root), "apply", "-R", f"-p{lvl}", "--check", str(patch_path)], - timeout=120.0, - ) - if cp is not None and cp.returncode == 0: - return True - return False - - -def _git_apply_collect_feedback( - framework_root: Path, - patch_path: Path, -) -> "tuple[bool, str, ApplyFeedback | None]": - """Like :func:`_git_apply` but also returns an :class:`ApplyFeedback` on failure. - - On success returns ``(True, "", None)``. On failure returns - ``(False, stderr, ApplyFeedback)`` where *ApplyFeedback* carries the - stderr of every tried ``-p`` level and a source-context snippet. - - Args: - framework_root: The git checkout to apply into. - patch_path: The patch file to apply. - - Returns: - ``(ok, err, feedback)`` — feedback is ``None`` on success. - """ - from ._nogit_patch import _P_LEVELS - - # Collect per-level check stderr for the feedback record. - tried_levels: list[int] = [] - level_stderrs: list[str] = [] - for lvl in _P_LEVELS: - ok_check, stderr_check = _run_git_apply(framework_root, patch_path, p_level=lvl, check_only=True) - tried_levels.append(lvl) - if stderr_check: - level_stderrs.append(f"-p{lvl}: {stderr_check}") - if ok_check: - # Level works; now apply for real. - ok_apply, stderr_apply = _run_git_apply(framework_root, patch_path, p_level=lvl, check_only=False) - if ok_apply: - return True, "", None - feedback = build_apply_feedback( - patch_path, - channel="git", - tried_levels=tried_levels, - stderr=stderr_apply, - framework_root=framework_root, - ) - return False, stderr_apply, feedback - - # A subset of a patch already applied is a no-op that ``--check`` still - # rejects; a clean reverse check proves every hunk is in the tree. - if _git_reverse_applies_cleanly(framework_root, patch_path): - log.info( - "integrate_patch: %s is already fully applied (clean git apply -R --check); treating as a no-op", - patch_path.name, - ) - return True, "", None - all_stderrs = "\n".join(level_stderrs) - feedback = build_apply_feedback( - patch_path, - channel="git", - tried_levels=tried_levels, - stderr=all_stderrs, - framework_root=framework_root, - ) - return False, all_stderrs, feedback - - def _git_apply_reverse( framework_root: Path, patch_path: Path, @@ -2022,7 +1903,7 @@ async def _stage_resolve( # Upstream-PR mode has no specialist to look up and no specialist # verdict to enforce: the Critic verdict on this proposal is the gate. if resolve_patch_source(params) == PATCH_SOURCE_UPSTREAM_PR: - return self._stage_resolve_upstream_pr(attempt, params, shared_state) + return await self._stage_resolve_upstream_pr(attempt, params, extra) specialist_task_id = str(params.get("specialist_task_id") or "").strip() if not specialist_task_id: @@ -2072,11 +1953,11 @@ async def _stage_resolve( attempt.done_payload = done_payload return None - def _stage_resolve_upstream_pr( + async def _stage_resolve_upstream_pr( self, attempt: IntegrateAttempt, params: dict[str, Any], - shared_state: Any, + extra: dict[str, Any], ) -> dict[str, Any] | None: """Resolve an upstream-PR candidate into patches on the task's own scratch. @@ -2087,12 +1968,13 @@ def _stage_resolve_upstream_pr( Args: attempt: The integration attempt receiving the resolved inputs. params: Task params, mutated with the resolved patch paths. - shared_state: SharedState, or ``None``. + extra: The runner ``extra`` mapping, carrying SharedState when there is one. Returns: A terminal result when the candidate has no permissive Critic - verdict or could not be materialised, else ``None``. + verdict, could not be materialised or is already in the tree, else ``None``. """ + shared_state = extra.get("shared_state") or extra.get("state") candidate = params.get("candidate") or {} if not isinstance(candidate, dict) or not candidate: return { @@ -2116,8 +1998,7 @@ def _stage_resolve_upstream_pr( if critic_reject is not None: return critic_reject - task_id = attempt.task_id - scratch = runs_dir(self.session_dir, "integrate_patch", task_id) + scratch = runs_dir(self.session_dir, "integrate_patch", attempt.task_id) scratch.mkdir(parents=True, exist_ok=True) framework_root = _resolve_framework_root(str(params.get("framework_source_root") or "").strip() or None) if framework_root is None: @@ -2129,6 +2010,29 @@ def _stage_resolve_upstream_pr( "patches_reverted": [], } + attempt.specialist_task_id = attempt.task_id + attempt.shared_state = shared_state + attempt.specialist_workspace = scratch + return await self._materialize_upstream_pr( + attempt, params, extra, framework_root=framework_root, scratch=scratch + ) + + async def _materialize_upstream_pr( + self, + attempt: IntegrateAttempt, + params: dict[str, Any], + extra: dict[str, Any], + *, + framework_root: Path, + scratch: Path, + ) -> dict[str, Any] | None: + """Fetch the candidate's diff into ``params['patches']``, or end the attempt when there is none to apply. + + Returns: + A terminal result when no diff could be fetched or the tree already + holds the candidate, else ``None``. + """ + candidate = params["candidate"] materialized = materialize_candidate_patches( candidate=candidate, params=params, @@ -2137,6 +2041,8 @@ def _stage_resolve_upstream_pr( slug=_candidate_slug(candidate), diff_fetch_timeout_sec=DEFAULT_DIFF_FETCH_TIMEOUT_SEC, ) + if materialized.already_present: + return await self._end_already_present(attempt, params, extra, materialized.already_present) if materialized.failure is not None: return { **materialized.failure, @@ -2148,10 +2054,6 @@ def _stage_resolve_upstream_pr( } params["patches"] = [str(p) for p in materialized.patches] params["patch_source_mode"] = materialized.mode - - attempt.specialist_task_id = task_id - attempt.shared_state = shared_state - attempt.specialist_workspace = scratch return None async def _stage_provision_attempt_runtime( @@ -2514,14 +2416,10 @@ async def _stage_apply( # A runtime this round acquired changes what the next boot executes; a # re-provisioned kept one is the stack the last KEEP was already graded on. acquired_runtime = bool(attempt.attempt_venv_root) and not attempt.shared_state.enablement.kept_stack_action - if ( - not patch_paths - and not proposal_extra_args - and not proposal_extra_envs - and not artifact_specs - and not _setup_ran - and not acquired_runtime - ): + changes_beyond_patches = bool( + proposal_extra_args or proposal_extra_envs or artifact_specs or _setup_ran or acquired_runtime + ) + if not patch_paths and not changes_beyond_patches: # Launch-only mode: skip the no-patches early-return and fall through to bench. if params.get("enablement_launch_only"): output_root = runs_dir(self.session_dir, "integrate_patch", specialist_task_id) @@ -2736,7 +2634,6 @@ async def _stage_apply( recovery["phase"] = "ready" if shared_state is not None: shared_state.save(self.session_dir) - nogit_patch_backups: list[dict[str, Any]] = [] # An artifact preimage has to describe the tree as this attempt found # it, so it is taken before the patches run: a patch that creates the @@ -2771,52 +2668,12 @@ async def _stage_apply( if shared_state is not None: shared_state.save(self.session_dir) - applied: list[Path] = [] applied_artifacts: list[dict[str, Any]] = [] - apply_errors: list[dict[str, str]] = [] - apply_feedbacks: list[ApplyFeedback] = [] - # ``applied`` is published by identity and appended to in place. - attempt.applied = applied attempt.applied_artifacts = applied_artifacts - for patch in patch_paths: - # ``vet_patches`` runs at authoring time inside the specialist - # runner, so a patch from anywhere else -- ``params.patches``, and - # every remotely-fetched ``upstream_pr`` diff -- arrives unvetted. - try: - patch_text = patch.read_text(encoding="utf-8", errors="replace") - except OSError as exc: - apply_errors.append({"patch": str(patch), "stderr": f"unreadable: {exc!r}"}) - break - if not is_unified_diff(patch_text): - apply_errors.append({"patch": str(patch), "stderr": "not a unified diff"}) - break - escaping = patch_escapes_tree(patch_text) - if escaping is not None: - apply_errors.append({"patch": str(patch), "stderr": f"path escapes tree: {escaping!r}"}) - break - if git_head: - ok, err, fb = _git_apply_collect_feedback(framework_root, patch) - if not ok: - apply_errors.append({"patch": str(patch), "stderr": err}) - if fb is not None: - apply_feedbacks.append(fb) - break - else: - ok, err, backups, fb = _apply_patch_no_git( - framework_root, - patch, - recovery_root / "patch_backups", - seq_offset=len(nogit_patch_backups), - ) - nogit_patch_backups.extend(backups) - if not ok: - apply_errors.append({"patch": str(patch), "stderr": err}) - if fb is not None: - apply_feedbacks.append(fb) - break - applied.append(patch) - - if apply_errors: + patch_set = apply_patch_set( + attempt, patch_paths, git_tree=bool(git_head), backup_root=recovery_root / "patch_backups" + ) + if patch_set.errors: await self._maybe_write_framework_kb_record( params=params, done_payload=done_payload, @@ -2828,18 +2685,20 @@ async def _stage_apply( base_result: dict[str, Any] = { "status": "apply_failed", "error_class": "git_apply_failed", - "error": apply_errors, + "error": patch_set.errors, "specialist_task_id": specialist_task_id, "patches_applied": [], "patches_reverted": [], "workspace": str(output_root), "lane": lane, - "retry_feedback": [fb.to_dict() for fb in apply_feedbacks], + "retry_feedback": [fb.to_dict() for fb in patch_set.feedbacks], "prior_patches": [str(p) for p in patch_paths], } if bool(params.get("enablement")): base_result["enablement"] = True return base_result + if patch_set.changed_nothing and not changes_beyond_patches: + return await self._end_already_present(attempt, params, extra, patch_set.already_present) if artifact_specs: applied_artifacts, artifact_apply_errors = self._apply_artifacts( @@ -2874,7 +2733,7 @@ async def _stage_apply( return { "status": "applied_no_bench", "specialist_task_id": specialist_task_id, - "patches_applied": [str(p) for p in applied], + "patches_applied": [str(p) for p in attempt.applied], "patches_reverted": [], "artifacts_applied": applied_artifacts, "extra_server_args_applied": extra_server_args_applied, @@ -2890,6 +2749,47 @@ async def _stage_apply( attempt.dropped_env_overrides = dropped_env_overrides return None + async def _end_already_present( + self, + attempt: IntegrateAttempt, + params: dict[str, Any], + extra: dict[str, Any], + evidence: list[dict[str, str]], + ) -> dict[str, Any]: + """End an attempt whose every change the tree already holds: there is nothing to benchmark. + + Args: + attempt: The attempt; it applied nothing, so it reports nothing reverted. + params: Task params, read for the KB record and the enablement lane. + extra: The runner ``extra`` mapping, read for the KB record. + evidence: What showed each change to be present, and how. + + Returns: + The ``already_present`` result. + """ + from ...knowledge import kb_writeback as _kb + + log.info("integrate_patch: the framework tree already holds this candidate; not benchmarking: %s", evidence) + attempt.applied.clear() + await self._maybe_write_framework_kb_record( + params=params, + done_payload=attempt.done_payload, + outcome=_kb.OUTCOME_ALREADY_PRESENT, + tps_delta_pct=0.0, + extra=extra, + ) + result: dict[str, Any] = { + "status": "already_present", + "specialist_task_id": attempt.specialist_task_id, + "already_present": evidence, + "patches_applied": [], + "patches_reverted": [], + "reason": "the framework tree already holds every change in this candidate, so there is nothing to measure", + } + if params.get("enablement"): + result["enablement"] = True + return result + async def _stage_gate( self, attempt: IntegrateAttempt, @@ -4319,8 +4219,7 @@ async def _maybe_write_framework_kb_record( proposal = self._find_framework_proposal(done_payload) if proposal is None: # The upstream-PR lane carries the PR identity on the candidate - # rather than in a specialist's ``fa_*`` markers. Discovery dedups - # on this ledger, so a lane that writes none is re-benched forever. + # rather than in a specialist's ``fa_*`` markers. proposal = self._upstream_pr_kb_proposal(params or {}) if proposal is None: return diff --git a/src/hyperloom/orchestrator/actions/executors/tests/test_integrate_patch_executor.py b/src/hyperloom/orchestrator/actions/executors/tests/test_integrate_patch_executor.py index d49d97f141..3938126371 100644 --- a/src/hyperloom/orchestrator/actions/executors/tests/test_integrate_patch_executor.py +++ b/src/hyperloom/orchestrator/actions/executors/tests/test_integrate_patch_executor.py @@ -22,9 +22,9 @@ from hyperloom.orchestrator.actions.executors import _setup_replay as sr_mod from hyperloom.orchestrator.actions.executors._accuracy_gate import framework_run_eval_envs +from hyperloom.orchestrator.actions.executors._nogit_patch import _apply_patch_no_git from hyperloom.orchestrator.actions.executors.integrate_patch import ( IntegratePatchExecutor, - _apply_patch_no_git, _git_apply, _git_apply_reverse, _is_git_tree, @@ -2061,11 +2061,11 @@ def fake_run(cmd, *args, **kwargs): monkeypatch.setattr(subprocess, "run", fake_run) - ok, err, backups, *_ = _apply_patch_no_git(framework_root, patch_file, tmp_path / "backups") + result = _apply_patch_no_git(framework_root, patch_file, tmp_path / "backups") - assert ok is False - assert "escapes framework root" in err - assert backups == [] + assert result.ok is False + assert "escapes framework root" in result.error + assert result.backups == [] assert outside.read_text(encoding="utf-8") == "SAFE\n" assert len(calls) == 1 @@ -2653,7 +2653,7 @@ def _explode(*_a, **_k): raise AssertionError("git apply was reached with an unvetted patch") monkeypatch.setattr( - "hyperloom.orchestrator.actions.executors.integrate_patch._git_apply_collect_feedback", + "hyperloom.orchestrator.actions.executors._patch_set_apply._git_apply_collect_feedback", _explode, ) diff --git a/src/hyperloom/orchestrator/actions/executors/tests/test_integrate_patch_helpers_unit.py b/src/hyperloom/orchestrator/actions/executors/tests/test_integrate_patch_helpers_unit.py index c50a4064b5..cde3d290c0 100644 --- a/src/hyperloom/orchestrator/actions/executors/tests/test_integrate_patch_helpers_unit.py +++ b/src/hyperloom/orchestrator/actions/executors/tests/test_integrate_patch_helpers_unit.py @@ -11,6 +11,7 @@ import pytest from hyperloom.orchestrator.actions.executors import _git as gitmod +from hyperloom.orchestrator.actions.executors import _patch_set_apply as psa from hyperloom.orchestrator.actions.executors import integrate_patch as ip @@ -598,9 +599,10 @@ def test_a_patch_that_does_not_apply_leaves_the_tree_untouched(tmp_path): "diff --git a/g.py b/g.py\nnew file mode 100644\n--- /dev/null\n+++ b/g.py\n@@ -0,0 +1 @@\n+created\n" ) - ok, _, feedback = ip._git_apply_collect_feedback(repo, patch) + result = psa._git_apply_collect_feedback(repo, patch) - assert not ok and feedback is not None + assert not result.ok and result.feedback is not None + assert result.present_by is None assert _git_out(repo, "status", "--porcelain") == "" @@ -794,7 +796,8 @@ class _NoMethod: assert out["status"] == "rejected_by_critic" -def test_upstream_pr_lane_refuses_an_unreviewed_candidate(tmp_path: Path) -> None: +@pytest.mark.asyncio +async def test_upstream_pr_lane_refuses_an_unreviewed_candidate(tmp_path: Path) -> None: """The lane fetches a diff from a remote and applies it to the live tree.""" ex = ip.IntegratePatchExecutor(session_dir=tmp_path) attempt = ip.IntegrateAttempt(task_id="t-cand") @@ -803,7 +806,7 @@ def test_upstream_pr_lane_refuses_an_unreviewed_candidate(tmp_path: Path) -> Non "framework_agent_candidate_id": "vllm-project/vllm#1015", } - out = ex._stage_resolve_upstream_pr(attempt, params, _Verdict("reject")) + out = await ex._stage_resolve_upstream_pr(attempt, params, {"shared_state": _Verdict("reject")}) assert out is not None assert out["status"] == "rejected_by_critic" diff --git a/src/hyperloom/orchestrator/actions/executors/tests/test_nogit_patch_unit.py b/src/hyperloom/orchestrator/actions/executors/tests/test_nogit_patch_unit.py index 61ca809cf7..439cda8a39 100644 --- a/src/hyperloom/orchestrator/actions/executors/tests/test_nogit_patch_unit.py +++ b/src/hyperloom/orchestrator/actions/executors/tests/test_nogit_patch_unit.py @@ -28,7 +28,6 @@ def test_reexport_from_integrate_patch(): assert ip._P_LEVELS is ng._P_LEVELS assert ip._is_within is ng._is_within assert ip._is_git_tree is ng._is_git_tree - assert ip._apply_patch_no_git is ng._apply_patch_no_git def test_reexport_from_patch_snapshot(): @@ -211,9 +210,9 @@ def run(cmd, **kwargs): monkeypatch.setattr(ng.subprocess, "run", run) monkeypatch.setattr(ng, "mark_prepared", lambda *_args, **_kwargs: False) - ok, error, _records, _feedback = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "backups") - assert not ok - assert "prepare" in error + result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "backups") + assert not result.ok + assert "prepare" in result.error assert len(calls) == 1 assert target.read_text() == "original\n" @@ -255,27 +254,29 @@ def dry_run(cmd, **kwargs): return _CP(1, "", "previously applied patch") monkeypatch.setattr(ng.subprocess, "run", dry_run) - ok, error, backups, feedback = ng._apply_patch_no_git(tmp_path, patch_file, backup_root) + result = ng._apply_patch_no_git(tmp_path, patch_file, backup_root) - assert ok is prepare_succeeds + assert result.ok is prepare_succeeds assert target.read_text(encoding="utf-8") == "patched\n" - assert backups == [] + assert result.backups == [] assert len(calls) == len(ng._P_LEVELS) + 1 if prepare_succeeds: - assert error == "" - assert feedback is None + assert result.error == "" + assert result.feedback is None + assert result.present_by is af.PresenceCheck.PATCH_REVERSE_DRY_RUN assert ledger.load_prepared_records(backup_root) == prior_records else: - assert "prepare" in error - assert isinstance(feedback, af.ApplyFeedback) - assert feedback.stderr == error + assert "prepare" in result.error + assert isinstance(result.feedback, af.ApplyFeedback) + assert result.feedback.stderr == result.error + assert result.present_by is None if prior_prepared: assert ledger.load_prepared_records(backup_root) == prior_records assert backup.read_text(encoding="utf-8") == "original\n" def test_apply_patch_no_git_dry_run_failure(tmp_path, monkeypatch): - """All strip levels fail dry-run → returns (False, …, []).""" + """All strip levels fail dry-run: a failure with no backups.""" patch_file = tmp_path / "bad.patch" patch_file.write_text("not a patch\n", encoding="utf-8") @@ -289,9 +290,9 @@ class _FailCP: monkeypatch.setattr(_sp, "run", lambda *a, **k: _FailCP()) monkeypatch.setattr(ng.subprocess, "run", lambda *a, **k: _FailCP()) - ok, err, backups, *_ = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") - assert ok is False - assert backups == [] + result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") + assert result.ok is False + assert result.backups == [] # Backup name uniqueness across multiple patches sharing a backup_root @@ -326,14 +327,16 @@ def test_backup_names_unique_across_patches_same_basename(tmp_path): backup_root = tmp_path / "shared_backups" accumulated: list = [] - ok, err, backups_a, *_ = ng._apply_patch_no_git(tmp_path, patch_a, backup_root, seq_offset=len(accumulated)) - if not ok: - pytest.skip(f"patch CLI unavailable: {err}") + first = ng._apply_patch_no_git(tmp_path, patch_a, backup_root, seq_offset=len(accumulated)) + if not first.ok: + pytest.skip(f"patch CLI unavailable: {first.error}") + backups_a = first.backups accumulated.extend(backups_a) - ok, err, backups_b, *_ = ng._apply_patch_no_git(tmp_path, patch_b, backup_root, seq_offset=len(accumulated)) - if not ok: - pytest.skip(f"patch CLI unavailable: {err}") + second = ng._apply_patch_no_git(tmp_path, patch_b, backup_root, seq_offset=len(accumulated)) + if not second.ok: + pytest.skip(f"patch CLI unavailable: {second.error}") + backups_b = second.backups accumulated.extend(backups_b) assert len(backups_a) >= 1 @@ -358,11 +361,11 @@ def test_seq_offset_zero_gives_deterministic_names(tmp_path): patch_file = tmp_path / "mypatch.patch" patch_file.write_text(SIMPLE_DIFF, encoding="utf-8") - ok, err, backups, *_ = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bk") - if not ok: - pytest.skip(f"patch CLI unavailable: {err}") + result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bk") + if not result.ok: + pytest.skip(f"patch CLI unavailable: {result.error}") - for r in backups: + for r in result.backups: if r.get("backup_path"): name = Path(r["backup_path"]).name assert "mypatch" in name, f"patch stem missing from backup name: {name!r}" @@ -409,9 +412,9 @@ def test_rename_is_recorded_even_though_it_has_no_hunks(tmp_path): patch_file = tmp_path / "pure_rename.patch" patch_file.write_text(PURE_RENAME_DIFF, encoding="utf-8") - _ok, _err, backups, *_ = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bk") + result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bk") - records = {Path(r["target"]).name: r for r in backups} + records = {Path(r["target"]).name: r for r in result.backups} assert {name: r["revert_action"] for name, r in records.items()} == { "old_name.py": "restore", "new_name.py": "delete", @@ -422,8 +425,8 @@ def test_rename_is_recorded_even_though_it_has_no_hunks(tmp_path): # ApplyFeedback structure tests -def test_apply_feedback_dry_run_failure_returns_fourth_item(tmp_path, monkeypatch): - """When all dry-run levels fail, _apply_patch_no_git returns a 4-tuple with an ApplyFeedback carrying the accumulated per-level stderr.""" +def test_apply_feedback_dry_run_failure_carries_feedback(tmp_path, monkeypatch): + """When all dry-run levels fail, the failure carries an ApplyFeedback with the accumulated per-level stderr.""" import subprocess as _sp patch_file = tmp_path / "bad.patch" @@ -438,19 +441,17 @@ class _FailCP: monkeypatch.setattr(ng.subprocess, "run", lambda *a, **k: _FailCP()) result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") - assert len(result) == 4, "must return a 4-tuple" - ok, err, backups, feedback = result - assert ok is False - assert backups == [] - assert isinstance(feedback, af.ApplyFeedback) - assert feedback.channel == "nogit" - assert len(feedback.tried_levels) > 0 + assert result.ok is False + assert result.backups == [] + assert isinstance(result.feedback, af.ApplyFeedback) + assert result.feedback.channel == "nogit" + assert len(result.feedback.tried_levels) > 0 # Per-level stderr should be present. - assert "hunk FAILED" in feedback.stderr + assert "hunk FAILED" in result.feedback.stderr def test_apply_feedback_success_returns_none_feedback(tmp_path): - """A successful apply returns (True, '', backups, None).""" + """A successful apply carries no feedback.""" target = tmp_path / "target.py" target.write_text("original\n", encoding="utf-8") patch_file = tmp_path / "fix.patch" @@ -458,12 +459,9 @@ def test_apply_feedback_success_returns_none_feedback(tmp_path): backup_root = tmp_path / "bak" result = ng._apply_patch_no_git(tmp_path, patch_file, backup_root) - if len(result) != 4: - pytest.skip("unexpected return length") - ok, err, backups, feedback = result - if not ok: - pytest.skip(f"patch CLI unavailable: {err}") - assert feedback is None, "successful apply must return None feedback" + if not result.ok: + pytest.skip(f"patch CLI unavailable: {result.error}") + assert result.feedback is None, "successful apply must return None feedback" def test_apply_feedback_roundtrip_serialization(): @@ -548,11 +546,11 @@ def _raise(*a, **k): raise FileNotFoundError("patch") monkeypatch.setattr(ng.subprocess, "run", _raise) - ok, err, backups, feedback = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") - assert ok is False - assert backups == [] - assert isinstance(feedback, af.ApplyFeedback) - assert "unavailable" in err + result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") + assert result.ok is False + assert result.backups == [] + assert isinstance(result.feedback, af.ApplyFeedback) + assert "unavailable" in result.error # _apply_patch_no_git — create-file hunk (old = /dev/null) @@ -570,16 +568,16 @@ def test_apply_create_file_then_revert_deletes(tmp_path): patch_file = tmp_path / "create.patch" patch_file.write_text(CREATE_DIFF, encoding="utf-8") - ok, err, backups, *_ = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") - if not ok: - pytest.skip(f"patch CLI unavailable: {err}") + result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") + if not result.ok: + pytest.skip(f"patch CLI unavailable: {result.error}") created = tmp_path / "created.py" assert created.exists() - delete_recs = [r for r in backups if r.get("revert_action") == "delete"] + delete_recs = [r for r in result.backups if r.get("revert_action") == "delete"] assert delete_recs, "create hunk must record a delete revert action" - ledger.restore_records(backups) + ledger.restore_records(result.backups) assert not created.exists(), "revert of a create hunk must delete the new file" @@ -601,15 +599,15 @@ def test_apply_delete_file_backs_up_and_reverts(tmp_path): patch_file = tmp_path / "delete.patch" patch_file.write_text(DELETE_DIFF, encoding="utf-8") - ok, err, backups, *_ = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") - if not ok: - pytest.skip(f"patch CLI unavailable: {err}") + result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") + if not result.ok: + pytest.skip(f"patch CLI unavailable: {result.error}") assert not doomed.exists(), "delete hunk must remove the file" - restore_recs = [r for r in backups if r.get("revert_action") == "restore"] + restore_recs = [r for r in result.backups if r.get("revert_action") == "restore"] assert restore_recs, "delete hunk must back up the existing file" - ledger.restore_records(backups) + ledger.restore_records(result.backups) assert doomed.exists(), "revert must restore the deleted file" assert doomed.read_text() == "goodbye\n" @@ -627,9 +625,9 @@ def _fake_run(cmd, *a, **k): patch_file = tmp_path / "delete.patch" patch_file.write_text(DELETE_DIFF, encoding="utf-8") - ok, err, backups, *_ = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") - assert ok is True - delete_recs = [r for r in backups if r.get("revert_action") == "delete"] + result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") + assert result.ok is True + delete_recs = [r for r in result.backups if r.get("revert_action") == "delete"] assert delete_recs, "absent delete target must still record a delete action" assert delete_recs[0]["backup_path"] is None @@ -655,9 +653,9 @@ def _fake_run(cmd, *a, **k): patch_file = tmp_path / "escape.patch" patch_file.write_text(ESCAPE_DIFF, encoding="utf-8") - ok, err, backups, *_ = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") - assert ok is False - assert "escapes framework root" in err + result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") + assert result.ok is False + assert "escapes framework root" in result.error # _apply_patch_no_git — backup copy failure @@ -679,9 +677,9 @@ def _boom(*a, **k): raise OSError("disk full") monkeypatch.setattr(ng.shutil, "copy2", _boom) - ok, err, backups, *_ = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") - assert ok is False - assert "backup of" in err + result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") + assert result.ok is False + assert "backup of" in result.error # _apply_patch_no_git — real apply failure reports the rejected hunks @@ -718,12 +716,12 @@ def _run(cmd, *a, **k): def test_apply_real_failure_reports_rejected_hunks(tmp_path): """A failed real apply carries the rejected hunk body, not just the CLI's one-line summary.""" - ok, _err, _backups, feedback = _apply_failing_after_dry_run(tmp_path, tmp_path / "bak") + result = _apply_failing_after_dry_run(tmp_path, tmp_path / "bak") - assert ok is False - assert isinstance(feedback, af.ApplyFeedback) - assert feedback.channel == "nogit" - assert "patched" in feedback.rejected_hunks, "the rejected hunk body must reach the patch author" + assert result.ok is False + assert isinstance(result.feedback, af.ApplyFeedback) + assert result.feedback.channel == "nogit" + assert "patched" in result.feedback.rejected_hunks, "the rejected hunk body must reach the patch author" def test_apply_real_failure_leaves_the_framework_tree_alone(tmp_path): @@ -731,10 +729,10 @@ def test_apply_real_failure_leaves_the_framework_tree_alone(tmp_path): foreign = tmp_path / "other.py.rej" foreign.write_text("@@ -1 +1 @@\n-foreign\n", encoding="utf-8") - _ok, _err, _backups, feedback = _apply_failing_after_dry_run(tmp_path, tmp_path / "bak") + result = _apply_failing_after_dry_run(tmp_path, tmp_path / "bak") assert foreign.exists() - assert "foreign" not in feedback.rejected_hunks + assert "foreign" not in result.feedback.rejected_hunks assert list(tmp_path.glob("*.orig")) == [] @@ -759,11 +757,10 @@ def _run(cmd, *a, **k): with um.patch.object(ng.subprocess, "run", _run): result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") - ok, err, backups, feedback = result - if ok: + if result.ok: pytest.skip("dry-run failed; cannot reach real-apply branch") - assert isinstance(feedback, af.ApplyFeedback) - assert "patch apply failed" in err + assert isinstance(result.feedback, af.ApplyFeedback) + assert "patch apply failed" in result.error # ledger.restore_records — a failed restore is reported, never raised @@ -829,10 +826,10 @@ def test_modification_existing_file_backed_up(tmp_path, monkeypatch): patch_file = tmp_path / "fix.patch" patch_file.write_text(MODIFY_SAME_PATH_DIFF, encoding="utf-8") - ok, err, backups, feedback = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") - assert ok is True - assert feedback is None - restore = [r for r in backups if r.get("revert_action") == "restore"] + result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") + assert result.ok is True + assert result.feedback is None + restore = [r for r in result.backups if r.get("revert_action") == "restore"] assert restore and restore[0]["backup_path"] is not None @@ -844,9 +841,9 @@ def test_unprefixed_modification_at_p0_records_one_restore(tmp_path, monkeypatch patch_file = tmp_path / "fix.patch" patch_file.write_text(MODIFY_SAME_PATH_DIFF, encoding="utf-8") - ok, err, backups, *_ = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") - assert ok, err - assert [(r["target"], r["revert_action"]) for r in backups] == [(str(target.resolve()), "restore")] + result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") + assert result.ok, result.error + assert [(r["target"], r["revert_action"]) for r in result.backups] == [(str(target.resolve()), "restore")] def test_modification_missing_file_records_delete(tmp_path, monkeypatch): @@ -854,9 +851,9 @@ def test_modification_missing_file_records_delete(tmp_path, monkeypatch): monkeypatch.setattr(ng.subprocess, "run", _fake_ok_run) patch_file = tmp_path / "fix.patch" patch_file.write_text(MODIFY_SAME_PATH_DIFF, encoding="utf-8") - ok, err, backups, *_ = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") - assert ok is True - delete = [r for r in backups if r.get("revert_action") == "delete"] + result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") + assert result.ok is True + delete = [r for r in result.backups if r.get("revert_action") == "delete"] assert delete and delete[0]["backup_path"] is None @@ -868,9 +865,9 @@ def test_modification_target_escape_rejected(tmp_path, monkeypatch): "--- ../../../etc/evil.py\n+++ ../../../etc/evil.py\n@@ -1 +1 @@\n-x\n+y\n", encoding="utf-8", ) - ok, err, backups, *_ = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") - assert ok is False - assert "escapes framework root" in err + result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") + assert result.ok is False + assert "escapes framework root" in result.error def test_delete_existing_file_backed_up(tmp_path, monkeypatch): @@ -881,9 +878,9 @@ def test_delete_existing_file_backed_up(tmp_path, monkeypatch): patch_file = tmp_path / "del.patch" patch_file.write_text(DELETE_DIFF, encoding="utf-8") - ok, err, backups, *_ = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") - assert ok is True - restore = [r for r in backups if r.get("revert_action") == "restore"] + result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") + assert result.ok is True + restore = [r for r in result.backups if r.get("revert_action") == "restore"] assert restore and restore[0]["backup_path"] is not None @@ -895,13 +892,13 @@ def test_create_target_escape_rejected(tmp_path, monkeypatch): "--- /dev/null\n+++ b/../../../etc/evil.py\n@@ -0,0 +1 @@\n+pwn\n", encoding="utf-8", ) - ok, err, backups, *_ = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") - assert ok is False - assert "escapes framework root" in err + result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") + assert result.ok is False + assert "escapes framework root" in result.error def test_patch_file_unreadable_after_dry_run(tmp_path, monkeypatch): - """An OSError reading the patch text after dry-run returns a 3-tuple error.""" + """An OSError reading the patch text after dry-run is a failure.""" monkeypatch.setattr(ng.subprocess, "run", _fake_ok_run) patch_file = tmp_path / "fix.patch" patch_file.write_text(SIMPLE_DIFF, encoding="utf-8") @@ -919,10 +916,8 @@ def _read_text(self, *a, **k): monkeypatch.setattr(Path, "read_text", _read_text) result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") - ok = result[0] - err = result[1] - assert ok is False - assert "cannot read patch file" in err + assert result.ok is False + assert "cannot read patch file" in result.error # Escape / continue edge branches in the create / delete / rename arms @@ -934,11 +929,11 @@ def test_devnull_both_sides_is_rejected(tmp_path, monkeypatch): patch_file = tmp_path / "noop.patch" patch_file.write_text("--- /dev/null\n+++ /dev/null\n@@ -0,0 +0,0 @@\n", encoding="utf-8") - ok, err, backups, *_ = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") + result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") - assert ok is False - assert "unparseable patch" in err - assert backups == [] + assert result.ok is False + assert "unparseable patch" in result.error + assert result.backups == [] def test_delete_target_escape_rejected(tmp_path, monkeypatch): @@ -949,9 +944,9 @@ def test_delete_target_escape_rejected(tmp_path, monkeypatch): "--- ../../../etc/evil.py\n+++ /dev/null\n@@ -1 +0,0 @@\n-x\n", encoding="utf-8", ) - ok, err, backups, *_ = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") - assert ok is False - assert "escapes framework root" in err + result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") + assert result.ok is False + assert "escapes framework root" in result.error def test_delete_backup_failure_returns_error(tmp_path, monkeypatch): @@ -966,9 +961,9 @@ def _boom(*a, **k): raise OSError("disk full") monkeypatch.setattr(ng.shutil, "copy2", _boom) - ok, err, backups, *_ = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") - assert ok is False - assert "backup of" in err + result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") + assert result.ok is False + assert "backup of" in result.error def test_rename_new_target_escape_rejected(tmp_path, monkeypatch): @@ -981,9 +976,9 @@ def test_rename_new_target_escape_rejected(tmp_path, monkeypatch): "--- a/old_name.py\n+++ b/../../../etc/evil.py\n@@ -1 +1 @@\n-moved\n+moved\n", encoding="utf-8", ) - ok, err, backups, *_ = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") - assert ok is False - assert "escapes framework root" in err + result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") + assert result.ok is False + assert "escapes framework root" in result.error def test_modification_backup_failure_returns_error(tmp_path, monkeypatch): @@ -998,9 +993,9 @@ def _boom(*a, **k): raise OSError("disk full") monkeypatch.setattr(ng.shutil, "copy2", _boom) - ok, err, backups, *_ = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") - assert ok is False - assert "backup of" in err + result = ng._apply_patch_no_git(tmp_path, patch_file, tmp_path / "bak") + assert result.ok is False + assert "backup of" in result.error # _sanitize_git_index_lines — placeholder git index headers @@ -1076,10 +1071,10 @@ def test_apply_no_git_tolerates_placeholder_index_header(tmp_path): patch_file.parent.mkdir(parents=True) patch_file.write_text(ZERO_INDEX_MODIFY_DIFF, encoding="utf-8") - ok, err, backups, _feedback = ng._apply_patch_no_git(root, patch_file, tmp_path / "bak") + result = ng._apply_patch_no_git(root, patch_file, tmp_path / "bak") - assert ok is True, err + assert result.ok is True, result.error assert "patched = True" in target.read_text() - ledger.restore_records(backups) + ledger.restore_records(result.backups) assert target.read_text() == original diff --git a/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py b/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py index 5dfa50c220..a2e058b1be 100644 --- a/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py +++ b/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py @@ -1,20 +1,30 @@ # SPDX-FileCopyrightText: 2026 Advanced Micro Devices, Inc. # SPDX-License-Identifier: MIT -"""Already-applied patches must be a satisfied no-op, not an apply failure.""" +"""Already-applied patches are a satisfied no-op, and a set of nothing else is ``already_present``, not benched.""" from __future__ import annotations +import json import subprocess from pathlib import Path import pytest +from hyperloom.agents.framework.kb import read_pr_ledger +from hyperloom.inference_optimizer.session.session_paths import runs_dir +from hyperloom.orchestrator.actions.executors import _patch_set_apply as psa from hyperloom.orchestrator.actions.executors import integrate_patch as ip +from hyperloom.orchestrator.actions.executors._apply_feedback import PatchApplyResult, PresenceCheck +from hyperloom.orchestrator.actions.executors._integrate_attempt import IntegrateAttempt from hyperloom.orchestrator.actions.executors._nogit_patch import ( _apply_patch_no_git, _reverse_applies_cleanly, ) +from hyperloom.orchestrator.loop.sub_agent_runner import RunnerContext +from hyperloom.orchestrator.state.shared_state import SharedState +from hyperloom.orchestrator.state.task_registry import Task +from hyperloom.orchestrator.tests._helpers import git_commit_all _SUBSET = """\ @@ -51,6 +61,20 @@ footer """ +# Deletes alpha.py; undoing it re-creates the file, which needs no matching content in the tree. +_DELETE_ALPHA = """\ +diff --git a/mod/alpha.py b/mod/alpha.py +deleted file mode 100644 +--- a/mod/alpha.py ++++ /dev/null +@@ -1,3 +0,0 @@ +-header +-old_alpha +-footer +""" + +_PR_URL = "https://github.com/example-org/example-framework/pull/1" + def _tree(root: Path) -> None: """Materialize the two-file source tree both patches target.""" @@ -82,15 +106,17 @@ def test_nogit_superset_then_subset_is_a_noop(workspace): sup = _patch(patches, "superset.patch", _SUPERSET) sub = _patch(patches, "subset.patch", _SUBSET) - ok, err, recs, fb = _apply_patch_no_git(src, sup, backups, seq_offset=0) - assert ok, err - assert recs, "superset should have backed up the files it changed" + first = _apply_patch_no_git(src, sup, backups, seq_offset=0) + assert first.ok, first.error + assert first.backups, "superset should have backed up the files it changed" + assert first.present_by is None - ok2, err2, recs2, fb2 = _apply_patch_no_git(src, sub, backups, seq_offset=len(recs)) - assert ok2, err2 - assert fb2 is None + second = _apply_patch_no_git(src, sub, backups, seq_offset=len(first.backups)) + assert second.ok, second.error + assert second.feedback is None + assert second.present_by is PresenceCheck.PATCH_REVERSE_DRY_RUN # The no-op owns no backups — the patch that made the edits owns them, so a revert restores the tree exactly once. - assert recs2 == [] + assert second.backups == [] assert "new_alpha" in (src / "mod" / "alpha.py").read_text(encoding="utf-8") assert "new_beta" in (src / "mod" / "beta.py").read_text(encoding="utf-8") @@ -102,12 +128,13 @@ def test_nogit_subset_then_superset_fails_closed(workspace): sup = _patch(patches, "superset.patch", _SUPERSET) sub = _patch(patches, "subset.patch", _SUBSET) - ok, _, recs, _ = _apply_patch_no_git(src, sub, backups, seq_offset=0) - assert ok - ok2, err2, recs2, fb2 = _apply_patch_no_git(src, sup, backups, seq_offset=len(recs)) - assert not ok2 - assert "failed at all strip levels" in err2 - assert fb2 is not None, "a real failure must carry reauthor feedback" + first = _apply_patch_no_git(src, sub, backups, seq_offset=0) + assert first.ok + second = _apply_patch_no_git(src, sup, backups, seq_offset=len(first.backups)) + assert not second.ok + assert "failed at all strip levels" in second.error + assert second.feedback is not None, "a real failure must carry reauthor feedback" + assert second.present_by is None # Nothing was half-applied: beta is untouched. assert "old_beta" in (src / "mod" / "beta.py").read_text(encoding="utf-8") @@ -116,8 +143,7 @@ def test_nogit_partial_overlap_still_fails(workspace): """A patch only *partly* present is a real failure, not a no-op.""" src, patches, backups = workspace sup = _patch(patches, "superset.patch", _SUPERSET) - ok, _, recs, _ = _apply_patch_no_git(src, sup, backups, seq_offset=0) - assert ok + assert _apply_patch_no_git(src, sup, backups, seq_offset=0).ok # Undo only beta, leaving alpha applied -> superset is half-present. (src / "mod" / "beta.py").write_text("header\nold_beta\nfooter\n", encoding="utf-8") @@ -129,11 +155,11 @@ def test_nogit_unrelated_patch_still_fails(workspace): src, patches, backups = workspace bad = _patch(patches, "bad.patch", _UNRELATED) - ok, err, recs, fb = _apply_patch_no_git(src, bad, backups, seq_offset=0) - assert not ok - assert "failed at all strip levels" in err - assert fb is not None - assert recs == [] + result = _apply_patch_no_git(src, bad, backups, seq_offset=0) + assert not result.ok + assert "failed at all strip levels" in result.error + assert result.feedback is not None + assert result.backups == [] def _git_tree(root: Path) -> None: @@ -145,6 +171,11 @@ def _git_tree(root: Path) -> None: subprocess.run(["git", "-C", str(root), "commit", "-qm", "init"], check=True) +def _git(root: Path, *args: str) -> str: + """Run git in ``root`` and return its stdout.""" + return subprocess.run(["git", "-C", str(root), *args], check=True, capture_output=True, text=True).stdout + + def test_git_channel_superset_then_subset_is_a_noop(tmp_path): """The git channel resolves the same ambiguity via ``git apply -R --check``.""" src = tmp_path / "src" @@ -155,11 +186,13 @@ def test_git_channel_superset_then_subset_is_a_noop(tmp_path): sup = _patch(patches, "superset.patch", _SUPERSET) sub = _patch(patches, "subset.patch", _SUBSET) - ok, err, _ = ip._git_apply_collect_feedback(src, sup) - assert ok, err - ok2, err2, fb2 = ip._git_apply_collect_feedback(src, sub) - assert ok2, err2 - assert fb2 is None + first = psa._git_apply_collect_feedback(src, sup) + assert first.ok, first.error + assert first.present_by is None + second = psa._git_apply_collect_feedback(src, sub) + assert second.ok, second.error + assert second.feedback is None + assert second.present_by is PresenceCheck.GIT_REVERSE_APPLY assert "new_alpha" in (src / "mod" / "alpha.py").read_text(encoding="utf-8") @@ -172,6 +205,391 @@ def test_git_channel_unrelated_patch_still_fails(tmp_path): patches.mkdir() bad = _patch(patches, "bad.patch", _UNRELATED) - ok, err, fb = ip._git_apply_collect_feedback(src, bad) - assert not ok - assert fb is not None + result = psa._git_apply_collect_feedback(src, bad) + assert not result.ok + assert result.feedback is not None + assert result.present_by is None + + +def test_git_channel_reports_a_committed_blob_id_patch_present(tmp_path): + """A committed diff carrying blob ids is reported present by the reverse check; the index stays untouched.""" + src = tmp_path / "src" + _tree(src) + _git_tree(src) + (src / "mod" / "alpha.py").write_text("header\nnew_alpha\nfooter\n", encoding="utf-8") + committed = _patch(tmp_path, "committed.patch", _git(src, "diff")) + assert "index " in committed.read_text(encoding="utf-8") + git_commit_all(src, "land the change") + + result = psa._git_apply_collect_feedback(src, committed) + + assert result.ok, result.error + assert result.present_by is PresenceCheck.GIT_REVERSE_APPLY + assert _git(src, "status", "--porcelain") == "" + + +def test_patch_set_with_one_subset_noop_still_changes_the_tree(tmp_path): + """One satisfied no-op inside a larger set lands with the set; the set is not ``already_present``.""" + src = tmp_path / "src" + _tree(src) + _git_tree(src) + sup = _patch(tmp_path, "superset.patch", _SUPERSET) + sub = _patch(tmp_path, "subset.patch", _SUBSET) + attempt = IntegrateAttempt(task_id="t", framework_root=src) + + outcome = psa.apply_patch_set(attempt, [sup, sub], git_tree=True, backup_root=tmp_path / "bak") + + assert outcome.errors == [] + assert outcome.changed_nothing is False + assert outcome.already_present == [{"patch": str(sub), "check": "git_reverse_apply_check"}] + assert attempt.applied == [sup, sub] + + +_CHANNEL_CHECKS = [(True, PresenceCheck.GIT_REVERSE_APPLY), (False, PresenceCheck.PATCH_REVERSE_DRY_RUN)] + + +def _apply_one(git_tree: bool, root: Path, patch: Path, backups: Path) -> PatchApplyResult: + """Apply one patch through the channel ``git_tree`` selects.""" + if git_tree: + return psa._git_apply_collect_feedback(root, patch) + return _apply_patch_no_git(root, patch, backups) + + +def _channel_tree(tmp_path: Path, git_tree: bool) -> Path: + """The two-file source tree, as a committed git checkout when ``git_tree``.""" + src = tmp_path / "src" + _tree(src) + if git_tree: + _git_tree(src) + return src + + +def _settle(root: Path, git_tree: bool, message: str) -> None: + """Commit the tree's current state when it is a git checkout.""" + if git_tree: + git_commit_all(root, message) + + +@pytest.mark.parametrize(("git_tree", "check"), _CHANNEL_CHECKS) +def test_patch_set_the_tree_already_holds_changes_nothing(tmp_path, git_tree, check): + """Both channels report a set whose every patch is in the tree as having changed nothing.""" + src = _channel_tree(tmp_path, git_tree) + (src / "mod" / "alpha.py").write_text("header\nnew_alpha\nfooter\n", encoding="utf-8") + (src / "mod" / "beta.py").write_text("header\nnew_beta\nfooter\n", encoding="utf-8") + _settle(src, git_tree, "already merged") + sup = _patch(tmp_path, "superset.patch", _SUPERSET) + sub = _patch(tmp_path, "subset.patch", _SUBSET) + + outcome = psa.apply_patch_set( + IntegrateAttempt(task_id="t", framework_root=src), [sup, sub], git_tree=git_tree, backup_root=tmp_path / "bak" + ) + + assert outcome.errors == [] + assert outcome.changed_nothing is True + assert outcome.already_present == [{"patch": str(p), "check": check.value} for p in (sup, sub)] + + +@pytest.mark.parametrize("git_tree", [True, False]) +def test_deletion_of_a_drifted_file_is_a_failure_not_present(tmp_path, git_tree): + """The file the patch deletes is still there with other content, so undoing the deletion cannot prove presence.""" + src = _channel_tree(tmp_path, git_tree) + (src / "mod" / "alpha.py").write_text("header\ndrifted\nfooter\n", encoding="utf-8") + _settle(src, git_tree, "drift") + patch = _patch(tmp_path, "delete.patch", _DELETE_ALPHA) + + result = _apply_one(git_tree, src, patch, tmp_path / "bak") + + assert not result.ok + assert result.present_by is None + assert result.feedback is not None + assert (src / "mod" / "alpha.py").read_text(encoding="utf-8") == "header\ndrifted\nfooter\n" + + +@pytest.mark.parametrize(("git_tree", "check"), _CHANNEL_CHECKS) +def test_deletion_already_carried_out_is_present(tmp_path, git_tree, check): + """With the file gone there is nothing left to anchor on, and the deletion is a satisfied no-op.""" + src = _channel_tree(tmp_path, git_tree) + (src / "mod" / "alpha.py").unlink() + _settle(src, git_tree, "deleted upstream") + patch = _patch(tmp_path, "delete.patch", _DELETE_ALPHA) + + result = _apply_one(git_tree, src, patch, tmp_path / "bak") + + assert result.ok, result.error + assert result.present_by is check + + +# Shapes ``git apply`` and ``patch`` accept whose leading block names no file, so the manifest cannot be parsed. +_FORMAT_PATCH = ( + "From 0 Mon Sep 17 00:00:00 2001\nSubject: [PATCH] alpha\n\n---\ndiff --git a/mod/alpha.py b/mod/alpha.py\n" +) +_DIFF_RUN = "diff -ruN a/mod/alpha.py b/mod/alpha.py\n" + + +@pytest.mark.parametrize("preamble", [_FORMAT_PATCH, _DIFF_RUN], ids=["format_patch", "diff_ruN"]) +@pytest.mark.parametrize(("git_tree", "check"), _CHANNEL_CHECKS) +def test_patch_with_unparseable_manifest_is_probed_at_every_level(tmp_path, git_tree, check, preamble): + """A patch the manifest parser rejects is still reported present when the tree holds it.""" + src = _channel_tree(tmp_path, git_tree) + (src / "mod" / "alpha.py").write_text("header\nnew_alpha\nfooter\n", encoding="utf-8") + _settle(src, git_tree, "already merged") + patch = _patch(tmp_path, "mail.patch", preamble + _SUBSET) + + result = _apply_one(git_tree, src, patch, tmp_path / "bak") + + assert result.ok, result.error + assert result.present_by is check + + +def _make_ctx(task_id: str, params: dict, state: SharedState) -> RunnerContext: + task = Task( + task_id=task_id, + kind="integrate_patch", + state="queued", + params=params, + idempotency_key=task_id, + requires_lanes=tuple(), + ) + return RunnerContext(task=task, lease=None, extra={"shared_state": state}) + + +def _approved_state() -> SharedState: + """Session state carrying the Critic's approval of the test PR.""" + state = SharedState() + state.record_specialist_patch_verdict(_PR_URL, "approve") + return state + + +@pytest.fixture +def executor_env(tmp_path, monkeypatch): + """A session dir, a KB root, and an executor whose benchmark fails the test if it is reached.""" + from hyperloom.orchestrator.actions.executors import _multi_node_env + + def no_bench(*_args, **_kwargs): + pytest.fail("a candidate the tree already holds must not be benchmarked") + + monkeypatch.setattr(_multi_node_env, "is_multi_node", lambda: False) + monkeypatch.setattr(ip.IntegratePatchExecutor, "_bench_patch", no_bench) + monkeypatch.setattr(ip, "run_grid", no_bench) + kb_root = tmp_path / "kb" + monkeypatch.setenv("INFERENCE_OPTIMIZER_FA_KB_PATH", str(kb_root)) + session_dir = tmp_path / "session" + session_dir.mkdir() + return ip.IntegratePatchExecutor(session_dir=session_dir), kb_root + + +def _repo_holding_the_superset(tmp_path: Path) -> Path: + """A git checkout whose HEAD already carries every hunk of ``_SUPERSET``.""" + repo = tmp_path / "framework" + _tree(repo) + _git_tree(repo) + (repo / "mod" / "alpha.py").write_text("header\nnew_alpha\nfooter\n", encoding="utf-8") + (repo / "mod" / "beta.py").write_text("header\nnew_beta\nfooter\n", encoding="utf-8") + git_commit_all(repo, "upstream already merged it") + return repo + + +def _specialist_round(session_dir: Path, repo: Path) -> tuple[dict, list[Path]]: + """A specialist-authored round delivering the superset and its subset, with a PR identity.""" + sid = "spec-present" + workspace = runs_dir(session_dir, "specialist", sid) + patches = [ + _patch(_mkdir(workspace / "worktree"), "superset.patch", _SUPERSET), + _patch(workspace / "worktree", "subset.patch", _SUBSET), + ] + done = { + "patches_written": [p.name for p in patches], + "proposal_set": [{"provenance": "specialist:serving:framework", "fa_pr_url": _PR_URL}], + } + (workspace / "specialist_done.json").write_text(json.dumps(done), encoding="utf-8") + return {"specialist_task_id": sid, "framework_source_root": str(repo)}, patches + + +def _upstream_round(session_dir: Path, task_id: str, repo: Path) -> tuple[dict, list[Path]]: + """An upstream-PR round landing the superset and its subset from the task's own scratch.""" + scratch = _mkdir(runs_dir(session_dir, "integrate_patch", task_id)) + patches = [_patch(scratch, "superset.patch", _SUPERSET), _patch(scratch, "subset.patch", _SUBSET)] + params = { + "patch_source": "upstream_pr", + "candidate": {"pr_url": _PR_URL, "repo": "example-org/example-framework"}, + "framework_agent_candidate_id": _PR_URL, + "framework_source_root": str(repo), + "patches": [str(p) for p in patches], + } + return params, patches + + +def _mkdir(path: Path) -> Path: + path.mkdir(parents=True, exist_ok=True) + return path + + +@pytest.mark.asyncio +@pytest.mark.parametrize("lane", ["upstream_pr", "specialist_authored"]) +async def test_executor_ends_an_all_noop_patch_set_as_already_present(tmp_path, executor_env, lane): + """Every patch already in the tree: no benchmark, an ``already_present`` result and one KB row saying so.""" + executor, kb_root = executor_env + repo = _repo_holding_the_superset(tmp_path) + head = _git(repo, "rev-parse", "HEAD") + if lane == "upstream_pr": + params, patches = _upstream_round(executor.session_dir, "t-present", repo) + else: + params, patches = _specialist_round(executor.session_dir, repo) + state = SharedState() + state.record_specialist_patch_verdict(_PR_URL if lane == "upstream_pr" else params["specialist_task_id"], "approve") + + result = await executor(_make_ctx("t-present", params, state)) + + assert result["status"] == "already_present" + assert result["already_present"] == [{"patch": str(p), "check": "git_reverse_apply_check"} for p in patches] + assert result["patches_applied"] == [] + assert result["patches_reverted"] == [] + rows = read_pr_ledger(kb_root=kb_root) + assert [(row["outcome"], row["pr_url"]) for row in rows] == [("already_present", _PR_URL)] + assert _git(repo, "rev-parse", "HEAD") == head + assert _git(repo, "status", "--porcelain") == "" + + +def _record_bench(monkeypatch) -> list[str]: + """Replace the benchmark with one that records who reached it and then fails, ending the round.""" + reached: list[str] = [] + + def bench(*_args, **kwargs): + reached.append(kwargs["specialist_task_id"]) + raise RuntimeError("benchmark stand-in") + + monkeypatch.setattr(ip.IntegratePatchExecutor, "_bench_patch", bench) + return reached + + +def _repo_with_merged_pr_head(tmp_path: Path, *, revert: bool) -> tuple[Path, str]: + """A checkout whose ``main`` merged ``pr-head`` (the superset) with a merge commit, optionally reverted since.""" + repo = tmp_path / "framework" + _tree(repo) + _git_tree(repo) + _git(repo, "checkout", "-qb", "pr-head") + (repo / "mod" / "alpha.py").write_text("header\nnew_alpha\nfooter\n", encoding="utf-8") + (repo / "mod" / "beta.py").write_text("header\nnew_beta\nfooter\n", encoding="utf-8") + git_commit_all(repo, "the pr") + head = _git(repo, "rev-parse", "HEAD").strip() + _git(repo, "checkout", "-q", "-") + (repo / "other.py").write_text("x = 1\n", encoding="utf-8") + git_commit_all(repo, "unrelated") + _git(repo, "merge", "-q", "--no-ff", "-m", "merge the pr", "pr-head") + if revert: + _git(repo, "revert", "--no-edit", "-m", "1", "HEAD") + _git(repo, "remote", "add", "origin", str(repo)) + return repo, head + + +def _pr_head_round(repo: Path, head: str, **candidate_extra: object) -> dict: + """An upstream-PR round naming the PR by its head ref, as discovery does.""" + return { + "patch_source": "upstream_pr", + "candidate": { + "pr_url": _PR_URL, + "repo": "example-org/example-framework", + "ref": "pr-head", + "head_sha": head, + "changed_files": ["mod/alpha.py", "mod/beta.py"], + **candidate_extra, + }, + "framework_agent_candidate_id": _PR_URL, + "framework_source_root": str(repo), + } + + +@pytest.mark.asyncio +async def test_executor_ends_a_merged_pr_head_as_already_present_without_a_fetch(tmp_path, executor_env, monkeypatch): + """The head is merged and the tree has its copy of every file the PR changes: no fetch, no apply, no benchmark.""" + from hyperloom.orchestrator.actions.executors import _patch_source_pr + + def no_fetch(*_args, **_kwargs): + pytest.fail("a PR the live tree already holds needs no diff") + + monkeypatch.setattr(_patch_source_pr, "_fetch_diff_to_path", no_fetch) + executor, kb_root = executor_env + repo, head = _repo_with_merged_pr_head(tmp_path, revert=False) + + result = await executor(_make_ctx("t-merged", _pr_head_round(repo, head), _approved_state())) + + assert result["status"] == "already_present" + assert result["specialist_task_id"] == "t-merged" + assert result["already_present"] == [{"diff_range": f"{head}..{head}", "check": "pr_head_files_in_tree"}] + rows = read_pr_ledger(kb_root=kb_root) + assert [(row["outcome"], row["pr_url"]) for row in rows] == [("already_present", _PR_URL)] + + +def _serve_superset(monkeypatch) -> None: + """Make every ``diff_url`` fetch return the PR's diff.""" + from hyperloom.orchestrator.actions.executors import _patch_source_pr + + def fetch(_url, dest, *, timeout_sec): + dest.write_text(_SUPERSET, encoding="utf-8") + return True, "" + + monkeypatch.setattr(_patch_source_pr, "_fetch_diff_to_path", fetch) + + +@pytest.mark.asyncio +async def test_executor_ends_a_merged_pr_fetched_by_diff_url_as_already_present(tmp_path, executor_env, monkeypatch): + """An empty head diff defers to the diff_url, and the apply's reverse check finds the PR in the tree.""" + _serve_superset(monkeypatch) + executor, kb_root = executor_env + repo, head = _repo_with_merged_pr_head(tmp_path, revert=False) + params = _pr_head_round(repo, head, diff_url="https://example.invalid/pr.diff", prefer_checkout=True) + + result = await executor(_make_ctx("t-merged-url", params, _approved_state())) + + assert result["status"] == "already_present" + assert [row["check"] for row in result["already_present"]] == ["git_reverse_apply_check"] + assert [row["outcome"] for row in read_pr_ledger(kb_root=kb_root)] == ["already_present"] + + +@pytest.mark.asyncio +async def test_executor_benchmarks_a_reverted_pr_head(tmp_path, executor_env, monkeypatch): + """A merged-then-reverted PR is still in the history but not in the tree, so it is applied and measured.""" + _serve_superset(monkeypatch) + reached = _record_bench(monkeypatch) + executor, kb_root = executor_env + repo, head = _repo_with_merged_pr_head(tmp_path, revert=True) + params = _pr_head_round(repo, head, diff_url="https://example.invalid/pr.diff", prefer_checkout=True) + + result = await executor(_make_ctx("t-reverted", params, _approved_state())) + + assert reached == ["t-reverted"] + assert result["status"] != "already_present" + assert "already_present" not in [row["outcome"] for row in read_pr_ledger(kb_root=kb_root)] + + +@pytest.mark.asyncio +async def test_executor_benchmarks_an_all_noop_set_that_changes_server_args(tmp_path, executor_env, monkeypatch): + """Patches already in the tree do not end a round that also changes the server args: those still need measuring.""" + reached = _record_bench(monkeypatch) + executor, kb_root = executor_env + repo = _repo_holding_the_superset(tmp_path) + params, _patches = _upstream_round(executor.session_dir, "t-args", repo) + params["extra_server_args"] = "--max-num-seqs 64" + + result = await executor(_make_ctx("t-args", params, _approved_state())) + + assert reached == ["t-args"] + assert result["status"] != "already_present" + assert "already_present" not in [row["outcome"] for row in read_pr_ledger(kb_root=kb_root)] + + +@pytest.mark.asyncio +async def test_executor_lands_a_set_whose_noop_rides_with_a_real_change(tmp_path, executor_env): + """Only a set that changes nothing ends early; a no-op beside a real change lands with it.""" + executor, kb_root = executor_env + repo = tmp_path / "framework" + _tree(repo) + _git_tree(repo) + params, patches = _upstream_round(executor.session_dir, "t-mixed", repo) + params["apply_only"] = True + + result = await executor(_make_ctx("t-mixed", params, _approved_state())) + + assert result["status"] == "applied_no_bench" + assert result["patches_applied"] == [str(p) for p in patches] + assert read_pr_ledger(kb_root=kb_root) == [] diff --git a/src/hyperloom/orchestrator/delivery/tests/test_delivery_channels.py b/src/hyperloom/orchestrator/delivery/tests/test_delivery_channels.py index 4b2ffe4554..d454920ddd 100644 --- a/src/hyperloom/orchestrator/delivery/tests/test_delivery_channels.py +++ b/src/hyperloom/orchestrator/delivery/tests/test_delivery_channels.py @@ -82,8 +82,8 @@ def test_the_non_git_channel_records_every_backup_before_it_mutates(tmp_path: Pa ) backup_root = tmp_path / "backups" - ok, err, backups, _ = _apply_patch_no_git(root, patch, backup_root) - assert ok, err + result = _apply_patch_no_git(root, patch, backup_root) + assert result.ok, result.error assert target.read_text(encoding="utf-8") == "two\n" # The record is on disk, not only in the list the caller happens to hold. diff --git a/src/hyperloom/orchestrator/framework/artifacts.py b/src/hyperloom/orchestrator/framework/artifacts.py index 2db89e8b57..c425547911 100644 --- a/src/hyperloom/orchestrator/framework/artifacts.py +++ b/src/hyperloom/orchestrator/framework/artifacts.py @@ -10,7 +10,9 @@ # Per-candidate terminal statuses that mean the candidate reached the apply/bench stage (as opposed to being filtered # before any source change). -_TESTED_STATUSES: frozenset[str] = frozenset({"kept", "reverted", "applied_no_bench", "apply_failed"}) +_TESTED_STATUSES: frozenset[str] = frozenset( + {"kept", "reverted", "applied_no_bench", "apply_failed", "already_present"} +) def candidate_key(row: dict[str, Any] | None) -> str: diff --git a/src/hyperloom/orchestrator/knowledge/kb_writeback.py b/src/hyperloom/orchestrator/knowledge/kb_writeback.py index 21759f766e..73a1a38798 100644 --- a/src/hyperloom/orchestrator/knowledge/kb_writeback.py +++ b/src/hyperloom/orchestrator/knowledge/kb_writeback.py @@ -22,7 +22,7 @@ def _default_kb_root() -> Path: OUTCOME_INTEGRATED: str = "integrated" OUTCOME_REVERTED_SMOKE_FAIL: str = "reverted_smoke_fail" OUTCOME_REJECTED_APPLY_FAIL: str = "rejected_apply_fail" -# Candidate skipped: semantic audit found it already in the live tree. +# Not benchmarked: the live tree already held every change the candidate makes. OUTCOME_ALREADY_PRESENT: str = "already_present" # An env-gated rewrite whose switch-off parity leg measured a behavioural change: with every switch unset the patch # did not reproduce the base. diff --git a/src/hyperloom/orchestrator/phases/framework.py b/src/hyperloom/orchestrator/phases/framework.py index ed2784b329..a03ec45a39 100644 --- a/src/hyperloom/orchestrator/phases/framework.py +++ b/src/hyperloom/orchestrator/phases/framework.py @@ -122,7 +122,7 @@ def _framework_config_levers_from_done( parsed_args = tokenize_server_args_preserving_json(" ".join(arg_tokens)) if parsed_args is None: log.warning( - "FRAMEWORK config lever %r has unparseable server args; dropping the args%s", + "FRAMEWORK config lever %r has unparsable server args; dropping the args%s", entry.get("name"), " while preserving its environment overrides" if extra_envs else "", ) @@ -1889,6 +1889,7 @@ def _collect_framework_agent_candidate_priors(self) -> dict[str, Any]: "kept", "kept_inert", "reverted", + "already_present", "no_patch", "enqueue_failed", FRAMEWORK_CRITIC_DENIED_STATUS, @@ -2576,20 +2577,11 @@ def _record_framework_agent_authoring_empty_outcome( if not cand_id: return batch_id = str(params.get("framework_batch_id") or "") - # Map the cached audit verdict to a terminal status. - audit = params.get("framework_audit") if isinstance(params.get("framework_audit"), dict) else {} - sem = str((audit or {}).get("semantic_status") or "").strip().lower() - if sem in ("already_equivalent", "already_superset"): - status = "already_present" - elif sem in ("not_present", "partially_present"): - status = "not_applicable" - else: - status = "author_empty" reason = str(inner.get("summary") or "").strip()[:500] recorded = self._stamp_framework_progress( candidate_id=cand_id, batch_id=batch_id, - status=status, + status="author_empty", kept=False, rationale=reason, provenance="authored_empty", @@ -2601,12 +2593,7 @@ def _record_framework_agent_authoring_empty_outcome( ) if not recorded: return - log.info( - "FRAMEWORK: authoring specialist empty deliverable candidate=%s batch=%s status=%s", - cand_id, - batch_id, - status, - ) + log.info("FRAMEWORK: authoring specialist empty deliverable candidate=%s batch=%s", cand_id, batch_id) async def _candidate_discovery_inflight(self) -> bool: """True while a candidate-discovery specialist is queued or running.""" diff --git a/src/hyperloom/orchestrator/specialists/patch_safety.py b/src/hyperloom/orchestrator/specialists/patch_safety.py index dbc736dfa6..5dda1f74e4 100644 --- a/src/hyperloom/orchestrator/specialists/patch_safety.py +++ b/src/hyperloom/orchestrator/specialists/patch_safety.py @@ -217,6 +217,28 @@ def patch_file_targets(patch_text: str) -> list[tuple[str, str]]: return pairs +def path_resolves_at(root: Path, path: str, level: int) -> bool: + """True when the diff path ``path``, stripped as ``-p`` would strip it, names an entry under ``root``. + + Args: + root: The tree to resolve it in. + path: A raw ``---``/``+++`` path token. + level: The ``-p`` strip level. + + Returns: + Whether the stripped path exists under ``root``. + """ + stripped = strip_path_components(path, level) + # A bare filename matches any root holding that name, so deep + # strips of a nested path would implicate unrelated repos. + if "/" not in stripped and level > 1: + return False + try: + return (root / stripped).exists() + except OSError: + return False + + def patch_targets_missing( patch_text: str, root: Path, @@ -247,24 +269,7 @@ def patch_targets_missing( existing = list(parse_patch_targets(patch_text).existing) except ValueError: return [""] - missing: list[str] = [] - for old in existing: - found = False - for lvl in strip_levels: - try: - stripped = strip_path_components(old, lvl) - # A bare filename matches any root holding that name, so deep - # strips of a nested path would implicate unrelated repos. - if "/" not in stripped and lvl > 1: - continue - if (root / stripped).exists(): - found = True - break - except OSError: - continue - if not found: - missing.append(old) - return missing + return [old for old in existing if not any(path_resolves_at(root, old, lvl) for lvl in strip_levels)] def _collapse_nested_roots(roots: tuple[Path, ...]) -> tuple[Path, ...]: @@ -1100,6 +1105,7 @@ def vet_patches( "patch_is_annotation_only", "patch_targets_missing", "patch_work_artifact_targets", + "path_resolves_at", "quantitative_claim_rule_descriptor", "resolve_patch_apply_root", "scan_numeric_claims", From ba4d3b0ad1a4ade1bd2f9070c6c6a39678ce50f9 Mon Sep 17 00:00:00 2001 From: okorzh Date: Tue, 13 Oct 2026 21:20:54 -0500 Subject: [PATCH 02/11] fix(integrate_patch): keep _patch_set_apply out of the executor's import cycle apply_patch_set took the whole IntegrateAttempt, so _patch_set_apply imported _integrate_attempt. That module already reaches back to integrate_patch on main (through shared_state, gaps, collaborator, the coordinator and writeback), so the new module joined that cycle and CodeQL flagged both ends of it. apply_patch_set reads only the attempt's framework root and appends to its applied list, so it now takes those two. The list is still filled as each patch lands, so an interrupted stage still reports what reached the tree. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../actions/executors/_patch_set_apply.py | 21 ++++++++----------- .../actions/executors/integrate_patch.py | 6 +++++- .../tests/test_patch_already_applied_unit.py | 11 ++++------ 3 files changed, 18 insertions(+), 20 deletions(-) diff --git a/src/hyperloom/orchestrator/actions/executors/_patch_set_apply.py b/src/hyperloom/orchestrator/actions/executors/_patch_set_apply.py index 2d06e1eef9..a4a91f1b3a 100644 --- a/src/hyperloom/orchestrator/actions/executors/_patch_set_apply.py +++ b/src/hyperloom/orchestrator/actions/executors/_patch_set_apply.py @@ -13,16 +13,13 @@ import logging from dataclasses import dataclass, field from pathlib import Path -from typing import TYPE_CHECKING, Any +from typing import Any from ...specialists.patch_safety import is_unified_diff, patch_escapes_tree from ._apply_feedback import ApplyFeedback, PatchApplyResult, PresenceCheck, build_apply_feedback from ._git import _run_git_cp from ._nogit_patch import _P_LEVELS, _apply_patch_no_git, _reverse_probe_levels -if TYPE_CHECKING: - from ._integrate_attempt import IntegrateAttempt - log = logging.getLogger(__name__) @@ -167,30 +164,30 @@ def _unvetted_reason(patch: Path) -> str: def apply_patch_set( - attempt: IntegrateAttempt, + framework_root: Path, patch_paths: list[Path], *, git_tree: bool, backup_root: Path, + applied: list[Path], ) -> PatchSetOutcome: - """Apply ``patch_paths`` in order into ``attempt.framework_root``, stopping at the first that fails. + """Apply ``patch_paths`` in order into ``framework_root``, stopping at the first that fails. A patch the tree already holds still counts as landed, so one subset patch - inside a larger set does not fail the set. Each landed patch is appended to - ``attempt.applied`` as it lands, so a stage that never returns still - reports what reached the tree. + inside a larger set does not fail the set. Args: - attempt: The attempt whose tree the set lands in. + framework_root: The tree the set lands in. patch_paths: The patches, in apply order. git_tree: Whether the tree is a git checkout, which goes back through its HEAD; any other tree goes through ``patch`` with backups. backup_root: Where the ``patch`` channel keeps its per-file backups. + applied: Receives each patch as it lands, so a caller whose stage + never returns still knows what reached the tree. Returns: What the set did to the tree. """ - framework_root = attempt.framework_root outcome = PatchSetOutcome() backups: list[dict[str, Any]] = [] for patch in patch_paths: @@ -210,7 +207,7 @@ def apply_patch_set( break if result.present_by is not None: outcome.already_present.append({"patch": str(patch), "check": result.present_by.value}) - attempt.applied.append(patch) + applied.append(patch) outcome.changed_nothing = ( bool(patch_paths) and not outcome.errors and len(outcome.already_present) == len(patch_paths) ) diff --git a/src/hyperloom/orchestrator/actions/executors/integrate_patch.py b/src/hyperloom/orchestrator/actions/executors/integrate_patch.py index 1aed7352ec..28871c4895 100644 --- a/src/hyperloom/orchestrator/actions/executors/integrate_patch.py +++ b/src/hyperloom/orchestrator/actions/executors/integrate_patch.py @@ -2671,7 +2671,11 @@ async def _stage_apply( applied_artifacts: list[dict[str, Any]] = [] attempt.applied_artifacts = applied_artifacts patch_set = apply_patch_set( - attempt, patch_paths, git_tree=bool(git_head), backup_root=recovery_root / "patch_backups" + framework_root, + patch_paths, + git_tree=bool(git_head), + backup_root=recovery_root / "patch_backups", + applied=attempt.applied, ) if patch_set.errors: await self._maybe_write_framework_kb_record( diff --git a/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py b/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py index a2e058b1be..26ea0a07a1 100644 --- a/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py +++ b/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py @@ -16,7 +16,6 @@ from hyperloom.orchestrator.actions.executors import _patch_set_apply as psa from hyperloom.orchestrator.actions.executors import integrate_patch as ip from hyperloom.orchestrator.actions.executors._apply_feedback import PatchApplyResult, PresenceCheck -from hyperloom.orchestrator.actions.executors._integrate_attempt import IntegrateAttempt from hyperloom.orchestrator.actions.executors._nogit_patch import ( _apply_patch_no_git, _reverse_applies_cleanly, @@ -235,14 +234,14 @@ def test_patch_set_with_one_subset_noop_still_changes_the_tree(tmp_path): _git_tree(src) sup = _patch(tmp_path, "superset.patch", _SUPERSET) sub = _patch(tmp_path, "subset.patch", _SUBSET) - attempt = IntegrateAttempt(task_id="t", framework_root=src) + applied: list[Path] = [] - outcome = psa.apply_patch_set(attempt, [sup, sub], git_tree=True, backup_root=tmp_path / "bak") + outcome = psa.apply_patch_set(src, [sup, sub], git_tree=True, backup_root=tmp_path / "bak", applied=applied) assert outcome.errors == [] assert outcome.changed_nothing is False assert outcome.already_present == [{"patch": str(sub), "check": "git_reverse_apply_check"}] - assert attempt.applied == [sup, sub] + assert applied == [sup, sub] _CHANNEL_CHECKS = [(True, PresenceCheck.GIT_REVERSE_APPLY), (False, PresenceCheck.PATCH_REVERSE_DRY_RUN)] @@ -280,9 +279,7 @@ def test_patch_set_the_tree_already_holds_changes_nothing(tmp_path, git_tree, ch sup = _patch(tmp_path, "superset.patch", _SUPERSET) sub = _patch(tmp_path, "subset.patch", _SUBSET) - outcome = psa.apply_patch_set( - IntegrateAttempt(task_id="t", framework_root=src), [sup, sub], git_tree=git_tree, backup_root=tmp_path / "bak" - ) + outcome = psa.apply_patch_set(src, [sup, sub], git_tree=git_tree, backup_root=tmp_path / "bak", applied=[]) assert outcome.errors == [] assert outcome.changed_nothing is True From a03264591c652141bea3d10f239ed0e29e63f519 Mon Sep 17 00:00:00 2001 From: okorzh Date: Tue, 13 Oct 2026 21:24:47 -0500 Subject: [PATCH 03/11] fix(integrate_patch): end a merged PR head as already_present only when the round changes nothing else The upstream-PR lane ended a candidate whose head the tree already holds in _stage_resolve, before the round's server args, env, artifacts, setup and runtime were read. A task carrying any of them therefore ended already_present and wrote the KB row without measuring that change, while the patch-set exit in _stage_apply benchmarks the same round. The round's proposal (config changes, server args, safe env with the switch env, artifact specs) is now read by one function, and the "changes beyond patches" rule is one method on its result. _stage_apply and the PR-head exit both use it. The PR-head exit runs before setup replay and runtime provisioning, which happen only in an enablement round, so such a round counts as changing both. When the round changes more, the exit is not taken and the checkout-head failure (fetch_failed / checkout_head_failed) is returned, as on main: the evidence now rides on that failure instead of replacing it. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/hyperloom/inference_optimizer/SKILL.md | 7 +- .../tests/test_patch_source_pr.py | 8 +- .../actions/executors/_patch_source_pr.py | 23 +-- .../actions/executors/integrate_patch.py | 170 +++++++++++------- .../tests/test_patch_already_applied_unit.py | 46 +++++ 5 files changed, 175 insertions(+), 79 deletions(-) diff --git a/src/hyperloom/inference_optimizer/SKILL.md b/src/hyperloom/inference_optimizer/SKILL.md index ee22c73d5c..709355494a 100644 --- a/src/hyperloom/inference_optimizer/SKILL.md +++ b/src/hyperloom/inference_optimizer/SKILL.md @@ -448,9 +448,10 @@ top; REVERT does `git reset --hard`. A candidate the tree already holds is not benchmarked: when every patch in the set reverse-applies cleanly and the round changes nothing else, it ends `already_present`, with that evidence on the result and an `already_present` row in the framework KB. An upstream PR whose -head is already in the live history ends the same way only when the tree still -has the head's copy of every file the PR changes; with a `diff_url`, the -fetched diff's reverse check decides instead, so a reverted PR is benchmarked. +head is already in the live history ends the same way when the round changes +nothing else and the tree still has the head's copy of every file the PR +changes; otherwise it ends `fetch_failed`. With a `diff_url`, the fetched +diff's reverse check decides instead, so a reverted PR is benchmarked. Resume skips completed candidates by idempotency key. The phase exits when both arms are dry, when its budget is spent, or at the diff --git a/src/hyperloom/inference_optimizer/tests/test_patch_source_pr.py b/src/hyperloom/inference_optimizer/tests/test_patch_source_pr.py index b5dc7129b7..009e3502f2 100644 --- a/src/hyperloom/inference_optimizer/tests/test_patch_source_pr.py +++ b/src/hyperloom/inference_optimizer/tests/test_patch_source_pr.py @@ -137,14 +137,18 @@ def _materialize_head(repo: Path, tmp_path: Path, head_sha: str, **extra: object def test_materialize_reports_a_merged_pr_head_as_already_present(tmp_path: Path): - """The tree has the head's copy of every file the PR changes: nothing to apply, nothing failed.""" + """The tree has the head's copy of every file the PR changes: nothing to apply, and evidence that it is in. + + The empty-diff failure stays on the result for a round that changes more than the PR. + """ repo = tmp_path / "framework" head_sha = _init_repo_with_pr_branch(repo) _merge_pr_head(repo, revert=False) out = _materialize_head(repo, tmp_path, head_sha, changed_files=["src.py"]) - assert out.failure is None + assert out.failure is not None and out.failure["error_class"] == "checkout_head_failed" + assert "empty diff" in out.failure["error"] assert out.patches == [] assert out.already_present == [{"diff_range": f"{head_sha}..{head_sha}", "check": "pr_head_files_in_tree"}] diff --git a/src/hyperloom/orchestrator/actions/executors/_patch_source_pr.py b/src/hyperloom/orchestrator/actions/executors/_patch_source_pr.py index 98b0d2b573..ebcadefc51 100644 --- a/src/hyperloom/orchestrator/actions/executors/_patch_source_pr.py +++ b/src/hyperloom/orchestrator/actions/executors/_patch_source_pr.py @@ -203,7 +203,7 @@ def _materialize_pr_diff_from_head( class PrMaterialization: """What an upstream-PR candidate resolved to on disk.""" - #: The patch files to apply; empty when :attr:`failure` or :attr:`already_present` is set. + #: The patch files to apply; empty when :attr:`failure` is set. patches: list[Path] = field(default_factory=list) #: Which source produced them: ``explicit`` / ``checkout_head`` / #: ``diff_url_fallback`` / ``diff_url``. @@ -212,7 +212,8 @@ class PrMaterialization: #: caller returns it as-is rather than benching an unpatched tree. failure: dict[str, Any] | None = None #: Evidence that the live tree already holds every change the candidate - #: makes, so there is nothing to apply. + #: makes. :attr:`failure` is set with it, for a round that changes more + #: than the candidate and so cannot end on this evidence. already_present: list[dict[str, str]] = field(default_factory=list) @@ -237,19 +238,19 @@ def _empty_head_diff( That shows only that the head is in the live history, which stays true after a revert. A candidate with a ``diff_url`` therefore falls back to it, and the apply's reverse check decides. Without one, the tree holds the PR - only when it still has the head's copy of every file the PR changes. + only when it still has the head's copy of every file the PR changes, and + that evidence rides on the empty-diff failure rather than replacing it. """ + empty = _checkout_head_failed( + _CHECKOUT_HEAD, + f"checkout-head produced an empty diff for range {diff_range!r} (PR head already merged into live tree?)", + ) has_diff_url = bool(str(candidate.get("diff_url") or "").strip()) if has_diff_url or not _tree_has_head_files(framework_root, head_sha, candidate.get("changed_files")): - return _checkout_head_failed( - _CHECKOUT_HEAD, - f"checkout-head produced an empty diff for range {diff_range!r} (PR head already merged into live tree?)", - ) + return empty log.info("upstream_pr: %s diffs empty and the tree has the head's files; the PR is already in it", diff_range) - return PrMaterialization( - mode=_CHECKOUT_HEAD, - already_present=[{"diff_range": diff_range, "check": PresenceCheck.PR_HEAD_FILES.value}], - ) + empty.already_present.append({"diff_range": diff_range, "check": PresenceCheck.PR_HEAD_FILES.value}) + return empty def _tree_has_head_files(framework_root: Path, head_sha: str, changed_files: Any) -> bool: diff --git a/src/hyperloom/orchestrator/actions/executors/integrate_patch.py b/src/hyperloom/orchestrator/actions/executors/integrate_patch.py index 28871c4895..6a1defd0ab 100644 --- a/src/hyperloom/orchestrator/actions/executors/integrate_patch.py +++ b/src/hyperloom/orchestrator/actions/executors/integrate_patch.py @@ -1422,6 +1422,80 @@ def _resolve_artifact_specs( return specs, errors +@dataclass(frozen=True) +class _RoundProposal: + """What a round proposes besides its patches: launch config and installable artifacts.""" + + config_changes: dict[str, str] + extra_server_args: str + #: The proposed env that passed the safety filter, with the switch env on top. + extra_envs: dict[str, str] + dropped_env_overrides: list[str] + switch_manifest: list[dict[str, Any]] + switch_problems: list[str] + artifact_specs: list[_ArtifactSpec] + artifact_errors: list[dict[str, str]] + + def changes_beyond_patches(self, *, setup: bool, runtime: bool) -> bool: + """Whether there is something to measure even when every patch is already in the tree. + + ``setup`` and ``runtime`` say whether the round replays setup commands and boots a runtime it acquired. + """ + return bool(self.extra_server_args or self.extra_envs or self.artifact_specs or setup or runtime) + + +def _read_round_proposal( + params: dict[str, Any], + done_payload: dict[str, Any] | None, + *, + workspace: Path, + delivers_code: bool, +) -> _RoundProposal: + """Read a round's proposal from its params, then its specialist's done payload; nothing is applied or logged. + + An artifact source must resolve inside ``workspace``, and the framework-rewrite switches gate the code the + round carries, so they are discarded unless ``delivers_code``. + """ + config_changes = dict(params.get("config_changes") or {}) + if not config_changes and done_payload: + cc = done_payload.get("config_changes") + if isinstance(cc, dict): + config_changes = {str(k): str(v) for k, v in cc.items()} + extra_args = params.get("extra_server_args") + envs = dict(config_changes) + raw_extra_envs = params.get("extra_envs") + if isinstance(raw_extra_envs, dict): + envs.update({str(k): str(v) for k, v in raw_extra_envs.items()}) + envs, dropped = filter_untrusted_env_mapping(envs, allow_predicate=is_allowed_variant_env_key) + switch_manifest, switch_problems = _parse_framework_switches(params=params, done_payload=done_payload) + if switch_manifest and not delivers_code: + # A manifest without a patch describes switches that gate code which + # was never delivered. Setting them would be a no-op, and registering + # them as levers would leave the ledger pointing at absent code. + switch_problems.append( + f"discarded {len(switch_manifest)} switch(es): the deliverable carries no patch, " + f"so there is no rewrite for them to gate" + ) + switch_manifest = [] + envs.update(_switch_manifest.switch_env(switch_manifest)) + explicit_artifacts = params.get("artifacts") + artifact_specs, artifact_errors = _resolve_artifact_specs( + specialist_workspace=workspace, + explicit_artifacts=(list(explicit_artifacts) if isinstance(explicit_artifacts, list) else None), + done_payload=done_payload, + ) + return _RoundProposal( + config_changes=config_changes, + extra_server_args=extra_args if isinstance(extra_args, str) and extra_args.strip() else "", + extra_envs=envs, + dropped_env_overrides=sorted(dropped), + switch_manifest=switch_manifest, + switch_problems=switch_problems, + artifact_specs=artifact_specs, + artifact_errors=artifact_errors, + ) + + def _is_aiter_gemm_model_config(spec: _ArtifactSpec) -> bool: """Return whether an artifact is an AITER runtime GEMM model-config CSV.""" if spec.source.suffix.lower() != ".csv": @@ -2028,6 +2102,8 @@ async def _materialize_upstream_pr( ) -> dict[str, Any] | None: """Fetch the candidate's diff into ``params['patches']``, or end the attempt when there is none to apply. + A tree that already holds the PR ends it only when the round changes nothing else, as in :meth:`_stage_apply`. + Returns: A terminal result when no diff could be fetched or the tree already holds the candidate, else ``None``. @@ -2042,7 +2118,12 @@ async def _materialize_upstream_pr( diff_fetch_timeout_sec=DEFAULT_DIFF_FETCH_TIMEOUT_SEC, ) if materialized.already_present: - return await self._end_already_present(attempt, params, extra, materialized.already_present) + proposal = _read_round_proposal(params, attempt.done_payload, workspace=scratch, delivers_code=True) + # Setup replay and runtime provisioning come after this point, and + # only in an enablement round, so such a round counts as doing both. + enablement = bool(params.get("enablement")) + if not proposal.changes_beyond_patches(setup=enablement, runtime=enablement): + return await self._end_already_present(attempt, params, extra, materialized.already_present) if materialized.failure is not None: return { **materialized.failure, @@ -2319,69 +2400,33 @@ async def _stage_apply( explicit_patches=(list(explicit_patches) if isinstance(explicit_patches, list) else None), done_payload=done_payload, ) - config_changes = dict(params.get("config_changes") or {}) - if not config_changes and done_payload: - cc = done_payload.get("config_changes") - if isinstance(cc, dict): - config_changes = {str(k): str(v) for k, v in cc.items()} - proposal_extra_args_raw = params.get("extra_server_args") - proposal_extra_args = ( - proposal_extra_args_raw - if isinstance(proposal_extra_args_raw, str) and proposal_extra_args_raw.strip() - else "" + proposal = _read_round_proposal( + params, done_payload, workspace=specialist_workspace, delivers_code=bool(patch_paths) ) - proposal_extra_envs = dict(config_changes) - raw_extra_envs = params.get("extra_envs") - if isinstance(raw_extra_envs, dict): - proposal_extra_envs.update({str(k): str(v) for k, v in raw_extra_envs.items()}) - proposal_extra_envs, _dropped = filter_untrusted_env_mapping( - proposal_extra_envs, - allow_predicate=is_allowed_variant_env_key, - ) - dropped_env_overrides = sorted(_dropped) - if dropped_env_overrides: + if proposal.dropped_env_overrides: log.warning( "integrate_patch: dropping unsafe env override keys: %s", - ", ".join(dropped_env_overrides), + ", ".join(proposal.dropped_env_overrides), ) # Framework-rewrite switches. Every rewrite in such a patch sits behind # a switch that defaults OFF, so the applied patch is inert and benching - # it as-is would measure the baseline. Turn the switches on for the - # measurement and carry the parsed manifest to the gate, which needs the - # dependency edges to decide between a throughput KEEP and an inert one. - switch_manifest, switch_problems = _parse_framework_switches( - params=params, - done_payload=done_payload, - ) - if switch_manifest and not patch_paths: - # A manifest without a patch describes switches that gate code which - # was never delivered. Setting them would be a no-op, and registering - # them as levers would leave the ledger pointing at absent code. - switch_problems.append( - f"discarded {len(switch_manifest)} switch(es): the deliverable carries no patch, " - f"so there is no rewrite for them to gate" - ) - switch_manifest = [] + # it as-is would measure the baseline. The proposal turns the switches on + # for the measurement, and the parsed manifest goes to the gate, which needs + # the dependency edges to decide between a throughput KEEP and an inert one. + switch_manifest = proposal.switch_manifest if switch_manifest: - proposal_extra_envs.update(_switch_manifest.switch_env(switch_manifest)) log.info( "integrate_patch: benching with %d framework rewrite switch(es) on\n%s", len(switch_manifest), - _switch_manifest.summarize(switch_manifest, switch_problems), + _switch_manifest.summarize(switch_manifest, proposal.switch_problems), ) - elif switch_problems: + elif proposal.switch_problems: log.warning( "integrate_patch: framework switch manifest unusable\n%s", - _switch_manifest.summarize(switch_manifest, switch_problems), + _switch_manifest.summarize(switch_manifest, proposal.switch_problems), ) attempt.switch_manifest = switch_manifest - - explicit_artifacts = params.get("artifacts") - artifact_specs, artifact_resolve_errors = _resolve_artifact_specs( - specialist_workspace=specialist_workspace, - explicit_artifacts=(list(explicit_artifacts) if isinstance(explicit_artifacts, list) else None), - done_payload=done_payload, - ) + artifact_specs = proposal.artifact_specs target_gpu_type = str( getattr(shared_state, "gpu_type", "") or params.get("gpu_type") or params.get("target_platform") or "" ).strip() @@ -2412,12 +2457,11 @@ async def _stage_apply( ), } - _setup_ran = bool(attempt.setup_commands_applied) # A runtime this round acquired changes what the next boot executes; a # re-provisioned kept one is the stack the last KEEP was already graded on. - acquired_runtime = bool(attempt.attempt_venv_root) and not attempt.shared_state.enablement.kept_stack_action - changes_beyond_patches = bool( - proposal_extra_args or proposal_extra_envs or artifact_specs or _setup_ran or acquired_runtime + changes_beyond_patches = proposal.changes_beyond_patches( + setup=bool(attempt.setup_commands_applied), + runtime=bool(attempt.attempt_venv_root) and not attempt.shared_state.enablement.kept_stack_action, ) if not patch_paths and not changes_beyond_patches: # Launch-only mode: skip the no-patches early-return and fall through to bench. @@ -2434,7 +2478,7 @@ async def _stage_apply( "patches_applied": [], "patches_reverted": [], "artifacts_applied": [], - "artifact_errors": artifact_resolve_errors, + "artifact_errors": proposal.artifact_errors, "reason": ( "neither patches, config_changes, installable artifacts, nor " "allowlisted setup commands were supplied / discoverable for " @@ -2583,9 +2627,9 @@ async def _stage_apply( "task_id": attempt.task_id, "patches": [str(p) for p in patch_paths], "artifacts": [{"target": str(s.target), "rel_target": s.rel_target} for s in artifact_specs], - "config_changes": dict(config_changes), - "extra_server_args": proposal_extra_args, - "extra_envs": dict(proposal_extra_envs), + "config_changes": dict(proposal.config_changes), + "extra_server_args": proposal.extra_server_args, + "extra_envs": dict(proposal.extra_envs), "framework_source_root": str(framework_root or ""), "workspace": str(output_root), "attempt_venv_root": attempt.attempt_venv_root, @@ -2657,7 +2701,7 @@ async def _stage_apply( return { "status": "apply_failed", "error_class": "artifact_backup_failed", - "error": artifact_resolve_errors + artifact_backup_errors, + "error": proposal.artifact_errors + artifact_backup_errors, "specialist_task_id": specialist_task_id, "patches_applied": [], "patches_reverted": [], @@ -2721,7 +2765,7 @@ async def _stage_apply( return { "status": "apply_failed", "error_class": "artifact_install_failed", - "error": artifact_resolve_errors + artifact_apply_errors, + "error": proposal.artifact_errors + artifact_apply_errors, "specialist_task_id": specialist_task_id, "patches_applied": [], "patches_reverted": [], @@ -2730,8 +2774,8 @@ async def _stage_apply( } # Inherited first, this round last: inheriting is not pinning. - extra_server_args_applied = _merge_established_server_args(inherited_args, proposal_extra_args) - extra_envs_applied = {**inherited_envs, **proposal_extra_envs} + extra_server_args_applied = _merge_established_server_args(inherited_args, proposal.extra_server_args) + extra_envs_applied = {**inherited_envs, **proposal.extra_envs} if params.get("apply_only"): return { @@ -2742,7 +2786,7 @@ async def _stage_apply( "artifacts_applied": applied_artifacts, "extra_server_args_applied": extra_server_args_applied, "extra_envs_applied": extra_envs_applied, - "dropped_env_overrides": dropped_env_overrides, + "dropped_env_overrides": proposal.dropped_env_overrides, "reason": "apply_only=True; benchmark skipped", "workspace": str(output_root), } @@ -2750,7 +2794,7 @@ async def _stage_apply( attempt.output_root = output_root attempt.extra_envs_applied = extra_envs_applied attempt.extra_server_args_applied = extra_server_args_applied - attempt.dropped_env_overrides = dropped_env_overrides + attempt.dropped_env_overrides = proposal.dropped_env_overrides return None async def _end_already_present( diff --git a/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py b/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py index 26ea0a07a1..22575ad2c1 100644 --- a/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py +++ b/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py @@ -517,6 +517,33 @@ def no_fetch(*_args, **_kwargs): assert [(row["outcome"], row["pr_url"]) for row in rows] == [("already_present", _PR_URL)] +@pytest.mark.asyncio +@pytest.mark.parametrize( + "change", + [ + pytest.param({"extra_server_args": "--max-num-seqs 64"}, id="extra_server_args"), + pytest.param({"config_changes": {"VLLM_ROCM_USE_AITER": "1"}}, id="config_changes"), + pytest.param({"extra_envs": {"VLLM_ROCM_USE_AITER": "1"}}, id="extra_envs"), + pytest.param({"artifacts": [{"source": "tuned.json", "target": "mod/tuned.json"}]}, id="artifacts"), + pytest.param({"enablement": True, "enablement_setup_commands": ["pip install example-pkg==1.0"]}, id="setup"), + ], +) +async def test_executor_fails_a_merged_pr_head_whose_round_changes_more(tmp_path, executor_env, monkeypatch, change): + """A merged head ends the round only when nothing else would change; otherwise it is the checkout-head failure.""" + executor, kb_root = executor_env + repo, head = _repo_with_merged_pr_head(tmp_path, revert=False) + _mkdir(runs_dir(executor.session_dir, "integrate_patch", "t-more")).joinpath("tuned.json").write_text("{}") + monkeypatch.setattr(ip, "resolve_kernel_search_roots", lambda: [str(repo)]) + + result = await executor(_make_ctx("t-more", {**_pr_head_round(repo, head), **change}, _approved_state())) + + assert result["status"] == "fetch_failed", result + assert result["error_class"] == "checkout_head_failed" + assert "empty diff" in result["error"] + assert "already_present" not in result + assert read_pr_ledger(kb_root=kb_root) == [] + + def _serve_superset(monkeypatch) -> None: """Make every ``diff_url`` fetch return the PR's diff.""" from hyperloom.orchestrator.actions.executors import _patch_source_pr @@ -543,6 +570,25 @@ async def test_executor_ends_a_merged_pr_fetched_by_diff_url_as_already_present( assert [row["outcome"] for row in read_pr_ledger(kb_root=kb_root)] == ["already_present"] +@pytest.mark.asyncio +async def test_executor_benchmarks_a_merged_pr_fetched_by_diff_url_that_changes_server_args( + tmp_path, executor_env, monkeypatch +): + """The diff the tree already holds does not end a round whose server args still need measuring.""" + _serve_superset(monkeypatch) + reached = _record_bench(monkeypatch) + executor, kb_root = executor_env + repo, head = _repo_with_merged_pr_head(tmp_path, revert=False) + params = _pr_head_round(repo, head, diff_url="https://example.invalid/pr.diff", prefer_checkout=True) + params["extra_server_args"] = "--max-num-seqs 64" + + result = await executor(_make_ctx("t-merged-args", params, _approved_state())) + + assert reached == ["t-merged-args"] + assert result["status"] != "already_present" + assert "already_present" not in [row["outcome"] for row in read_pr_ledger(kb_root=kb_root)] + + @pytest.mark.asyncio async def test_executor_benchmarks_a_reverted_pr_head(tmp_path, executor_env, monkeypatch): """A merged-then-reverted PR is still in the history but not in the tree, so it is applied and measured.""" From e88a36b83a8e5060a71091238a18bbb4ca15c0f2 Mon Sep 17 00:00:00 2001 From: okorzh Date: Wed, 14 Oct 2026 20:59:47 -0500 Subject: [PATCH 04/11] fix(_nogit_patch): anchor the reverse probe on the preflight's patch parser The probe read its paths with parse_patch_manifest and probed every -p level when that parser raised. A git format-patch file raises there (the mail header is a section with no target), so a drifted deletion in mail format passed the reverse check at a level where the stripped path is absent and was reported present with the file untouched. Read the paths from patch_file_targets, the parser the target preflight uses, and drop the ValueError fallback. The drifted-deletion test gains a format-patch case; the test that pinned the fallback goes. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../actions/executors/_nogit_patch.py | 15 ++++---- .../tests/test_patch_already_applied_unit.py | 34 ++++++------------- 2 files changed, 17 insertions(+), 32 deletions(-) diff --git a/src/hyperloom/orchestrator/actions/executors/_nogit_patch.py b/src/hyperloom/orchestrator/actions/executors/_nogit_patch.py index 317212522f..194a8b4468 100644 --- a/src/hyperloom/orchestrator/actions/executors/_nogit_patch.py +++ b/src/hyperloom/orchestrator/actions/executors/_nogit_patch.py @@ -17,7 +17,7 @@ from ...delivery.ledger import append_record, mark_prepared from ...delivery import file_digest as _file_digest -from ...specialists.patch_safety import path_resolves_at +from ...specialists.patch_safety import patch_file_targets, path_resolves_at from ._apply_feedback import ApplyFeedback, PatchApplyResult, PresenceCheck, read_patch_source_context log = logging.getLogger(__name__) @@ -94,7 +94,8 @@ def _reverse_probe_levels(framework_root: Path, patch_path: Path) -> tuple[int, Undoing a file deletion creates the file, which succeeds at any level where the stripped path happens to be absent, so a level counts only when some - file the patch names resolves at it. A deletion already carried out leaves + file the patch names resolves at it, read from the ``---``/``+++`` headers + as the target preflight reads them. A deletion already carried out leaves nothing to resolve, and then every level counts. Args: @@ -102,16 +103,14 @@ def _reverse_probe_levels(framework_root: Path, patch_path: Path) -> tuple[int, patch_path: The unified-diff patch file to probe. Returns: - The levels to probe, in priority order: every level when the manifest - cannot be parsed, and none when the patch cannot be read. + The levels to probe, in priority order; none when the patch cannot be read. """ try: - manifest = parse_patch_manifest(patch_path.read_text(encoding="utf-8", errors=_DECODE_ERRORS)) + pairs = patch_file_targets(patch_path.read_text(encoding="utf-8", errors=_DECODE_ERRORS)) except OSError: return () - except ValueError: - return _P_LEVELS - anchored = tuple(lvl for lvl in _P_LEVELS if any(path_resolves_at(framework_root, t.path, lvl) for t in manifest)) + paths = {path for pair in pairs for path in pair if path != _PATCH_DEV_NULL} + anchored = tuple(lvl for lvl in _P_LEVELS if any(path_resolves_at(framework_root, p, lvl) for p in paths)) return anchored or _P_LEVELS diff --git a/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py b/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py index 22575ad2c1..bed906d02d 100644 --- a/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py +++ b/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py @@ -286,13 +286,21 @@ def test_patch_set_the_tree_already_holds_changes_nothing(tmp_path, git_tree, ch assert outcome.already_present == [{"patch": str(p), "check": check.value} for p in (sup, sub)] +# What ``git format-patch`` puts before the diff. +_MAIL_HEADER = ( + "From 0 Mon Sep 17 00:00:00 2001\nFrom: t \nSubject: [PATCH] delete alpha\n\n---\n" + " mod/alpha.py | 3 ---\n 1 file changed, 3 deletions(-)\n\n" +) + + +@pytest.mark.parametrize("header", ["", _MAIL_HEADER], ids=["diff", "format_patch"]) @pytest.mark.parametrize("git_tree", [True, False]) -def test_deletion_of_a_drifted_file_is_a_failure_not_present(tmp_path, git_tree): +def test_deletion_of_a_drifted_file_is_a_failure_not_present(tmp_path, git_tree, header): """The file the patch deletes is still there with other content, so undoing the deletion cannot prove presence.""" src = _channel_tree(tmp_path, git_tree) (src / "mod" / "alpha.py").write_text("header\ndrifted\nfooter\n", encoding="utf-8") _settle(src, git_tree, "drift") - patch = _patch(tmp_path, "delete.patch", _DELETE_ALPHA) + patch = _patch(tmp_path, "delete.patch", header + _DELETE_ALPHA) result = _apply_one(git_tree, src, patch, tmp_path / "bak") @@ -316,28 +324,6 @@ def test_deletion_already_carried_out_is_present(tmp_path, git_tree, check): assert result.present_by is check -# Shapes ``git apply`` and ``patch`` accept whose leading block names no file, so the manifest cannot be parsed. -_FORMAT_PATCH = ( - "From 0 Mon Sep 17 00:00:00 2001\nSubject: [PATCH] alpha\n\n---\ndiff --git a/mod/alpha.py b/mod/alpha.py\n" -) -_DIFF_RUN = "diff -ruN a/mod/alpha.py b/mod/alpha.py\n" - - -@pytest.mark.parametrize("preamble", [_FORMAT_PATCH, _DIFF_RUN], ids=["format_patch", "diff_ruN"]) -@pytest.mark.parametrize(("git_tree", "check"), _CHANNEL_CHECKS) -def test_patch_with_unparseable_manifest_is_probed_at_every_level(tmp_path, git_tree, check, preamble): - """A patch the manifest parser rejects is still reported present when the tree holds it.""" - src = _channel_tree(tmp_path, git_tree) - (src / "mod" / "alpha.py").write_text("header\nnew_alpha\nfooter\n", encoding="utf-8") - _settle(src, git_tree, "already merged") - patch = _patch(tmp_path, "mail.patch", preamble + _SUBSET) - - result = _apply_one(git_tree, src, patch, tmp_path / "bak") - - assert result.ok, result.error - assert result.present_by is check - - def _make_ctx(task_id: str, params: dict, state: SharedState) -> RunnerContext: task = Task( task_id=task_id, From 57f30d849400e6c3599839953203d8dc35d257c8 Mon Sep 17 00:00:00 2001 From: okorzh Date: Wed, 14 Oct 2026 21:01:58 -0500 Subject: [PATCH 05/11] refactor(integrate_patch): drop the merged-PR-head already_present exit An empty merge-base..head diff already ended fetch_failed with no boot when the candidate had no diff_url, and with one the round falls back to the diff, whose reverse check in apply_patch_set decides. The exit only relabelled fetch_failed as already_present, at the cost of a third PresenceCheck member, a head-files probe over discovery's changed_files, a second result field on PrMaterialization, an async upstream-PR stage and a second "changes beyond patches" decision read from params. Revert _patch_source_pr.py and _stage_resolve_upstream_pr to main, drop PresenceCheck.PR_HEAD_FILES, _materialize_upstream_pr, _RoundProposal and _read_round_proposal, and name the existing no-patches predicate once in _stage_apply as changes_beyond_patches, where both its uses read it. The exit's tests and doc sentences go with it. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/conceptual/optimization-loop.md | 8 +- src/hyperloom/inference_optimizer/SKILL.md | 8 +- .../test_framework_agent_selection_audit.py | 24 +- .../tests/test_patch_source_pr.py | 100 +------- .../actions/executors/_apply_feedback.py | 2 - .../actions/executors/_patch_source_pr.py | 121 +++------ .../actions/executors/integrate_patch.py | 231 +++++++----------- .../test_integrate_patch_helpers_unit.py | 5 +- .../tests/test_patch_already_applied_unit.py | 49 ---- 9 files changed, 146 insertions(+), 402 deletions(-) diff --git a/docs/conceptual/optimization-loop.md b/docs/conceptual/optimization-loop.md index 674697ef90..c3fed13f18 100644 --- a/docs/conceptual/optimization-loop.md +++ b/docs/conceptual/optimization-loop.md @@ -369,11 +369,9 @@ One phase, two levers worked in parallel: a specialist wrote, `upstream_pr` for a diff fetched from an upstream PR that a `candidate_discovery_specialist` found, ranked and judged. Both serialise on the `workspace_mutation` lane: one landing at a time, - independent of how fast proposals arrive. A candidate the tree already - holds — every patch reverse-applies cleanly, or the PR head is in the live - history and the tree still has its copy of every file the PR changes — and - that changes nothing else is not benchmarked: it ends `already_present` and - is recorded so in the framework KB. + independent of how fast proposals arrive. A candidate whose every patch + reverse-applies cleanly, and that changes nothing else, is not benchmarked: + it ends `already_present` and is recorded so in the framework KB. `specialist` serves both levers — investigation, patch authoring, and candidate discovery are all dispatches of the one specialist action. An diff --git a/src/hyperloom/inference_optimizer/SKILL.md b/src/hyperloom/inference_optimizer/SKILL.md index 709355494a..770839936d 100644 --- a/src/hyperloom/inference_optimizer/SKILL.md +++ b/src/hyperloom/inference_optimizer/SKILL.md @@ -447,12 +447,8 @@ it came from. KEEP commits to the live tree so the next candidate stacks on top; REVERT does `git reset --hard`. A candidate the tree already holds is not benchmarked: when every patch in the set reverse-applies cleanly and the round changes nothing else, it ends `already_present`, with that evidence on the -result and an `already_present` row in the framework KB. An upstream PR whose -head is already in the live history ends the same way when the round changes -nothing else and the tree still has the head's copy of every file the PR -changes; otherwise it ends `fetch_failed`. With a `diff_url`, the fetched -diff's reverse check decides instead, so a reverted PR is benchmarked. -Resume skips completed candidates by idempotency key. +result and an `already_present` row in the framework KB. Resume skips +completed candidates by idempotency key. The phase exits when both arms are dry, when its budget is spent, or at the absolute per-phase cap. diff --git a/src/hyperloom/inference_optimizer/tests/test_framework_agent_selection_audit.py b/src/hyperloom/inference_optimizer/tests/test_framework_agent_selection_audit.py index 07ddb91b71..5ec3925da8 100644 --- a/src/hyperloom/inference_optimizer/tests/test_framework_agent_selection_audit.py +++ b/src/hyperloom/inference_optimizer/tests/test_framework_agent_selection_audit.py @@ -231,51 +231,51 @@ def _fake(args, timeout=None): def test_materialize_pr_diff_success(monkeypatch, tmp_path) -> None: monkeypatch.setattr(fpr_mod, "_run_git", _scripted_run_git()) dest = tmp_path / "out" / "cand.patch" - out = fpr_mod._materialize_pr_diff_from_head(tmp_path / "root", {"pr_number": 1015}, dest, timeout_sec=30.0) - assert out.failure is None and out.patches == [dest.resolve()] + ok, err = fpr_mod._materialize_pr_diff_from_head(tmp_path / "root", {"pr_number": 1015}, dest, timeout_sec=30.0) + assert ok is True and err == "" assert dest.read_text().startswith("diff --git") def test_materialize_pr_diff_fetch_fails(monkeypatch, tmp_path) -> None: monkeypatch.setattr(fpr_mod, "_run_git", _scripted_run_git(fetch_ok=False)) - out = fpr_mod._materialize_pr_diff_from_head( + ok, err = fpr_mod._materialize_pr_diff_from_head( tmp_path / "root", {"ref": "refs/pull/1/head"}, tmp_path / "c.patch", timeout_sec=30.0 ) - assert out.failure is not None and "git fetch" in out.failure["error"] + assert ok is False and "git fetch" in err def test_materialize_pr_diff_empty_diff(monkeypatch, tmp_path) -> None: monkeypatch.setattr(fpr_mod, "_run_git", _scripted_run_git(diff_text=" \n")) - out = fpr_mod._materialize_pr_diff_from_head( + ok, err = fpr_mod._materialize_pr_diff_from_head( tmp_path / "root", {"head_sha": "deadbeef"}, tmp_path / "c.patch", timeout_sec=30.0 ) - assert out.failure is not None and "empty diff" in out.failure["error"] + assert ok is False and "empty diff" in err def test_materialize_pr_diff_no_head_resolvable(monkeypatch, tmp_path) -> None: monkeypatch.setattr(fpr_mod, "_run_git", _scripted_run_git()) - out = fpr_mod._materialize_pr_diff_from_head(tmp_path / "root", {}, tmp_path / "c.patch", timeout_sec=30.0) - assert out.failure is not None and "cannot resolve PR head" in out.failure["error"] + ok, err = fpr_mod._materialize_pr_diff_from_head(tmp_path / "root", {}, tmp_path / "c.patch", timeout_sec=30.0) + assert ok is False and "cannot resolve PR head" in err def test_materialize_pr_diff_checks_out_nothing(monkeypatch, tmp_path) -> None: """Both ends of the diff range are shas, so no tree has to be materialized.""" seen: list[str] = [] monkeypatch.setattr(fpr_mod, "_run_git", _scripted_run_git(seen=seen)) - out = fpr_mod._materialize_pr_diff_from_head( + ok, _err = fpr_mod._materialize_pr_diff_from_head( tmp_path / "root", {"head_sha": "deadbeef"}, tmp_path / "c.patch", timeout_sec=30.0 ) - assert out.failure is None + assert ok is True assert "worktree" not in seen def test_materialize_pr_diff_ignores_an_unusable_pr_number(monkeypatch, tmp_path) -> None: """The row reaches us from the KB and from LLM-authored proposals alike.""" monkeypatch.setattr(fpr_mod, "_run_git", _scripted_run_git()) - out = fpr_mod._materialize_pr_diff_from_head( + ok, err = fpr_mod._materialize_pr_diff_from_head( tmp_path / "root", {"pr_number": "not-a-number"}, tmp_path / "c.patch", timeout_sec=30.0 ) - assert out.failure is not None and "cannot resolve PR head" in out.failure["error"] + assert ok is False and "cannot resolve PR head" in err # Dispatch pause on a spent phase budget diff --git a/src/hyperloom/inference_optimizer/tests/test_patch_source_pr.py b/src/hyperloom/inference_optimizer/tests/test_patch_source_pr.py index 009e3502f2..a82230276d 100644 --- a/src/hyperloom/inference_optimizer/tests/test_patch_source_pr.py +++ b/src/hyperloom/inference_optimizer/tests/test_patch_source_pr.py @@ -10,7 +10,6 @@ from pathlib import Path from hyperloom.orchestrator.actions.executors._patch_source_pr import ( - PrMaterialization, _fetch_diff_to_path, _materialize_pr_diff_from_head, materialize_candidate_patches, @@ -93,8 +92,8 @@ def test_materialize_pr_diff_from_head_extracts_net_diff(tmp_path: Path): head_sha = _init_repo_with_pr_branch(repo, pr_ref="pr-head") dest = tmp_path / "out" / "pr.patch" cand = {"repo": "x/y", "pr_number": 7, "ref": "pr-head", "head_sha": head_sha} - out = _materialize_pr_diff_from_head(repo, cand, dest, timeout_sec=60.0) - assert out.failure is None, out.failure + ok, err = _materialize_pr_diff_from_head(repo, cand, dest, timeout_sec=60.0) + assert ok, err text = dest.read_text() assert "src.py" in text assert "return 2" in text @@ -106,98 +105,9 @@ def test_materialize_pr_diff_empty_when_no_ref(tmp_path: Path): repo = tmp_path / "framework" _init_repo_with_pr_branch(repo) dest = tmp_path / "pr.patch" - out = _materialize_pr_diff_from_head(repo, {"repo": "x/y"}, dest, timeout_sec=30.0) - assert out.failure is not None - assert "cannot resolve PR head" in out.failure["error"] - - -def _git(repo: Path, *args: str) -> None: - subprocess.run(["git", "-C", str(repo), *args], check=True, capture_output=True, env=_git_env()) - - -def _merge_pr_head(repo: Path, *, revert: bool) -> None: - """Merge ``pr-head`` into ``main`` with a merge commit, and optionally revert that merge.""" - (repo / "other.py").write_text("x = 1\n", encoding="utf-8") - _git(repo, "add", "other.py") - _git(repo, "commit", "-qm", "unrelated") - _git(repo, "merge", "-q", "--no-ff", "-m", "merge pr", "pr-head") - if revert: - _git(repo, "revert", "--no-edit", "-m", "1", "HEAD") - - -def _materialize_head(repo: Path, tmp_path: Path, head_sha: str, **extra: object) -> PrMaterialization: - return materialize_candidate_patches( - candidate={"repo": "x/y", "ref": "pr-head", "head_sha": head_sha, "prefer_checkout": True, **extra}, - params={}, - framework_root=repo, - output_root=tmp_path / "out", - slug="s", - diff_fetch_timeout_sec=30.0, - ) - - -def test_materialize_reports_a_merged_pr_head_as_already_present(tmp_path: Path): - """The tree has the head's copy of every file the PR changes: nothing to apply, and evidence that it is in. - - The empty-diff failure stays on the result for a round that changes more than the PR. - """ - repo = tmp_path / "framework" - head_sha = _init_repo_with_pr_branch(repo) - _merge_pr_head(repo, revert=False) - - out = _materialize_head(repo, tmp_path, head_sha, changed_files=["src.py"]) - - assert out.failure is not None and out.failure["error_class"] == "checkout_head_failed" - assert "empty diff" in out.failure["error"] - assert out.patches == [] - assert out.already_present == [{"diff_range": f"{head_sha}..{head_sha}", "check": "pr_head_files_in_tree"}] - - -def test_materialize_does_not_report_a_reverted_pr_head_as_already_present(tmp_path: Path): - """A reverted merge leaves the head in the history but not its change in the tree.""" - repo = tmp_path / "framework" - head_sha = _init_repo_with_pr_branch(repo) - _merge_pr_head(repo, revert=True) - - out = _materialize_head(repo, tmp_path, head_sha, changed_files=["src.py"]) - - assert out.already_present == [] - assert out.failure is not None and out.failure["status"] == "fetch_failed" - assert "empty diff" in out.failure["error"] - - -def test_materialize_needs_changed_files_that_exist_at_the_head(tmp_path: Path): - """A path that names nothing at the head would compare equal in any tree, so it proves nothing.""" - repo = tmp_path / "framework" - head_sha = _init_repo_with_pr_branch(repo) - _merge_pr_head(repo, revert=False) - - out = _materialize_head(repo, tmp_path, head_sha, changed_files=["python/src.py"]) - - assert out.already_present == [] - assert out.failure is not None - - -def test_materialize_falls_back_to_the_diff_url_when_the_head_diff_is_empty(tmp_path: Path, monkeypatch): - """With a diff_url the fetched diff goes to the apply, whose reverse check tells merged from reverted.""" - repo = tmp_path / "framework" - head_sha = _init_repo_with_pr_branch(repo) - _merge_pr_head(repo, revert=False) - - def fetch(_url, dest, *, timeout_sec): - dest.write_text(_VALID_PATCH, encoding="utf-8") - return True, "" - - monkeypatch.setattr("hyperloom.orchestrator.actions.executors._patch_source_pr._fetch_diff_to_path", fetch) - - out = _materialize_head( - repo, tmp_path, head_sha, changed_files=["src.py"], diff_url="https://example.invalid/pr.diff" - ) - - assert out.already_present == [] - assert out.failure is None - assert out.mode == "diff_url_fallback" - assert out.patches == [(tmp_path / "out" / "s.patch").resolve()] + ok, err = _materialize_pr_diff_from_head(repo, {"repo": "x/y"}, dest, timeout_sec=30.0) + assert not ok + assert "cannot resolve PR head" in err or "head" in err.lower() def test_materialize_explicit_patches_are_used_verbatim(tmp_path: Path): diff --git a/src/hyperloom/orchestrator/actions/executors/_apply_feedback.py b/src/hyperloom/orchestrator/actions/executors/_apply_feedback.py index 85e135e177..5c466cc634 100644 --- a/src/hyperloom/orchestrator/actions/executors/_apply_feedback.py +++ b/src/hyperloom/orchestrator/actions/executors/_apply_feedback.py @@ -23,8 +23,6 @@ class PresenceCheck(Enum): GIT_REVERSE_APPLY = "git_reverse_apply_check" #: ``patch -R --fuzz=0 --dry-run`` passes with no offset: the same proof without git. PATCH_REVERSE_DRY_RUN = "patch_reverse_dry_run" - #: The PR head is in the live history, and the tree still has the head's copy of every file the PR changes. - PR_HEAD_FILES = "pr_head_files_in_tree" @dataclass diff --git a/src/hyperloom/orchestrator/actions/executors/_patch_source_pr.py b/src/hyperloom/orchestrator/actions/executors/_patch_source_pr.py index ebcadefc51..661f575403 100644 --- a/src/hyperloom/orchestrator/actions/executors/_patch_source_pr.py +++ b/src/hyperloom/orchestrator/actions/executors/_patch_source_pr.py @@ -14,7 +14,6 @@ from hyperloom.common.url_safety import require_http_url -from ._apply_feedback import PresenceCheck from ._git import _git_head_sha, _run_git log = logging.getLogger(__name__) @@ -24,9 +23,6 @@ DEFAULT_DIFF_FETCH_TIMEOUT_SEC: float = 30.0 #: Cap on a downloaded unified-diff body (bytes). Larger payloads are refused. DEFAULT_DIFF_MAX_BYTES: int = 32 * 1024 * 1024 -#: Timeout for a local git query against the live tree (merge base, tree listing, file comparison). -_GIT_QUERY_TIMEOUT_SEC: float = 60.0 -_CHECKOUT_HEAD: str = "checkout_head" def _pr_number_of(candidate: dict[str, Any]) -> int: @@ -140,7 +136,7 @@ def _materialize_pr_diff_from_head( dest: Path, *, timeout_sec: float, -) -> PrMaterialization: +) -> tuple[bool, str]: """checkout-head (diff source) mode.""" dest.parent.mkdir(parents=True, exist_ok=True) root = str(framework_root) @@ -163,7 +159,7 @@ def _materialize_pr_diff_from_head( timeout=timeout_sec, ) if not ok: - return _checkout_head_failed(_CHECKOUT_HEAD, f"git fetch {fetch_ref!r} failed: {err}") + return False, f"git fetch {fetch_ref!r} failed: {err}" if not head_sha: # FETCH_HEAD now points at the fetched head. ok2, out2, err2 = _run_git( @@ -171,32 +167,32 @@ def _materialize_pr_diff_from_head( timeout=30.0, ) if not ok2 or not out2.strip(): - return _checkout_head_failed(_CHECKOUT_HEAD, f"could not resolve FETCH_HEAD: {err2}") + return False, f"could not resolve FETCH_HEAD: {err2}" head_sha = out2.strip() if not head_sha: - return _checkout_head_failed( - _CHECKOUT_HEAD, "checkout-head: no head_sha / ref / pr_number on candidate; cannot resolve PR head" - ) + return False, ("checkout-head: no head_sha / ref / pr_number on candidate; cannot resolve PR head") # Diff against the merge-base so applying introduces only the PR's own commits, not the full divergence from the # live HEAD. live_head = _git_head_sha(framework_root) merge_base = "" if live_head: - ok_mb, mb_out, _mb_e = _run_git(["-C", root, "merge-base", live_head, head_sha], timeout=_GIT_QUERY_TIMEOUT_SEC) + ok_mb, mb_out, _mb_e = _run_git(["-C", root, "merge-base", live_head, head_sha], timeout=60.0) if ok_mb and mb_out.strip(): merge_base = mb_out.strip() diff_range = f"{merge_base}..{head_sha}" if merge_base else head_sha ok_d, diff_out, err_d = _run_git(["-C", root, "diff", "--binary", diff_range], timeout=timeout_sec) if not ok_d: - return _checkout_head_failed(_CHECKOUT_HEAD, f"git diff {diff_range!r} failed: {err_d}") + return False, f"git diff {diff_range!r} failed: {err_d}" if not diff_out.strip(): - return _empty_head_diff(framework_root, candidate, head_sha, diff_range) + return False, ( + f"checkout-head produced an empty diff for range {diff_range!r} (PR head already merged into live tree?)" + ) try: dest.write_text(diff_out, encoding="utf-8") except OSError as exc: - return _checkout_head_failed(_CHECKOUT_HEAD, f"could not write diff to {dest}: {exc!r}") - return PrMaterialization(patches=[dest.resolve()], mode=_CHECKOUT_HEAD) + return False, f"could not write diff to {dest}: {exc!r}" + return True, "" @dataclass @@ -208,70 +204,9 @@ class PrMaterialization: #: Which source produced them: ``explicit`` / ``checkout_head`` / #: ``diff_url_fallback`` / ``diff_url``. mode: str = "" - #: A terminal result dict when no patch to apply could be produced. The + #: A terminal result dict when nothing applyable could be produced. The #: caller returns it as-is rather than benching an unpatched tree. failure: dict[str, Any] | None = None - #: Evidence that the live tree already holds every change the candidate - #: makes. :attr:`failure` is set with it, for a round that changes more - #: than the candidate and so cannot end on this evidence. - already_present: list[dict[str, str]] = field(default_factory=list) - - -def _checkout_head_failed(mode: str, err: str) -> PrMaterialization: - """The terminal result of a checkout-head extraction that produced no diff to apply.""" - return PrMaterialization( - mode=mode, - failure={ - "status": "fetch_failed", - "error_class": "checkout_head_failed", - "error": err, - "reason": f"checkout-head diff extraction failed: {err}", - }, - ) - - -def _empty_head_diff( - framework_root: Path, candidate: dict[str, Any], head_sha: str, diff_range: str -) -> PrMaterialization: - """Map a PR head that diffs empty against the live tree's merge base. - - That shows only that the head is in the live history, which stays true - after a revert. A candidate with a ``diff_url`` therefore falls back to it, - and the apply's reverse check decides. Without one, the tree holds the PR - only when it still has the head's copy of every file the PR changes, and - that evidence rides on the empty-diff failure rather than replacing it. - """ - empty = _checkout_head_failed( - _CHECKOUT_HEAD, - f"checkout-head produced an empty diff for range {diff_range!r} (PR head already merged into live tree?)", - ) - has_diff_url = bool(str(candidate.get("diff_url") or "").strip()) - if has_diff_url or not _tree_has_head_files(framework_root, head_sha, candidate.get("changed_files")): - return empty - log.info("upstream_pr: %s diffs empty and the tree has the head's files; the PR is already in it", diff_range) - empty.already_present.append({"diff_range": diff_range, "check": PresenceCheck.PR_HEAD_FILES.value}) - return empty - - -def _tree_has_head_files(framework_root: Path, head_sha: str, changed_files: Any) -> bool: - """True when the working tree matches ``head_sha`` on every path in ``changed_files``. - - Every path must exist at the head, so one that names nothing in either tree - cannot pass as unchanged. - """ - files = {str(f).strip() for f in changed_files if str(f).strip()} if isinstance(changed_files, list) else set() - if not files: - return False - root = str(framework_root) - ok_ls, listed, _err = _run_git( - ["-C", root, "ls-tree", "-z", "--name-only", head_sha, "--", *files], timeout=_GIT_QUERY_TIMEOUT_SEC - ) - if not ok_ls or set(filter(None, listed.split("\0"))) != files: - return False - ok_diff, _out, _err = _run_git( - ["-C", root, "diff", "--quiet", head_sha, "--", *files], timeout=_GIT_QUERY_TIMEOUT_SEC - ) - return ok_diff def materialize_candidate_patches( @@ -313,7 +248,7 @@ def materialize_candidate_patches( or str(candidate.get("ref") or "").strip() or bool(_pr_number_of(candidate)) ) - explicit_checkout = apply_mode in {_CHECKOUT_HEAD, "checkout-head", "checkout"} or prefer_checkout + explicit_checkout = apply_mode in {"checkout_head", "checkout-head", "checkout"} or prefer_checkout use_checkout_head = explicit_checkout or (not diff_url and has_checkout_ref) # Same-repo guard: checkout-head fetches from the live origin, so a cross-repo candidate would fetch the wrong # ref. @@ -334,15 +269,29 @@ def materialize_candidate_patches( dest = output_root / f"{slug}.patch" if use_checkout_head: - head = _materialize_pr_diff_from_head(framework_root, candidate, dest, timeout_sec=diff_fetch_timeout_sec * 4.0) - if head.failure is None or not diff_url: - return head - # A worktree/fetch hiccup must not strand a candidate that would apply. - log.warning("upstream_pr: checkout-head failed (%s); falling back to diff_url", head.failure["error"]) - ok, err = _fetch_diff_to_path(diff_url, dest, timeout_sec=diff_fetch_timeout_sec) + mode = "checkout_head" + ok, err = _materialize_pr_diff_from_head( + framework_root, + candidate, + dest, + timeout_sec=diff_fetch_timeout_sec * 4.0, + ) + if not ok and diff_url: + # A worktree/fetch hiccup must not strand an applyable candidate. + log.warning("upstream_pr: checkout-head failed (%s); falling back to diff_url", err) + mode = "diff_url_fallback" + ok, err = _fetch_diff_to_path(diff_url, dest, timeout_sec=diff_fetch_timeout_sec) if not ok: - return _checkout_head_failed("diff_url_fallback", err) - return PrMaterialization(patches=[dest.resolve()], mode="diff_url_fallback") + return PrMaterialization( + mode=mode, + failure={ + "status": "fetch_failed", + "error_class": "checkout_head_failed", + "error": err, + "reason": f"checkout-head diff extraction failed: {err}", + }, + ) + return PrMaterialization(patches=[dest.resolve()], mode=mode) ok, err = _fetch_diff_to_path(diff_url, dest, timeout_sec=diff_fetch_timeout_sec) if not ok: diff --git a/src/hyperloom/orchestrator/actions/executors/integrate_patch.py b/src/hyperloom/orchestrator/actions/executors/integrate_patch.py index 6a1defd0ab..7d707ffadd 100644 --- a/src/hyperloom/orchestrator/actions/executors/integrate_patch.py +++ b/src/hyperloom/orchestrator/actions/executors/integrate_patch.py @@ -95,13 +95,13 @@ _candidate_slug, materialize_candidate_patches, ) +from ._patch_set_apply import _run_git_apply, apply_patch_set from ._nogit_patch import ( _P_LEVELS, _is_git_tree, _is_within, ) from ...enablement.lane import bind_integrate_to_lane -from ._patch_set_apply import _run_git_apply, apply_patch_set from ...enablement.recipe.credentials import detect_credential_channels from ...enablement.recipe.projections import project_launch_evidence from ._patch_snapshot import _git_commit_kept, _patch_touched_paths @@ -1422,80 +1422,6 @@ def _resolve_artifact_specs( return specs, errors -@dataclass(frozen=True) -class _RoundProposal: - """What a round proposes besides its patches: launch config and installable artifacts.""" - - config_changes: dict[str, str] - extra_server_args: str - #: The proposed env that passed the safety filter, with the switch env on top. - extra_envs: dict[str, str] - dropped_env_overrides: list[str] - switch_manifest: list[dict[str, Any]] - switch_problems: list[str] - artifact_specs: list[_ArtifactSpec] - artifact_errors: list[dict[str, str]] - - def changes_beyond_patches(self, *, setup: bool, runtime: bool) -> bool: - """Whether there is something to measure even when every patch is already in the tree. - - ``setup`` and ``runtime`` say whether the round replays setup commands and boots a runtime it acquired. - """ - return bool(self.extra_server_args or self.extra_envs or self.artifact_specs or setup or runtime) - - -def _read_round_proposal( - params: dict[str, Any], - done_payload: dict[str, Any] | None, - *, - workspace: Path, - delivers_code: bool, -) -> _RoundProposal: - """Read a round's proposal from its params, then its specialist's done payload; nothing is applied or logged. - - An artifact source must resolve inside ``workspace``, and the framework-rewrite switches gate the code the - round carries, so they are discarded unless ``delivers_code``. - """ - config_changes = dict(params.get("config_changes") or {}) - if not config_changes and done_payload: - cc = done_payload.get("config_changes") - if isinstance(cc, dict): - config_changes = {str(k): str(v) for k, v in cc.items()} - extra_args = params.get("extra_server_args") - envs = dict(config_changes) - raw_extra_envs = params.get("extra_envs") - if isinstance(raw_extra_envs, dict): - envs.update({str(k): str(v) for k, v in raw_extra_envs.items()}) - envs, dropped = filter_untrusted_env_mapping(envs, allow_predicate=is_allowed_variant_env_key) - switch_manifest, switch_problems = _parse_framework_switches(params=params, done_payload=done_payload) - if switch_manifest and not delivers_code: - # A manifest without a patch describes switches that gate code which - # was never delivered. Setting them would be a no-op, and registering - # them as levers would leave the ledger pointing at absent code. - switch_problems.append( - f"discarded {len(switch_manifest)} switch(es): the deliverable carries no patch, " - f"so there is no rewrite for them to gate" - ) - switch_manifest = [] - envs.update(_switch_manifest.switch_env(switch_manifest)) - explicit_artifacts = params.get("artifacts") - artifact_specs, artifact_errors = _resolve_artifact_specs( - specialist_workspace=workspace, - explicit_artifacts=(list(explicit_artifacts) if isinstance(explicit_artifacts, list) else None), - done_payload=done_payload, - ) - return _RoundProposal( - config_changes=config_changes, - extra_server_args=extra_args if isinstance(extra_args, str) and extra_args.strip() else "", - extra_envs=envs, - dropped_env_overrides=sorted(dropped), - switch_manifest=switch_manifest, - switch_problems=switch_problems, - artifact_specs=artifact_specs, - artifact_errors=artifact_errors, - ) - - def _is_aiter_gemm_model_config(spec: _ArtifactSpec) -> bool: """Return whether an artifact is an AITER runtime GEMM model-config CSV.""" if spec.source.suffix.lower() != ".csv": @@ -1977,7 +1903,7 @@ async def _stage_resolve( # Upstream-PR mode has no specialist to look up and no specialist # verdict to enforce: the Critic verdict on this proposal is the gate. if resolve_patch_source(params) == PATCH_SOURCE_UPSTREAM_PR: - return await self._stage_resolve_upstream_pr(attempt, params, extra) + return self._stage_resolve_upstream_pr(attempt, params, shared_state) specialist_task_id = str(params.get("specialist_task_id") or "").strip() if not specialist_task_id: @@ -2027,11 +1953,11 @@ async def _stage_resolve( attempt.done_payload = done_payload return None - async def _stage_resolve_upstream_pr( + def _stage_resolve_upstream_pr( self, attempt: IntegrateAttempt, params: dict[str, Any], - extra: dict[str, Any], + shared_state: Any, ) -> dict[str, Any] | None: """Resolve an upstream-PR candidate into patches on the task's own scratch. @@ -2042,13 +1968,12 @@ async def _stage_resolve_upstream_pr( Args: attempt: The integration attempt receiving the resolved inputs. params: Task params, mutated with the resolved patch paths. - extra: The runner ``extra`` mapping, carrying SharedState when there is one. + shared_state: SharedState, or ``None``. Returns: A terminal result when the candidate has no permissive Critic - verdict, could not be materialised or is already in the tree, else ``None``. + verdict or could not be materialised, else ``None``. """ - shared_state = extra.get("shared_state") or extra.get("state") candidate = params.get("candidate") or {} if not isinstance(candidate, dict) or not candidate: return { @@ -2072,7 +1997,8 @@ async def _stage_resolve_upstream_pr( if critic_reject is not None: return critic_reject - scratch = runs_dir(self.session_dir, "integrate_patch", attempt.task_id) + task_id = attempt.task_id + scratch = runs_dir(self.session_dir, "integrate_patch", task_id) scratch.mkdir(parents=True, exist_ok=True) framework_root = _resolve_framework_root(str(params.get("framework_source_root") or "").strip() or None) if framework_root is None: @@ -2084,31 +2010,6 @@ async def _stage_resolve_upstream_pr( "patches_reverted": [], } - attempt.specialist_task_id = attempt.task_id - attempt.shared_state = shared_state - attempt.specialist_workspace = scratch - return await self._materialize_upstream_pr( - attempt, params, extra, framework_root=framework_root, scratch=scratch - ) - - async def _materialize_upstream_pr( - self, - attempt: IntegrateAttempt, - params: dict[str, Any], - extra: dict[str, Any], - *, - framework_root: Path, - scratch: Path, - ) -> dict[str, Any] | None: - """Fetch the candidate's diff into ``params['patches']``, or end the attempt when there is none to apply. - - A tree that already holds the PR ends it only when the round changes nothing else, as in :meth:`_stage_apply`. - - Returns: - A terminal result when no diff could be fetched or the tree already - holds the candidate, else ``None``. - """ - candidate = params["candidate"] materialized = materialize_candidate_patches( candidate=candidate, params=params, @@ -2117,13 +2018,6 @@ async def _materialize_upstream_pr( slug=_candidate_slug(candidate), diff_fetch_timeout_sec=DEFAULT_DIFF_FETCH_TIMEOUT_SEC, ) - if materialized.already_present: - proposal = _read_round_proposal(params, attempt.done_payload, workspace=scratch, delivers_code=True) - # Setup replay and runtime provisioning come after this point, and - # only in an enablement round, so such a round counts as doing both. - enablement = bool(params.get("enablement")) - if not proposal.changes_beyond_patches(setup=enablement, runtime=enablement): - return await self._end_already_present(attempt, params, extra, materialized.already_present) if materialized.failure is not None: return { **materialized.failure, @@ -2135,6 +2029,10 @@ async def _materialize_upstream_pr( } params["patches"] = [str(p) for p in materialized.patches] params["patch_source_mode"] = materialized.mode + + attempt.specialist_task_id = task_id + attempt.shared_state = shared_state + attempt.specialist_workspace = scratch return None async def _stage_provision_attempt_runtime( @@ -2400,33 +2298,69 @@ async def _stage_apply( explicit_patches=(list(explicit_patches) if isinstance(explicit_patches, list) else None), done_payload=done_payload, ) - proposal = _read_round_proposal( - params, done_payload, workspace=specialist_workspace, delivers_code=bool(patch_paths) + config_changes = dict(params.get("config_changes") or {}) + if not config_changes and done_payload: + cc = done_payload.get("config_changes") + if isinstance(cc, dict): + config_changes = {str(k): str(v) for k, v in cc.items()} + proposal_extra_args_raw = params.get("extra_server_args") + proposal_extra_args = ( + proposal_extra_args_raw + if isinstance(proposal_extra_args_raw, str) and proposal_extra_args_raw.strip() + else "" ) - if proposal.dropped_env_overrides: + proposal_extra_envs = dict(config_changes) + raw_extra_envs = params.get("extra_envs") + if isinstance(raw_extra_envs, dict): + proposal_extra_envs.update({str(k): str(v) for k, v in raw_extra_envs.items()}) + proposal_extra_envs, _dropped = filter_untrusted_env_mapping( + proposal_extra_envs, + allow_predicate=is_allowed_variant_env_key, + ) + dropped_env_overrides = sorted(_dropped) + if dropped_env_overrides: log.warning( "integrate_patch: dropping unsafe env override keys: %s", - ", ".join(proposal.dropped_env_overrides), + ", ".join(dropped_env_overrides), ) # Framework-rewrite switches. Every rewrite in such a patch sits behind # a switch that defaults OFF, so the applied patch is inert and benching - # it as-is would measure the baseline. The proposal turns the switches on - # for the measurement, and the parsed manifest goes to the gate, which needs - # the dependency edges to decide between a throughput KEEP and an inert one. - switch_manifest = proposal.switch_manifest + # it as-is would measure the baseline. Turn the switches on for the + # measurement and carry the parsed manifest to the gate, which needs the + # dependency edges to decide between a throughput KEEP and an inert one. + switch_manifest, switch_problems = _parse_framework_switches( + params=params, + done_payload=done_payload, + ) + if switch_manifest and not patch_paths: + # A manifest without a patch describes switches that gate code which + # was never delivered. Setting them would be a no-op, and registering + # them as levers would leave the ledger pointing at absent code. + switch_problems.append( + f"discarded {len(switch_manifest)} switch(es): the deliverable carries no patch, " + f"so there is no rewrite for them to gate" + ) + switch_manifest = [] if switch_manifest: + proposal_extra_envs.update(_switch_manifest.switch_env(switch_manifest)) log.info( "integrate_patch: benching with %d framework rewrite switch(es) on\n%s", len(switch_manifest), - _switch_manifest.summarize(switch_manifest, proposal.switch_problems), + _switch_manifest.summarize(switch_manifest, switch_problems), ) - elif proposal.switch_problems: + elif switch_problems: log.warning( "integrate_patch: framework switch manifest unusable\n%s", - _switch_manifest.summarize(switch_manifest, proposal.switch_problems), + _switch_manifest.summarize(switch_manifest, switch_problems), ) attempt.switch_manifest = switch_manifest - artifact_specs = proposal.artifact_specs + + explicit_artifacts = params.get("artifacts") + artifact_specs, artifact_resolve_errors = _resolve_artifact_specs( + specialist_workspace=specialist_workspace, + explicit_artifacts=(list(explicit_artifacts) if isinstance(explicit_artifacts, list) else None), + done_payload=done_payload, + ) target_gpu_type = str( getattr(shared_state, "gpu_type", "") or params.get("gpu_type") or params.get("target_platform") or "" ).strip() @@ -2459,9 +2393,13 @@ async def _stage_apply( # A runtime this round acquired changes what the next boot executes; a # re-provisioned kept one is the stack the last KEEP was already graded on. - changes_beyond_patches = proposal.changes_beyond_patches( - setup=bool(attempt.setup_commands_applied), - runtime=bool(attempt.attempt_venv_root) and not attempt.shared_state.enablement.kept_stack_action, + acquired_runtime = bool(attempt.attempt_venv_root) and not attempt.shared_state.enablement.kept_stack_action + changes_beyond_patches = bool( + proposal_extra_args + or proposal_extra_envs + or artifact_specs + or attempt.setup_commands_applied + or acquired_runtime ) if not patch_paths and not changes_beyond_patches: # Launch-only mode: skip the no-patches early-return and fall through to bench. @@ -2478,7 +2416,7 @@ async def _stage_apply( "patches_applied": [], "patches_reverted": [], "artifacts_applied": [], - "artifact_errors": proposal.artifact_errors, + "artifact_errors": artifact_resolve_errors, "reason": ( "neither patches, config_changes, installable artifacts, nor " "allowlisted setup commands were supplied / discoverable for " @@ -2627,9 +2565,9 @@ async def _stage_apply( "task_id": attempt.task_id, "patches": [str(p) for p in patch_paths], "artifacts": [{"target": str(s.target), "rel_target": s.rel_target} for s in artifact_specs], - "config_changes": dict(proposal.config_changes), - "extra_server_args": proposal.extra_server_args, - "extra_envs": dict(proposal.extra_envs), + "config_changes": dict(config_changes), + "extra_server_args": proposal_extra_args, + "extra_envs": dict(proposal_extra_envs), "framework_source_root": str(framework_root or ""), "workspace": str(output_root), "attempt_venv_root": attempt.attempt_venv_root, @@ -2701,7 +2639,7 @@ async def _stage_apply( return { "status": "apply_failed", "error_class": "artifact_backup_failed", - "error": proposal.artifact_errors + artifact_backup_errors, + "error": artifact_resolve_errors + artifact_backup_errors, "specialist_task_id": specialist_task_id, "patches_applied": [], "patches_reverted": [], @@ -2712,15 +2650,19 @@ async def _stage_apply( if shared_state is not None: shared_state.save(self.session_dir) + applied: list[Path] = [] applied_artifacts: list[dict[str, Any]] = [] + # ``applied`` is published by identity and appended to in place. + attempt.applied = applied attempt.applied_artifacts = applied_artifacts patch_set = apply_patch_set( framework_root, patch_paths, git_tree=bool(git_head), backup_root=recovery_root / "patch_backups", - applied=attempt.applied, + applied=applied, ) + if patch_set.errors: await self._maybe_write_framework_kb_record( params=params, @@ -2765,7 +2707,7 @@ async def _stage_apply( return { "status": "apply_failed", "error_class": "artifact_install_failed", - "error": proposal.artifact_errors + artifact_apply_errors, + "error": artifact_resolve_errors + artifact_apply_errors, "specialist_task_id": specialist_task_id, "patches_applied": [], "patches_reverted": [], @@ -2774,19 +2716,19 @@ async def _stage_apply( } # Inherited first, this round last: inheriting is not pinning. - extra_server_args_applied = _merge_established_server_args(inherited_args, proposal.extra_server_args) - extra_envs_applied = {**inherited_envs, **proposal.extra_envs} + extra_server_args_applied = _merge_established_server_args(inherited_args, proposal_extra_args) + extra_envs_applied = {**inherited_envs, **proposal_extra_envs} if params.get("apply_only"): return { "status": "applied_no_bench", "specialist_task_id": specialist_task_id, - "patches_applied": [str(p) for p in attempt.applied], + "patches_applied": [str(p) for p in applied], "patches_reverted": [], "artifacts_applied": applied_artifacts, "extra_server_args_applied": extra_server_args_applied, "extra_envs_applied": extra_envs_applied, - "dropped_env_overrides": proposal.dropped_env_overrides, + "dropped_env_overrides": dropped_env_overrides, "reason": "apply_only=True; benchmark skipped", "workspace": str(output_root), } @@ -2794,7 +2736,7 @@ async def _stage_apply( attempt.output_root = output_root attempt.extra_envs_applied = extra_envs_applied attempt.extra_server_args_applied = extra_server_args_applied - attempt.dropped_env_overrides = proposal.dropped_env_overrides + attempt.dropped_env_overrides = dropped_env_overrides return None async def _end_already_present( @@ -2804,13 +2746,13 @@ async def _end_already_present( extra: dict[str, Any], evidence: list[dict[str, str]], ) -> dict[str, Any]: - """End an attempt whose every change the tree already holds: there is nothing to benchmark. + """End an attempt whose every patch the tree already holds, and that changes nothing else: no benchmark. Args: attempt: The attempt; it applied nothing, so it reports nothing reverted. params: Task params, read for the KB record and the enablement lane. extra: The runner ``extra`` mapping, read for the KB record. - evidence: What showed each change to be present, and how. + evidence: One ``{patch, check}`` row per patch: the reverse check that found it present. Returns: The ``already_present`` result. @@ -4267,7 +4209,8 @@ async def _maybe_write_framework_kb_record( proposal = self._find_framework_proposal(done_payload) if proposal is None: # The upstream-PR lane carries the PR identity on the candidate - # rather than in a specialist's ``fa_*`` markers. + # rather than in a specialist's ``fa_*`` markers. Discovery dedups + # on this ledger, so a lane that writes none is re-benched forever. proposal = self._upstream_pr_kb_proposal(params or {}) if proposal is None: return diff --git a/src/hyperloom/orchestrator/actions/executors/tests/test_integrate_patch_helpers_unit.py b/src/hyperloom/orchestrator/actions/executors/tests/test_integrate_patch_helpers_unit.py index cde3d290c0..dce95e63e6 100644 --- a/src/hyperloom/orchestrator/actions/executors/tests/test_integrate_patch_helpers_unit.py +++ b/src/hyperloom/orchestrator/actions/executors/tests/test_integrate_patch_helpers_unit.py @@ -796,8 +796,7 @@ class _NoMethod: assert out["status"] == "rejected_by_critic" -@pytest.mark.asyncio -async def test_upstream_pr_lane_refuses_an_unreviewed_candidate(tmp_path: Path) -> None: +def test_upstream_pr_lane_refuses_an_unreviewed_candidate(tmp_path: Path) -> None: """The lane fetches a diff from a remote and applies it to the live tree.""" ex = ip.IntegratePatchExecutor(session_dir=tmp_path) attempt = ip.IntegrateAttempt(task_id="t-cand") @@ -806,7 +805,7 @@ async def test_upstream_pr_lane_refuses_an_unreviewed_candidate(tmp_path: Path) "framework_agent_candidate_id": "vllm-project/vllm#1015", } - out = await ex._stage_resolve_upstream_pr(attempt, params, {"shared_state": _Verdict("reject")}) + out = ex._stage_resolve_upstream_pr(attempt, params, _Verdict("reject")) assert out is not None assert out["status"] == "rejected_by_critic" diff --git a/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py b/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py index bed906d02d..67146a4cb1 100644 --- a/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py +++ b/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py @@ -474,7 +474,6 @@ def _pr_head_round(repo: Path, head: str, **candidate_extra: object) -> dict: "repo": "example-org/example-framework", "ref": "pr-head", "head_sha": head, - "changed_files": ["mod/alpha.py", "mod/beta.py"], **candidate_extra, }, "framework_agent_candidate_id": _PR_URL, @@ -482,54 +481,6 @@ def _pr_head_round(repo: Path, head: str, **candidate_extra: object) -> dict: } -@pytest.mark.asyncio -async def test_executor_ends_a_merged_pr_head_as_already_present_without_a_fetch(tmp_path, executor_env, monkeypatch): - """The head is merged and the tree has its copy of every file the PR changes: no fetch, no apply, no benchmark.""" - from hyperloom.orchestrator.actions.executors import _patch_source_pr - - def no_fetch(*_args, **_kwargs): - pytest.fail("a PR the live tree already holds needs no diff") - - monkeypatch.setattr(_patch_source_pr, "_fetch_diff_to_path", no_fetch) - executor, kb_root = executor_env - repo, head = _repo_with_merged_pr_head(tmp_path, revert=False) - - result = await executor(_make_ctx("t-merged", _pr_head_round(repo, head), _approved_state())) - - assert result["status"] == "already_present" - assert result["specialist_task_id"] == "t-merged" - assert result["already_present"] == [{"diff_range": f"{head}..{head}", "check": "pr_head_files_in_tree"}] - rows = read_pr_ledger(kb_root=kb_root) - assert [(row["outcome"], row["pr_url"]) for row in rows] == [("already_present", _PR_URL)] - - -@pytest.mark.asyncio -@pytest.mark.parametrize( - "change", - [ - pytest.param({"extra_server_args": "--max-num-seqs 64"}, id="extra_server_args"), - pytest.param({"config_changes": {"VLLM_ROCM_USE_AITER": "1"}}, id="config_changes"), - pytest.param({"extra_envs": {"VLLM_ROCM_USE_AITER": "1"}}, id="extra_envs"), - pytest.param({"artifacts": [{"source": "tuned.json", "target": "mod/tuned.json"}]}, id="artifacts"), - pytest.param({"enablement": True, "enablement_setup_commands": ["pip install example-pkg==1.0"]}, id="setup"), - ], -) -async def test_executor_fails_a_merged_pr_head_whose_round_changes_more(tmp_path, executor_env, monkeypatch, change): - """A merged head ends the round only when nothing else would change; otherwise it is the checkout-head failure.""" - executor, kb_root = executor_env - repo, head = _repo_with_merged_pr_head(tmp_path, revert=False) - _mkdir(runs_dir(executor.session_dir, "integrate_patch", "t-more")).joinpath("tuned.json").write_text("{}") - monkeypatch.setattr(ip, "resolve_kernel_search_roots", lambda: [str(repo)]) - - result = await executor(_make_ctx("t-more", {**_pr_head_round(repo, head), **change}, _approved_state())) - - assert result["status"] == "fetch_failed", result - assert result["error_class"] == "checkout_head_failed" - assert "empty diff" in result["error"] - assert "already_present" not in result - assert read_pr_ledger(kb_root=kb_root) == [] - - def _serve_superset(monkeypatch) -> None: """Make every ``diff_url`` fetch return the PR's diff.""" from hyperloom.orchestrator.actions.executors import _patch_source_pr From dba9af8b3c49bd3f267909777b57e3c3df7d271e Mon Sep 17 00:00:00 2001 From: okorzh Date: Wed, 14 Oct 2026 21:02:16 -0500 Subject: [PATCH 06/11] refactor(integrate_patch): record "nothing applied" once when a set is already present _end_already_present cleared attempt.applied and also wrote patches_reverted: [] into its result. _finish_attempt overwrites patches_reverted from attempt.applied for every non-KEEP status, so the literal was dead; the cleared list is the one encoding left. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../orchestrator/actions/executors/integrate_patch.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/hyperloom/orchestrator/actions/executors/integrate_patch.py b/src/hyperloom/orchestrator/actions/executors/integrate_patch.py index 7d707ffadd..35fba47a8e 100644 --- a/src/hyperloom/orchestrator/actions/executors/integrate_patch.py +++ b/src/hyperloom/orchestrator/actions/executors/integrate_patch.py @@ -2749,7 +2749,9 @@ async def _end_already_present( """End an attempt whose every patch the tree already holds, and that changes nothing else: no benchmark. Args: - attempt: The attempt; it applied nothing, so it reports nothing reverted. + attempt: The attempt. Its ``applied`` list is emptied, because + :meth:`_finish_attempt` reports that list as reverted, and + nothing was applied. params: Task params, read for the KB record and the enablement lane. extra: The runner ``extra`` mapping, read for the KB record. evidence: One ``{patch, check}`` row per patch: the reverse check that found it present. @@ -2773,7 +2775,6 @@ async def _end_already_present( "specialist_task_id": attempt.specialist_task_id, "already_present": evidence, "patches_applied": [], - "patches_reverted": [], "reason": "the framework tree already holds every change in this candidate, so there is nothing to measure", } if params.get("enablement"): From a47a74d0e82d317cdf3f263877e0b704d1f6f872 Mon Sep 17 00:00:00 2001 From: okorzh Date: Wed, 14 Oct 2026 21:03:15 -0500 Subject: [PATCH 07/11] test(integrate_patch): trim the already_present tests to the decisions they pin Drop the tests that repeat another one or freeze the current shape: the committed blob-id patch (the git superset/subset test already shows the reverse check), the one-no-op-in-a-set channel test (the executor's no-op-riding-with-a-real-change test covers it end to end), and the merged-PR diff_url test with server args (same decision as the all-noop set with server args). The channel-level all-present set test becomes a non-git parameter of the executor's already_present exit test. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../tests/test_patch_already_applied_unit.py | 104 ++++-------------- 1 file changed, 21 insertions(+), 83 deletions(-) diff --git a/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py b/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py index 67146a4cb1..300403722c 100644 --- a/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py +++ b/src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py @@ -210,40 +210,6 @@ def test_git_channel_unrelated_patch_still_fails(tmp_path): assert result.present_by is None -def test_git_channel_reports_a_committed_blob_id_patch_present(tmp_path): - """A committed diff carrying blob ids is reported present by the reverse check; the index stays untouched.""" - src = tmp_path / "src" - _tree(src) - _git_tree(src) - (src / "mod" / "alpha.py").write_text("header\nnew_alpha\nfooter\n", encoding="utf-8") - committed = _patch(tmp_path, "committed.patch", _git(src, "diff")) - assert "index " in committed.read_text(encoding="utf-8") - git_commit_all(src, "land the change") - - result = psa._git_apply_collect_feedback(src, committed) - - assert result.ok, result.error - assert result.present_by is PresenceCheck.GIT_REVERSE_APPLY - assert _git(src, "status", "--porcelain") == "" - - -def test_patch_set_with_one_subset_noop_still_changes_the_tree(tmp_path): - """One satisfied no-op inside a larger set lands with the set; the set is not ``already_present``.""" - src = tmp_path / "src" - _tree(src) - _git_tree(src) - sup = _patch(tmp_path, "superset.patch", _SUPERSET) - sub = _patch(tmp_path, "subset.patch", _SUBSET) - applied: list[Path] = [] - - outcome = psa.apply_patch_set(src, [sup, sub], git_tree=True, backup_root=tmp_path / "bak", applied=applied) - - assert outcome.errors == [] - assert outcome.changed_nothing is False - assert outcome.already_present == [{"patch": str(sub), "check": "git_reverse_apply_check"}] - assert applied == [sup, sub] - - _CHANNEL_CHECKS = [(True, PresenceCheck.GIT_REVERSE_APPLY), (False, PresenceCheck.PATCH_REVERSE_DRY_RUN)] @@ -269,23 +235,6 @@ def _settle(root: Path, git_tree: bool, message: str) -> None: git_commit_all(root, message) -@pytest.mark.parametrize(("git_tree", "check"), _CHANNEL_CHECKS) -def test_patch_set_the_tree_already_holds_changes_nothing(tmp_path, git_tree, check): - """Both channels report a set whose every patch is in the tree as having changed nothing.""" - src = _channel_tree(tmp_path, git_tree) - (src / "mod" / "alpha.py").write_text("header\nnew_alpha\nfooter\n", encoding="utf-8") - (src / "mod" / "beta.py").write_text("header\nnew_beta\nfooter\n", encoding="utf-8") - _settle(src, git_tree, "already merged") - sup = _patch(tmp_path, "superset.patch", _SUPERSET) - sub = _patch(tmp_path, "subset.patch", _SUBSET) - - outcome = psa.apply_patch_set(src, [sup, sub], git_tree=git_tree, backup_root=tmp_path / "bak", applied=[]) - - assert outcome.errors == [] - assert outcome.changed_nothing is True - assert outcome.already_present == [{"patch": str(p), "check": check.value} for p in (sup, sub)] - - # What ``git format-patch`` puts before the diff. _MAIL_HEADER = ( "From 0 Mon Sep 17 00:00:00 2001\nFrom: t \nSubject: [PATCH] delete alpha\n\n---\n" @@ -361,14 +310,12 @@ def no_bench(*_args, **_kwargs): return ip.IntegratePatchExecutor(session_dir=session_dir), kb_root -def _repo_holding_the_superset(tmp_path: Path) -> Path: - """A git checkout whose HEAD already carries every hunk of ``_SUPERSET``.""" - repo = tmp_path / "framework" - _tree(repo) - _git_tree(repo) +def _repo_holding_the_superset(tmp_path: Path, git_tree: bool = True) -> Path: + """A framework tree already carrying every hunk of ``_SUPERSET``: a git checkout at that HEAD when ``git_tree``.""" + repo = _channel_tree(tmp_path / "framework", git_tree) (repo / "mod" / "alpha.py").write_text("header\nnew_alpha\nfooter\n", encoding="utf-8") (repo / "mod" / "beta.py").write_text("header\nnew_beta\nfooter\n", encoding="utf-8") - git_commit_all(repo, "upstream already merged it") + _settle(repo, git_tree, "upstream already merged it") return repo @@ -408,12 +355,20 @@ def _mkdir(path: Path) -> Path: @pytest.mark.asyncio -@pytest.mark.parametrize("lane", ["upstream_pr", "specialist_authored"]) -async def test_executor_ends_an_all_noop_patch_set_as_already_present(tmp_path, executor_env, lane): +@pytest.mark.parametrize( + ("lane", "git_tree", "check"), + [ + ("upstream_pr", True, PresenceCheck.GIT_REVERSE_APPLY), + ("specialist_authored", True, PresenceCheck.GIT_REVERSE_APPLY), + ("upstream_pr", False, PresenceCheck.PATCH_REVERSE_DRY_RUN), + ], +) +async def test_executor_ends_an_all_noop_patch_set_as_already_present(tmp_path, executor_env, lane, git_tree, check): """Every patch already in the tree: no benchmark, an ``already_present`` result and one KB row saying so.""" executor, kb_root = executor_env - repo = _repo_holding_the_superset(tmp_path) - head = _git(repo, "rev-parse", "HEAD") + repo = _repo_holding_the_superset(tmp_path, git_tree) + before = {f: (repo / "mod" / f).read_text(encoding="utf-8") for f in ("alpha.py", "beta.py")} + head = _git(repo, "rev-parse", "HEAD") if git_tree else "" if lane == "upstream_pr": params, patches = _upstream_round(executor.session_dir, "t-present", repo) else: @@ -424,13 +379,15 @@ async def test_executor_ends_an_all_noop_patch_set_as_already_present(tmp_path, result = await executor(_make_ctx("t-present", params, state)) assert result["status"] == "already_present" - assert result["already_present"] == [{"patch": str(p), "check": "git_reverse_apply_check"} for p in patches] + assert result["already_present"] == [{"patch": str(p), "check": check.value} for p in patches] assert result["patches_applied"] == [] assert result["patches_reverted"] == [] rows = read_pr_ledger(kb_root=kb_root) assert [(row["outcome"], row["pr_url"]) for row in rows] == [("already_present", _PR_URL)] - assert _git(repo, "rev-parse", "HEAD") == head - assert _git(repo, "status", "--porcelain") == "" + assert {f: (repo / "mod" / f).read_text(encoding="utf-8") for f in before} == before + if git_tree: + assert _git(repo, "rev-parse", "HEAD") == head + assert _git(repo, "status", "--porcelain") == "" def _record_bench(monkeypatch) -> list[str]: @@ -507,25 +464,6 @@ async def test_executor_ends_a_merged_pr_fetched_by_diff_url_as_already_present( assert [row["outcome"] for row in read_pr_ledger(kb_root=kb_root)] == ["already_present"] -@pytest.mark.asyncio -async def test_executor_benchmarks_a_merged_pr_fetched_by_diff_url_that_changes_server_args( - tmp_path, executor_env, monkeypatch -): - """The diff the tree already holds does not end a round whose server args still need measuring.""" - _serve_superset(monkeypatch) - reached = _record_bench(monkeypatch) - executor, kb_root = executor_env - repo, head = _repo_with_merged_pr_head(tmp_path, revert=False) - params = _pr_head_round(repo, head, diff_url="https://example.invalid/pr.diff", prefer_checkout=True) - params["extra_server_args"] = "--max-num-seqs 64" - - result = await executor(_make_ctx("t-merged-args", params, _approved_state())) - - assert reached == ["t-merged-args"] - assert result["status"] != "already_present" - assert "already_present" not in [row["outcome"] for row in read_pr_ledger(kb_root=kb_root)] - - @pytest.mark.asyncio async def test_executor_benchmarks_a_reverted_pr_head(tmp_path, executor_env, monkeypatch): """A merged-then-reverted PR is still in the history but not in the tree, so it is applied and measured.""" From 50aff03a820f3755e0cd95b3108675ab30133060 Mon Sep 17 00:00:00 2001 From: okorzh Date: Wed, 14 Oct 2026 21:06:43 -0500 Subject: [PATCH 08/11] refactor(framework): leave phases/framework.py to its own changes The semantic_status mapping in _record_framework_agent_authoring_empty_outcome, its fixtures and tests, and the "unparseable" spelling fix in the same module are independent of ending a present patch set early, and move to a separate change. With them gone, adding already_present to the Critic priors set would be this change's only line in phases/framework.py, and the code-metrics gate refuses any growth of that module: main is at 3204 lines against a 3139-line baseline entry. The priors are review context only (candidate re-selection rides on the processed-candidate keys, which cover any keyed row), and they already leave out the other apply-stage ends, apply_failed and fetch_failed, so already_present stays out with them. The tested-outcome summary in framework/artifacts.py keeps counting it. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../tests/test_framework_agent_authoring.py | 11 ++++-- .../test_framework_agent_selection_audit.py | 38 ++++++++++++++++--- .../orchestrator/phases/framework.py | 21 ++++++++-- 3 files changed, 58 insertions(+), 12 deletions(-) diff --git a/src/hyperloom/inference_optimizer/tests/test_framework_agent_authoring.py b/src/hyperloom/inference_optimizer/tests/test_framework_agent_authoring.py index d82bd79246..f39c71921d 100644 --- a/src/hyperloom/inference_optimizer/tests/test_framework_agent_authoring.py +++ b/src/hyperloom/inference_optimizer/tests/test_framework_agent_authoring.py @@ -654,6 +654,7 @@ def test_empty_outcome_fires_when_patch_dropped_by_vetting(tmp_path: Path): "framework_agent_authoring": True, "framework_agent_candidate_id": cand, "framework_batch_id": "b1", + "framework_audit": {"semantic_status": "not_present"}, }, ) done_payload = { @@ -670,7 +671,7 @@ def test_empty_outcome_fires_when_patch_dropped_by_vetting(tmp_path: Path): rows = stub.shared_state.framework_agent_phase_progress assert len(rows) == 1 assert rows[0]["candidate_id"] == cand - assert rows[0]["status"] == "author_empty" + assert rows[0]["status"] == "not_applicable" assert rows[0]["kept"] is False @@ -772,6 +773,7 @@ def test_empty_outcome_skips_when_config_levers_present(tmp_path: Path): "framework_agent_authoring": True, "framework_agent_candidate_id": "https://github.com/ROCm/vllm/pull/1014", "framework_batch_id": "b1", + "framework_audit": {"semantic_status": "not_present"}, }, ) done_payload = { @@ -816,6 +818,7 @@ def test_empty_outcome_skips_when_artifacts_written_routable(tmp_path: Path): "framework_agent_authoring": True, "framework_agent_candidate_id": "https://github.com/ROCm/aiter/pull/4130", "framework_batch_id": "b1", + "framework_audit": {"semantic_status": "not_present"}, }, ) done_payload = { @@ -850,6 +853,7 @@ def test_empty_outcome_stamps_when_artifacts_source_missing(tmp_path: Path): "framework_agent_authoring": True, "framework_agent_candidate_id": "https://github.com/ROCm/aiter/pull/9999", "framework_batch_id": "b1", + "framework_audit": {"semantic_status": "not_present"}, }, ) done_payload = { @@ -871,7 +875,7 @@ def test_empty_outcome_stamps_when_artifacts_source_missing(tmp_path: Path): ) rows = stub.shared_state.framework_agent_phase_progress assert len(rows) == 1 - assert rows[0]["status"] == "author_empty" + assert rows[0]["status"] == "not_applicable" assert rows[0]["kept"] is False @@ -894,6 +898,7 @@ def test_empty_outcome_stamps_when_artifacts_source_outside_sandbox(tmp_path: Pa "framework_agent_authoring": True, "framework_agent_candidate_id": "https://github.com/ROCm/aiter/pull/8888", "framework_batch_id": "b1", + "framework_audit": {"semantic_status": "not_present"}, }, ) done_payload = { @@ -914,7 +919,7 @@ def test_empty_outcome_stamps_when_artifacts_source_outside_sandbox(tmp_path: Pa ) rows = stub.shared_state.framework_agent_phase_progress assert len(rows) == 1 - assert rows[0]["status"] == "author_empty" + assert rows[0]["status"] == "not_applicable" assert rows[0]["kept"] is False diff --git a/src/hyperloom/inference_optimizer/tests/test_framework_agent_selection_audit.py b/src/hyperloom/inference_optimizer/tests/test_framework_agent_selection_audit.py index 5ec3925da8..efd14239d8 100644 --- a/src/hyperloom/inference_optimizer/tests/test_framework_agent_selection_audit.py +++ b/src/hyperloom/inference_optimizer/tests/test_framework_agent_selection_audit.py @@ -121,11 +121,10 @@ def test_collect_framework_agent_candidate_priors(coord: Coordinator) -> None: {"candidate_id": "c1", "status": "kept", "gain_pct": 3.2}, {"candidate_id": "c2", "status": "in_flight"}, # non-terminal -> excluded {"candidate_id": "c3", "status": "critic_denied", "rationale": "off the bottleneck"}, - {"candidate_id": "c4", "status": "already_present"}, ] priors = coord.phase_framework._collect_framework_agent_candidate_priors() statuses = {o["status"] for o in priors["recent_outcomes"]} - assert statuses == {"kept", "critic_denied", "already_present"} + assert statuses == {"kept", "critic_denied"} # The denial reason has to reach the Critic, or the priors carry the verdict without the argument behind it. denied = next(o for o in priors["recent_outcomes"] if o["status"] == "critic_denied") assert denied["rationale"] == "off the bottleneck" @@ -550,7 +549,7 @@ def test_record_authoring_empty_guards(coord: Coordinator) -> None: assert not (coord.shared_state.framework_agent_phase_progress or []) -def test_record_authoring_empty_stamps_author_empty_once(coord: Coordinator) -> None: +def test_record_authoring_empty_already_present(coord: Coordinator) -> None: _enter_fpr(coord) task = types.SimpleNamespace( task_id="spec-2", @@ -558,14 +557,15 @@ def test_record_authoring_empty_stamps_author_empty_once(coord: Coordinator) -> "framework_agent_authoring": True, "framework_agent_candidate_id": "cand-2", "framework_batch_id": "b1", + "framework_audit": {"semantic_status": "already_equivalent"}, }, ) coord.phase_framework._record_framework_agent_authoring_empty_outcome( - task=task, done_payload={"payload": {"patches_written": [], "summary": "nothing to write"}} + task=task, done_payload={"payload": {"patches_written": [], "summary": "already there"}} ) row = coord.shared_state.framework_agent_phase_progress[-1] assert row["candidate_id"] == "cand-2" - assert row["status"] == "author_empty" + assert row["status"] == "already_present" # Idempotent: a second call does not append a duplicate. coord.phase_framework._record_framework_agent_authoring_empty_outcome( task=task, done_payload={"payload": {"patches_written": [], "summary": "again"}} @@ -573,4 +573,32 @@ def test_record_authoring_empty_stamps_author_empty_once(coord: Coordinator) -> assert sum(1 for p in coord.shared_state.framework_agent_phase_progress if p["candidate_id"] == "cand-2") == 1 +def test_record_authoring_empty_status_variants(coord: Coordinator) -> None: + _enter_fpr(coord) + coord.shared_state.framework_agent_phase_progress = [] + # not_present -> not_applicable. + coord.phase_framework._record_framework_agent_authoring_empty_outcome( + task=types.SimpleNamespace( + task_id="s-a", + params={ + "framework_agent_authoring": True, + "framework_agent_candidate_id": "na-1", + "framework_audit": {"semantic_status": "not_present"}, + }, + ), + done_payload={"patches_written": [], "summary": "missing"}, + ) + # no audit -> author_empty. + coord.phase_framework._record_framework_agent_authoring_empty_outcome( + task=types.SimpleNamespace( + task_id="s-b", + params={"framework_agent_authoring": True, "framework_agent_candidate_id": "ae-1"}, + ), + done_payload={"patches_written": []}, + ) + statuses = {p["candidate_id"]: p["status"] for p in coord.shared_state.framework_agent_phase_progress} + assert statuses["na-1"] == "not_applicable" + assert statuses["ae-1"] == "author_empty" + + # Lenient ranker: an "applicable: false" reply never vetoes the phase diff --git a/src/hyperloom/orchestrator/phases/framework.py b/src/hyperloom/orchestrator/phases/framework.py index a03ec45a39..ed2784b329 100644 --- a/src/hyperloom/orchestrator/phases/framework.py +++ b/src/hyperloom/orchestrator/phases/framework.py @@ -122,7 +122,7 @@ def _framework_config_levers_from_done( parsed_args = tokenize_server_args_preserving_json(" ".join(arg_tokens)) if parsed_args is None: log.warning( - "FRAMEWORK config lever %r has unparsable server args; dropping the args%s", + "FRAMEWORK config lever %r has unparseable server args; dropping the args%s", entry.get("name"), " while preserving its environment overrides" if extra_envs else "", ) @@ -1889,7 +1889,6 @@ def _collect_framework_agent_candidate_priors(self) -> dict[str, Any]: "kept", "kept_inert", "reverted", - "already_present", "no_patch", "enqueue_failed", FRAMEWORK_CRITIC_DENIED_STATUS, @@ -2577,11 +2576,20 @@ def _record_framework_agent_authoring_empty_outcome( if not cand_id: return batch_id = str(params.get("framework_batch_id") or "") + # Map the cached audit verdict to a terminal status. + audit = params.get("framework_audit") if isinstance(params.get("framework_audit"), dict) else {} + sem = str((audit or {}).get("semantic_status") or "").strip().lower() + if sem in ("already_equivalent", "already_superset"): + status = "already_present" + elif sem in ("not_present", "partially_present"): + status = "not_applicable" + else: + status = "author_empty" reason = str(inner.get("summary") or "").strip()[:500] recorded = self._stamp_framework_progress( candidate_id=cand_id, batch_id=batch_id, - status="author_empty", + status=status, kept=False, rationale=reason, provenance="authored_empty", @@ -2593,7 +2601,12 @@ def _record_framework_agent_authoring_empty_outcome( ) if not recorded: return - log.info("FRAMEWORK: authoring specialist empty deliverable candidate=%s batch=%s", cand_id, batch_id) + log.info( + "FRAMEWORK: authoring specialist empty deliverable candidate=%s batch=%s status=%s", + cand_id, + batch_id, + status, + ) async def _candidate_discovery_inflight(self) -> bool: """True while a candidate-discovery specialist is queued or running.""" From d2f7ea7dfd97f35b7245baa96a0d82f744f3cb29 Mon Sep 17 00:00:00 2001 From: okorzh Date: Wed, 14 Oct 2026 21:21:44 -0500 Subject: [PATCH 09/11] docs(integrate_patch): list already_present among _stage_apply's early exits Co-Authored-By: Claude Opus 5.5 (1M context) --- src/hyperloom/orchestrator/actions/executors/integrate_patch.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hyperloom/orchestrator/actions/executors/integrate_patch.py b/src/hyperloom/orchestrator/actions/executors/integrate_patch.py index 35fba47a8e..3797b1a8e4 100644 --- a/src/hyperloom/orchestrator/actions/executors/integrate_patch.py +++ b/src/hyperloom/orchestrator/actions/executors/integrate_patch.py @@ -2269,7 +2269,7 @@ async def _stage_apply( ) -> dict[str, Any] | None: """Setup replay, patch/artifact apply, pending_integrate sentinel. - Returns an early-exit result dict on failure/no-patches/apply_only, + Returns an early-exit result dict on failure/no-patches/already_present/apply_only, or None to continue to bench+gate. Records mutations on the attempt. """ specialist_task_id = attempt.specialist_task_id From a00f92a022c5bc8cdb5a23124850dc95801abc93 Mon Sep 17 00:00:00 2001 From: okorzh Date: Wed, 14 Oct 2026 23:12:51 -0500 Subject: [PATCH 10/11] docs(integrate_patch): say the already_present KB row needs a PR identity The framework KB writer records a candidate only when it carries an upstream PR identity (an upstream-PR URL or head SHA, or a framework proposal's fa_pr_url / fa_pr_sha). A specialist patch without one, or an enablement round, still ends already_present but writes no row, so the three places that describe the outcome now say when the row is written. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/conceptual/optimization-loop.md | 3 ++- src/hyperloom/inference_optimizer/SKILL.md | 4 ++-- src/hyperloom/inference_optimizer/actions/integrate_patch.md | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/docs/conceptual/optimization-loop.md b/docs/conceptual/optimization-loop.md index c3fed13f18..a430b1c6bb 100644 --- a/docs/conceptual/optimization-loop.md +++ b/docs/conceptual/optimization-loop.md @@ -371,7 +371,8 @@ One phase, two levers worked in parallel: serialise on the `workspace_mutation` lane: one landing at a time, independent of how fast proposals arrive. A candidate whose every patch reverse-applies cleanly, and that changes nothing else, is not benchmarked: - it ends `already_present` and is recorded so in the framework KB. + it ends `already_present`, and a candidate with an upstream PR identity is + recorded so in the framework KB. `specialist` serves both levers — investigation, patch authoring, and candidate discovery are all dispatches of the one specialist action. An diff --git a/src/hyperloom/inference_optimizer/SKILL.md b/src/hyperloom/inference_optimizer/SKILL.md index 770839936d..7c695412d9 100644 --- a/src/hyperloom/inference_optimizer/SKILL.md +++ b/src/hyperloom/inference_optimizer/SKILL.md @@ -447,8 +447,8 @@ it came from. KEEP commits to the live tree so the next candidate stacks on top; REVERT does `git reset --hard`. A candidate the tree already holds is not benchmarked: when every patch in the set reverse-applies cleanly and the round changes nothing else, it ends `already_present`, with that evidence on the -result and an `already_present` row in the framework KB. Resume skips -completed candidates by idempotency key. +result and, for a candidate with an upstream PR identity, an `already_present` +row in the framework KB. Resume skips completed candidates by idempotency key. The phase exits when both arms are dry, when its budget is spent, or at the absolute per-phase cap. diff --git a/src/hyperloom/inference_optimizer/actions/integrate_patch.md b/src/hyperloom/inference_optimizer/actions/integrate_patch.md index 478cc59244..f62e4c5b80 100644 --- a/src/hyperloom/inference_optimizer/actions/integrate_patch.md +++ b/src/hyperloom/inference_optimizer/actions/integrate_patch.md @@ -114,7 +114,7 @@ hash and `optimization_stack` is the authoritative ledger. | Symptom | Cause | Mitigation | |---|---|---| | `git apply` rejects | base HEAD drifted under specialist | reject patch, record `apply_failed`, REVERT path | -| every patch is already in the tree | the change landed upstream or in an earlier round | `already_present`, with the reverse check per patch under `already_present`; framework KB row `already_present` | +| every patch is already in the tree | the change landed upstream or in an earlier round | `already_present`, with the reverse check per patch under `already_present`; framework KB row `already_present` when the candidate carries an upstream PR identity | | server fails to start | broken patch | `git checkout` + restart prior config, mark REVERT | | accuracy drops >0.05 absolute | semantic regression | REVERT, record into pitfalls KB | | throughput unchanged within keep_threshold | no-op patch | REVERT, log low-confidence specialist for the gap | From 945b0cb3ea25c19a0216024ace1957caed56363e Mon Sep 17 00:00:00 2001 From: okorzh Date: Thu, 15 Oct 2026 14:43:03 -0500 Subject: [PATCH 11/11] chore(code-metrics): tighten the baseline to the units this change shrinks Co-Authored-By: Claude Opus 5.5 (1M context) --- scripts/code_metrics_baseline.txt | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/scripts/code_metrics_baseline.txt b/scripts/code_metrics_baseline.txt index 625537f99f..98da7c143a 100644 --- a/scripts/code_metrics_baseline.txt +++ b/scripts/code_metrics_baseline.txt @@ -76,7 +76,7 @@ cognitive-complexity src/hyperloom/orchestrator/actions/executors/integrate_patc cognitive-complexity src/hyperloom/orchestrator/actions/executors/integrate_patch.py::IntegratePatchExecutor._enablement_keep_records 38 cognitive-complexity src/hyperloom/orchestrator/actions/executors/integrate_patch.py::IntegratePatchExecutor._gate_enablement 31 cognitive-complexity src/hyperloom/orchestrator/actions/executors/integrate_patch.py::IntegratePatchExecutor._gate_perf 101 -cognitive-complexity src/hyperloom/orchestrator/actions/executors/integrate_patch.py::IntegratePatchExecutor._stage_apply 131 +cognitive-complexity src/hyperloom/orchestrator/actions/executors/integrate_patch.py::IntegratePatchExecutor._stage_apply 110 cognitive-complexity src/hyperloom/orchestrator/actions/executors/integrate_patch.py::_resolve_patch_paths 36 cognitive-complexity src/hyperloom/orchestrator/actions/executors/integrate_patch.py::restore_pending_integrate 83 cognitive-complexity src/hyperloom/orchestrator/actions/executors/profile.py::ProfileExecutor.__call__ 235 @@ -302,7 +302,7 @@ cyclomatic-complexity src/hyperloom/orchestrator/actions/executors/baseline.py:: cyclomatic-complexity src/hyperloom/orchestrator/actions/executors/benchmark_result.py::harvest_mn_gpu_metrics 30 cyclomatic-complexity src/hyperloom/orchestrator/actions/executors/explore.py::ExploreExecutor.__call__ 76 cyclomatic-complexity src/hyperloom/orchestrator/actions/executors/integrate_patch.py::IntegratePatchExecutor._gate_perf 29 -cyclomatic-complexity src/hyperloom/orchestrator/actions/executors/integrate_patch.py::IntegratePatchExecutor._stage_apply 53 +cyclomatic-complexity src/hyperloom/orchestrator/actions/executors/integrate_patch.py::IntegratePatchExecutor._stage_apply 45 cyclomatic-complexity src/hyperloom/orchestrator/actions/executors/integrate_patch.py::restore_pending_integrate 25 cyclomatic-complexity src/hyperloom/orchestrator/actions/executors/profile.py::ProfileExecutor.__call__ 66 cyclomatic-complexity src/hyperloom/orchestrator/actions/executors/profile.py::_validate_trace_structure 28 @@ -552,7 +552,7 @@ function-lines src/hyperloom/orchestrator/actions/executors/_multi_node_server_l function-lines src/hyperloom/orchestrator/actions/executors/_multi_node_server_lifecycle.py::_wait_for_server_health_async 188 function-lines src/hyperloom/orchestrator/actions/executors/_multi_node_server_lifecycle.py::_wait_for_workers_ready_async 97 function-lines src/hyperloom/orchestrator/actions/executors/_multi_node_server_lifecycle.py::restart_server_for_round 269 -function-lines src/hyperloom/orchestrator/actions/executors/_nogit_patch.py::_apply_patch_no_git 169 +function-lines src/hyperloom/orchestrator/actions/executors/_nogit_patch.py::_apply_patch_no_git 167 function-lines src/hyperloom/orchestrator/actions/executors/_partition_shape.py::validate_session_shape 127 function-lines src/hyperloom/orchestrator/actions/executors/_patch_snapshot.py::replayed_stack_ops 81 function-lines src/hyperloom/orchestrator/actions/executors/_patch_source_pr.py::materialize_candidate_patches 96 @@ -571,7 +571,7 @@ function-lines src/hyperloom/orchestrator/actions/executors/baseline.py::Benchma function-lines src/hyperloom/orchestrator/actions/executors/baseline.py::BenchmarkRunExecutor._run_once 682 function-lines src/hyperloom/orchestrator/actions/executors/baseline.py::BenchmarkRunExecutor._run_retrying 97 function-lines src/hyperloom/orchestrator/actions/executors/baseline.py::BenchmarkRunExecutor._run_single_benchmark 578 -function-lines src/hyperloom/orchestrator/actions/executors/baseline.py::_apply_warm_patches 416 +function-lines src/hyperloom/orchestrator/actions/executors/baseline.py::_apply_warm_patches 414 function-lines src/hyperloom/orchestrator/actions/executors/benchmark_result.py::extract_benchmark_measurement 135 function-lines src/hyperloom/orchestrator/actions/executors/benchmark_result.py::harvest_leaked_artifacts 85 function-lines src/hyperloom/orchestrator/actions/executors/benchmark_result.py::harvest_mn_gpu_metrics 122 @@ -586,7 +586,7 @@ function-lines src/hyperloom/orchestrator/actions/executors/integrate_patch.py:: function-lines src/hyperloom/orchestrator/actions/executors/integrate_patch.py::IntegratePatchExecutor._gate_perf 445 function-lines src/hyperloom/orchestrator/actions/executors/integrate_patch.py::IntegratePatchExecutor._keep_inert_switches 111 function-lines src/hyperloom/orchestrator/actions/executors/integrate_patch.py::IntegratePatchExecutor._maybe_write_framework_kb_record 114 -function-lines src/hyperloom/orchestrator/actions/executors/integrate_patch.py::IntegratePatchExecutor._stage_apply 509 +function-lines src/hyperloom/orchestrator/actions/executors/integrate_patch.py::IntegratePatchExecutor._stage_apply 477 function-lines src/hyperloom/orchestrator/actions/executors/integrate_patch.py::IntegratePatchExecutor._stage_gate 84 function-lines src/hyperloom/orchestrator/actions/executors/integrate_patch.py::IntegratePatchExecutor._stage_provision_attempt_runtime 105 function-lines src/hyperloom/orchestrator/actions/executors/integrate_patch.py::IntegratePatchExecutor._stage_resolve 123 @@ -941,7 +941,7 @@ module-lines src/hyperloom/orchestrator/actions/executors/_multi_node_server_lif module-lines src/hyperloom/orchestrator/actions/executors/_workload_envs.py 2261 module-lines src/hyperloom/orchestrator/actions/executors/baseline.py 4253 module-lines src/hyperloom/orchestrator/actions/executors/explore.py 1880 -module-lines src/hyperloom/orchestrator/actions/executors/integrate_patch.py 4799 +module-lines src/hyperloom/orchestrator/actions/executors/integrate_patch.py 4690 module-lines src/hyperloom/orchestrator/actions/executors/profile.py 1565 module-lines src/hyperloom/orchestrator/actions/executors/report.py 1391 module-lines src/hyperloom/orchestrator/actions/executors/roofline.py 1303