Skip to content

Better validation #25

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
TopSwagCode merged 1 commit into from
Feb 12, 2026
Merged

Better validation #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion .claude/settings.local.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
"permissions": {
"allow": [
"Bash(dotnet test:*)",
"Bash(dotnet build:*)"
"Bash(dotnet build:*)",
"Bash(ls:*)",
"Bash(dotnet publish:*)"
Comment on lines +5 to +7

Copilot AI Feb 12, 2026

Copy link

Choose a reason for hiding this comment

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

This PR is titled "Better validation", but this change expands Claude tool permissions (ls/publish). If this is intentional, it should be called out in the PR description; otherwise consider moving it to a separate PR to keep the scope focused.

Copilot uses AI. Check for mistakes.
]
}
}
21 changes: 21 additions & 0 deletions src/MinimalWorker.Generators/InvocationModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System.Collections.Generic;
using Microsoft.CodeAnalysis;

namespace MinimalWorker.Generators;

Expand All @@ -15,6 +16,26 @@ internal sealed class WorkerInvocationModel
public string ReturnType { get; set; } = "Task";
public WorkerType Type { get; set; }
public string? ScheduleArgument { get; set; } // TimeSpan or cron expression

/// <summary>
/// Diagnostics collected during analysis. Reported during code generation.
/// </summary>
public List<DiagnosticInfo> Diagnostics { get; set; } = new();

/// <summary>
/// Indicates whether the model has errors that prevent code generation.
/// </summary>
public bool HasErrors { get; set; }
}

/// <summary>
/// Represents a diagnostic to be reported during code generation.
/// </summary>
internal sealed class DiagnosticInfo
{
public DiagnosticDescriptor Descriptor { get; set; } = null!;
public Location Location { get; set; } = Location.None;
public object?[]? MessageArgs { get; set; }
}

internal sealed class ParameterModel
Expand Down
66 changes: 25 additions & 41 deletions src/MinimalWorker.Generators/WorkerEmitter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,18 @@ private static void EmitContinuousWorkerInit(StringBuilder sb, WorkerInvocationM
sb.AppendLine();
sb.AppendLine(" // Start worker immediately - host has already started");
sb.AppendLine(" var token = lifetime.ApplicationStopping;");
sb.AppendLine(" var workerId = registration.Id.ToString();");
sb.AppendLine(" var workerName = registration.DisplayName;");
sb.AppendLine();
sb.AppendLine(" // Cache service lookups at initialization (outside Task.Run)");
sb.AppendLine(" var workerLogger = host.Services.GetService<ILoggerFactory>()?.CreateLogger($\"MinimalWorker.{workerName}\");");
sb.AppendLine();
sb.AppendLine(" // Register worker for status tracking");
sb.AppendLine(" MinimalWorkerObservability.RegisterWorker(workerId, workerName, \"continuous\");");
sb.AppendLine();
sb.AppendLine(" // Log worker started");
sb.AppendLine(" if (workerLogger != null) WorkerLogMessages.WorkerStarted(workerLogger, workerName, \"continuous\", workerId, null);");
sb.AppendLine();
sb.AppendLine(" _ = Task.Run(async () =>");
sb.AppendLine(" {");
sb.AppendLine(" using var scope = host.Services.CreateScope();");
Expand All @@ -413,18 +425,6 @@ private static void EmitContinuousWorkerInit(StringBuilder sb, WorkerInvocationM
}
}

sb.AppendLine();
sb.AppendLine(" var workerId = registration.Id.ToString();");
sb.AppendLine(" var workerName = registration.DisplayName;");
sb.AppendLine();
sb.AppendLine(" // Get logger for structured logging");
sb.AppendLine(" var workerLogger = scope.ServiceProvider.GetService<ILoggerFactory>()?.CreateLogger($\"MinimalWorker.{workerName}\");");
sb.AppendLine();
sb.AppendLine(" // Register worker for status tracking");
sb.AppendLine(" MinimalWorkerObservability.RegisterWorker(workerId, workerName, \"continuous\");");
sb.AppendLine();
sb.AppendLine(" // Log worker started");
sb.AppendLine(" if (workerLogger != null) WorkerLogMessages.WorkerStarted(workerLogger, workerName, \"continuous\", workerId, null);");
sb.AppendLine();
sb.AppendLine(" var tags = new TagList");
sb.AppendLine(" {");
Expand Down Expand Up @@ -472,13 +472,9 @@ private static void EmitContinuousWorkerInit(StringBuilder sb, WorkerInvocationM
sb.AppendLine(" activity?.SetStatus(ActivityStatusCode.Error, ex.Message);");
sb.AppendLine(" activity?.RecordException(ex);");
sb.AppendLine();
sb.AppendLine(" var errorTags = new TagList");
sb.AppendLine(" {");
sb.AppendLine(" { \"worker.id\", workerId },");
sb.AppendLine(" { \"worker.name\", workerName },");
sb.AppendLine(" { \"worker.type\", \"continuous\" },");
sb.AppendLine(" { \"exception.type\", ex.GetType().FullName }");
sb.AppendLine(" };");
sb.AppendLine(" // Copy base tags struct and add exception type (avoids full allocation in catch)");
sb.AppendLine(" var errorTags = tags;");
sb.AppendLine(" errorTags.Add(\"exception.type\", ex.GetType().FullName);");
sb.AppendLine(" MinimalWorkerObservability.ErrorCounter.Add(1, errorTags);");
sb.AppendLine(" MinimalWorkerObservability.RecordFailure(workerId);");
sb.AppendLine();
Expand Down Expand Up @@ -533,8 +529,9 @@ private static void EmitPeriodicWorkerInit(StringBuilder sb, WorkerInvocationMod
sb.AppendLine(" var workerName = registration.DisplayName;");
sb.AppendLine(" var token = lifetime.ApplicationStopping;");
sb.AppendLine();
sb.AppendLine(" // Get logger for structured logging");
sb.AppendLine(" // Cache service lookups at initialization (outside Task.Run)");
sb.AppendLine(" var workerLogger = host.Services.GetService<ILoggerFactory>()?.CreateLogger($\"MinimalWorker.{workerName}\");");
sb.AppendLine(" var timeProvider = host.Services.GetService<TimeProvider>() ?? TimeProvider.System;");
sb.AppendLine();
sb.AppendLine(" // Register worker for status tracking");
sb.AppendLine(" MinimalWorkerObservability.RegisterWorker(workerId, workerName, \"periodic\");");
Expand All @@ -553,9 +550,6 @@ private static void EmitPeriodicWorkerInit(StringBuilder sb, WorkerInvocationMod
sb.AppendLine(" };");
sb.AppendLine(" var scheduleString = schedule.ToString();");
sb.AppendLine();
sb.AppendLine(" // Resolve TimeProvider from DI, fallback to system time");
sb.AppendLine(" var timeProvider = host.Services.GetService<TimeProvider>() ?? TimeProvider.System;");
sb.AppendLine();
sb.AppendLine(" try");
sb.AppendLine(" {");
sb.AppendLine(" using var timer = new PeriodicTimer(schedule, timeProvider);");
Expand Down Expand Up @@ -618,13 +612,9 @@ private static void EmitPeriodicWorkerInit(StringBuilder sb, WorkerInvocationMod
sb.AppendLine(" activity?.SetStatus(ActivityStatusCode.Error, ex.Message);");
sb.AppendLine(" activity?.RecordException(ex);");
sb.AppendLine();
sb.AppendLine(" var errorTags = new TagList");
sb.AppendLine(" {");
sb.AppendLine(" { \"worker.id\", workerId },");
sb.AppendLine(" { \"worker.name\", workerName },");
sb.AppendLine(" { \"worker.type\", \"periodic\" },");
sb.AppendLine(" { \"exception.type\", ex.GetType().FullName }");
sb.AppendLine(" };");
sb.AppendLine(" // Copy base tags struct and add exception type (avoids full allocation in catch)");
sb.AppendLine(" var errorTags = tags;");
sb.AppendLine(" errorTags.Add(\"exception.type\", ex.GetType().FullName);");
sb.AppendLine(" MinimalWorkerObservability.ErrorCounter.Add(1, errorTags);");
sb.AppendLine(" MinimalWorkerObservability.RecordFailure(workerId);");
sb.AppendLine();
Expand Down Expand Up @@ -689,8 +679,9 @@ private static void EmitCronWorkerInit(StringBuilder sb, WorkerInvocationModel w
sb.AppendLine(" var workerName = registration.DisplayName;");
sb.AppendLine(" var token = lifetime.ApplicationStopping;");
sb.AppendLine();
sb.AppendLine(" // Get logger for structured logging");
sb.AppendLine(" // Cache service lookups at initialization (outside Task.Run)");
sb.AppendLine(" var workerLogger = host.Services.GetService<ILoggerFactory>()?.CreateLogger($\"MinimalWorker.{workerName}\");");
sb.AppendLine(" var timeProvider = host.Services.GetService<TimeProvider>() ?? TimeProvider.System;");
sb.AppendLine();
sb.AppendLine(" // Register worker for status tracking");
sb.AppendLine(" MinimalWorkerObservability.RegisterWorker(workerId, workerName, \"cron\");");
Expand All @@ -708,9 +699,6 @@ private static void EmitCronWorkerInit(StringBuilder sb, WorkerInvocationModel w
sb.AppendLine(" { \"worker.type\", \"cron\" }");
sb.AppendLine(" };");
sb.AppendLine();
sb.AppendLine(" // Resolve TimeProvider from DI, fallback to system time");
sb.AppendLine(" var timeProvider = host.Services.GetService<TimeProvider>() ?? TimeProvider.System;");
sb.AppendLine();
sb.AppendLine(" try");
sb.AppendLine(" {");
sb.AppendLine(" while (!token.IsCancellationRequested)");
Expand Down Expand Up @@ -785,13 +773,9 @@ private static void EmitCronWorkerInit(StringBuilder sb, WorkerInvocationModel w
sb.AppendLine(" activity?.SetStatus(ActivityStatusCode.Error, ex.Message);");
sb.AppendLine(" activity?.RecordException(ex);");
sb.AppendLine();
sb.AppendLine(" var errorTags = new TagList");
sb.AppendLine(" {");
sb.AppendLine(" { \"worker.id\", workerId },");
sb.AppendLine(" { \"worker.name\", workerName },");
sb.AppendLine(" { \"worker.type\", \"cron\" },");
sb.AppendLine(" { \"exception.type\", ex.GetType().FullName }");
sb.AppendLine(" };");
sb.AppendLine(" // Copy base tags struct and add exception type (avoids full allocation in catch)");
sb.AppendLine(" var errorTags = tags;");
sb.AppendLine(" errorTags.Add(\"exception.type\", ex.GetType().FullName);");
sb.AppendLine(" MinimalWorkerObservability.ErrorCounter.Add(1, errorTags);");
sb.AppendLine(" MinimalWorkerObservability.RecordFailure(workerId);");
sb.AppendLine();
Expand Down
45 changes: 38 additions & 7 deletions src/MinimalWorker.Generators/WorkerGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,10 +139,11 @@ private static bool IsWorkerInvocation(SyntaxNode node)
private static bool AnalyzeDelegate(GeneratorSyntaxContext context, ExpressionSyntax delegateExpression, WorkerInvocationModel model)
{
var typeInfo = context.SemanticModel.GetTypeInfo(delegateExpression);

var location = delegateExpression.GetLocation();

// Try to get the actual delegate type, not just the converted type
INamedTypeSymbol? delegateType = null;

if (typeInfo.Type is INamedTypeSymbol namedType && namedType.DelegateInvokeMethod != null)
{
delegateType = namedType;
Expand All @@ -151,7 +152,7 @@ private static bool AnalyzeDelegate(GeneratorSyntaxContext context, ExpressionSy
{
delegateType = convertedNamedType;
}

if (delegateType == null)
return false;

Expand All @@ -167,7 +168,7 @@ private static bool AnalyzeDelegate(GeneratorSyntaxContext context, ExpressionSy
{
Name = param.Name,
Type = param.Type.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat),
IsCancellationToken = param.Type.Name == "CancellationToken" &&
IsCancellationToken = param.Type.Name == "CancellationToken" &&
param.Type.ContainingNamespace?.ToDisplayString() == "System.Threading"
};

Expand All @@ -177,20 +178,34 @@ private static bool AnalyzeDelegate(GeneratorSyntaxContext context, ExpressionSy
model.Parameters.Add(paramModel);
}

// Validate only one CancellationToken
// Validate only one CancellationToken - report MW0001
if (cancellationTokenCount > 1)
{
model.Diagnostics.Add(new DiagnosticInfo
{
Descriptor = Diagnostics.MultipleCancellationTokens,
Location = location
});
model.HasErrors = true;
return false;
}

// Determine return type
var returnType = invokeMethod.ReturnType;
model.IsAsync = returnType.Name == "Task" || returnType.Name == "ValueTask";
model.ReturnType = returnType.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat);

// Ensure Task or void for now (simplification)
// Ensure Task or void for now (simplification) - report MW0002
if (model.ReturnType != "void" &&
!model.ReturnType.Contains("Task") &&
!model.ReturnType.Contains("ValueTask"))
{
model.Diagnostics.Add(new DiagnosticInfo
{
Descriptor = Diagnostics.InvalidReturnType,
Location = location
});
model.HasErrors = true;
return false;
}

Expand All @@ -207,6 +222,21 @@ private static bool AnalyzeDelegate(GeneratorSyntaxContext context, ExpressionSy

private static void Execute(SourceProductionContext context, ImmutableArray<WorkerInvocationModel> workers)
{
// Report all collected diagnostics first
foreach (var worker in workers)
{
if (worker?.Diagnostics != null)
{
foreach (var diag in worker.Diagnostics)
{
context.ReportDiagnostic(Diagnostic.Create(
diag.Descriptor,
diag.Location,
diag.MessageArgs ?? System.Array.Empty<object>()));
}
Comment on lines +226 to +236

Copilot AI Feb 12, 2026

Copy link

Choose a reason for hiding this comment

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

This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.

Suggested change
foreach (var worker in workers)
{
if (worker?.Diagnostics != null)
{
foreach (var diag in worker.Diagnostics)
{
context.ReportDiagnostic(Diagnostic.Create(
diag.Descriptor,
diag.Location,
diag.MessageArgs ?? System.Array.Empty<object>()));
}
foreach (var worker in workers.Where(w => w?.Diagnostics != null))
{
foreach (var diag in worker.Diagnostics!)
{
context.ReportDiagnostic(Diagnostic.Create(
diag.Descriptor,
diag.Location,
diag.MessageArgs ?? System.Array.Empty<object>()));

Copilot uses AI. Check for mistakes.
}
}

// Always generate a marker file to confirm generator is running
var markerSource = $@"// <auto-generated/>
// MinimalWorker Source Generator
Expand All @@ -231,7 +261,8 @@ internal static class GeneratorMarker
return;
}

var validWorkers = workers.Where(w => w != null).ToList();
// Filter out workers with errors - they should not have code generated
var validWorkers = workers.Where(w => w != null && !w.HasErrors).ToList();
if (validWorkers.Count == 0)
return;

Expand Down
6 changes: 6 additions & 0 deletions src/MinimalWorker/BackgroundWorkerExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,9 @@ public static IWorkerBuilder RunBackgroundWorker(this IHost host, Delegate actio
/// </example>
public static IWorkerBuilder RunPeriodicBackgroundWorker(this IHost host, TimeSpan timespan, Delegate action)
{
if (timespan <= TimeSpan.Zero)
throw new ArgumentOutOfRangeException(nameof(timespan), timespan, "TimeSpan must be greater than zero.");

var id = System.Threading.Interlocked.Increment(ref _registrationCounter);
var parameters = action.Method.GetParameters();
var signature = string.Join(",", parameters.Select(p => FormatTypeName(p.ParameterType)));
Expand Down Expand Up @@ -343,6 +346,9 @@ public static IWorkerBuilder RunPeriodicBackgroundWorker(this IHost host, TimeSp
/// </example>
public static IWorkerBuilder RunCronBackgroundWorker(this IHost host, string cronExpression, Delegate action)
{
if (string.IsNullOrWhiteSpace(cronExpression))
throw new ArgumentException("Cron expression cannot be null or empty.", nameof(cronExpression));

Copilot AI Feb 12, 2026

Copy link

Choose a reason for hiding this comment

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

The validation uses string.IsNullOrWhiteSpace(cronExpression) but the thrown message says "null or empty". This is inaccurate for whitespace-only inputs; update the message to mention whitespace (or change the guard to IsNullOrEmpty to match the message).

Suggested change
throw new ArgumentException("Cron expression cannot be null or empty.", nameof(cronExpression));
throw new ArgumentException("Cron expression cannot be null, empty, or whitespace.", nameof(cronExpression));

Copilot uses AI. Check for mistakes.

var id = System.Threading.Interlocked.Increment(ref _registrationCounter);
var parameters = action.Method.GetParameters();
var signature = string.Join(",", parameters.Select(p => FormatTypeName(p.ParameterType)));
Expand Down
23 changes: 23 additions & 0 deletions test/MinimalWorker.Test/CronWorkerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,4 +272,27 @@ public async Task CronBackgroundWorker_With_Invalid_Expression_Should_Fail_Fast(
$"Expected either error about invalid cron expression or worker should not execute. " +
$"Worker executed: {workerExecuted}. Logs:\n{errorOutput}");
}

[Theory]
[InlineData(null)]
[InlineData("")]
[InlineData(" ")]
public void CronBackgroundWorker_With_Empty_Expression_Should_Throw_ArgumentException(string? cronExpression)
{
// Arrange
BackgroundWorkerExtensions.ClearRegistrations();

using var host = Host.CreateDefaultBuilder().Build();

// Act & Assert - Empty cron expression should throw ArgumentException
var exception = Assert.Throws<ArgumentException>(() =>
{
host.RunCronBackgroundWorker(cronExpression!, (CancellationToken token) =>
{
return Task.CompletedTask;
});
});

Assert.Equal("cronExpression", exception.ParamName);
}
}
37 changes: 23 additions & 14 deletions test/MinimalWorker.Test/PeriodicWorkerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,33 +177,42 @@ public async Task PeriodicWorker_Should_Not_Overlap_Executions()
}

[Fact]
public async Task PeriodicWorker_With_Zero_Interval_Should_Handle_Gracefully()
public void PeriodicWorker_With_Zero_Interval_Should_Throw_ArgumentOutOfRangeException()
{
// TODO: Perhaps we should fire an exception here :)
// Arrange
BackgroundWorkerExtensions.ClearRegistrations();

using var host = Host.CreateDefaultBuilder().Build();

// Act - TimeSpan.Zero is handled gracefully by the library
// The worker starts but the PeriodicTimer never fires with zero interval
var exception = await Record.ExceptionAsync(async () =>
// Act & Assert - TimeSpan.Zero should throw ArgumentOutOfRangeException
var exception = Assert.Throws<ArgumentOutOfRangeException>(() =>
{
host.RunPeriodicBackgroundWorker(TimeSpan.Zero, (CancellationToken token) =>
{
return Task.CompletedTask;
});
});

Assert.Equal("timespan", exception.ParamName);
}

[Fact]
public void PeriodicWorker_With_Negative_Interval_Should_Throw_ArgumentOutOfRangeException()
{
// Arrange
BackgroundWorkerExtensions.ClearRegistrations();

using var host = Host.CreateDefaultBuilder().Build();

await host.StartAsync();
await Task.Delay(50);
await host.StopAsync();
// Act & Assert - Negative TimeSpan should throw ArgumentOutOfRangeException
var exception = Assert.Throws<ArgumentOutOfRangeException>(() =>
{
host.RunPeriodicBackgroundWorker(TimeSpan.FromSeconds(-5), (CancellationToken token) =>
{
return Task.CompletedTask;
});
});

// Assert - Library handles zero interval gracefully without crashing
// The worker registers and starts but the periodic timer doesn't fire
// This documents the actual behavior: no exception thrown, worker doesn't execute
Assert.Null(exception);
// Note: With TimeSpan.Zero, PeriodicTimer never fires, so execution count is 0
// This is the expected graceful handling behavior
Assert.Equal("timespan", exception.ParamName);
}
}
Loading