Feat/cli export exit codes#56
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 (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe ChangesCLI Exit-Code Documentation Clarification
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (1 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.
Actionable comments posted: 1
🤖 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 `@scripts/export.py`:
- Line 18: Change the misleading comment that states "--session single-export
always exits 0" to clarify that the 0/1/2 exit-code mapping applies only to bulk
export; mention that single-session exports (handled by cmd_export) may still
exit non-zero (e.g., via _die("Session not found...") which returns 1). Update
the parenthetical text to something like "0/1/2 mapping applies only to bulk
export; single-session exports may exit 0 or non-zero (e.g., cmd_export can call
_die)."
🪄 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: c0e7937c-7a4b-4df4-8f59-c9dd37da2dab
📒 Files selected for processing (3)
README.mdscripts/export.pytests/test_cli_export_exit_codes.py
|
Actionable comments posted: 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.
…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
clean6378-max-it
force-pushed
the
feat/cli-export-exit-codes
branch
from
3898453 to
b022a70
Compare
|
Actionable comments posted: 0 |
|
@CodeRabbit, I need you to reeview this PR in more details |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== utils/tool_dispatch.py ==="
cat utils/tool_dispatch.py 2>/dev/null || echo "FILE NOT FOUND"
echo ""
echo "=== utils/session_peek.py ==="
cat utils/session_peek.py 2>/dev/null || echo "FILE NOT FOUND"
echo ""
echo "=== utils/jsonl_helpers.py ==="
cat utils/jsonl_helpers.py 2>/dev/null || echo "FILE NOT FOUND"
echo ""
echo "=== utils/jsonl_parser.py ==="
cat utils/jsonl_parser.py 2>/dev/null || echo "FILE NOT FOUND"
echo ""
echo "=== scripts/export.py ==="
cat scripts/export.py 2>/dev/null || echo "FILE NOT FOUND"
echo ""
echo "=== docs/architecture.md ==="
cat docs/architecture.md 2>/dev/null || echo "FILE NOT FOUND"
echo ""
echo "=== utils/__init__.py ==="
cat utils/__init__.py 2>/dev/null || echo "FILE NOT FOUND"
echo ""
echo "=== All files in utils/ ==="
ls -la utils/ 2>/dev/null || echo "DIR NOT FOUND"
echo ""
echo "=== Check for circular import risks ==="
rg -n "^from utils\." utils/ --include="*.py" 2>/dev/null
rg -n "^import utils\." utils/ --include="*.py" 2>/dev/null
echo ""
echo "=== Check re-exports in jsonl_parser.py ==="
rg -n "^from .* import|^import " utils/jsonl_parser.py 2>/dev/null
echo ""
echo "=== Check all imports of jsonl_parser symbols from elsewhere ==="
rg -rn "from utils.jsonl_parser import|from utils import.*parse_session\|quick_session_info\|_parse_tool_result\|_TOOL_RESULT_DISPATCH\|_strip_system_tags" --include="*.py" 2>/dev/null
echo ""
echo "=== Check tests directory ==="
ls tests/ 2>/dev/null || ls test/ 2>/dev/null || echo "No tests dir found"
find . -name "test_*.py" -o -name "*_test.py" 2>/dev/null | head -20
echo ""
echo "=== Check for any TODO / FIXME in new files ==="
rg -n "TODO|FIXME|HACK|XXX" utils/tool_dispatch.py utils/session_peek.py utils/jsonl_helpers.py 2>/dev/null
echo ""
echo "=== Check type annotations in new modules ==="
rg -n "def " utils/tool_dispatch.py utils/session_peek.py utils/jsonl_helpers.py 2>/dev/null
echo ""
echo "=== Check _TOOL_RESULT_DISPATCH structure ==="
grep -A 60 "_TOOL_RESULT_DISPATCH" utils/tool_dispatch.py 2>/dev/null | head -80
echo ""
echo "=== PR diff summary ==="
gh pr view 56 --json files,title,body,additions,deletions 2>/dev/null || echo "gh not available"Length of output: 71654
Detailed Review
|
| Area | Severity | Finding |
|---|---|---|
| PR scope | 🔴 Critical | New modules (tool_dispatch.py, session_peek.py, jsonl_helpers.py) were not committed — the monolith split described in the PR body is absent from the diff |
scripts/export.py docstring |
✅ Good | Accurate and useful exit-code clarification |
_strip_system_tags |
🟡 Minor | import re inside function should be at module level |
task_message predicate |
🟠 Medium | Intentionally broad predicate shadows narrower task predicates; acknowledged but carries classification risk |
quick_session_info |
🟡 Minor | Off-by-one (reads 81 lines, not 80); constant undocumented |
architecture.md |
ℹ️ Info | Accurate today; will need updating once the split actually lands |
Recommendation: Either push the refactoring commits so the branch matches the PR description, or update the PR title/description to reflect that this is only the exit-code documentation fix (which is a valid standalone change).
1 participant
closes #53
Summary
Mechanical split of the
utils/jsonl_parser.pymonolith (~762 lines) into focused modules with no behavior change. The 15-predicate tool-result dispatch table is isolated inutils/tool_dispatch.pywith an explicit ordering contract.quick_session_infomoves toutils/session_peek.py; shared content helpers move toutils/jsonl_helpers.py. All existing imports fromutils.jsonl_parsercontinue to work via re-exports. No test file changes.Changes
utils/tool_dispatch.py—_TOOL_RESULT_DISPATCH, all_tool_result_pred_*/_tool_result_build_*,_parse_tool_resultutils/session_peek.py—quick_session_info()two-pass metadata peekutils/jsonl_helpers.py—_entry_message,_normalize_content,_extract_text,_extract_images,_infer_title,_strip_system_tagsutils/jsonl_parser.py(slimmed) —parse_session(), message processors,_track_file_activity, re-exportsdocs/architecture.md— one-line update: dispatch table lives inutils/tool_dispatch.pyDispatch predicate order is unchanged (first match wins;
task_messagebefore narrower task arms).Backward compatibility
Existing imports unchanged:
from utils.jsonl_parser import parse_sessionfrom utils.jsonl_parser import quick_session_infofrom utils.jsonl_parser import _parse_tool_result, _TOOL_RESULT_DISPATCHfrom utils.jsonl_parser import _strip_system_tagsTest plan
Out of scope
Summary by CodeRabbit
Release Notes