Skip to content

test: union and dedup prior refs in merge_staged_sections (PRPUNDIT-14) - #1329

Merged
chaojhou merged 5 commits into
AMD-AGI:mainfrom
jiagaoxiang:testgap/PRPUNDIT-14
Sep 1, 2026
Merged

test: union and dedup prior refs in merge_staged_sections (PRPUNDIT-14)#1329
chaojhou merged 5 commits into
AMD-AGI:mainfrom
jiagaoxiang:testgap/PRPUNDIT-14

Conversation

@jiagaoxiang

Copy link
Copy Markdown
Collaborator

Summary

  • Stages a framework patch onto a section that already holds replayed refs and locks union/dedup order.

Closes test gap PRPUNDIT-14.

Test plan

  • PYTHONPATH=src pytest src/hyperloom/inference_optimizer/tests/test_remote_recipe_v2.py::test_merge_staged_sections_unions_and_dedups_prior_refs

Stage a framework patch onto a section that already holds replayed refs and lock union/dedup order.
@jiagaoxiang
jiagaoxiang requested a review from a team as a code owner August 28, 2026 07:13

@zoroyihan7 zoroyihan7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks — this is the right target, and it does pin dedup, insertion order, and before-preservation.

One problem though: the union half — the actual point of the branch — isn't pinned.

The test makes after a subset of before:

value = {"framework": {"patches": [prior_patch, staged_ref], ...}}   # before
# staged.knowledge["patches"] == [staged_ref]                        # after

so the expected result [prior_patch, staged_ref] is byte-identical whether the code unions, ignores after entirely, or skips the branch. I confirmed by mutation at values.py:1054:

if not before and after:      # was: if before or after:

i.e. newly staged refs are silently discarded whenever the section already holds prior replayed refs. 478 tests stay green, including this one.

That mutation is worth taking seriously because in production it's silent data loss rather than an error: files.adopt() has already copied the staged patch into the files tree, the ref never lands in value, and files.prune_superseded(knowledge) (values.py:1200) then deletes the orphaned file — so a newly authored patch disappears from the published recipe with no exception raised.

The subset shape is also the opposite of the real producer shape: _adopt_replayed_prior (values.py:854-867) writes refs at the replay overlay index while stage_patches writes at the stack index, so in production before and after are normally disjoint and the append is the whole reason the branch exists.

Fix is small — stage two patches and duplicate only one into before. That gives you union, dedup and order in a single case:

before = [prior_patch, staged_ref_a]      # prior + one overlapping
after  = [staged_ref_a, staged_ref_b]     # overlapping + one new
expect = [prior_patch, staged_ref_a, staged_ref_b]

Two minor notes, non-blocking:

  • prior_artifact = "framework/artifacts/prior.bin" exercises a shape no real producer creates — _adopt_replayed_prior only ever writes value[owner]["patches"], never "artifacts". Still a valid pin of the generic per-key loop, just noting the docstring's framing only strictly applies to the patches half.
  • Asserting exact insertion order is slightly stronger than the downstream contract, since _patch_timeline re-sorts by (stack_index, member_index). Fine to keep — just an implementation-order lock rather than an observable-behavior one.

@zoroyihan7 zoroyihan7 added the skip-e2e-test It's a PR that doesn't need to be e2e tested label Aug 31, 2026
jiagaoxiang and others added 2 commits August 31, 2026 19:47
… overlap one

Stage two patches and put only one (staged_ref_a) in 'before'; after (sections)
holds both. Mutation 'if not before and after:' at values.py:1054 now turns this
red. The subset shape also matches the real producer: disjoint stack vs replay
indices mean before and after are normally disjoint in production.

Addresses zoroyihan7 review on PR AMD-AGI#1329.
… are explicit

The prior follow-up already turns the `if not before and after:` mutation
red; this makes the reviewer-requested before/after shape visible in the
test rather than implied by stage_patches.
@jiagaoxiang

Copy link
Copy Markdown
Collaborator Author

Follow-up for the union-half pin: before = [prior, staged_ref_a], after = [staged_ref_a, staged_ref_b], expect [prior, a, b]. Confirmed locally that mutating if before or after:if not before and after: at values.py:1054 now fails this test (drops staged_ref_b).

@zoroyihan7 zoroyihan7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed. The union shape is fixed and now genuinely pinned — before = [prior, staged_a], after = [staged_a, staged_b] — and asserting after itself as a precondition is a nice touch.

Re-ran the mutations on values.py:1055; all four are red now:

  • if before or after:if not before and after: (the silent-data-loss one) — red
  • dedup removed (list(dict.fromkeys(...))list(...)) — red
  • [*before, *after][*after]red
  • operands reversed — red

123 passed, no regressions. LGTM.

@jiagaoxiang
jiagaoxiang enabled auto-merge (squash) September 1, 2026 03:35
@zoroyihan7 zoroyihan7 added the retest Re-run E2E smoke without a new commit (runs once) label Sep 1, 2026
@zoroyihan7

Copy link
Copy Markdown
Contributor

Heads-up — I updated this branch from main so it would pick up the skipped-status fix from #1357, and that surfaced a break. skipped-status is green now, but ruff went red:

F821 Undefined name `FrameworkAgentKB`
 --> src/hyperloom/inference_optimizer/tests/test_remote_recipe_v2.py:2020

FrameworkAgentKB was removed somewhere in the 207 commits this branch was behind — it doesn't exist anywhere on main any more. It's now PatchKB in orchestrator/knowledge/agent_kb.py, and the pre-existing test right above yours (line 2002) already uses it:

refs = PatchKB(sections).stage_patches(...)

So it should be a one-word change on line 2020:

staged_refs = PatchKB(sections).stage_patches([patch_a, patch_b], stack_index=2)

PatchKB is already imported in the file post-merge, so nothing else should need touching. I confirmed it's a real runtime break, not just lint — locally the test errors with NameError: name 'FrameworkAgentKB' is not defined (1 failed, 125 passed).

I ran the other four updated branches too and they're all clean (#1324 51 passed, #1327 17, #1328 74, #1330 44) — this is the only one the merge caught out.

Happy to push the rename myself if you'd rather (maintainerCanModify is on) — just say the word, otherwise it's yours.

Merging main into this branch surfaced two renames that landed in the 207
commits it was behind:

  FrameworkAgentKB -> PatchKB   (orchestrator/knowledge/agent_kb.py)
  section "framework" -> "patch" (PatchKB.SECTION)

The test failed with NameError, then KeyError, before this. Re-verified that
it still pins the producer after the rename: dropping the staged side
(after = []), skipping the union loop (current = {}), removing dedup, dropping
before, and reversing operand order each turn it red.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@zoroyihan7

Copy link
Copy Markdown
Contributor

Pushed the fix to this branch (acf74f665) — it turned out to be more than the one-word rename I described above, so here's exactly what changed and how I checked it.

Two renames landed in the 207 commits this branch was behind, not one:

was is now
FrameworkAgentKB PatchKB (orchestrator/knowledge/agent_kb.py)
section "framework" section "patch" (PatchKB.SECTION)

So after fixing the class name the test still failed, first with NameError, then AttributeError: 'NoneType' object has no attribute 'knowledge' (because sections.staged("framework") no longer resolves), then KeyError: 'framework'. The diff is 8 lines, all inside your test — the class name, the two prior_* ref prefixes, the staged(...) lookup, the value key, and the three assertions.

merge_staged_sections itself was also refactored on main, which matters for what the test is pinning. It used to be:

combined = {...before...}
if before or after:
    combined[ref_key] = list(dict.fromkeys([*before, *after]))

and is now seeded from the staged side instead:

combined = dict(staged.knowledge)
for ref_key, before in current.items():
    after = combined.get(ref_key, [])
    ...
    combined[ref_key] = list(dict.fromkeys(str(ref) for ref in [*before, *after] if str(ref)))

I re-ran the mutation set against the new shape to make sure the test still catches a regression rather than just passing:

  • after = [] (drop the newly staged side — the silent-data-loss direction) — red
  • current = {} (skip the union loop, drop prior refs) — red
  • dedup removed — red
  • [*before, *after][*after]red
  • operands reversed — red

126 passed, ruff check and format both clean.

One thing I deliberately left alone: the comment above after_patches still describes the old if not before and after: shape, which no longer exists. The test's behaviour is right, but that comment now points at code that isn't there. Wanted you to word that one rather than guess at it.

The other four branches I updated are unaffected — #1324, #1327, #1328 are all green and #1330 is nearly there.

@chaojhou
chaojhou disabled auto-merge September 1, 2026 10:10
@chaojhou
chaojhou merged commit 1462134 into AMD-AGI:main Sep 1, 2026
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

retest Re-run E2E smoke without a new commit (runs once) skip-e2e-test It's a PR that doesn't need to be e2e tested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants