Extract stdout/stderr/cmd/rc from failed tasks in step1 parser#20
Open
PalmPalm7 wants to merge 3 commits intoredhat-et:mainfrom
Open
Extract stdout/stderr/cmd/rc from failed tasks in step1 parser#20PalmPalm7 wants to merge 3 commits intoredhat-et:mainfrom
PalmPalm7 wants to merge 3 commits intoredhat-et:mainfrom
Conversation
The job parser previously only captured res.msg from failed task events, which is often just a generic message like "non-zero return code". Now extracts res.stdout, res.stderr, res.cmd, and res.rc when present, with a fallback to event-level stdout when res fields are missing/suppressed. Closes redhat-et#15 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis pull request extends the root-cause-analysis skill to capture additional diagnostic metadata from failed Ansible tasks. The schema now includes stdout, stderr, cmd, and return code fields, while the job parser implementation adds extraction logic with fallback chains to populate these fields from available event data sources. Changes
Sequence Diagram(s)sequenceDiagram
participant AnsibleEvent as Ansible Event
participant Parser as Job Parser
participant FallbackLogic as Fallback Chain
participant Output as Task Info Object
AnsibleEvent->>Parser: Send event_data.res with diagnostics
Parser->>Parser: Check res.stdout present?
alt res.stdout exists and non-empty
Parser->>Output: Use res.stdout
else res.stdout missing/empty
Parser->>FallbackLogic: Parse event["stdout"]
FallbackLogic->>FallbackLogic: Strip ANSI codes
FallbackLogic->>FallbackLogic: Extract trailing JSON
alt JSON parsed successfully
FallbackLogic->>Output: Populate stdout, stderr, cmd
else JSON extraction fails
FallbackLogic->>Output: Use raw event["stdout"]
end
end
Parser->>Parser: Extract rc, cmd from res
Parser->>Output: Populate rc, cmd
Parser->>Parser: Extract stderr from res
Parser->>Output: Populate stderr
Output-->>AnsibleEvent: Return enriched task_info
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
When Ansible suppresses res fields (e.g. _ansible_no_log), stdout/stderr/cmd are missing from the structured result but still embedded as a JSON blob in the ANSI-rendered event.stdout. This adds a fallback parser that extracts structured fields from the rendered output before falling back to raw stdout. Fixes: redhat-et#15 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Hiding the detailed output since it's a public demo. After re-running |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/root-cause-analysis/scripts/job_parser.py (1)
13-33:⚠️ Potential issue | 🟠 MajorAdd type cast for
json.load()returns to match declared return type.The function declares
dict[str, Any]butjson.load()returnsAny, triggering mypy'swarn_return_anycheck. Cast all three return statements to the declared type.Fix
from typing import Any +from typing import cast @@ - return json.load(f) + return cast(dict[str, Any], json.load(f)) @@ - return json.load(f) + return cast(dict[str, Any], json.load(f)) @@ - return json.load(f) + return cast(dict[str, Any], json.load(f))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/root-cause-analysis/scripts/job_parser.py` around lines 13 - 33, The function load_job_log returns dict[str, Any] but calls json.load() which is typed as Any; update all three return sites to cast the json.load() result to dict[str, Any] (e.g., using typing.cast) so the return type matches the function signature: cast the value returned inside the gzip branch (inside the gzip.open try), the plain JSON branch (open try), and the fallback gzip branch (except UnicodeDecodeError) to dict[str, Any].
🧹 Nitpick comments (2)
skills/root-cause-analysis/tests/scripts/test_job_parser.py (2)
208-210: Tighten thestdoutabsence assertion.Line 209 is overly permissive (
or), so unexpected non-emptystdoutcan still pass. In this fixture path, an exact absence check is clearer and safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/root-cause-analysis/tests/scripts/test_job_parser.py` around lines 208 - 210, The current assertion for "stdout" uses an OR which allows a non-empty stdout to slip through; update the check in the test (in tests/scripts/test_job_parser.py where variable task is asserted) to require the key is absent exactly — replace the permissive assertion with one that asserts "stdout" not in task so the test fails if the key is present at all.
236-308: Add a direct regression test for rendered JSON fallback fields.Current tests validate raw
event.stdoutfallback andres.stdoutpriority, but they don’t directly assert extraction of parsedrc/cmd/stderrfrom rendered output (FAILED! => {...}). Add one case to lock that branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/root-cause-analysis/tests/scripts/test_job_parser.py` around lines 236 - 308, Add a new unit test that covers the fallback where rendered JSON in the event-level stdout contains the diagnostic fields (e.g., "FAILED! => { ... }") and those parsed values should populate rc/cmd/stderr when res lacks them: create a test (similar to existing tests) that constructs an event with event "runner_on_failed", a res with only msg, and an event-level stdout containing the "FAILED! => {\"rc\":...,\"cmd\":...,\"stderr\":\"...\"}" payload; call _extract_failed_tasks and assert that the returned task includes rc, cmd, and stderr populated from the parsed JSON; use the existing test names as a pattern (e.g., test_extract_failed_tasks_parsed_rendered_stdout_fallback) and reference _extract_failed_tasks to locate where behavior is validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/root-cause-analysis/scripts/job_parser.py`:
- Around line 209-216: The fallback that parses rendered output only runs when
"stdout" or "stderr" are missing and only copies ("stdout","stderr","cmd"),
which means missing "cmd" or "rc" can be ignored; update the conditional around
task_info/event to run the fallback when any of the expected fields are missing
(at least "stdout","stderr","cmd","rc"), and expand the loop that copies parsed
values from _parse_result_from_rendered_output(event.get("stdout","")) to
include "rc" as well as "cmd"/"stdout"/"stderr" so task_info gets recovered for
any missing field; adjust the identical logic in the later occurrence the same
way referencing task_info, event, and _parse_result_from_rendered_output.
---
Outside diff comments:
In `@skills/root-cause-analysis/scripts/job_parser.py`:
- Around line 13-33: The function load_job_log returns dict[str, Any] but calls
json.load() which is typed as Any; update all three return sites to cast the
json.load() result to dict[str, Any] (e.g., using typing.cast) so the return
type matches the function signature: cast the value returned inside the gzip
branch (inside the gzip.open try), the plain JSON branch (open try), and the
fallback gzip branch (except UnicodeDecodeError) to dict[str, Any].
---
Nitpick comments:
In `@skills/root-cause-analysis/tests/scripts/test_job_parser.py`:
- Around line 208-210: The current assertion for "stdout" uses an OR which
allows a non-empty stdout to slip through; update the check in the test (in
tests/scripts/test_job_parser.py where variable task is asserted) to require the
key is absent exactly — replace the permissive assertion with one that asserts
"stdout" not in task so the test fails if the key is present at all.
- Around line 236-308: Add a new unit test that covers the fallback where
rendered JSON in the event-level stdout contains the diagnostic fields (e.g.,
"FAILED! => { ... }") and those parsed values should populate rc/cmd/stderr when
res lacks them: create a test (similar to existing tests) that constructs an
event with event "runner_on_failed", a res with only msg, and an event-level
stdout containing the "FAILED! => {\"rc\":...,\"cmd\":...,\"stderr\":\"...\"}"
payload; call _extract_failed_tasks and assert that the returned task includes
rc, cmd, and stderr populated from the parsed JSON; use the existing test names
as a pattern (e.g., test_extract_failed_tasks_parsed_rendered_stdout_fallback)
and reference _extract_failed_tasks to locate where behavior is validated.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7ac6c14-ce1e-43ee-9424-30dc9c770044
📒 Files selected for processing (4)
skills/root-cause-analysis/schemas/job_context.schema.jsonskills/root-cause-analysis/scripts/job_parser.pyskills/root-cause-analysis/tests/data/sample_job.jsonskills/root-cause-analysis/tests/scripts/test_job_parser.py
Comment on lines
+209
to
+216
| if "stdout" not in task_info or "stderr" not in task_info: | ||
| event_stdout = event.get("stdout", "") | ||
| if event_stdout: | ||
| parsed = _parse_result_from_rendered_output(event_stdout) | ||
| if parsed: | ||
| for field in ("stdout", "stderr", "cmd"): | ||
| if field not in task_info and field in parsed: | ||
| task_info[field] = parsed[field] |
There was a problem hiding this comment.
Fallback extraction misses rc and can skip missing cmd/rc.
Line 214 copies only stdout/stderr/cmd, and Line 209 only enters fallback when stdout or stderr is missing. If cmd or rc are the only missing fields, they are never recovered from rendered output.
Proposed fix
- if "stdout" not in task_info or "stderr" not in task_info:
+ if any(field not in task_info for field in ("stdout", "stderr", "cmd", "rc")):
event_stdout = event.get("stdout", "")
if event_stdout:
parsed = _parse_result_from_rendered_output(event_stdout)
if parsed:
- for field in ("stdout", "stderr", "cmd"):
- if field not in task_info and field in parsed:
- task_info[field] = parsed[field]
+ for field in ("stdout", "stderr", "cmd", "rc"):
+ value = parsed.get(field)
+ if field not in task_info and value is not None and value != "":
+ task_info[field] = valueAlso applies to: 219-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/root-cause-analysis/scripts/job_parser.py` around lines 209 - 216, The
fallback that parses rendered output only runs when "stdout" or "stderr" are
missing and only copies ("stdout","stderr","cmd"), which means missing "cmd" or
"rc" can be ignored; update the conditional around task_info/event to run the
fallback when any of the expected fields are missing (at least
"stdout","stderr","cmd","rc"), and expand the loop that copies parsed values
from _parse_result_from_rendered_output(event.get("stdout","")) to include "rc"
as well as "cmd"/"stdout"/"stderr" so task_info gets recovered for any missing
field; adjust the identical logic in the later occurrence the same way
referencing task_info, event, and _parse_result_from_rendered_output.
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
1 participant
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.
The job parser previously only captured res.msg from failed task events, which is often just a generic message like "non-zero return code". Now extracts res.stdout, res.stderr, res.cmd, and res.rc when present, with a fallback to event-level stdout when res fields are missing/suppressed.
Ref: #15
Testing: in progress.
Summary by CodeRabbit
New Features
Tests