PR Review: feat: add Gemini CLI / AI Studio session import support

Executive Summary

Affected Areas: normalize.py parser chain, _try_gemini_json new parser, test suite

Business Impact: Users importing Gemini CLI sessions via {"messages": [...]} wrapper with 2+ user turns would get silently broken transcripts (assistant responses dropped).

Flow Changes: New _try_gemini_json parser inserted into the JSON parser chain between _try_chatgpt_json and _try_slack_json. Parser ordering creates a precedence conflict with _try_claude_ai_json for one of the three documented layouts.

Ratings

PR Health

• Has clear description
• References ticket/issue (if applicable)
• Appropriate size (or justified if large)
• Has relevant tests (if applicable)

High Priority Issues

(Must fix before merge)

🐛 #1: Parser precedence bug — _try_claude_ai_json silently claims Gemini {"messages": [...]} data

Location: mempalace/normalize.py:63 | Confidence: ✅ HIGH

_try_claude_ai_json runs before _try_gemini_json in the parser chain. When Gemini data uses the {"messages": [...]} wrapper (Layout 2a), _try_claude_ai_json extracts the list via data.get("messages", ...), captures all role="user" entries, but silently skips role="model" entries (not in its accepted roles: user, human, assistant, ai). If there are 2+ user messages, it returns a broken transcript with only user turns — the Gemini parser never executes.

Trace for input {"messages": [{"role": "user", "content": "Q1"}, {"role": "model", "content": "A1"}, {"role": "user", "content": "Q2"}, {"role": "model", "content": "A2"}]}:

1. _try_claude_ai_json → extracts messages list → captures ("user", "Q1"), skips model, captures ("user", "Q2"), skips model
2. len(messages) = 2 >= 2 → returns broken transcript: > Q1\n\n> Q2\n
3. _try_gemini_json never runs

Fix (choose one):

Option A — Move Gemini parser before Claude AI in the chain (minimal change, safe due to has_model_role guard):

- for parser in (_try_claude_ai_json, _try_chatgpt_json, _try_gemini_json, _try_slack_json):
+ for parser in (_try_gemini_json, _try_claude_ai_json, _try_chatgpt_json, _try_slack_json):

Option B — Add assistant-role guard to _try_claude_ai_json (more defensive, prevents the class of bug):

+ has_assistant = any(r == "assistant" for r, _ in messages)
- if len(messages) >= 2:
+ if len(messages) >= 2 and has_assistant:
      return _messages_to_transcript(messages)

Option A is recommended — _try_gemini_json already has its own has_model_role discriminator that prevents false positives against Claude/ChatGPT data.

🔗 #2: Missing test for Layout 2a — the exact layout affected by bug #1

Location: tests/test_normalize.py (new tests) | Confidence: ✅ HIGH

The PR documents three supported layouts but only tests two:

• ✅ Layout 1: {"contents": [...]} — tested in test_gemini_contents_format
• ❌ Layout 2a: {"messages": [...]} — NOT tested
• ✅ Layout 2b: flat list [...] — tested in test_gemini_flat_messages_format

The missing test is for the exact layout affected by bug #1. Adding this test would have caught the parser precedence issue.

def test_gemini_messages_wrapper_format():
    """Gemini data wrapped in {"messages": [...]} should parse correctly."""
    data = {
        "messages": [
            {"role": "user", "content": "What is Python?"},
            {"role": "model", "content": "A programming language."},
            {"role": "user", "content": "And Java?"},
            {"role": "model", "content": "Also a programming language."},
        ]
    }
    # write to tmp file, normalize, assert both user and model content present

Medium Priority Issues

(Should fix, not blocking)

🔄 #3: Test file will have merge conflicts with current main

Location: tests/test_normalize.py | Confidence: ✅ HIGH

The PR's test diff targets an older base where tests/test_normalize.py has bare functions (test_empty()) with tempfile/os.unlink. Current main has been restructured into pytest classes (TestNormalizePlainText, TestClaudeAiJson, etc.) using the tmp_path fixture. The new tests will conflict on merge.

The new tests should be rebased onto current main and restructured as a TestGeminiJson class using tmp_path:

class TestGeminiJson:
    def test_contents_format(self, tmp_path):
        data = {"contents": [
            {"role": "user", "parts": [{"text": "Capital of France?"}]},
            {"role": "model", "parts": [{"text": "Paris."}]},
        ]}
        f = tmp_path / "gemini.json"
        f.write_text(json.dumps(data))
        result = normalize(str(f))
        assert "> Capital of France?" in result
        assert "Paris." in result

🎨 #4: Tests use tempfile/os.unlink instead of pytest tmp_path

Location: tests/test_normalize.py (all new test functions) | Confidence: ✅ HIGH

All 5 new tests use tempfile.NamedTemporaryFile with manual os.unlink(f.name). The codebase convention (every existing test) uses pytest's tmp_path fixture. The tempfile pattern has a cleanup risk: if an assertion fails before os.unlink(), the temp file leaks. tmp_path handles cleanup automatically.

Related to #3 — fixing this during rebase would address both issues.

Low Priority Issues

(Nice to have)

🎨 #5: elif not text: is always true — simplify to else:

Location: mempalace/normalize.py:+279 (in _try_gemini_json) | Confidence: ✅ HIGH

text = ""
parts = item.get("parts")
if isinstance(parts, list):
    # ... sets text from parts ...
elif not text:  # ← always True here since text="" and parts wasn't a list
    text = _extract_content(item.get("content", ""))

Since text = "" is assigned just above and the elif branch only executes when isinstance(parts, list) is False, not text is always True. This should be else: for clarity.

- elif not text:
+ else:
      text = _extract_content(item.get("content", ""))

🎨 #6: Module docstring mentions "Gemini CLI" but not "Google AI Studio"

Location: mempalace/normalize.py:10 | Confidence: ✅ HIGH

The PR title and body say this supports both "Gemini CLI" and "Google AI Studio" sessions. The docstring update only adds "Gemini CLI JSON sessions". Consider:

-    - Gemini CLI JSON sessions
+    - Gemini CLI / Google AI Studio JSON sessions

Flow Impact Analysis

normalize()
  └─ _try_normalize_json(content)
       ├─ _try_claude_code_jsonl(content)    # JSONL, runs on raw string
       ├─ json.loads(content) → data
       ├─ _try_claude_ai_json(data)          # ⚠️ Claims {"messages":[...]} with 2+ user msgs
       ├─ _try_chatgpt_json(data)            # Requires "mapping" key — no conflict
       ├─ _try_gemini_json(data)             # NEW — handles "contents", "messages", list
       └─ _try_slack_json(data)              # Requires type="message" items — no conflict

Conflict zone: _try_claude_ai_json and _try_gemini_json both handle {"messages": [...]} and top-level lists. The has_model_role guard in the Gemini parser is correct, but it never gets a chance to run when the Claude parser claims the data first.

Safe layouts (no precedence issue):

• {"contents": [...]} — Claude parser doesn't check this key
• [{"role": "user", ...}, {"role": "model", ...}] (flat, ≤1 user msg) — Claude parser returns None (< 2 messages)

Unsafe layout:

• {"messages": [{"role": "user", ...}, {"role": "model", ...}, {"role": "user", ...}, ...]} with 2+ user messages

Created by Octocode MCP https://octocode.ai 🔍🐙

Hi, thanks for the contribution.

This PR has merge conflicts with develop, and the branch has not been updated in over 7 days, which puts it before our most recent release. The conflicts are likely against work that landed in that release.

Could you rebase onto develop so we can take another look?

If this change is no longer relevant, feel free to close the PR.

(This message is part of a periodic backlog pass, sent to all open PRs that match this state.)

@bgauryy thanks for the thorough review — all six points addressed in the rebase (a9a2c35).

High priority

• Congratulations Milla #1 Parser precedence bug: went with Option A — _try_gemini_json now runs before _try_claude_ai_json in the dispatch chain. The Gemini parser's has_model_role guard (returns None if no role="model" entry is seen) prevents false-positives against Claude / ChatGPT data, so the reordering is safe.
• Integrating MemPalace with SoulForge's code intelligence system #2 Layout 2a coverage: added two regression tests — test_messages_wrapper_format (unit-level) and test_messages_wrapper_does_not_get_claimed_by_claude (full normalize() pipeline). Both fail without the precedence fix.

Medium priority

• feat: Hermes memory provider integration #3 Test conflicts with current develop: rebased the branch onto current develop. develop has since gained its own _try_gemini_jsonl (for ~/.gemini/tmp/.../chats/session-*.jsonl runtime sessions) — that's a complementary parser to the JSON one this PR adds. Coexistence intentional and called out in the docstring.
• Using AAAK as language for agents #4 tempfile/os.unlink → tmp_path: all new tests now use the tmp_path fixture inside a TestGeminiJson pytest class, matching the existing convention.

Low priority

• feat: add OpenAI Codex CLI JSONL normalizer #5 elif not text → else: fixed.
• fix: handle Windows file-lock errors in test cleanup #6 Module docstring: now says Gemini CLI / Google AI Studio JSON sessions.

tests/test_normalize.py runs clean locally (126 passed, including the 9 new TestGeminiJson cases). PR is back to mergeable — git pull should be a fast-forward.

@igorls heads up — rebased and force-pushed onto current develop per your earlier ask.