fix(errors): render user-config and base-alias failures as structured errors#1082
Open
tonyandrewmeyer wants to merge 8 commits into
Open
fix(errors): render user-config and base-alias failures as structured errors#1082tonyandrewmeyer wants to merge 8 commits into
tonyandrewmeyer wants to merge 8 commits into
Conversation
tonyandrewmeyer
force-pushed
the
agent/structured-validation-errors
branch
from
e5276c2 to
dff017d
Compare
bepri
reviewed
May 26, 2026
Comment on lines
+120
to
+123
| # TODO: This is a shallow guard keyed off the exception # noqa: FIX002 | ||
| # message. The proper fix is to raise a typed CraftError at the | ||
| # base-alias lookup site (in craft_providers/craft_platforms) so | ||
| # this string match can be removed. |
Member
There was a problem hiding this comment.
I agree with the fix proposed by this, but instead of silencing the lint error could you please:
- Create an issue on the relevant repository to fix it at the lookup site
- Create an issue on this repository that says "this block of code should be removed/improved"
- Link the first issue to the second, noting that the first one blocks the second
- Add a link to the second issue in this comment in place of the "TODO:"
| return_code = os.EX_DATAERR | ||
| transformed = errors.CraftValidationError.from_pydantic( | ||
| error, | ||
| file_name=f"{app.name}.yaml", |
Member
There was a problem hiding this comment.
It's not a guarantee that this is the offending file. For example, Snapcraft could also be any one of:
- .snapcraft.yaml
- snap/snapcraft.yaml
- build-aux/snap/snapcraft.yaml
I'm not sure what the right fix is here to pass this information down. @lengau WDYT about a solution involving kwargs to optionally pass the output of ProjectService.resolve_project_file_path() into this method?
Collaborator
There was a problem hiding this comment.
Longer term yes, but let's not block this PR on it.
Member
|
Requesting an author's review because this adds new user-oriented error messages. |
… errors Unhandled pydantic ValidationErrors and host base-alias lookup failures fell through to the generic handler and were reported as '<app> internal error: ...' with exit code 70, leaking a raw Python/ pydantic traceback (including the errors.pydantic.dev link) for what are plainly user configuration problems. handle_runtime_error now converts pydantic.ValidationError into a structured CraftValidationError with a 'Recommended resolution' (exit EX_DATAERR), and surfaces 'not a valid BuilddBaseAlias' ValueErrors as a structured config error (exit EX_CONFIG). pydantic.ValidationError is a ValueError subclass, so it is matched first. A TODO marks the deeper fix for the base-alias lookup site.
Signed-off-by: Tony Meyer <tony.meyer@canonical.com>
tonyandrewmeyer
force-pushed
the
agent/structured-validation-errors
branch
from
dff017d to
b52c3c3
Compare
Per review on canonical#1082: replace the TODO/lint-silencer with references to canonical#1086 (remove this guard) and canonical/craft-providers#969 (raise typed error at the lookup site). Signed-off-by: Tony Meyer <tony.meyer@canonical.com>
lengau
reviewed
Jun 3, 2026
Comment on lines
+70
to
+81
| Fixes | ||
| ===== | ||
|
|
||
| - Unhandled ``pydantic.ValidationError`` is now reported as a structured | ||
| project configuration error (exit ``EX_DATAERR``) with a recommended | ||
| resolution, rather than as an "internal error" with a raw pydantic | ||
| traceback. | ||
| - ``ValueError`` failures from a host base-alias lookup | ||
| (``'<release>' is not a valid BuilddBaseAlias``) are now reported as a | ||
| structured configuration error (exit ``EX_CONFIG``) instead of as an | ||
| "internal error". | ||
|
|
Collaborator
There was a problem hiding this comment.
7.0.0 is released, so let's move this to 7.1
Member
There was a problem hiding this comment.
Could you move it to the top of the version sections so it comes before 7.0.0?
bepri
approved these changes
Jun 3, 2026
7.0.0 has already been released without these entries, so move them to a new 7.1.0 (unreleased) section. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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.
Unhandled pydantic
ValidationErrors and host base-alias lookup failures fall through to the generic handler and are reported as ' internal error: ...' with exit code 70, leaking a raw Python / pydantic traceback (including the errors.pydantic.dev link) for what are plainly user configuration problems.This PR changes
handle_runtime_errorto convert pydantic.ValidationError into a structuredCraftValidationErrorwith a 'Recommended resolution' (exitEX_DATAERR), and surfaces 'not a valid BuilddBaseAlias'ValueErrors as a structured config error (exitEX_CONFIG).pydantic.ValidationErroris aValueErrorsubclass, so it is matched first. A TODO marks a potential deeper fix for the base-alias lookup site.make lint && make test? (I only get the same failures as main).docs/reference/changelog.rst)?