feat(x2a): Error handling - error from python log#2585
Open
elai-shalev wants to merge 4 commits intoredhat-developer:mainfrom
Open
feat(x2a): Error handling - error from python log#2585elai-shalev wants to merge 4 commits intoredhat-developer:mainfrom
elai-shalev wants to merge 4 commits intoredhat-developer:mainfrom
Conversation
Review Summary by QodoPropagate x2a-convertor errors to users via wrapper function
WalkthroughsDescription• Add run_x2a() wrapper function to capture tool output and extract error messages • Replace manual error handling with wrapper calls in all 5 x2a tool invocations • Extract actual convertor errors from output and propagate to backend as errorDetails • Preserve real-time output streaming while capturing for error message extraction Diagramflowchart LR
A["x2a tool execution"] -->|"streams output"| B["tee to file"]
B -->|"captures output"| C["X2A_OUTPUT variable"]
C -->|"on failure"| D["Extract Error: message"]
D -->|"set ERROR_MESSAGE"| E["Backend receives actual error"]
C -->|"on success"| F["Clear ERROR_MESSAGE"]
File Changes1. workspaces/x2a/plugins/x2a-backend/templates/x2a-job-script.sh
|
Code Review by Qodo
1.
|
Changed Packages
|
Contributor
Author
|
@eloycoto do you like the approach with the grepping, tailing etc? or should we have a more structured way to propagate the error? |
elai-shalev
changed the title
![rhdh-qodo-merge[bot]](https://avatars.githubusercontent.com/in/1961108?s=60&v=4){kind=small}
Comment on lines
+96
to
+106
| if [ ${rc} -ne 0 ]; then | ||
| # Extract the error block from x2a-convertor output. | ||
| # The convertor prints: \n\nError: <message>\n (potentially multi-line, always last thing before exit) | ||
| # Find the last "Error: " line number, then grab everything from there to the end. | ||
| local error_block="" | ||
| local last_error_line | ||
| last_error_line=$(echo "${X2A_OUTPUT}" | grep -n "^Error: " | tail -1 | cut -d: -f1) | ||
| if [ -n "${last_error_line}" ]; then | ||
| error_block=$(echo "${X2A_OUTPUT}" | tail -n +"${last_error_line}") | ||
| fi | ||
| ERROR_MESSAGE="${error_block:-Unexpected error during ${PHASE} phase. See the job log for details.}" |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Comment on lines
+93
to
+107
| X2A_OUTPUT=$(cat "${tmpfile}") | ||
| rm -f "${tmpfile}" | ||
|
|
||
| if [ ${rc} -ne 0 ]; then | ||
| # Extract the error block from x2a-convertor output. | ||
| # The convertor prints: \n\nError: <message>\n (potentially multi-line, always last thing before exit) | ||
| # Find the last "Error: " line number, then grab everything from there to the end. | ||
| local error_block="" | ||
| local last_error_line | ||
| last_error_line=$(echo "${X2A_OUTPUT}" | grep -n "^Error: " | tail -1 | cut -d: -f1) | ||
| if [ -n "${last_error_line}" ]; then | ||
| error_block=$(echo "${X2A_OUTPUT}" | tail -n +"${last_error_line}") | ||
| fi | ||
| ERROR_MESSAGE="${error_block:-Unexpected error during ${PHASE} phase. See the job log for details.}" | ||
| exit ${rc} |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
|
can we have a think more deeply about this? I do not think that the error here is amazing, and can cause ton of problems. What about if we do something here: And if there is a problem, we can create some kind of "error-details" file based on a env variable? We can gather after that the exceptions with proper names, etc.. so there is no weird tail things out there. |
Contributor
Author
Yeah I agree, this approach is also a non optimal workaround. An "error-details" file could work. I'm contemplating if we need to change the way the log is structured on the convertor side as well to fit this. |
Review Summary by QodoPropagate x2a-convertor errors to users via error file
WalkthroughsDescription• Add run_x2a() wrapper function for consistent error handling • Capture x2a-convertor errors from file and propagate to backend • Replace manual ERROR_MESSAGE management with wrapper calls • Support real-time log streaming while capturing error details Diagramflowchart LR
A["x2a-convertor execution"] -->|writes error on failure| B["X2A_ERROR_FILE"]
C["run_x2a wrapper"] -->|executes command| A
C -->|reads error file| B
C -->|sets ERROR_MESSAGE| D["Backend error reporting"]
E["Job phases"] -->|init, analyze, migrate, publish| C
File Changes1. workspaces/x2a/plugins/x2a-backend/templates/x2a-job-script.sh
|
Code Review by Qodo
1. run_x2a fallback lacks exit code
|
Comment on lines
+93
to
+97
| if [ -f "${X2A_ERROR_FILE}" ]; then | ||
| ERROR_MESSAGE=$(cat "${X2A_ERROR_FILE}") | ||
| else | ||
| ERROR_MESSAGE="Unexpected error during ${PHASE} phase. See the job log for details." | ||
| fi |
There was a problem hiding this comment.
1. run_x2a fallback lacks exit code 📎 Requirement gap ✧ Quality
When x2a-convertor fails and ${X2A_ERROR_FILE} is missing, the new fallback sets ERROR_MESSAGE
to a generic string instead of the required Script failed with exit code N. This can prevent the
backend from surfacing the mandated fallback errorDetails format.
Agent Prompt
## Issue description
`run_x2a()` sets a generic `ERROR_MESSAGE` when `${X2A_ERROR_FILE}` is missing, but compliance requires the fallback to be `Script failed with exit code N`.
## Issue Context
The backend relies on `ERROR_MESSAGE` for `errorDetails`. If no meaningful convertor error can be extracted, the required fallback format includes the exit code.
## Fix Focus Areas
- workspaces/x2a/plugins/x2a-backend/templates/x2a-job-script.sh[93-97]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+450
to
+457
| --project-id "${PROJECT_DIR}" 2>&1 | tee /dev/stderr) || { | ||
| if [ -f "${X2A_ERROR_FILE}" ]; then | ||
| ERROR_MESSAGE=$(cat "${X2A_ERROR_FILE}") | ||
| else | ||
| ERROR_MESSAGE="Unexpected error during publish phase (publish-aap). See the job log for details." | ||
| fi | ||
| exit 1 | ||
| } |
There was a problem hiding this comment.
2. publish-aap exits with 1 📎 Requirement gap ✧ Quality
The new publish-aap failure handler always exit 1 and uses a generic fallback message when
${X2A_ERROR_FILE} is missing. This prevents the required Script failed with exit code N fallback
and can hide the true exit code from backend errorDetails.
Agent Prompt
## Issue description
The `publish-aap` command failure handler currently hardcodes `exit 1` and uses a generic fallback message when `${X2A_ERROR_FILE}` is missing. Compliance requires `Script failed with exit code N` using the actual command exit code.
## Issue Context
If the convertor doesn't write `${X2A_ERROR_FILE}`, the backend should still receive an actionable fallback that includes the real non-zero exit code.
## Fix Focus Areas
- workspaces/x2a/plugins/x2a-backend/templates/x2a-job-script.sh[450-457]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+93
to
+96
| if [ -f "${X2A_ERROR_FILE}" ]; then | ||
| ERROR_MESSAGE=$(cat "${X2A_ERROR_FILE}") | ||
| else | ||
| ERROR_MESSAGE="Unexpected error during ${PHASE} phase. See the job log for details." |
There was a problem hiding this comment.
3. Error-file read can abort 🐞 Bug ⛯ Reliability
run_x2a and the publish-aap failure handler read ${X2A_ERROR_FILE} via cat while set -e is
in effect, so if the file exists but becomes unreadable/vanishes between the -f check and the
read, the script exits during cat before setting a fallback ERROR_MESSAGE. That defeats the PR’s
goal because the cleanup/report path will then fall back to a generic “Script failed…” message
instead of the real errorDetails.
Agent Prompt
### Issue description
`cat "${X2A_ERROR_FILE}"` can fail under `set -e` after a successful `-f` test (TOCTOU), causing an early exit before setting `ERROR_MESSAGE`.
### Issue Context
This occurs in both `run_x2a` and the publish-aap `|| { ... }` handler.
### Fix Focus Areas
- workspaces/x2a/plugins/x2a-backend/templates/x2a-job-script.sh[93-96]
- workspaces/x2a/plugins/x2a-backend/templates/x2a-job-script.sh[450-457]
### Suggested approach
- Read defensively, e.g.:
- `ERROR_MESSAGE=$(cat "${X2A_ERROR_FILE}" 2>/dev/null) || ERROR_MESSAGE="Unexpected error ..."`
- Or temporarily `set +e` around the `cat` and check its rc.
- Consider trimming to a safe size and removing trailing newlines (optional).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
|
Providing sort of "keys" to the UI will help with translations. |
eloycoto
reviewed
Mar 26, 2026
| if [ ${rc} -ne 0 ]; then | ||
| if [ -f "${X2A_ERROR_FILE}" ]; then | ||
| ERROR_MESSAGE=$(cat "${X2A_ERROR_FILE}") | ||
| else |
Contributor
There was a problem hiding this comment.
You added the error_message at the end, to be honest, I'll use this as the defult, and if there is ERROR_MESSAGE I'll append to the error message, it'll make more sense to the user.
ERROR_MESSAGE="Unexpected error during ${PHASE} phase. See the job log for more details."
if [ -f "${X2A_ERROR_FILE}" ]; then
ERROR_MESSAGE+="MESSAGE: $cat"
fi
| # On failure, reads the error details file written by x2a-convertor and sets ERROR_MESSAGE. | ||
| # On success, clears ERROR_MESSAGE. | ||
| # Usage: run_x2a uv run app.py <phase> [args...] | ||
| run_x2a() { |
Contributor
There was a problem hiding this comment.
now that we have this fucntion, what about if we add the log here:
echo "Command: uv run app.py init --source-dir ${SOURCE_BASE} \"${USER_REQ}\""
because it's already in all phases, and can be log here without issues, no?
| --project-id "${PROJECT_DIR}" 2>&1 | tee /dev/stderr) | ||
| ERROR_MESSAGE="" | ||
| --project-id "${PROJECT_DIR}" 2>&1 | tee /dev/stderr) || { | ||
| if [ -f "${X2A_ERROR_FILE}" ]; then |
Contributor
There was a problem hiding this comment.
why this is different? You can use the same function as previous, no?
elai-shalev
force-pushed
the
errors-propagate
branch
from
2317852 to
65e349f
Compare
|
mareklibra
approved these changes
Mar 31, 2026
Member
mareklibra
left a comment
mareklibra
left a comment
There was a problem hiding this comment.
The error messages will be hard to internationalize but otherwise LGTM
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.
related to https://redhat.atlassian.net/browse/FLPATH-3456 and to https://redhat.atlassian.net/browse/FLPATH-3454
relies on x2ansible/x2a-convertor#155
This PR fixes propagation of x2a-convertor errors to the user. Previously, when the convertor failed (e.g., AWS ThrottlingException, access denied), the error message was only visible in the k8s pod log. The user would see a generic "Script failed with exit code 1".
Added a run_x2a() wrapper function in the job script that:
All 5 uv run app.py calls (init, analyze, migrate, publish-project, publish-aap) now go through this wrapper. Shell-level errors (git clone failures, missing files) continue using the existing pre-set ERROR_MESSAGE pattern.