Skip to content

[TRTLLMINF-45][infra] Upload rendered HTML failure analysis#13526

Merged
dpitman-nvda merged 4 commits intoNVIDIA:mainfrom
dpitman-nvda:feat/swiftstack-html-render
Apr 28, 2026
Merged

[TRTLLMINF-45][infra] Upload rendered HTML failure analysis#13526
dpitman-nvda merged 4 commits intoNVIDIA:mainfrom
dpitman-nvda:feat/swiftstack-html-render

Conversation

@dpitman-nvda
Copy link
Copy Markdown
Collaborator

@dpitman-nvda dpitman-nvda commented Apr 27, 2026

Summary by CodeRabbit

  • New Features
    • Enhanced failure analysis reports with improved formatting and readability.
    • Failure analysis links are now directly embedded in pull request comments for easier access and faster issue diagnosis.

Description

Replace the plain-text failure_analysis.txt upload with a self-rendering failure_analysis.html: a small HTML page that embeds the analysis markdown JSON-encoded and parses it client-side via marked.js from cdn.jsdelivr.net. Anyone with the link gets a rendered view in their browser, no extra tooling. Upload uses --content-type text/html so SwiftStack serves it correctly.

The Swift-style URL is also written into currentBuild.description as an HTML anchor. The upstream PR_Github wrapper (in trtllm-github-ci) reads this via the build handle and includes the link in the GitHub PR comment when the L0 pipeline finishes unsuccessfully — so authors don't have to dig through Jenkins to find the analysis.

Test Coverage

N/A, CI change

PR Checklist

Please review the following before submitting your PR:

  • PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.

  • PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.

  • Test cases are provided for new code paths (see test instructions)

  • Any new dependencies have been scanned for license and vulnerabilities

  • CODEOWNERS updated if ownership changes

  • Documentation updated as needed

  • Update tava architecture diagram if there is a significant design change in PR.

  • The reviewers assigned automatically/manually are appropriate for the PR.

  • Please check this after reviewing the above items as appropriate for this PR.

GitHub Bot Help

To see a list of available CI bot commands, please comment /bot help.

…ce URL

Replace the plain-text failure_analysis.txt upload with a self-rendering
failure_analysis.html: a small HTML page that embeds the analysis
markdown JSON-encoded and parses it client-side via marked.js from
cdn.jsdelivr.net. Anyone with the link gets a rendered view in their
browser, no extra tooling. Upload uses --content-type text/html so
SwiftStack serves it correctly.

The Swift-style URL is also written into currentBuild.description as an
HTML anchor. The upstream PR_Github wrapper (in trtllm-github-ci) reads
this via the build handle and includes the link in the GitHub PR
comment when the L0 pipeline finishes unsuccessfully — so authors don't
have to dig through Jenkins to find the analysis.

Signed-off-by: Derek Pitman <dpitman@nvidia.com>
@dpitman-nvda
Copy link
Copy Markdown
Collaborator Author

/bot run

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 27, 2026

📝 Walkthrough

Walkthrough

The pipeline failure analysis artifact generation changed from plain-text format to HTML format. The artifact is now generated as failure_analysis.html with JSON-encoded Markdown content, client-side rendering via marked.js, proper content-type header, and an HTML link appended to the build description.

Changes

Cohort / File(s) Summary
Pipeline Failure Analysis Artifact Format
jenkins/L0_MergeRequest.groovy
Modified artifact generation to produce HTML instead of plain text; updated S3 object key from failure_analysis.txt to failure_analysis.html; added --content-type text/html to S3 upload command; integrated marked.js for client-side Markdown rendering with JSON-encoded content; appended HTML link to currentBuild.description for downstream PR comment inclusion.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: uploading a rendered HTML failure analysis instead of plain text.
Description check ✅ Passed The PR description covers the key aspects: what changed (plain-text to HTML), why (browser rendering, better UX), how it works (client-side marked.js), and integration details (currentBuild.description update).
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@jenkins/L0_MergeRequest.groovy`:
- Around line 1433-1435: The S3 destination URI built in the concatenated shell
command (the aws s3 cp call that constructs "s3://${bucket}/${key}") must be
quoted to avoid shell-splitting; update the string concatenation so the command
emits the destination as a quoted string (e.g. "'s3://${bucket}/${key}'" or wrap
with single quotes) when constructing the aws s3 cp invocation in the Jenkins
pipeline code that builds the command.
- Around line 1407-1413: The current code directly injects untrusted markdown
via marked.parse(...) into element.innerHTML and embeds raw JSON from
groovy.json.JsonOutput.toJson(analysis) inside an inline <script>, enabling XSS;
fix by (1) including pinned, integrity-checked CDN references for marked and
DOMPurify, (2) calling DOMPurify.sanitize(marked.parse(...)) and assigning that
sanitized string to document.getElementById('md').innerHTML instead of the raw
parse output, and (3) escaping '<' in the JSON before embedding (e.g. replace
"<" with "\\u003c" in the string returned by JsonOutput.toJson(analysis)) or,
better, avoid inline script-inserted JSON and place the escaped JSON in a data-*
attribute or use textContent to safely parse—update occurrences of marked.parse,
innerHTML assignment, and JsonOutput.toJson(analysis) accordingly.
🪄 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: 950b43ed-17fc-4e4e-9efd-5f3c062d4e1d

📥 Commits

Reviewing files that changed from the base of the PR and between bf96323 and 7c10b9b.

📒 Files selected for processing (1)
  • jenkins/L0_MergeRequest.groovy

Comment thread jenkins/L0_MergeRequest.groovy Outdated
Comment thread jenkins/L0_MergeRequest.groovy
@tensorrt-cicd
Copy link
Copy Markdown
Collaborator

PR_Github #45783 [ run ] triggered by Bot. Commit: 7c10b9b Link to invocation

The CI agent's analysis text is derived from build logs, which can
include attacker-controlled PR content, so it must be treated as
untrusted at render time. Three hardenings on the failure_analysis.html
upload:

1. marked and DOMPurify are pinned to specific versions and loaded with
   subresource integrity hashes so a CDN compromise can't inject code.
2. The analysis JSON is embedded in a `<script type="application/json">`
   data block and read via textContent + JSON.parse. Every `<` in the
   JSON is rewritten to its JSON unicode escape so a payload cannot
   smuggle a closing script tag and break out of the data block.
3. marked's parsed HTML is run through DOMPurify before innerHTML
   assignment, stripping event-handler attributes and other XSS vectors
   that marked itself would otherwise allow through by default.

Also single-quote the s3:// destination URI in the aws s3 cp command
so future shell-metacharacter values in JOB_NAME or BUILD_NUMBER can't
cause word-splitting. Defensive — current values are already safe.

Signed-off-by: Derek Pitman <dpitman@nvidia.com>
@dpitman-nvda
Copy link
Copy Markdown
Collaborator Author

@coderabbitai Have the issues been addressed?

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 27, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get PR review comments (both resolved and unresolved)
gh pr view 13526 --repo NVIDIA/TensorRT-LLM --json reviews,reviewDecision,state

echo "---REVIEW_COMMENTS---"
gh api repos/NVIDIA/TensorRT-LLM/pulls/13526/comments --paginate | jq '[.[] | {id, body, path, line, user: .user.login, created_at}]'

echo "---PR_REVIEW_THREADS---"
gh api graphql -f query='
{
  repository(owner: "NVIDIA", name: "TensorRT-LLM") {
    pullRequest(number: 13526) {
      reviewThreads(first: 50) {
        nodes {
          id
          isResolved
          isOutdated
          comments(first: 5) {
            nodes {
              author { login }
              body
              path
              line
            }
          }
        }
      }
    }
  }
}'

Length of output: 21095


🏁 Script executed:

#!/bin/bash
# Check the current state of the relevant lines in the PR branch
git diff origin/main...HEAD -- jenkins/L0_MergeRequest.groovy | head -300

echo "=== Current file around HTML generation and S3 upload ==="
# Find the lines around the htmlDoc and S3 upload
grep -n "htmlDoc\|marked\|DOMPurify\|dompurify\|JsonOutput\|u003c\|s3 cp\|failure_analysis\|analysisJson\|innerHTML\|bucket\|content-type" jenkins/L0_MergeRequest.groovy | head -60

Length of output: 7292


@dpitman-nvda Yes, both issues raised in the review have been thoroughly addressed in the latest commit. Here's a summary:


✅ Issue 1 — XSS via innerHTML / unescaped JSON (Critical)

All three hardening steps were applied:

  1. < escaping in JSON (Line 1415): JsonOutput.toJson(analysis).replace("<", "\\u003c") prevents </script> injection.
  2. Safe JSON embedding (Lines 1426–1428): The analysis is now embedded in a <script type="application/json"> data block and read via textContent + JSON.parse — never inlined into executable JS.
  3. DOMPurify sanitization (Line 1428): DOMPurify.sanitize(marked.parse(src)) strips any XSS vectors from the rendered markdown before assignment to innerHTML.
  4. Pinned versions with SRI (Lines 1419–1420): Both marked@14.1.4 and dompurify@3.2.4 include integrity and crossorigin attributes.

✅ Issue 2 — Unquoted S3 destination URI (Minor)

The S3 URI is now wrapped in single quotes (Line 1451):

" 's3://${bucket}/${key}' --endpoint-url https://pbss.s8k.io" +

Would you like me to resolve both of these review comments?

@tensorrt-cicd
Copy link
Copy Markdown
Collaborator

PR_Github #45783 [ run ] completed with state FAILURE. Commit: 7c10b9b
/LLM/main/L0_MergeRequest_PR pipeline #35973 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

Link to invocation

CI failure-analysis text frequently contains literal tildes (~/path,
~50ms, regex anchors, ...) that are not intended as markup. marked's
default GFM strikethrough tokenizer wraps these in <del>, garbling the
output. Override the del tokenizer to never match while leaving other
GFM extensions enabled.

Signed-off-by: Derek Pitman <dpitman@nvidia.com>
@dpitman-nvda
Copy link
Copy Markdown
Collaborator Author

dpitman-nvda commented Apr 27, 2026

Well if nothing else this was a great test run of the new workflow (Markdown version of the report on SwiftStack), but I had to push some changes (and I see a small formatting issue with tildes and strikethrough, just updated that).

Will retry after merging main.

@dpitman-nvda
Copy link
Copy Markdown
Collaborator Author

/bot run

@tensorrt-cicd
Copy link
Copy Markdown
Collaborator

PR_Github #45800 [ run ] triggered by Bot. Commit: 4580371 Link to invocation

@tensorrt-cicd
Copy link
Copy Markdown
Collaborator

PR_Github #45800 [ run ] completed with state SUCCESS. Commit: 4580371
/LLM/main/L0_MergeRequest_PR pipeline #35990 completed with status: 'SUCCESS'

CI Report

Link to invocation

@dpitman-nvda dpitman-nvda merged commit da35deb into NVIDIA:main Apr 28, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants