feat(cli): non-zero exit codes on partial export failure#55
Conversation
|
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 as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds explicit process exit codes (0 success, 1 total failure, 2 partial failure) to the ChangesExport CLI Exit Codes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/export.py (1)
479-493:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
--since lastearly returns bypass failure exit codes.On Line 482 and Line 492,
cmd_export()returns before_exit_bulk_export(export_result)runs. If failures were recorded during bulk export in this mode, the command exits0instead of1/2.💡 Suggested fix
if since == "last": if latest_day is None: print("Nothing to export (no qualifying sessions in scope).") + _exit_bulk_export(export_result) return print( f"Latest activity end-date (UTC): {latest_day.isoformat()} — " "exporting sessions that overlap that calendar day." ) if export_result.latest_day_match_count == 0: print( f"No sessions overlap {latest_day.isoformat()} (UTC); " "nothing to export." ) + _exit_bulk_export(export_result) return🤖 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 `@scripts/export.py` around lines 479 - 493, The early returns inside cmd_export when since == "last" skip calling _exit_bulk_export(export_result), causing success exit codes even if export_result recorded failures; replace the plain returns after the print statements (the branches that handle latest_day is None and export_result.latest_day_match_count == 0) with a call to _exit_bulk_export(export_result) so the process exits using the proper failure code, retaining the printed messages; ensure you reference cmd_export, latest_day, export_result, and _exit_bulk_export when making the change.
🤖 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.
Outside diff comments:
In `@scripts/export.py`:
- Around line 479-493: The early returns inside cmd_export when since == "last"
skip calling _exit_bulk_export(export_result), causing success exit codes even
if export_result recorded failures; replace the plain returns after the print
statements (the branches that handle latest_day is None and
export_result.latest_day_match_count == 0) with a call to
_exit_bulk_export(export_result) so the process exits using the proper failure
code, retaining the printed messages; ensure you reference cmd_export,
latest_day, export_result, and _exit_bulk_export when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 633a09ec-15fc-4e4e-930a-15504692dffc
📒 Files selected for processing (3)
README.mdscripts/export.pytests/test_cli_export_exit_codes.py
Call _exit_bulk_export(export_result) when cmd_export returns early for latest_day is None or zero overlap, matching the empty-export path so recorded failures map to exit 1/2 instead of always exiting 0.
Only print the Exported N of M summary when exported_session_count or failure_count is non-zero, so clean --since incremental cron runs with unchanged mtimes stay silent on stderr. Add subprocess test for the no-op path; tighten since-last early-return test to realistic 0/0 counts.
|
@CodeRabbit, can you review this one more time in more depth? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch the PR diff
gh pr view 55 --repo cppalliance/claude-code-chat-browser --json files,body,title,commits
echo "=== DIFF ==="
gh pr diff 55 --repo cppalliance/claude-code-chat-browserLength of output: 15395 🏁 Script executed: #!/bin/bash
echo "=== scripts/export.py ==="
cat scripts/export.py
echo ""
echo "=== utils/export_engine.py ==="
cat utils/export_engine.py 2>/dev/null || echo "NOT FOUND"
echo ""
echo "=== tests/test_cli_e2e.py ==="
cat tests/test_cli_e2e.py 2>/dev/null || echo "NOT FOUND"Length of output: 42461
Deep Review:
|
| Mode | total_candidates value |
|---|---|
all |
every session on disk ✓ (correct) |
incremental |
every session on disk, including mtime-skipped ones |
last |
scan_total — every session scanned across all projects |
incremental example: 10 sessions on disk, 8 unchanged (skipped), 2 new (both succeed):
Exported 2 of 10 sessions (0 failed) ← implies 8 failures, but they were skipped
Expected: Exported 2 of 2 sessions (0 failed) (only counting attempted sessions).
last example: 20 sessions across all projects, 5 match the latest calendar day:
Exported 5 of 20 sessions (0 failed) ← implies 15 were lost, but 15 were date-filtered out
The fix is to compute m as "attempted = total − skipped" instead of raw total_candidates:
def _exit_bulk_export(result: BulkExportResult) -> None:
n = result.exported_session_count
k = result.failure_count
# For "last" mode: only latest-day matches were attempted
if result.latest_day is not None:
m = result.latest_day_match_count
else:
# "all"/"incremental": attempted = total minus explicitly skipped
m = result.total_candidates - result.skipped_mtime_unchanged_count
if n > 0 or k > 0:
print(f"Exported {n} of {m} sessions ({k} failed)", file=sys.stderr)
if n == 0 and k > 0:
sys.exit(1)
elif k > 0:
sys.exit(2)2. 🔶 Code Clarity — Use elif in _exit_bulk_export
# current
if n == 0 and k > 0:
sys.exit(1)
if k > 0: # only reachable when n > 0, but reads as independent check
sys.exit(2)sys.exit() raises SystemExit so the second if is never reached when the first fires, but a reader has to know that. Use elif:
if n == 0 and k > 0:
sys.exit(1)
elif k > 0:
sys.exit(2)3. 🔶 Type Annotation — -> None is wrong for exit paths
_exit_bulk_export has two branches that never return. The annotation should communicate this:
from typing import NoReturn
def _exit_bulk_export(result: BulkExportResult) -> None:
...
if n == 0 and k > 0:
sys.exit(1) # these branches are NoReturn
elif k > 0:
sys.exit(2)Since the function may return (when k == 0), the overall signature stays -> None, but you could add an overloaded or split helper if you want to be precise. At minimum, a comment noting the exit paths would help type checkers and readers.
4. 🐛 Test Gap — test_since_last_early_return_invokes_exit_bulk_export doesn't validate exit code 1
The docstring says "exits with code 1 when failure_count>0", but:
fake_result = BulkExportResult(latest_day=None) # failure_count=0 (default)
monkeypatch.setattr(export, "_exit_bulk_export", _track_exit) # mocked out entirely_track_exit never calls sys.exit(), so no SystemExit is raised. The test only checks len(exit_calls) == 1 — it verifies _exit_bulk_export is called, but not that it would produce exit code 1. To actually test exit code 1 from an early-return:
fake_result = BulkExportResult(latest_day=None, failure_count=1, total_candidates=1)
# Don't monkeypatch _exit_bulk_export; instead catch SystemExit
with pytest.raises(SystemExit) as exc:
export.cmd_export(args)
assert exc.value.code == 15. 🔶 Test Fragility — next(base.iterdir()) depends on directory iteration order
In test_cli_export_partial_failure_exits_two:
project_dir = next(base.iterdir())base = tmp_path, which at that point contains only test-project/, so this works today. But if any earlier test fixture or pytest plugin creates additional entries under tmp_path, this could pick up the wrong directory. Use an explicit path:
project_dir = base / "test-project"6. 🔶 Silent unhandled exception in --session path
if session_filter:
filepath = _find_session(session_filter, base_dir)
if not filepath:
_die(f"Session not found: {session_filter}")
session = parse_session(filepath) # can raise; no try/except
stats = compute_stats(session)
_export_single(session, stats, fmt, out_dir)
returnA corrupt JSONL for a targeted session produces an uncaught traceback instead of a user-friendly error message. Consider wrapping with:
try:
session = parse_session(filepath)
stats = compute_stats(session)
_export_single(session, stats, fmt, out_dir)
except Exception as exc:
_die(f"Failed to export session {session_filter}: {exc}")This is pre-existing but now more visible since bulk export has polished error handling.
7. 🟡 Noisy stderr on clean success
A successful full export now always prints to stderr:
Exported 5 of 5 sessions (0 failed)
Scripts that treat any stderr output as a warning will flag clean exports. POSIX convention is: stderr for errors/diagnostics, stdout for normal output. Consider printing the success summary to stdout and reserving stderr for the failure cases only, or gating the summary on k > 0:
if n > 0 or k > 0:
out = sys.stderr if k > 0 else sys.stdout
print(f"Exported {n} of {m} sessions ({k} failed)", file=out)8. ℹ️ --session export is excluded from exit-code semantics (undocumented)
The --session single-export path returns early before _exit_bulk_export is ever called. This is arguably correct, but neither the docstring nor README mention it. A note in the module docstring would prevent confusion:
Exit codes apply to bulk export only (no --session flag).
Summary
| # | Severity | Issue |
|---|---|---|
| 1 | 🐛 Bug | M in stderr summary wrong for incremental and last modes |
| 4 | 🐛 Bug | Test doesn't actually validate exit code 1 from early-return path |
| 2 | 🔶 Clarity | Nested if instead of elif in _exit_bulk_export |
| 3 | 🔶 Typing | -> None annotation on a function with sys.exit() branches |
| 5 | 🔶 Fragile | next(base.iterdir()) in test |
| 6 | 🔶 UX | No error handling in --session path |
| 7 | 🟡 Minor | Success summary prints to stderr (may surprise scripts) |
| 8 | ℹ️ Doc | --session exclusion from exit codes is undocumented |
The core logic of exit codes (0/1/2 mapping) and the state-before-exit ordering are correct. Issues 1 and 4 are the ones most worth addressing before merge.
…mary - m = n + k (exported + failed) instead of total_candidates; avoids misleading counts for incremental no-ops and --since last date-filtered sessions - Use elif so mutual exclusion of exit-1/exit-2 is explicit - Route summary to stdout when k==0 (clean success), stderr when k>0; prevents cron monitors treating clean exports as failures - Add test_since_last_early_return_exits_one_on_failure: validates actual exit code 1 without mocking _exit_bulk_export - Use explicit base/test-project instead of next(base.iterdir()) - Add comment on sys.exit branches and --session doc note in docstring
3 participants
Closes #51
Summary
Adds scriptable exit codes to the CLI
exportsubcommand so callers (CI, cron, monitoring) can detect partial data loss in bulk exports. Exit-code mapping lives incmd_export()only; the shared export engine (run_bulk_export) continues to exposefailure_countwithout callingsys.exit().Exported N of M sessions (K failed)when any session was attemptedscripts/export.pymodule docstring (--helpepilog) and README CLI Export sectionDepends on PR1 (
feat/export-engine) forBulkExportResult.failure_count.Changes
scripts/export.py_exit_bulk_export()wrapper; invoked after successful write and on empty-export pathREADME.mdtests/test_cli_export_exit_codes.pyparse_sessionmonkeypatch)Behavior notes
Test plan
pytest tests/test_cli_export_exit_codes.py tests/test_cli_e2e.py -vpytest tests/test_export_engine_parity.py tests/test_export_api_bulk.py -qpython scripts/export.py export --base-dir <seed> --no-zip --out ./out→$LASTEXITCODE0Summary by CodeRabbit
New Features
Documentation
Tests