Explicit --output: always regenerate, infer format from suffix, fix stdout hang (#221, #222, #223 part 1)#237
Conversation
Three related fixes to how an explicit --output destination is handled. #221 — stale content silently kept. The single-file regeneration gate only compared the embedded `Generated by claude-code-log vX` marker, with no notion of which source produced the file, so converting a *different* transcript into an existing same-version path was skipped while the CLI still reported success. convert_jsonl_to gains a `force_regenerate` flag (first in the should_regenerate `or`, and gating the directory-mode early exit); the CLI sets it for an explicit `-o` *file* destination. Scoped to files because directory inputs already track source staleness via the cache (is_html_stale) — a different transcript to the same dir regenerates correctly without forcing — and `--all-projects` converts with output=None, so its incremental skip is never forced. This aligns convert_jsonl_to with the --session-id path, which already always overwrites. #222 — format ignored the suffix. `-o foo.md` was routed as a file but still rendered HTML. When `-f` is omitted, the format is now inferred from a recognised --output suffix (.md/.markdown -> markdown, .html, .json); an explicit conflict (`-o foo.md -f html`) is a clear UsageError instead of mismatched content. Adds utils.format_from_output_suffix and uses the Click parameter source to detect an omitted -f. #223 part 1 — `-o /dev/stdout` hung. is_outdated / check_html_version opened the path read-only and readline()'d to sniff the version; on a pipe that deadlocks. They now guard on Path.is_file() (not exists()), so a non-regular destination is treated as outdated/no-version without being opened. (Part 2 — routing status to stderr for clean stdout streaming — follows separately.) Tests in test/test_output_explicit.py cover all three (incl. the directory-input cross-source case handled by the cache). Plan for the wider --output/--format/TUI set in work/tui-output-fixes.md. Closes #221 Closes #222 Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds suffix-based output-format inference, forces regeneration for explicit file outputs, and changes renderer version checks to treat non-regular destinations as outdated or versionless. Tests and docs cover the new output behavior and pipe-safe handling. ChangesOutput handling changes
Sequence Diagram(s)sequenceDiagram
participant User
participant cli as "claude_code_log.cli.main"
participant suffix as "claude_code_log.utils.format_from_output_suffix"
participant conv as "claude_code_log.converter.convert_jsonl_to"
participant renderer as "renderer.is_outdated / check_html_version"
User->>cli: run with --output out.md
cli->>suffix: infer format from suffix
alt explicit file output
cli->>conv: call with force_regenerate=True
conv->>renderer: skip stale-file shortcut for explicit output
conv-->>User: write regenerated output
else conflict with --format
cli-->>User: raise click.UsageError
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.22.1)work/tui-output-fixes.mdmarkdownlint-cli2 v0.22.1 (markdownlint v0.40.0) 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: 2
🤖 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_output_explicit.py`:
- Around line 71-93: The test currently uses a file-like output path, so it
exercises the file-output regeneration path instead of the directory-output
behavior described in the test name/docstring. Update the test in
test_directory_input_cross_source_still_regenerates to use a suffixless output
destination for -o so force_regenerate is not triggered, or else adjust the
docstring to explicitly describe the file-output case; keep the assertions about
ALPHA_dir_source and BETA_dir_source matching the intended directory-input
scenario.
In `@work/tui-output-fixes.md`:
- Around line 14-15: Update the `#223` hang-fix note to reflect only the remaining
HTML version-sniffing path, since markdown/renderer.py and json/renderer.py
already use Path.is_file() in is_outdated(); adjust the plan wording to point at
html/renderer.py’s is_outdated() logic and keep the scope narrow so it no longer
claims those other renderers still need the guard.
🪄 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: 759777b5-c4d2-44a9-b932-3e0ab3e1f8cf
📒 Files selected for processing (9)
claude_code_log/cli.pyclaude_code_log/converter.pyclaude_code_log/html/renderer.pyclaude_code_log/json/renderer.pyclaude_code_log/markdown/renderer.pyclaude_code_log/utils.pydev-docs/application_model.mdtest/test_output_explicit.pywork/tui-output-fixes.md
- os.mkfifo is POSIX-only, so the three FIFO-based is_outdated/no-hang tests AttributeError'd on windows-latest. Guard them with skipif and add cross-platform directory-based tests (a directory is also not a regular file, so it exercises the same Path.is_file() guard without a FIFO). - Rename/retarget the directory-input regeneration test: it uses an explicit `-o combined.html` (a *file* destination), so force_regenerate is set — the test exercises the directory-input → file-output #221 path, not a directory-output path. Docstring corrected to match (CodeRabbit). - Clarify work/tui-output-fixes.md "Verified findings" are the pre-fix state each PR then fixes (CodeRabbit read them as current). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
1 participant
First of a small series addressing the
--output/--format/ stdout group of issues (#220–#223). Three related fixes around how an explicit--outputdestination is handled.#221 — existing
-ofile silently kept stale contentThe single-file regeneration gate only compared the embedded
<!-- Generated by claude-code-log vX -->marker — it had no notion of which source produced the file. Converting a different transcript into an existing same-version path was skipped while the CLI still printedSuccessfully converted.convert_jsonl_togains aforce_regenerateflag (first in theshould_regenerateor, and gating the directory-mode early exit); the CLI sets it for an explicit-ofile destination. Scoped to files because directory inputs already track source staleness via the cache (is_html_stale) — a different transcript written to the same directory regenerates correctly without forcing — and--all-projectsconverts withoutput=None, so its incremental skip is never forced. This also alignsconvert_jsonl_towith the--session-idpath, which already always overwrites.#222 — format ignored the
--outputsuffix-o foo.mdwas routed as a file but still rendered HTML. When-fis omitted, the format is now inferred from a recognised--outputsuffix (.md/.markdown→ markdown,.html,.json). An explicit conflict (-o foo.md -f html) is a clearUsageError(before any write) instead of mismatched content. Addsutils.format_from_output_suffix; uses the Click parameter source to detect an omitted-f.#223 (part 1) —
-o /dev/stdouthungis_outdated/check_html_versionopened the path read-only andreadline()'d to sniff the version; on a pipe that deadlocks. They now guard onPath.is_file()(notexists()), so a non-regular destination is treated as outdated/no-version without being opened. (Part 2 — routing status to stderr for clean stdout streaming — follows in a separate PR.)Tests
test/test_output_explicit.py(11 cases): stale-overwrite for file output, the directory-input cross-source case (handled by the cache), the three suffix inferences, matching-format OK, conflict error (asserts nothing written), and FIFO no-hang for all three renderers.just cigreen; no snapshot churn.Closes #221
Closes #222
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--formatis omitted.Bug Fixes
--formatand the output filename suffix now fail with a nonzero exit.Tests
Documentation
--output.