fix: CLI ErrorCode reuse + result_type→renderer parity contract test#134
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
📝 WalkthroughWalkthroughBulk export now propagates structured ChangesStructured export failure reporting
Tool result dispatch contracts
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CLI
participant run_bulk_export
participant ExportFailure
participant _exit_bulk_export
participant ErrorCode
CLI->>run_bulk_export: start bulk export
run_bulk_export->>ExportFailure: classify failed session
run_bulk_export-->>CLI: invoke callback with failure
run_bulk_export-->>_exit_bulk_export: return BulkExportResult
_exit_bulk_export->>ErrorCode: select dominant failure code
_exit_bulk_export-->>CLI: print standardized message and exit code
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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`:
- Around line 760-763: Update _die and its callers so INTERNAL_ERROR uses a
generic CLI failure message instead of the export-specific
failure_message_for_code text, while preserving existing messages for other
error codes. Ensure list and stats report the generic message when the projects
directory is missing.
🪄 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: d4678000-270d-48c4-b6e2-2f48a4a8d7d3
📒 Files selected for processing (6)
api/export_api.pyscripts/export.pytests/test_cli_export_exit_codes.pytests/test_error_codes.pytests/test_tool_dispatch_sync.pyutils/export_engine.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_tool_dispatch_sync.py (1)
80-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo fallback when the sliced function is the last top-level
defin the file.The lookahead
(?=^\ndef )requires a subsequent top-leveldef; if_render_tool_result(or any other target) ever becomes the last function inmd_exporter.py,re.searchreturnsNoneand the helper raisesValueError, breaking the contract test with an opaque error rather than a clear assertion failure.♻️ Allow end-of-file as a valid boundary
match = re.search( - rf"^def {re.escape(func_name)}\(.*?(?=^\ndef )", + rf"^def {re.escape(func_name)}\(.*?(?=^\ndef |\Z)", text, re.DOTALL | re.MULTILINE, )Note: the static analysis ReDoS hint on this regex is a false positive —
func_nameis always a hardcoded literal in this file's call sites, and the single non-greedy quantifier with a lookahead boundary runs in linear time, not exponential.🤖 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 `@tests/test_tool_dispatch_sync.py` around lines 80 - 90, Update _parse_top_level_function_body so its regex accepts either the next top-level def or end-of-file as the function boundary. Preserve the existing missing-function ValueError and slicing behavior, while allowing a target function such as _render_tool_result to be the final top-level definition.
🤖 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 `@tests/test_error_codes.py`:
- Around line 149-152: Update the stderr assertions in the stats CLI test to
reject SESSION_NOT_FOUND across the complete proc.stderr, rather than only the
substring preceding SESSION_AMBIGUOUS_PREFIX; preserve the existing return-code
and SESSION_AMBIGUOUS_PREFIX checks.
---
Nitpick comments:
In `@tests/test_tool_dispatch_sync.py`:
- Around line 80-90: Update _parse_top_level_function_body so its regex accepts
either the next top-level def or end-of-file as the function boundary. Preserve
the existing missing-function ValueError and slicing behavior, while allowing a
target function such as _render_tool_result to be the final top-level
definition.
🪄 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: 0674b0ee-c2ba-4ba8-a31e-1d407a561b8c
📒 Files selected for processing (5)
models/error_codes.pyscripts/export.pytests/test_error_codes.pytests/test_tool_dispatch_sync.pyutils/export_engine.py
🚧 Files skipped from review as they are similar to previous changes (2)
- utils/export_engine.py
- scripts/export.py
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 `@tests/test_tool_dispatch_sync.py`:
- Around line 82-83: Update the regex boundary in the test’s function-extraction
logic to stop at the next top-level def regardless of whether a blank line
precedes it, while retaining the end-of-file boundary. Add a fixture covering
adjacent top-level definitions and verify parity extraction remains correct.
🪄 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: c737406b-ca4a-4c90-a6b5-55b31e5323a1
📒 Files selected for processing (2)
tests/test_error_codes.pytests/test_tool_dispatch_sync.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_error_codes.py
3 participants
Closes #130
export_engine already records an ExportFailure with an Error Code for each sesison that fails.
The CLI was not using that. It picked exit status from counts alone and printed str(exc) in warnings, so the command line named failures differently than the HTTP API.
This branch routes bulk exit codes through the failure list , prints stable codes and messages on stderr, and updates _die so missing project dirs and unknown sessions report SESSION_NOT_FOUND or INTERNAL_ERROR instead of a free-form string.
The on_export_error callback now passes an ExportFailure to callersl the API looger was updated to match.
A contract test checks that result_type values from the Python dispatch builders stay in sync with TOOL_RESULT_RENDERS in registery.js and the _render_tool_result branches in md_exporter.py.
Before this, drift only showed up as a degraded fallback renderer with no test failure.
Run pytest tests/test_cli_export_exit_codes.py tests/test_tool_dispatch_sync.py tests/test_error_codes.py -q for the targeted pass, or pytest -q for the full suite.
For a quick manual check, bulk-export a tree where every session fails: stderr should show PARSE_ERROR (or another stable code). not the raw exception, and the process should exit 1.
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores