fix: re-validate proposed changes at merge time by ajtmccarty · Pull Request #9705 · opsmill/infrahub

ajtmccarty · 2026-06-24T21:28:06Z

Problem

Merging two branches that each introduce the same unique value (e.g. a BuiltinTag named orange) can leave duplicate values on the destination branch.

A proposed change validates its schema and data integrity during its check pipeline, but those run once (point-in-time) and the merge relies on the stored validator conclusions. So:

PC for branch A and PC for branch B both run their checks while main is clean → both pass.
A is merged → main now has the value.
B is merged → it reuses its stale "passing" schema-integrity conclusion and merges anyway → duplicate.

This is a time-of-check/time-of-use gap. It exists for constraints (uniqueness, cardinality, …) specifically because a constraint is not a diff conflict — nothing re-detects it after another branch merges. (Data conflicts do not have this problem; see below.)

Fix

Move constraint validation into the shared merge flow so it runs against the current state of the destination branch and gates both the proposed-change merge and the direct BranchMerge mutation.

MergeConstraintValidator (backend/infrahub/core/merge/constraints.py) — a component constructed with (db, branch, diff_repository) and injected into BranchMerger. Its single entry point validates the union of data-diff-derived constraints (the determiner over get_node_field_summaries, which is relationship-correct) and schema-diff-derived constraints, runs schema_validate_migrations, and returns the surviving violations.
Conflict-aware suppression — a violation on a (node, field) that has a resolved diff conflict is dropped. That field is reconciled by the conflict resolution applied during the merge, so flagging its pre-resolution cross-branch state would be a false positive (this is what made the earlier attempts misfire on cardinality-one conflicts — see below).
BranchMerger.merge() runs two gates before mutating the graph (so a rejection needs no rollback): unresolved conflicts → MergeConflictsUnresolvedError; constraint violations → MergeConstraintsViolatedError. Both subclass ValidationError (HTTP 422).
merge_proposed_change records a MergeConstraintsViolatedError on the Schema Integrity validator so the proposed change reflects the failure (instead of staying green while the merge is rejected), then returns a failed state. merge_branch_mutation drops its now-redundant inline determiner validation — the merge flow covers it.
Relationship constraints in the pipeline check — _get_proposed_change_schema_integrity_constraints compared the SDK diff-summary element_type ("RELATIONSHIP_ONE") against DiffElementType values ("RelationshipOne") instead of member names, so it silently dropped every relationship element. It now compares against member names. This keeps the proposed-change UI feedback accurate; the merge gate itself does not depend on it (it uses the backend diff summary).

Data conflicts are already handled (and unaffected)

The symmetric data case — branch B's PC is green, branch A merges a conflicting change to the same node/field, B now conflicts — is already handled by the existing merge logic, which updates a diff to the latest timestamp before merging. The diff update will identify any new conflicts and mark them as data integrity violations on the linked proposed change, if it exists.

Approaches considered and superseded

Inline validation in BranchMerger.merge without conflict-awareness (first attempt, reverted): fixed the TOCTOU but false-positived on cardinality-one relationship conflicts — the relationship-count check counted both peers (2 > max_count 1) before the merge applied the resolution. The conflict-aware suppression in this PR is the correct version of that idea.
Re-running the proposed-change integrity checks at merge (intermediate, superseded by this PR): re-executed two of the pipeline's checks out of band right before merging. It worked for uniqueness but was a bolt-on, only covered the proposed-change path (not the direct mutation), and its data-side half was both redundant (the post-merge diff refresh already handles data) and ineffective (the synchronizer short-circuits on a metadata diff with an existing validator). Validating in the shared merge flow is a single gate that covers both entry points.

Testing

backend/tests/integration/proposed_change/test_proposed_change_uniqueness_merge.py:

TestProposedChangeUniquenessMerge — the reported bug via the proposed-change path: the second merge is blocked, main keeps a single orange tag, and the Schema Integrity validator is FAILURE.
TestDirectMergeUniqueness — the same collision via the direct BranchMerge mutation, proving the unified gate covers that path.
TestProposedChangeDataConflictMerge — cross-branch data conflict (default config): blocked by the stored data-check gate, Data Integrity validator FAILURE, main unchanged.
TestProposedChangeDataConflictMergeBackstop — same, with diff_update_after_merge=False: blocked by the merge-time conflict backstop.

Also covered: test_proposed_change_cardinality_one_conflict (a resolved cardinality-one conflict stays mergeable), test_schema_integrity_relationship_constraints (regression guard for the element_type fix), and test_merge_relationship_count_validation (cross-branch cardinality-one never leaves two peers on main).

Regression suites run locally (all green): test_diff_update (incl. the cardinality-one case that broke the first attempt), test_branch_merge, test_coordinator_lock.

Notes / follow-ups

A constraint violation surfaces the generic Unable to merge proposed change containing failing checks; the detailed violation lives on the Schema Integrity validator's check.
Separate, pre-existing gap (not addressed here): constraint validation runs against the merging branch's data, but a data-conflict resolution can retain the destination's value, which is never re-validated against the newly-merged schema. So tightening a constraint on one branch while the value is resolved to the other side can let illegal data through. Worth a dedicated issue — the proper fix is validating the post-resolution state. IFC-2838
Orphaning a mandatory cardinality-one relationship by deleting its peer node is not caught by the relationship-count constraint (the change isn't attributed to the dependent node in the diff). Pre-existing, separate gap. IFC-2837

cubic-dev-ai

No issues found across 4 files

Confidence score: 5/5

Automated review surfaced no issues in the provided summaries.
No files require special attention.

_{Shadow auto-approve: would require human review. Moves constraint validation into core merge flow, affecting data integrity on all branch merges. Requires human review to ensure no unintended side effects.

Re-trigger cubic}

codspeed-hq · 2026-06-24T21:35:05Z

Merging this PR will not alter performance

✅ 12 untouched benchmarks

_{Comparing ajtm-06242026-uniqueness-into-merge-flow (9ced511) with stable (ea9a658)¹}

No successful run was found on stable (f723d79) during the generation of this report, so ea9a658 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

Merging two branches that each introduced the same unique value could leave duplicate values on the destination branch. A proposed change validated schema and data integrity during its pipeline but merged using the stored validator conclusions, so a violation introduced after those checks ran (e.g. another branch merging the same unique value) was not caught. - Re-run the data and schema integrity checks against the current destination state immediately before merging a proposed change. - Fix the proposed-change schema-integrity check silently skipping all relationship constraints: it compared the SDK diff-summary element_type against DiffElementType values ("RelationshipOne") instead of member names ("RELATIONSHIP_ONE"). - Skip schema-integrity violations on a node/field that has a diff conflict; those are reconciled by the data-conflict resolution applied during the merge. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

cubic-dev-ai

0 issues found across 1 file (changes from recent commits).

_{Shadow auto-approve: would require human review. Modifies proposed change merge validation to re-run integrity checks and fix relationship constraint detection; high-impact logic change in a critical path.

Re-trigger cubic}

…alues Merging two branches that each introduced the same unique value could leave duplicate values on the destination branch. Each proposed change validated its schema integrity earlier in its pipeline, but the merge relied on those point-in-time results, so a violation introduced by another branch merged in the meantime was missed (a time-of-check/time-of-use gap). Constraint validation now runs inside the shared merge flow, gating both the proposed-change merge and the direct BranchMerge mutation against the current state of the destination branch: - New MergeConstraintValidator (core/merge/constraints.py), injected into BranchMerger, validates data- and schema-diff-derived constraints and suppresses violations on a field that has a resolved diff conflict (so a cardinality-one conflict resolution is not flagged as a violation). - BranchMerger.merge() runs the constraint and unresolved-conflict gates before mutating the graph; violations raise MergeConstraintsViolatedError / MergeConflictsUnresolvedError (HTTP 422). - merge_proposed_change records a constraint violation on the Schema Integrity validator so it reflects the failure; merge_branch_mutation drops its now redundant inline validation. - The proposed-change schema-integrity check now compares the SDK diff summary element_type against DiffElementType member names, so relationship constraints are no longer silently skipped. Data conflicts are unaffected: they are already re-synchronized by the post-merge diff update, with the merge-time conflict gate as a backstop. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

cubic-dev-ai

0 issues found across 10 files (changes from recent commits).

_{Shadow auto-approve: would require human review. Changes modify core merge logic, add a new constraint validator, and touch the proposed change pipeline. These are high-impact, critical-path changes that require human review to ensure correctness and avoid subtle bugs.

Re-trigger cubic}

cubic-dev-ai

1 issue found across 10 files (changes from recent commits).

_{Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic}

cubic-dev-ai

0 issues found across 2 files (changes from recent commits).

_{Shadow auto-approve: would require human review. Significant changes to merge validation logic and constraint handling; high impact on data integrity and merge behavior.

Re-trigger cubic}

polmichel

LGTM, nice fix and it really pleasing to see how the MergeConstraintValidator just gets injected into the BranchMerger thanks to your new architecture around the merge process.

I've put optional comments and a few thoughts, nothing blocking from a correctness perspective

polmichel · 2026-06-30T09:30:51Z

+            # path_identifier is "data/<node_uuid>/<field>/...".
+            path_parts = conflict_path.split("/")
+            if len(path_parts) >= 3 and path_parts[0] == "data":
+                conflicted_fields.add((path_parts[1], path_parts[2]))


Just a thought, not mandatory and should not block this PR.

This code looks like specific logic that should be encapsulated in a dedicated method that makes the intent explicit. And as it happens, this block is duplicated (see infrahub.proposed_change.tasks.run_proposed_change_schema_integrity_check). That could lead to asymmetrical behavior on both ends if we ever need to fix something or add capabilities to this block.

very good point. this has been refactored to consolidate the conflict-parsing logic

polmichel · 2026-06-30T09:46:27Z

+        merged_car = await NodeManager.get_one(db=db, id=initial_dataset["car_id"], branch=default_branch)
+        assert merged_car
+        previous_owners = await merged_car.get_relationship("previous_owner").get_relationships(db=db)
+        # TODO: should assert the actual expected peer


is this intentionally kept for a later?

it was not. it's been addressed

polmichel · 2026-06-30T10:07:08Z

+    # the merge. Validating its pre-resolution cross-branch state here would report a spurious
+    # (and non-resolvable) schema violation, so those fields are skipped.


Not blocking at all, but the comment fragment
Validating its pre-resolution cross-branch state here would report a spurious (and non-resolvable) schema violation, so those fields are skipped.
is fairly far from the condition check it refers to, which is lower down in the loop. I'd say it's more maintainable to keep comments as close as possible to the code they refer to. One possible effect of this is a desync between code and comments.

moved to where the conflict-parsing logic was consolidated

polmichel · 2026-06-30T10:08:18Z

+        # TODO: use get_relationship("manufacturer") to clear typing ignore
+        await car_on_branch.manufacturer.update(db=db, data={"id": initial_dataset["omnicorp_id"]})  # type: ignore[attr-defined]


just highlighting in case this is not intentionally left here, not blocking

thanks. another leftover. addressed

polmichel · 2026-06-30T10:10:09Z

+    return len(tags)
+
+
+class TestProposedChangeUniquenessMerge(TestInfrahubApp):


Nice test suite

polmichel · 2026-06-30T10:15:20Z

+        # Merge the first branch: main now holds "from_one". No diff refresh is triggered for pc_two.
+        pc_one.state.value = ProposedChangeState.MERGED.value
+        await pc_one.save()
+        for _ in range(PREFECT_EVENT_WAIT_SECONDS):


Just a thought.
I totally understand why this is here, and why all the other identical blocks are too, but in your opinion, is there a possibility that we could (in the future) downgrade these tests to component tests and use a fake Prefect adapter?
Or do you think we should keep a real Prefect client in the loop?

My take on this one, without being an expert on the merge subject, would be to keep one integration test scenario and downgrade all the others to component tests and faking the Prefect client.

yes I think this is a good approach

polmichel · 2026-06-30T10:20:33Z

        self.diff_merger = diff_merger
        self.diff_repository = diff_repository
        self.diff_locker = diff_locker
+        self.constraint_validator = constraint_validator


Very clean ! new architecture pays off

cubic-dev-ai

0 issues found across 4 files (changes from recent commits).

_{Shadow auto-approve: would require human review. Changes add merge-time constraint validation and modify the core merge flow. Tests and logic changes affect critical data integrity paths. Requires human review.

Re-trigger cubic}

github-actions Bot added the group/backend Issue related to the backend (API Server, Git Agent) label Jun 24, 2026

cubic-dev-ai Bot reviewed Jun 24, 2026

View reviewed changes

ajtmccarty force-pushed the ajtm-06242026-uniqueness-into-merge-flow branch from 24c399e to 8cea523 Compare June 25, 2026 04:20

ajtmccarty changed the title ~~move constraint validation into branch merge flow~~ fix: re-validate proposed changes at merge time Jun 25, 2026

format

f43f21b

cubic-dev-ai Bot reviewed Jun 25, 2026

View reviewed changes

ajtmccarty and others added 2 commits June 29, 2026 15:30

formatting

6f288b7

cubic-dev-ai Bot reviewed Jun 29, 2026

View reviewed changes

Comment thread backend/infrahub/core/merge/branch_merger.py Outdated

ajtmccarty added 2 commits June 29, 2026 16:19

memoize has_schema_changes() to avoid duplicate DB queries

418e149

update fixture to use structure actually returned by SDK

d78eea7

cubic-dev-ai Bot reviewed Jun 29, 2026

View reviewed changes

ajtmccarty marked this pull request as ready for review June 29, 2026 23:44

ajtmccarty requested a review from a team as a code owner June 29, 2026 23:44

polmichel approved these changes Jun 30, 2026

View reviewed changes

ajtmccarty added 2 commits June 30, 2026 09:41

test cleanup

3900a43

refactor to consolidate conflict parsing

9ced511

cubic-dev-ai Bot reviewed Jun 30, 2026

View reviewed changes

ajtmccarty merged commit 54e2639 into stable Jun 30, 2026
71 checks passed

ajtmccarty deleted the ajtm-06242026-uniqueness-into-merge-flow branch June 30, 2026 19:09

ajtmccarty mentioned this pull request Jun 30, 2026

Merge stable into develop #9767

Merged

		# the merge. Validating its pre-resolution cross-branch state here would report a spurious
		# (and non-resolvable) schema violation, so those fields are skipped.

		# TODO: use get_relationship("manufacturer") to clear typing ignore
		await car_on_branch.manufacturer.update(db=db, data={"id": initial_dataset["omnicorp_id"]}) # type: ignore[attr-defined]

		return len(tags)


		class TestProposedChangeUniquenessMerge(TestInfrahubApp):

Uh oh!

Conversation

ajtmccarty commented Jun 24, 2026 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Data conflicts are already handled (and unaffected)

Approaches considered and superseded

Testing

Notes / follow-ups

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

codspeed-hq Bot commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Footnotes

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

polmichel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polmichel Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ajtmccarty commented Jun 24, 2026 •

edited by atlassian Bot

Loading

codspeed-hq Bot commented Jun 24, 2026 •

edited

Loading

cubic-dev-ai Bot left a comment •

edited

Loading

polmichel Jun 30, 2026 •

edited

Loading