feat: narrow MessageDict.role to RoleLiteral

[clean6378-max-it]

Closes #81

Summary

• Add RoleLiteral in models/session.py and narrow MessageDict.role from str to the literal union (user, assistant, system, result, progress).
• Propagate the narrowed type to SearchHitDict.role in models/search.py.
• Add _coerce_role() in utils/jsonl_parser.py: unknown JSONL entry types map to "system" with logging.warning, never raise.
• Emit fallback messages for unhandled entry types (including valid result entries).
• Export RoleLiteral from models/__init__.py.
• Add TestUnknownRoleCoercion and update tests that previously expected unknown types to be silently skipped.

Test plan

• mypy -p api -p utils -p models
• ruff check .
• pytest tests/test_jsonl_parser.py -q
• pytest -q

Summary by CodeRabbit

• New Features
  • Stricter role handling: session messages now only allow predefined roles (user, assistant, system, result, progress), enforced with runtime validation that rejects invalid roles.
  • Improved JSONL robustness: non-standard/unknown entry types are coerced into system messages with warnings, including safer fallback extraction and correct behavior for result and summary.
• Bug Fixes
  • Unknown record/type parsing now produces the expected additional coerced system message instead of skipping.
• Tests
  • Expanded fuzzing and parser/validation coverage, including structured-content fallback behavior and precise validation error-path assertions.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b46d6cb6-2113-4083-86ec-8a6006503072

📥 Commits

Reviewing files that changed from the base of the PR and between 5f1f326 and 27ef1b7.

📒 Files selected for processing (8)

• models/__init__.py
• models/search.py
• models/session.py
• tests/test_jsonl_parser.py
• tests/test_jsonl_validation.py
• tests/test_parser_fuzz.py
• utils/jsonl_parser.py
• utils/validation.py

✅ Files skipped from review due to trivial changes (1)

• models/init.py

🚧 Files skipped from review as they are similar to previous changes (7)

• models/session.py
• models/search.py
• tests/test_jsonl_validation.py
• tests/test_parser_fuzz.py
• utils/validation.py
• utils/jsonl_parser.py
• tests/test_jsonl_parser.py

---

📝 Walkthrough

Walkthrough

Introduces RoleLiteral = Literal["user", "assistant", "system", "result", "progress"] in models/session.py, narrows MessageDict.role and SearchHitDict.role from str to this type, re-exports it from the models package, and adds _coerce_role/_fallback_message helpers in the JSONL parser to map unknown entry types to "system" with a warning instead of silently ignoring them. Runtime role validation is added to utils/validation.py to reject messages with unsupported roles during session validation.

Changes

RoleLiteral contract, parser coercion, validation, and tests

Layer / File(s)	Summary
RoleLiteral definition and model propagation models/session.py, models/search.py, models/__init__.py	RoleLiteral type alias is defined with five fixed string values; MessageDict.role and SearchHitDict.role are narrowed from str to RoleLiteral; RoleLiteral is re-exported from the models package __all__.
Parser unknown-role coercion logic utils/jsonl_parser.py	Adds logging, get_args, and cast imports plus RoleLiteral; defines SKIP_ENTRY_TYPES set for metadata-only types; _coerce_role helper warns and maps unknowns to "system"; _fallback_message builder creates minimal MessageDict from common entry fields; parse_session extended to emit a fallback message for any unrecognized entry_type not in skipped set.
Runtime role validation utils/validation.py	Extracts _VALID_ROLES from RoleLiteral via get_args at import time; validate_session_dict checks each message's role field and raises SessionValidationError with path and expected-vs-got formatting when invalid.
Test updates for coercion and validation behavior tests/test_jsonl_parser.py, tests/test_parser_fuzz.py, tests/test_jsonl_validation.py	TestParseSession.test_unknown_entry_type_maps_to_system_with_warning asserts a system-role message is produced and WARNING is logged; new test_summary_entry_type_produces_no_message verifies summary entries are skipped; new TestUnknownRoleCoercion class covers unknown-to-system and "result"-to-result mappings with structured content fallback; TestCoerceRole verifies _coerce_role behavior; file-history snapshot test tightened to assert zero messages; fuzz test expects two messages instead of one; new test_invalid_role_in_message validates runtime rejection of unsupported roles.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested reviewers

• timon0305
• wpak-ai

Poem

A hop through the types, a skip through the code,
RoleLiteral narrows the message abode.
Unknown roles? No crash, just a "system" with care,
A warning emitted to float through the air.
Validation stands guard at the boundary line—
Invalid roles caught, every message divine! 🐇

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title 'feat: narrow MessageDict.role to RoleLiteral' accurately and concisely summarizes the main change: narrowing the role field from str to a Literal type.
Linked Issues check	✅ Passed	The PR implements all core coding requirements from issue #81: RoleLiteral definition, narrowed MessageDict.role type, parse-time role coercion with warnings, type propagation to SearchHitDict, runtime validation, proper logging, and comprehensive test coverage.
Out of Scope Changes check	✅ Passed	All changes are directly aligned with issue #81 objectives: type narrowing, parse-time guards, validation enhancements, test coverage, and proper API exports. No unrelated modifications were introduced.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/role-literal-typeddict

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@utils/jsonl_parser.py`:
- Around line 44-45: The `_HANDLED_ENTRY_TYPES` frozenset is missing
"file-history-snapshot" even though line 144 treats it as a known metadata
record type. Because it's absent from `_HANDLED_ENTRY_TYPES`, the entry type
falls through to the `_fallback_message()` call at lines 172-175, causing
spurious unknown message role warnings and synthetic system messages. Add
"file-history-snapshot" to the `_HANDLED_ENTRY_TYPES` frozenset so that this
entry type is recognized as a handled type and properly excluded from fallback
message processing.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1002b380-cb9c-4365-85ab-3c67ada2e761

📥 Commits

Reviewing files that changed from the base of the PR and between 0435b2d and 7c6fa96.

📒 Files selected for processing (6)

• models/__init__.py
• models/search.py
• models/session.py
• tests/test_jsonl_parser.py
• tests/test_parser_fuzz.py
• utils/jsonl_parser.py

[clean6378-max-it]

@CodeRabbit , can you pleasee review this in more depth?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
echo "=== models/session.py ==="
cat models/session.py

echo ""
echo "=== models/search.py ==="
cat models/search.py

echo ""
echo "=== models/__init__.py ==="
cat models/__init__.py

echo ""
echo "=== utils/jsonl_parser.py ==="
cat utils/jsonl_parser.py

echo ""
echo "=== utils/validation.py ==="
cat utils/validation.py

echo ""
echo "=== tests/test_jsonl_parser.py ==="
cat tests/test_jsonl_parser.py

echo ""
echo "=== tests/test_parser_fuzz.py ==="
cat tests/test_parser_fuzz.py

echo ""
echo "=== tests/test_jsonl_validation.py ==="
cat tests/test_jsonl_validation.py

echo ""
echo "=== models/tool_results.py (reference pattern) ==="
cat models/tool_results.py 2>/dev/null || echo "File not found"

Length of output: 77525

---

---

@clean6378-max-it Here is a thorough review of PR #89.

---

In-Depth Review: RoleLiteral Type Narrowing (closes #81)

Overall Assessment

The PR is well-structured and achieves its goals: type safety via RoleLiteral, runtime robustness via _coerce_role, and a validation guard in validate_session_dict. The tests cover the happy path and the new coercion behaviour. A few issues deserve attention before merge.

---

🔴 Issue 1 — Dead-code check in parse_session (utils/jsonl_parser.py)

The guard type_str not in _HANDLED_ENTRY_TYPES inside the elif entry_type: branch is always True and is therefore dead code. By the time execution reaches that branch, the preceding if/elif chain has already consumed every value in _HANDLED_ENTRY_TYPES ("user", "assistant", "system", "progress"). A future maintainer might assume the check is load-bearing and be confused when they can never trigger the False path.

# Current (redundant first condition):
elif entry_type:
    type_str = entry_type if isinstance(entry_type, str) else str(entry_type)
    if type_str not in _HANDLED_ENTRY_TYPES and type_str not in _SKIP_ENTRY_TYPES:
        messages.append(_fallback_message(entry, _coerce_role(type_str)))

# Simplified (equivalent, clearer intent):
elif entry_type:
    type_str = entry_type if isinstance(entry_type, str) else str(entry_type)
    if type_str not in _SKIP_ENTRY_TYPES:
        messages.append(_fallback_message(entry, _coerce_role(type_str)))

---

🟡 Issue 2 — Root logger in _coerce_role (utils/jsonl_parser.py)

logging.warning(...) writes to the root logger. In library code this is discouraged: it bypasses module-level log routing, prevents consumers from filtering just this module's warnings, and can interfere with application-level log configuration.

# Current:
def _coerce_role(raw: str) -> RoleLiteral:
    if raw in _VALID_ROLES:
        return cast(RoleLiteral, raw)
    logging.warning("Unknown message role %r; mapping to 'system'", raw)
    return "system"

# Preferred:
_log = logging.getLogger(__name__)

def _coerce_role(raw: str) -> RoleLiteral:
    if raw in _VALID_ROLES:
        return cast(RoleLiteral, raw)
    _log.warning("Unknown message role %r; mapping to 'system'", raw)
    return "system"

Note: If you switch to a named logger, the existing caplog test in TestUnknownRoleCoercion needs the logger name scoped: caplog.at_level("WARNING", logger="utils.jsonl_parser").

---

🟡 Issue 3 — _fallback_message mixes up text vs content (utils/jsonl_parser.py)

The helper computes a variable named text but stores it under the "content" key. The _infer_title function reads the "text" key, so fallback messages can never contribute to title inference. This is also inconsistent with _process_system, which populates both keys:

# _process_system (sets "content"):
"content": entry.get("content", ""),

# _fallback_message (also sets "content" only, despite naming the variable "text"):
content = entry.get("content", "")
text = content if isinstance(content, str) else (str(content) if content is not None else "")
return {
    ...
    "content": text,   # ← stored as "content", not "text"
    ...
}

Suggestion — mirror what _process_system and _process_user do:

def _fallback_message(entry: dict[str, Any], role: RoleLiteral) -> MessageDict:
    raw_content = entry.get("content", "")
    if isinstance(raw_content, str):
        text = raw_content
    elif raw_content is not None:
        # Reuse the same normalization pipeline used by other processors
        text = _extract_text(_normalize_content(raw_content))
    else:
        text = ""
    return {
        "role": role,
        "uuid": entry.get("uuid"),
        "parent_uuid": entry.get("parentUuid"),
        "timestamp": entry.get("timestamp"),
        "text": text,
        "content": text,
        "is_sidechain": entry.get("isSidechain", False),
    }

This also fixes Issue 4 below as a side-effect.

---

🟡 Issue 4 — _fallback_message stringifies list content as a raw repr

If a result (or future forward-compat) entry carries structured content blocks, str([{"type": "text", "text": "…"}]) produces an unreadable Python repr rather than the actual text. Using _extract_text(_normalize_content(content)) (already imported) aligns with every other processor. See the suggestion in Issue 3.

---

🟢 Issue 5 — Missing direct unit tests for _coerce_role

_coerce_role is covered only indirectly through parse_session. A few focused unit tests would document its contract and catch regressions independently:

class TestCoerceRole:
    def test_known_roles_returned_unchanged(self):
        from utils.jsonl_parser import _coerce_role
        for role in ("user", "assistant", "system", "result", "progress"):
            assert _coerce_role(role) == role

    def test_unknown_role_maps_to_system_with_warning(self, caplog):
        from utils.jsonl_parser import _coerce_role
        with caplog.at_level("WARNING", logger="utils.jsonl_parser"):
            result = _coerce_role("totally_unknown")
        assert result == "system"
        assert "totally_unknown" in caplog.text

---

✅ What's done well

Area	Observation
RoleLiteral definition	Mirrors the established ToolNameLiteral pattern exactly. Clean.
_VALID_ROLES from get_args	Deriving the set at import time from the type alias means the set is always in sync with RoleLiteral — no manual duplication.
validate_session_dict enhancement	Adding role membership validation closes the runtime safety gap. Correct path format (messages[0].role), correct error detail.
_SKIP_ENTRY_TYPES	Explicit allowlist of types to silently skip is much cleaner than implicit ignore.
Test for result role	test_valid_unhandled_result_type_emits_result_role correctly exercises that a known-valid-but-unhandled type is emitted with the right role (not coerced to "system").
Fuzz test update	test_unknown_record_type_is_graceful correctly updated to assert 2 messages instead of 1.
models/__init__.py export	RoleLiteral is properly added to __all__.

---

Summary of actionable items

Priority	Item	File
🔴	Remove always-true not in _HANDLED_ENTRY_TYPES guard	utils/jsonl_parser.py
🟡	Use logging.getLogger(__name__) in _coerce_role	utils/jsonl_parser.py
🟡	Store derived text in both "text" and "content" in _fallback_message	utils/jsonl_parser.py
🟡	Use _extract_text(_normalize_content(...)) for list content	utils/jsonl_parser.py
🟢	Add direct unit tests for _coerce_role	tests/test_jsonl_parser.py

[clean6378-max-it]

@CodeRabbit , my last commit addressed seecond comment of @timon0305 ?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get all review comments on PR `#89`
gh pr view 89 --repo cppalliance/claude-code-chat-browser --comments --json comments,reviews 2>/dev/null | jq -r '
  .comments[] | select(.author.login == "timon0305") | 
  "=== Comment by timon0305 (id: \(.id)) ===\n\(.body)\n"
' 2>/dev/null || true

echo "--- Review comments ---"
gh api repos/cppalliance/claude-code-chat-browser/pulls/89/comments 2>/dev/null | jq -r '
  .[] | select(.user.login == "timon0305") | 
  "=== Review comment (id: \(.id)) at \(.path):\(.line) ===\n\(.body)\n"
' 2>/dev/null || true

echo "--- All issue/PR comments ---"
gh api repos/cppalliance/claude-code-chat-browser/issues/89/comments 2>/dev/null | jq -r '
  .[] | select(.user.login == "timon0305") | 
  "=== Comment (id: \(.id)) ===\n\(.body)\n"
' 2>/dev/null || true

Length of output: 2020

---

🏁 Script executed:

#!/bin/bash
# Get the latest commit on the PR branch and its diff
gh pr view 89 --repo cppalliance/claude-code-chat-browser --json headRefName,commits | jq -r '
  "Branch: \(.headRefName)\nLatest commit: \(.commits[-1].oid) - \(.commits[-1].messageHeadline)"
'

echo ""
echo "--- Latest commit diff ---"
gh pr view 89 --repo cppalliance/claude-code-chat-browser --json commits | jq -r '.commits[-1].oid' | xargs -I{} git show {} --stat 2>/dev/null || true

Length of output: 1016

---

🏁 Script executed:

#!/bin/bash
# Show the full diff of the latest commit
gh api repos/cppalliance/claude-code-chat-browser/commits/5f1f3262dfaf83b14fb34c59ec0bf148e1dc2ec8 | jq -r '.files[] | "=== \(.filename) ===\n\(.patch)\n"'

Length of output: 1586

---

---

@clean6378-max-it Yes, the latest commit directly and completely addresses @timon0305's second comment. Here's the breakdown:

@timon0305's concern (comment #3423532607):

Claude Code JSONL also emits summary entries — those would now each surface as a system message… Worth confirming the known non-message types and adding the metadata-only ones to _SKIP_ENTRY_TYPES.

What your commit does:

1. utils/jsonl_parser.py — adds "summary" to _SKIP_ENTRY_TYPES:

   # Before:
   _SKIP_ENTRY_TYPES = frozenset({"file-history-snapshot"})
   
   # After:
   _SKIP_ENTRY_TYPES = frozenset({"file-history-snapshot", "summary"})

2. tests/test_jsonl_parser.py — adds test_summary_entry_type_produces_no_message which asserts:

   • len(s["messages"]) == 0 (no spurious system message)
   • entry_counts.get("summary") == 1 (still counted for metadata)
   • "Unknown message role" not in caplog.text (no warning noise)

That's a clean, well-tested fix that matches exactly what @timon0305 requested. ✅

As a bonus, the same commit also switched to _log = logging.getLogger(__name__), which resolves the 🟡 Issue 2 I flagged about the root logger.