Skip to content

fix(integrate_patch): end a patch set the tree already holds as already_present - #1845

Merged
chaojhou merged 4 commits into
AMD-AGI:mainfrom
okorzh-amd:okorzh/already-present-outcome
Oct 23, 2026
Merged

chaojhou merged 4 commits into
AMD-AGI:mainfrom
okorzh-amd:okorzh/already-present-outcome

Conversation

@okorzh-amd

@okorzh-amd okorzh-amd commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor
  • Description: what and why

    integrate_patch restarted the server and benchmarked candidates whose every patch the framework tree already held. That happens with an upstream PR that has since merged, and with a specialist patch that repeats a change the tree already has. Both apply channels already detect this with a reverse check: git apply -R --check on a git checkout, and patch -R --fuzz=0 --dry-run on any other tree. But they reported it as an ordinary success. The round then benchmarked an unchanged tree, ran the accuracy eval for a FRAMEWORK candidate, and settled on noise. OUTCOME_ALREADY_PRESENT was an allowed framework-KB outcome that nothing wrote.

    Everything below describes head 223ec0129, rebased onto main at 8b0ae5500 (origin/main when the rebase was made). The previous head, ce8d6726b, sat on cc71f343b; the earlier description named 8a4171daf there by mistake.

    How main shaped this PR:

    What changes:

    • Presence is a field. _apply_patch_no_git used to return a 4-tuple and the git channel's _git_apply_collect_feedback a 3-tuple. Both now return a frozen PatchApplyResult with fields ok, error, backups, feedback and present_by. present_by is a PresenceCheck, either git_reverse_apply_check or patch_reverse_dry_run (_apply_feedback.py:19-25, :77-88).
    • The apply loop reports what it found. main's _apply_patch_series now reads each patch's PatchApplyResult from either channel and records a {patch, check} row for each patch whose present_by is set. It returns a _PatchSeries instead of main's (errors, feedback) tuple (_PatchSeries at _patch_git.py:221-234, _apply_patch_series at :237-284). changed_nothing holds only when the series is non-empty and every patch was present, localization patches included. A series that stopped on a failure never reaches that check, because the refusal returns first.
    • The git channel changes in place. In _patch_git.py, _git_apply_collect_feedback (:145-180) returns PatchApplyResult and builds its failures through the new _git_apply_failure (:131-142). _git_reverse_applies_cleanly (:104-128) probes only the levels _reverse_probe_levels names (see below). It drops the two function-local imports of _P_LEVELS that main kept next to the module-level one.
    • One exit. main's no-patches predicate gets a name, _changes_beyond_patches(attempt, deliverable) (_integrate_apply.py:44-52), which reads main's _Deliverable. It is true when the round has proposed server args, proposed envs, artifacts, setup commands that ran, or a newly acquired runtime. The no-patches early return (its check at :101) and the new exit both read it.
      • _stage_apply hands the series to _end_patch_series right after _apply_patch_series and before artifact install (:197-205). _end_patch_series (:310-331) holds both ways a series ends the round: main's git_apply_failed refusal through _refuse_apply, and the new exit.
      • A series that changed_nothing, in a round with no _changes_beyond_patches, ends there through _end_already_present (:333-361).
      • _end_already_present writes the KB row and empties attempt.applied, the list that _finish_attempt (integrate_patch.py:96) reports as patches_reverted.
    • The reverse probe is anchored. "Present" now ends a round, so both reverse probes accept a -p level only where some path named in the patch's ---/+++ headers resolves (_nogit_patch._reverse_probe_levels, :92-114).
      • The headers are read with patch_file_targets, the same parser the target preflight uses. Each level is checked with the same rule, path_resolves_at, which this PR extracts from patch_targets_missing (specialists/patch_safety.py:221-240).
      • Before, both probes tried every level. Undoing a deletion recreates the file. So a deletion-only patch passed the probe at a level where the stripped path names nothing (a/mod/alpha.py at -p0), even though its target still existed with other content. This held for a plain diff and for git format-patch output.
      • When no path resolves at any level, every level is still probed. In that case only a pure deletion whose files are all gone can reverse-check cleanly, and that deletion is in the tree.
    • Consumers.
      • The FRAMEWORK phase summary counts already_present as tested (framework/artifacts.py:13-15).
      • Warm-patch replay reads present_by, where it used to infer "already present" from an empty backup list (baseline.py:1298, in main's _apply_warm_patch_sequence). Its no-git call reads the result's ok, error and backups instead of unpacking main's 4-tuple, which would now raise TypeError.
      • The OUTCOME_ALREADY_PRESENT comment used to credit a semantic audit, which never wrote the outcome. It now says what the outcome means (knowledge/kb_writeback.py:25).
    • Docs. These pages now describe the new outcome and say that its KB row needs a PR identity:
      • docs/conceptual/optimization-loop.md;
      • src/hyperloom/inference_optimizer/SKILL.md;
      • src/hyperloom/inference_optimizer/actions/integrate_patch.md (step 3, the status enum and a failure-mode row).

    Not in this PR: integrate_patch.py, _patch_source_pr.py, test_patch_source_pr.py and phases/framework.py are the same as on main. An upstream PR head with no diff_url is handled exactly as on main (see Observable effect).

  • Linked issue(s): none filed. The defect and its cost are described above.

  • Tests: added/updated? commands run? (what to test; if a test was replaced, where its assertions live now):

    Added in src/hyperloom/orchestrator/actions/executors/tests/test_patch_already_applied_unit.py, which collects 19 cases at this head and 6 on main.

    Executor tests on a temporary tree. The benchmark is replaced by pytest.fail unless a test records that it was reached.

    • :367: a set whose every patch is already in the tree ends already_present.
      • It runs for an upstream_pr round and a specialist-authored round on a git checkout, and for an upstream_pr round on a non-git tree.
      • The result carries one {patch, check} row per patch, naming the channel's check, plus empty patches_applied and patches_reverted.
      • Exactly one KB already_present row is written. The tree is unchanged: file contents, and on the checkout also HEAD and git status.
    • :454: a PR merged into the tree, with a diff_url. The empty head diff falls back to the diff_url diff, and the round ends already_present through git_reverse_apply_check.
    • :469: the same PR merged and then reverted is applied and benchmarked, and no already_present row is written.
    • :485: a set already in the tree, in a round that also sets extra_server_args, is benchmarked.
    • :501: a no-op patch that rides with a real change lands with it. The result is applied_no_bench with both patches applied.

    Channel tests, over git and patch:

    • :248: a deletion-only patch whose target drifted is a failure with feedback, and the file is left untouched. This is tested as a plain diff and as git format-patch output (mail header before the diff).
    • :264: a deletion-only patch whose file is already gone is present at the channel level. In integrate_patch the target preflight ends such a set first (see Observable effect).

    Added in test_warm_patch_apply.py:823: a patch that a non-git install tree already holds is replayed through _apply_warm_patches and main's _apply_warm_patch_sequence. It ends with status already_present, the file unchanged and no backups.

    What each guard pins. Each was checked by breaking it at this head:

    • removing the exit fails 4 of these tests: the 3 :367 cases and :454;
    • dropping not _changes_beyond_patches(...) from the exit fails :485;
    • making changed_nothing true when any patch is present fails :501;
    • dropping attempt.applied.clear() fails the 3 :367 cases;
    • restoring the old probe, which read paths with parse_patch_manifest and probed every level when that parser raised, fails both format_patch cases of :248;
    • probing every level with no anchoring fails all 4 cases of :248;
    • restoring main's 4-tuple unpack at baseline.py:1289 fails 9 tests in test_warm_patch_apply.py, :823 among them.

    Added elsewhere: src/hyperloom/inference_optimizer/tests/test_framework_agent_selection_audit.py:133. An already_present row counts as tested in the phase summary.

    Updated:

    • test_patch_already_applied_unit.py: the six tests from main (:103, :125, :142, :153, :179, :199) now read the result's fields. :103, :125, :179 and :199 also assert present_by.
    • test_nogit_patch_unit.py: 30 tuple unpackings and the one test that indexed the tuple (:903, result[0], result[1]) now read the result's fields.
      • :225 asserts present_by in both cases. It is patch_reverse_dry_run when the prepare record persists, and unset on the failure when it cannot persist.
      • :431 replaces the 4-tuple length assertion with field assertions, and :456 drops its skip on a tuple of another length.
      • :28 keeps main's re-export identity checks unchanged. patch_git._apply_patch_no_git is ng._apply_patch_no_git still holds, because _patch_git imports the function.
    • test_integrate_patch_executor.py:2054 reads the result's fields. The "vet before any apply" test (:2662) is unchanged: it still patches _patch_git._git_apply_collect_feedback, which is where main's loop looks the function up.
    • test_integrate_patch_helpers_unit.py:601 calls patch_git._git_apply_collect_feedback, reads the result's fields and asserts that present_by is unset on a failed apply.
    • test_delivery_channels.py:72 reads the result's fields.

    Not covered by a test:

    • the exit with envs, artifacts, setup commands or an acquired runtime. Only extra_server_args is tested. Every one of these terms feeds the single _changes_beyond_patches flag, which the existing no-patches return also reads;
    • the git reverse check matching a hunk at an offset;
    • that warm-patch replay decides from present_by and not from the backup count. Restoring the empty-backups test at baseline.py:1298 fails nothing, because the two agree for every patch that touches a file (see Removed). :823 pins the outcome and the result type, not that choice.

    Commands run at head 223ec0129:

    • ruff check and ruff format --check on the 15 changed Python files: pass.
    • lint-imports: 10 contracts kept, 0 broken.
    • python scripts/code_metrics.py --base-ref origin/main: PASSED, with 0 new, 0 worse and 0 baseline out of date in every dimension, and 0 findings in every check. It lists _stage_apply as already over at the base (see Size/complexity).
    • scripts/check_cli_references.py, and scripts/check_broad_suppress.py on the changed Python files: pass.
    • scripts/pr_hygiene.py size --base origin/main --head HEAD: 361 production Python lines (size/L).
    • pytest on the seven changed test files plus test_patch_source_pr.py, test_specialist_patch_safety_unit.py and test_sglang_artifact_fix.py: 481 passed, 1 skipped (466 passed, 1 skipped on main 8b0ae5500; the 15 extra are this PR's new tests).
    • pytest -n 8 -m 'not critic_agent_e2e and not targeted_build_e2e' over orchestrator/actions/executors/tests, orchestrator/enablement/tests, orchestrator/framework, orchestrator/delivery/tests and test_framework_agent_selection_audit.py: 984 passed, 1 skipped, 3 failed. The same 3 fail on main 8b0ae5500 (969 passed):
      • two need the optional TraceLens package, which was not installed;
      • the third is test_setup_script.py::test_each_root_gets_its_final_state_at_its_own_absolute_path, which expects a umask of 022 and passes on both trees under one; the shell that ran the suite had 002.
    • The full suite (-n 24 -k 'not kernel_entry_auto_runs', clean environment), run twice on each tree:
      • here: 26052 passed, 31 skipped, 44 failed, 26 errors, both times;
      • on main 8b0ae5500: 26037 passed and 44 failed in one run, 26039 passed and 42 failed in the other, with 31 skipped and 26 errors in both.
      • Every id that fails here failed on main in at least one of its runs, so this PR adds none. The two ids that came and went on main, test_geak_resume_recovery.py::test_geak_kernel_phase_does_not_reuse_already_promoted_result and test_optimize_loop_walkthrough.py::test_both_arms_dry_walks_the_rest_of_the_chain[geak], refuse the GPU handoff when another process holds GPU memory, and pass when run alone on both trees.
      • The other failures need something this environment lacks, such as the TraceLens package, rocprof/ROCm, a git default branch other than master, or a umask of 022.
  • Size/complexity triggers crossed: split, or why the shape is right:

    • Diff size. 361 production Python lines (size/L): under the 400-line review trigger and the 1000-line limit.
      • _patch_git.py +62/−49 (677 → 690 lines): _PatchSeries, _git_apply_failure, and the result types through the git channel and _apply_patch_series.
      • _integrate_apply.py +71/−21 (326 → 376 lines): _changes_beyond_patches, _end_patch_series and _end_already_present, and main's inline refusal and no-patches predicate taken out of _stage_apply.
      • _nogit_patch.py +48/−22 (383 → 409 lines), _apply_feedback.py +27/−1, patch_safety.py +24/−18, baseline.py +5/−7.
    • Why the exit is its own method. On main, _stage_apply (_integrate_apply.py:47; :58 at this head) sits at cyclomatic 20, the ceiling. Its cognitive complexity (33, limit 30) and length (200 lines, limit 80) are over the limits. refactor(integrate-patch): give each stage of an integration attempt a module of its own #2098 moved it without a baseline entry, so the gate holds it to main's values, and neither may rise. The exit therefore lives in _end_patch_series. This PR takes _stage_apply to cognitive 32 and 186 lines; its cyclomatic complexity stays at 20.
    • The code-metrics gate reports 0 new and 0 worse units. scripts/code_metrics_baseline.txt only tightens: 2 entries lowered, none added, raised or removed. Each is a unit this PR shrinks:
      • _apply_patch_no_git: 141 → 140 lines;
      • the baseline.py module: 4050 → 4048 lines.
    • The other 4 entries the previous head lowered are gone from main's baseline. _stage_apply and the integrate_patch.py module lost theirs in refactor(integrate-patch): give each stage of an integration attempt a module of its own #2098. _apply_warm_patches was split by refactor(baseline): name how a run applies its patches, and the failures that end it #1961, and its helper _apply_warm_patch_sequence (64 → 62 lines) is under the limit.
    • New and changed units stay under the review triggers (cyclomatic / cognitive, length):
      • the largest is _apply_patch_series, which goes from 8 / 15 and 44 lines on main to 9 / 17 and 48 lines;
      • _git_apply_collect_feedback is 6 / 9 and 36 lines (58 lines on main); _git_apply_failure is 1 / 0, 12 lines;
      • _end_already_present is 2 / 1, 29 lines; _end_patch_series is 3 / 3, 22 lines; _changes_beyond_patches is 1 / 2, 9 lines;
      • _reverse_probe_levels is 2 / 9, 23 lines;
      • path_resolves_at is 3 / 3, 20 lines. Extracting it takes patch_targets_missing from 9 / 20 and 48 lines to 3 / 7 and 31 lines.
    • patch_safety.py grows from 1152 to 1158 lines. That is over the 800-line warning, as on main, and under the 1200-line limit. _patch_git.py (690) and _integrate_apply.py (376) stay under the warning.
  • If this simplifies or refactors: which mechanism was removed, which contract was preserved, and the tests that show it:

    Removed:

    • the tuple results of _apply_patch_no_git (4 elements), _git_apply_collect_feedback (3) and _apply_patch_series (2), now PatchApplyResult and _PatchSeries;
    • warm-patch replay's empty-backups heuristic for "already present". A real apply records a backup for every target, created files included, so the old heuristic and present_by differ only for a patch that touches no file;
    • the inline strip-level loop in patch_targets_missing, now path_resolves_at;
    • the inline no-patches predicate in _stage_apply, now _changes_beyond_patches.

    Changed where main put it (no function changes module):

    • _git_reverse_applies_cleanly probes only the levels _reverse_probe_levels names;
    • _git_apply_collect_feedback returns PatchApplyResult and builds its failures through _git_apply_failure;
    • main's git_apply_failed refusal moves from _stage_apply into _end_patch_series, unchanged.

    Contracts preserved, with the tests that pin them:

    • An apply failure keeps its status and error_class (apply_failed / git_apply_failed): test_integrate_patch_executor.py:702.
    • Unvetted patches are refused before any git call: test_integrate_patch_executor.py:2662.
    • A subset no-op is accepted and lands with its set: test_patch_already_applied_unit.py:103, :179, :501.
    • A no-op counts only with a durable prepare record: test_nogit_patch_unit.py:225.
    • The preflight reports the same missing targets: test_specialist_patch_safety_unit.py:66-76, orchestrator/enablement/tests/test_sglang_artifact_fix.py:81, test_integrate_patch_executor.py:730.
    • Also preserved, with no dedicated test:
      • an apply failure's retry_feedback and prior_patches;
      • the series stops at the first failure;
      • no-git backup numbering continues across patches.
  • Observable effect: what an operator will now see, and what the old behaviour cost them (if none, why the change is unobservable):

    When every patch of a candidate is already in the framework tree. integrate_patch ends with status: already_present, and no server restart, benchmark or accuracy eval runs. This requires that the round change nothing else:

    • no server args or envs the candidate itself proposes (extra_server_args, config_changes, extra_envs, framework-switch envs). Args and envs inherited from earlier rounds do not count;
    • no artifacts, no setup commands that ran, and no newly acquired runtime.

    The exit applies to FRAMEWORK upstream-PR candidates, FRAMEWORK and explore specialist patches, and enablement rounds.

    What the result contains.

    • An already_present list with one {patch, check} row per patch. check is git_reverse_apply_check on a git checkout and patch_reverse_dry_run on any other tree.
    • Empty patches_applied and patches_reverted, and no workspace.
    • An INFO log line: integrate_patch: the framework tree already holds this candidate; not benchmarking: ....

    The framework KB row. lessons.jsonl gets a row with outcome: already_present and tps_delta_pct: 0 when the candidate carries a PR identity. That is an upstream-PR URL or head SHA, or a framework proposal's fa_pr_url / fa_pr_sha. Without one, no row is written, as for every other outcome. Nothing reads the row to skip candidates.

    How the round settles.

    • Writeback settles it with verdict failed, like apply_failed and no_patches. optimization_journal.json records it as no_promote, and the intervention mix records a code_patch_attempt.
    • In the enablement lane it is a stall. It is neither kept nor advanced, so it is charged as a failed round and counts toward the consecutive-stall cap. Nothing boots.
    • For a FRAMEWORK candidate (upstream-PR and authored candidates both reach the authored-outcome bridge):
      • the progress row has status already_present, is not kept, and has gain 0;
      • the candidate's proposal is settled as dropped, because the progress stamp settles every status other than kept and reverted that way (phases/framework.py:2074, unchanged);
      • the framework_agent_phase_done event counts the candidate in candidate_outcomes.already_present (a count per status) and in tested, and so does the tested= figure of the "tested candidates but none cleared the gate" advisory;
      • the Critic's session priors leave it out, as they leave out apply_failed and fetch_failed, because already_present is not in their terminal set (phases/framework.py:1860-1867, unchanged). The candidate already has a progress row, so it is not selected again this session.

    What it cost before. Each such candidate paid for a restart, a benchmark and, for FRAMEWORK candidates, an accuracy eval on an unchanged tree. The measured delta was noise, which came back in one of two ways:

    • reverted, with a KB reverted_smoke_fail row carrying a noise tps_delta_pct;
    • kept, when the noise cleared the keep threshold. That wrote an integrated KB row and added an optimization_stack layer and a current_best lift for a gain the code did not produce.

    A deletion-only patch whose target file still exists with different content, in plain diff or git format-patch form:

    • Before: it passed as an already-applied no-op, and the file stayed in the tree.
    • Now: it ends apply_failed with reauthoring feedback. In warm-recipe replay on a non-git tree, the same overlay reports failed with reason nogit_apply_failed instead of already_present.

    An upstream PR merged into the tree.

    • When it is fetched through its diff_url, the reverse check on the fetched diff decides. If every hunk of the PR, with its context, is still in the tree and the round changes nothing else, the round ends already_present. Before, it was benchmarked.
    • A PR merged and then reverted is applied and benchmarked, as before.
    • Without a diff_url, an empty head diff still ends fetch_failed (checkout_head_failed), as on main.

    Hunks at an offset. On a git checkout, git apply -R --check accepts a hunk at an offset; the patch channel does not. A hunk whose post-image and context also appear elsewhere in the same file can therefore pass the reverse check while its own lines still hold the old code. This was already treated as a satisfied no-op. A candidate made only of such patches now ends already_present where it used to be benchmarked.

    Unchanged:

    • A set in which only some patches are present lands and is benchmarked.
    • A single patch with only some of its hunks in the tree is not present, and ends apply_failed.
    • A deletion that upstream already made never reaches the presence check. The target preflight finds its pre-image missing and ends the round apply_failed with error_class: patch_target_missing, before any apply.

    Minor: with apply_only, a set already in the tree returns already_present where it used to return applied_no_bench. Only tests set apply_only; no production code or agent prompt does.

  • Breaking changes: no.

    • already_present was already an allowed KB outcome, and the lessons.jsonl fields are unchanged. The only in-repo reader of lessons.jsonl is the config-lever accuracy check. It matches on applicability and a negative accuracy_delta_pct. The exit measures no accuracy, so an already_present row carries an accuracy_delta_pct of 0 unless the candidate's own proposal supplies one, which no in-repo producer does.
    • Progress rows in state.json can carry a new status value, but gain no new field.
    • The integrate_patch result gains a new status and an already_present key; both are additive. Writeback, the FRAMEWORK bridge and the enablement lane handle them as described above.
    • No CLI flag, env var or artifact path changes.
    • The signatures that change are underscore-private, and every in-repo caller is migrated, including main's warm-replay call in baseline.py:
  • PR addresses single concern: yes. The refactor is part of the fix:

    • the exit could not be added inline to _stage_apply under the code-metrics gate, so it lives in _end_patch_series;
    • the presence signal needs a field the positional tuples did not have, so they became PatchApplyResult and _PatchSeries;
    • the probe's header read is a third lenient errors="replace" read in _nogit_patch.py. The gate's repeated-literal check refuses a third copy, so the value is named _DECODE_ERRORS.

    The unrelated changes this PR used to carry are now refactor(framework): drop the semantic_status mapping nothing produces; correct the KB-ledger docstring and warning; fix three misspellings #2029: dropping the dead semantic_status mapping in phases/framework.py, correcting a false KB-ledger comment, and fixing three misspellings.

  • Root cause is upstream (Magpie/TraceLens/GEAK/IntelliKit/AgentKernelArena), ticket filed: no, the cause is in this repo.

@chaojhou chaojhou left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @okorzh-amd. No blockers:

  • A set ends already_present only when every patch is fully present and nothing errored.
  • Both channels' reverse checks fail closed.
  • The extracted apply_patch_set matches the old loop.

Two must-fix items:

  1. The merged-PR-head exit skips the "round changes nothing else" check.

    • integrate_patch.py:2042-2043 ends the attempt as already_present in _stage_resolve. That is before _stage_apply reads config_changes, extra_server_args, extra_envs, artifacts or the enablement setup. changes_beyond_patches (:2419, :2704) guards only the patch-set exit.
    • Example: an upstream_pr task for a merged PR with no diff_url, which also carries extra_server_args to turn on the PR's opt-in switch. It ends already_present and writes the KB row, and the arg change is never measured. The same candidate with a diff_url is benchmarked.
    • That contradicts SKILL.md ("ends the same way") and the description.
    • Please take this exit only when the params carry no other change, and otherwise return the checkout-head failure as before.
  2. The description predates the rebase onto #1858.

    • Main no longer has a git apply -3 retry; #1858 removed it. So three statements describe nothing this PR changes: "runs the reverse check before the -3 retry", "reports a committed patch present instead of three-way merging it" and "no longer goes through git apply -3".
    • The complexity numbers are pre-rebase. At head:
      • _stage_apply is cyclomatic 53→45, cognitive 131→110, and 513→470 lines;
      • integrate_patch.py is 4804→4703 lines;
      • the baseline edits lower 9 entries and remove 1.
    • Please drop the -3 statements and refresh the numbers.

CI has not run yet; the fork workflows are awaiting approval. In a local run, ruff, import-linter and the code-metrics checks pass, and pr-hygiene will warn at 825 production lines.

@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Gate job conclusion: success (run) on c5bee6ec04f2e0439581bc2e913f4a65790c7aca, as reported by GitHub.

The report below was uploaded by that run; the conclusion above is the result.


Code metrics gate: PASSED

Judged: the 19 files this change touches. Files it does not touch never fail it; anything found there is listed under Outside this change for information.

Dimension Limit New Worse Baseline out of date Baseline entries vs base
Cyclomatic complexity fails > 20 0 0 0 11 +0
Cognitive complexity fails > 30 0 0 0 65 +0
Function length (lines) fails > 80 0 0 0 188 +0
Nested block depth fails > 5 0 0 0 8 +0
Parameter count fails > 5 0 0 0 83 +0
Module length (lines) warns > 800, fails > 1200 0 0 0 45 +0
Duplicated lines per file fails > 0 0 0 0 95 +0
Dead code (vulture) fails > 0 0 0 0 2 +0
Check Findings Effect
Comments 0
English only 0
Production code imports test code 0
Repeated literals 0
A1 Private name of another package 0
A8 Flag argument 0
A9 Tuple return 0
A10 Raw vocabulary comparison 0
A11 Hidden global write 0
A12 Pure forwarder 0
A12 Single-implementation abstraction 0

Module length warning: over 800 lines, within the 1200-line failure limit (not failing): 1

Module Lines Limit
src/hyperloom/orchestrator/specialists/patch_safety.py 1158 warns > 800

Already over at the base (backlog, not this change's): 2

Unit Dimension Base Now
src/hyperloom/orchestrator/actions/executors/_integrate_apply.py:58 ApplyStageMixin._stage_apply Cognitive complexity 33 32
src/hyperloom/orchestrator/actions/executors/_integrate_apply.py:58 ApplyStageMixin._stage_apply Function length (lines) 200 186
66 findings in files this change does not touch

Outside this change (informational): 66

Unit Dimension Baseline Now
src/hyperloom_kb/collect/mapping.py _experience Cognitive complexity absent 31
src/kernelforge/cli/forge_loop_command.py forge_loop Cognitive complexity absent 31
src/kernelforge/loop/runner.py IterationLoop._collect_session_candidate Cognitive complexity absent 44
src/kernelforge/loop/runner.py IterationLoop._rule_on_attempt Cognitive complexity absent 38
src/hyperloom/orchestrator/phases/kernel.py KernelPhase._record_geak_delegation_timeline Parameter count absent 7
src/hyperloom/inference_optimizer/cli/__init__.py _reexport_session_environment Function length (lines) absent 107
src/hyperloom/inference_optimizer/cli/__init__.py _resolve_agent_lineup Function length (lines) absent 89
src/hyperloom/inference_optimizer/cli/__init__.py _resolve_run_topology Function length (lines) absent 92
src/hyperloom/inference_optimizer/multi_node/scripts/launch_multinode.py _build_parser Function length (lines) absent 84
src/hyperloom/inference_optimizer/multi_node/scripts/launch_multinode.py _spawn_rank_actors Function length (lines) absent 84
src/hyperloom/orchestrator/actions/executors/_integrate_bench.py BenchStageMixin._bench_patch Function length (lines) absent 223
src/hyperloom/orchestrator/actions/executors/_integrate_enablement_gate.py EnablementGateMixin._enablement_keep_records Function length (lines) absent 110
src/hyperloom/orchestrator/actions/executors/_integrate_enablement_gate.py EnablementGateMixin._gate_enablement Function length (lines) absent 222
src/hyperloom/orchestrator/actions/executors/_integrate_perf_gate.py PerfGateMixin._gate_perf Function length (lines) absent 241
src/hyperloom/orchestrator/actions/executors/_integrate_perf_gate.py PerfGateMixin._keep_inert_switches Function length (lines) absent 92
src/hyperloom/orchestrator/actions/executors/_integrate_perf_gate.py PerfGateMixin._switch_off_parity Function length (lines) absent 116
src/hyperloom/orchestrator/actions/executors/_integrate_resolve.py ResolveStageMixin._stage_provision_attempt_runtime Function length (lines) absent 105
src/hyperloom/orchestrator/actions/executors/_integrate_resolve.py ResolveStageMixin._stage_resolve Function length (lines) absent 104
src/hyperloom/orchestrator/trace_analysis/_analysis_reports.py write_reports Function length (lines) absent 204
src/hyperloom_kb/sync.py GlobalSync.pull Function length (lines) absent 84
src/kernelforge/loop/runner.py IterationLoop._close_iteration Function length (lines) absent 117
src/kernelforge/loop/runner.py IterationLoop._collect_session_candidate Function length (lines) absent 185
src/kernelforge/loop/runner.py IterationLoop._implementer_history Function length (lines) absent 112
src/kernelforge/loop/runner.py IterationLoop._rule_on_attempt Function length (lines) absent 102
src/kernelforge/loop/runner.py IterationLoop._source_attempt Function length (lines) absent 175

...and 41 more; the job log lists every row.

Verdict computed by the gate scripts in /home/runner/work/_temp/code-metrics-gate, not by this tree's copy.

Thresholds and sources: [tool.hyperloom.code_metrics] in pyproject.toml. Tools: complexipy 8.0.1, jscpd 5.4.1, ruff 0.16.2, vulture 2.16.

Comment thread src/hyperloom/orchestrator/actions/executors/_patch_set_apply.py Fixed
Comment thread src/hyperloom/orchestrator/actions/executors/integrate_patch.py Fixed
@okorzh-amd
okorzh-amd requested a review from chaojhou October 14, 2026 18:04
Comment thread src/hyperloom/orchestrator/actions/executors/integrate_patch.py Fixed

@chaojhou chaojhou left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #1845 -- fix(integrate_patch): end a patch set the tree already holds as already_present

What it does: integrate_patch used to restart the server and benchmark candidates whose every patch the framework tree already held, because both apply channels reported "already present" as an ordinary success. The apply helpers now return a named PatchApplyResult carrying which reverse check found the patch present, the apply loop moves into _patch_set_apply.apply_patch_set, and an attempt whose patches are all present (or whose merged PR head matches the tree on its changed files) and that changes nothing else ends already_present with a KB row and no benchmark.

Blocking issues: 1

  1. [X2] The PR description still describes a git apply -3 path that exists at neither base nor head, and its complexity numbers are stale [verified]
    Problem: The body says "The git channel runs the reverse check before the -3 retry", that it "reports a committed patch present instead of three-way merging it", and that "a fully present patch no longer goes through git apply -3". The git channel, _git_apply_collect_feedback, has no -3 path at base 1a444f6 or at head. The body also gives _stage_apply cyclomatic 55 to 47 and the largest new function at 9; c901.py measures 53 to 41 and 7 at head. src/hyperloom/orchestrator/actions/executors/_patch_set_apply.py:95
    Impact: Reviewers read an index-safety change and complexity figures that this PR does not have.
    Action: Drop the -3 statements, including the one in the opening paragraph, and refresh every figure in the size and complexity list against the current head.

Checked: integrate_patch.py _stage_apply, _materialize_upstream_pr, _RoundProposal.changes_beyond_patches, _end_already_present; _patch_set_apply.py apply_patch_set, _git_apply_collect_feedback; _nogit_patch.py _reverse_probe_levels; _patch_source_pr.py _empty_head_diff, _tree_has_head_files; patch_safety.py path_resolves_at; baseline.py _apply_warm_patches; consumers of already_present in framework/artifacts.py, phases/framework.py, loop/writeback.py; ci.txt | Ran: python ..\pr_tools\hl_fetch.py 1845; git grep for -3/3way at base and head; python ..\pr_tools\c901.py 1a444f6 510fddd on the changed src files; git log/diff --stat fcbcb4a..510fddd; gh run list --commit 510fddd after approving the fork runs (9 workflows success, the 2 label-gated E2E skipped) | Base: 1a444f6 | Head: 510fddd

@chaojhou chaojhou left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #1845 -- fix(integrate_patch): end a patch set the tree already holds as already_present

What it does: integrate_patch restarted the server and benchmarked candidates whose every patch the framework tree already held, because both apply channels reported "already present" as an ordinary success. The apply helpers now return a named PatchApplyResult with present_by, the apply loop moves into _patch_set_apply.apply_patch_set, and an attempt whose patches are all present (or whose merged PR head matches the tree on its changed files) and that changes nothing else ends already_present with a KB row and no benchmark.

Blocking issues: 2

  1. [S4] A drifted deletion-only patch in mail format still reads as present, so the round ends already_present while the file is untouched [verified]
    Problem: _reverse_probe_levels falls back to every -p level when parse_patch_manifest raises ValueError. A git format-patch deletion (mail header before the diff) raises there, while the preflight parser reads it (patch_file_targets gives [('a/mod/alpha.py', '/dev/null')], nothing missing). At a level where the stripped path is absent, the reverse check of the deletion passes. src/hyperloom/orchestrator/actions/executors/_nogit_patch.py:112
    Impact: on a scratch repo whose mod/alpha.py drifted from the patch's pre-image, _git_apply_collect_feedback returns ok=True, present_by=GIT_REVERSE_APPLY and the file is left as it was, so apply_patch_set reports changed_nothing and the round writes an already_present KB row for a change the tree does not have. This is the false positive the anchoring in this PR closes, reopened for mail-format input; the description says such a patch "now fails to apply".
    Action: take the probe's paths from the parser the preflight already uses (patch_file_targets) and delete the ValueError branch; add a mail-format case to the drifted-deletion test.

  2. [X2] The description still describes code this PR does not contain, and its figures are stale [verified]
    Problem: unchanged since the last round at this head. It still says "a git apply -3 of a patch the tree already holds ...", "The git channel runs the reverse check before the -3 retry", "reports a committed patch present instead of three-way merging it" and "no longer goes through git apply -3", though neither base nor head has a -3 path; and it still gives _stage_apply "55 to 47, cognitive 140 to 119, 538 to 495", "4915 to 4780" and a largest new function of 9. src/hyperloom/orchestrator/actions/executors/_patch_set_apply.py:111
    Impact: reviewers and the release notes read an index-safety change and figures this PR does not have.
    Action: remove every -3 statement, refresh the figures from the head, and drop "the preflight and the probe resolve paths the same way" (they use different parsers, item 1).


Design review (requested by the maintainer; beyond the blocking bar)

The aim of this section is a smaller PR, not more guards: each point says what to delete or merge.

Purpose, route, files. Purpose and route as above. Production: _apply_feedback.py (result types), _nogit_patch.py (anchored probe), the new _patch_set_apply.py (git channel and set loop moved out of integrate_patch.py), integrate_patch.py (_RoundProposal, two exits, _end_already_present), _patch_source_pr.py (head-diff result type, _tree_has_head_files), patch_safety.py (path_resolves_at), baseline.py (warm replay reads present_by), status consumers in artifacts.py and framework.py. Tests: test_patch_already_applied_unit.py, test_patch_source_pr.py, tuple-to-dataclass migrations. Docs: SKILL.md, the integrate_patch playbook, optimization-loop.md.

1. Completeness. The core exit agrees with its consumers: writeback journals already_present as no_promote and the enablement lane counts it as a stall; the description states neither. Apart from item 1, a set whose deletion upstream already made never reaches the probe, because the preflight ends it apply_failed/patch_target_missing; that is fine, say so.

2. Simpler / smaller. The merged-PR-head exit buys nothing at runtime. At base an empty merge-base..head diff without a diff_url already ended fetch_failed with no boot (base _patch_source_pr.py:288, :301); with a diff_url, base and head both fall back to the diff and apply_patch_set now decides. Its only effect is relabelling fetch_failed as already_present, and for that the PR adds PresenceCheck.PR_HEAD_FILES, _tree_has_head_files (which trusts discovery's changed_files and fails once any of them is touched after the merge), _empty_head_diff, PrMaterialization.already_present riding on failure, _materialize_upstream_pr, _RoundProposal, an async _stage_resolve_upstream_pr, and a second "changes beyond patches" decision. My first-round must-fix asked for that guard; dropping the exit is the better fix. Ride-alongs that belong in their own PR: the semantic_status cleanup and its test edits, the comment and codespell fixes.

3. Redundancy and silent failure. "Changes beyond patches" is decided twice from different inputs: integrate_patch.py:2122-2123 uses params["enablement"] as a stand-in for setup and runtime, :2462-2465 uses the measured setup_commands_applied and the attempt runtime. PrMaterialization encodes three states in two fields. _end_already_present writes patches_reverted: [] and also clears attempt.applied; one is enough. The ValueError fallback is item 1; git errors otherwise fail closed.

4. Docs and comments. Besides item 2: _patch_source_pr.py:211-212 says the caller returns the failure as is, but integrate_patch.py:2118-2124 may return already_present instead; SKILL.md, optimization-loop.md and _apply_feedback.py:26 say "every file the PR changes", while the code checks the candidate's changed_files.

5. Tests. Keep: the all-present exit (test_patch_already_applied_unit.py:426), all-present plus server args benchmarks (:609), a no-op riding with a real change (:625), merged PR via diff_url (:559), reverted PR (:593), the drifted deletion (:290), the present_by assertions and the mechanical migrations. Redundant or freezing the current shape: :213 (the -3 story, repeats :178), :230 (duplicates :625 one layer down), :273 (fold into :426 as a non-git parameter), :574 (same decision as :609), :328 (pins the fail-open fallback of item 1), and the PR-head-exit tests :500, :531 and test_patch_source_pr.py:139-200.

Starting from zero. The mechanism is worth having in its core form: the channels already prove presence with a reverse check; surface it as a field, end an all-present round that changes nothing else, write the KB row. That is PatchApplyResult, apply_patch_set (needed for the metrics gate and the import cycle), a probe anchored on the preflight parser, one exit in _stage_apply and _end_already_present -- roughly half of today's production diff, with _patch_source_pr.py and _stage_resolve_upstream_pr left as at base.

Change list, in order of impact:

  1. Fix the probe (item 1) and add the mail-format test; delete :328.
  2. Delete the merged-PR-head exit: revert _patch_source_pr.py and _stage_resolve_upstream_pr to base, drop PR_HEAD_FILES, _materialize_upstream_pr and _RoundProposal, and name the existing predicate once inside _stage_apply; delete its tests and doc sentences.
  3. Trim the tests listed in point 5.
  4. Keep one encoding of "nothing applied" in _end_already_present.
  5. Move the semantic_status cleanup to its own PR.
  6. Rewrite the description (item 2), and state the no_promote journalling, the enablement stall, and that a deletion already made upstream ends patch_target_missing.

Checked: _nogit_patch._reverse_probe_levels, _patch_set_apply._git_apply_collect_feedback / apply_patch_set, patch_safety.patch_file_targets / path_resolves_at, integrate_patch._materialize_upstream_pr / _stage_apply / _end_already_present and both changes_beyond_patches calls, _patch_source_pr at base and head, the tests named above, the description against the head | Ran: _git_apply_collect_feedback on a scratch git repo with a drifted mod/alpha.py and a git format-patch deletion (present_by=GIT_REVERSE_APPLY, file unchanged); no test suite | Base: 1a444f6 | Head: 510fddd

@okorzh-amd
okorzh-amd force-pushed the okorzh/already-present-outcome branch 3 times, most recently from 945b0cb to 5a5d3a3 Compare October 19, 2026 17:02

@chaojhou chaojhou left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #1845 -- fix(integrate_patch): end a patch set the tree already holds as already_present

What it does: PR #1845 makes integrate_patch stop benchmarking candidates that the framework tree already holds. The per-patch apply helpers now return a PatchApplyResult whose present_by field records which reverse check found the patch. The apply loop moves into _patch_set_apply.apply_patch_set, and the reverse probe only tries -p levels where a patched path resolves. When every patch is present and the round changes nothing else, _stage_apply ends with status already_present and writes a KB row through the existing writeback, without restarting the server.

Blocking issues: 1

  1. [X2-stale-description] The PR description still describes the merged-PR-head exit, the Critic-priors and semantic_status changes, the -3 path and old figures, none of which are in the code at head [verified]
    Problem: The head diff leaves _patch_source_pr.py and phases/framework.py exactly as they are at the base. The commit 'refactor(integrate_patch): drop the merged-PR-head already_present exit' removed that exit. The live body (checked with gh pr view at 2026-10-20 and identical to body.txt) still claims all of the following. (a) Without a diff_url, a merged PR head ends already_present ('pr_head_files_in_tree') and returns a {diff_range, check} row. (b) test_patch_source_pr.py pins that mapping. (c) 'The Critic priors now include already_present'. At head, phases/framework.py:1936-1943 does not have it in the terminal set. (d) The semantic_status mapping is deleted. At head it is still at phases/framework.py:2623-2629. (e) The 'Discovery dedups on this ledger' comment is corrected. It is still at integrate_patch.py:4163. (f) Three statements about a git apply -3 retry, which exists at neither the base nor head. (g) Old figures: _stage_apply '55 to 47 / 140 to 119 / 538 to 495', '4915 to 4780', 'materialize_candidate_patches 96 to 82' and '11 lowered, 2 removed'. The baseline at head shows 53->45 / 131->110 / 489->457 and 4741->4640, with 7 entries lowered and none removed. Two earlier CHANGES_REQUESTED reviews already flagged items (f) and (g). The author's later commits fixed the code but not the body. src/hyperloom/orchestrator/actions/executors/integrate_patch.py:2738
    Impact: The release notes would announce operator-visible behaviour that does not ship. Examples: a merged PR head with no diff_url ending already_present with a diff_range evidence row, and already_present appearing in the Critic's recent_outcomes. They would also announce an index-safety change that does not exist. In fact a merged PR head with no diff_url still ends fetch_failed, and the Critic priors never see already_present.
    Action: Rewrite the body against head 5a5d3a3. Remove the merged-PR-head exit and its tests, the Critic-priors bullet, the semantic_status bullet and its test edits, the ledger-comment bullet, every -3 statement and the _patch_source_pr figures. Recompute the complexity and baseline figures. State the remaining observable effects that exist: the FRAMEWORK progress row is settled as 'dropped', the enablement lane counts already_present as a stall, and the phase summary counts it as tested.

Checked: Diff at head 5a5d3a3 against merge base b7004d1. Files read: integrate_patch.py (call, _finish_attempt, _stage_apply, _end_already_present, _maybe_write_framework_kb_record), _patch_set_apply.py, _nogit_patch.py (_reverse_probe_levels), patch_safety.path_resolves_at, baseline._apply_warm_patches, kb_writeback ALLOWED_OUTCOMES, framework/artifacts.py. Consumers of already_present read: phases/framework.py (Critic priors, semantic_status, _stamp_framework_progress disposition, the apply_failed retry path) and enablement/lane.maybe_rearm_enablement. git merge-tree against origin/main 1cdc6ff merges cleanly. Since the base, main has 8 commits and none touches the files this PR changes, so no call this PR adds is written against an old signature. CI at head is green. Ran a local script and probe2.py against the merged tree, using git archive into a local script and a stubbed fcntl. The probe runs apply_patch_set on scratch trees, git and non-git, with these cases: all present, partly present, a drifted deletion (unified and mail format), a deletion already carried out, and a file-creation patch that is already present. | Base: b7004d1 | Head: 5a5d3a3

@okorzh-amd

Copy link
Copy Markdown
Contributor Author

PR #1845 -- fix(integrate_patch): end a patch set the tree already holds as already_present

What it does: PR #1845 makes integrate_patch stop benchmarking candidates that the framework tree already holds. The per-patch apply helpers now return a PatchApplyResult whose present_by field records which reverse check found the patch. The apply loop moves into _patch_set_apply.apply_patch_set, and the reverse probe only tries -p levels where a patched path resolves. When every patch is present and the round changes nothing else, _stage_apply ends with status already_present and writes a KB row through the existing writeback, without restarting the server.

Blocking issues: 1

  1. [X2-stale-description] The PR description still describes the merged-PR-head exit, the Critic-priors and semantic_status changes, the -3 path and old figures, none of which are in the code at head [verified]
    Problem: The head diff leaves _patch_source_pr.py and phases/framework.py exactly as they are at the base. The commit 'refactor(integrate_patch): drop the merged-PR-head already_present exit' removed that exit. The live body (checked with gh pr view at 2026-10-20 and identical to body.txt) still claims all of the following. (a) Without a diff_url, a merged PR head ends already_present ('pr_head_files_in_tree') and returns a {diff_range, check} row. (b) test_patch_source_pr.py pins that mapping. (c) 'The Critic priors now include already_present'. At head, phases/framework.py:1936-1943 does not have it in the terminal set. (d) The semantic_status mapping is deleted. At head it is still at phases/framework.py:2623-2629. (e) The 'Discovery dedups on this ledger' comment is corrected. It is still at integrate_patch.py:4163. (f) Three statements about a git apply -3 retry, which exists at neither the base nor head. (g) Old figures: _stage_apply '55 to 47 / 140 to 119 / 538 to 495', '4915 to 4780', 'materialize_candidate_patches 96 to 82' and '11 lowered, 2 removed'. The baseline at head shows 53->45 / 131->110 / 489->457 and 4741->4640, with 7 entries lowered and none removed. Two earlier CHANGES_REQUESTED reviews already flagged items (f) and (g). The author's later commits fixed the code but not the body. src/hyperloom/orchestrator/actions/executors/integrate_patch.py:2738
    Impact: The release notes would announce operator-visible behaviour that does not ship. Examples: a merged PR head with no diff_url ending already_present with a diff_range evidence row, and already_present appearing in the Critic's recent_outcomes. They would also announce an index-safety change that does not exist. In fact a merged PR head with no diff_url still ends fetch_failed, and the Critic priors never see already_present.
    Action: Rewrite the body against head 5a5d3a3. Remove the merged-PR-head exit and its tests, the Critic-priors bullet, the semantic_status bullet and its test edits, the ledger-comment bullet, every -3 statement and the _patch_source_pr figures. Recompute the complexity and baseline figures. State the remaining observable effects that exist: the FRAMEWORK progress row is settled as 'dropped', the enablement lane counts already_present as a stall, and the phase summary counts it as tested.

Checked: Diff at head 5a5d3a3 against merge base b7004d1. Files read: integrate_patch.py (call, _finish_attempt, _stage_apply, _end_already_present, _maybe_write_framework_kb_record), _patch_set_apply.py, _nogit_patch.py (_reverse_probe_levels), patch_safety.path_resolves_at, baseline._apply_warm_patches, kb_writeback ALLOWED_OUTCOMES, framework/artifacts.py. Consumers of already_present read: phases/framework.py (Critic priors, semantic_status, _stamp_framework_progress disposition, the apply_failed retry path) and enablement/lane.maybe_rearm_enablement. git merge-tree against origin/main 1cdc6ff merges cleanly. Since the base, main has 8 commits and none touches the files this PR changes, so no call this PR adds is written against an old signature. CI at head is green. Ran a local script and probe2.py against the merged tree, using git archive into a local script and a stubbed fcntl. The probe runs apply_patch_set on scratch trees, git and non-git, with these cases: all present, partly present, a drifted deletion (unified and mail format), a deletion already carried out, and a file-creation patch that is already present. | Base: b7004d1 | Head: 5a5d3a3

Thanks. You were right that the description no longer matched the code. It is now rewritten against 033cb06, and it covers every item in your Action list:

  • (a)/(b) The merged-PR-head exit and its tests are gone. The body says _patch_source_pr.py, test_patch_source_pr.py and phases/framework.py are the same as on main, and that a PR head with no diff_url still ends fetch_failed (checkout_head_failed).
  • (c) The body now says the Critic priors leave already_present out, because it is not in their terminal set.
  • (d)/(e) The semantic_status change and the ledger-comment fix are listed under refactor(framework): drop the semantic_status mapping nothing produces; correct the KB-ledger docstring and warning; fix three misspellings #2029, where they now live.
  • (f) Every statement about a three-way retry is removed.
  • (g) All figures are recomputed at head: _stage_apply 53->45 / 131->110 / 489->457, the integrate_patch.py baseline entry 4741->4640, _apply_patch_no_git 141->140, _apply_warm_patches 416->414, baseline.py 4197->4195, and 7 entries lowered with none removed.
  • I checked the three observable effects you listed in the code, and the body now states them: the proposal is settled dropped (phases/framework.py:2139), the enablement lane charges it as a stall, and the phase summary counts it as tested.

The branch is rebased onto main 7ac8c12. Main's split of _apply_patch_no_git (#1980) was kept, and this PR's PatchApplyResult change now sits on top of it. One test stub now raises OSError rather than RuntimeError, because #2049 narrowed the handler around the _bench_patch call in _stage_gate. The diff is 608 production lines (was 632), and the gates and tests pass. I have no disagreements with the review.

@chaojhou chaojhou left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #1845 -- fix(integrate_patch): end a patch set the tree already holds as already_present

What it does: when every patch of a set reverse-applies cleanly, the tree already holds the change. integrate_patch now ends such a set as already_present instead of failing it as git_apply_failed, through _end_patch_series and _end_already_present.

The PR also restructures the apply path:

  • The git apply channel moves into _git_patch.py.
  • _apply_patch_series returns a _PatchSeries with changed_nothing.
  • The no-git reverse probe is anchored on the preflight's patch parser (_reverse_probe_levels).
  • _apply_patch_no_git now returns a PatchApplyResult.

Blocking issues: 1

  1. [V5-merge-conflict] The branch conflicts with main, and the rebase must migrate main's new call site [verified]
    Problem: git merge-tree --write-tree origin/main ce8d6726b reports content conflicts in src/hyperloom/orchestrator/actions/executors/baseline.py and scripts/code_metrics_baseline.txt. Main's #1961 (3153699) split _apply_warm_patches into helpers. Main's no-git call now sits at baseline.py:1289 and still unpacks the old tuple: ok, err, backups, _feedback = _apply_patch_no_git(...).
    Impact: CI cannot run on a conflicting branch. If a resolution only clears the conflict markers and keeps main's structure, every no-git warm replay raises TypeError, because a PatchApplyResult cannot be unpacked.
    Action:
    • Rebase onto current main.
    • Migrate baseline.py:1289, and the method = ... if backups else ... line after it, to PatchApplyResult.
    • Re-run the code-metrics baseline update.
    • Refresh the rebase point in the description: it names 8a4171d, but the actual parent was cc71f34.

The earlier blocker ([X2-stale-description] at 5a5d3a3) is resolved. The description now matches the code at ce8d672: the merged-PR-head exit, the Critic-priors and semantic_status changes and the -3 path are gone, and the figures and line references are correct. CI was not approved on this head because of the conflict; it will be approved after the rebase.

Checked: the full diff cc71f34..ce8d672 (integrate_patch.py, _git_patch.py, _nogit_patch.py, _apply_feedback.py, patch_safety.py, baseline.py, the tests and the docs), and main's baseline.py at a4bdd38. On CI safety: the tests use only local tmp_path git repos and monkeypatched fetch and bench calls, and the new probes are read-only dry runs | Ran: git merge-tree --write-tree --name-only origin/main ce8d6726b (exit 1, two files) | Head: ce8d672

@okorzh-amd
okorzh-amd force-pushed the okorzh/already-present-outcome branch from ce8d672 to 223ec01 Compare October 22, 2026 16:47
@okorzh-amd

Copy link
Copy Markdown
Contributor Author

PR #1845 -- fix(integrate_patch): end a patch set the tree already holds as already_present

What it does: when every patch of a set reverse-applies cleanly, the tree already holds the change. integrate_patch now ends such a set as already_present instead of failing it as git_apply_failed, through _end_patch_series and _end_already_present.

The PR also restructures the apply path:

  • The git apply channel moves into _git_patch.py.
  • _apply_patch_series returns a _PatchSeries with changed_nothing.
  • The no-git reverse probe is anchored on the preflight's patch parser (_reverse_probe_levels).
  • _apply_patch_no_git now returns a PatchApplyResult.

Blocking issues: 1

  1. [V5-merge-conflict] The branch conflicts with main, and the rebase must migrate main's new call site [verified]
    Problem: git merge-tree --write-tree origin/main ce8d6726b reports content conflicts in src/hyperloom/orchestrator/actions/executors/baseline.py and scripts/code_metrics_baseline.txt. Main's refactor(baseline): name how a run applies its patches, and the failures that end it #1961 (3153699) split _apply_warm_patches into helpers. Main's no-git call now sits at baseline.py:1289 and still unpacks the old tuple: ok, err, backups, _feedback = _apply_patch_no_git(...).
    Impact: CI cannot run on a conflicting branch. If a resolution only clears the conflict markers and keeps main's structure, every no-git warm replay raises TypeError, because a PatchApplyResult cannot be unpacked.
    Action:

    • Rebase onto current main.
    • Migrate baseline.py:1289, and the method = ... if backups else ... line after it, to PatchApplyResult.
    • Re-run the code-metrics baseline update.
    • Refresh the rebase point in the description: it names 8a4171d, but the actual parent was cc71f34.

The earlier blocker ([X2-stale-description] at 5a5d3a3) is resolved. The description now matches the code at ce8d672: the merged-PR-head exit, the Critic-priors and semantic_status changes and the -3 path are gone, and the figures and line references are correct. CI was not approved on this head because of the conflict; it will be approved after the rebase.

Checked: the full diff cc71f34..ce8d672 (integrate_patch.py, _git_patch.py, _nogit_patch.py, _apply_feedback.py, patch_safety.py, baseline.py, the tests and the docs), and main's baseline.py at a4bdd38. On CI safety: the tests use only local tmp_path git repos and monkeypatched fetch and bench calls, and the new probes are read-only dry runs | Ran: git merge-tree --write-tree --name-only origin/main ce8d6726b (exit 1, two files) | Head: ce8d672

@chaojhou Thanks. The branch is rebased onto main 8b0ae55 (origin/main when I rebased), and the new head is 223ec01. The old head ce8d672 sat on cc71f34, not on 8a4171d as the description said. The description now names the real parent.

  • Conflict and baseline.py: main's no-git warm replay, now in _apply_warm_patch_sequence, reads the PatchApplyResult at baseline.py:1289, and the method line at baseline.py:1298 now reads present_by instead of backups (6d06eb9). The same commit adds test_warm_patch_apply.py:823, which replays a patch the tree already holds through that path and expects already_present. Putting the old 4-tuple unpack back fails 9 tests there, :823 among them.
  • refactor(integrate-patch): give each stage of an integration attempt a module of its own #2098 also split integrate_patch.py, so I moved the change into main's modules instead of adding _git_patch.py. The git-channel changes are in _patch_git.py: _PatchSeries at :221 and _apply_patch_series at :237. The exit is in _integrate_apply.py: _end_patch_series at :310 and _end_already_present at :333.
  • Code-metrics baseline: I took main's file and re-ran --update-baseline (223ec01). Only 2 entries change, both lowered: _apply_patch_no_git 141 -> 140 and baseline.py module-lines 4050 -> 4048. Nothing was added or raised, and the gate passes against origin/main.
  • Tests: in the full clean pytest run, every test id that fails on this head also failed on main 8b0ae55, and all of them are environment-only. Two GPU-handoff tests failed on main in one of its two runs and passed in the other; they fail whenever another process holds GPU memory, and pass when run alone on both trees. The targeted files give 481 passed and 1 skipped (466 passed on main). The description is updated with the new SHAs, line numbers and figures; I will paste it.

okorzh-amd and others added 4 commits October 22, 2026 21:22
…dy_present

A patch set whose every patch reverse-applies cleanly was benchmarked as a
new candidate. Both apply channels now return a named PatchApplyResult that
says when a patch was already present, and _apply_patch_series returns a
_PatchSeries that records each such patch and whether the set changed
nothing. When it changed nothing and the round changes nothing else
(server args, env, artifacts, setup commands or an acquired runtime), the
executor ends the round as already_present without a benchmark, writing
OUTCOME_ALREADY_PRESENT through the framework-KB writeback. One present
patch inside a set that also changes the tree still lands and is measured.

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. The
baseline warm replay reads present_by instead of inferring presence from
an empty backup set, and a test replays a patch a non-git tree already
holds through it. The phase summary counts already_present as tested, and
the operator docs describe the outcome.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…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) <noreply@anthropic.com>
…tity

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) <noreply@anthropic.com>
…rinks

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@okorzh-amd
okorzh-amd force-pushed the okorzh/already-present-outcome branch from 223ec01 to c5bee6e Compare October 23, 2026 02:24
@chaojhou
chaojhou dismissed their stale review October 23, 2026 02:33

Resolved at the rebased head; superseded by the re-review.

@chaojhou
chaojhou dismissed stale reviews from themself October 23, 2026 02:33

Resolved at the rebased head; superseded by the re-review.

@chaojhou chaojhou left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #1845 -- fix(integrate_patch): end a patch set the tree already holds as already_present

What it does: when every patch in a candidate's set reverse-applies cleanly and the round changes nothing else, integrate_patch now ends with already_present. Before, it failed the set as git_apply_failed. The new ending also skips the restart, the benchmark and the eval on an unchanged tree.

  • Both apply channels return a PatchApplyResult whose present_by names the check that found the patch.
  • _apply_patch_series returns a _PatchSeries with changed_nothing.
  • _end_patch_series and _end_already_present (in _integrate_apply.py) write the KB already_present row and empty attempt.applied.
  • The reverse probes try only the -p levels where a header path resolves.
  • Warm replay reads present_by instead of inferring presence from an empty backup list.

Blocking issues: none

The previous blocker ([V5-merge-conflict] at ce8d672) is resolved at 223ec01:

  • The branch sits on 8b0ae55 and merges cleanly with current main 7f1c5e8.
  • Main's no-git warm-replay call at baseline.py:1289 reads the PatchApplyResult, and :1298 now reads present_by. No tuple unpack of _apply_patch_no_git, _git_apply_collect_feedback or _apply_patch_series remains in the merged tree.
  • The code-metrics baseline only lowers two entries.
  • The description names the real rebase point.

The change moved into main's split modules (_patch_git.py, _integrate_apply.py) with the same behaviour as before the move.

The 02:24 push only rebased onto 267a0f4; git range-diff shows all four commits unchanged from 223ec01, which this review covered. It merges cleanly with current main, and CI was approved at this head.

Checked:

  • every caller of the changed signatures and every consumer of already_present, at head and in the merge with main;
  • the description's line references and figures.

Ran:

  • git merge-tree --write-tree origin/main 223ec0129 (clean);
  • the 7 changed test files plus the patch-safety, warm-replay and source-PR suites on head, on main 8b0ae55 and on the merged tree. The failing test ids are identical across all three and are Windows-only.

Head: c5bee6e

LGTM

@chaojhou
chaojhou merged commit e99ec91 into AMD-AGI:main Oct 23, 2026
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants