feat: branch-aware conversation processing with SQLite persistence#14
Closed
Sinity wants to merge 3 commits into
Closed
feat: branch-aware conversation processing with SQLite persistence#14Sinity wants to merge 3 commits into
Sinity wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughThis PR introduces a branching system for multi-path conversations alongside SQLite-backed persistence, enhanced importers, and CLI refinements. It normalizes footnote formatting, tracks word counts, adds a Changes
Sequence DiagramsequenceDiagram
actor User
participant Importer
participant MessageBuilder as Message<br/>Builder
participant Branching as Branching<br/>Algorithm
participant ConvProc as Conversation<br/>Processor
participant Rendering as Markdown<br/>Renderer
participant DB as SQLite<br/>Database
User->>Importer: import_*_export(force=True)
Importer->>MessageBuilder: Extract & build MessageRecord list
activate MessageBuilder
MessageBuilder->>MessageBuilder: Track message_id, parent_id, role, text, timestamps
MessageBuilder->>MessageBuilder: Apply normalise_inline_footnotes()
deactivate MessageBuilder
Importer->>Branching: build_branch_plan(message_records)
activate Branching
Branching->>Branching: Build node & parent→children map
Branching->>Branching: Identify root messages (cycle detection)
Branching->>Branching: Enumerate root-to-leaf paths (DFS)
Branching->>Branching: Select canonical path, assign branches
deactivate Branching
Importer->>ConvProc: process_conversation(messages, branches, force)
activate ConvProc
ConvProc->>ConvProc: Compute per-branch stats (tokens, words)
ConvProc->>Rendering: Render canonical branch to Markdown
activate Rendering
Rendering->>Rendering: Build timestamped chunks with relative deltas
Rendering->>Rendering: Compute totalWordsApprox, inputWordsApprox
Rendering-->>ConvProc: Return Document + metadata
deactivate Rendering
ConvProc->>DB: upsert_conversation(stats, dirty flag)
ConvProc->>DB: replace_branches(branch metadata)
ConvProc->>DB: replace_messages(per-branch messages)
ConvProc->>ConvProc: Write branch-aware file structure<br/>(canonical, conversation.md, branches/)
deactivate ConvProc
Importer-->>User: ImportResult(branch_count, paths)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
Sinity
added a commit
that referenced
this pull request
Apr 7, 2026
#14) Introduce a unified process_conversation() pipeline that all four importers now funnel through. Each import builds a message tree from parent-child IDs, computes a branch plan, persists metadata to SQLite, and writes a branch-aware filesystem layout alongside the canonical transcript. Core pipeline (polylogue/branching.py, conversation.py, db.py): - MessageRecord dataclass carries per-message identity, parentage, token/word counts, and the original chunk payload - build_branch_plan() walks the message DAG iteratively (no recursion) to identify branches; longest path wins canonical status - process_conversation() orchestrates: plan → SQLite upsert → canonical render → per-branch directory tree with overlay Markdown - New SQLite schema at XDG_STATE_HOME/polylogue/polylogue.db stores conversations, branches, and messages with FTS5 full-text index Importer refactoring (chatgpt, claude_ai, claude_code, codex): - All four importers build MessageRecord lists with parent_id linkage and delegate to process_conversation() instead of calling build_markdown_from_chunks + persist_document directly - Pydantic models (_ResponseItem, _Payload, ChunkModel, ContentPart) removed; replaced with plain-dict _sanitise_chunk() in models.py and direct JSON parsing in the Codex importer - --force flag wired through all import subcommands to overwrite locally-edited files - normalise_inline_footnotes() in importers/utils.py strips escaped \[N\] markers so citations render cleanly across all providers Rendering (polylogue/render.py): - Word counts (totalWordsApprox, inputWordsApprox, outputWordsApprox) tracked alongside token counts in stats and front matter - Per-turn timestamps show UTC with relative offset from conversation start, e.g. "2024-04-30T19:00:46Z (+22s)" - firstMessageTime, lastMessageTime, conversationDurationSeconds added to document metadata from chunk timestamp bounds - _prepare_chunks_for_render() drops empty chunks before stat computation so counts match visible output - sourceDriveId renamed to sourceId; empty attachment arrays omitted - ChatGPT citation tokens (U+E200..U+E201) resolved to Markdown footnotes referencing files, webpages, or live market quotes CLI formatting (polylogue/cli.py, cli_common.py): - Summary tables display "Tokens (~words)" columns and branch counts - Session picker gains column alignment and default skim key bindings (tab toggle, ctrl-a select-all, alt-a accept-all) - JSON sync payloads made null-safe for missing document metadata Opportunistic fixes: - polylogue.py entry-point shim deleted (CLI entry is the package) - ui.py: PlainConsole fallback when Rich is not installed; Panel rendering handles bracket-containing text without markup errors - util.py: slugify_title() for Unicode-safe kebab-case slug generation - document_store.py: _prune_empty_values() strips null/empty fields from persisted polylogue metadata Tests: - test_branching.py: 1500-node deep chains, cycle detection - test_render_utils.py: citation injection, time bounds metadata - test_importers_multi.py: footnote unescaping, --force overwrite, timestamp formatting, branch directory creation for each provider - test_codex_importer.py: footnote normalisation, word count presence - test_stats.py: word count propagation through stats pipeline - test_ui.py: Rich Panel with bracket text does not raise MarkupError
![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: ec32f1d52f
ℹ️ 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".
Comment on lines
+132
to
+134
| def path_score(path: List[str]) -> Tuple[int, Optional[str]]: | ||
| last_ts = node_map[path[-1]].timestamp if path else None | ||
| return (len(path), last_ts) # length first, then timestamp ordering |
There was a problem hiding this comment.
build_branch_plan() ranks candidate paths with path_score = (len(path), last_ts), but last_ts can be a mix of None, numeric epoch values, and ISO strings depending on provider/message metadata. When two paths have equal length and their leaf timestamp types differ, Python tuple comparison raises TypeError, which aborts import for branched conversations. This is reproducible with one branch leaf missing a timestamp and another having a string timestamp.
Useful? React with 👍 / 👎.
Comment on lines
+170
to
+173
| conn.execute( | ||
| "DELETE FROM branches WHERE provider = ? AND conversation_id = ?", | ||
| (provider, conversation_id), | ||
| ) |
There was a problem hiding this comment.
replace_branches() deletes rows from branches, which cascades messages, but it never clears messages_fts for the same conversation. Because replace_messages() only deletes FTS rows for currently reinserted branch IDs, any branch that disappears between imports leaves orphaned full-text rows behind, causing stale search results from deleted branches.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR centralizes all provider importers behind a single process_conversation() pipeline that preserves conversation branching, persists queryable metadata in SQLite, and improves rendered Markdown with per-turn timestamps plus deterministic word-count stats.
Changes:
- Add branch planning (
build_branch_plan) and a shared orchestration pipeline (process_conversation) used by all importers. - Introduce SQLite persistence for conversations/branches/messages (plus FTS) and propagate word-count stats throughout CLI/status reporting.
- Improve rendering and importer normalization: per-turn timestamps, inline footnote unescaping, ChatGPT citation token injection,
--forceoverwrite behavior, and optional-Rich UI hardening.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_ui.py | Adds regression test to ensure UI summary rendering tolerates bracketed text without Rich markup errors. |
| tests/test_stats.py | Extends stats tests to include totalWordsApprox aggregation and provider breakdowns. |
| tests/test_render_utils.py | Adds unit tests for ChatGPT citation injection and conversation timestamp bounds/duration metadata. |
| tests/test_importers_multi.py | Expands multi-importer tests for branching outputs, timestamps, inline footnote normalization, and --force overwrite behavior. |
| tests/test_codex_importer.py | Adds Codex tests asserting word counts and footnote normalization behavior. |
| tests/test_branching.py | Introduces tests for deep DAG traversal (non-recursive) and cycle handling in branch planning. |
| README.md | Documents quality gates and new branch-aware output layout + SQLite DB location. |
| polylogue/util.py | Introduces slugify_title and switches slug assignment to be more kebab-case oriented. |
| polylogue/ui.py | Makes Rich optional and wraps panel content in Text to avoid markup parsing issues. |
| polylogue/render.py | Adds word counting, per-turn timestamp formatting, chunk pre-filtering, and new conversation time metadata. |
| polylogue/models.py | Removes Pydantic-based chunk validation in favor of lightweight dict sanitization. |
| polylogue/local_sync.py | Tracks and reports word totals alongside token totals for local sync results. |
| polylogue/importers/utils.py | Adds normalise_inline_footnotes() to unescape provider-exported \\[n\\] markers safely. |
| polylogue/importers/codex.py | Refactors Codex importer to emit MessageRecords and delegate writing/persistence to process_conversation(); adds --force. |
| polylogue/importers/claude_code.py | Refactors Claude Code importer to process_conversation(), normalizes footnotes, and adds --force. |
| polylogue/importers/claude_ai.py | Refactors Claude AI importer to process_conversation(), captures parent IDs where present, normalizes footnotes, and adds --force. |
| polylogue/importers/chatgpt.py | Refactors ChatGPT importer to process_conversation(), injects citations, normalizes footnotes, captures node parent pointers, and adds --force. |
| polylogue/importers/base.py | Extends ImportResult to include branch counts and branch directory paths. |
| polylogue/document_store.py | Improves slug generation fallback and prunes empty polylogue metadata/state values. |
| polylogue/db.py | Adds SQLite schema v1, connection management, and upsert/replace helpers (including FTS5 population). |
| polylogue/conversation.py | New shared conversation pipeline: branch planning, SQLite upserts, canonical + branch filesystem rendering, and persistence delegation. |
| polylogue/commands.py | Adds word stats to render/sync/status aggregation payloads. |
| polylogue/cli.py | Adds --force to import commands, propagates word stats into summaries/tables/JSON, and improves skim selection UX. |
| polylogue/cli_common.py | Adds default skim multi-select bindings and prompt/cycle support. |
| polylogue/branching.py | New branch planner that builds branch paths iteratively with cycle detection and canonical selection. |
| docs/providers/openai-codex.md | Documents --force, footnote normalization, word stats, SQLite, and branch-aware outputs for Codex. |
| docs/providers/claude-code.md | Documents --force, footnote normalization, word stats, SQLite, and branch-aware outputs for Claude Code. |
| docs/providers/claude-ai.md | Documents --force, footnote normalization, word stats, SQLite, and branch-aware outputs for Claude AI. |
| docs/providers/chatgpt.md | Documents --force, footnote normalization, timestamps, word stats, SQLite, and branch-aware outputs for ChatGPT. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
107
to
117
| created = msg.get("create_time") or node.get("create_time") | ||
| messages.append( | ||
| { | ||
| "id": msg.get("id") or node.get("id"), | ||
| "author": author, | ||
| "content": msg.get("content"), | ||
| "metadata": msg.get("metadata") or {}, | ||
| "create_time": created, | ||
| "node_id": node.get("id"), | ||
| "parent": node.get("parent"), | ||
| } |
There was a problem hiding this comment.
ChatGPT export mapping uses node IDs for the parent pointers, but _gather_messages() sets id to the message ID when present while also capturing parent as the node parent. That mixes ID namespaces (message-id vs node-id), so parent_id usually won’t exist in MessageRecord IDs and the branch DAG will collapse into multiple roots / incorrect branches. Consider using the node ID as the stable MessageRecord.message_id (and store the message UUID separately), or translate parent node IDs to the corresponding message IDs before building MessageRecords.
Comment on lines
+313
to
+321
| return process_conversation( | ||
| provider="chatgpt", | ||
| conversation_id=conversation_id, | ||
| slug=slug, | ||
| title=title, | ||
| document=document, | ||
| output_dir=output_dir, | ||
| collapse_threshold=collapse_threshold, | ||
| message_records=message_records, | ||
| attachments=attachments, | ||
| updated_at=conv.get("update_time") or conv.get("modified_time"), | ||
| created_at=conv.get("create_time"), | ||
| canonical_leaf_id=conv.get("current_node"), | ||
| collapse_threshold=collapse_threshold, |
There was a problem hiding this comment.
canonical_leaf_id is passed as conv.get("current_node"), which is a ChatGPT node ID. If MessageRecord.message_id remains a message UUID (or a mix of message/node IDs), the canonical leaf selection will never match and the canonical branch can be wrong. Once IDs are normalized, ensure canonical_leaf_id uses the same ID namespace as MessageRecord.message_id (likely the node ID).
|
|
||
| canonical_path: List[str] | ||
| if canonical_leaf_id: | ||
| canonical_candidates = [p for p in all_paths if canonical_leaf_id in p] |
There was a problem hiding this comment.
build_branch_plan() chooses the canonical path by checking canonical_leaf_id in p, but canonical_leaf_id is described/used as a leaf identifier. If the ID appears earlier in a longer path, this can select the wrong canonical branch. Prefer matching p[-1] == canonical_leaf_id (or reconstruct the exact root→leaf path) so the canonical branch is deterministic and actually ends at the requested leaf.
Suggested change
| canonical_candidates = [p for p in all_paths if canonical_leaf_id in p] | |
| canonical_candidates = [p for p in all_paths if p and p[-1] == canonical_leaf_id] |
| token_count=total_tokens, | ||
| word_count=total_words, | ||
| attachment_bytes=sum(att.size_bytes or 0 for att in attachments), | ||
| dirty=False, |
There was a problem hiding this comment.
SQLite is upserted before persist_document() runs, but the row is written with content_hash=None and dirty=False unconditionally. This makes the DB metadata inaccurate (and it can still be updated even when persist_document() skips due to dirty-local). Consider moving the conversation upsert until after persistence, or doing a second update using persist_result.content_hash and persist_result.dirty/skipped so the DB reflects what was actually written.
Suggested change
| dirty=False, | |
| dirty=True, |
Comment on lines
+261
to
+272
| if not persist_result.skipped and persist_result.markdown_path: | ||
| conversation_dir = persist_result.markdown_path.parent / persist_result.slug | ||
| branches_dir = conversation_dir / "branches" | ||
| attachments_dir = persist_result.attachments_dir | ||
| conversation_dir.mkdir(parents=True, exist_ok=True) | ||
| branches_dir.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| canonical_path = conversation_dir / "conversation.md" | ||
| canonical_path.write_text(persist_result.document.to_markdown() if persist_result.document else canonical_document.to_markdown(), encoding="utf-8") | ||
|
|
||
| common_ids = _common_prefix(plan) | ||
| common_records = [records_by_id[mid] for mid in common_ids if mid in records_by_id] |
There was a problem hiding this comment.
When persist_document() skips (e.g. dirty-local without --force), branch files aren’t written (guarded by if not persist_result.skipped), but the SQLite branches/messages tables have already been replaced. That leaves the DB describing a branch tree that isn’t present on disk. Consider short-circuiting DB writes on skip, or persisting a dirty/skipped flag and only replacing branch/message rows when the canonical document write succeeds.
Suggested change
| if not persist_result.skipped and persist_result.markdown_path: | |
| conversation_dir = persist_result.markdown_path.parent / persist_result.slug | |
| branches_dir = conversation_dir / "branches" | |
| attachments_dir = persist_result.attachments_dir | |
| conversation_dir.mkdir(parents=True, exist_ok=True) | |
| branches_dir.mkdir(parents=True, exist_ok=True) | |
| canonical_path = conversation_dir / "conversation.md" | |
| canonical_path.write_text(persist_result.document.to_markdown() if persist_result.document else canonical_document.to_markdown(), encoding="utf-8") | |
| common_ids = _common_prefix(plan) | |
| common_records = [records_by_id[mid] for mid in common_ids if mid in records_by_id] | |
| if persist_result.markdown_path: | |
| conversation_dir = persist_result.markdown_path.parent / persist_result.slug | |
| branches_dir = conversation_dir / "branches" | |
| attachments_dir = persist_result.attachments_dir | |
| conversation_dir.mkdir(parents=True, exist_ok=True) | |
| branches_dir.mkdir(parents=True, exist_ok=True) | |
| if not persist_result.skipped: | |
| canonical_path = conversation_dir / "conversation.md" | |
| canonical_path.write_text(persist_result.document.to_markdown() if persist_result.document else canonical_document.to_markdown(), encoding="utf-8") | |
| common_ids = _common_prefix(plan) | |
| common_records = [records_by_id[mid] for mid in records_by_id if mid in common_ids] |
Comment on lines
+161
to
+162
| fallback = sanitize_filename(base_source) | ||
| base = fallback.replace(" ", "-") or (conversation_id[:8] if conversation_id else "conversation") |
There was a problem hiding this comment.
If slugify_title() produces an empty slug (e.g. titles that are entirely non-ASCII / emoji), the fallback uses sanitize_filename() and only replaces spaces with -, which can still yield non-kebab-case/unicode slugs. If the intent is deterministic kebab-case, consider falling back to conversation_id[:8] (or a hash) rather than emitting a unicode filename.
Suggested change
| fallback = sanitize_filename(base_source) | |
| base = fallback.replace(" ", "-") or (conversation_id[:8] if conversation_id else "conversation") | |
| base = conversation_id[:8] if conversation_id else "conversation" |
Comment on lines
+202
to
+211
| def slugify_title(value: str) -> str: | ||
| if not value: | ||
| return "" | ||
| normalized = unicodedata.normalize("NFKD", value) | ||
| ascii_text = normalized.encode("ascii", "ignore").decode("ascii") | ||
| ascii_text = ascii_text.lower() | ||
| slug = re.sub(r"[^a-z0-9]+", "-", ascii_text) | ||
| slug = slug.strip("-") | ||
| slug = re.sub(r"-+", "-", slug) | ||
| return slug |
There was a problem hiding this comment.
sanitize_filename() previously enforced a byte-length cap (max_len = 200) to avoid OS filename/path limits. slugify_title() doesn’t apply any length trimming, so very long titles can generate slugs that exceed filesystem limits and make imports fail. Consider truncating the slug (ideally by bytes, like sanitize_filename) and/or reusing that helper after slugification.
Comment on lines
+84
to
+94
| except json.JSONDecodeError: | ||
| fallback = raw.strip() | ||
| if not fallback: | ||
| continue | ||
| chunk_text = "Unparsed Codex log line\n```\n{}```".format(fallback) | ||
| chunks.append( | ||
| { | ||
| "role": "model", | ||
| "text": chunk_text, | ||
| "tokenCount": estimate_token_count(chunk_text, model="gpt-5-codex"), | ||
| } |
There was a problem hiding this comment.
The fallback chunk_text code fence for JSON decode failures is missing a newline before the closing backticks (...\n```\n{}````). This can break Markdown rendering because the closing fence is on the same line as the payload. Consider formatting as ...\n\n{payload}\n\n` (and similarly for the 'Unparsed ... entry' case).
Comment on lines
+52
to
+67
| except json.JSONDecodeError: | ||
| fallback = line.strip() | ||
| if not fallback: | ||
| continue | ||
| chunk_text = "Unparsed Claude Code log line\n```\n{}```".format(fallback) | ||
| chunks.append( | ||
| { | ||
| "role": "model", | ||
| "text": chunk_text, | ||
| "tokenCount": estimate_token_count(chunk_text, model="claude-code"), | ||
| } | ||
| ) | ||
| continue | ||
| if not isinstance(entry, dict): | ||
| chunk_text = "Unparsed Claude Code entry\n```\n{}```".format(entry) | ||
| chunks.append( |
There was a problem hiding this comment.
The fallback chunk_text code fence for JSON decode / type failures is missing a newline before the closing backticks (...\n```\n{}````). This can break Markdown rendering because the closing fence is on the same line as the payload. Consider formatting as ...\n\n{payload}\n\n` for both 'Unparsed ... log line' and 'Unparsed ... entry'.
| from pathlib import Path | ||
| from typing import Dict, Iterable, List, Optional, Sequence, Tuple | ||
|
|
||
| from .branching import BranchPlan, BranchInfo, MessageRecord, build_branch_plan |
There was a problem hiding this comment.
BranchInfo is imported but not used in this module. Consider removing it to avoid dead imports (and to keep future linting clean).
Suggested change
| from .branching import BranchPlan, BranchInfo, MessageRecord, build_branch_plan | |
| from .branching import BranchPlan, MessageRecord, build_branch_plan |
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
polylogue/importers/chatgpt.py (1)
384-426:⚠️ Potential issue | 🔴 CriticalSanitize exported attachment names before joining them into
attachments_dir.
namecomes straight from export metadata, so../segments or absolute paths can escape the attachments folder and overwrite arbitrary files on disk.🛡️ Suggested fix
- name = ref.get("name") or ref.get("filename") or file_id + raw_name = ref.get("name") or ref.get("filename") or file_id + name = Path(str(raw_name)).name if raw_name else None if not name: return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@polylogue/importers/chatgpt.py` around lines 384 - 426, The attachment name from metadata (variable name) is untrusted and may contain path traversal or absolute paths; sanitize it before constructing target by deriving a safe filename (e.g., use pathlib.PurePath(name).name or os.path.basename and strip/replace dangerous characters) and then build target = attachments_dir / safe_name; after writing, validate that target.resolve().is_relative_to(attachments_dir.resolve()) (or catch ValueError from relative_to) to ensure it did not escape the attachments_dir, and if validation fails, skip or log and treat as remote; update all uses of name when creating AttachmentInfo and per_chunk_links to use the sanitized safe_name or the validated relative path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@polylogue/branching.py`:
- Around line 177-185: The code sets BranchInfo.depth to len(path) (message-path
length) but should store the branch-tree depth (distance from root); update the
branch creation in the block that calls assign_branch_id() so depth = 0 for root
(parent_branch_id is None) and otherwise depth =
branches[parent_branch_id].depth + 1 (or compute from whatever parent BranchInfo
store is used). Modify the BranchInfo construction (the branches[...] =
BranchInfo(...) call) to use that computed tree depth instead of len(path),
referencing assign_branch_id, branches, BranchInfo, and parent_branch_id to
locate and compute the correct value.
- Around line 96-130: The current logic only builds paths starting from roots
(roots = _find_root_ids(...)) and thus drops disconnected/rootless components
(e.g., cycles) when roots exist; fix by after building all_paths from roots
compute covered_ids = set().union(*all_paths) (handle empty), then determine
remaining_ids = set(node_map.keys()) - covered_ids and call
_build_paths(node_map, child_map, node_id) for each node_id in remaining_ids (or
a single call per remaining node) and extend all_paths with those results so
cyclic or rootless components are traversed and included in the branch plan;
update variable names all_paths, roots, node_map, child_map, and use
_build_paths to locate the change.
- Around line 132-163: The path_score function returns (len(path), last_ts)
where last_ts can be None, causing TypeError when tuples with None and str are
compared by max()/sorted(); update path_score to return a totally ordered tuple
by normalizing the timestamp (e.g., replace None with a consistent sentinel
string or numeric value) so comparisons never see None; ensure callers
(max(all_paths, key=path_score) and sorted(all_paths, key=path_score)) will now
work without raising when MessageRecord.timestamp is Optional[str].
In `@polylogue/conversation.py`:
- Around line 169-170: The attachment_bytes and markdown/metadata generation use
the conversation-wide attachments list, causing non-canonical-branch files to be
included; update the logic that builds the top-level export (the code that
computes attachment_bytes and renders the top-level file) to first filter
attachments by the conversation's canonical branch (use
conversation.canonical_branch) and then compute attachment_bytes =
sum(att.size_bytes or 0 for att in filtered_attachments) and pass
filtered_attachments into the renderer; apply the same filtering change to the
other block mentioned around the render/export code at the 224-239 region so
both places only include attachments from the canonical branch.
- Around line 157-172: The conversation row is being written too early:
upsert_conversation(...) is called with content_hash=None and dirty=False before
persist_document(...) runs, so the DB never records the real file hash and local
skips get stored as clean; move the upsert_conversation(...) call to after
persist_document(...) completes (use the return/metadata from persist_document
to obtain the actual content_hash and whether the save was skipped/dirty) and
pass that content_hash and the correct dirty boolean into upsert_conversation;
apply the same fix for the other occurrence around lines 222-258 (ensure you use
the result from persist_document to set content_hash and dirty when calling
upsert_conversation).
- Around line 261-266: The current guard (if not persist_result.skipped and
persist_result.markdown_path) prevents creation of branch and conversation
layout when markdown is skipped; change the logic so directory creation is gated
only on persist_result.markdown_path (not on persist_result.skipped) and keep
writing/updating the canonical markdown file conditional on not
persist_result.skipped. Concretely, always create conversation_dir, branches_dir
and ensure persist_result.attachments_dir exists when
persist_result.markdown_path is present, and only skip the writing of
conversation.md / conversation.common.md when persist_result.skipped is True;
reference persist_result.markdown_path, persist_result.skipped,
persist_result.slug, conversation_dir, branches_dir, and
persist_result.attachments_dir to locate and update the code.
In `@polylogue/db.py`:
- Around line 170-173: The branch deletion currently removes rows from branches
(via conn.execute) which cascades to messages but does not touch the standalone
FTS index; update the same removal path to also delete orphaned FTS entries by
executing a DELETE against messages_fts for message rows that belonged to the
removed branch (identify them via messages.provider/messages.conversation_id or
messages.id as appropriate). In other words, after the existing
conn.execute("DELETE FROM branches WHERE provider = ? AND conversation_id = ?",
(provider, conversation_id)), add a second deletion that removes entries from
messages_fts that correspond to the messages removed by that branch deletion so
messages_fts stays in sync with messages and replace_messages only needs to
handle existing branches.
In `@polylogue/document_store.py`:
- Around line 410-415: The persisted attachmentsDir is never deleted because you
filter out None values before calling update_conversation_state; instead pass
explicit None for attachmentsDir so the updater can remove it, and make
update_conversation_state treat None as "delete this key" (not "ignore");
specifically, in document_store.py ensure you set
state_payload["attachmentsDir"] = None when attachments is falsy (and stop
dropping None keys), and modify update_conversation_state(...) to detect keys
with value None and remove those fields from the stored conversation state
(rather than merging/ignoring them).
In `@polylogue/importers/chatgpt.py`:
- Around line 462-476: The replacer() advances cite_index for every marker which
causes hidden/tool citations to consume public citation slots; instead, when
handling the citation branch in replacer(), advance cite_index forward to the
next visible citation by skipping entries where citation.get("hidden") or
citation.get("tool") is truthy (e.g., while cite_index < total_cites and
(citations[cite_index].get("hidden") or citations[cite_index].get("tool")):
cite_index += 1), then if still within range use that citation and increment
cite_index once to mark it consumed; do not change cite_index in the ref_map
branch. This update touches replacer(), the citations list, cite_index,
total_cites, ref_map, and the ensure_label/_format_citation_description calls.
In `@polylogue/importers/claude_code.py`:
- Around line 76-92: The code assumes entry.get("message") is a dict when
computing entry_id and parent_id (and later in _extract_text), which can raise
if message is a string; change these sites to first grab message =
entry.get("message") and check isinstance(message, dict) before calling
message.get(...), falling back to the existing alternatives (e.g.,
entry.get("id") or f"line-{line_idx}"); likewise update the _extract_text
call/site to handle a non-dict message (skip message.get access or treat
non-dict message as text) so a malformed line isolates a bad entry instead of
aborting the whole import.
In `@polylogue/importers/codex.py`:
- Around line 178-197: The code uses an empty string "" as a key for call_id
which causes id-less function calls and outputs to collide; update the logic in
the function_call handling and function_call_output handling (symbols: call_id,
call_index, chunks, ptype == "function_call_output") to avoid using "" as a key
— either use None or a unique sentinel and/or skip inserting a mapping when
call_id is missing, and when processing outputs skip matching if call_id is
falsy so you only look up call_index for real IDs; ensure you also don't set
chunks[-1]["parentId"]/["branchParent"] for missing call_id if that behavior is
undesired.
- Around line 81-104: The parser currently drops valid JSON scalars/arrays
because after json.loads(entry) non-dict values cause an immediate continue;
modify the handling around entry and payload (the block using entry, entry_type,
payload and chunks) to treat non-dict JSON as unparsed content: when payload (or
entry for non-response_item) is not a dict, format it with _format_json into a
chunk_text like the existing JSONDecodeError fallback, append a chunk dict to
chunks (using estimate_token_count with model="gpt-5-codex") and then continue;
this preserves scalar/array session content instead of silently discarding it.
In `@polylogue/models.py`:
- Around line 16-18: The code only validates that role is a string but not that
its value is canonical; update the logic handling the variable role (and
data["role"]) in polylogue/models.py to canonicalize values: normalize role =
role.lower() when present, map known synonyms like "assistant" or "bot" to
"model", and ensure only "model" or "user" are set (default to "model" for
anything else or non-string). This keeps downstream code in polylogue/render.py
(which counts "model" and "user") accurate.
- Line 8: The code currently does a full deepcopy of chunk into data which is
expensive; replace the deepcopy with a shallow copy (e.g., data = dict(chunk) or
data = chunk.copy()) and then explicitly normalize only the top-level keys and
rebuild the 'content' field as you already do so that nested structures that are
intentionally reconstructed are not duplicated unnecessarily; update any places
that assumed deep copies to ensure you create new objects for mutated nested
fields before modifying them.
- Line 4: Replace deprecated typing aliases with builtin generics: remove Dict
and List from the import line (keep Any), update the validate_chunks signature
from validate_chunks(raw_chunks: List[Any]) -> List[dict] to
validate_chunks(raw_chunks: list[Any]) -> list[dict], and change the local
variable annotation validated: List[dict] = [] to validated: list[dict] = [];
ensure all occurrences of List[...] and Dict[...] in this module (including the
validate_chunks function and its local vars) are migrated to list[...] and
dict[...] respectively.
In `@polylogue/ui.py`:
- Around line 97-101: Remove the redundant defensive check for Text in the UI
printing path: when Console and Panel are present (as checked by the
if-statement), Text will also be available, so in the branch that constructs
renderable use Text(text) directly instead of the ternary. Update the code that
builds renderable (the lines creating renderable and calling
self.console.print(Panel(renderable, title=title))) to call Text(text)
unconditionally, referencing the same Console, Panel, Text symbols and the
self.console.print / Panel(...) usage to locate the change.
- Around line 37-42: The __post_init__ currently forces self.plain = True when
Rich (Console) is missing, silently changing caller intent; instead preserve the
caller-provided self.plain value, always assign self.console = PlainConsole()
when Console is None, and if the caller requested interactive mode (self.plain
is False) emit a clear warning (e.g., warnings.warn or logger.warning)
explaining Rich is unavailable and interactive features will be disabled; update
__post_init__ (and any tests) to reference __post_init__, self.plain, Console,
and PlainConsole so downstream readers like DriveClient and the CLI will see the
unchanged plain flag and be notified about the degraded behavior.
In `@polylogue/util.py`:
- Around line 202-211: slugify_title currently can produce arbitrarily long
slugs which may exceed filesystem filename limits; update slugify_title to
enforce a maximum slug length (introduce a MAX_SLUG_LENGTH constant, e.g. 200 to
leave room for extensions) and truncate the final slug to that length after
collapsing repeated dashes, then strip any leading/trailing dashes; to avoid
collisions when truncating, optionally append a short deterministic hash (e.g.,
first 8 chars of sha1 of the original value) if the original slug was longer
than the limit (use functions slugify_title and any helper you add to compute
the hash).
In `@README.md`:
- Around line 19-20: Add a blank line before the markdown heading "## What You
Can Do" so there is an empty line separating it from the preceding
paragraph/content to satisfy markdownlint MD022; locate the heading string in
README.md and insert a single empty newline immediately above it.
- Around line 6-12: Add a blank line above the "## Quality Gates" heading to
satisfy MD022; locate the "## Quality Gates" heading in README.md and insert a
single empty line immediately before it (i.e., ensure there is a blank line
after the previous "## Quick Start" content), then run markdownlint or a quick
render to confirm the spacing is fixed.
In `@tests/test_branching.py`:
- Around line 39-49: The test test_build_branch_plan_handles_cycles_gracefully
is brittle because it asserts a specific message order from
build_branch_plan/_find_root_ids; change the assertion to be order-agnostic by
verifying the canonical messages as a set (e.g., assert set(canonical_messages)
== {"a","b","c"}) and/or assert the expected length (assert
len(canonical_messages) == 3) while keeping the branch count assertion (assert
len(plan.branches) == 1); alternatively, if the exact order must be enforced,
add a clear comment in the test explaining why "a","b","c" is the deterministic
fallback chosen by _find_root_ids.
In `@tests/test_importers_multi.py`:
- Line 471: The compound assertion "assert first and not first[0].skipped"
should be split into two assertions for clearer failure messages: first assert
that the collection variable (e.g., first) is truthy/non-empty, then assert that
the first element's skipped flag is False (e.g., assert not first[0].skipped).
Apply the same split for the other similar compound assertions in the file (the
other occurrences that check a collection and then access [0].skipped) so each
condition fails with a distinct message.
- Line 183: Split the compound assertion into two separate asserts: first assert
that the collection variable (e.g., first) is truthy/non-empty, then assert the
item-level property (e.g., not first[0].skipped); replace occurrences like
"assert first and not first[0].skipped" with "assert first" followed by "assert
not first[0].skipped" and apply the same pattern for the other similar
assertions on lines referencing the same variables.
- Around line 388-431: In test_chatgpt_unescapes_inline_footnotes update the
assistant message content in the mapping (the "parts" value for
"node-assistant") to use single escaping for the bracketed footnote instead of
double-escaping; replace the double-escaped sequence "\\\\[5\\\\]" with the
single-escaped sequence "\\[5\\]" so the test matches the documented ChatGPT
export format.
In `@tests/test_ui.py`:
- Around line 26-27: The test currently only asserts "Copy config" is in output;
strengthen it to also verify the bracketed text renders by asserting the
bracketed content appears in output (e.g., assert the specific string containing
'[' and ']' that the test provides or use a simple bracket-presence check on the
output variable), updating the assertions around the existing output =
capture.getvalue() and the current assert "Copy config" check.
---
Outside diff comments:
In `@polylogue/importers/chatgpt.py`:
- Around line 384-426: The attachment name from metadata (variable name) is
untrusted and may contain path traversal or absolute paths; sanitize it before
constructing target by deriving a safe filename (e.g., use
pathlib.PurePath(name).name or os.path.basename and strip/replace dangerous
characters) and then build target = attachments_dir / safe_name; after writing,
validate that target.resolve().is_relative_to(attachments_dir.resolve()) (or
catch ValueError from relative_to) to ensure it did not escape the
attachments_dir, and if validation fails, skip or log and treat as remote;
update all uses of name when creating AttachmentInfo and per_chunk_links to use
the sanitized safe_name or the validated relative path.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 56959871-54f4-4a71-91e0-7ce670fa9137
📒 Files selected for processing (29)
README.mddocs/providers/chatgpt.mddocs/providers/claude-ai.mddocs/providers/claude-code.mddocs/providers/openai-codex.mdpolylogue/branching.pypolylogue/cli.pypolylogue/cli_common.pypolylogue/commands.pypolylogue/conversation.pypolylogue/db.pypolylogue/document_store.pypolylogue/importers/base.pypolylogue/importers/chatgpt.pypolylogue/importers/claude_ai.pypolylogue/importers/claude_code.pypolylogue/importers/codex.pypolylogue/importers/utils.pypolylogue/local_sync.pypolylogue/models.pypolylogue/render.pypolylogue/ui.pypolylogue/util.pytests/test_branching.pytests/test_codex_importer.pytests/test_importers_multi.pytests/test_render_utils.pytests/test_stats.pytests/test_ui.py
Comment on lines
+96
to
+130
| roots = _find_root_ids(node_map) | ||
| if not roots: | ||
| return BranchPlan( | ||
| branches={ | ||
| "branch-000": BranchInfo( | ||
| branch_id="branch-000", | ||
| parent_branch_id=None, | ||
| message_ids=[], | ||
| is_canonical=True, | ||
| depth=0, | ||
| divergence_index=0, | ||
| ) | ||
| }, | ||
| canonical_branch_id="branch-000", | ||
| ) | ||
|
|
||
| all_paths: List[List[str]] = [] | ||
| for root_id in roots: | ||
| all_paths.extend(_build_paths(node_map, child_map, root_id)) | ||
|
|
||
| if all_paths: | ||
| # Deduplicate while preserving traversal order. | ||
| seen: set[tuple[str, ...]] = set() | ||
| unique_paths: List[List[str]] = [] | ||
| for path in all_paths: | ||
| key = tuple(path) | ||
| if key in seen: | ||
| continue | ||
| seen.add(key) | ||
| unique_paths.append(path) | ||
| all_paths = unique_paths | ||
|
|
||
| if not all_paths: | ||
| # No explicit leaves – treat roots as single-node branches. | ||
| all_paths = [[root_id] for root_id in roots] |
There was a problem hiding this comment.
Traverse disconnected rootless components too.
The fallback in _find_root_ids() only helps when there are no roots at all. If a malformed export contains a normal rooted chain plus a separate cycle, Lines 113-114 walk only the rooted component and silently drop the cyclic one from the branch plan.
🩹 Proposed fix
all_paths: List[List[str]] = []
for root_id in roots:
all_paths.extend(_build_paths(node_map, child_map, root_id))
+
+ covered_ids = {message_id for path in all_paths for message_id in path}
+ for message_id in node_map:
+ if message_id in covered_ids:
+ continue
+ extra_paths = _build_paths(node_map, child_map, message_id)
+ all_paths.extend(extra_paths)
+ covered_ids.update(node_id for path in extra_paths for node_id in path)🧰 Tools
🪛 Ruff (0.15.9)
[warning] 129-129: Comment contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@polylogue/branching.py` around lines 96 - 130, The current logic only builds
paths starting from roots (roots = _find_root_ids(...)) and thus drops
disconnected/rootless components (e.g., cycles) when roots exist; fix by after
building all_paths from roots compute covered_ids = set().union(*all_paths)
(handle empty), then determine remaining_ids = set(node_map.keys()) -
covered_ids and call _build_paths(node_map, child_map, node_id) for each node_id
in remaining_ids (or a single call per remaining node) and extend all_paths with
those results so cyclic or rootless components are traversed and included in the
branch plan; update variable names all_paths, roots, node_map, child_map, and
use _build_paths to locate the change.
Comment on lines
+132
to
+163
| def path_score(path: List[str]) -> Tuple[int, Optional[str]]: | ||
| last_ts = node_map[path[-1]].timestamp if path else None | ||
| return (len(path), last_ts) # length first, then timestamp ordering | ||
|
|
||
| canonical_path: List[str] | ||
| if canonical_leaf_id: | ||
| canonical_candidates = [p for p in all_paths if canonical_leaf_id in p] | ||
| canonical_path = canonical_candidates[0] if canonical_candidates else max(all_paths, key=path_score) | ||
| else: | ||
| canonical_path = max(all_paths, key=path_score) | ||
|
|
||
| branches: Dict[str, BranchInfo] = {} | ||
| canonical_branch_id = "branch-000" | ||
| branches[canonical_branch_id] = BranchInfo( | ||
| branch_id=canonical_branch_id, | ||
| parent_branch_id=None, | ||
| message_ids=canonical_path, | ||
| is_canonical=True, | ||
| depth=0, | ||
| divergence_index=0, | ||
| ) | ||
|
|
||
| assigned_paths = {tuple(canonical_path): canonical_branch_id} | ||
| counter = 1 | ||
|
|
||
| def assign_branch_id() -> str: | ||
| nonlocal counter | ||
| branch_id = f"branch-{counter:03d}" | ||
| counter += 1 | ||
| return branch_id | ||
|
|
||
| for path in sorted(all_paths, key=path_score): |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n polylogue/branching.py | head -170 | tail -50Repository: Sinity/polylogue
Length of output: 2148
🏁 Script executed:
find . -type f -name "*.py" | xargs grep -l "class MessageRecord" | head -5Repository: Sinity/polylogue
Length of output: 84
🏁 Script executed:
rg "class MessageRecord" -A 10Repository: Sinity/polylogue
Length of output: 600
🏁 Script executed:
python3 << 'EOF'
# Test Python 3 comparison behavior with None and strings
try:
result = (1, None) < (1, "2026-04-07T12:00:00Z")
print(f"Comparison succeeded: {result}")
except TypeError as e:
print(f"TypeError as expected: {e}")
# Test the proposed fix approach
tuple1 = (1, False, "") # None case
tuple2 = (1, True, "2026-04-07T12:00:00Z") # Timestamp case
try:
result = tuple1 < tuple2
print(f"Proposed fix comparison: {tuple1} < {tuple2} = {result}")
result2 = tuple1 > tuple2
print(f"Proposed fix reverse: {tuple1} > {tuple2} = {result2}")
except TypeError as e:
print(f"Fix still fails: {e}")
# Verify sorting works
tuples = [
(2, True, "2026-04-07T12:00:00Z"),
(1, False, ""),
(2, True, "2026-04-06T10:00:00Z"),
(1, True, "2026-04-08T15:00:00Z"),
]
sorted_tuples = sorted(tuples)
print(f"Sorting works: {sorted_tuples}")
EOFRepository: Sinity/polylogue
Length of output: 443
🏁 Script executed:
# Verify the actual usage context - check if any tests exist
find . -type f -name "*test*branching*" -o -name "*branching*test*" | head -5Repository: Sinity/polylogue
Length of output: 85
🏁 Script executed:
# Check imports and type hints in branching.py
head -30 polylogue/branching.pyRepository: Sinity/polylogue
Length of output: 744
Fix path_score() to handle optional timestamps in tuple comparisons.
MessageRecord.timestamp is Optional[str]. When comparing paths of equal length, Python 3 raises TypeError: '<' not supported between instances of 'NoneType' and 'str' at lines 139, 141, and 163 when max() or sorted() attempt to order tuples containing mixed None and string values. This causes branch planning to abort.
Change the return type to produce a totally ordered tuple:
Proposed fix
- def path_score(path: List[str]) -> Tuple[int, Optional[str]]:
- last_ts = node_map[path[-1]].timestamp if path else None
- return (len(path), last_ts) # length first, then timestamp ordering
+ def path_score(path: List[str]) -> Tuple[int, bool, str]:
+ last_ts = node_map[path[-1]].timestamp if path else None
+ return (len(path), last_ts is not None, last_ts or "")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@polylogue/branching.py` around lines 132 - 163, The path_score function
returns (len(path), last_ts) where last_ts can be None, causing TypeError when
tuples with None and str are compared by max()/sorted(); update path_score to
return a totally ordered tuple by normalizing the timestamp (e.g., replace None
with a consistent sentinel string or numeric value) so comparisons never see
None; ensure callers (max(all_paths, key=path_score) and sorted(all_paths,
key=path_score)) will now work without raising when MessageRecord.timestamp is
Optional[str].
Comment on lines
+177
to
+185
| branch_id = assign_branch_id() | ||
| branches[branch_id] = BranchInfo( | ||
| branch_id=branch_id, | ||
| parent_branch_id=parent_branch_id, | ||
| message_ids=path, | ||
| is_canonical=False, | ||
| depth=len(path), | ||
| divergence_index=divergence_index, | ||
| ) |
There was a problem hiding this comment.
Store branch-tree depth here, not message-path length.
Line 183 writes len(path) into depth, but this module already carries the full message_ids path and the DB layer persists message_count separately. A direct child branch should have depth 1, not the number of messages in the branch.
🩹 Proposed fix
branches[branch_id] = BranchInfo(
branch_id=branch_id,
parent_branch_id=parent_branch_id,
message_ids=path,
is_canonical=False,
- depth=len(path),
+ depth=branches[parent_branch_id].depth + 1,
divergence_index=divergence_index,
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@polylogue/branching.py` around lines 177 - 185, The code sets
BranchInfo.depth to len(path) (message-path length) but should store the
branch-tree depth (distance from root); update the branch creation in the block
that calls assign_branch_id() so depth = 0 for root (parent_branch_id is None)
and otherwise depth = branches[parent_branch_id].depth + 1 (or compute from
whatever parent BranchInfo store is used). Modify the BranchInfo construction
(the branches[...] = BranchInfo(...) call) to use that computed tree depth
instead of len(path), referencing assign_branch_id, branches, BranchInfo, and
parent_branch_id to locate and compute the correct value.
Comment on lines
+157
to
+172
| with open_connection() as conn: | ||
| upsert_conversation( | ||
| conn, | ||
| provider=provider, | ||
| conversation_id=conversation_id, | ||
| slug=slug, | ||
| title=title, | ||
| current_branch=plan.canonical_branch_id, | ||
| last_updated=modified_time, | ||
| content_hash=None, | ||
| token_count=total_tokens, | ||
| word_count=total_words, | ||
| attachment_bytes=sum(att.size_bytes or 0 for att in attachments), | ||
| dirty=False, | ||
| metadata=None, | ||
| ) |
There was a problem hiding this comment.
The SQLite conversation row is written before the real persist outcome exists.
upsert_conversation(...) is called with content_hash=None and dirty=False before persist_document(...) runs, so the DB never records the actual file hash and dirty-local skips are stored as clean.
Also applies to: 222-258
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@polylogue/conversation.py` around lines 157 - 172, The conversation row is
being written too early: upsert_conversation(...) is called with
content_hash=None and dirty=False before persist_document(...) runs, so the DB
never records the real file hash and local skips get stored as clean; move the
upsert_conversation(...) call to after persist_document(...) completes (use the
return/metadata from persist_document to obtain the actual content_hash and
whether the save was skipped/dirty) and pass that content_hash and the correct
dirty boolean into upsert_conversation; apply the same fix for the other
occurrence around lines 222-258 (ensure you use the result from persist_document
to set content_hash and dirty when calling upsert_conversation).
Comment on lines
+169
to
+170
| attachment_bytes=sum(att.size_bytes or 0 for att in attachments), | ||
| dirty=False, |
There was a problem hiding this comment.
Scope attachment totals to the canonical branch before rendering the top-level file.
attachments here is conversation-wide, so the canonical markdown/metadata and attachment_bytes can include files that only exist on non-canonical branches. That makes the root export inconsistent with the rendered transcript.
Also applies to: 224-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@polylogue/conversation.py` around lines 169 - 170, The attachment_bytes and
markdown/metadata generation use the conversation-wide attachments list, causing
non-canonical-branch files to be included; update the logic that builds the
top-level export (the code that computes attachment_bytes and renders the
top-level file) to first filter attachments by the conversation's canonical
branch (use conversation.canonical_branch) and then compute attachment_bytes =
sum(att.size_bytes or 0 for att in filtered_attachments) and pass
filtered_attachments into the renderer; apply the same filtering change to the
other block mentioned around the render/export code at the 224-239 region so
both places only include attachments from the canonical branch.
Comment on lines
+39
to
+49
| def test_build_branch_plan_handles_cycles_gracefully() -> None: | ||
| records = [ | ||
| _make_record("a", "c"), # Cycle a -> b -> c -> a | ||
| _make_record("b", "a"), | ||
| _make_record("c", "b"), | ||
| ] | ||
|
|
||
| plan = build_branch_plan(records) | ||
| canonical_messages = plan.messages_for_branch(plan.canonical_branch_id) | ||
| assert canonical_messages == ["a", "b", "c"] | ||
| assert len(plan.branches) == 1 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Cycle test assertion depends on dict insertion order.
The assertion canonical_messages == ["a", "b", "c"] relies on the specific order that _find_root_ids picks "a" as the fallback root (via next(iter(nodes))) and the subsequent path traversal order. While Python 3.7+ guarantees dict insertion order, this makes the test fragile if:
- The record insertion order changes
- The implementation's tie-breaking logic changes
Consider asserting the set of messages and the count instead, or add a comment explaining why this specific order is expected.
♻️ Alternative assertion that's order-agnostic
plan = build_branch_plan(records)
canonical_messages = plan.messages_for_branch(plan.canonical_branch_id)
- assert canonical_messages == ["a", "b", "c"]
+ # Cycle handling produces a deterministic path containing all nodes
+ assert set(canonical_messages) == {"a", "b", "c"}
+ assert len(canonical_messages) == 3
assert len(plan.branches) == 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_build_branch_plan_handles_cycles_gracefully() -> None: | |
| records = [ | |
| _make_record("a", "c"), # Cycle a -> b -> c -> a | |
| _make_record("b", "a"), | |
| _make_record("c", "b"), | |
| ] | |
| plan = build_branch_plan(records) | |
| canonical_messages = plan.messages_for_branch(plan.canonical_branch_id) | |
| assert canonical_messages == ["a", "b", "c"] | |
| assert len(plan.branches) == 1 | |
| def test_build_branch_plan_handles_cycles_gracefully() -> None: | |
| records = [ | |
| _make_record("a", "c"), # Cycle a -> b -> c -> a | |
| _make_record("b", "a"), | |
| _make_record("c", "b"), | |
| ] | |
| plan = build_branch_plan(records) | |
| canonical_messages = plan.messages_for_branch(plan.canonical_branch_id) | |
| # Cycle handling produces a deterministic path containing all nodes | |
| assert set(canonical_messages) == {"a", "b", "c"} | |
| assert len(canonical_messages) == 3 | |
| assert len(plan.branches) == 1 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_branching.py` around lines 39 - 49, The test
test_build_branch_plan_handles_cycles_gracefully is brittle because it asserts a
specific message order from build_branch_plan/_find_root_ids; change the
assertion to be order-agnostic by verifying the canonical messages as a set
(e.g., assert set(canonical_messages) == {"a","b","c"}) and/or assert the
expected length (assert len(canonical_messages) == 3) while keeping the branch
count assertion (assert len(plan.branches) == 1); alternatively, if the exact
order must be enforced, add a clear comment in the test explaining why
"a","b","c" is the deterministic fallback chosen by _find_root_ids.
| html=False, | ||
| html_theme="light", | ||
| ) | ||
| assert first and not first[0].skipped |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Split compound assertions for better debugging.
Per Ruff PT018, compound assertions like assert first and not first[0].skipped make it harder to diagnose failures. Split them for clearer error messages.
♻️ Proposed fix for lines 183, 195, 206
- assert first and not first[0].skipped
+ assert first
+ assert not first[0].skipped
md_path = first[0].markdown_path- assert second and second[0].skipped
+ assert second
+ assert second[0].skipped
assert "MANUAL EDIT" in md_path.read_text(encoding="utf-8")- assert third and not third[0].skipped
+ assert third
+ assert not third[0].skipped
assert "MANUAL EDIT" not in md_path.read_text(encoding="utf-8")Also applies to: 195-195, 206-206
🧰 Tools
🪛 Ruff (0.15.9)
[warning] 183-183: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_importers_multi.py` at line 183, Split the compound assertion into
two separate asserts: first assert that the collection variable (e.g., first) is
truthy/non-empty, then assert the item-level property (e.g., not
first[0].skipped); replace occurrences like "assert first and not
first[0].skipped" with "assert first" followed by "assert not first[0].skipped"
and apply the same pattern for the other similar assertions on lines referencing
the same variables.
Comment on lines
+388
to
+431
| def test_chatgpt_unescapes_inline_footnotes(tmp_path): | ||
| export_dir = tmp_path / "chatgpt_footnotes" | ||
| export_dir.mkdir() | ||
| conversations = [ | ||
| { | ||
| "id": "conv-footnote", | ||
| "title": "Footnote Chat", | ||
| "mapping": { | ||
| "node-user": { | ||
| "id": "node-user", | ||
| "message": { | ||
| "id": "msg-user", | ||
| "author": {"role": "user"}, | ||
| "content": {"content_type": "text", "parts": ["Say something"]}, | ||
| }, | ||
| }, | ||
| "node-assistant": { | ||
| "id": "node-assistant", | ||
| "message": { | ||
| "id": "msg-assistant", | ||
| "author": {"role": "assistant"}, | ||
| "content": { | ||
| "content_type": "text", | ||
| "parts": ["\\\\[5\\\\] Sample response with footnote"], | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
| ] | ||
| (export_dir / "conversations.json").write_text(json.dumps(conversations), encoding="utf-8") | ||
| out_dir = tmp_path / "footnote_out" | ||
| results = import_chatgpt_export( | ||
| export_dir, | ||
| output_dir=out_dir, | ||
| collapse_threshold=10, | ||
| html=False, | ||
| html_theme="light", | ||
| ) | ||
| assert results | ||
| content = results[0].markdown_path.read_text(encoding="utf-8") | ||
| assert "[5] Sample response with footnote" in content | ||
| assert "\\[5\\]" not in content | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for how footnote markers appear in ChatGPT importer code vs Claude importer
echo "=== ChatGPT importer footnote/escape handling ==="
rg -n -C3 'unescape|footnote|\\\\?\[' --type=py polylogue/importers/chatgpt.py 2>/dev/null || echo "File not found"
echo ""
echo "=== Claude importer footnote/escape handling ==="
rg -n -C3 'unescape|footnote|\\\\?\[' --type=py polylogue/importers/claude_ai.py 2>/dev/null || echo "File not found"Repository: Sinity/polylogue
Length of output: 2559
🏁 Script executed:
# Find and examine the normalise_inline_footnotes function
fd -n "utils.py" polylogue/importers/ -exec cat -n {} \;Repository: Sinity/polylogue
Length of output: 290
🏁 Script executed:
# Get the Claude test around line 136 to compare escaping patterns
sed -n '100,170p' tests/test_importers_multi.py | cat -nRepository: Sinity/polylogue
Length of output: 2787
🏁 Script executed:
# Also check the exact test data structure for both tests to understand escaping
sed -n '136,200p' tests/test_importers_multi.py | cat -nRepository: Sinity/polylogue
Length of output: 2479
🏁 Script executed:
# Find and examine the normalise_inline_footnotes function
find . -name "*.py" -path "*/importers/utils.py" -exec cat -n {} \;Repository: Sinity/polylogue
Length of output: 4427
🏁 Script executed:
# Search for normalise_inline_footnotes implementation
rg -n "def normalise_inline_footnotes" --type=pyRepository: Sinity/polylogue
Length of output: 141
🏁 Script executed:
# Search for any comments or documentation about ChatGPT export format
rg -n "ChatGPT.*export|chatgpt.*format|double.*escape" --type=py polylogue/Repository: Sinity/polylogue
Length of output: 727
🏁 Script executed:
# Check if there are any fixtures or real export examples
find . -name "*chatgpt*" -o -name "*export*" | grep -E "\.(json|txt|md)$" | head -20Repository: Sinity/polylogue
Length of output: 87
🏁 Script executed:
# Search for any issues or discussions about escaping in the codebase
rg -n "escape.*ChatGPT|ChatGPT.*escape" --type=pyRepository: Sinity/polylogue
Length of output: 42
🏁 Script executed:
cat ./docs/providers/chatgpt.mdRepository: Sinity/polylogue
Length of output: 3170
🏁 Script executed:
# Also search for any test comments or issues about the escaping
rg -n "\\\\\\\\\\[|double.*escap" tests/Repository: Sinity/polylogue
Length of output: 836
Correct ChatGPT test to use single escaping, matching documented export format.
The documentation states ChatGPT exports contain \[1] (single escaping), but the test at line 411 uses \\\\[5\\\\] (double escaping). The Claude test (line 136) and Codex test correctly use single escaping \\[3\\] and \\[9\\] respectively. Update the ChatGPT test data to match:
Fix escaping in ChatGPT test data
"parts": ["\\[5\\] Sample response with footnote"],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_importers_multi.py` around lines 388 - 431, In
test_chatgpt_unescapes_inline_footnotes update the assistant message content in
the mapping (the "parts" value for "node-assistant") to use single escaping for
the bracketed footnote instead of double-escaping; replace the double-escaped
sequence "\\\\[5\\\\]" with the single-escaped sequence "\\[5\\]" so the test
matches the documented ChatGPT export format.
| html=False, | ||
| html_theme="light", | ||
| ) | ||
| assert first and not first[0].skipped |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Split compound assertions for better debugging.
Same PT018 issue as the Claude tests—split compound assertions for clearer failure messages.
♻️ Proposed fix for lines 471, 483, 494
- assert first and not first[0].skipped
+ assert first
+ assert not first[0].skipped- assert second and second[0].skipped
+ assert second
+ assert second[0].skipped- assert third and not third[0].skipped
+ assert third
+ assert not third[0].skippedAlso applies to: 483-483, 494-494
🧰 Tools
🪛 Ruff (0.15.9)
[warning] 471-471: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_importers_multi.py` at line 471, The compound assertion "assert
first and not first[0].skipped" should be split into two assertions for clearer
failure messages: first assert that the collection variable (e.g., first) is
truthy/non-empty, then assert that the first element's skipped flag is False
(e.g., assert not first[0].skipped). Apply the same split for the other similar
compound assertions in the file (the other occurrences that check a collection
and then access [0].skipped) so each condition fails with a distinct message.
Comment on lines
+26
to
+27
| output = capture.getvalue() | ||
| assert "Copy config" in output |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Assertion doesn't fully verify the test objective.
The test name indicates it validates that bracket-containing text renders without markup errors, but the assertion only checks for "Copy config". To fully validate the test objective, also assert that the bracketed content appears in the output.
Strengthen assertion to match test intent
output = capture.getvalue()
assert "Copy config" in output
+ assert "/tmp/example.json" in output📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| output = capture.getvalue() | |
| assert "Copy config" in output | |
| output = capture.getvalue() | |
| assert "Copy config" in output | |
| assert "/tmp/example.json" in output |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_ui.py` around lines 26 - 27, The test currently only asserts "Copy
config" is in output; strengthen it to also verify the bracketed text renders by
asserting the bracketed content appears in output (e.g., assert the specific
string containing '[' and ']' that the test provides or use a simple
bracket-presence check on the output variable), updating the assertions around
the existing output = capture.getvalue() and the current assert "Copy config"
check.
Sinity
added
the
faux-pull-request
Sinity
added a commit
that referenced
this pull request
Apr 22, 2026
#14) Introduce a unified process_conversation() pipeline that all four importers now funnel through. Each import builds a message tree from parent-child IDs, computes a branch plan, persists metadata to SQLite, and writes a branch-aware filesystem layout alongside the canonical transcript. Core pipeline (polylogue/branching.py, conversation.py, db.py): - MessageRecord dataclass carries per-message identity, parentage, token/word counts, and the original chunk payload - build_branch_plan() walks the message DAG iteratively (no recursion) to identify branches; longest path wins canonical status - process_conversation() orchestrates: plan → SQLite upsert → canonical render → per-branch directory tree with overlay Markdown - New SQLite schema at XDG_STATE_HOME/polylogue/polylogue.db stores conversations, branches, and messages with FTS5 full-text index Importer refactoring (chatgpt, claude_ai, claude_code, codex): - All four importers build MessageRecord lists with parent_id linkage and delegate to process_conversation() instead of calling build_markdown_from_chunks + persist_document directly - Pydantic models (_ResponseItem, _Payload, ChunkModel, ContentPart) removed; replaced with plain-dict _sanitise_chunk() in models.py and direct JSON parsing in the Codex importer - --force flag wired through all import subcommands to overwrite locally-edited files - normalise_inline_footnotes() in importers/utils.py strips escaped \[N\] markers so citations render cleanly across all providers Rendering (polylogue/render.py): - Word counts (totalWordsApprox, inputWordsApprox, outputWordsApprox) tracked alongside token counts in stats and front matter - Per-turn timestamps show UTC with relative offset from conversation start, e.g. "2024-04-30T19:00:46Z (+22s)" - firstMessageTime, lastMessageTime, conversationDurationSeconds added to document metadata from chunk timestamp bounds - _prepare_chunks_for_render() drops empty chunks before stat computation so counts match visible output - sourceDriveId renamed to sourceId; empty attachment arrays omitted - ChatGPT citation tokens (U+E200..U+E201) resolved to Markdown footnotes referencing files, webpages, or live market quotes CLI formatting (polylogue/cli.py, cli_common.py): - Summary tables display "Tokens (~words)" columns and branch counts - Session picker gains column alignment and default skim key bindings (tab toggle, ctrl-a select-all, alt-a accept-all) - JSON sync payloads made null-safe for missing document metadata Opportunistic fixes: - polylogue.py entry-point shim deleted (CLI entry is the package) - ui.py: PlainConsole fallback when Rich is not installed; Panel rendering handles bracket-containing text without markup errors - util.py: slugify_title() for Unicode-safe kebab-case slug generation - document_store.py: _prune_empty_values() strips null/empty fields from persisted polylogue metadata Tests: - test_branching.py: 1500-node deep chains, cycle detection - test_render_utils.py: citation injection, time bounds metadata - test_importers_multi.py: footnote unescaping, --force overwrite, timestamp formatting, branch directory creation for each provider - test_codex_importer.py: footnote normalisation, word count presence - test_stats.py: word count propagation through stats pipeline - test_ui.py: Rich Panel with bracket text does not raise MarkupError
Owner
Author
Sinity
added a commit
that referenced
this pull request
May 29, 2026
Findings addressed: - #5: SIGKILL test uses min_count=N_SESSIONS*MESSAGES//2 not 1, so kill lands during active ingestion. - #6: VmRSS replaced with VmPeak (high-water mark) so the memory assertion measures peak rather than post-cleanup RSS. - #7: _wait_for_daemon_ready uses proc.poll() instead of os.kill(pid, 0), avoiding PID-reuse TOCTOU. SIGKILL via raw PID is kept where proc.poll() guards it. - #8: _has_systemd_scope() memoised to avoid repeated fork+exec at module-load / collection time. - #9: Dead assert snap1["session_profiles"] >= 0 replaced with explicit == 0 assertion documenting daemon-only materialization. - #10: _parse_code_records (private API) replaced with parse_payload("claude-code", ...) in test_provider_contracts and test_corpus_edge_cases. - #11: test_concurrent_access_safety docstring updated to document that --no-api mode tests WAL read-during-write, not daemon API concurrency. - #12: _write_corpus_files dead return value removed; callers iterate corpus_dir directly. - #13: topology-target.yaml regenerated via build-topology-projection. - #14: Stale verification_reports domain entry removed from test-coverage-domains.yaml (all referenced files deleted). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Sinity
added a commit
that referenced
this pull request
Jun 9, 2026
…ce_payload_json Problem (#14): session_profiles stores workflow_shape / terminal_state (and their confidences) in BOTH the authoritative native columns AND inside inference_payload_json. The native columns are the queryable ranking authority and both read paths already reconcile the inference payload onto them (insights.archive.SessionProfileInsight.from_record and the live _session_profile_components_from_archive_row), so the payload copy is a pure write/read drift surface with no read consumer. What changed: - The canonical session-profile writer now strips the four native-mirrored fields from the stored inference_payload_json (_stored_inference_payload_json in storage.py). The SessionInferencePayload model keeps the fields because they remain the read-API surface (resume ranking, MCP insight tools); the read paths repopulate them from the native columns, so reads are unchanged. - Tests: assert inference_payload_json no longer carries the mirrored fields while non-mirrored inference fields (e.g. inferred_topic_source) persist, and that SessionProfileInsight.from_record returns the native-column value when a rehydrated payload would otherwise report the model default. Scope note: evidence_payload_json still mirrors workflow_shape / terminal_state (workflow_shape_features / terminal_state_evidence). That surface is read un-reconciled (resume.py evidence fallback) and is out of #14's stated scope (inference_payload_json); collapsing it to single-source needs an evidence-path reconciliation or a read-API lift on the MCP/query surface. Verification: python -m pytest tests/unit/storage/test_session_insight_profiles.py \ tests/unit/storage/test_session_insight_mapper_fallbacks.py \ tests/unit/storage/test_session_insight_refresh.py tests/unit/insights \ tests/unit/api/test_facade_contracts.py tests/unit/mcp \ tests/unit/cli/test_plain_cli_snapshots.py -p no:randomly -> all pass (one pre-existing, environmental failure: codex-topic test asserts repo_names==('polylogue',) but the worktree dir is named differently) mypy polylogue/storage/insights/session/storage.py tests/...: Success Ref #14 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Every importer previously assembled chunks, called
build_markdown_from_chunks(), thenpersist_document()— four independent paths doing roughly the same thing with no awareness of conversation branching. This PR extracts that into a singleprocess_conversation()pipeline that all importers delegate to. The pipeline adds three capabilities that were previously impossible: branch-aware filesystem output, SQLite metadata persistence, and per-turn timestamp rendering.Motivation
ChatGPT exports expose a full message DAG via
mappingwithparentpointers — edits and regenerations create forks. Claude AI conversations can have branching too (alternative responses, parent UUIDs). Until now we flattened everything to a single linear transcript and lost the branch structure. I wanted to preserve forks without breaking the existing canonical<slug>.mdoutput.The second driver was that we had no queryable metadata store. Each import wrote Markdown and optionally updated the FTS index, but there was no way to ask "which branches exist for this conversation?" or "what's the total word count across all Claude imports?" without reparsing front matter. SQLite solves that.
Word counts alongside token counts were a recurring request when spot-checking imports — tokens are model-dependent approximations, but word counts are deterministic and easier to sanity-check.
Architecture: the conversation pipeline
The new flow for every importer is:
build_branch_plan()is iterative (stack-based DFS) rather than recursive, which matters because ChatGPT exports can have thousands of nodes. The cycle detection handles malformed exports where parent pointers loop. I tested with a 1500-node synthetic chain.Key implementation decisions
Pydantic removal: The Codex importer used
_ResponseItemand_PayloadPydantic models for JSONL parsing, andmodels.pyhadChunkModel/ContentPart. These were replaced with plain dict handling. The Pydantic validation was rejecting valid Codex entries that had unexpected fields (theextra="allow"config helped but added complexity). The new_sanitise_chunk()function normalises aliases and coerces types without the import overhead.Citation injection for ChatGPT: ChatGPT exports embed citation markers using private-use Unicode characters (U+E200 through U+E202).
_inject_citations()resolves these to Markdown footnotes by matching against thecitationsandcontent_referencesarrays in message metadata. Finance tool results from hidden refs get a descriptive "Live market quote (tool result)" label.Footnote normalisation: Many providers double-escape bracket references (
\[1\]instead of[1]). Thenormalise_inline_footnotes()regex is conservative — it only unescapes short labels that look like citation markers (digits, short alphanumeric), leaving LaTeX-style math escapes untouched.--forceflag: Added to all four import subcommands. Without it, re-importing a conversation whose local Markdown has been hand-edited will skip and reportdirty-local. With--force, the file is overwritten. Tests verify the three-step cycle: import → edit → skip → force-reimport.Changes by concern
Branch planning (
branching.py)New module.
MessageRecordcarries per-message identity and stats.BranchInfodescribes one path through the DAG.build_branch_plan()returns aBranchPlanwith named branches (branch-000canonical,branch-001+).SQLite persistence (
db.py)Schema v1 with three tables:
conversations(provider + conversation_id PK),branches(FK to conversations),messages(FK to branches, position-ordered). FTS5 virtual table on message bodies.open_connection()is a context manager that auto-creates the DB atXDG_STATE_HOME/polylogue/polylogue.db.Conversation orchestrator (
conversation.py)process_conversation()is the funnel. Computes branch stats, upserts SQLite, renders the canonical document, then writes the branch directory tree. The common-prefix document (conversation.common.md) contains only messages shared by all branches.Rendering improvements (
render.py)Before:
After:
_format_timestamp()computes a relative delta from the first message. Empty chunks are filtered by_prepare_chunks_for_render()so chunk counts in the front matter match what's visible in the output.CLI formatting
Summary tables now show
Tokens (~words)column headers and display word counts inline:Testing
_inject_citations()tested with mixed webpage/file/finance references\[N\]unescaping in ChatGPT, Claude AI, Claude Code, CodexRef #13
Summary by CodeRabbit
Release Notes
New Features
--forceflag for import commands to overwrite locally-edited transcriptsDocumentation
Improvements