Regenerate single-file output when its source grows; fix success-message accuracy (#221 follow-up)#253
Merged
Merged
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesRegeneration tracking and messaging
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
claude_code_log/cli.py (1)
1343-1361: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle
--combined noas a session-only success path.When
write_combinedis false,convert_jsonl_to()deliberately returnsoutput_pathwithout writing that combined file. This branch is now reachable becausewas_regeneratedbecomes true from session-file writes, so the CLI reportsSuccessfully combined ... to {output_path}andclick.launch()can target a path that does not exist.Suggested fix
- if not was_regenerated: - pass - elif input_path.is_file(): + if not was_regenerated: + pass + elif input_path.is_file(): click.echo(f"Successfully converted {input_path} to {output_path}") else: jsonl_count = len(list(input_path.glob("*.jsonl"))) - if write_individual: + if write_combined and write_individual: ext = get_file_extension(output_format) session_files = list(input_path.glob(f"session-*.{ext}")) click.echo( f"Successfully combined {jsonl_count} transcript files from {input_path} to {output_path} and generated {len(session_files)} individual session files" ) - else: + elif write_combined: click.echo( f"Successfully combined {jsonl_count} transcript files from {input_path} to {output_path}" ) + else: + ext = get_file_extension(output_format) + session_files = list(input_path.glob(f"session-*.{ext}")) + click.echo( + f"Successfully generated {len(session_files)} individual session files from {jsonl_count} transcript files in {input_path}" + ) - if open_browser: + if open_browser and write_combined: click.launch(str(output_path))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@claude_code_log/cli.py` around lines 1343 - 1361, The success handling in cli.py’s convert_jsonl_to flow is treating session-only regeneration as a combined-output success, which can report a nonexistent output_path and launch it in the browser. Update the branch around was_regenerated, input_path.is_file(), and write_individual so that when write_combined is false the CLI uses a session-only success message and only calls click.launch on a real existing file path, while preserving the combined-file messaging when output_path was actually written.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/test_html_regeneration.py`:
- Around line 331-352: The regeneration assertion in this test is too dependent
on `time.sleep(0.1)` and can flake on coarse mtime filesystems; update the
`convert_jsonl_to_html` setup so the source file is explicitly made newer than
the HTML output using `os.utime(...)`, matching the approach already used in
`test_output_explicit.py`. Keep the checks around `skip_calls`,
`output_file.stat().st_mtime`, and the `"Single file mode test."` content, but
ensure the test advances the source timestamp in a filesystem-safe way before
calling `convert_jsonl_to_html`.
---
Outside diff comments:
In `@claude_code_log/cli.py`:
- Around line 1343-1361: The success handling in cli.py’s convert_jsonl_to flow
is treating session-only regeneration as a combined-output success, which can
report a nonexistent output_path and launch it in the browser. Update the branch
around was_regenerated, input_path.is_file(), and write_individual so that when
write_combined is false the CLI uses a session-only success message and only
calls click.launch on a real existing file path, while preserving the
combined-file messaging when output_path was actually written.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f13ccd68-ef0e-4edc-8d40-7b4af1b1b440
📒 Files selected for processing (4)
claude_code_log/cli.pyclaude_code_log/converter.pytest/test_html_regeneration.pytest/test_output_explicit.py
…essage (#221 follow-up) The default single-file output path (no `-o`, detail-suffixed naming like `<file>.low.html`) rode the version-only staleness skip: `is_outdated()` compares only the embedded tool version, not the source's freshness. So a single `.jsonl` that grew between runs (an in-progress session re-exported with the same tool version) was wrongly reported "current" and served stale HTML. #237 only forced regeneration for the explicit `-o file` case. Bug 1: add a source-mtime term to the single-file `else` branch in convert_jsonl_to — regenerate when the source file is newer than the existing output. Scoped to a file source (a directory source has no single mtime and is already handled by the cache / `cache_was_updated` path). The version-marker check is kept for tool-version bumps. Bug 2: on a skip the CLI printed "Successfully converted" on top of the converter's own "is current, skipping regeneration" line. convert_jsonl_to now reports whether it actually (re)wrote the output via a non-breaking optional `regenerated` out-parameter (the `Path` return contract that ~20 callers rely on is unchanged); the CLI prints the success line only on a real regeneration. Tests: flip test_single_file_mode_regeneration_behavior (it enshrined the stale behavior as "expected") to assert regeneration on a grown source; add TestSingleFileStaleness covering the exact CLI repro — grown source regenerates, unchanged source skips with no redundant success line. Both fail on the prior code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ty, mtime rationale Non-blocking review follow-ups on the #221 single-file-staleness fix: 1. Directory-branch symmetry: the skip-suppression of the success line was gated only on the single-file branch, so a directory whose combined output is current (Phase-1b early-exit) still printed "Successfully combined N transcript files ... and generated M individual session files" on a pure skip. The `regenerated` out-param already carries the bool — extend the same guard to the directory branch so skip-messaging is consistent for single-file AND directory inputs. 2. Readability: `regenerated != [False]` -> `any(regenerated)`. 3. Mtime rationale: add a comment explaining why the *output* file's mtime is the correct, sufficient, persistence-free basis in single-file mode (no cache DB to store the source mtime as the directory path does), and that both share the same filesystem-granularity limit. Test: add test_directory_skip_suppresses_success_line pinning note 1 (RED on the prior file-only guard, which printed the misleading combined line on a no-op directory re-run). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…bined no silent run) The skip-suppression added earlier gated the CLI success line on the COMBINED-output write only (`did_regenerate`). But per-session generation is an independent axis: `--combined no` writes only individual session files and never a combined transcript, so `did_regenerate` stayed False and the run printed NOTHING despite producing every session file — a regression vs. the prior unconditional "Successfully combined N transcript files ... and generated M individual session files" line. The same gap silently dropped the session count whenever the combined was current but a session was (re)written. `_generate_individual_session_files` already returns a regenerated count; it was discarded. Capture it and OR it into the `regenerated` out-param so the gate means "combined OR any session file (re)written". Pure skips (single-file or a fully-current directory) still report False and stay quiet, preserving the original double-message fix. Surfaced by a self-review pass (/code-review) on the branch; not caught by the diff-scoped manual review because it only manifests under --combined no / session-only regeneration, which the review didn't execute. Test: test_combined_no_confirms_session_generation — `--combined no` writes session files AND confirms the work (RED before: the run printed only Processing/Loading with no summary line). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e test
Two non-blocking follow-ups from PR review:
- CodeRabbit: test_single_file_mode_regeneration_behavior sleeps 0.1s then
relies on source.mtime > output.mtime to force regeneration — flaky on
filesystems with coarse mtime granularity. Force strict ordering with
os.utime (matching TestSingleFileStaleness) instead of racing the clock.
- monk: under `--combined no` (write_combined=False) the success line still
read "Successfully combined N transcript files ... and generated M individual
session files" — "combined" is inaccurate when no combined file is written.
Make the line write_combined-aware: keep "Successfully combined ... to
<file>" for the combined case, and report "Successfully processed N transcript
files from <dir> and generated M individual session files" for --combined no
("processed" matches the verb already used for the --all-projects index).
The per-session suffix is shared, so --combined only stays "Successfully
combined ... to <file>" unchanged. Test updated to pin the new wording.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cboos
force-pushed
the
dev/single-file-staleness
branch
from
4d7f498 to
2fd72ef
Compare
Collaborator
Author
|
(Claude) The branch was rebased onto current
@coderabbitai full review |
|
(=^･ω･^=) ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
claude_code_log/converter.py (2)
1914-1950: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
did_regenerate = Trueis set unconditionally for the paginated branch, even if no page was actually rewritten.
_generate_paginated_htmlskips each page individually whennot is_stale and page_file.exists()(its owncontinue), so it can return without writing anything. This branch is reached whenever the early-exit at Lines 1821-1844 didn't trigger — e.g.stale_sessionsis non-empty (a session needs regen) while page-level staleness (tracked separately viacache_manager.is_page_stale) says no page needs regen. In that case,did_regenerateis set toTrueregardless, so the CLI'swas_regeneratedgate reports a combined rewrite that never happened, undermining the exact "no misleading success message" goal of this PR for the paginated (large-project) path.🐛 Suggested approach
Have
_generate_paginated_htmlreport whether it actually wrote any page (e.g. return a tuple or track via a mutable counter), then setdid_regeneratefrom that instead of unconditionally:- output_path = _generate_paginated_html( + output_path, did_regenerate = _generate_paginated_html( messages, effective_output_dir, title, page_size, cache_manager, session_data, working_directories, silent=silent, session_tree=session_tree, detail=detail, compact=compact, no_recaps=no_recaps, ) - did_regenerate = TrueAnd inside
_generate_paginated_html, track a boolean that flipsTrueonly when a page loop iteration doesn't hit the earlycontinue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@claude_code_log/converter.py` around lines 1914 - 1950, The paginated HTML branch in converter.py sets did_regenerate to True even when _generate_paginated_html skips all pages and writes nothing, which makes the regeneration status inaccurate. Update _generate_paginated_html to report whether it actually wrote any page (for example by returning a flag or count), then use that result in the use_pagination branch instead of unconditionally setting did_regenerate. Keep the fix localized around _generate_paginated_html and the did_regenerate assignment so the CLI only reports regeneration when a page was really rewritten.
2028-2057: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSingle boolean
regeneratedis too coarse to let the CLI distinguish "combined rewritten" from "only sessions rewritten".
regenerated.append(did_regenerate or sessions_regenerated > 0)collapses two independent signals into one. As shown in the pairedcli.pycomment, this causes the CLI to print "Successfully combined ... to {output_path}" even when only session files were rewritten and the combined output was actually skipped (already printed "is current, skipping regeneration" by this same function). Consider exposing the two signals separately (e.g. append a(did_regenerate, sessions_regenerated > 0)tuple, or two separate out-params) so the CLI can gate the "combined" wording specifically ondid_regenerate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@claude_code_log/converter.py` around lines 2028 - 2057, The current `regenerated` out-parameter in `converter.py` is too coarse because `regenerated.append(did_regenerate or sessions_regenerated > 0)` merges combined-output regeneration with session-file regeneration. Update the `regenerated` signal in the conversion flow around `_generate_individual_session_files` so the caller can distinguish `did_regenerate` from `sessions_regenerated > 0` separately, such as by returning two flags or a tuple, and then adjust the CLI-side handling to use the combined-specific flag for the “Successfully combined” message.
🧹 Nitpick comments (1)
test/test_output_explicit.py (1)
273-296: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGood regression coverage for
--combined no; consider adding a case for the mixed combined-skip/session-regen scenario.This test correctly pins the
--combined nofix. It doesn't cover the adjacent bug flagged incli.py/converter.py—write_combined=Truewith the combined output current but a session file regenerated (e.g. a manually-deleted session file), which currently still prints "Successfully combined ... to {output_path}" despite the combined file being skipped. Once that's fixed, a test asserting "Successfully combined" is not shown (and something more accurate is) in that scenario would guard the regression.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_output_explicit.py` around lines 273 - 296, Add a regression test for the mixed case in `main`/`converter` where `write_combined=True`, the combined output is already current, but a per-session file is regenerated (for example after deleting one session HTML file). Assert that the CLI does not print “Successfully combined” when the combined file was skipped, and instead emits a more accurate confirmation tied to the session regeneration path; use the existing `test_combined_no_confirms_session_generation` pattern as the reference for locating the output assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@claude_code_log/cli.py`:
- Around line 1342-1368: The final success message in the CLI flow is using the
merged was_regenerated flag, so it can claim “Successfully combined” even when
only individual session files were rewritten and the combined output was
skipped. Update the logic in cli.py around the post-conversion click.echo branch
to use a combined-output-specific signal from converter.py instead of
was_regenerated, and surface the actual regenerated session count from
_generate_individual_session_files rather than counting all session-* files with
glob. Ensure the message reflects whether output_path was truly rewritten and
that session_suffix reports only work done in this run.
---
Outside diff comments:
In `@claude_code_log/converter.py`:
- Around line 1914-1950: The paginated HTML branch in converter.py sets
did_regenerate to True even when _generate_paginated_html skips all pages and
writes nothing, which makes the regeneration status inaccurate. Update
_generate_paginated_html to report whether it actually wrote any page (for
example by returning a flag or count), then use that result in the
use_pagination branch instead of unconditionally setting did_regenerate. Keep
the fix localized around _generate_paginated_html and the did_regenerate
assignment so the CLI only reports regeneration when a page was really
rewritten.
- Around line 2028-2057: The current `regenerated` out-parameter in
`converter.py` is too coarse because `regenerated.append(did_regenerate or
sessions_regenerated > 0)` merges combined-output regeneration with session-file
regeneration. Update the `regenerated` signal in the conversion flow around
`_generate_individual_session_files` so the caller can distinguish
`did_regenerate` from `sessions_regenerated > 0` separately, such as by
returning two flags or a tuple, and then adjust the CLI-side handling to use the
combined-specific flag for the “Successfully combined” message.
---
Nitpick comments:
In `@test/test_output_explicit.py`:
- Around line 273-296: Add a regression test for the mixed case in
`main`/`converter` where `write_combined=True`, the combined output is already
current, but a per-session file is regenerated (for example after deleting one
session HTML file). Assert that the CLI does not print “Successfully combined”
when the combined file was skipped, and instead emits a more accurate
confirmation tied to the session regeneration path; use the existing
`test_combined_no_confirms_session_generation` pattern as the reference for
locating the output assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3506a76e-bf4b-46b0-98c2-a33d9218ab03
📒 Files selected for processing (4)
claude_code_log/cli.pyclaude_code_log/converter.pytest/test_html_regeneration.pytest/test_output_explicit.py
…mbined" message) CodeRabbit review of the prior commit: folding session regeneration into a single merged bool made the success message too coarse. When the combined transcript was current but a session file was rewritten (e.g. a deleted session HTML), the CLI printed "Successfully combined ... to combined_transcripts.html" on top of the converter's "combined_transcripts.html is current, skipping regeneration" — directly contradictory, and the exact misleading-message class this PR set out to remove. The paginated branch had the mirror problem: it hard-coded did_regenerate=True even when every page was current and nothing was written. Track the two axes separately via a small RegenerationReport out-param (combined_regenerated: bool, sessions_regenerated: int), replacing the list[bool] mailbox: - _generate_paginated_html now returns (first_page_path, wrote_any) so the combined flag is accurate for large/paginated projects too. - The CLI gates the word "combined" (and the combined-file path) strictly on combined_regenerated; when only session files were written it reports "Successfully processed N transcript files from <dir> and generated M individual session files". Pure skips still print nothing. - The per-session count now reflects files actually regenerated this run (report.sessions_regenerated), not a glob of all session-*.html. Test: test_combined_current_but_session_regenerated_says_processed pins the mixed case (RED before: printed the false "Successfully combined"). All three --combined modes + the mixed case verified by running the CLI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
(Claude) Addressed the review of
@coderabbitai full review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 253 --repo daaain/claude-code-log --json headRefOid,commits -q '.headRefOid'
git fetch origin pull/253/head:pr253 2>&1 | tail -5 || true
git checkout pr253 2>&1 | tail -5 || true
echo "---cli.py---"
sed -n '1,200p' claude_code_log/cli.py | grep -n "Successfully\|regenerated\|write_combined"
echo "---converter.py---"
grep -n "RegenerationReport\|regenerated\|did_regenerate\|_generate_paginated_html\|sessions_regenerated\|combined_regenerated" claude_code_log/converter.pyLength of output: 2895 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "---cli.py relevant sections---"
grep -n "RegenerationReport\|report\.\|Successfully combined\|Successfully processed\|Successfully converted" claude_code_log/cli.py
echo "---"
sed -n '1,250p' claude_code_log/cli.py | sed -n '150,250p'Length of output: 4911 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '1290,1375p' claude_code_log/cli.pyLength of output: 4261 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '1470,1560p' claude_code_log/converter.py
echo "----"
sed -n '1990,2085p' claude_code_log/converter.pyLength of output: 8442 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n "test_combined_current_but_session_regenerated_says_processed" -A 40 test/Length of output: 3489 All the fixes described in the update are correctly implemented and well-tested:
This is a solid, well-reasoned fix with clear separation of the combined vs. per-session regeneration signals, and the accompanying test verifies the previously-buggy scenario. I don't see any remaining issues with this change. ✅ Action performedReview finished.
|
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.
Summary
Follow-up to #221/#237. Two user-visible staleness/messaging bugs in the
default single-file conversion path, plus message-accuracy fixes surfaced
while reviewing the change.
Bug 1 — stale HTML on a grown single-file source
claude-code-log <file>.jsonl [--detail low]writes<file>[.low].html, thenre-runs were skipped purely on the embedded tool version
(
renderer.is_outdated), which ignores source freshness. So an in-progresssession that grew between runs was wrongly reported "current" and served stale
HTML. #237 only forced regeneration for the explicit
-o filecase.Fix: add a source-mtime term to the single-file/no-cache branch —
regenerate when the source file is newer than the existing output. Scoped to a
file source (a directory source has no single mtime and is handled by the
cache path). The version-marker check is kept for tool-version bumps.
Bug 2 — misleading success message
On a skip, the CLI printed
Successfully converted …on top of theconverter's
… is current, skipping regeneration. The converter now reportswhether it actually (re)wrote output via a non-breaking optional
regeneratedout-param (the
Pathreturn is unchanged — ~20 callers unaffected); the CLIprints the success line only on a real regeneration. Applied symmetrically to
the directory branch (
Successfully combined …).Message-accuracy fix
The regeneration gate initially tracked only the combined write, so
--combined no(which writes only per-session files, never a combined) fellcompletely silent despite producing every session file — and the per-session
count was dropped whenever the combined was current but a session was
rewritten. The gate now also counts
_generate_individual_session_files'sreturn, i.e. "combined or any session file (re)written". Pure skips still
stay quiet.
Tests
TestSingleFileStaleness(test_output_explicit.py): grown source regenerates;unchanged source skips with no redundant success line; directory no-op skip
suppresses the combined success line;
--combined noconfirms itsper-session work.
test_single_file_mode_regeneration_behavior, which previouslyenshrined the stale behavior as "expected".
All four behavioral tests were verified RED on the prior code and GREEN after.
Not addressed here
--no-cacheon a directory with a grown source can still serve stale HTML(the source-mtime term is scoped to file sources). This is pre-existing, not
introduced here, and tracked separately.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
--combined no.Tests
--combined nobehavior.