Address PR review: metadata parity guard, runtime validation, shared field names

- Add SESSION_METADATA_FIELD_NAMES constant and _new_session_metadata_builder()
- Drive _finalize_session_metadata() from the field-name set (loop, not duplicate list)
- Validate required metadata keys in validate_session_dict at runtime
- Add parity tests and metadata validation tests
- Ignore non-str timestamps when accumulating first_timestamp/last_timestamp

Author: clean6378-max-it

models/session.py

@@ -97,6 +97,45 @@ class SessionMetadataDict(TypedDict):
     entry_counts: NotRequired[dict[str, int]]
 
 
+# Canonical metadata field set for parse_session builder / finalize parity.
+# Keep in sync with SessionMetadataDict above.
+SESSION_METADATA_REQUIRED_KEYS = frozenset({"session_id", "models_used", "first_timestamp"})
+
+SESSION_METADATA_FIELD_NAMES = frozenset(
+    {
+        "session_id",
+        "models_used",
+        "first_timestamp",
+        "last_timestamp",
+        "total_input_tokens",
+        "total_output_tokens",
+        "total_cache_read_tokens",
+        "total_cache_creation_tokens",
+        "total_tool_calls",
+        "tool_call_counts",
+        "version",
+        "cwd",
+        "git_branch",
+        "permission_mode",
+        "compactions",
+        "total_ephemeral_5m_tokens",
+        "total_ephemeral_1h_tokens",
+        "service_tiers",
+        "session_wall_time_seconds",
+        "compact_boundaries",
+        "api_errors",
+        "files_read",
+        "files_written",
+        "files_created",
+        "bash_commands",
+        "web_fetches",
+        "sidechain_messages",
+        "stop_reasons",
+        "entry_counts",
+    }
+)
+
+
 class SessionDict(TypedDict):
     session_id: str
     title: str

tests/test_jsonl_parser.py

@@ -7,14 +7,20 @@
 
 sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))
 
+from models.session import SESSION_METADATA_FIELD_NAMES
 from utils.jsonl_helpers import (
     extract_images,
     extract_text,
     infer_title,
     normalize_content,
     strip_system_tags,
 )
-from utils.jsonl_parser import parse_session, quick_session_info
+from utils.jsonl_parser import (
+    _finalize_session_metadata,
+    _new_session_metadata_builder,
+    parse_session,
+    quick_session_info,
+)
 from utils.tool_dispatch import _parse_tool_result
 
 # ---------------------------------------------------------------------------
@@ -38,6 +44,18 @@ def _parse_entries(entries: list) -> dict:
         os.unlink(path)
 
 
+class TestSessionMetadataFinalize:
+    def test_builder_keys_match_field_names_constant(self):
+        raw = _new_session_metadata_builder("parity-test")
+        assert set(raw.keys()) == SESSION_METADATA_FIELD_NAMES
+
+    def test_finalize_preserves_all_builder_keys(self):
+        raw = _new_session_metadata_builder("parity-test")
+        finalized = _finalize_session_metadata(raw)
+        assert set(finalized.keys()) == SESSION_METADATA_FIELD_NAMES
+        assert set(finalized.keys()) == set(raw.keys())
+
+
 # ---------------------------------------------------------------------------
 # _parse_tool_result
 # ---------------------------------------------------------------------------

tests/test_jsonl_validation.py

@@ -60,6 +60,35 @@ def test_metadata_not_dict(self):
             validate_session_dict(_valid_payload(metadata="not-a-dict"))
         assert exc_info.value.path == "metadata"
 
+    def test_metadata_missing_session_id(self):
+        with pytest.raises(SessionValidationError) as exc_info:
+            validate_session_dict(
+                _valid_payload(metadata={"models_used": [], "first_timestamp": None})
+            )
+        assert exc_info.value.path == "metadata.session_id"
+
+    def test_metadata_missing_models_used(self):
+        with pytest.raises(SessionValidationError) as exc_info:
+            validate_session_dict(
+                _valid_payload(metadata={"session_id": "abc123", "first_timestamp": None})
+            )
+        assert exc_info.value.path == "metadata.models_used"
+
+    def test_metadata_missing_first_timestamp(self):
+        with pytest.raises(SessionValidationError) as exc_info:
+            validate_session_dict(
+                _valid_payload(metadata={"session_id": "abc123", "models_used": []})
+            )
+        assert exc_info.value.path == "metadata.first_timestamp"
+
+    def test_metadata_first_timestamp_null_allowed(self):
+        result = validate_session_dict(
+            _valid_payload(
+                metadata={"session_id": "abc123", "models_used": [], "first_timestamp": None}
+            )
+        )
+        assert result["metadata"]["first_timestamp"] is None
+
     def test_message_not_dict(self):
         with pytest.raises(SessionValidationError) as exc_info:
             validate_session_dict(_valid_payload(messages=["not-a-dict"]))

utils/jsonl_parser.py

@@ -9,7 +9,14 @@
 from typing import Any, cast, get_args
 
 from models.record_data import RecordDataUnion
-from models.session import MessageDict, RoleLiteral, SessionDict, SessionMetadataDict, ToolUseDict
+from models.session import (
+    SESSION_METADATA_FIELD_NAMES,
+    MessageDict,
+    RoleLiteral,
+    SessionDict,
+    SessionMetadataDict,
+    ToolUseDict,
+)
 from models.tool_results import ToolResultUnion, is_tool_result_dict
 from utils.jsonl_helpers import (
     entry_message as _entry_message,
@@ -70,48 +77,15 @@ def _safe_int(val: Any) -> int:
     return 0
 
 
-def _finalize_session_metadata(raw: dict[str, Any]) -> SessionMetadataDict:
-    """Convert the mutable parse-time metadata builder into a SessionMetadataDict."""
-    return {
-        "session_id": raw["session_id"],
-        "models_used": sorted(raw["models_used"]),
-        "first_timestamp": raw["first_timestamp"],
-        "last_timestamp": raw["last_timestamp"],
-        "total_input_tokens": raw["total_input_tokens"],
-        "total_output_tokens": raw["total_output_tokens"],
-        "total_cache_read_tokens": raw["total_cache_read_tokens"],
-        "total_cache_creation_tokens": raw["total_cache_creation_tokens"],
-        "total_tool_calls": raw["total_tool_calls"],
-        "tool_call_counts": raw["tool_call_counts"],
-        "version": raw["version"],
-        "cwd": raw["cwd"],
-        "git_branch": raw["git_branch"],
-        "permission_mode": raw["permission_mode"],
-        "compactions": raw["compactions"],
-        "total_ephemeral_5m_tokens": raw["total_ephemeral_5m_tokens"],
-        "total_ephemeral_1h_tokens": raw["total_ephemeral_1h_tokens"],
-        "service_tiers": sorted(raw["service_tiers"]),
-        "session_wall_time_seconds": raw["session_wall_time_seconds"],
-        "compact_boundaries": raw["compact_boundaries"],
-        "api_errors": raw["api_errors"],
-        "files_read": sorted(raw["files_read"]),
-        "files_written": sorted(raw["files_written"]),
-        "files_created": sorted(raw["files_created"]),
-        "bash_commands": raw["bash_commands"],
-        "web_fetches": raw["web_fetches"],
-        "sidechain_messages": raw["sidechain_messages"],
-        "stop_reasons": raw["stop_reasons"],
-        "entry_counts": raw["entry_counts"],
-    }
+# Set-backed fields are sorted when finalized; all other builder keys pass through as-is.
+_METADATA_SET_FIELDS = frozenset(
+    {"models_used", "service_tiers", "files_read", "files_written", "files_created"}
+)
 
 
-def parse_session(filepath: str) -> SessionDict:
-    """Main entry point. Reads every line from a .jsonl file and builds up
-    a session dict with messages, metadata (tokens, models, tool counts),
-    and file/command activity."""
-    session_id = os.path.basename(filepath).replace(".jsonl", "")
-    messages: list[MessageDict] = []
-    metadata: dict[str, Any] = {
+def _new_session_metadata_builder(session_id: str) -> dict[str, Any]:
+    """Fresh mutable metadata accumulator for ``parse_session()``."""
+    return {
         "session_id": session_id,
         "models_used": set(),
         "total_input_tokens": 0,
@@ -127,30 +101,42 @@ def parse_session(filepath: str) -> SessionDict:
         "git_branch": None,
         "permission_mode": None,
         "compactions": 0,
-        # Extended token accounting
         "total_ephemeral_5m_tokens": 0,
         "total_ephemeral_1h_tokens": 0,
         "service_tiers": set(),
-        # Timing
         "session_wall_time_seconds": None,
-        # Compaction details
         "compact_boundaries": [],
-        # Error tracking
         "api_errors": 0,
-        # File activity (from tool_use inputs)
         "files_read": set(),
         "files_written": set(),
         "files_created": set(),
         "bash_commands": [],
         "web_fetches": [],
-        # Sidechain tracking
         "sidechain_messages": 0,
-        # Stop reasons
         "stop_reasons": {},
-        # Entry type counts
         "entry_counts": {},
     }
 
+
+def _finalize_session_metadata(raw: dict[str, Any]) -> SessionMetadataDict:
+    """Convert the mutable parse-time metadata builder into a SessionMetadataDict."""
+    finalized: dict[str, Any] = {}
+    for key in SESSION_METADATA_FIELD_NAMES:
+        val = raw[key]
+        if key in _METADATA_SET_FIELDS:
+            val = sorted(val)
+        finalized[key] = val
+    return cast(SessionMetadataDict, finalized)
+
+
+def parse_session(filepath: str) -> SessionDict:
+    """Main entry point. Reads every line from a .jsonl file and builds up
+    a session dict with messages, metadata (tokens, models, tool counts),
+    and file/command activity."""
+    session_id = os.path.basename(filepath).replace(".jsonl", "")
+    messages: list[MessageDict] = []
+    metadata = _new_session_metadata_builder(session_id)
+
     with open(filepath, "r", encoding="utf-8", errors="replace") as f:
         for line in f:
             line = line.strip()
@@ -172,7 +158,7 @@ def parse_session(filepath: str) -> SessionDict:
                 if isinstance(snap, dict):
                     ts = snap.get("timestamp")
 
-            if ts:
+            if isinstance(ts, str) and ts:
                 if metadata["first_timestamp"] is None:
                     metadata["first_timestamp"] = ts
                 metadata["last_timestamp"] = ts

utils/validation.py

@@ -37,6 +37,23 @@ def _require_value(
     return val
 
 
+def _require_optional_str(path: str, val: Any) -> str | None:
+    if val is None:
+        return None
+    if not isinstance(val, str):
+        raise SessionValidationError(path, f"expected str or null, got {type(val).__name__}")
+    return val
+
+
+def _validate_session_metadata(metadata: dict[str, Any]) -> None:
+    """Enforce SessionMetadataDict required keys at the runtime boundary."""
+    _require_field(metadata, "session_id", str, "str", path="metadata.session_id")
+    _require_field(metadata, "models_used", list, "list", path="metadata.models_used")
+    if "first_timestamp" not in metadata:
+        raise SessionValidationError("metadata.first_timestamp", "missing required field")
+    _require_optional_str("metadata.first_timestamp", metadata["first_timestamp"])
+
+
 def validate_session_dict(data: dict[str, Any]) -> SessionDict:
     """Validate a plain dict matches SessionDict before returning it."""
     # Runtime guard for dynamic callers; mypy already types the parameter as dict.
@@ -46,7 +63,8 @@ def validate_session_dict(data: dict[str, Any]) -> SessionDict:
     _require_field(data, "session_id", str, "str")
     _require_field(data, "title", str, "str")
     messages = _require_field(data, "messages", list, "list")
-    _require_field(data, "metadata", dict, "dict")
+    metadata = _require_field(data, "metadata", dict, "dict")
+    _validate_session_metadata(metadata)
 
     for index, message in enumerate(messages):
         path = f"messages[{index}]"