Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/workflows/workflow.yml
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,8 @@ jobs:
# DevCli ships C# source that consumers compile with their own analyzers.
# Nuru builds it under its own .editorconfig, so RCS1194 (hidden there)
# reached consumers first (tasks 558, 562). The check compiles the
# content as a consumer would, without the repo .editorconfig.
# content as a consumer would, without the repo .editorconfig and with
# CA1812 on (task 573).
if: matrix.stage == 'samples'
working-directory: tests/devcli-consumer-check
run: dotnet build
Expand Down
6 changes: 4 additions & 2 deletions Directory.Build.props
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,11 @@
<!-- Suppress specific warnings -->
<!-- CA1014: CLS compliance not required for this library -->
<!-- CA1724: Type name conflicts (acceptable in this context) -->
<!-- CA1812: False positives for DI-instantiated classes -->
<!-- IL2026, IL2067, IL2070, IL2075, IL3050, IL2104, IL3053: AOT warnings (not yet implemented) -->
<NoWarn>$(NoWarn);CA1014;CA1724;CA1812;IL2026;IL2067;IL2070;IL2075;IL3050;IL2104;IL3053;IDE0290</NoWarn>
<NoWarn>$(NoWarn);CA1014;CA1724;IL2026;IL2067;IL2070;IL2075;IL3050;IL2104;IL3053;IDE0290</NoWarn>
<!-- CA1812: False positives for DI-instantiated classes. tests/devcli-consumer-check sets
NuruConsumerCheck=true to keep it on, as a DevCli consumer has it (task 573). -->
<NoWarn Condition="'$(NuruConsumerCheck)' != 'true'">$(NoWarn);CA1812</NoWarn>
</PropertyGroup>

<!-- Code analyzers applied to all projects -->
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Disposition — task 573

**Date:** 2026-11-14
**Outcome:** clean
**Rounds:** 1
**Final open count:** 0

## Summary

Round 1 (general, effort 2) found no bugs, suggestions, or nits. The generator factory makes the four shipped nested handlers visible to CA1812, the consumer check compiles that content with the rule on and without suppression, and the recorded build, test, and audit results cover the change. The restatement matches the task.

## Exception log (if accepted-exceptions)

None.

## Escalations

- None.
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# Review framework — task 573

**Date:** 2026-11-14
**Host task:** kanban/in-progress/573-devcli-internal-handler-classes-trip-ca1812-in-consumer-projects/
**Diff scope:** branch `task/573-devcli-internal-handler-classes-trip-ca1812-in` vs merge-base `d3f5856c` (master). Product diff is the generator factory, the `NuruConsumerCheck` CA1812 gate, generator-73, and the consumer-check project. Kanban `task.md` is in the same range and is not product code.
**Plan / brief:** DevCli's nested internal `Handler` types are activated only by the mediator, so a consumer with CA1812 as an error cannot compile the shipped content. Make that instantiation visible in generated code, without suppression and without bringing back CA1034 or CA2000, and compile the content with CA1812 on in `tests/devcli-consumer-check`.
**Effort:** 2
**Reviewer roster:** general
**Session IDs:** reviewer grok (Grok 4.7), body tw-implementation-review, print mode. Session id not reported. Turn cap 120; cursor session uncapped (no `--max-turns` flag).

## Budget (by-diff)

- Lines changed: 490
- Effort: 2
- TCB hits: none
- Roster axes: general
- Turn cap: 120 (--max-turns; cursor uncapped)

## Ground rules

- Reviewers are read-only on product code; they write only under `review/round-N/`
- Severity: bug | suggestion | nit — Status starts as open
- Do not invent issues to fill space; zero issues is a valid outcome
- Address the diff and surrounding call sites; re-verify falsifiable claims by reading the diff and the kitchen `## Results` and `### How to validate` record
- Do not re-run a full build, a full test run, an audit, or the hooks; those belong to the implementer's recorded results, the host audit node, and CI
- Exception: one targeted command may check one suspected bug; the finding names the command and the reason. A full build, full test run, or audit is never that command
- A missing or thin recorded result is a finding, not a reason to run the gates
- Prior rounds are immutable; new work goes in `round-(N+1)/`
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# Round 1 — general
**Date:** 2026-11-14
**Scope reviewed:** branch `task/573-devcli-internal-handler-classes-trip-ca1812-in` vs merge-base `d3f5856c` (master). Generator factory emission, consumer-check CA1812 gate, generator-73.

## Summary

The change leaves the shipped DevCli handlers `internal` and gives the Nuru generator an explicit `new` for each nested `[NuruRoute]` handler the Microsoft DI container would build with one public constructor of required parameters. `__UseHandlerFactory` runs immediately after `AddGeneratedMediator` and replaces that mediator registration (non-keyed, implementation type equal to the handler, same lifetime). `tests/devcli-consumer-check` sets `NuruConsumerCheck` before importing root props, so CA1812 stays out of `NoWarn` there, with no `#pragma`, `[SuppressMessage]`, or `<auto-generated/>` in the shipped content. The restatement matches the task, including the requirement to check every shipped content type. Recorded results cover `./bin/dev build`, `./bin/dev test` (2403 passed, generator-73 4/4), `ganda repo audit` for the checks this branch touches, and the consumer-check negative case (CA1812 on all four handlers with factories disabled, clean when restored). No product defect.

## Issues
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Round 1 — merged findings
**Date:** 2026-11-14
**Sources:** general

## Counts

| Severity | open | fixed | wontfix |
|----------|------|-------|---------|
| bug | 0 | 0 | 0 |
| suggestion | 0 | 0 | 0 |
| nit | 0 | 0 | 0 |

## Issues

No issues were raised.

## Duplicates / conflicts

- None. Single reviewer.
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ Acceptance:
- `tests/devcli-consumer-check` compiles the content with CA1812 enabled (regression gate).
- Endpoint dispatch through the generated mediator still works (full test suite green).

## Checklist

- [x] Reproduce: compile DevCli content with CA1812 as error, list every type that fires
- [x] Choose a fix that satisfies CA1812, CA1034 and CA2000 together without suppression
Expand All @@ -63,6 +64,105 @@ Acceptance:
## Session

- Created: b2020e9f-5c06-487a-a0b3-831ae1639f80 (2026-11-11)
- 2026-11-14 implement oracle (ganda task work, implementer-claude): reproduced, fixed in the
generator, gate added. Boy-scout: the first commit tripped `runfile-executable` (new
generator-73 lacked +x) and `analyzer-suppressions` (a NoWarn `.Replace(...)` property function
whose tokens the scanner counted); both fixed on this branch.
- Implement oracle: implemented by implementer-claude (claude, vendor anthropic, model claude-opus-5-5, response model claude-opus-5-5), session 3a16c70d-30c5-45db-b5a9-0061e951ce93, max-turns 500, Done — 2026-11-13T19:59:03Z
- Review oracle round 1: Grok 4.7 (grok, vendor xai), body tw-implementation-review, effort 2, roster general, turn cap 120 (cursor session uncapped; no --max-turns flag), session not reported, disposition clean — 2026-11-14. No findings. The restatement matches the task; no misread. See `review/round-1/merged.md` and `review/disposition.md`.
- Review oracle: review by reviewer-cursor (cursor, vendor xai, model grok-4.7-high), session not reported, max-turns uncapped (no --max-turns flag; bound 120), artifacts general.md, merged.md, disposition.md — 2026-11-13T20:08:28Z

## Results

### Reproduction

With CA1812 enabled in `tests/devcli-consumer-check`, exactly four shipped content types fired,
the four the description lists: `CheckVersionCommand.Handler`, `CleanCommand.Handler`,
`ReleaseCommand.Handler`, `SelfInstallCommand.Handler`. No service or other content type fired;
the generator already builds the content services and commands with `new`.

### Cause

The TimeWarp.Mediator generator registers each nested handler as `AddScoped<Handler>(services)`.
The container builds it by reflection, so no `new Handler(...)` appears in the compilation.

### Fix (generator, no change to the shipped content)

- `EndpointExtractor.IsFactoryConstructible` flags a nested `[NuruRoute]` Handler declared in
this compilation, not abstract or generic, with exactly one public constructor whose
parameters are all required (no default, `params` or `ref`). Model field:
`HandlerDefinition.NestedHandlerFactoryConstructible`.
- `interceptor-emitter.handler-factories.cs` emits `__UseHandlerFactory<THandler>` and, right
after `AddGeneratedMediator(services)` in both the source-gen mediator container and the
runtime-DI container, one call per flagged handler:
`__UseHandlerFactory<H>(services, static sp => new H(GetRequiredService<T1>(sp), ...))`. It
replaces the mediator's own descriptor with the same lifetime (it only swaps a non-keyed
descriptor whose implementation type is the handler, and adds nothing otherwise). That matches
how the container builds a single-constructor type, so dispatch does not change. Registrations
made later (a runtime-DI app's `ConfigureServices`) still win. The explicit `new` in the
generated code is what CA1812 counts.
- Handlers the container could build another way (several public constructors, optional
arguments) keep the reflection registration.
- Shipped content is unchanged: still `internal sealed class Handler` (no CA1034), no pragma,
`[SuppressMessage]` or `<auto-generated/>`. The consumer check also runs CA2000 and CA1034
(AnalysisMode All, warnings as errors) and passes.

### Regression gate

- Root `Directory.Build.props` adds CA1812 to NoWarn only when `NuruConsumerCheck` is not
`true` (same per-file suppression count). `tests/devcli-consumer-check/Directory.Build.props`
sets `NuruConsumerCheck=true` before its import, so the check (CI samples stage,
`workflow.yml` "DevCli consumer check") compiles the content with CA1812 on.
- Negative check: with the factory emission disabled, the consumer check fails with CA1812 on
all four handlers; restored, it passes.
- `tests/timewarp-nuru-tests/generator/generator-73-nested-handler-factories.cs` (Roslyn-hosted,
standalone list + CiTestExcludes): factory emitted with resolved arguments, emitted after
`AddGeneratedMediator`, a zero-dependency handler uses `static _`, skipped for two
constructors and for an optional argument, none without a generated mediator.

### Gates

- `./bin/dev build`: passed (0 warnings, 0 errors).
- `./bin/dev test`: passed (exit 0; 2403 PASSED, 0 FAILED; generator-73 4/4 in standalone).
- `ganda repo audit`: `analyzer-suppressions` and every check this branch touches pass. The
run before the fix commit failed `runfile-executable` on generator-73 (index lacked +x, fixed
with `git add --chmod=+x`). It also reported `kanban-landed` for task 575, which is not this
task: 575 is a live `ganda task work-set` walk (into `feature/overnight-2026-11-14`) whose
claimed kitchen has not landed yet. That walk's host owns that kitchen, so this branch leaves it alone.
- `vscode-window-icon` is a Warning (missing local `.vscode/settings.json`, ignored file), not
blocking.

### How to validate

Smoke:

```bash
(cd tests/devcli-consumer-check && dotnet build)
grep -n "__UseHandlerFactory" artifacts/generated/devcli-consumer-check/TimeWarp.Nuru.Analyzers/*/NuruGenerated.g.cs
dotnet run tests/timewarp-nuru-tests/generator/generator-73-nested-handler-factories.cs
dotnet tests/devcli-consumer-check/bin/Debug/net11.0/devcli-consumer-check.dll check-version --package TimeWarp.Nuru
```

Expect: the consumer check prints `Build succeeded.` with no CA1812 (CA1812 is on there); the
generated file has the helper plus four `__UseHandlerFactory<global::DevCli.*.Handler>` lines
after `AddGeneratedMediator(services);`; generator-73 passes 4/4; the check-version run
dispatches through the factory-built handler and prints the version report (currently
"4.0.0-beta.13 was already released").

### Review

Round 1 (2026-11-14): tw-implementation-review, effort 2, roster general, Grok 4.7, turn cap 120 (cursor uncapped; no `--max-turns` flag), session not reported. Disposition clean. Counts: bug 0 open / 0 fixed / 0 wontfix; suggestion 0; nit 0. The restatement matches the task; no misread. Paths: `review/review-framework.md`, `review/round-1/general.md`, `review/round-1/merged.md`, `review/disposition.md`.

### Follow-ups

- Ships in the next DevCli beta (checklist item left open). Then multiavatar can drop
`tools/dev-cli/endpoints/suppressions.cs` and amuru its CA1812 baseline entries.
- Mediator handlers that are not nested in a `[NuruRoute]` class (`Map<T>` with an external
handler) still use reflection registration. No DevCli content uses that shape.

## Review

- Round 1: Done

## Notes

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,22 @@ .. model.Apps
string? currentAppLoggerFactory = model.Apps.Any(HasLoggerFactorySource) ? LoggerExpressions.CurrentAppFactory : null;
ImmutableArray<BehaviorDefinition> allBehaviors = [.. model.AllBehaviors];
ImmutableArray<ServiceDefinition> allServices = [.. model.AllServices];
EmitServiceFields(sb, sourceGenServices, model.SourceGenDIRequiresMediator, model.GeneratedMediatorNamespace, currentAppLoggerFactory, sourceGenHttpClients, allBehaviors, allServices);
// Kanban 573: nested handlers get explicit constructor factories in every container that
// holds the generated mediator, so CA1812 sees them instantiated.
ImmutableArray<HandlerDefinition> handlerFactories = model.GeneratedMediatorNamespace is null
? []
: CollectHandlerFactories(model);
bool emitsMediatorContainer = model.SourceGenDIRequiresMediator || model.UsesMicrosoftDependencyInjection;
if (handlerFactories.Length > 0 && emitsMediatorContainer)
EmitHandlerFactoryHelper(sb);

EmitServiceFields(sb, sourceGenServices, model.SourceGenDIRequiresMediator, model.GeneratedMediatorNamespace, currentAppLoggerFactory, sourceGenHttpClients, allBehaviors, allServices, handlerFactories);

// Emit runtime DI infrastructure for apps that use UseMicrosoftDependencyInjection()
// Each app gets its own __ConfigureServices and GetServiceProvider methods
if (model.UsesMicrosoftDependencyInjection)
{
EmitRuntimeDIInfrastructure(sb, model.Apps, model.GeneratedMediatorNamespace);
EmitRuntimeDIInfrastructure(sb, model.Apps, model.GeneratedMediatorNamespace, handlerFactories);
}

EmitLoggingFactoryFields(sb, model);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
#region Purpose
// InterceptorEmitter partial: explicit constructor factories for nested [NuruRoute] handlers
// in the containers that hold the TimeWarp.Mediator generated mediator.
#endregion
#region Design
// The TimeWarp.Mediator generator registers each handler as AddScoped<Handler>(services), so the
// container builds it by reflection and the compiler never sees `new Handler(...)`. An internal
// handler then trips CA1812 in any consumer that compiles it with the rule on: TimeWarp.Nuru.DevCli
// ships its endpoints as source, and every repo taking the generated dev CLI hit it (kanban 573).
//
// After AddGeneratedMediator(services) the generator swaps each such descriptor for one with the
// same service type and lifetime whose factory calls the handler's constructor, resolving every
// argument with GetRequiredService. That is how the container activates a single-constructor type
// with required parameters, so behavior does not change, and CA1812 counts the instantiation.
// __UseHandlerFactory replaces only a descriptor the mediator added (non-keyed, implementation type
// is the handler); with none it adds nothing. Registrations made after it (a runtime-DI app's own
// ConfigureServices) still win. Handlers the container could build differently (several public
// constructors, optional or params arguments) keep the reflection registration; see
// EndpointExtractor.IsFactoryConstructible.
#endregion

namespace TimeWarp.Nuru.Generators;

using System.Text;

internal static partial class InterceptorEmitter
{
private const string HandlerFactoryHelper = "__UseHandlerFactory";

/// <summary>
/// Nested endpoint handlers the generator can construct with an explicit factory, one per type.
/// </summary>
private static ImmutableArray<HandlerDefinition> CollectHandlerFactories(GeneratorModel model) =>
[
.. model.AllRoutes
.Select(static route => route.Handler)
.Where(static handler => handler.HandlerKind == HandlerKind.Command
&& handler.NestedHandlerTypeName is not null
&& handler.NestedHandlerFactoryConstructible)
.DistinctBy(static handler => handler.NestedHandlerTypeName, StringComparer.Ordinal)
.OrderBy(static handler => handler.NestedHandlerTypeName, StringComparer.Ordinal)
];

/// <summary>
/// Emits <c>__UseHandlerFactory&lt;THandler&gt;</c>, which swaps a reflection-activated handler
/// registration for a factory registration with the same lifetime.
/// </summary>
private static void EmitHandlerFactoryHelper(StringBuilder sb)
{
const string Di = "global::Microsoft.Extensions.DependencyInjection";

sb.AppendLine(" // Swaps the generated mediator's reflection-activated handler registration for an explicit");
sb.AppendLine(" // constructor call with the same lifetime, so code analysis (CA1812) sees the handler built.");
sb.AppendLine($" private static void {HandlerFactoryHelper}<THandler>({Di}.IServiceCollection services, global::System.Func<global::System.IServiceProvider, THandler> factory)");
sb.AppendLine(" where THandler : class");
sb.AppendLine(" {");
sb.AppendLine(" for (int i = services.Count - 1; i >= 0; i--)");
sb.AppendLine(" {");
sb.AppendLine($" {Di}.ServiceDescriptor descriptor = services[i];");
sb.AppendLine(" if (descriptor.ServiceType != typeof(THandler) || descriptor.IsKeyedService || descriptor.ImplementationType != typeof(THandler))");
sb.AppendLine(" continue;");
sb.AppendLine();
sb.AppendLine($" services[i] = new {Di}.ServiceDescriptor(typeof(THandler), factory, descriptor.Lifetime);");
sb.AppendLine(" return;");
sb.AppendLine(" }");
sb.AppendLine(" }");
sb.AppendLine();
}

/// <summary>
/// Emits one <c>__UseHandlerFactory</c> call per handler. Call it right after the line that
/// registers the generated mediator into <c>services</c>.
/// </summary>
private static void EmitHandlerFactoryRegistrations(StringBuilder sb, ImmutableArray<HandlerDefinition> handlers, string indent)
{
const string Resolve = "global::Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService";

foreach (HandlerDefinition handler in handlers)
{
ImmutableArray<ParameterBinding> dependencies = handler.ConstructorDependencies;
string lambdaParameter = dependencies.Length == 0 ? "_" : "sp";
string arguments = string.Join(", ", dependencies.Select(static dependency => $"{Resolve}<{dependency.ParameterTypeName}>(sp)"));

sb.AppendLine($"{indent}{HandlerFactoryHelper}<{handler.NestedHandlerTypeName}>(services, static {lambdaParameter} => new {handler.NestedHandlerTypeName}({arguments}));");
}
}
}
Loading
Loading