fix: replace mutable default arguments with None + in-body defaults

[yzxcj797]

Three mutable default arguments (list literals in function signatures) replaced with None + in-body assignment:

1. GraphAnalyzer.analyze_temporal_evolution(metrics=[...])
2. HierarchicalChunker.init(levels=[...])
3. split_hierarchical(levels=[...])

Mutable defaults persist across calls — any mutation inside the function leaks to subsequent calls with the same default. The None pattern creates a fresh list each time.

[qodo-code-review]

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Fix mutable default args by defaulting to None in graph/chunker APIs

🐞 Bug fix 🕐 20-40 Minutes

AI Description

• Replace list-literal defaults with None to avoid cross-call state leakage.
• Update graph analysis and hierarchical chunking entrypoints to construct defaults at runtime.
• Tighten type hints for configurable metric/level parameters (Optional[List[str]]).

Diagram

graph TD
  U["Caller"] --> GA["GraphAnalyzer.analyze_temporal_evolution"] --> TQ["TemporalGraphQuery.analyze_evolution"]
  U --> HC["HierarchicalChunker.__init__"] --> SH["split_hierarchical"] --> SB["split_by_paragraphs/sentences"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use immutable tuples as defaults

• ➕ Avoids None checks and preserves docstrings as the first statement
• ➕ Prevents mutation leaks if callers treat the value as read-only
• ➖ Type changes from List[str] to Tuple[str, ...] (or requires conversion)
• ➖ Still risky if code expects list operations (slicing/mutation semantics)

2. Module-level constants + copy

• ➕ Keeps function signatures simple and avoids Optional types
• ➕ Centralizes defaults (easier to reuse/modify)
• ➖ Must defensively copy (e.g., list(DEFAULT_LEVELS)) to avoid shared state
• ➖ Easy to regress if a future edit forgets the copy

3. Add regression tests for default isolation

• ➕ Locks in the intent: no shared state across calls
• ➕ Catches None-handling regressions in recursion paths
• ➖ Adds test maintenance overhead
• ➖ Requires deciding how to assert no-leak behavior (mutate and re-call)

Recommendation: Keep the None + in-body default pattern (it matches the stated intent and avoids shared mutable state), but ensure defaults are assigned before any use and keep docstrings as the first statement in each function/method body. As-is, analyze_temporal_evolution’s metrics initialization appears inside the docstring, HierarchicalChunker.init assigns levels before the docstring (so the docstring won’t attach), and split_hierarchical now allows levels=None but immediately uses it (e.g., "section" in levels), which can raise at runtime. Consider adding small tests to prevent future regressions.

Files changed (3) +7 / -3

Bug fix (3) +7 / -3

graph_analyzer.pyDefault temporal-evolution metrics via None sentinel +3/-1

Default temporal-evolution metrics via None sentinel

• Changes analyze_temporal_evolution(metrics=...) to default to None and attempts to initialize the default metrics list in-body. Review needed: the initialization currently sits inside the docstring block, so it may not execute and may propagate None downstream.

semantica/kg/graph_analyzer.py

kg_chunkers.pyAvoid shared default levels in HierarchicalChunker initializer +3/-1

Avoid shared default levels in HierarchicalChunker initializer

• Updates HierarchicalChunker.__init__ to accept levels: Optional[List[str]] = None and assigns the standard default list when omitted. Note: the assignment precedes the docstring, which means the docstring is no longer the method docstring (string literal becomes a no-op).

semantica/split/kg_chunkers.py

methods.pyMake split_hierarchical levels optional (default None) +1/-1

Make split_hierarchical levels optional (default None)

• Changes split_hierarchical(levels=...) to levels: Optional[List[str]] = None to avoid a mutable default in the signature. Follow-up required: the function currently uses 'levels' immediately without a None-guard, which can raise when levels is not provided.

semantica/split/methods.py

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Metrics init inside docstring ✓ Resolved 🐞 Bug ≡ Correctness

Description

GraphAnalyzer.analyze_temporal_evolution places the new if metrics is None: block inside the
triple-quoted docstring, so it never executes and metrics remains None. This changes behavior (and
metrics_tracked becomes None) compared to the previous default list and also pollutes the
docstring content with code.

Code

semantica/kg/graph_analyzer.py[R261-262]

+        if metrics is None:
+            metrics = ["node_count", "edge_count", "density", "communities"]

Evidence

The added if metrics is None: lines are inside the docstring block, so they are treated as string
content and not executed; metrics remains None when passed through and when returned as
metrics_tracked. The downstream TemporalGraphQuery sets its own internal default when receiving
None, which further demonstrates the wrapper’s returned metrics_tracked will not match the
effective metrics used.

semantica/kg/graph_analyzer.py[251-291]
semantica/kg/temporal_query.py[487-530]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GraphAnalyzer.analyze_temporal_evolution` currently includes executable-looking code inside the docstring, so the default metrics list is never applied.
## Issue Context
Because `metrics` stays `None`, the wrapper passes `None` to `TemporalGraphQuery.analyze_evolution` and returns `metrics_tracked=None`, diverging from the prior behavior where GraphAnalyzer used a specific default list.
## Fix Focus Areas
- semantica/kg/graph_analyzer.py[251-291]
### Suggested fix
Keep the docstring as the first statement, then immediately after it add:

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. split_hierarchical None crash ✓ Resolved 🐞 Bug ≡ Correctness

Description

split_hierarchical now defaults levels to None but the body still assumes it is a list and
performs membership tests and slicing on it. Calling split_hierarchical(text) without specifying
levels will raise a TypeError (e.g., 'section' in None).

Code

semantica/split/methods.py[1404]

+    levels: Optional[List[str]] = None,

Evidence

The signature change allows levels to be None, but the implementation immediately checks
membership and slices levels, which requires a list-like object.

semantica/split/methods.py[1402-1437]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`split_hierarchical` changed `levels` default to `None` but does not assign a list when `levels is None`, and later treats it like a list.
## Issue Context
The function uses `"section" in levels` and `levels[1:]`, which will throw when `levels` is `None`.
## Fix Focus Areas
- semantica/split/methods.py[1402-1460]
### Suggested fix
Near the start of the function (after the docstring), add:

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. __init__ docstring not first ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

HierarchicalChunker.__init__ now runs code before the triple-quoted string, so the string is no
longer recognized as the method docstring and becomes a useless runtime constant. This breaks
introspection/help() and doc tooling that relies on __init__.__doc__.

Code

semantica/split/kg_chunkers.py[R382-384]

+        if levels is None:
+            levels = ["section", "paragraph", "sentence"]
 """

Evidence

The file shows the newly added if levels is None: block precedes the triple-quoted string in
__init__, meaning the string literal is no longer the docstring.

semantica/split/kg_chunkers.py[376-394]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`HierarchicalChunker.__init__` now has an `if levels is None:` block before the triple-quoted string, so the constructor loses its docstring.
## Issue Context
In Python, only the first statement in a function body can be treated as the docstring.
## Fix Focus Areas
- semantica/split/kg_chunkers.py[376-394]
### Suggested fix
Move the docstring to be the first statement in `__init__`, then place the `levels is None` defaulting immediately after the docstring.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can add REVIEW.md to your repo root and Qodo follows it on every PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[yzxcj797]

All three Qodo findings addressed and pushed:

1. Metrics init inside docstring — the if metrics is None block had landed inside the docstring and never executed (metrics_tracked came back None). Moved below the docstring; verified the default list is applied again.
2. __init__ docstring not first — same misplacement in HierarchicalChunker.__init__ turned the docstring into a dead constant and broke help()/introspection. Reordered; __init__.__doc__ is recognized again.
3. split_hierarchical None crash — with levels=None defaulted at the signature, the body still ran "section" in levels and raised TypeError on every default call. Now defaults to the documented hierarchy (verified: split_hierarchical(text) works, tests/split 42/42).

tests/kg shows no outcome changes vs main (the 21 failures there are pre-existing on main in my environment).

[ZohaibHassan16]

Looks good to me. The default values are preserved and the previous review points have been addressed.

Approved.

[Sameer6305]

Review — Approved ✅

At the start of the review, this PR addressed three mutable list defaults in function signatures:

GraphAnalyzer.analyze_temporal_evolution(metrics=[...])
HierarchicalChunker.init(levels=[...])
split_hierarchical(levels=[...])

The original fix correctly replaces these shared mutable defaults with None and creates the default lists inside the function body. This prevents state/mutations from leaking between calls while preserving the existing default behavior.

During review, we also added focused regression coverage for all three affected sites. The tests specifically verify that:

default lists are independent between calls/instances,
the expected default values are preserved,
explicit values continue to work correctly,
mutations do not affect subsequent calls.

The affected test suites pass 96/96, and the reviewed changes contain no unrelated modifications.

The additional regression tests strengthen the PR by ensuring this mutable-default bug cannot silently return in the future.

Conclusion: The implementation is correct, the scope is appropriate, and regression coverage is now in place. Approved for merge. ✅