HTML Report: source link + code snippet on Source tab (#5993)

[thomhurst]

Closes #5993. Reworks the approach from #5994 to be cleaner, lighter, and AOT-friendly.

What

The HTML report's Source tab only showed path:line. When running in GitHub Actions it now also renders:

• a "Jump to source ↗" link to the GitHub blob at the test's line range, and
• a code snippet of the test method, fetched lazily client-side.

Context	Source tab shows
GitHub Actions CI	path, jump-to-source link, fetched snippet
Local / non-GitHub CI	path + line only (unchanged)
Fetch failure (private repo / offline)	path, link, "Source unavailable"

How

• No code embedded in HTML. The snippet is fetched from raw.githubusercontent.com (or the Enterprise raw URL) on Source-tab activation and cached per file — keeps report size small, as the issue requested.
• No runtime source parsing / disk I/O. Source-gen mode already carries an exact method end line (Roslyn span) → full-method snippet for free. Reflection mode has no end line, so the client falls back to a fixed window from the declaration line. The jump link works either way.
• Extracted a shared GitHubSourceLink.ToRepoRelativePath helper so GitHubReporter and HtmlReporter strip workspace-relative paths identically (removed duplication). CI context (repo/sha/workspace/serverUrl) is read once per report build, not per test.
• Added GITHUB_WORKSPACE / GITHUB_SERVER_URL to EnvironmentConstants.

Difference from #5994

Drops that PR's SourceLocationResolver — a hand-rolled C# parser that read .cs files from disk on every reflection-mode discovery run (even with no HTML report) and broke on raw strings / interpolation. Not needed: source-gen already has end lines; reflection degrades gracefully.

Bonus fix

GitHubReporter source links added +1 to an already-1-based line number, so failure-summary links pointed one line below the test. Removed the +1 and pinned it with a regression test.

Tests

HtmlReporterTests 28/28, GitHubReporterTests 15/15 — green. New tests cover path-stripping, end-line population, JSON emission, and the off-by-one fix.

[codacy-production]

Not up to standards ⛔

🔴 Issues 6 minor

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

Results:
6 new issues

Category	Results
CodeStyle	6 minor

View in Codacy

🟢 Metrics 51 complexity

Metric	Results
Complexity	51

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Code Review — HTML Report: source link + code snippet (#6100)

Overall this is a well-structured, clean PR that addresses issue #5993 thoughtfully. The approach of deferring file reads to the browser (client-side fetch on demand) rather than reading .cs files at test-discovery time (the dropped SourceLocationResolver approach from #5994) is the right architectural call — zero performance overhead on test execution and no disk I/O coupling to the test runner.

---

✅ What's Done Well

• Off-by-one fix in GitHubReporter is correct and backed by a dedicated regression test.
• GitHubSourceLink.ToRepoRelativePath cleanly de-duplicates path-stripping logic that was previously scattered.
• Test coverage is solid across all new paths: workspace stripping, fallback, null cases, end-line extraction, JSON round-trip, and the off-by-one regression.
• GitHub Enterprise raw URL construction is correct — serving raw content at /<repo>/raw/<commit>/<path> under the same domain is handled properly.
• AOT compatibility is maintained — no runtime reflection, all serialization uses Utf8JsonWriter.
• Graceful degradation is present for failed fetches.

---

Findings

1. ToRepoRelativePath silently depends on callers normalizing workspace

The method normalizes filePath (backslashes → forward slashes) but does not normalize the workspace parameter. Both current callers do ?.Replace('\\', '/') before passing it in, but the method's own signature doesn't enforce or document this.

// Current: workspace is expected to already be normalized by caller
public static string? ToRepoRelativePath(string filePath, string? workspace, string? repoName)
{
    filePath = filePath.Replace('\\', '/');
    // workspace is NOT normalized here
    if (workspace != null && filePath.StartsWith(workspace, StringComparison.OrdinalIgnoreCase))

Recommendation: Normalize workspace inside the method. This removes the hidden coupling at no meaningful cost:

public static string? ToRepoRelativePath(string filePath, string? workspace, string? repoName)
{
    filePath = filePath.Replace('\\', '/');
    workspace = workspace?.Replace('\\', '/');
    ...
}

Why this matters: If a future caller forgets to normalize, the StartsWith check silently fails and falls back to repo-name matching — which may still find the right path, masking the bug. Failing fast is better than a silent fallback with subtly wrong results.

---

2. JS slice boundary is subtly correct but unexplained

In loadSourceSnippet, the slice that extracts the method body is correct but works for a non-obvious reason:

const from = Math.max(0, startLine - 1);  // 1-based → 0-based index
const to = endLine > startLine
    ? Math.min(lines.length, endLine)      // endLine is 1-based; slice() is exclusive
    : Math.min(lines.length, from + SNIPPET_FALLBACK_LINES);
const snippet = lines.slice(from, to);

For a method at lines 12–20 (1-based): from = 11, to = 20, slice(11, 20) yields indices 11–19 = lines 12–20. Correct — but only because endLine is 1-based and Array.slice's exclusive upper bound happens to equal the 1-based end value.

Recommendation: Add a short inline comment explaining the arithmetic so this doesn't get "fixed" into a bug:

// from: convert 1-based startLine to 0-based index
// to: endLine is 1-based; slice() excludes the upper index,
//     so a 1-based endLine is the correct exclusive bound
const from = Math.max(0, startLine - 1);
const to = endLine > startLine
    ? Math.min(lines.length, endLine)
    : Math.min(lines.length, from + SNIPPET_FALLBACK_LINES);

---

3. Minor asymmetry in path/URL normalization

GetCiContext applies TrimEnd('/') to serverUrl but not to workspace. There's no bug here (an extra leading slash in the relative path is handled by TrimStart('/') in ToRepoRelativePath), but the inconsistency is mildly surprising. If workspace normalization moves inside ToRepoRelativePath (finding #1), adding a TrimEnd('/') there too would make the contract complete and symmetric.

---

4. Gap in test coverage: un-normalized workspace in ToRepoRelativePath

There is no test for ToRepoRelativePath receiving a workspace string that still contains backslashes. Given that fixing #1 above would make this scenario handled correctly, it's worth adding a test case:

[Arguments(@"C:\actions-runner\_work\TUnit\TUnit\MyTest.cs",
           @"C:\actions-runner\_work\TUnit\TUnit",
           "TUnit",
           "MyTest.cs")]

---

Summary

No blocking issues. The two actionable improvements are:

1. Normalize workspace inside ToRepoRelativePath to remove the hidden caller contract.
2. Add a comment in the JS explaining the 1-based/exclusive-bound arithmetic.

Both are small, low-risk changes. The off-by-one fix, test coverage, and architectural approach are all solid.

[claude]

Code Review — HTML Report: source link + code snippet (#6100) (Round 2)

The previous review raised four concerns; all have been addressed in the current state:

Prior finding	Status
should normalize workspace internally	✅ Fixed — inside the method
JS slice arithmetic should have inline comments	✅ Added — clear comment block explains the 1-based / exclusive-bound relationship
trims but not (minor asymmetry)	✅ Workspace now normalized in too
Missing test for backslash-workspace input to	✅ added

---

Remaining Issue: Incomplete Environment Cleanup in New Regression Test

TUnit.Engine.Tests/GitHubReporterTests.cs — AfterRunAsync_SourceLink_Uses_OneBased_Line_Without_Extra_Increment:

Environment.SetEnvironmentVariable("GITHUB_REPOSITORY", "thomhurst/TUnit");
Environment.SetEnvironmentVariable("GITHUB_SHA", "abc123");
Environment.SetEnvironmentVariable("GITHUB_WORKSPACE", "/work/TUnit");

try { ... }
finally
{
    Environment.SetEnvironmentVariable("GITHUB_WORKSPACE", null); // only this is restored
}

and are set but never cleaned up. If the test suite runs in-process (no process isolation between tests), these values leak into subsequent tests that read those env vars — including existing tests that exercise GitHub reporter paths with different expectations. The fix is to restore all three vars in the block, or use a helper that restores to the prior value:

finally
{
    Environment.SetEnvironmentVariable("GITHUB_REPOSITORY", null);
    Environment.SetEnvironmentVariable("GITHUB_SHA", null);
    Environment.SetEnvironmentVariable("GITHUB_WORKSPACE", null);
}

---

Minor Observation: Fallback Path Produces Invalid Blob URL

In :

var filePath = GitHubSourceLink.ToRepoRelativePath(fileLocation.FilePath, workspace, repo)
    ?? fileLocation.FilePath.Replace('\\', '/');

When returns null (path cannot be resolved), the fallback emits the full absolute path into the blob URL: . This was the pre-existing behaviour before this PR and the PR doesn't make it worse — just worth a follow-up issue if anyone hits it.

---

Everything Else Looks Good

• Off-by-one fix is correct; the test with / is watertight.
• ** extraction** is clean and the four test cases cover workspace, fallback, and null paths.
• XSS prevention: is applied on all user-controlled strings rendered into HTML and all attributes. present on the external link.
• Lazy-load correctness: is set synchronously before the starts, so rapid tab-switch/re-switch cannot trigger duplicate requests.
• Cache key () is collision-safe in practice given the commit SHA.
• GitHub Enterprise raw URL: correctly handles the GHE format.
• AOT compatibility maintained throughout — no runtime reflection added, model serialization uses .
• Graceful degradation: non-CI, fetch-failure, and single-line-span paths all fall through cleanly.

The env-var cleanup bug is the only blocking item. Everything else is solid.

[claude]

Code Review — HTML Report: source link + code snippet (#6100) (Round 2)

The previous review raised four concerns; all have been addressed in the current state:

Prior finding	Status
ToRepoRelativePath should normalize workspace internally	Fixed — normalizedWorkspace = workspace?.Replace('\\', '/').TrimEnd('/') inside the method
JS slice arithmetic should have inline comments	Added — clear comment block explains the 1-based / exclusive-bound relationship
GetCiContext trims serverUrl but not workspace (minor asymmetry)	Workspace now normalized in GetCiContext too
Missing test for backslash-workspace input to ToRepoRelativePath	GitHubSourceLink_Strips_Workspace_Prefix_When_Workspace_Has_Backslashes added

---

Remaining Issue: Incomplete Environment Cleanup in New Regression Test

TUnit.Engine.Tests/GitHubReporterTests.cs — AfterRunAsync_SourceLink_Uses_OneBased_Line_Without_Extra_Increment:

Environment.SetEnvironmentVariable("GITHUB_REPOSITORY", "thomhurst/TUnit");
Environment.SetEnvironmentVariable("GITHUB_SHA", "abc123");
Environment.SetEnvironmentVariable("GITHUB_WORKSPACE", "/work/TUnit");

try { ... }
finally
{
    Environment.SetEnvironmentVariable("GITHUB_WORKSPACE", null); // only this is restored
}

GITHUB_REPOSITORY and GITHUB_SHA are set but never cleaned up. If tests run in-process without process isolation, these values leak into subsequent tests that read those env vars — including existing GitHub reporter tests with different expectations. Fix:

finally
{
    Environment.SetEnvironmentVariable("GITHUB_REPOSITORY", null);
    Environment.SetEnvironmentVariable("GITHUB_SHA", null);
    Environment.SetEnvironmentVariable("GITHUB_WORKSPACE", null);
}

---

Minor Observation: Fallback Path Produces Invalid Blob URL (Pre-existing)

In GitHubReporter.cs:

var filePath = GitHubSourceLink.ToRepoRelativePath(fileLocation.FilePath, workspace, repo)
    ?? fileLocation.FilePath.Replace('\\', '/');

When ToRepoRelativePath returns null, the fallback emits the full absolute path into the blob URL: .../blob/abc123/C:/actions-runner/path/to/File.cs. This pre-dates this PR and is not introduced here — worth a follow-up issue if anyone hits it.

---

Everything Else Looks Good

• Off-by-one fix is correct; the test asserting #L12 and checking #L13 is absent is watertight.
• GitHubSourceLink extraction is clean, with four test cases covering workspace stripping, fallback to repo-name matching, and null/unresolvable inputs.
• XSS prevention: esc() applied on all user-controlled strings rendered into HTML and all href attributes. rel="noopener" present on the external link.
• Lazy-load correctness: dataset.loaded = '1' is set synchronously before fetch() starts, preventing duplicate requests on rapid tab switch/re-switch.
• Cache key (repo + '/' + commit + '/' + path) is collision-safe in practice given the full commit SHA.
• GitHub Enterprise raw URL construction (serverUrl + '/' + repo + '/raw/' + commit + '/' + path) correctly handles the GHE raw-file path format.
• AOT compatibility maintained throughout — no runtime reflection added, model serialization uses Utf8JsonWriter.
• Graceful degradation: non-CI, fetch-failure, and single-line-span (reflection mode) paths all fall through cleanly with sensible UI state.

The env-var cleanup in the regression test is the only actionable item. The overall design is solid.

[claude]

Code Review — PR #6100: HTML Report source link + code snippet on Source tab

Summary

All issues raised in previous review rounds (three rounds total) have been addressed. The PR is in good shape. Here's a round-up of what was verified and one minor pre-existing observation.

---

Previously Raised Issues — All Resolved ✓

Round 1:

• ToRepoRelativePath now normalizes workspace internally (backslash→slash, TrimEnd('/')) — callers no longer need to do this.
• JS slice arithmetic has an explanatory comment block.
• workspace normalization asymmetry resolved.
• Backslash-workspace test added (GitHubSourceLink_Strips_Workspace_Prefix_When_Workspace_Has_Backslashes).

Round 2:

• Env-var leak in AfterRunAsync_SourceLink_Uses_OneBased_Line_Without_Extra_Increment — the finally block now correctly cleans up all three env vars (GITHUB_REPOSITORY, GITHUB_SHA, GITHUB_WORKSPACE).

---

Current Code Quality

Architecture: Client-side lazy fetch on tab activation with per-file caching is the right design — zero overhead outside CI, no disk I/O during the test run, and the report file stays small.

Off-by-one fix: Removing the spurious +1 from GitHubReporter.cs is correct and well-tested. Line numbers from fileLocation.LineNumber are already 1-based when they arrive.

XSS safety: esc() is applied consistently to all user-controlled strings embedded in the HTML template. rel="noopener" is present on the external link.

AOT compatibility: No runtime reflection added; all serialization uses Utf8JsonWriter. ✓

Graceful degradation: Fetch failures show "Source unavailable" rather than breaking the report. The dataset.loaded = '1' flag is set synchronously before fetch() starts, which correctly prevents duplicate in-flight requests on rapid tab switching.

---

Minor Observation (Pre-existing, Not a Regression)

In GitHubReporter.cs, the fallback when ToRepoRelativePath returns null:

?? fileLocation.FilePath.Replace('\\', '/')

...emits an absolute path (e.g. C:/actions-runner/.../File.cs) into the GitHub blob URL, producing an invalid link. This pre-dates this PR and isn't introduced here — worth a follow-up issue but not a blocker for this change.

---

Overall

This is solid work. All three rounds of review feedback have been incorporated cleanly. The feature is well-scoped, the test coverage is thorough, and the implementation follows the project's patterns correctly. Ready to merge.

[claude]

Code Review — PR #6100 (Round 4): Multi-Provider Source Links

Reviewing the new commit (29303639) which adds GitLab and Bitbucket support via a unified SourceControlContext / SourceLinkTemplates design. All findings from prior rounds remain resolved.

---

What's New Since Last Review

The SourceControlContext.cs and SourceLinkTemplates additions are the main new surface. The design — detect provider once, bake server/slug/SHA into URL templates, fill {path}/{line} client-side — is clean and well-suited for adding future providers without touching the JS or the report data model.

---

Finding: Asymmetry Between SourceControlContext and TryGitHubIntegrationAsync on Missing GITHUB_SERVER_URL

SourceControlContext.BuildGitHub correctly requires server to be non-null before emitting any source links:

if (!string.IsNullOrEmpty(server) && !string.IsNullOrEmpty(slug) && !string.IsNullOrEmpty(sha))
{
    links = new SourceLinkTemplates(...);
}

But TryGitHubIntegrationAsync in HtmlReporter.cs still defaults to https://github.com when the variable is absent:

var serverUrl = (Environment.GetEnvironmentVariable(EnvironmentConstants.GitHubServerUrl)
    ?? EnvironmentConstants.GitHubDefaultServerUrl).TrimEnd('/');

These two code paths now have different answers to "what happens when GITHUB_SERVER_URL is unset?" — one silently omits links, the other guesses GitHub.com. In standard GitHub Actions both variables are always present, so there's no real-world bug, but the inconsistency will surprise a future maintainer touching either path. A comment on the artifact-URL line explaining why it retains the default while the source-link path does not would close the gap without changing behavior.

---

Everything Else Looks Good

CORS-awareness is well-handled: GitHub.com raw is enabled, GitHub Enterprise raw is skipped (unknown CORS policy), GitLab raw is skipped (documented CORS gap), Bitbucket Cloud raw is enabled. The server == EnvironmentConstants.GitHubDefaultServerUrl equality check for determining the raw-URL strategy is correct after TrimEnd('/') normalization.

GitLab range URL format (#L{start}-{end}) matches GitLab's actual anchor syntax. Bitbucket format (#lines-{start}:{end}) is also correct.

Bitbucket ServerUrl as a constant is the right call — Bitbucket Cloud has no self-hosted equivalent that would need a dynamic server URL.

Test coverage for the four providers (GitHub, GitHub Enterprise, GitLab, Bitbucket, no-CI) is comprehensive and uses a local dictionary rather than process-level env vars — good isolation.

SourceControlContext.Empty sentinel is a clean pattern; the Detect call-site is appropriately simple.

ExtractTestResult optional parameter order (ciRepo, ciWorkspace after attempts) is slightly awkward but the method is internal and the tests confirm the right values flow through.

---

Summary

The asymmetry in GITHUB_SERVER_URL handling is worth a short comment to aid future readers, but it's not a functional bug. The multi-provider design is solid, coverage is thorough, and all prior findings remain resolved. Ready to merge.

[claude]

Code Review — PR #6100 (Round 5): Final Tidy Pass

Reviewing the latest commit (a5329d08) — a pure refactor: renames GitHubSourceLink → SourcePathResolver, drops redundant workspace normalization at call sites, and switches the source-tab JS to template literals. No new behaviour.

---

Previous Round Findings — Status

Finding	Status
Round 1: ToRepoRelativePath should normalize workspace internally	✅ Resolved (earlier round)
Round 1–2: JS slice arithmetic comment	✅ Resolved
Round 2: env-var leak in regression test	✅ Resolved
Round 4: Asymmetry in GITHUB_SERVER_URL handling between SourceControlContext.BuildGitHub and TryGitHubIntegrationAsync — reviewer asked for a comment explaining why the artifact URL path retains the default while the source-link path does not	⚠️ Still open — no comment added

---

Round 4 Finding — Still Unresolved

HtmlReporter.cs:770:

var serverUrl = (Environment.GetEnvironmentVariable(EnvironmentConstants.GitHubServerUrl)
    ?? EnvironmentConstants.GitHubDefaultServerUrl).TrimEnd('/');

SourceControlContext.BuildGitHub correctly omits source links when GITHUB_SERVER_URL is absent. This path retains the https://github.com default. The previous review noted this isn't a functional bug (the variable is always present in real GitHub Actions), but suggested a short comment to prevent future reader confusion. That comment was not added in this round.

Suggested addition (one line, no behavior change):

// Artifact URL is safe to default to github.com — ACTIONS_RUNTIME_TOKEN gating above ensures
// we only reach here when the Actions API is available, which implies a real GitHub host.
var serverUrl = (Environment.GetEnvironmentVariable(EnvironmentConstants.GitHubServerUrl)
    ?? EnvironmentConstants.GitHubDefaultServerUrl).TrimEnd('/');

---

Refactor Quality

The rename GitHubSourceLink → SourcePathResolver is the right call now that the helper is used for all three providers, not just GitHub. The JS template literal switch is a clean readability improvement with no semantic change. Dropping the call-site normalizations that ToRepoRelativePath already handles internally removes dead code correctly.

---

Overall

The PR is in excellent shape. The single open item from Round 4 is a comment-only clarification, not a bug. All functional and structural review concerns across five rounds have been addressed. Ready to merge once the comment is added (or if the author decides it's not worth the noise, the code is correct as-is).