Repository navigation
fix(integrate_patch): end a patch set the tree already holds as already_present - #1845
Conversation
9e8e41c to
fcbcb4a
Compare
chaojhou
left a comment
There was a problem hiding this comment.
Thanks @okorzh-amd. No blockers:
- A set ends
already_presentonly when every patch is fully present and nothing errored. - Both channels' reverse checks fail closed.
- The extracted
apply_patch_setmatches the old loop.
Two must-fix items:
-
The merged-PR-head exit skips the "round changes nothing else" check.
integrate_patch.py:2042-2043ends the attempt asalready_presentin_stage_resolve. That is before_stage_applyreadsconfig_changes,extra_server_args,extra_envs,artifactsor the enablement setup.changes_beyond_patches(:2419,:2704) guards only the patch-set exit.- Example: an
upstream_prtask for a merged PR with nodiff_url, which also carriesextra_server_argsto turn on the PR's opt-in switch. It endsalready_presentand writes the KB row, and the arg change is never measured. The same candidate with adiff_urlis 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.
-
The description predates the rebase onto #1858.
- Main no longer has a
git apply -3retry; #1858 removed it. So three statements describe nothing this PR changes: "runs the reverse check before the-3retry", "reports a committed patch present instead of three-way merging it" and "no longer goes throughgit apply -3". - The complexity numbers are pre-rebase. At head:
_stage_applyis cyclomatic 53→45, cognitive 131→110, and 513→470 lines;integrate_patch.pyis 4804→4703 lines;- the baseline edits lower 9 entries and remove 1.
- Please drop the
-3statements and refresh the numbers.
- Main no longer has a
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.
|
Gate job conclusion: success (run) on The report below was uploaded by that run; the conclusion above is the result. Code metrics gate: PASSEDJudged: 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.
Module length warning: over 800 lines, within the 1200-line failure limit (not failing): 1
Already over at the base (backlog, not this change's): 2
66 findings in files this change does not touchOutside this change (informational): 66
...and 41 more; the job log lists every row.
Thresholds and sources: |
chaojhou
left a comment
There was a problem hiding this comment.
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
- [X2] The PR description still describes a
git apply -3path 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-3retry", that it "reports a committed patch present instead of three-way merging it", and that "a fully present patch no longer goes throughgit apply -3". The git channel,_git_apply_collect_feedback, has no-3path at base 1a444f6 or at head. The body also gives_stage_applycyclomatic 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-3statements, 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
left a comment
There was a problem hiding this comment.
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
-
[S4] A drifted deletion-only patch in mail format still reads as present, so the round ends
already_presentwhile the file is untouched [verified]
Problem:_reverse_probe_levelsfalls back to every-plevel whenparse_patch_manifestraisesValueError. Agit format-patchdeletion (mail header before the diff) raises there, while the preflight parser reads it (patch_file_targetsgives[('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 whosemod/alpha.pydrifted from the patch's pre-image,_git_apply_collect_feedbackreturnsok=True, present_by=GIT_REVERSE_APPLYand the file is left as it was, soapply_patch_setreportschanged_nothingand the round writes analready_presentKB 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 theValueErrorbranch; add a mail-format case to the drifted-deletion test. -
[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 "agit apply -3of a patch the tree already holds ...", "The git channel runs the reverse check before the-3retry", "reports a committed patch present instead of three-way merging it" and "no longer goes throughgit apply -3", though neither base nor head has a-3path; 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-3statement, 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:
- Fix the probe (item 1) and add the mail-format test; delete
:328. - Delete the merged-PR-head exit: revert
_patch_source_pr.pyand_stage_resolve_upstream_prto base, dropPR_HEAD_FILES,_materialize_upstream_prand_RoundProposal, and name the existing predicate once inside_stage_apply; delete its tests and doc sentences. - Trim the tests listed in point 5.
- Keep one encoding of "nothing applied" in
_end_already_present. - Move the
semantic_statuscleanup to its own PR. - Rewrite the description (item 2), and state the
no_promotejournalling, the enablement stall, and that a deletion already made upstream endspatch_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
945b0cb to
5a5d3a3
Compare
chaojhou
left a comment
There was a problem hiding this comment.
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
- [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
5a5d3a3 to
033cb06
Compare
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:
The branch is rebased onto main 7ac8c12. Main's split of |
f7371ff to
ce8d672
Compare
chaojhou
left a comment
There was a problem hiding this comment.
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_seriesreturns a_PatchSerieswithchanged_nothing.- The no-git reverse probe is anchored on the preflight's patch parser (
_reverse_probe_levels). _apply_patch_no_gitnow returns aPatchApplyResult.
Blocking issues: 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 ce8d6726breports content conflicts in src/hyperloom/orchestrator/actions/executors/baseline.py and scripts/code_metrics_baseline.txt. Main's #1961 (3153699) split_apply_warm_patchesinto 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 raisesTypeError, because aPatchApplyResultcannot be unpacked.
Action:
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
ce8d672 to
223ec01
Compare
@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.
|
…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>
223ec01 to
c5bee6e
Compare
Resolved at the rebased head; superseded by the re-review.
Resolved at the rebased head; superseded by the re-review.
chaojhou
left a comment
There was a problem hiding this comment.
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
PatchApplyResultwhosepresent_bynames the check that found the patch. _apply_patch_seriesreturns a_PatchSerieswithchanged_nothing._end_patch_seriesand_end_already_present(in_integrate_apply.py) write the KBalready_presentrow and emptyattempt.applied.- The reverse probes try only the
-plevels where a header path resolves. - Warm replay reads
present_byinstead 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 readspresent_by. No tuple unpack of_apply_patch_no_git,_git_apply_collect_feedbackor_apply_patch_seriesremains 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
Description: what and why
integrate_patchrestarted 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 --checkon a git checkout, andpatch -R --fuzz=0 --dry-runon 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_PRESENTwas an allowed framework-KB outcome that nothing wrote.Everything below describes head
223ec0129, rebased ontomainat8b0ae5500(origin/mainwhen the rebase was made). The previous head,ce8d6726b, sat oncc71f343b; the earlier description named8a4171dafthere by mistake.How
mainshaped this PR:_stage_applyinto_apply_patch_series, and thegit_apply_failedrefusal into_refuse_apply.integrate_patch.py(now 136 lines) into one module per stage. The apply stage (_stage_apply,_refuse_apply) isApplyStageMixinin_integrate_apply.py. The git apply channel (_run_git_apply,_git_reverse_applies_cleanly,_git_apply_collect_feedback,_apply_patch_series) is in_patch_git.py. This PR changes those functions wheremainput them. It no longer adds a_git_patch.pyof its own, and it does not touchintegrate_patch.py._apply_warm_patchesinbaseline.pyinto helpers. The no-git warm replay now sits in_apply_warm_patch_sequence, and this PR migrates its call there.What changes:
_apply_patch_no_gitused to return a 4-tuple and the git channel's_git_apply_collect_feedbacka 3-tuple. Both now return a frozenPatchApplyResultwith fieldsok,error,backups,feedbackandpresent_by.present_byis aPresenceCheck, eithergit_reverse_apply_checkorpatch_reverse_dry_run(_apply_feedback.py:19-25,:77-88).main's_apply_patch_seriesnow reads each patch'sPatchApplyResultfrom either channel and records a{patch, check}row for each patch whosepresent_byis set. It returns a_PatchSeriesinstead ofmain's(errors, feedback)tuple (_PatchSeriesat_patch_git.py:221-234,_apply_patch_seriesat:237-284).changed_nothingholds 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._patch_git.py,_git_apply_collect_feedback(:145-180) returnsPatchApplyResultand builds its failures through the new_git_apply_failure(:131-142)._git_reverse_applies_cleanly(:104-128) probes only the levels_reverse_probe_levelsnames (see below). It drops the two function-local imports of_P_LEVELSthatmainkept next to the module-level one.main's no-patches predicate gets a name,_changes_beyond_patches(attempt, deliverable)(_integrate_apply.py:44-52), which readsmain'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_applyhands the series to_end_patch_seriesright after_apply_patch_seriesand before artifact install (:197-205)._end_patch_series(:310-331) holds both ways a series ends the round:main'sgit_apply_failedrefusal through_refuse_apply, and the new exit.changed_nothing, in a round with no_changes_beyond_patches, ends there through_end_already_present(:333-361)._end_already_presentwrites the KB row and emptiesattempt.applied, the list that_finish_attempt(integrate_patch.py:96) reports aspatches_reverted.-plevel only where some path named in the patch's---/+++headers resolves (_nogit_patch._reverse_probe_levels,:92-114).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 frompatch_targets_missing(specialists/patch_safety.py:221-240).a/mod/alpha.pyat-p0), even though its target still existed with other content. This held for a plain diff and forgit format-patchoutput.already_presentas tested (framework/artifacts.py:13-15).present_by, where it used to infer "already present" from an empty backup list (baseline.py:1298, inmain's_apply_warm_patch_sequence). Its no-git call reads the result'sok,errorandbackupsinstead of unpackingmain's 4-tuple, which would now raiseTypeError.OUTCOME_ALREADY_PRESENTcomment used to credit a semantic audit, which never wrote the outcome. It now says what the outcome means (knowledge/kb_writeback.py:25).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.pyandphases/framework.pyare the same as onmain. An upstream PR head with nodiff_urlis handled exactly as onmain(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 onmain.Executor tests on a temporary tree. The benchmark is replaced by
pytest.failunless a test records that it was reached.:367: a set whose every patch is already in the tree endsalready_present.upstream_prround and a specialist-authored round on a git checkout, and for anupstream_prround on a non-git tree.{patch, check}row per patch, naming the channel's check, plus emptypatches_appliedandpatches_reverted.already_presentrow is written. The tree is unchanged: file contents, and on the checkout also HEAD andgit status.:454: a PR merged into the tree, with adiff_url. The empty head diff falls back to thediff_urldiff, and the round endsalready_presentthroughgit_reverse_apply_check.:469: the same PR merged and then reverted is applied and benchmarked, and noalready_presentrow is written.:485: a set already in the tree, in a round that also setsextra_server_args, is benchmarked.:501: a no-op patch that rides with a real change lands with it. The result isapplied_no_benchwith 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 asgit format-patchoutput (mail header before the diff).:264: a deletion-only patch whose file is already gone is present at the channel level. Inintegrate_patchthe 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_patchesandmain's_apply_warm_patch_sequence. It ends with statusalready_present, the file unchanged and no backups.What each guard pins. Each was checked by breaking it at this head:
:367cases and:454;not _changes_beyond_patches(...)from the exit fails:485;changed_nothingtrue when any patch is present fails:501;attempt.applied.clear()fails the 3:367cases;parse_patch_manifestand probed every level when that parser raised, fails bothformat_patchcases of:248;:248;main's 4-tuple unpack atbaseline.py:1289fails 9 tests intest_warm_patch_apply.py,:823among them.Added elsewhere:
src/hyperloom/inference_optimizer/tests/test_framework_agent_selection_audit.py:133. Analready_presentrow counts as tested in the phase summary.Updated:
test_patch_already_applied_unit.py: the six tests frommain(:103,:125,:142,:153,:179,:199) now read the result's fields.:103,:125,:179and:199also assertpresent_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.:225assertspresent_byin both cases. It ispatch_reverse_dry_runwhen the prepare record persists, and unset on the failure when it cannot persist.:431replaces the 4-tuple length assertion with field assertions, and:456drops its skip on a tuple of another length.:28keepsmain's re-export identity checks unchanged.patch_git._apply_patch_no_git is ng._apply_patch_no_gitstill holds, because_patch_gitimports the function.test_integrate_patch_executor.py:2054reads the result's fields. The "vet before any apply" test (:2662) is unchanged: it still patches_patch_git._git_apply_collect_feedback, which is wheremain's loop looks the function up.test_integrate_patch_helpers_unit.py:601callspatch_git._git_apply_collect_feedback, reads the result's fields and asserts thatpresent_byis unset on a failed apply.test_delivery_channels.py:72reads the result's fields.Not covered by a test:
extra_server_argsis tested. Every one of these terms feeds the single_changes_beyond_patchesflag, which the existing no-patches return also reads;present_byand not from the backup count. Restoring the empty-backups test atbaseline.py:1298fails nothing, because the two agree for every patch that touches a file (see Removed).:823pins the outcome and the result type, not that choice.Commands run at head
223ec0129:ruff checkandruff format --checkon 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_applyas already over at the base (see Size/complexity).scripts/check_cli_references.py, andscripts/check_broad_suppress.pyon the changed Python files: pass.scripts/pr_hygiene.py size --base origin/main --head HEAD: 361 production Python lines (size/L).pyteston the seven changed test files plustest_patch_source_pr.py,test_specialist_patch_safety_unit.pyandtest_sglang_artifact_fix.py: 481 passed, 1 skipped (466 passed, 1 skipped onmain8b0ae5500; the 15 extra are this PR's new tests).pytest -n 8 -m 'not critic_agent_e2e and not targeted_build_e2e'overorchestrator/actions/executors/tests,orchestrator/enablement/tests,orchestrator/framework,orchestrator/delivery/testsandtest_framework_agent_selection_audit.py: 984 passed, 1 skipped, 3 failed. The same 3 fail onmain8b0ae5500(969 passed):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.-n 24 -k 'not kernel_entry_auto_runs', clean environment), run twice on each tree:main8b0ae5500: 26037 passed and 44 failed in one run, 26039 passed and 42 failed in the other, with 31 skipped and 26 errors in both.mainin at least one of its runs, so this PR adds none. The two ids that came and went onmain,test_geak_resume_recovery.py::test_geak_kernel_phase_does_not_reuse_already_promoted_resultandtest_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.master, or a umask of 022.Size/complexity triggers crossed: split, or why the shape is right:
_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_seriesand_end_already_present, andmain'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.main,_stage_apply(_integrate_apply.py:47;:58at 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 tomain's values, and neither may rise. The exit therefore lives in_end_patch_series. This PR takes_stage_applyto cognitive 32 and 186 lines; its cyclomatic complexity stays at 20.scripts/code_metrics_baseline.txtonly tightens: 2 entries lowered, none added, raised or removed. Each is a unit this PR shrinks:_apply_patch_no_git: 141 → 140 lines;baseline.pymodule: 4050 → 4048 lines.main's baseline._stage_applyand theintegrate_patch.pymodule lost theirs in refactor(integrate-patch): give each stage of an integration attempt a module of its own #2098._apply_warm_patcheswas 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._apply_patch_series, which goes from 8 / 15 and 44 lines onmainto 9 / 17 and 48 lines;_git_apply_collect_feedbackis 6 / 9 and 36 lines (58 lines onmain);_git_apply_failureis 1 / 0, 12 lines;_end_already_presentis 2 / 1, 29 lines;_end_patch_seriesis 3 / 3, 22 lines;_changes_beyond_patchesis 1 / 2, 9 lines;_reverse_probe_levelsis 2 / 9, 23 lines;path_resolves_atis 3 / 3, 20 lines. Extracting it takespatch_targets_missingfrom 9 / 20 and 48 lines to 3 / 7 and 31 lines.patch_safety.pygrows from 1152 to 1158 lines. That is over the 800-line warning, as onmain, 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:
_apply_patch_no_git(4 elements),_git_apply_collect_feedback(3) and_apply_patch_series(2), nowPatchApplyResultand_PatchSeries;present_bydiffer only for a patch that touches no file;patch_targets_missing, nowpath_resolves_at;_stage_apply, now_changes_beyond_patches.Changed where
mainput it (no function changes module):_git_reverse_applies_cleanlyprobes only the levels_reverse_probe_levelsnames;_git_apply_collect_feedbackreturnsPatchApplyResultand builds its failures through_git_apply_failure;main'sgit_apply_failedrefusal moves from_stage_applyinto_end_patch_series, unchanged.Contracts preserved, with the tests that pin them:
error_class(apply_failed/git_apply_failed):test_integrate_patch_executor.py:702.test_integrate_patch_executor.py:2662.test_patch_already_applied_unit.py:103,:179,:501.test_nogit_patch_unit.py:225.test_specialist_patch_safety_unit.py:66-76,orchestrator/enablement/tests/test_sglang_artifact_fix.py:81,test_integrate_patch_executor.py:730.retry_feedbackandprior_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_patchends withstatus: already_present, and no server restart, benchmark or accuracy eval runs. This requires that the round change nothing else:extra_server_args,config_changes,extra_envs, framework-switch envs). Args and envs inherited from earlier rounds do not count;The exit applies to FRAMEWORK upstream-PR candidates, FRAMEWORK and explore specialist patches, and enablement rounds.
What the result contains.
already_presentlist with one{patch, check}row per patch.checkisgit_reverse_apply_checkon a git checkout andpatch_reverse_dry_runon any other tree.patches_appliedandpatches_reverted, and noworkspace.integrate_patch: the framework tree already holds this candidate; not benchmarking: ....The framework KB row.
lessons.jsonlgets a row withoutcome: already_presentandtps_delta_pct: 0when the candidate carries a PR identity. That is an upstream-PR URL or head SHA, or a framework proposal'sfa_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.
failed, likeapply_failedandno_patches.optimization_journal.jsonrecords it asno_promote, and the intervention mix records acode_patch_attempt.keptnoradvanced, so it is charged as a failed round and counts toward the consecutive-stall cap. Nothing boots.already_present, is not kept, and has gain 0;dropped, because the progress stamp settles every status other thankeptandrevertedthat way (phases/framework.py:2074, unchanged);framework_agent_phase_doneevent counts the candidate incandidate_outcomes.already_present(a count per status) and intested, and so does thetested=figure of the "tested candidates but none cleared the gate" advisory;apply_failedandfetch_failed, becausealready_presentis 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 KBreverted_smoke_failrow carrying a noisetps_delta_pct;kept, when the noise cleared the keep threshold. That wrote anintegratedKB row and added anoptimization_stacklayer and acurrent_bestlift 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-patchform:apply_failedwith reauthoring feedback. In warm-recipe replay on a non-git tree, the same overlay reportsfailedwith reasonnogit_apply_failedinstead ofalready_present.An upstream PR merged into the tree.
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 endsalready_present. Before, it was benchmarked.diff_url, an empty head diff still endsfetch_failed(checkout_head_failed), as onmain.Hunks at an offset. On a git checkout,
git apply -R --checkaccepts a hunk at an offset; thepatchchannel 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 endsalready_presentwhere it used to be benchmarked.Unchanged:
apply_failed.apply_failedwitherror_class: patch_target_missing, before any apply.Minor: with
apply_only, a set already in the tree returnsalready_presentwhere it used to returnapplied_no_bench. Only tests setapply_only; no production code or agent prompt does.Breaking changes: no.
already_presentwas already an allowed KB outcome, and thelessons.jsonlfields are unchanged. The only in-repo reader oflessons.jsonlis the config-lever accuracy check. It matches onapplicabilityand a negativeaccuracy_delta_pct. The exit measures no accuracy, so analready_presentrow carries anaccuracy_delta_pctof 0 unless the candidate's own proposal supplies one, which no in-repo producer does.state.jsoncan carry a newstatusvalue, but gain no new field.integrate_patchresult gains a new status and analready_presentkey; both are additive. Writeback, the FRAMEWORK bridge and the enablement lane handle them as described above.main's warm-replay call inbaseline.py:_apply_patch_no_gitand_git_apply_collect_feedbackreturnPatchApplyResult;_apply_patch_seriesreturns_PatchSeries;PR addresses single concern: yes. The refactor is part of the fix:
_stage_applyunder the code-metrics gate, so it lives in_end_patch_series;PatchApplyResultand_PatchSeries;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_statusmapping inphases/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.