feat: warn when setup hooks pass the test execution token

[Sing303]

Description

A setup hook can pass context.Execution.CancellationToken to an awaited operation and silently bypass its own timeout. The hook's injected CancellationToken includes that timeout; the test execution token does not. A runtime probe with [Timeout(100)] confirmed that the hook token is cancelled while the execution token remains uncancelled. We found this mistake in 37 setup hooks in a downstream application.

Add warning TUnit0075 for the direct property chain from a [Before(Test)] or [BeforeEvery(Test)] hook's TestContext parameter, when passed as a cancellation argument to an operation the hook awaits or returns. The rule includes ConfigureAwait, expression-bodied and conditional returns, and overrides of virtual setup hooks without repeated attributes. It also applies when the default hook timeout is used. Documentation explains which token to pass and that cancellation remains cooperative.

Related Issue

Related to #4789, but does not attempt to resolve its broader timeout/lifecycle design. This PR detects a concrete incorrect-token use; it does not detach timed-out hooks, alter initialization or cleanup, or change runtime APIs.

Type of Change

• New analyzer diagnostic for incorrect setup cancellation
• Documentation update

Scope and compatibility

The diagnostic checks actual TUnit symbols and a CancellationToken parameter. It does not inspect cleanup-only methods, ordinary tests/helpers, cancellation-state reads, nested lambdas/local functions, or other contexts. It follows the actual override chain rather than matching method names, so new methods do not inherit a setup role. It deliberately does not follow aliases or add an automatic code fix. This introduces a new compiler warning, which will surface as an error in projects that treat all warnings as errors.

This is a proposed diagnostic; there is no prior maintainer agreement on the new warning. Feedback on whether this narrow scope belongs in the existing analyzer is welcome.

Validation

• Reproduced the missing warning before implementation.
• 33 new positive/negative cases plus 20 existing analyzer/code-fix cases: 53 passed, 0 failed, 0 skipped.
• Confirmed both review findings with failing regressions before fixing them: four conditional-return cases and three inherited-hook cases. Boundary checks cover hidden methods, cleanup-only overrides, and calls used as a conditional's condition rather than its result.
• Debug/net10.0 build succeeded. Existing nullable/release-tracking warnings remain.
• Documentation changes contain prose and inline examples; no C# fenced snippets or source-generator output changed.

dotnet test --project tests/TUnit.Analyzers.Tests/TUnit.Analyzers.Tests.csproj -c Debug -f net10.0 --no-build --no-restore --treenode-filter "/*/*/*CancellationToken*/*" --minimum-expected-tests 53

Checklist

• Read the contributing guidelines.
• Added failing regression tests before implementing the diagnostic.
• Added rule documentation, localized resources and release tracking.
• No test discovery/execution or engine public API changes; source-generated/reflection runtime changes are not applicable.
• Full repository test suite (not run; validation was scoped to cancellation-token analyzer and code-fix tests).

Summary by CodeRabbit

• New Features
  • Added a warning when setup hooks pass the test-context cancellation token directly to an awaited or returned operation. Use the hook’s injected cancellation token to respect hook timeouts.
  • The warning also covers conditional branches and overrides of virtual setup hooks.
  • Added reference documentation for the warning, including its scope and examples.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: c28c61b5-3877-4da1-901a-10433e858677

📥 Commits

Reviewing files that changed from the base of the PR and between 534a101 and c3b5841.

📒 Files selected for processing (4)

• docs/docs/reference/tunit0075.md
• docs/docs/writing-tests/hooks.md
• src/TUnit.Analyzers/TimeoutCancellationTokenAnalyzer.cs
• tests/TUnit.Analyzers.Tests/BeforeHookCancellationTokenAnalyzerTests.cs

Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.

---

📝 Walkthrough

Walkthrough

This change adds TUnit0075 for setup-hook calls that pass the hook’s TestContext.Execution.CancellationToken to a cancellation-token parameter. It checks conditional branches and inherited setup-hook overrides. It also adds analyzer tests and documents the warning’s scope and hook cancellation-token guidance.

Changes

Setup Hook Cancellation Token Warning

Layer / File(s)	Summary
Diagnostic contract and analyzer behavior src/TUnit.Analyzers/DiagnosticIds.cs, src/TUnit.Analyzers/Resources.resx, src/TUnit.Analyzers/Rules.cs, src/TUnit.Analyzers/AnalyzerReleases.Unshipped.md, src/TUnit.Analyzers/TimeoutCancellationTokenAnalyzer.cs	Adds the TUnit0075 warning and reports it when a direct awaited or returned setup operation passes the hook’s Execution.CancellationToken to a cancellation-token parameter. The analyzer also recognizes conditional branches and checks the overridden-method chain for setup-hook attributes.
Analyzer tests and usage documentation tests/TUnit.Analyzers.Tests/BeforeHookCancellationTokenAnalyzerTests.cs, docs/docs/writing-tests/hooks.md, docs/docs/reference/tunit0075.md, docs/sidebars.ts	Adds warning and no-warning tests. Documents the warning scope and hook-token guidance, and adds the reference page to the sidebar.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Feature

Suggested reviewers: thomhurst

Merge Risk: ⚪ Minimal · up to c3b58

The new setup-hook warning appears mergeable after normal checks. No concrete issue requiring a pre-merge change was identified.

Security Architecture Review

Security architecture risk: 🔵 Low · up to c3b58

The change is limited to a narrowly scoped warning, but that warning can interrupt builds configured to reject warnings. No new runtime security exposure was identified; the evidence does not fully establish the upstream source of the hook’s timeout token.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The independently affected scope is compilation of projects using the analyzer, not a newly reachable hook-execution or privileged runtime sink.

Trust Boundaries and Controls

• observed — Source code supplies the operation being inspected, but symbol identity and hook-shape checks constrain reporting; the observed outcome is a diagnostic, not execution of source-selected operations.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 5 files. (2 skipped: 2…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: adding a warning when setup hooks pass the test execution token.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 5 files. (2 skipped: 2 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit checks each setup call,
And spots the token passed along.
Branches, overrides join the trail,
Tests mark where warnings hold or fail.
The hook token guides the way.}

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Review: TUnit0075 — warn on setup hooks passing the test execution token

Reviewed the analyzer implementation, docs, and tests. No prior substantive review comments exist on this PR to reconcile against (only CodeRabbit's automated "in progress" placeholder).

Summary

This is a well-scoped, correctly-implemented diagnostic. The core detection logic in TimeoutCancellationTokenAnalyzer.AnalyzeSetupArgument is sound:

• The IPropertyReferenceOperation pattern match correctly anchors on TestContext.Execution (ITestExecution.CancellationToken) reached via a direct parameter reference, which excludes TestContext.Current, locals holding a captured token, and lookalike types (all covered by tests: Before_Hook_With_Unrelated_Context_Shows_No_Warning, Before_Hook_With_Lookalike_Context_Shows_No_Warning).
• IsDirectSetupOperation's parent-chain walk correctly excludes nested lambdas/local functions by checking for IAnonymousFunctionOperation/ILocalFunctionOperation ancestors — verified against the Func<Task> setup = () => ... and local-function test cases.
• The HookLevel.Test + HookType.Before gate correctly limits this to [Before(Test)]/[BeforeEvery(Test)], matching the PR's stated scope (Class/Assembly/Session/Discovery hooks use different context types entirely, so the property chain wouldn't match them anyway).
• Severity (Warning), diagnostic ID assignment, Rules.cs/DiagnosticIds.cs/Resources.resx/AnalyzerReleases.Unshipped.md entries all follow the existing conventions in this file exactly.

Minor gaps (not blocking, already implicitly scoped-out)

• IsDirectSetupOperation only looks one level up from the invocation (await/return or .ConfigureAwait().await/return). Conditional returns (return cond ? Op(token) : Other();) or an intermediate IConversionOperation (e.g. an implicit numeric/interface conversion before the return) would produce a false negative — no warning even though the pattern is the same bug. This is consistent with the PR's explicitly documented scope ("does not follow ... other contexts"), so it's a reasonable, disclosed limitation rather than an oversight.
• The doc addition in hooks.md (line 60) states the "pass the injected token" guidance generically under the shared "Hook Parameters" section (which also covers Class/Assembly/Session/Discovery/After hooks), while the diagnostic itself only fires for Before(Test)/BeforeEvery(Test). This is clarified immediately after in the next paragraph, so it doesn't mislead, but consider tightening the first sentence's framing so it's unambiguous that the automated check has narrower reach than the general advice.

Verification

• Diagnostic ID (TUnit0075), Rules.cs descriptor, Resources.resx entries, and AnalyzerReleases.Unshipped.md row are all consistent with each other and with the file's existing pattern.
• Doc file tunit0075.md and the hooks.md addition contain no C# fenced code blocks (only inline code spans), consistent with the PR description's claim that no compiled documentation snippets were touched.
• sidebars.ts entry and the hooks.md #hook-parameters anchor referenced from tunit0075.md both resolve correctly.
• Test coverage in BeforeHookCancellationTokenAnalyzerTests.cs is thorough: direct await, ConfigureAwait, expression-bodied return, named arguments, default vs. explicit [Timeout], BeforeEvery, non-setup methods (After/AfterEvery/[Test]/no attribute), and nine distinct "should not warn" scenarios (locals, helper methods, lambdas, local functions, IsCancellationRequested reads, unrelated/lookalike contexts).
• Was unable to execute dotnet build/dotnet test in this sandboxed review environment (command execution was blocked), so I relied on static reading of the operation-tree logic against the test cases rather than confirming a green test run directly. The PR description's reported result (42 passed, 0 failed) is plausible given the logic traced through by hand.

Overall: solid, narrowly-scoped analyzer addition with good test coverage and consistent doc/release-tracking updates. No blocking issues found.

[greptile-apps]

Confidence Score: 5/5

[Medium risk] Adds a new code analyzer rule for test setup hooks.

The PR appears safe to merge based on the changes reviewed.

Summary

The PR adds TUnit0075 to warn when a setup hook passes the test execution token to an awaited or returned operation, with tests and documentation. The latest changes extend detection to inherited overrides and conditional returns.

_{Reviews (2) · Last reviewed commit: "fix: detect inherited hooks and conditio..."}

[github-actions]

Review: TUnit0075 — warn on setup hooks passing the test execution token

Re-reviewed against the previous CodeRabbit/greptile/github-actions review comments on this PR. Both previously-raised findings have since been fixed:

• Inherited setup overrides missed (greptile) — IsBeforeTestHook now walks the OverriddenMethod chain and checks attributes at each level, so a [Before(Test)] declared on a base class is correctly picked up by a non-attributed override. Verified against the new Inherited_Before_Hook_With_Test_Token_Shows_Warning cases (direct base, intermediate abstract override, and an override annotated with an unrelated [After(Test)]), plus the negative case confirming new (hiding, not overriding) correctly does not inherit the hook role.
• Conditional returns evade warning (greptile) — IsDirectSetupOperation now climbs through IConditionalOperation (as well as ConfigureAwait and non-user-defined conversions) before checking for the enclosing await/return, including the nested-conditional case (useCache ? ... : (useCache ? ... : Delay(token))).

Design

The core matching strategy is solid: anchoring on the IPropertyReferenceOperation chain for TestContext.Execution.CancellationToken (checked by symbol identity against TUnit.Core.TestContext/TUnit.Core.Interfaces.ITestExecution, not by name) correctly avoids false positives on lookalike types or unrelated TestContext instances — covered by Before_Hook_With_Lookalike_Context_Shows_No_Warning and Before_Hook_With_Unrelated_Context_Shows_No_Warning. The lambda/local-function exclusion (walking parent operations for IAnonymousFunctionOperation/ILocalFunctionOperation) is a reasonable, cheaply-testable way to keep the rule's scope to "directly awaited or returned from the hook body," matching the documented scope.

Extending the existing TimeoutCancellationTokenAnalyzer rather than adding a new analyzer type is a fine call given the shared CancellationToken/timeout domain and that SupportedDiagnostics/registration is centralized there already.

Suggestions (non-blocking)

1. Cache well-known type symbols. AnalyzeSetupArgument is registered on OperationKind.Argument, so it runs for every argument in every invocation across the whole compilation, and on each near-match it re-resolves CancellationToken, ITestExecution, and TestContext via compilation.GetTypeByMetadataName(...) from scratch. The cheap string-based pattern match filters most call sites out first, so this is unlikely to be a real hotspot, but the idiomatic fix is to resolve these three INamedTypeSymbols once per compilation (e.g. via context.RegisterCompilationStartAction, capturing the symbols in a closure passed to the operation action) rather than on every candidate argument.
2. Doc placement sets an unreviewed precedent. docs/docs/reference/tunit0075.md is the only per-diagnostic doc page in the repo — none of the other ~70 shipped TUnit00xx rules have one, and there's no analyzer-rules index it belongs to; it's sandwiched into reference/ next to generic topics like command-line-flags and test-configuration. That's fine as a one-off, but if this is meant to be the template for future rule docs, it'd read better as the first entry in a dedicated "Analyzer Rules" index page rather than a standalone page with no siblings. Not a blocker for this PR, but worth deciding before more rules copy the pattern.

Verification

• Traced the operation-tree logic in TimeoutCancellationTokenAnalyzer.cs by hand against all 33 new test cases in BeforeHookCancellationTokenAnalyzerTests.cs, including the conditional-return and inheritance cases added since the last review pass — the logic matches every asserted warn/no-warn outcome.
• DiagnosticIds.cs, Rules.cs, Resources.resx, and AnalyzerReleases.Unshipped.md entries are consistent with each other and with the file's existing conventions (ID, category, severity).
• Docs contain no C# fenced code blocks (only inline spans), consistent with the PR description's claim that no compiled documentation snippets changed.
• Did not execute dotnet build/dotnet test in this review environment; relying on static reading of the logic against the test suite, as in the prior review pass.

No blocking issues. Good, narrowly-scoped diagnostic with strong test coverage; the two suggestions above are minor polish, not correctness concerns.

[thomhurst]

Thanks!