Codex provider rendering across all CLI modalities#296
Merged
Conversation
First slice of the modality work: the cheap sniff that lets an INPUT_PATH route to the Codex pipeline instead of falling through to the Claude parser (which skips every rollout record and emits a near-empty page — the worst current gap). - BaseProvider.detect_path(path) -> bool: new concrete default (False). Providers that recognize their own files by a cheap check override it; the base opts out so only such providers participate in INPUT_PATH auto-detection. - CodexProvider.detect_path: a rollout file, or a directory containing one. Backed by _looks_like_rollout_file (filename `rollout-*.jsonl`, else a single first-line `session_meta` sniff incl. the legacy no-`type`/`id` header; never parses the body) and _contained_rollouts (recursive discovery with the same resolve()+is_relative_to containment rule as the loader, so an INPUT_PATH dir can't pull in files via an escaping symlink). Nine detection tests on synthetic fixtures (filename, session_meta, legacy header, Claude-transcript negative, garbage/empty, dir-with/without-rollout, symlink containment, base-default opt-out). Dispatch wiring + the silent-empty regression pin land next in M1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…etect The provider/registry primitives the CLI dispatch will call to route a rollout INPUT_PATH through the Codex pipeline: - CodexProvider.load_session_from_path(path): load a single rollout file directly by path, independent of the configured data dir. No sibling index, so inherited-prefix stripping is a no-op and the file renders standalone — the right behavior for "render that one rollout". Missing file → clear FileNotFoundError. - ProviderRegistry.detect_provider_for_path(path): return the single provider whose cheap detect_path sniff claims the path, else None (falls through to the Claude default). Independent of is_available (an INPUT_PATH may be handed in with no provider data dir). More than one claimant → ValueError telling the user to disambiguate with --provider (DECIDED #2). Five tests: registry routes a rollout to codex / None for a Claude transcript / raises on an ambiguous double-claim; load_session_from_path yields real user+assistant entries from the fixture (not an empty page) and raises on a missing file. Next: the CLI dispatch branch + the silent-empty regression pin. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Wire the detection primitives into the CLI so a bare rollout handed as an
INPUT_PATH renders via the provider pipeline instead of falling through to the
Claude parser (which skips every record and emits a near-empty page — the worst
gap this work closes).
- cli.py: at the top of the single-file fall-through (before convert_jsonl_to),
when no --provider is set and INPUT_PATH is a file, auto-detect the provider
(registry.detect_provider_for_path). A match loads via load_session_from_path
and renders via render_normalized_session_file, honoring --format / -o (file/
dir/stdout) / --open-browser. Zero interaction with the --provider fence
(which only fires when provider is not None), so the rejection matrix and the
Claude default path are untouched.
- SILENT-EMPTY PIN: a detected rollout that yields no messages raises a loud
click error, never an empty page.
- base.py: load_session_from_path added to the provider contract (default
raises) so the CLI can call it on a BaseProvider ref.
Critical fix found by asserting CONTENT, not bytes: generate_session filters
entries by sessionId, so the render key must be the session's own thread id
(carried on the entries), not the filename stem — a mismatch silently dropped
every message into a full-size-but-empty page, i.e. the very bug being fixed.
The regression pin asserts rendered content ("synthetic files") across
html/md/json, plus a loud-error test and a non-rollout-untouched test.
Full just ci green (2561 unit; snapshot suite green = Claude path byte-stable).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Relax the provider-mode fence so `--provider codex <rollout-file>` renders that
file (it was categorically rejected). Provider mode now renders one session by
either --session-id (export) or an INPUT_PATH (a rollout file); wholesale/cache/
Claude-only flags are still rejected loudly (M2-M4).
- cli.py fence: --provider + INPUT_PATH + --session-id → ambiguity error ("drop
--session-id"); --provider with neither → "requires --session-id or an
INPUT_PATH"; INPUT_PATH alone is no longer a conflict. The rejection-matrix
rows still assert their key substrings, so they stay meaningful (the
INPUT_PATH row now pins the both-set ambiguity).
- Branch A: an explicit --provider + INPUT_PATH renders via the shared helper
and returns, before the --session-id export path.
- Factor the render (load_session_from_path → silent-empty guard →
render_normalized_session_file → file/dir/stdout output) into a module-level
_render_provider_input_file, called by both the explicit path and the
auto-detect path (no duplication; the sessionId render-key fix lives in one
place).
New positive test for the explicit render (asserts content). Full just ci green
(2562 unit; snapshot suite green = Claude path byte-stable). Remaining M1: the
INPUT_PATH-directory mini-root case.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…PATH
Close the hole main flagged: a directory of rollouts handed as an INPUT_PATH
would fall through to the Claude parser (its *.jsonl glob matches rollout-*.jsonl,
so should_convert stays false) and render a near-empty page — the silent-empty
gap for the directory case. Until the wholesale walker lands directory
rendering (merged M1-dir + M2), a claimed directory must error loudly, not lie.
- Auto-detect guard relaxed from is_file() to exists(), so a rollout DIRECTORY
is detected too (detect_path already handles dirs); a non-rollout dir (a
Claude project dir with no rollout-*.jsonl) still returns None and reaches the
Claude path unchanged.
- _render_provider_input_file errors loudly on a directory ("... is a codex
sessions directory; directory rendering lands with the wholesale walker. For
now point at a single rollout file, or use --provider codex --session-id
<id>."), covering both the auto-detect and explicit --provider paths.
Test: a rollout directory errors loudly (auto + explicit); the file path and a
non-rollout Claude directory are unaffected. Full just ci green (2563 unit;
snapshot suite green = Claude byte-stable).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add provider-neutral seams for rendering a whole sessions tree, not just a single session: - BaseProvider.discover_sessions_under(root) / load_session_under(root, id): discover and load every session within an explicit root — the provider's own data dir, or a directory handed in as an INPUT_PATH. Unlike the standalone load_session_from_path, sibling context within the root (e.g. fork-prefix stripping) is honored. Default raises so a provider that does not support wholesale rendering opts out loudly. - CodexProvider: refactor discovery/loading to be parameterized by an explicit sessions root. discover_sessions / load_session now derive <data_dir>/sessions and delegate to the same root-scoped internals the new methods use, so one code path serves both. The thread-id index is memoized per resolved root, so a wholesale run reads each rollout header once instead of once per session (O(n) rather than O(n^2)). Tests pin discovery grouping by cwd, deterministic sorted order, round-trip loading, the sibling-context difference vs. standalone path loading, index memoization, and the loud base-class default for non-participating providers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add render_provider_wholesale: render every session of one provider under a root into a project hierarchy, reusing the shared renderers rather than forking the Claude project pipeline. - Discovery/loading via the provider-neutral root-scoped seams; sessions are grouped into "projects" by their cwd, each getting per-session pages, a single combined page (renderer.generate), and a card in the master index (generate_projects_index). A no-cwd bucket keeps orphan sessions addressable. - Output lands under an explicit output root (the caller resolves the default <provider_home>/claude-code-log/ and any -o override); the sessions tree is never written into. - render_normalized_session_file gains suppress_combined_link so per-session pages under --combined no don't emit a back-link to an absent combined page. - The index-summary dict is shape-identical to the Claude path (including the zero token totals), so the drift pin below fails RED on a key rename either side instead of silently rendering an empty provider index. Cache participation and paginated combined pages are intentionally deferred: the combined page is a single unpaginated document (cache_manager=None), as for the single-session provider export. Tests cover the e2e hierarchy across all three formats, cwd grouping, the --combined no / --no-individual variants, determinism, date filtering, and the required claude-vs-walker index-summary shape pin (project- and session-dict key sets asserted equal). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Dispatch a whole provider sessions tree through the walker, and make the provider fence sub-mode aware: - New provider sub-mode "wholesale": no --session-id and either no INPUT_PATH (walk the provider's data dir) or an INPUT_PATH directory / --projects-dir (a mini sessions root). It renders the project hierarchy via the walker. Auto-detection routes a bare rollout DIRECTORY (no --provider) the same way, so a directory of rollouts can never fall through to the empty Claude parse. - Output root: an explicit -o directory wins (a file-shaped -o is rejected — wholesale writes many files); otherwise the default is <provider_home>/claude-code-log/, so the sessions tree stays pristine. - The fence now rejects only the flags that don't apply to the active sub-mode: Claude-only projection flags (--expand-paths, --filter-path) and --tui are always illegal; cache flags stay illegal pending cache support; the wholesale-only flags (--all-projects, --projects-dir, date range, --combined, --page-size, --no-individual-sessions) are legal in wholesale and rejected only in single-session mode. Tests: bare --provider wholesale, --projects-dir root override, the default output-root placement, file-output rejection, the still-illegal flag combos, and the auto-detected rollout directory now rendering (replacing the interim loud dir-error). The rejection matrix flips its bare-provider row. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The wholesale walker emits zero token totals per project (Codex rollouts carry no token accounting the provider surfaces yet), so the index token summary is blank for Codex projects. Record the gap so it is tracked rather than remembered. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Give the provider wholesale walker render-skip caching under its output root, reusing CacheManager with an explicit db_path + output_dir so none of its Claude-bound assumptions (source-dir DB location, source-in-place staleness) apply and the pristine sessions tree is never touched. - The cache DB lives at <output_root>/claude-code-log-cache.db (honoring CLAUDE_CODE_LOG_CACHE_PATH). Each cwd-project uses a CacheManager whose project_path is the synthetic OUTPUT project dir, so output_dir == project_dir and the source-dir-default staleness trap can't fire. - Per project: load sessions fresh, populate the session/project/file cache, then render-skip. A session or combined page is re-rendered on any staleness signal (source rollout changed by mtime, or output missing / version-stale / message-count drifted) and skipped otherwise. The index is always regenerated. - Warm/cold byte-stability is guaranteed by deterministic rendering — a skip leaves the identical file — and pinned by a test. - SessionInfo gains source_path so the walker can key source-mtime staleness off the session's rollout file. - v1 caches for render-skip only: save_cached_entries populates the messages table for schema uniformity but the walker never loads from it (it re-parses rollouts each run — cheap, and it avoids a serialization round-trip fidelity risk). Documented at the call site and in work/codex-backlog.md. CLI: --no-cache / --clear-cache / --clear-output flip from loud-rejected to honored in wholesale (still rejected in single-session mode). --clear-cache and --clear-output are scoped to the output root only, so a test pins that the sessions tree is byte-identical after --clear-output (DECIDED #4). Paginated combined pages and cache-backed load remain deferred (backlog); --page-size / --jobs stay loud-rejected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A provider that does not implement the wholesale seams (agy) must fail loudly rather than crash or render nothing. Lock the CLI-level surfacing of the base default's NotImplementedError with a test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review findings on untested edge combinations that produced a silent divergence: - --clear-output / --clear-cache with a date filter now clears then REGENERATES the filtered view (mirroring the Claude path), instead of clearing and returning unconditionally — which left an empty directory for `--clear-output --from-date`. The comment claiming to mirror the Claude path is now true. - Directory detection and discovery are sniff-aware, symmetric with the single-file path: a rollout recognised only by its first-line session_meta header (a non-rollout-*.jsonl name) inside a directory is now discovered and rendered, rather than falling through to an empty Claude parse. Real data dirs hold only rollout-*.jsonl, which still short-circuits on the name. - The wholesale walker fails loudly when no sessions are discovered under an explicitly-targeted root, instead of writing an empty index and exiting 0 (a date filter that legitimately matches nothing still renders an empty index — that is a legible filter result, not this). - An unknown --provider is validated up front into a clean UsageError (exit 2) rather than a broad-except "Error converting file" (exit 1). Each fix is pinned by a regression test (clear-output-with-date regenerates, sniff-only directory renders, empty directory errors loudly, unknown provider is a clean usage error). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…mbos Land dedicated regression tests for two edge combos that were previously only verified by execution, so they don't stay execution-only: - --provider codex --all-projects walks the data dir into the wholesale hierarchy, identically to bare --provider codex (accepted synonym). - A nonexistent INPUT_PATH + --provider routes to single-file mode and errors loudly with rollout-not-found, never a silent empty render. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe CLI detects Codex rollouts, renders individual provider sessions, and walks provider session trees into project-grouped outputs. Provider APIs support rooted discovery and direct loading, with cache-aware rendering, cleanup, filtering, and validation. ChangesProvider rendering flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant ProviderRegistry
participant CodexProvider
participant Renderer
User->>CLI: provide rollout path and rendering options
CLI->>ProviderRegistry: detect_provider_for_path(input_path)
ProviderRegistry->>CodexProvider: detect_path(input_path)
CodexProvider-->>ProviderRegistry: Codex match
CLI->>CodexProvider: discover_sessions_under or load_session_from_path
CodexProvider-->>Renderer: session metadata and transcript entries
Renderer-->>User: session pages, combined pages, and index
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
test_cli_wholesale_for_provider_without_support_errors_loudly relied on an agy data directory existing in the environment: with one present (a dev box) the walker reaches discover_sessions_under and raises "does not support wholesale rendering"; without one (CI) it short-circuits earlier on "No agy data directory found". Both are loud errors, but the test pinned the former string. Pass --projects-dir so the walker gets an explicit sessions root and reaches the NotImplementedError deterministically, regardless of ambient install. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
test_discover_under_groups_by_cwd compared str(info.project_path) to POSIX
literals, but str(WindowsPath("/proj/a")) is "\\proj\\a", so the grouping keys
diverge on Windows (the test only reaches the windows CI jobs once the earlier
matrix stops fail-fast-cancelling). Use Path.as_posix() for the key — the
recorded cwds are POSIX and the per-cwd counts are what the test pins.
Co-Authored-By: Claude Opus 4.8 (1M context) <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
1 participant
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.
Provider rendering across all CLI modalities
Extends provider-based rendering (notably
codex) from the narrow--provider codex --session-id <id>single-session export to the whole CLIsurface — wholesale runs,
--projects-dir, an INPUT_PATH that is a rolloutfile or a directory,
--combined/--page-size/date filters, and the SQLitecache. The Claude default path (no
--provider) is byte-stable throughout.The design goal is a no-gaps modality matrix: every modality × flag
combination either works with the provider or fails loudly — nothing
silently bypasses the provider or renders a Codex rollout as an empty Claude
transcript (previously the worst gap: a rollout handed to the CLI fell through
to the Claude parser, which skipped every record and emitted a near-empty page).
Target matrix
--provider--provider codex(no INPUT_PATH) /--all-projects--provider codex --projects-dir DIRDIRoverrides the sessions rootrollout-*.jsonl(or asession_meta-sniffed file)--provideris omitted--session-id(+--provider)--combined yes/no/only--format html/md/json,-ofile/dir-ofor a multi-file wholesale run errors loudly--from-date/--to-date--no-cache/--clear-cache/--clear-output--expand-paths/--filter-path/--tuiKey decisions
--provider NAMEselects the single provider.rollout-*.jsonl, else a first-linesession_metasniff); ambiguity errors and asks for--provider.<codex_home>/claude-code-log/(created on demand,-ooverrides); the cache DB lives there and the date-sharded sessions tree is never written into.Cache
Reuses
CacheManagerwith an explicitdb_path+output_dirand a syntheticper-cwd output project dir as the project identity, so none of its
write-next-to-source assumptions apply. v1 caches for render-skip only
(unchanged sessions/combined pages are skipped by source mtime + output
staleness); warm/cold runs are byte-stable by deterministic rendering. A
--clear-outputrun is pinned to leave the sessions tree byte-identical.Deferred (loud-rejected, tracked in
work/codex-backlog.md)--page-size/--jobs(ride on cache-coupled machinery).archived_sessions.Testing
Detection primitives, root-scoped discovery/loading, the wholesale walker e2e
across all three formats (grouping,
--combinedvariants, determinism, datefiltering), the required Claude-vs-walker index-contract drift pin, cache
warm/cold byte-stability + render-skip + change-detection, the CLI flag flips
and the loud-error rejection matrix, and the DECIDED sessions-tree-untouched
invariant. Full
just cigreen; the Claude snapshot suite is unchanged.🤖 Generated with Claude Code
Summary by CodeRabbit