Conversation
openshift-ci
Bot
added
the
approved
WalkthroughThis PR introduces a new ChangesParse JUnit XML Skill
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 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.
🧹 Nitpick comments (3)
plugins/ci/skills/parse-junit/parse_junit.py (2)
124-130: XML parsing security consideration — acceptable tradeoff for stdlib-only design.Ruff flags
ET.parse()as vulnerable to XML attacks (S314). In this context:
- Input sources are CI job artifacts from controlled OpenShift CI infrastructure, not arbitrary untrusted user input
- Adding
defusedxmlwould introduce an external dependency, contradicting the stdlib-only design goal stated in SKILL.mdThe risk is acceptable given the controlled input source. Consider adding a brief comment noting the tradeoff for future maintainers.
📝 Optional: Add clarifying comment
def parse_junit_xml(source, source_name: str = "<stdin>") -> list: """Parse a JUnit XML file or stream, returning a list of TestResult.""" + # Note: Using stdlib xml.etree is acceptable here because input is from + # controlled CI artifacts, not arbitrary untrusted user data. Using + # defusedxml would add an external dependency. try: tree = ET.parse(source)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ci/skills/parse-junit/parse_junit.py` around lines 124 - 130, The ET.parse() call in parse_junit_xml is flagged for XML parsing security (S314); since inputs are trusted CI artifacts and the project intentionally avoids defusedxml, add a concise in-place comment above ET.parse() explaining this accepted tradeoff and rationale (trusted OpenShift CI inputs + stdlib-only policy per SKILL.md) so future maintainers understand why defusedxml was not used; reference the parse_junit_xml function and the ET.parse() call in the comment.
164-179: Note: Status precedence when multiple elements present.If a
<testcase>contains both<failure>and<error>elements (unusual but possible), the final status will be determined by whichever is checked last. Currently:failure→error→skipped, soskippedtakes final precedence if present.This is likely fine for real-world JUnit XML, but worth noting. If strict priority is needed (e.g., error > failure > skipped), consider using
elif:📝 Optional: Make precedence explicit with elif
- if failure_el is not None: + if error_el is not None: + status = "error" + error_message = error_el.get("message", "") + error_text = error_el.text or "" + elif failure_el is not None: status = "failed" failure_message = failure_el.get("message", "") failure_text = failure_el.text or "" - if error_el is not None: - status = "error" - error_message = error_el.get("message", "") - error_text = error_el.text or "" - if skipped_el is not None: + elif skipped_el is not None: status = "skipped" skipped_message = skipped_el.get("message", "")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ci/skills/parse-junit/parse_junit.py` around lines 164 - 179, The status determination uses independent ifs (failure_el, error_el, skipped_el) so later checks can override earlier ones; update the logic in parse_junit.py (the block that sets status, failure_message/failure_text, error_message/error_text, skipped_message using failure_el, error_el, skipped_el and status) to use elifs in the desired precedence order (e.g., if error_el: set status="error" and error_*; elif failure_el: set status="failed" and failure_*; elif skipped_el: set status="skipped" and skipped_message) so only the highest-priority element wins.plugins/ci/skills/parse-junit/SKILL.md (1)
156-169: Consider adding language specifiers to fenced code blocks.The CLI reference and error message code blocks would benefit from a
textlanguage specifier for consistency and to satisfy markdown linting.📝 Suggested fix for code block language specifiers
-``` +```text python3 parse_junit.py [FILES...] [OPTIONS]Similarly for the error message blocks at lines 221, 229, and 266.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ci/skills/parse-junit/SKILL.md` around lines 156 - 169, Update the fenced code blocks in SKILL.md to include a language specifier (use "text") so markdownlint passes; specifically add ```text before the CLI usage block that begins with "python3 parse_junit.py [FILES...] [OPTIONS]" and likewise prepend ```text to the three error-message/code-example blocks referenced in the review (the blocks containing the error output examples shown later in the file). Ensure you only modify the fences (opening triple backticks) and do not alter the block contents or surrounding descriptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugins/ci/skills/parse-junit/parse_junit.py`:
- Around line 124-130: The ET.parse() call in parse_junit_xml is flagged for XML
parsing security (S314); since inputs are trusted CI artifacts and the project
intentionally avoids defusedxml, add a concise in-place comment above ET.parse()
explaining this accepted tradeoff and rationale (trusted OpenShift CI inputs +
stdlib-only policy per SKILL.md) so future maintainers understand why defusedxml
was not used; reference the parse_junit_xml function and the ET.parse() call in
the comment.
- Around line 164-179: The status determination uses independent ifs
(failure_el, error_el, skipped_el) so later checks can override earlier ones;
update the logic in parse_junit.py (the block that sets status,
failure_message/failure_text, error_message/error_text, skipped_message using
failure_el, error_el, skipped_el and status) to use elifs in the desired
precedence order (e.g., if error_el: set status="error" and error_*; elif
failure_el: set status="failed" and failure_*; elif skipped_el: set
status="skipped" and skipped_message) so only the highest-priority element wins.
In `@plugins/ci/skills/parse-junit/SKILL.md`:
- Around line 156-169: Update the fenced code blocks in SKILL.md to include a
language specifier (use "text") so markdownlint passes; specifically add ```text
before the CLI usage block that begins with "python3 parse_junit.py [FILES...]
[OPTIONS]" and likewise prepend ```text to the three error-message/code-example
blocks referenced in the review (the blocks containing the error output examples
shown later in the file). Ensure you only modify the fences (opening triple
backticks) and do not alter the block contents or surrounding descriptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f3b27329-d356-4339-95d0-aea07a14dd9f
📒 Files selected for processing (6)
.claude-plugin/marketplace.jsondocs/data.jsonplugins/ci/.claude-plugin/plugin.jsonplugins/ci/skills/parse-junit/SKILL.mdplugins/ci/skills/parse-junit/parse_junit.pyplugins/ci/skills/prow-job-analyze-test-failure/SKILL.md
|
/hold Want to test against a few payloads first |
openshift-ci
Bot
added
the
do-not-merge/hold
cblecker
left a comment
cblecker
left a comment
There was a problem hiding this comment.
Nice addition — having a reusable JUnit parser instead of the agent reinventing XML parsing inline every session is a clear win. The code is clean, the SKILL.md is thorough, and the integration with prow-job-analyze-test-failure is well done.
The main thing I'd want fixed before merge is the status precedence issue (the if/if/if chain at line 169-179) — that one could silently hide test failures, which is the opposite of what this tool should do. The error handling around gzip files could also use some hardening so a single corrupt file doesn't crash the whole run.
Smaller comments inline on the docs and a couple of other edge cases.
| if failure_el is not None: | ||
| status = "failed" | ||
| failure_message = failure_el.get("message", "") | ||
| failure_text = failure_el.text or "" | ||
| if error_el is not None: | ||
| status = "error" | ||
| error_message = error_el.get("message", "") | ||
| error_text = error_el.text or "" | ||
| if skipped_el is not None: | ||
| status = "skipped" | ||
| skipped_message = skipped_el.get("message", "") |
There was a problem hiding this comment.
These are if rather than elif, so the last match wins. If a testcase has both a <failure> and a <skipped> element, this reports it as "skipped" — silently hiding a real failure.
I'd either switch to elif throughout, or if you want to capture data from all elements, separate the data extraction from the status determination so you can set an explicit priority (error > failed > skipped > passed).
| """Filter results by name regex, status, and/or lifecycle.""" | ||
| filtered = results | ||
| if name_pattern: | ||
| regex = re.compile(name_pattern, re.IGNORECASE) |
There was a problem hiding this comment.
If someone passes a bad regex pattern (e.g. --filter "["), this will crash with a raw re.error traceback. Might be worth wrapping in a try/except with a friendly error message, especially since this runs after all files have already been parsed — so you'd lose all the work.
| return [] | ||
|
|
||
| if p.suffix == ".gz" or filepath.endswith(".xml.gz"): | ||
| with gzip.open(filepath, "rt", encoding="utf-8", errors="replace") as f: |
There was a problem hiding this comment.
gzip.open() decompresses lazily, so if the .gz file is corrupt the exception (BadGzipFile, zlib.error, EOFError) gets raised inside ET.parse() — but the except ET.ParseError handler won't catch it. A single bad .gz file in a batch would crash the whole run. Could wrap this in a broader try/except, or catch OSError alongside ParseError in parse_junit_xml.
| data = sys.stdin.buffer.read() | ||
| # Transparently decompress gzip | ||
| if data[:2] == b"\x1f\x8b": | ||
| data = gzip.decompress(data) |
There was a problem hiding this comment.
Same theme as the .gz file handling — gzip.decompress() here is outside any try/except. Corrupt or truncated gzip data on stdin (not uncommon when piping from a flaky gcloud storage cat) would crash with a traceback instead of a clean error.
| # CLI | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| def main(): |
There was a problem hiding this comment.
Nit: main() always exits 0, even if every file fails to parse. The SKILL.md documents exit code 0 = success and 2 = argument error, but there's no exit code for "ran but encountered errors." Might not matter much since this is primarily consumed by AI agents, but if anyone uses it in a set -e script they'd get silent failures.
| agg_failures = [] | ||
| agg_skips = [] | ||
| if system_out and any( | ||
| k in system_out for k in ("passes:", "failures:", "skips:") |
There was a problem hiding this comment.
Minor: this is a substring match, so system-out text that happens to contain "failures:" in a log message (e.g. "checking for failures: none found") would trigger the YAML parser on non-YAML content. Probably fine in practice, but a line-start-anchored regex like re.search(r"^(?:passes|failures|skips):", system_out, re.MULTILINE) would be more precise if you wanted to tighten it up.
| ### Example 5: Pipe from GCS | ||
|
|
||
| ```bash | ||
| gcloud storage cat gs://test-platform-results/logs/{job_name}/{build_id}/artifacts/{target}/openshift-e2e-test/artifacts/junit/junit_e2e_*.xml.gz | \ |
There was a problem hiding this comment.
Heads up — if this glob matches multiple files, gcloud storage cat will concatenate them. The decompressed result would be multiple XML documents back-to-back, which isn't valid XML. ET.parse() would raise a ParseError that gets caught, and you'd silently get empty results. This example should probably either note that it expects a single file match, or use --stdin only with a specific file path.
| - `system_out`: Raw `<system-out>` content | ||
| - `aggregated`: Only present for aggregated JUnit — contains per-run pass/fail/skip data with Prow URLs | ||
|
|
||
| Fields with empty values are omitted from JSON output to reduce noise. |
There was a problem hiding this comment.
This isn't quite accurate — _to_json() always includes name, status, suite_name, classname, time_seconds, lifecycle, and source_file even when empty/zero. Only the optional fields (failure_message, failure_text, error_message, error_text, skipped_message, system_out, aggregated) are conditionally omitted. Worth clarifying so consumers know what to expect.
|
|
||
| Each test result in the JSON output includes an `aggregated` field with `passes`, `failures`, | ||
| and `skips` lists. Each entry has: | ||
| - `jobRunID`: The build ID of the underlying job run |
There was a problem hiding this comment.
The YAML parser preserves field names as-is from the source without normalizing casing. I notice the formatter code in parse_junit.py (around line 392) defensively checks both humanURL and humanUrl, and both jobRunID and jobrunid — which suggests the upstream aggregator's casing isn't guaranteed. Might be worth either normalizing in the parser, or adding a note here that casing depends on the upstream aggregator.
openshift-ci
Bot
added
the
needs-rebase
The payload agent reinvents JUnit XML parsing inline ~500+ times across sessions. This adds a reusable parse-junit skill with a stdlib-only Python script that handles standard JUnit, aggregated JUnit (with system-out YAML for per-run results), gzip-compressed files, and stdin. Key features: - Extracts test metadata: suite/binary source, lifecycle (informing vs blocking), failure messages, and output text - Informing tests are auto-detected and clearly labeled — they don't cause job failures on their own but can impact the cluster - Parses aggregated JUnit system-out YAML to extract per-run job URLs - Supports filtering by name pattern, status, and lifecycle - Four output formats: json, summary, failures, names Also updates prow-job-analyze-test-failure to use parse-junit instead of inline XML parsing. Bumps ci plugin version to 0.0.38. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix status precedence: use elif chain (error > failed > skipped) so a testcase with both <failure> and <skipped> isn't silently reported as skipped - Handle corrupt gzip: catch OSError in parse_junit_xml() and wrap stdin gzip.decompress() in try/except - Add friendly error for invalid --filter regex patterns - Use line-anchored regex for YAML detection in system-out to avoid false positives from log messages containing "failures:" - Return exit code 1 when files fail to parse - Clarify always-present vs optional fields in JSON output docs - Fix SKILL.md: add code block language specifiers, note single file requirement for gcloud storage cat with --stdin - Note YAML field casing variability in prow-job-analyze docs - Bump CI plugin version to 0.0.43 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: stbenjam The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@plugins/ci/skills/parse-junit/parse_junit.py`:
- Around line 543-544: The code sets had_errors when "not results" which treats
a valid empty JUnit file (empty results list) as a parse error; change the
condition to only mark had_errors when the parser actually failed (e.g., results
is None or a separate parse_error flag) rather than any falsy value — update the
check around results and filepath so it does something like "if results is None
and Path(filepath).exists(): had_errors = True" (use the existing results,
filepath and had_errors symbols to locate and modify the logic).
- Around line 124-130: The parse_junit_xml function currently uses
xml.etree.ElementTree.parse which is vulnerable to malicious DTD/entity attacks;
update the function to use defusedxml.ElementTree.parse (or
defusedxml.ElementTree.fromstring if you first read bytes) instead and add the
appropriate import (defusedxml.ElementTree as ET) so that parsing of untrusted
JUnit input is safe; ensure error handling still catches defusedxml's
ParseError/OSError equivalents and that source_name remains in the stderr
message.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 559078ed-c718-40b5-b8ea-8e6d21a248e3
📒 Files selected for processing (6)
.claude-plugin/marketplace.jsondocs/index.htmlplugins/ci/.claude-plugin/plugin.jsonplugins/ci/skills/parse-junit/SKILL.mdplugins/ci/skills/parse-junit/parse_junit.pyplugins/ci/skills/prow-job-analyze-test-failure/SKILL.md
✅ Files skipped from review due to trivial changes (3)
- .claude-plugin/marketplace.json
- plugins/ci/skills/prow-job-analyze-test-failure/SKILL.md
- plugins/ci/skills/parse-junit/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/ci/.claude-plugin/plugin.json
| def parse_junit_xml(source, source_name: str = "<stdin>") -> list: | ||
| """Parse a JUnit XML file or stream, returning a list of TestResult.""" | ||
| try: | ||
| tree = ET.parse(source) | ||
| except (ET.ParseError, OSError) as e: | ||
| print(f"Error: Failed to parse XML from {source_name}: {e}", file=sys.stderr) | ||
| return [] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Python 3's xml.etree.ElementTree, what are the known security risks when parsing untrusted XML (e.g., entity expansion/XXE), and what mitigations are recommended in production CLIs?
💡 Result:
When parsing untrusted XML data in Python, xml.etree.ElementTree has historically been considered insecure due to its vulnerability to denial-of-service (DoS) attacks, specifically entity expansion attacks like the Billion Laughs attack and quadratic blowup [1][2]. These attacks exploit the parser's handling of XML entities to consume excessive CPU or memory, potentially crashing the application [1][3]. However, the security landscape has evolved. Python's xml.etree.ElementTree uses the Expat parser library internally. Modern versions of Expat (specifically 2.6.0 and later) have addressed many of these underlying issues, leading to discussions within the Python community about whether historical security warnings remain necessary [4][5]. For production CLI applications where security is a priority, the following mitigations are recommended: 1. Use defusedxml: The most common, widely accepted mitigation is to use the defusedxml library as a drop-in replacement for parsing XML from untrusted sources [6][7][8]. The library is designed to disable dangerous features—such as DTD processing and entity expansion—that are typically exploited in these attacks [6][9]. 2. Keep Dependencies Updated: Since the security of XML parsing in Python relies heavily on the underlying Expat library, ensure that your environment (including your Python interpreter and system libraries) is kept up to date to receive the latest security patches for Expat [4][5]. 3. Configuration (if using other libraries): If you choose to use libraries like lxml, which are often more feature-rich than ElementTree, you must explicitly configure them to be secure. For lxml, this involves disabling entity resolution and network access during parsing (e.g., using resolve_entities=False, no_network=True, and load_dtd=False) [7][10]. While defusedxml is still frequently recommended, some members of the community have noted that it has seen less frequent updates in recent years [11]. Regardless of the tool chosen, the fundamental best practice remains: never trust XML input from external or untrusted sources and, if possible, consider safer data interchange formats like JSON, which are not susceptible to this class of XML-specific vulnerabilities [7].
Citations:
- 1: https://stackoverflow.com/questions/47104413/why-is-xml-etree-elementtree-considered-insecure
- 2: https://deepsource.com/directory/python/issues/BAN-B314
- 3: https://www.jbs.dev/resources/resource-center/blog/preventing-xml-external-entity-attacks-python/
- 4: Reconsider XML Security warnings / obsolete vulnerabilities python/cpython#127502
- 5: using
xml.etree.ElementTree.fromstringshouldn't raise errors in python versions 3.11 and up PyCQA/bandit#1344 - 6: https://runebook.dev/en/docs/python/library/xml/xml-vulnerabilities
- 7: https://aquilax.ai/blog/xxe-injection-xml-external-entity
- 8: https://www.linkedin.com/pulse/protecting-your-application-from-xml-based-attacks-importance-koshy
- 9: https://github.com/tiran/defusedxml/
- 10: Use of defusedxml instead of xml to avoid known vulnerabilities openmc-dev/openmc#1775
- 11: https://discuss.python.org/t/status-of-defusedxml-and-recommendation-in-docs/34762
Harden XML parsing for untrusted inputs at plugins/ci/skills/parse-junit/parse_junit.py (Line ~127).
xml.etree.ElementTree.parse() can be abused via DTD/entity processing (notably entity-expansion/“Billion Laughs” style DoS). Use defusedxml.ElementTree.parse() (or otherwise disable DTD/entity expansion) before parsing externally sourced JUnit.
🧰 Tools
🪛 Ruff (0.15.14)
[error] 127-127: Using xml to parse untrusted data is known to be vulnerable to XML attacks; use defusedxml equivalents
(S314)
🤖 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 `@plugins/ci/skills/parse-junit/parse_junit.py` around lines 124 - 130, The
parse_junit_xml function currently uses xml.etree.ElementTree.parse which is
vulnerable to malicious DTD/entity attacks; update the function to use
defusedxml.ElementTree.parse (or defusedxml.ElementTree.fromstring if you first
read bytes) instead and add the appropriate import (defusedxml.ElementTree as
ET) so that parsing of untrusted JUnit input is safe; ensure error handling
still catches defusedxml's ParseError/OSError equivalents and that source_name
remains in the stderr message.
| if not results and Path(filepath).exists(): | ||
| had_errors = True |
There was a problem hiding this comment.
Don’t treat “empty but valid” files as parse errors.
At Line 543, not results marks had_errors=True for any existing file with zero testcases, so valid empty JUnit can incorrectly force exit code 1.
Proposed fix
-def parse_junit_xml(source, source_name: str = "<stdin>") -> list:
+def parse_junit_xml(source, source_name: str = "<stdin>"):
"""Parse a JUnit XML file or stream, returning a list of TestResult."""
try:
tree = ET.parse(source)
except (ET.ParseError, OSError) as e:
print(f"Error: Failed to parse XML from {source_name}: {e}", file=sys.stderr)
- return []
+ return [], True
@@
- return results
+ return results, False
@@
-def parse_file(filepath: str) -> list:
+def parse_file(filepath: str):
@@
if not p.exists():
print(f"Error: File not found: {filepath}", file=sys.stderr)
- return []
+ return [], True
@@
- return parse_junit_xml(f, source_name=filepath)
+ return parse_junit_xml(f, source_name=filepath)
- return parse_junit_xml(filepath, source_name=filepath)
+ return parse_junit_xml(filepath, source_name=filepath)
@@
- all_results.extend(parse_junit_xml(io.StringIO(text), source_name="<stdin>"))
+ stdin_results, stdin_error = parse_junit_xml(io.StringIO(text), source_name="<stdin>")
+ had_errors = had_errors or stdin_error
+ all_results.extend(stdin_results)
@@
- results = parse_file(filepath)
- if not results and Path(filepath).exists():
- had_errors = True
+ results, parse_error = parse_file(filepath)
+ had_errors = had_errors or parse_error
all_results.extend(results)🤖 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 `@plugins/ci/skills/parse-junit/parse_junit.py` around lines 543 - 544, The
code sets had_errors when "not results" which treats a valid empty JUnit file
(empty results list) as a parse error; change the condition to only mark
had_errors when the parser actually failed (e.g., results is None or a separate
parse_error flag) rather than any falsy value — update the check around results
and filepath so it does something like "if results is None and
Path(filepath).exists(): had_errors = True" (use the existing results, filepath
and had_errors symbols to locate and modify the logic).
|
I am going to take a different approach |
2 participants
Summary
parse-junitskill to the CI plugin with a stdlib-only Python script that parses JUnit XML files from OpenShift CI jobsprow-job-analyze-test-failureto useparse-junitinstead of inline XML parsing instructionsTest plan
--helpoutputmake update— docs and marketplace syncedmake lint— passes🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores