feat(reporters): overhaul GitHub Actions step summary

[thomhurst]

Summary

Comprehensive overhaul of the GitHub Actions step summary output for richer, more actionable CI feedback.

Changes

• Status emoji header — title line shows ✅ or ❌ with optional (View Report) link when the HTML artifact was uploaded
• Compact inline summary — replaces the old markdown table with a single line showing total tests, duration, and pass rate, plus a segmented breakdown (passed · failed · skipped · timed out · cancelled)
• Flaky test detection — tests that passed after retry are highlighted in a blockquote with attempt count and duration
• Skipped test grouping — collapsible section groups skipped tests by reason with counts
• Quick diagnosis — one-liner showing top exception types and the most-affected class
• Inline top failures — up to 5 failures shown as collapsible <details> blocks with error messages and source links (links to the exact file/line on GitHub)
• Source links — uses GITHUB_WORKSPACE for reliable path stripping, with repo-name fallback
• Artifact upload tip — when HTML reporter runs but artifact upload isn't configured, shows a tip linking to setup docs
• Horizontal rule separators — --- between projects for multi-TFM runs
• Removed standalone emoji links — old "View HTML Report" links removed from HtmlReporter; the link is now in the GitHubReporter title
• Human-readable durations — FormatDuration helper (e.g., 1.2s, 3m 12s, 2h 5m)
• Env var caching — GITHUB_REPOSITORY, GITHUB_SHA, GITHUB_WORKSPACE read once per run, not per test

Architecture

• HtmlReporter pushes ArtifactUrl and ShowArtifactUploadTip to GitHubReporter via simple properties (GitHubReporter has no knowledge of HtmlReporter)
• Wiring done in TestApplicationBuilderExtensions.AddTUnit() via htmlReporter.SetGitHubReporter(githubReporter)
• MTP lifecycle guarantees OnTestSessionFinishingAsync (HtmlReporter) runs before AfterRunAsync (GitHubReporter)

Files changed

• TUnit.Engine/Reporters/GitHubReporter.cs — main overhaul
• TUnit.Engine/Reporters/Html/HtmlReporter.cs — removed standalone links, added push to GitHubReporter
• TUnit.Engine/Extensions/TestApplicationBuilderExtensions.cs — wiring

Test plan

• Verify summary renders with all tests passing (✅ emoji, no failures section)
• Verify summary renders with failures (❌ emoji, inline failures with source links, quick diagnosis)
• Verify flaky test detection (retry a test, confirm blockquote appears)
• Verify skipped tests grouped by reason in collapsible section
• Verify (View Report) link appears when artifact upload is configured
• Verify artifact upload tip appears when HTML reporter runs without runtime token
• Verify multi-TFM runs show --- separators between projects
• Verify source links point to correct file/line on GitHub

[codacy-production]

Not up to standards ⛔

🔴 Issues 3 minor

Alerts:
⚠ 3 issues (≤ 0 issues of at least minor severity)

Results:
3 new issues

Category	Results
CodeStyle	3 minor

View in Codacy

🟢 Metrics 49 complexity

Metric	Results
Complexity	49

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[claude]

Code Review

Overall, the feature is a nice UX improvement — adding the (View Report) link directly to the summary title is cleaner than hunting for it elsewhere. The dependency direction choice (HtmlReporter → GitHubReporter, not the reverse) is also sensible. A few concerns worth addressing:

---

🐛 Tip footer shows when HTML reporting is intentionally disabled

This is the main issue. The tip is gated solely on ArtifactUrl being null/empty, but that condition is true in multiple distinct situations:

1. TUNIT_DISABLE_HTML_REPORTER=true — MTP never calls HtmlReporter.OnTestSessionFinishingAsync, so ArtifactUrl is never set. The tip will tell users to "enable artifact upload" when they've intentionally disabled HTML reporting entirely.
2. No tests ran (_updates.Count == 0) — TryGitHubIntegrationAsync is never reached, same result.
3. Artifact upload was configured but failed (exception caught in TryGitHubIntegrationAsync) — the tip suggests "set it up" when the user already has it set up.

Only the "HTML reporter ran but runtime token is not available" case should show the tip.

Suggested fix: Make the tip opt-in via an explicit flag rather than inferring it from ArtifactUrl == null:

// GitHubReporter.cs
internal string? ArtifactUrl { get; set; }
internal bool ShowHtmlArtifactTip { get; set; }  // explicitly set by HtmlReporter

// GitHubReporter.AfterRunAsync — change the condition:
if (ShowHtmlArtifactTip)  // was: string.IsNullOrEmpty(ArtifactUrl)
{
    stringBuilder.AppendLine();
    stringBuilder.AppendLine("> **Tip:** ...");
}

// HtmlReporter.TryGitHubIntegrationAsync — signal the tip only when appropriate:
if (!hasRuntimeToken)
{
    if (_githubReporter is not null)
        _githubReporter.ShowHtmlArtifactTip = true;  // <-- explicit signal
    Console.WriteLine("Tip: To enable automatic HTML report artifact upload...");
}

This way the tip is only shown when HtmlReporter ran, reached GitHub integration, and found no runtime token — which is precisely when the tip is actionable.

---

Minor: The ArtifactUrl backdoor mutation

The internal string? ArtifactUrl { get; set; } exposed as a settable property is an implicit contract. The comment documents the MTP ordering assumption well, but the property is a public mutation point in disguise. A minor improvement would be to make it init-only or add a method to make the cross-component communication more intentional:

internal void SetArtifactUrl(string url) => ArtifactUrl = url;

This doesn't change semantics but signals "this is an intentional write from a peer component" vs. an accidental external mutation. Minor — take or leave it.

---

The temporal ordering is well-documented ✅

The comment // Set by HtmlReporter during OnTestSessionFinishingAsync, which MTP invokes before AfterRunAsync is exactly the right thing to say here. This is easy to miss and could bite someone during a refactor, so the comment earns its keep.

---

Summary: The feature concept is solid. The main thing to fix before merging is the false-positive tip scenario when HTML reporting is explicitly disabled or not running. The explicit ShowHtmlArtifactTip flag approach above resolves all three false-positive cases cleanly.

[claude]

Follow-up Review

The main concern from the previous review has been addressed cleanly.

✅ False-positive tip — fixed

The second commit () introduced the explicit flag exactly as suggested. The tip is now only set to true inside the !hasRuntimeToken branch of TryGitHubIntegrationAsync, so it fires only when:

• We're running on GitHub Actions
• The HTML reporter ran and attempted integration
• No runtime token was found (i.e., artifact upload is not configured)

All three false-positive scenarios from the previous review are now correctly avoided.

Minor note (not blocking)

The internal string? ArtifactUrl { get; set; } / internal bool ShowArtifactUploadTip { get; set; } are still plain settable properties. The previous review flagged this as optional ("take or leave it"), and leaving it as-is is fine given that both are internal and the ordering is documented in comments.

Overall

The feature is clean, the dependency direction (HtmlReporter → GitHubReporter) is correct, and the main correctness issue is resolved. LGTM.

[claude]

Follow-up Review

The main concern from the previous review has been addressed cleanly.

Fixed: False-positive tip

The second commit directly introduced the explicit ShowArtifactUploadTip flag exactly as suggested. The tip is now only set to true inside the !hasRuntimeToken branch of TryGitHubIntegrationAsync, so it fires only when:

• We are running on GitHub Actions
• The HTML reporter ran and attempted integration
• No runtime token was found (artifact upload is not configured)

All three false-positive scenarios from the previous review (HTML reporting disabled, no tests ran, upload configured but failed) are now correctly avoided.

Minor note (not blocking)

The internal string? ArtifactUrl { get; set; } and internal bool ShowArtifactUploadTip { get; set; } remain plain settable properties. The previous review flagged this as optional, and leaving it as-is is fine since both are internal and the MTP ordering assumption is documented in a comment.

Overall

The feature is clean, the dependency direction (HtmlReporter sets state on GitHubReporter, not the reverse) is correct, and the main correctness issue is resolved. LGTM.

[claude]

Follow-up Review (final pass)

All concerns from previous reviews have been addressed. This is a summary review of the complete PR.

What the PR does

• Adds a (View Report) link to the GitHub summary table title when the HTML artifact upload succeeds.
• Shows a tip footer when artifact upload is not configured (instead of a vague null-based condition).
• Removes the now-redundant standalone View HTML Report step summary lines from HtmlReporter.

Previous review concerns — all resolved

False-positive tip (main issue, commit 2): The explicit ShowArtifactUploadTip flag is set only inside the !hasRuntimeToken branch of TryGitHubIntegrationAsync. This fires exclusively when the HTML reporter ran, reached GitHub integration, and found no runtime token — which is the only case where the tip is actionable. The three false-positive scenarios (HTML reporting disabled, no tests ran, upload configured but failed) are all correctly avoided.

Settable internal properties (minor, commit 3): Remains as plain internal settable properties. This is fine given they are internal and the MTP ordering assumption is documented in a comment. The refactor in commit 3 actually reduced surface area by removing the old summary-writing code from HtmlReporter entirely, which simplifies the overall design.

Code quality observations (no blocking issues)

• The temporal ordering dependency (OnTestSessionFinishingAsync before AfterRunAsync) is documented with a clear comment on the properties. Good.
• The dependency direction (HtmlReporter → GitHubReporter, not the reverse) is correctly maintained: GitHubReporter has no import or knowledge of HtmlReporter.
• SetGitHubReporter is called unconditionally before registration. If GitHubReporter.IsEnabledAsync() returns false (e.g., not in a GitHub Actions environment), the _githubReporter field in HtmlReporter is set but never written to from TryGitHubIntegrationAsync because the early-return guard if (Environment.GetEnvironmentVariable(EnvironmentConstants.GitHubActions) is not "true") return; fires first. So the coupling is harmless in non-GitHub environments.
• The URL format https://github.com/{repo}/actions/runs/{runId}/artifacts/{artifactId} is consistent with the old code and standard GitHub artifact URLs.

Overall

The PR is clean, correct, and the design is well-reasoned. LGTM.

[claude]

Follow-up Review (latest commit 2e5b8c1)

One new commit landed after the last review, touching HtmlReporter.TryGitHubIntegrationAsync.

What changed

The _githubReporter interaction was restructured: rather than two separate null-checks (one inside the !hasRuntimeToken branch and one outside), there is now a single outer if (_githubReporter is not null) block with the !hasRuntimeToken / else if artifactId != null branching inside it.

Semantics

Logically identical to the previous version — the ShowArtifactUploadTip = true assignment still only fires when !hasRuntimeToken, and ArtifactUrl is still only set when upload succeeded. No regression.

Code quality

The restructuring is a minor improvement: one null check instead of two, and the full GitHubReporter state mutation is co-located in one place. Easier to read.

Overall

All concerns from previous reviews remain addressed. The PR is clean, correct, and ready to merge. LGTM.

[claude]

Follow-up Review (commit cc14a4e)

One new commit landed after the last review, addressing the bugs flagged previously.

Previous issues — all fixed ✅

FormatDuration hours truncation (was rounding): Fixed — (int)d.Value.TotalHours correctly truncates instead of :F0 which was rounding up at 30-minute boundaries.

Source link path stripping (double repo-name bug): Fixed — GITHUB_WORKSPACE is now the primary mechanism. filePath.StartsWith(workspace) strips the exact checkout directory prefix, so /home/runner/work/TUnit/TUnit/TUnit.Engine/... correctly becomes TUnit.Engine/.... The fallback (repo-name search) is only hit when GITHUB_WORKSPACE is absent, which in practice means non-GH-Actions environments — acceptable as best-effort.

totalNonPassing count: Fixed — now uses remainingFailures.Count + otherMessages.Count, matching exactly what's in the collapsible table (remaining failures after inline 5, plus non-failed non-passed).

Env var caching: Fixed — GITHUB_REPOSITORY, GITHUB_SHA, GITHUB_WORKSPACE are all read once above the loop.

---

🐛 New concern: totalDuration shows CPU time, not wall-clock time

// GitHubReporter.cs line 136-144
var totalDuration = TimeSpan.Zero;
foreach (var msg in last.Values)
{
    var timing = msg.TestNode.Properties.AsEnumerable().OfType<TimingProperty>().FirstOrDefault();
    if (timing is not null)
        totalDuration += timing.GlobalTiming.Duration;
}
// ...
stringBuilder.AppendLine($"**{totalCount} tests** completed in **{FormatDuration(totalDuration)}** ...");

totalDuration is the sum of individual test durations. Because TUnit runs tests in parallel, this is cumulative CPU time, not wall-clock elapsed time. A suite of 200 tests each taking 500ms, running with 16-way parallelism, would display "100 tests completed in 1m 40s" when the actual run was ~6 seconds. The phrasing "completed in" strongly implies elapsed time.

Better approach: Capture wall-clock start time in BeforeRunAsync and compute elapsed time in AfterRunAsync:

private long _runStartTimestamp;

public Task BeforeRunAsync(CancellationToken cancellationToken)
{
    _runStartTimestamp = Stopwatch.GetTimestamp();
    return Task.CompletedTask;
}

// In AfterRunAsync:
var elapsed = Stopwatch.GetElapsedTime(_runStartTimestamp);
stringBuilder.AppendLine($"**{totalCount} tests** completed in **{FormatDuration(elapsed)}** ...");

The cumulative test duration is still valuable (it represents total compute cost), but could be surfaced differently, e.g. as a tooltip or secondary line: Wall clock: 6s · Total CPU: 1m 40s.

---

Minor: redundant null-bang operators in GetSourceLink

if (!string.IsNullOrEmpty(workspace) && filePath.StartsWith(workspace!, StringComparison.OrdinalIgnoreCase))
{
    filePath = filePath[workspace!.Length..].TrimStart('/');
}

The workspace! null-bang is unnecessary — !string.IsNullOrEmpty(workspace) already guarantees non-null, and the compiler knows it. Not a bug, just noise.

---

Minor (carry-forward): hardcoded https://github.com/ URL

return $"[{fileName}:{line}](https://github.com/{repo}/blob/{sha}/{filePath}#L{line})";

GitHub Enterprise Server users get broken links. GITHUB_SERVER_URL env var provides the correct base URL. Low-risk to fix:

var serverUrl = Environment.GetEnvironmentVariable("GITHUB_SERVER_URL") ?? "https://github.com";
return $"[{fileName}:{line}]({serverUrl}/{repo}/blob/{sha}/{filePath}#L{line})";

(This was flagged in the previous review as a low-risk improvement, not a blocker.)

---

Overall

The critical bugs from the prior review are all resolved. The totalDuration wall-clock vs. CPU-time issue is worth addressing before merging — the "completed in X" phrasing will produce confusing output for any project using TUnit's parallelism. The other items are minor.

[claude]

Follow-up Review (commit 3c97396)

One new commit landed after the last review, addressing the GHES source link concern.

✅ GHES source link fix — resolved

The latest commit wires up GITHUB_SERVER_URL (with "https://github.com" fallback) and passes it through to GetSourceLink as the serverUrl parameter. The serverUrl.TrimEnd('/') call also prevents double-slash when the env var has a trailing slash. Both the GHES compatibility concern and the double-slash edge case are handled correctly.

---

🐛 Outstanding: totalDuration is CPU time, not wall-clock time (carry-forward)

This was raised in the previous review and is still unaddressed:

var totalDuration = TimeSpan.Zero;
foreach (var msg in last.Values)
{
    var timing = msg.TestNode.Properties.AsEnumerable().OfType<TimingProperty>().FirstOrDefault();
    if (timing is not null)
        totalDuration += timing.GlobalTiming.Duration;
}
// ...
stringBuilder.AppendLine($"**{totalCount} tests** completed in **{FormatDuration(totalDuration)}** ...");

totalDuration is the sum of individual test durations, not elapsed wall-clock time. For a parallel test run (which is TUnit's default), this number will be far larger than actual elapsed time — a 200-test suite each taking 500ms, running 16-way parallel, would show "completed in 1m 40s" when the real run took ~6 seconds. The "completed in" phrasing implies elapsed time, making this actively misleading.

Recommended fix — capture wall-clock time using Stopwatch:

// Add field:
private long _runStartTimestamp;

// In BeforeRunAsync (currently empty):
public Task BeforeRunAsync(CancellationToken cancellationToken)
{
    _runStartTimestamp = Stopwatch.GetTimestamp();
    return Task.CompletedTask;
}

// In AfterRunAsync, replace totalDuration accumulation:
var elapsed = Stopwatch.GetElapsedTime(_runStartTimestamp);
// then use elapsed instead of totalDuration

If the per-test cumulative duration is still wanted (it represents compute cost, which is useful), it could be labelled differently to avoid ambiguity — e.g., "CPU time" or surfaced in a secondary position.

---

Minor: redundant null-bang operators in GetSourceLink

if (!string.IsNullOrEmpty(workspace) && filePath.StartsWith(workspace!, StringComparison.OrdinalIgnoreCase))
{
    filePath = filePath[workspace!.Length..].TrimStart('/');
}

The workspace! null-bang is unnecessary — !string.IsNullOrEmpty(workspace) already proves non-null to the compiler. Not a bug, just noise. Safe to clean up in passing.

---

Minor: ArtifactUrl still hardcodes https://github.com/

In HtmlReporter.TryGitHubIntegrationAsync:

_githubReporter.ArtifactUrl = $"https://github.com/{repo}/actions/runs/{runId}/artifacts/{artifactId}";

Source links now correctly use GITHUB_SERVER_URL (fixed in this commit), but the artifact URL does not. GHES users will get a working source link but a broken "View Report" link. Low-risk to fix alongside the source link change:

var serverUrl = Environment.GetEnvironmentVariable("GITHUB_SERVER_URL") ?? "https://github.com";
_githubReporter.ArtifactUrl = $"{serverUrl.TrimEnd('/')}/{repo}/actions/runs/{runId}/artifacts/{artifactId}";

---

Overall

The GHES source link concern from the previous review is resolved. The main remaining issue is the totalDuration CPU-vs-wall-clock mislabelling, which will produce actively misleading output for any project using TUnit's parallelism (i.e., virtually everyone). The artifact URL GHES gap is a minor consistency issue. Everything else is clean and the overall UX improvement is significant.

[claude]

Follow-up Review (commit 048fffc)

One new commit landed after the last review, addressing the two remaining issues.

Previous concerns — all resolved ✅

Wall-clock vs CPU time (main issue): Fixed. _runStopwatch = Stopwatch.StartNew() is now started in BeforeRunAsync and elapsed time is read in AfterRunAsync. The summary now correctly shows wall-clock elapsed time rather than the sum of individual test durations, which would have been wildly inflated for parallel test runs.

Artifact URL GHES compatibility: Fixed. _githubReporter.ArtifactUrl is now built using GITHUB_SERVER_URL with the https://github.com fallback, matching the approach used for source links. The .TrimEnd('/') guard prevents double-slash. GHES users will now get a correct "View Report" link.

---

Carry-forward nit: redundant null-bang operators in GetSourceLink

These were flagged in two prior reviews and are still present:

if (!string.IsNullOrEmpty(workspace) && filePath.StartsWith(workspace!, StringComparison.OrdinalIgnoreCase))
{
    filePath = filePath[workspace!.Length..].TrimStart('/');
}
// ...
var repoName = repo!.Split('/').LastOrDefault() ?? "";

The workspace! and repo! null-bangs are unnecessary — !string.IsNullOrEmpty() and the earlier string.IsNullOrEmpty(repo) null guard already prove non-null to the compiler. Not a bug, purely noise. Fine to leave if the PR has already had enough churn.

---

Overall

All substantive issues across the entire review history are now resolved. The PR is in good shape — wall-clock timing is correct, GHES links work end-to-end, the false-positive tip is prevented, and the UX output is significantly richer than before. LGTM.

[claude]

Follow-up Review (commit 0822252)

One new commit landed after the last LGTM on `048fffc4`.

New commit: "fix: hide redundant segment breakdown when all tests pass" ✅

The if (passedCount != totalCount) guard is correct and well-motivated. When all tests pass, the top summary line (9 tests completed in 1.9s — 100.0% passed) already conveys everything the "✅ 9 passed" breakdown would repeat. Clean, focused change.

---

Carry-forward nit (from prior reviews): redundant null-bangs in GetSourceLink

Still present on lines 575, 577, 581 of GitHubReporter.cs:

filePath.StartsWith(workspace!, ...)   // workspace is proven non-null by the !IsNullOrEmpty check
filePath = filePath[workspace!.Length..]
var repoName = repo!.Split('/')        // repo is proven non-null by the earlier null guard

Not a bug — acknowledged in the previous review as noise. Fine to leave as-is.

---

New concern: unencoded HTML in <summary> and table cells

The existing HtmlReportGenerator.cs encodes all user-supplied content via WebUtility.HtmlEncode before inserting it into HTML. GitHubReporter does not follow this practice, and there are two spots where user-controlled content is embedded raw:

1. Inline failure <details> summary (line ~363):

stringBuilder.AppendLine($"<summary><code>{name}</code> ({duration}){sourcePart}</summary>");

name is $"{className}.{displayName}" where displayName is the MTP test node's display name, which for parametrized tests can include arbitrary string arguments. A parameter like "</code></summary><img src=x onerror=alert(1)>" would break the <summary> tag.

2. GetDetails → error <pre> (used in both inline details and the table):

return $"<pre>{GetError(stateProperty)}</pre>";

Exception messages are user-controlled (e.g., throw new Exception("</pre><b>injected</b>")), which can break out of the <pre> block.

Why it matters: GitHub does sanitize its Markdown-rendered step summaries (disallowing <script> etc.), but it permits many structural HTML tags. Breaking out of <details>/<summary> is likely safe from XSS in practice, but can corrupt the rendered summary layout in a confusing way.

Suggested fix — consistent with the existing HtmlReportGenerator pattern:

using System.Net;

// in the summary line:
$"<summary><code>{WebUtility.HtmlEncode(name)}</code> ({duration}){sourcePart}</summary>"

// in GetDetails:
return $"<pre>{WebUtility.HtmlEncode(GetError(stateProperty))}</pre>";

// in table cells:
detailsBuilder.AppendLine($"<td>{WebUtility.HtmlEncode(name)}</td>");
detailsBuilder.AppendLine($"<td>{WebUtility.HtmlEncode(details)}</td>");

Note: status and duration are reporter-generated strings (not user input), so they don't need encoding.

---

Overall

The new commit is clean and correct. The one substantive concern worth addressing is the missing HtmlEncode on user-supplied content that gets embedded in HTML tags — especially given that the codebase already has the right pattern in HtmlReportGenerator. The null-bang nit remains a carry-forward non-issue.

[claude]

Follow-up Review (latest state)

All major issues from previous review rounds have been addressed. This is a comprehensive pass over the full PR as merged.

---

✅ All prior concerns resolved

• False-positive tip — fixed via flag
• Wall-clock vs CPU time — fixed via in
• FormatDuration hours rounding — fixed via (int) cast instead of :F0
• Source link path double-stripping — fixed; is primary, repo-name fallback is secondary
• GHES compatibility — fixed via env var with fallback
• Segment breakdown on all-pass — fixed via guard

---

Remaining nit: redundant early return

The if (passedCount == last.Count) early return (around line 276 in the new file) is still present. With the new structure, when all tests pass:

• failureMessages and otherMessages will both be empty after the loop
• All subsequent if (failureMessages.Count > 0) and if (hasRemainingDetails) blocks are skipped
• The final --- separator is written by both code paths

The early return and the unified path produce identical output. It's not a bug, just slightly redundant. If you ever want to clean it up, you can remove it and rely on the conditional sections below it.

---

Minor: flaky test timing shows last-attempt duration only

flakyTests.Add((name, finalStateCount, timing?.GlobalTiming.Duration));

The timing is taken from the last update (the successful attempt). For a test that timed out twice before passing, the summary would show something like 3 attempts (0.8s) — the 0.8s is the final fast pass, not the total. This could feel misleading when debugging flaky tests.

Suggested improvement: Sum durations across all attempts in _updates[kvp.Key], or at minimum append a note like "(final attempt)". This is low priority but worth a future issue.

---

Minor: HTML in test names / error messages

Test names are embedded in <summary><code>{name}</code>... and error details are written into table <td> cells without HTML-escaping. A test named or error message containing </code></summary></details> could break the summary structure. This is pre-existing (the old table had the same issue) and low-probability since test names come from code, but System.Net.WebUtility.HtmlEncode on names and detail strings would be strictly safer.

---

Overall

This is a well-executed feature PR. The GitHub step summary output goes from a minimal table to a genuinely useful CI artifact: status at a glance, flaky detection, skipped grouping, inline failure details with source links, and human-readable timing. The architecture (HtmlReporter pushes to GitHubReporter, not vice versa) is the right call for keeping concerns separated without over-engineering it.

The two minor points above are non-blocking suggestions for future work — they don't affect correctness today. LGTM.

[claude]

Follow-up Review (latest state)

All major issues from previous review rounds have been addressed. This is a comprehensive pass over the full PR as merged.

---

All prior concerns resolved

• False-positive tip — fixed via ShowArtifactUploadTip flag
• Wall-clock vs CPU time — fixed via Stopwatch in BeforeRunAsync
• FormatDuration hours rounding — fixed via (int) cast instead of :F0
• Source link path double-stripping — fixed; GITHUB_WORKSPACE is primary, repo-name fallback is secondary
• GHES compatibility — fixed via GITHUB_SERVER_URL env var with fallback
• Segment breakdown on all-pass — fixed via passedCount != totalCount guard

---

Remaining nit: redundant early return

The if (passedCount == last.Count) early return (around line 276 in the new file) is still present. With the new structure, when all tests pass:

• failureMessages and otherMessages will both be empty after the loop
• All subsequent if (failureMessages.Count > 0) and if (hasRemainingDetails) blocks are skipped
• The final --- separator is written by both code paths

The early return and the unified path produce identical output. Not a bug, just slightly redundant. Could be removed to rely on the conditional sections below it.

---

Minor: flaky test timing shows last-attempt duration only

flakyTests.Add((name, finalStateCount, timing?.GlobalTiming.Duration));

The timing is taken from the last update (the successful attempt). For a test that timed out twice before passing, the summary would show something like 3 attempts (0.8s) — the 0.8s is the final fast pass, not the total. This could feel misleading when debugging flaky tests.

Suggested improvement: Sum durations across all attempts in _updates[kvp.Key], or at minimum append a label like "(final attempt)". Low priority but worth a future issue.

---

Minor: HTML in test names / error messages (pre-existing)

Test names are embedded in <summary><code>{name}</code>... and error details go into table <td> cells without HTML-escaping. A test name or error message containing </code></summary></details> could break the summary structure. This is a pre-existing issue (old table had the same problem) and low-probability since test names come from code, but System.Net.WebUtility.HtmlEncode on names and detail strings would be strictly safer.

---

Overall

This is a well-executed feature. The GitHub step summary goes from a minimal table to a genuinely useful CI artifact: status at a glance, flaky detection, skipped grouping, inline failure details with source links, and human-readable timing. The architecture (HtmlReporter pushes to GitHubReporter, not vice versa) is the right call for keeping concerns separated without over-engineering.

The two minor points above are non-blocking suggestions — they don't affect correctness. LGTM.