fix: emit group.member_added only for the group named in the mutation (closes #10314)

[iddocohen]

Why

Adding a group as a member of another group emitted a second infrahub.group.member_added event naming the enrolled group as the one that gained a member, with the enclosing group as the member. Every repository import hits this, because object import wraps each pass in a CoreRepositoryGroup tracking group and enrols every group defined in objects/*.yml into it. A CoreGroupTriggerRule watching one of those groups matched the spurious event and ran its CoreGeneratorAction against the tracking group, which then failed with ValueError: Target <id> is not part of the group <id>.

Goal: one membership change produces one member_added event, for the group named in the mutation.

Non-goals: this PR does not change the import path, the tracking group, or which peers it collects (#6886, #9248), the import ordering for repository-defined trigger rules (#10063 / #10196), or the fact that a group-to-group membership is readable from both groups (see Risky or uncertain parts).

Closes #10314

What changed

Behavioral changes:

• Enrolling group B into group A now emits a single member_added event, for A with B as the member. Previously a second event fired for B, claiming it had gained A.
• The peer-side NodeUpdatedEvent that accompanied that spurious event is gone too, so a CoreNodeTriggerRule watching members on B no longer fires either.
• Secondary changelogs now pick the correct side of a hierarchical relationship. parent and children share one identifier on hierarchical kinds, and the previous lookup returned whichever appeared first in the schema's relationship list.

Implementation notes:

• Root cause was in RelationshipChangelogGetter: it resolved the peer's "reverse" relationship with get_relationship_by_identifier, which matches on identifier alone. Group kinds never receive member_of_groups (add_groups() skips anything inheriting CoreGroup), so a group peer's only group_member relationship is its own inherited members — the same side that was just changed, not the reverse.
• Added BaseNodeSchema.get_reverse_relationship(), which requires the candidate to point the opposite way (RelationshipDirection.neighbor_direction) and never accepts a bidirectional relationship as its own reverse. It reuses the existing get_relationships_by_identifier rather than introducing new machinery.
• Both _process_added_peers and _process_removed_peers now call it. The existing if peer_relation: guard already handles "no reverse relationship", so no control flow changed.

What stayed the same:

• No schema changes, no migration, no GraphQL contract change. add_groups() was deliberately not changed to inject member_of_groups onto group kinds: that would add a field to every group in the public API and still leave two bidirectional group_member relationships on one kind to disambiguate.
• get_relationship_by_identifier is untouched and keeps its eight other callers.
• No data repair is needed. The fix changes only which events are derived, never what is written.

How to review

Focus on backend/infrahub/core/schema/basenode_schema.py — specifically whether the two rejection rules in get_reverse_relationship are the right ones. Everything else follows from it; the changelog change is a two-line call-site swap.

The second rule compares relationship names, which deliberately also rejects two different kinds that share a name: the reported failure is exactly that shape, CoreRepositoryGroup.members and CoreGeneratorGroup.members, both inherited from CoreGroup. Restricting the rule to a self-referential relationship instead does not fix the report, and there is no identity token to compare on — inherited relationships get id = None and carry no source_*_id. The replication test therefore enrols a group into a group of a different kind; it fails again if the rule is narrowed to an identity comparison.

Risky or uncertain parts:

• Worth a maintainer's opinion: while adding the persistence assertions requested in review, the test showed that a group-to-group membership is readable from both groups — after adding B to A's members, reloading B also lists A. Both kinds read the one stored link through the same members relationship, so the graph genuinely cannot say which group gained a member; only the mutation can. This makes the issue's statement that "nothing in the graph changed" inaccurate, and it means suppressing the second event is a decision about event semantics (report the intent of the mutation) rather than a correction of a claim the graph contradicts. The test pins the current storage behavior so a future change to the group data model surfaces here. If you would rather group-to-group membership were asymmetric, that is a separate modelling change, adjacent to CoreAccountRole cannot be added to CoreRepositoryGroup.members — repository import fails (also affects CoreGraphQLQuery) #9248.
• The direction filter would stop emitting a secondary changelog for a pair whose two sides declare mismatched directions (e.g. one OUTBOUND, one BIDIR). Core schemas contain no such pair: the only explicit directions are the properly inverse hierarchy pair and the resource-pool relationship, whose identifier exists on one side only and already resolved to nothing. A user-defined mismatched pair cannot be traversed from the peer's side anyway.

Alternatives considered: guarding in GroupNodeMutationParser or in CoreGroupTriggerRule, and tolerating an empty intersection in _run_generators. All three leave the fabricated changelog in place and only hide its effects; the last one would also mask genuine misconfiguration, which is what that error exists to report. Gating on infrahub.node.action does not work: it is CREATED for every GroupMemberAddedEvent.

How to test

# Replication test (fails on stable, passes here) plus the regression canary in the same file
uv run pytest backend/tests/component/graphql/mutations/test_group_event_collection.py -v --neo4j

# Blast radius: both call sites of RelationshipChangelogGetter, and the action/trigger path
uv run pytest -n 4 --neo4j \
  backend/tests/component/core/changelog/ \
  backend/tests/component/graphql/mutations/ \
  backend/tests/component/graphql/test_mutation_relationship.py \
  backend/tests/component/actions/

uv run invoke backend.test-unit

Results locally: 2 passed in the test file (it failed as assert [enclosing] == [enclosing, enrolled] before the fix), 158 passed across the blast-radius suites, 1435 unit tests passed. uv run invoke format, main.lint and backend.lint (ruff, ty, mypy) clean. backend.generate, schema.generate-graphqlschema, schema.generate-jsonschema and docs.generate produce no diff, so frontend codegen cannot be affected either. docs.lint / docs.format could not run locally (markdownlint-cli2 not installed); no markdown other than the changelog fragment changed.

Impact & rollout

• Backward compatibility: no breaking change to any API or schema. Automation that relied on the spurious second event will stop receiving it — that is the point of the fix.
• Performance: unchanged. The new lookup walks the same relationship list as before.
• Config/env changes: none.
• Deployment notes: safe to deploy; no migration, no coordinated release.

Checklist

• Tests added/updated
• Changelog entry added (uv run towncrier create ...)
• External docs updated (if user-facing or ops-facing change) — N/A, no documented behavior changes
• Internal .md docs updated (internal knowledge and AI code tools knowledge) — N/A for the fix; the group-to-group symmetry noted above may deserve a line in dev/knowledge/backend/events.md once maintainers decide whether it is intended
• I have reviewed AI generated content

[cubic-dev-ai]

All reported issues were addressed across 1 file

<sub>Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic</sub>

[opsmill-bug-pipeline]

AGENT_REVIEW_VERDICT: TEST_APPROVED

Overall verdict: APPROVED WITH SUGGESTIONS

The test test_group_enrolled_into_group_only_emits_event_for_enclosing_group faithfully reproduces the reported bug (a group enrolled into another group emits a spurious member_added on the wrong node) and asserts the correct, expected behavior. It exercises the exact code path the analyst identified and cannot pass without the production fix. Approving so the pipeline can proceed to the fix stage; the suggestions below are non-blocking.

A. Test realism — PASS

• The mutation CoreStandardGroupUpdate(data: { id, members: [{id}] }) is precisely what real clients send. The SDK's group-context tracking (python_sdk/infrahub_sdk/query_groups.py:150-172) creates/updates a CoreStandardGroup with members = related_group_ids + related_node_ids — i.e. it routinely enrols groups as members of a tracking group through the ordinary node-mutation path. This is the real-world repository-import scenario from the issue, not a fabricated one.
• CoreStandardGroup, the members relationship, and identifier="group_member" are real system values. The node_group_schema fixture (backend/tests/conftest.py:1145-1160) reproduces the group_member identifier collision that exists in production (CoreGroup.members and member_of_groups share it), which is the precondition for the bug.

B. Test correctness — PASS

• Asserts the expected behavior, not the buggy one: exactly one GroupMemberAddedEvent, node_id == enclosing_group, and members == [EventNode(id=enrolled_group, kind="CoreStandardGroup")].
• Exercises the affected path: the node-mutation update flows through RelationshipChangelogGetter._process_added_peers (backend/infrahub/core/changelog/models.py:594), which resolves the reverse relationship via get_relationship_by_identifier (backend/infrahub/core/schema/basenode_schema.py:362-370). That helper matches identifier only, ignoring direction, so for a group peer it lands on the group's own members and emits the inverted secondary changelog. The test targets this exactly.
• Cannot pass without the fix — confirmed FAILING (assert [enclosing] == [enclosing, enrolled]).

C. Test quality — PASS

• Deterministic and isolated; uses the MemoryInfrahubEvent adapter rather than any mock, complying with the no-mock testing rule.
• Follows the conventions of the neighboring test_node_mutation_to_group_event (same fixtures, same event-collection pattern, same ordering assumptions).
• Exact-match assertions throughout (list equality, exact member list, exact total count == 2) per the "assert exact expectations" guideline. No issue numbers in the test name, docstring, or comments.

D. Alignment with analysis — PASS

• Matches the root-cause description (direction-blind identifier lookup lands on the group's own members).
• Scope is right: the pre-existing test_node_mutation_to_group_event covers the working direction (member_of_groups) and serves as the regression canary, while this new test covers the broken group-in-group direction. Not too narrow, not too broad.

Suggestions (non-blocking)

• Consider making the "wrong node" guarantee explicit by asserting no event has node_id == enrolled_group.get_id() (e.g. assert all(e.node_id != enrolled_group.get_id() for e in memory_event.events)). The current len(...) == 2 plus the member_added_events list-equality already imply this, but an explicit assertion would keep the intent obvious and survive unrelated changes to the total event count.
• Optional: assert the type of memory_event.events[1] (the GroupMemberAddedEvent) directly, mirroring how events[0] is checked, rather than relying on the filtered list — minor readability nicety.

Recommended next steps

• Proceed to the fix stage. The fix should make the peer-side reverse-relationship resolution direction-aware (and exclude the relationship just traversed) so a group peer resolves to member_of_groups (absent on group kinds) rather than its own members, per the analyst's primary fix site at backend/infrahub/core/changelog/models.py:594 / 628.
• Keep both test_node_mutation_to_group_event (working direction) and this new test green after the fix.

AGENT_REVIEW_ITERATION: test-1

Generated by Bug reviewer agent for #10316 · 134.8 AIC · ⌖ 28.8 AIC · ⊞ 12.3K · ◷

[codspeed-hq]

Merging this PR will not alter performance

✅ 13 untouched benchmarks

---

_{Comparing ai-bug-pipeline-10314-group-in-group-event (bc26b3d) with stable (0cb07cb)¹}

Footnotes

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

[cubic-dev-ai]

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

Confidence score: 3/5

• In backend/infrahub/core/schema/basenode_schema.py, the name-only guard can drop a valid peer relationship when different kinds reuse the same bidirectional relationship name, causing incorrect relationship behavior; compare schema/relationship identity so only genuinely self-referential relationships are excluded.

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="backend/infrahub/core/schema/basenode_schema.py">

<violation number="1" location="backend/infrahub/core/schema/basenode_schema.py:400">
P2: When two different kinds use the same name for a bidirectional relationship, this name-only guard drops the valid peer relationship. Compare schema/relationship identity so only a genuinely self-referential relationship is skipped.</violation>
</file>

<sub>Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: When two different kinds use the same name for a bidirectional relationship, this name-only guard drops the valid peer relationship. Compare schema/relationship identity so only a genuinely self-referential relationship is skipped.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/core/schema/basenode_schema.py, line 400:

<comment>When two different kinds use the same name for a bidirectional relationship, this name-only guard drops the valid peer relationship. Compare schema/relationship identity so only a genuinely self-referential relationship is skipped.</comment>

<file context>
@@ -378,6 +383,26 @@ def get_relationships_by_identifier(self, id: str) -> list[RelationshipSchema]:
+        for candidate in self.get_relationships_by_identifier(id=relationship.get_identifier()):
+            if candidate.direction != expected_direction:
+                continue
+            if candidate.direction == RelationshipDirection.BIDIR and candidate.name == relationship.name:
+                continue
+            return candidate
</file context>

[iddocohen]

The mechanism is real but the conclusion is inverted for this codebase, so keeping the name comparison.

The reported failure is two different kinds using the same name: CoreRepositoryGroup.members and CoreGeneratorGroup.members, both inherited from CoreGroup with identifier group_member. Skipping that is the fix, not a regression.

Comparing relationship identity cannot separate the two cases. Inherited relationships get id = None (node_inheritance_handler.py:81) and, unlike attributes, carry no source_*_id, so there is no token shared between the two members. Comparing owning kinds instead only skips a same-kind pair, which leaves the reported cross-kind case emitting the spurious event. Verified: swapping the guard for an identity comparison makes the replication test fail again.

Your comment did expose a genuine gap, and thanks for it: the test used one kind on both sides, so it could not tell the two implementations apart. 41b14eb8 enrols a CoreStandardGroup into a TestTrackingGroup instead, matching the reported scenario, and it now fails under the identity-based guard.

On the residual risk: a same-name bidirectional pair across two kinds that is a genuine two-sided relationship would lose its peer-side changelog. No such pair exists in the core schema — every two-sided pair names its sides differently (checks/validator, comments/thread, roles/permissions, ip_addresses/ip_prefix), and the one identifier shared by two same-named relationships (ippool__resource) is on sibling pool kinds that are never each other's peer.

[cubic-dev-ai]

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

Confidence score: 5/5

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

<sub>Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.

Re-trigger cubic</sub>

[opsmill-bug-pipeline]

AGENT_REVIEW_VERDICT: FIX_APPROVED

Overall verdict: APPROVED WITH SUGGESTIONS

The fix resolves the root cause precisely and minimally. Enrolling group B into group A now emits a single member_added event for A, and the spurious peer-side NodeUpdatedEvent/member_added for B is gone. The change is a two-line call-site swap plus one new, well-scoped schema helper. I traced the direction logic against the real schema definitions and the schema validator and it holds for every relevant pairing. Suggestions below are non-blocking.

A. Correctness — PASS

• Root cause addressed. RelationshipChangelogGetter._process_added_peers/_process_removed_peers (backend/infrahub/core/changelog/models.py:594, :628) now call get_reverse_relationship instead of the direction-blind get_relationship_by_identifier. For a group peer, whose only group_member relationship is its own inherited members, the helper correctly returns None, so no inverted secondary changelog is fabricated.
• Direction logic verified end-to-end. members (CoreGroup) and member_of_groups (added by add_groups) both default to RelationshipDirection.BIDIR (relationship_schema.py default). neighbor_direction maps BIDIR→BIDIR, so the working direction (node added to a group) still resolves member_of_groups (BIDIR, different name → not skipped). The group-in-group case is caught by the BIDIR and candidate.name == relationship.name guard → None. Confirmed correct at basenode_schema.py:395-404.
• Hierarchy pair also corrected. parent is OUTBOUND and children is INBOUND sharing PARENT_CHILD_IDENTIFIER (schema_branch.py:2356-2383). neighbor_direction now selects the opposite side deterministically, fixing the prior "first-in-list wins" bug in secondary changelogs.
• Edge case is not reachable. The concern that the direction filter could drop a mismatched OUTBOUND/BIDIR pair is blocked by validate_identifiers (schema_branch.py:836-878), which only permits BIDIR/BIDIR or INBOUND/OUTBOUND for a shared identifier. So no legitimate reverse relationship the old code would have found is silently dropped — the only behavior change is suppressing the fabricated one.
• Symmetry. Add and remove paths were switched together, so removal of a group-from-group no longer emits a spurious GroupMemberRemovedEvent either. Consistent.

B. Code quality — PASS

• The new get_reverse_relationship reuses get_relationships_by_identifier rather than introducing new traversal machinery; the if peer_relation: guard already handled the None case, so no control flow changed.
• Docstring explains the why (the identifier-sharing invariant and the asymmetric-BIDIR reasoning) without naming callers or issue IDs, matching the code-doc-style rule.
• No unnecessary refactoring; get_relationship_by_identifier and its other eight callers are untouched.

C. Documentation alignment — PASS

• No ADR or documented contract is contradicted; no schema/GraphQL/migration change. The PR appropriately flags that the group-to-group storage symmetry (a link readable from both sides) may deserve a line in dev/knowledge/backend/events.md and defers that to a maintainer decision — reasonable to leave as N/A for this fix.

D. Test quality — PASS

• test_group_enrolled_into_group_only_emits_event_for_enclosing_group asserts exact expectations (single member_added for the enclosing group, exact member list, exact total == 2), uses the MemoryInfrahubEvent adapter (no mocks), and reloads both nodes to pin the storage behavior per the testing guidelines. It cannot pass without the fix.
• The pre-existing test_node_mutation_to_group_event remains as the working-direction regression canary. Good coverage of both sides.

Suggestions (non-blocking)

• Unit-test the new helper directly. get_reverse_relationship is a pure, in-memory function; per the "pick the cheapest test tier" guideline it warrants a small unit test (e.g. under tests/unit/core/schema/) covering the three cases: BIDIR self → None, BIDIR cross-name → returns peer, INBOUND/OUTBOUND hierarchy → returns the opposite side. This locks the invariant at the cheapest tier instead of relying solely on the component test.
• Consider a direct assertion for the hierarchy-changelog fix. The corrected parent/children side-selection is a real behavioral change but is only exercised indirectly. A targeted test asserting the secondary changelog names the correct side on a hierarchical relationship would guard against regressions there.

Recommended next steps

• Optionally add the unit test for get_reverse_relationship above; otherwise this is ready to merge from a review standpoint.
• Note: I reviewed statically — the --neo4j component tests and lint results reported in the PR body were not independently re-run in this environment.

AGENT_REVIEW_ITERATION: fix-1

Generated by Bug reviewer agent for #10316 · 151.9 AIC · ⌖ 28.8 AIC · ⊞ 12.3K · ◷

[cubic-dev-ai]

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

Confidence score: 5/5

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

<sub>Shadow auto-approve: would auto-approve. Fixes a bug that emitted a spurious group.member_added event for the enrolled group when adding a group as a member of another group. The change corrects reverse-relationship resolution and adds a focused regression test; no schema, API, or operational tradeoffs are introduced.

Re-trigger cubic</sub>

[saltas888]

Can we unify this if in one and also leave a code comment explaining the logic?

[iddocohen]

Done in bc26b3d: one condition plus a comment on why the name is what separates an inherited same-name pair (CoreGroup.members on two kinds) from a genuine two-sided relationship, and I checked the new guard against the old two-if version across every schema/relationship pair in the core schema (33,028 comparisons, 2,830 resolved peers, 0 mismatches).

[ogenstad]

Adding a group as a member of another group

I think the premise of this bug report and fix needs some work. In Infrahub it's not currently possible to add a group as a member of another group. So it feels like something is wrong today. Possibly the root cause is that the mutation doesn't immediately reject such actions. Is this the case?

While we can argue about what should be possible the CoreGroup member relationship (https://github.com/opsmill/infrahub/blob/infrahub-v1.10.8/backend/infrahub/core/schema/definitions/core/group.py#L46) points to CoreNode and a group is not a CoreNode type object.

[cubic-dev-ai]

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

Confidence score: 5/5

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

<sub>Shadow auto-approve: would require human review. Fixes spurious group.member_added event by adding a name-based rejection rule in get_reverse_relationship; this heuristic may reject legitimate reverse relationships and the author explicitly requests a maintainer's opinion, so human review is needed.

Re-trigger cubic</sub>

[iddocohen]

@ogenstad groups, as far as I understand the code, can be members of other groups today.

The check allows any kind that inherits something, and every group kind inherits CoreGroup. Infrahub relies on this. Every repository import puts the repository's groups into a CoreRepositoryGroup so they appear in the repository's Objects tab, which is exactly the mutation in question. So rejecting the mutation is, as far as I think, not the right choice.

The use case: a customer's repository declares a group in objects/groups.yml and a CoreGroupTriggerRule that runs a generator when something is added to that group — the documented enrol-then-materialise pattern for auto-provisioning services. That is a normal, supported way to wire a repository.

The harm: every time they push a commit, the import fabricates a "member added" event for that group, so the generator runs against Infrahub's internal repository group and fails. They get a red generator run per trigger rule on every import, forever — failures that look exactly like real generator failures in the task list.

Why it matters: it makes "did my pipeline run clean?" unanswerable without opening each failure by hand, and it rules out any automated gate on failed tasks, since the baseline is never zero. For a customer running this in CI or during a DR rebuild, that is the difference between a usable signal and permanent noise they learn to ignore.