Fix: structured error types#36
Merged
Merged
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)
📝 WalkthroughWalkthroughCentralizes LocalCI error types in a new ChangesError Hierarchy & CLI Integration
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Config as ConfigLoader
participant Workflow as WorkflowAnalyzer
participant Docker as DockerUtils
participant FS as Filesystem
CLI->>Config: load_config(path)
alt config file missing
Config-->>CLI: ConfigFileNotFoundError
CLI->>CLI: print error & exit
else config IO error
Config-->>CLI: ConfigIOError
CLI->>CLI: print error & exit
else config invalid
Config-->>CLI: ConfigValidationError
CLI->>CLI: print error & exit
else config ok
CLI->>Workflow: analyze(workflow_path)
alt workflow missing
Workflow-->>CLI: WorkflowNotFoundError
CLI->>CLI: print error & exit
else parse error
Workflow-->>CLI: WorkflowParseError
CLI->>CLI: print error & exit
else workflow ok
CLI->>Docker: check_docker()
alt docker unavailable
Docker-->>CLI: DockerNotAvailableError
CLI->>CLI: print error & exit
else docker available
CLI->>FS: run jobs / read/write results
FS-->>CLI: results
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cli/localci/utils/docker.py (1)
52-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize
docker versionexecution failures toDockerNotAvailableError
subprocess.run(...)here can raiseTimeoutExpired/OSError, which currently escape as raw exceptions instead of the new structured type.Suggested fix
- result = subprocess.run( - [self._docker_path, "version", "--format", "{{.Server.Version}}"], - capture_output=True, - text=True, - timeout=10, - ) + try: + result = subprocess.run( + [self._docker_path, "version", "--format", "{{.Server.Version}}"], + capture_output=True, + text=True, + timeout=10, + ) + except (OSError, subprocess.TimeoutExpired) as exc: + raise DockerNotAvailableError(f"Docker daemon check failed: {exc}") from exc🤖 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/docker.py` around lines 52 - 57, The subprocess.run call that executes the Docker CLI ([self._docker_path, "version", ...]) can raise TimeoutExpired or OSError which should be normalized to the new DockerNotAvailableError; wrap the subprocess.run invocation in a try/except that catches subprocess.TimeoutExpired and OSError and then raise DockerNotAvailableError (preserving the original exception as the cause or including its message) so callers get the structured error instead of raw exceptions; update the block around the subprocess.run call (the code referencing self._docker_path and result) to perform this conversion.cli/localci/core/workflow.py (1)
389-397:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
FileNotFoundErrorcan still leak unwrapped fromanalyze()The Line 391 wrapper covers only
workflow_name(). Subsequent yq reads can still raise rawFileNotFoundError, which breaks the structured-error flow.Suggested direction
Wrap the full parsing block (all yq reads + job parsing) so any
FileNotFoundErroris converted toWorkflowNotFoundErrorconsistently, while preserving existingWorkflowErrorpassthrough.🤖 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/core/workflow.py` around lines 389 - 397, The analyze() flow currently only wraps self.yq.workflow_name(...) in a FileNotFoundError -> WorkflowNotFoundError conversion, so subsequent yq calls (e.g., self.yq.events(...)) or job parsing can still raise raw FileNotFoundError; wrap the entire parsing block that calls self.yq.workflow_name, self.yq.events and any job-parsing logic inside a single try/except that catches FileNotFoundError and re-raises WorkflowNotFoundError(workflow_path, exc), let existing WorkflowError (and WorkflowParseError) pass through unchanged, and for other exceptions continue to raise WorkflowParseError(workflow_path, str(exc)) so all file-not-found cases are consistently converted while preserving current error semantics.
🧹 Nitpick comments (2)
cli/tests/test_errors.py (1)
291-291: 💤 Low valueUnused constant
_FIXTURES.The
_FIXTURESpath constant is defined but not used in any of the tests in this file. Consider removing it if not needed, or verify if it's intended for future tests.♻️ Remove unused constant
-_FIXTURES = Path(__file__).parent / "fixtures" - - class TestCliWorkflowErrorPaths:🤖 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/tests/test_errors.py` at line 291, Remove the unused module-level constant _FIXTURES defined in test_errors.py (the Path(...) assignment) since it is not referenced by any tests; either delete the _FIXTURES declaration or replace its use where intended, ensuring no other tests import or rely on that symbol (search for _FIXTURES in the file to confirm).cli/tests/test_executor.py (1)
23-30: 💤 Low valueConsider importing error types from their canonical location.
The test imports
ActNotFoundErrorandDockerNotAvailableErrorfromlocalci.core.executor, but these are now defined inlocalci.errors. While this works (since executor imports them), importing from the canonical module improves clarity and future maintainability.♻️ Suggested import path update
from localci.core.executor import ( ActCommand, - ActNotFoundError, - DockerNotAvailableError, JobExecutor, JobResult, JobStatus, ) +from localci.errors import ActNotFoundError, DockerNotAvailableError🤖 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/tests/test_executor.py` around lines 23 - 30, Update the test imports to use the canonical error module: replace importing ActNotFoundError and DockerNotAvailableError from localci.core.executor with imports from localci.errors; modify the import statement in cli/tests/test_executor.py so JobExecutor, JobResult, JobStatus, and ActCommand still come from localci.core.executor but ActNotFoundError and DockerNotAvailableError are imported from localci.errors to reflect their true definitions.
🤖 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/run.py`:
- Around line 162-168: The code currently catches only WorkflowNotFoundError and
WorkflowParseError; add a final except block for the base WorkflowError to
prevent other structured workflow exceptions from bubbling out. After the two
existing excepts, add "except WorkflowError as exc:" that calls
print_error(str(exc)) and ctx.exit(1) (matching the other handlers) so all
WorkflowError subclasses are caught and handled; reference the existing
exception classes WorkflowNotFoundError, WorkflowParseError and WorkflowError
and the functions print_error and ctx.exit to locate where to insert this
fallback.
In `@cli/localci/core/config.py`:
- Around line 386-395: The YAML parsing call yaml.safe_load can raise
yaml.YAMLError which is not currently caught; update the try/except around
opening/reading/parsing (the block using config_path and yaml.safe_load) to also
catch yaml.YAMLError (either by adding a separate except or by combining
exceptions like except (OSError, yaml.YAMLError) as exc) and re-raise it wrapped
in the same structured error type (raise ConfigIOError(config_path, exc) from
exc) so malformed YAML is reported consistently; leave the subsequent
LocalCIConfig.model_validate and ConfigValidationError handling unchanged.
In `@cli/localci/errors.py`:
- Around line 149-152: The constructor for WorkflowParseError is currently
storing the cause as a string; change WorkflowParseError.__init__ to accept the
original exception object (e.g., parameter name cause: Exception or detail:
Exception) and assign self.cause = cause (the exception) while still using
str(cause) or a supplied message in the super().__init__ call; ensure self.path
remains Path(path) and the exception message remains "Failed to parse workflow
{self.path}: {detail_or_str(cause)}" so callers retain structured access to the
original exception via the self.cause attribute.
---
Outside diff comments:
In `@cli/localci/core/workflow.py`:
- Around line 389-397: The analyze() flow currently only wraps
self.yq.workflow_name(...) in a FileNotFoundError -> WorkflowNotFoundError
conversion, so subsequent yq calls (e.g., self.yq.events(...)) or job parsing
can still raise raw FileNotFoundError; wrap the entire parsing block that calls
self.yq.workflow_name, self.yq.events and any job-parsing logic inside a single
try/except that catches FileNotFoundError and re-raises
WorkflowNotFoundError(workflow_path, exc), let existing WorkflowError (and
WorkflowParseError) pass through unchanged, and for other exceptions continue to
raise WorkflowParseError(workflow_path, str(exc)) so all file-not-found cases
are consistently converted while preserving current error semantics.
In `@cli/localci/utils/docker.py`:
- Around line 52-57: The subprocess.run call that executes the Docker CLI
([self._docker_path, "version", ...]) can raise TimeoutExpired or OSError which
should be normalized to the new DockerNotAvailableError; wrap the subprocess.run
invocation in a try/except that catches subprocess.TimeoutExpired and OSError
and then raise DockerNotAvailableError (preserving the original exception as the
cause or including its message) so callers get the structured error instead of
raw exceptions; update the block around the subprocess.run call (the code
referencing self._docker_path and result) to perform this conversion.
---
Nitpick comments:
In `@cli/tests/test_errors.py`:
- Line 291: Remove the unused module-level constant _FIXTURES defined in
test_errors.py (the Path(...) assignment) since it is not referenced by any
tests; either delete the _FIXTURES declaration or replace its use where
intended, ensuring no other tests import or rely on that symbol (search for
_FIXTURES in the file to confirm).
In `@cli/tests/test_executor.py`:
- Around line 23-30: Update the test imports to use the canonical error module:
replace importing ActNotFoundError and DockerNotAvailableError from
localci.core.executor with imports from localci.errors; modify the import
statement in cli/tests/test_executor.py so JobExecutor, JobResult, JobStatus,
and ActCommand still come from localci.core.executor but ActNotFoundError and
DockerNotAvailableError are imported from localci.errors to reflect their true
definitions.
🪄 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: 9415c8bb-3fda-47cc-ba0c-0c86f5d0e53b
📒 Files selected for processing (14)
cli/localci/cli/images.pycli/localci/cli/list.pycli/localci/cli/main.pycli/localci/cli/run.pycli/localci/core/__init__.pycli/localci/core/config.pycli/localci/core/executor.pycli/localci/core/queue.pycli/localci/core/workflow.pycli/localci/errors.pycli/localci/utils/docker.pycli/localci/utils/yq.pycli/tests/test_errors.pycli/tests/test_executor.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/localci/core/workflow.py (1)
389-426:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve existing structured errors and wrap all yq reads consistently.
analyze()only wrapsworkflow_name(). Failures fromevents(),global_env(),concurrency(), orjob_names()still escape raw, and anyYqError/YqNotFoundErrorraised in these paths gets flattened intoWorkflowParseErrorinstead of keeping its original structured type.Suggested change
try: name = self.yq.workflow_name(workflow_path) + events = self.yq.events(workflow_path) + env = self.yq.global_env(workflow_path) + concurrency = self.yq.concurrency(workflow_path) + job_ids = self.yq.job_names(workflow_path) + except LocalCIError: + raise except FileNotFoundError as exc: raise WorkflowNotFoundError(workflow_path, exc) from exc except Exception as exc: raise WorkflowParseError(workflow_path, exc) from exc - - events = self.yq.events(workflow_path) - env = self.yq.global_env(workflow_path) - concurrency = self.yq.concurrency(workflow_path) - - job_ids = self.yq.job_names(workflow_path) jobs: dict[str, Job] = {} @@ try: jobs[job_id] = self._parse_job(workflow_path, job_id) except WorkflowError: raise + except LocalCIError: + raise except Exception as exc: raise WorkflowParseError( workflow_path, exc,🤖 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/core/workflow.py` around lines 389 - 426, The code only wraps self.yq.workflow_name in structured error handling; wrap the other yq reads (self.yq.events, self.yq.global_env, self.yq.concurrency, self.yq.job_names) the same way inside analyze(): for each call, catch FileNotFoundError and raise WorkflowNotFoundError(workflow_path, exc) from exc, let YqError and YqNotFoundError (or other existing yq-specific exceptions) propagate (or re-raise) so their structured types are preserved, and only convert unexpected exceptions into WorkflowParseError(workflow_path, exc) (including a helpful message). Apply this same pattern before iterating jobs and keep the existing job-parsing try/except around self._parse_job as-is.
🤖 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/errors.py`:
- Around line 167-179: Modify the two error classes so they preserve structured
metadata as attributes instead of just interpolating into the message: in
MissingFieldError.__init__ store self.field_name and self.context (and keep the
super().__init__ call with the message), and in UnsupportedMatrixError.__init__
store self.entry, self.detail and maybe self.name = entry.get("name", "unknown")
(again still call super().__init__ with the formatted message). This ensures
downstream code can access the original values from the MissingFieldError and
UnsupportedMatrixError instances without parsing the error string.
---
Outside diff comments:
In `@cli/localci/core/workflow.py`:
- Around line 389-426: The code only wraps self.yq.workflow_name in structured
error handling; wrap the other yq reads (self.yq.events, self.yq.global_env,
self.yq.concurrency, self.yq.job_names) the same way inside analyze(): for each
call, catch FileNotFoundError and raise WorkflowNotFoundError(workflow_path,
exc) from exc, let YqError and YqNotFoundError (or other existing yq-specific
exceptions) propagate (or re-raise) so their structured types are preserved, and
only convert unexpected exceptions into WorkflowParseError(workflow_path, exc)
(including a helpful message). Apply this same pattern before iterating jobs and
keep the existing job-parsing try/except around self._parse_job as-is.
🪄 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: d10bf786-003a-4387-b868-9c35f5c75bb0
📒 Files selected for processing (5)
cli/localci/core/config.pycli/localci/core/workflow.pycli/localci/errors.pycli/tests/test_errors.pycli/tests/test_workflow.py
wpak-ai
approved these changes
May 6, 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
2 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.
Closes #28
Summary by CodeRabbit
Bug Fixes
Tests