Add catch-all CLI exception handler#41
Merged
Merged
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a CatchAllGroup Click group that intercepts unhandled CLI exceptions, logs full tracebacks and environment metadata to a crash log under localci_home, prints a friendly error, supports ChangesCLI Catch-All Exception Handler
sequenceDiagram
participant User
participant CLI as "CLI Entry (CatchAllGroup)"
participant Invoke as "CatchAllGroup.invoke"
participant Command as "WorkflowAnalyzer / JobExecutor"
participant Crash as "log_crash"
participant File as "~/.localci/crash.log"
participant Stderr as "stderr"
User->>CLI: run localci [--debug]
CLI->>Invoke: invoke(ctx)
Invoke->>Command: execute command
alt Exception occurs
Command-->>Invoke: raises Exception
Invoke->>Invoke: catch Exception
alt debug flag set
Invoke-->>User: re-raise exception
else debug not set
Invoke->>Crash: log_crash(exc)
Crash-->>File: write timestamp, versions, traceback
Invoke->>Stderr: print friendly error and guidance
Invoke->>CLI: ctx.exit(2)
end
else Success
Invoke-->>User: normal output
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 3
🧹 Nitpick comments (2)
cli/localci/cli/main.py (1)
48-53: 💤 Low valueSimplify
_is_debuglogic.The check for
ctx.params.get("debug")on line 53 is redundant. The--debugflag is stored inctx.obj["debug"]on line 110, so checkingctx.paramsis unnecessary and could cause confusion if the two sources ever diverge.♻️ Simplified version
def _is_debug(ctx: click.Context) -> bool: """Return True when ``--debug`` was passed on the CLI.""" - obj = ctx.obj - if obj is not None and obj.get("debug"): - return True - return bool(ctx.params.get("debug")) + return bool(ctx.obj and ctx.obj.get("debug"))🤖 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 `@cli/localci/cli/main.py` around lines 48 - 53, The _is_debug function mixes two sources for the debug flag causing redundancy; update the function (named _is_debug) to only consult ctx.obj for the flag: ensure you read ctx.obj into a local variable (obj), return True when obj is not None and obj.get("debug") truthy, otherwise return False — remove the ctx.params.get("debug") check so the function consistently relies on ctx.obj["debug"] as the single source of truth.cli/localci/utils/crash.py (1)
22-35: ⚡ Quick winConsider adding error handling for file write failures.
If writing the crash log fails (e.g., disk full, permission denied), the exception will bubble up and could mask the original exception that was being logged. Consider wrapping the file write in a try/except block and falling back to stderr if the write fails.
🛡️ Proposed error handling
def log_crash(exc: BaseException) -> Path: """Write a full crash report for *exc* and return the log file path.""" path = crash_log_path() lines = [ f"timestamp: {datetime.now(timezone.utc).isoformat()}\n", f"localci_version: {__version__}\n", f"python_version: {sys.version}\n", f"platform: {platform.platform()}\n", "\n", "traceback:\n", *traceback.format_exception(type(exc), exc, exc.__traceback__), ] - path.write_text("".join(lines), encoding="utf-8") + try: + path.write_text("".join(lines), encoding="utf-8") + except Exception as write_error: + # Fallback to stderr if crash log write fails + import sys + print(f"Warning: Could not write crash log to {path}: {write_error}", file=sys.stderr) return 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 `@cli/localci/utils/crash.py` around lines 22 - 35, The log_crash function currently writes the crash report via path.write_text without guarding against IO failures; wrap the write in a try/except Exception as e around the path.write_text call in log_crash, and on failure write the same report to stderr (using sys.stderr.write or print) including both the original exception information and the write error (e) so the original crash isn't masked; still return the Path (or document behavior) after attempting the fallback. Use existing symbols: log_crash, crash_log_path, traceback.format_exception, and sys.stderr to implement the fallback.
🤖 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 `@cli/localci/cli/main.py`:
- Around line 56-70: The multi-line message built in _print_unhandled_error()
lets Rich wrap the long log path mid-word; change the implementation to emit the
crash log line as a single string (rather than two concatenated f-strings across
lines) or call print_error()/click.echo() with no_wrap behaviour so the path
isn't wrapped; specifically, update the messages construction around f"Please
file a bug report and attach {display_path} (contains the full traceback)." into
one contiguous string and/or pass a no-wrap option to print_error (or fall back
to click.echo(display_string, err=True)) when printing from
_print_unhandled_error so the crash log filename remains intact.
In `@cli/localci/utils/crash.py`:
- Around line 25-34: The metadata lines in the `lines` list are missing newline
terminators so joining with `"".join(lines)` concatenates fields; update the
construction of `lines` (the list used before `path.write_text`) to ensure each
metadata string ends with a newline (or join with `"\n"` and append a final
newline) so the timestamp, localci_version, python_version and platform entries
are on separate lines while leaving `traceback.format_exception(type(exc), exc,
exc.__traceback__)` unchanged.
In `@cli/tests/test_cli.py`:
- Around line 320-335: The failing test test_unhandled_exception_friendly_output
should not rely on an unwrapped literal "crash.log"; update the assertion to be
wrapping-resilient by either asserting the CRASH_LOG_NAME constant appears (or
the full crash path pattern) or by normalizing result.output (e.g., remove
whitespace/newlines) before checking; change the final assertion in
test_unhandled_exception_friendly_output to use CRASH_LOG_NAME or a
whitespace-stripped search so Rich's line-wrapping (which can split "crash.log"
into "crash.lo\ng") doesn't break the test.
---
Nitpick comments:
In `@cli/localci/cli/main.py`:
- Around line 48-53: The _is_debug function mixes two sources for the debug flag
causing redundancy; update the function (named _is_debug) to only consult
ctx.obj for the flag: ensure you read ctx.obj into a local variable (obj),
return True when obj is not None and obj.get("debug") truthy, otherwise return
False — remove the ctx.params.get("debug") check so the function consistently
relies on ctx.obj["debug"] as the single source of truth.
In `@cli/localci/utils/crash.py`:
- Around line 22-35: The log_crash function currently writes the crash report
via path.write_text without guarding against IO failures; wrap the write in a
try/except Exception as e around the path.write_text call in log_crash, and on
failure write the same report to stderr (using sys.stderr.write or print)
including both the original exception information and the write error (e) so the
original crash isn't masked; still return the Path (or document behavior) after
attempting the fallback. Use existing symbols: log_crash, crash_log_path,
traceback.format_exception, and sys.stderr to implement the fallback.
🪄 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: 3dfbe96e-b64f-4b65-869c-6bf08e49456b
📒 Files selected for processing (4)
.gitignorecli/localci/cli/main.pycli/localci/utils/crash.pycli/tests/test_cli.py
bradjin8
requested changes
May 27, 2026
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 `@cli/tests/test_cli.py`:
- Around line 367-371: The test currently replaces newlines with spaces
(output_no_newlines = result.output.replace("\n", " ")) which still leaves
spaces when Rich wraps lines and breaks filenames; change the normalization to
strip all whitespace from result.output (e.g., use a whitespace-collapsing
approach such as joining split tokens or a regex to remove \s) so that
comparisons against CRASH_LOG_NAME succeed; update the variable currently named
output_no_newlines (or create a new normalized_output) and use that when
asserting presence of CRASH_LOG_NAME or f".localci/{CRASH_LOG_NAME}" in the
test.
🪄 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: 809ea87e-3ab9-4d34-a827-167212f37478
📒 Files selected for processing (3)
cli/localci/cli/main.pycli/localci/utils/crash.pycli/tests/test_cli.py
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/localci/cli/main.py
|
Actionable comments posted: 0 |
bradjin8
approved these changes
May 28, 2026
wpak-ai
approved these changes
May 28, 2026
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
3 participants
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
Adds a top-level catch-all exception handler to the
localciCLI so unhandlederrors no longer surface as raw Python tracebacks. Unhandled exceptions now
produce a user-friendly message, write a full diagnostic report to
~/.localci/crash.log, and exit with code2(distinct from1used foruser/config errors). A
--debugflag re-raises for developer debugging.Resolves w4_issue_01.md (Eval Test 10).
Changes
cli/localci/utils/crash.py—log_crash(exc)writes timestamp,localciversion, Python version, OS platform, and full traceback to~/.localci/crash.log.cli/localci/cli/main.pyCatchAllGroup(click.Group)wrapsinvoke()withexcept Exception,re-raising
click.exceptions.Exit/click.ClickExceptionso existingexit flows are preserved.
and a pointer to the crash log for bug reports.
--debugflag re-raises unhandled exceptions instead ofcatching them.
1; catch-all uses2.cli/tests/test_cli.py— newTestCatchAllHandlercovering:friendly output (no traceback), crash log contents,
--debugre-raise,and config-error exit-code regression.
Behaviour
RuntimeError(e.g. act/Docker bug)~/.localci/crash.log, exit2localci --debug ...with unhandled exceptionConfigErrorfamily)1WorkflowError, etc.)1Test plan
pytest tests/test_cli.py::TestCatchAllHandler -v— 4/4 passingpytest(full suite) — 492 passedpytestjob on Python 3.10 / 3.11 / 3.12 greenAcceptance criteria
except Exceptionat CLI entry point(3) bug-report suggestion with log location
~/.localci/crash.log2for internal errors;1retained for user/config errorsNotes for reviewers
click.exceptions.Exitandclick.ClickExceptionso normal CLI exits and Click usage errors keeptheir current behaviour.
KeyboardInterruptandSystemExitare not subclasses ofException, soCtrl-C and explicit
sys.exit()calls still propagate.crash.logis overwritten per crash (single “latest crash” file) — matchesthe issue’s example wording.
cli/localci/errors.py; the existing exception hierarchy isunaffected.
Related issue
close #38
Summary by CodeRabbit
New Features
Improvements
Chores
Tests