fix: enforce minimum hyperedge cardinality - #3
Conversation
Treat hyperedges as 3+ member group relationships across semantic cleanup, cache persistence, deduplication, graph construction, incremental pruning, and attachment. Add regression coverage for invalid groups and legacy healing.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (17)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change defines a three-member minimum for hyperedges. Graph construction, merge pruning, deduplication, semantic cleanup, cache writes, and export validation now remove undersized hyperedges. Tests update fixtures and coverage for the new rule. ChangesHyperedge cardinality enforcement
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🟡 Moderate · up to The PR adds minimum-cardinality filtering, but duplicate or aliased members are counted before normalization in merge and cache paths, so a group with only two distinct members can still survive as a hyperedge. This is a bounded correctness issue in the contract enforcement and should be fixed or explicitly accepted before merge. Sequence Diagram(s)sequenceDiagram
participant GraphBuilder
participant SemanticCleanup
participant CacheWriter
participant Exporter
GraphBuilder->>GraphBuilder: Revalidate surviving hyperedge members
SemanticCleanup->>SemanticCleanup: Remove hyperedges below three members
CacheWriter->>CacheWriter: Filter undersized hyperedges before writing
Exporter->>Exporter: Validate and persist only valid candidates
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07330cd272
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if len(rewired) >= 3: | ||
| kept.append(he) | ||
| hyperedges[:] = kept |
There was a problem hiding this comment.
When a valid three-or-more-member hyperedge uses the supported members or node_ids alias, the default build(..., dedup=True) path calls this function before build_from_json() performs alias normalization. Such an entry has no nodes list, so it is skipped here and then permanently deleted by hyperedges[:] = kept; the same extraction survives with dedup=False. Normalize the member aliases before filtering, or retain entries that this function cannot yet process.
Useful? React with 👍 / 👎.
| for h in (hyperedges or []): | ||
| if not _has_minimum_hyperedge_members(h): | ||
| continue |
There was a problem hiding this comment.
For save_semantic_cache() callers that receive a valid hyperedge using the supported members or node_ids spelling, this predicate checks only canonical nodes and silently discards the group before it reaches the cache. Consequently a later cache replay loses that semantic relationship, and an otherwise hyperedge-only result can be treated as empty and retried. Normalize the aliases before both cardinality filters rather than rejecting them at this point.
Useful? React with 👍 / 👎.
egarcia74
left a comment
egarcia74
left a comment
There was a problem hiding this comment.
Ran CodeRabbit and verified both findings by hand against the diff. Two gaps in the new minimum-cardinality enforcement itself — details inline. Not blocking, just flagging before merge.
| if src: | ||
| by_file[src]["edges"].append(e) | ||
| for h in (hyperedges or []): | ||
| if not _has_minimum_hyperedge_members(h): |
There was a problem hiding this comment.
_has_minimum_hyperedge_members(h) runs on the raw hyperedge here, before _normalize_hyperedge_members (which dedupes member refs and folds members/node_ids aliases onto nodes) ever runs — I checked cli.py's call into save_semantic_cache and confirmed normalization never happens first on this path. So {"nodes": ["a", "a", "b"]} has len == 3 and passes, even though it only has 2 distinct members — which defeats the invariant this PR adds. Suggest normalizing/deduping members before checking cardinality here (and at the other _has_minimum_hyperedge_members call a few lines down in this same function).
| candidate = he if surviving == he["nodes"] else {**he, "nodes": surviving} | ||
| if _has_minimum_hyperedge_members(candidate): | ||
| final_hyperedges.append(candidate) | ||
| G.graph["hyperedges"] = final_hyperedges |
There was a problem hiding this comment.
This revalidation (dedupe + filter to surviving nodes + _has_minimum_hyperedge_members) is correct, but it's nested entirely inside if prune_sources: above. A normal incremental --update that doesn't delete/exclude any files skips this safety net entirely, so a carried hyperedge that degrades — e.g. via the semantic re-key step (~line 934) collapsing two distinct ids onto the same one without a subsequent re-dedupe — can slip through when no prune is involved. Since the goal of this PR is enforcing the 3+ member invariant universally, consider moving this block out of the if prune_sources: gate so it always runs before merge_raw_extraction/build_merge return.
Minor/separate nit while in this area: dedup.py's _remap_hyperedge_members (line ~502) still hardcodes len(rewired) >= 3 instead of the new MIN_HYPEREDGE_MEMBERS/_has_minimum_hyperedge_members this PR introduces as the canonical check — same value today, but now a second source of truth for the constant.
Neither existing test (test_hyperedge_member_shapes.py, test_build_merge_hyperedges_and_prune.py) currently covers the duplicate-raw-member case, so both gaps pass CI as-is.
Summary
Enforce Graphify's documented hyperedge contract: hyperedges represent group relationships and therefore require at least three surviving members. Pairwise relationships belong in the ordinary edge set.
The bug surfaced when unresolved members were pruned after semantic extraction. Valid 3+ member input could degrade into a one- or two-member hyperedge, then persist in semantic caches and
graph.json.Changes
3members).Validation
graphifyandtests: PASS.git diff --check: PASS.Origin
Observed against a real semantic corpus where under-cardinality cached hyperedges caused replay/provenance validation failures. A clean rebuild with the guards produced zero final or cached hyperedges below three members and zero final hyperedges lacking cache provenance.
Summary by CodeRabbit
Bug Fixes
Tests