Detect common module authoring mistakes

[thomhurst]

Summary

• add seven independently configurable analyzer rules for unregistered/non-public modules, async void, blocking task access, Thread.Sleep, unflowed cancellation, and duplicate DependsOn declarations
• track generic, params-Type, assembly-wide, instance, factory, and ServiceDescriptor module registrations across reachable code
• avoid cancellation and registration false positives across overloads, callbacks, fields/properties, and conditional flow
• include Analyzers: renumber to a single MP#### ID family, add HelpLinkUri, generate per-rule docs #3312's analyzer-ID normalization, generated help links, and per-rule documentation from merged stacked PR Unify analyzer diagnostic IDs and documentation #3437

Breaking change

This PR also carries #3312. It renumbers the ten previously shipped analyzer IDs to the MP#### family. Existing #pragma warning disable <old-id> and dotnet_diagnostic.<old-id>.severity entries must migrate to the new IDs; for example, MPDEP002 becomes MP0005. The release metadata and analyzer migration guide record every mapping.

Validation

• focused analyzer registration and async-safety regressions pass
• analyzer solution/project builds succeed
• generated analyzer documentation drift check passes
• changed-file formatting verified

Closes #3314

[claude]

Code review

Reviewed the new ModuleAuthoringAnalyzer (7 new rules: MPREG001, MPASYNC001-004, MPTYPE001, MPDEP004), its tests, and the release notes update. I didn't find any correctness bugs — the cancellation-token overload matching, duplicate-DependsOn grouping, and registration-tracking logic all check out against the test cases, and I confirmed there's no overlap with the existing MPDEP002 (circular dependency) rule. Two architectural inconsistencies with the rest of the analyzer project are worth addressing:

1. Bypasses the shared DiagnosticDescriptorFactory / resource-localization pattern. Every other analyzer in this project (SelfDependencyAnalyzer, StatefulModuleAnalyzer, AsyncModuleAnalyzer, ConflictingDependsOnAttributeAnalyzer, etc. — all 10 existing files) builds its DiagnosticDescriptor via DiagnosticDescriptorFactory.Create(...), sourcing title/message/description from Resources.resx for localization. This PR's private https://github.com/thomhurst/ModularPipelines/blob/f1527707c745c289983e9ad0d2ca387b19aeaea1/src/ModularPipelines.Analyzers/ModularPipelines.Analyzers/ModuleAuthoringAnalyzer.cs#L743-L757 helper instead hardcodes English string literals directly and drops the description field entirely. This is the only analyzer in the project that doesn't go through the shared factory, so it silently breaks localization support for these 7 new diagnostics and reintroduces the boilerplate the factory exists to eliminate. Recommend routing the 7 new descriptors through DiagnosticDescriptorFactory.Create with resource entries, consistent with e.g.

   ModularPipelines/src/ModularPipelines.Analyzers/ModularPipelines.Analyzers/ConflictingDependsOnAttributeAnalyzer.cs

   Lines 17 to 20 in f152770

   	public static DiagnosticDescriptor Rule { get; } = DiagnosticDescriptorFactory.Create(
   	DiagnosticId,
   	nameof(Resources.ConflictingDependsOnAttributeAnalyzerTitle),
   	nameof(Resources.ConflictingDependsOnAttributeAnalyzerMessageFormat),

   .

2. Single class owns 7 unrelated diagnostics. The project's established convention is one analyzer class per rule/concern — SelfDependencyAnalyzer, StatefulModuleAnalyzer, AsyncModuleAnalyzer, InvalidDependsOnTypeAnalyzer, MissingDependsOnAttributeAnalyzer, etc. are all single-purpose.

   ModularPipelines/src/ModularPipelines.Analyzers/ModularPipelines.Analyzers/ModuleAuthoringAnalyzer.cs

   Lines 294 to 360 in f152770

   	{
   	return;
   	}
   	var registered = registeredModules.ToImmutableHashSet<INamedTypeSymbol>(
   	SymbolEqualityComparer.Default);
   	foreach (var module in modules.Distinct<INamedTypeSymbol>(SymbolEqualityComparer.Default))
   	{
   	if (registered.Contains(module))
   	{
   	continue;
   	}
   	var location = module.Locations.FirstOrDefault(static item => item.IsInSource);
   	if (location is not null)
   	{
   	context.ReportDiagnostic(Diagnostic.Create(
   	UnregisteredModuleRule,
   	location,
   	module.Name));
   	}
   	}
   	}
   	private static void ReportDuplicateDependencies(
   	SymbolAnalysisContext context,
   	INamedTypeSymbol module)
   	{
   	var dependencies = module.GetAttributes()
   	.Select(attribute => new
   	{
   	Attribute = attribute,
   	Type = GetDependencyType(attribute, context.Compilation),
   	})
   	.Where(static item => item.Type is not null)
   	.GroupBy(static item => item.Type!, SymbolEqualityComparer.Default);
   	foreach (var duplicates in dependencies.Where(static group => group.Skip(1).Any()))
   	{
   	foreach (var duplicate in duplicates.Skip(1))
   	{
   	var location = duplicate.Attribute.ApplicationSyntaxReference?.GetSyntax(
   	context.CancellationToken).GetLocation();
   	if (location is not null)
   	{
   	context.ReportDiagnostic(Diagnostic.Create(
   	DuplicateDependsOnRule,
   	location,
   	module.Name,
   	duplicates.Key!.Name));
   	}
   	}
   	}
   	}
   	private static ITypeSymbol? GetDependencyType(
   	AttributeData attribute,
   	Compilation compilation)
   	{
   	if (attribute.AttributeClass is not { } attributeClass
   	|| !attributeClass.IsDependsOnAttribute(compilation))
   	{
   	return null;
   	}
   	return attributeClass.IsGenericType
   	? attributeClass.TypeArguments[0]

   instead bundles module registration, async-void detection, blocking-call detection, cancellation-token flow analysis, Thread.Sleep detection, public-accessibility checks, and duplicate-DependsOn detection into one 476-line, 476+ line class with 7 independent rule IDs. Splitting these into focused analyzers (as the rest of the project does) would let each rule be tested, suppressed, and reasoned about independently, and would avoid one shared StartAnalysis method fanning out into 6+ largely-unrelated helper methods. Worth splitting along the existing per-concern boundaries (at minimum: registration, async-safety, and duplicate-dependency into separate files), even if some low-level helpers stay shared via Extensions/.

Nothing else stood out — the concurrent-execution handling (ConcurrentBag + Interlocked/Volatile for the registration tracking) is correct, and the cancellation-overload matching correctly avoids false positives for unrelated overloads and derived tokens per the added tests.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f1527707c7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thomhurst]

Addressed both review points in 5dc7a0a: split the batch into ModuleRegistrationAnalyzer, ModuleAsyncSafetyAnalyzer, and DuplicateDependsOnAnalyzer while retaining shared low-level analysis helpers; routed all seven descriptors through DiagnosticDescriptorFactory with localized Resources.resx title/message/description entries. Validation: analyzer tests 67/67 passed; touched-file analyzer formatting passed. @codex review

[claude]

Code review (follow-up on 5dc7a0a)

Re-reviewed after the split/refactor commit. Both points from the previous review are properly addressed:

1. Single-class → three focused analyzers. ModuleRegistrationAnalyzer (MPREG001/MPTYPE001), ModuleAsyncSafetyAnalyzer (MPASYNC001-004), and DuplicateDependsOnAnalyzer (MPDEP004) now each own their own rule set and SupportedDiagnostics, matching the one-analyzer-per-concern convention used by SelfDependencyAnalyzer, StatefulModuleAnalyzer, etc. The shared low-level walk logic stays in ModuleAuthoringAnalysis (an internal static helper, not itself a DiagnosticAnalyzer), which is a reasonable way to keep the concurrent-tracking/operation-walking code from being duplicated three times.
2. Routed through DiagnosticDescriptorFactory. All 7 descriptors now go through the shared factory with Resources.resx entries instead of hardcoded literals, and the release notes/severities in AnalyzerReleases.Unshipped.md line up with what's actually configured in code (e.g. MPASYNC001/MPDEP004 correctly default to Error via the factory's default, the rest are explicit Warning).

One new inconsistency introduced by this fix, though: every other analyzer in the project passes the resource name as nameof(Resources.XyzTitle) (compile-time-checked), but the 3 new analyzers pass raw string literals instead, e.g.:

// ModuleRegistrationAnalyzer.cs
DiagnosticDescriptorFactory.Create(
    UnregisteredModuleId,
    "UnregisteredModuleAnalyzerTitle",       // string literal
    "UnregisteredModuleAnalyzerMessageFormat",
    "UnregisteredModuleAnalyzerDescription",
    severity: DiagnosticSeverity.Warning);

vs. every existing analyzer, e.g. AsyncModuleAnalyzer.cs:

DiagnosticDescriptorFactory.Create(
    DiagnosticId,
    nameof(Resources.AsyncModuleAnalyzerTitle),   // nameof
    nameof(Resources.AsyncModuleAnalyzerMessageFormat),
    nameof(Resources.AsyncModuleAnalyzerDescription));

This isn't cosmetic — Resources.Designer.cs was not regenerated in this PR (grep for the 7 new keys, e.g. UnregisteredModuleAnalyzerTitle, finds zero hits in Resources.Designer.cs), so nameof(Resources.UnregisteredModuleAnalyzerTitle) would not have compiled as-is; the string-literal form was almost certainly chosen to route around that. The trade-off is losing the exact compile-time safety net the factory-routing recommendation was meant to restore: a future rename/typo of one of these 7 .resx keys will silently produce a blank/missing diagnostic title instead of a build break, and Resources.Designer.cs is now out of sync with Resources.resx (it's missing 7 members that IDE tooling and other consumers would expect to see). Recommend regenerating Resources.Designer.cs (re-run the ResX custom tool, or add the members by hand to match the generator's output) and switching these 7 call sites to nameof(Resources.X) so all 17 descriptors in the project follow the same pattern.

Nothing else stood out on a re-check of the analysis logic itself: the cancellation-overload matching (IsCancellationOverload) correctly rejects the Call(string) vs Call(int, CancellationToken) case in the new tests by comparing non-token parameter types positionally, the ConfigureAwait unwrap loop in GetAwaitedInvocation handles chained calls, and the ConcurrentBag + Interlocked.Exchange/Volatile.Read combination for cross-symbol-action registration tracking is race-free (the ref capture into the compilation-end lambda is evaluated lazily at invocation time, not at registration time, so it correctly reflects the final value). Test coverage (13 test methods across the three analyzers) covers the meaningful branches: registered/unregistered, public/non-public, all three blocking-call forms, direct and ConfigureAwait-wrapped unflowed tokens, linked/derived tokens, unrelated overloads, and duplicate DependsOn.

Minor, non-blocking observation: MPASYNC001 (async void) fires for any async-void method inside a module class, not just ones invoked from ExecuteAsync. If modules ever wire up event handlers (the idiomatic case where async void is intentional, e.g. (object sender, EventArgs e) signatures), this would flag a legitimate pattern — worth keeping in mind if that comes up in practice, but not something to block on given modules don't appear to use event handlers today.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5dc7a0ad3d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Code review

Re-reviewed at the current HEAD (commit c2f8665457). The three-analyzer split, shared ModuleAuthoringAnalysis helper, and factory-routed descriptors from the last round still look good — nice separation of concerns (registration vs. async-safety vs. duplicate-dependency), and the cancellation-token detection is genuinely careful (handles ConfigureAwait chains, linked/derived tokens, and same-name overloads with unrelated parameter types via IsCancellationOverload).

Two things remain from my read:

1. Previous finding not yet addressed: nameof vs. string literals for resource keys.
All 7 new descriptors in ModuleRegistrationAnalyzer.cs, ModuleAsyncSafetyAnalyzer.cs, and DuplicateDependsOnAnalyzer.cs still pass raw string literals (e.g. "UnregisteredModuleAnalyzerTitle") instead of nameof(Resources.UnregisteredModuleAnalyzerTitle), unlike every other analyzer in the project (AsyncModuleAnalyzer, SelfDependencyAnalyzer, etc.), which use nameof. This is because Resources.Designer.cs hasn't been regenerated — grep for the 7 new keys against it still returns zero hits, so nameof genuinely wouldn't compile as written. The trade-off: Resources.Designer.cs is now out of sync with Resources.resx (IDE tooling/consumers won't see these 7 members), and a future rename/typo of one of these resx keys will silently produce a blank diagnostic title instead of a compiler error. Recommend regenerating Resources.Designer.cs (rerun the ResX custom tool or add the members by hand matching the generator's existing output shape) and switching these 7 call sites to nameof.

2. Test gap: assembly-wide and params-Type[] registration forms are implemented but untested.
The PR description calls out tracking "generic, params-Type, and assembly-wide module registration forms," and ModuleAuthoringAnalysis.TrackRegistrationInvocation does branch on AddModules(params Type[]), AddModulesFromAssembly, and AddModulesFromAssemblyContainingType<T> (all real APIs in PipelineBuilderExtensions). But ModuleAuthoringAnalyzerTests.cs only exercises the generic AddModule<T>() path (Does_Not_Report_Registered_Module). There's no test for:

• AddModules(typeof(A), typeof(B)) (the ITypeOfOperation-scanning branch), or
• AddModulesFromAssembly(...) / AddModulesFromAssemblyContainingType<T>() — which take the special path of suppressing all MPREG001 diagnostics compilation-wide once seen (assemblyRegistrationUsed).

That suppression is an all-or-nothing, whole-compilation behavior — exactly the kind of logic most likely to regress silently without a test pinning it down (e.g. a future refactor could flip the flag check and start reporting false positives for every module in an assembly-scanned project). Worth adding at least one test per form, especially one confirming that an assembly-registered module (no explicit AddModule<T>() call) is not flagged.

Minor / non-blocking: the AddModules/typeof detection is syntactic (DescendantsAndSelf().OfType<ITypeOfOperation>()), so it won't see indirection like Type[] types = { typeof(A) }; builder.AddModules(types); — that would still be flagged as "unregistered." Given MPREG001 defaults to Warning, this seems like an acceptable heuristic trade-off, but might be worth a one-line comment noting the limitation so it isn't rediscovered as a "bug" later.

Nothing here blocks merging on functional grounds — the analyzer test suite (67 passing per the PR description) covers the core detection logic well. The two items above are about closing out the previous round's feedback and hardening test coverage for functionality the PR explicitly claims to support.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c2f8665457

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thomhurst]

Addressed the follow-up review in 440c3ea:

• MPREG001 now runs only for executable compilations, follows required DependsOn dependencies, and scopes assembly scans to the resolved assembly.
• MPTYPE001 permits non-public modules registered explicitly by instance or factory.
• MPASYNC003 now requires token provenance from ExecuteAsync and handles constructed generic cancellation overloads.
• async-safety checks require the real Module.ExecuteAsync override.
• duplicate dependency analysis includes base/interface attributes.
• regenerated typed resource accessors and switched all new descriptors to nameof(Resources.*).
• added coverage for params-Type, both assembly forms, external scans, reusable libraries, auto-registration, instance/factory registration, token provenance, generic overloads, inherited dependencies, and unrelated ExecuteAsync overloads.

Validation: analyzer solution Release build succeeded (0 errors; 17 existing release-tracking warnings), 79/79 analyzer tests passed, and touched-file severity-info formatting passed. @codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 440c3ea7f1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 02e044fc08

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Code review (re-review at HEAD 02e044fc08)

Both items from my last review (commit c2f8665457) are now resolved:

1. nameof vs. string literals — all 7 descriptors in ModuleRegistrationAnalyzer, ModuleAsyncSafetyAnalyzer, and DuplicateDependsOnAnalyzer now use nameof(Resources.X), and Resources.Designer.cs has been regenerated with the matching members (verified all 7 new keys are present).
2. Test gap for registration forms — ModuleAuthoringAnalyzerTests.cs now covers AddModules(typeof(...)) (params-Type[]), AddModulesFromAssemblyContainingType<T>(), AddModulesFromAssembly(...), and an explicit External_Assembly_Scan_Does_Not_Hide_Unregistered_Local_Module test that pins down the all-or-nothing suppression behavior I flagged as the highest-risk-of-silent-regression path.

New since last review: scope of the PR has grown substantially

Commit 02e044fc08 ("Unify analyzer diagnostic IDs and documentation (#3437)") adds something well beyond the PR's stated purpose. The title/description/linked issue (#3314) are about detecting module-authoring mistakes, but this commit renumbers every pre-existing analyzer in the project — including ones this PR never touches, like AwaitThis, StatefulModule, MPDEP001-003, ConsoleUse, LoggerInConstructor, EnumerableModuleResult, MissingDependsOnAttribute, and the unrelated ModularPipelines.Development.Analyzers package (MPD0001/MPD0002) — into a new MP0001-MP0019 scheme, plus adds a whole docs-generation pipeline (docs/scripts/generate-analyzer-docs.mjs, 19 generated rule pages, docs/package.json changes).

This is a real breaking change for anyone with .editorconfig severity overrides or #pragma warning disable ConsoleUse/AwaitThis/etc. in their code — confirmed by the fact that this repo's own src/ModularPipelines.Build/Program.cs needed a pragma update (ConsoleUse → MP0004) as part of the same commit. To the PR's credit, it's not half-done: the migration is documented with a full legacy-ID → new-ID table in docs/docs/how-to/analyzers.md, and AnalyzerMetadataTests.PublicRulesUseUnifiedIdsAndHelpLinks pins the full ID sequence with no gaps or collisions.

Given the breaking-change nature and that it's unrelated to "detect module authoring mistakes," I'd suggest this ID-unification work land as its own PR with its own description/changelog entry rather than riding in on this one — it makes the diff harder to review for what's actually new analyzer logic vs. a mechanical rename, and ties an unrelated breaking change to this feature's release notes. Not a blocker since it's already correct and tested, but worth considering for the next PR of this shape.

Minor, non-blocking: the MP#### ID space is now shared across two independently-versioned packages (ModularPipelines.Analyzers owns 1-10 and 13-19, ModularPipelines.Development.Analyzers owns 11-12), but there's no single allocator — the reserved ranges only exist as a hardcoded Enumerable.Range split in AnalyzerMetadataTests.cs. A future rule added to either package without checking the other's range could collide silently (the test would fail, but only if someone remembers to update it in both places). A short comment at the top of each DiagnosticId block noting the reserved range, or a shared constants file, would make this more self-documenting.

Nothing else stood out — the core detection logic in ModuleAuthoringAnalysis.cs is unchanged since the last round (already reviewed in depth: cancellation-overload matching, ConfigureAwait unwrapping, concurrent registration tracking all check out), and the new DiagnosticDescriptorFactory.Create(..., helpLinkUri: ...) addition correctly threads through to every descriptor per AnalyzerMetadataTests.

[thomhurst]

Fixed the Analyzer Diagnostics CI failure in 44744a9. RS1038 was triggered because this internal project intentionally co-locates its analyzers and CodeFixProvider types (and therefore references Workspaces); the project now suppresses only RS1038 while retaining all other extended analyzer rules. Validation: ModularPipelines.Analyzers.sln Release build completed with 0 errors; development analyzer tests passed 30/30.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 44744a9aca

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Code review (re-review at HEAD 44744a9aca)

Went through the full history on this PR (five prior rounds from claude/codex bots) against the current HEAD, and independently re-verified the fixes rather than trusting the commit messages:

• Auto-registered required dependencies — AddRequiredDependencyClosure (ModuleAuthoringAnalysis.cs:322-341) now walks the non-optional [DependsOn<T>] closure and adds it to the registered set before reporting MP0013, matching ModuleAutoRegistrar's runtime behavior. Covered by Does_Not_Report_Required_AutoRegistered_Dependency.
• Reusable-library false positives — MP0013 is now gated behind IsApplication(context.Compilation.Options.OutputKind), so class libraries whose modules are registered by a downstream consumer no longer get flagged. Covered by Does_Not_Require_Registration_In_Reusable_Library.
• Assembly-scan suppression scope — this moved from the previous all-or-nothing boolean flag to a real scannedAssemblies set keyed by resolved IAssemblySymbol, so an unrelated AddModulesFromAssembly call no longer blanket-suppresses local unregistered-module warnings. External_Assembly_Scan_Does_Not_Hide_Unregistered_Local_Module pins this down.
• nameof vs. string literals / Resources.Designer.cs drift — resolved, all 7 descriptors use nameof(Resources.X) and the designer file is regenerated.
• Confirmed [with(SymbolEqualityComparer.Default)] at ModuleAuthoringAnalysis.cs:463 isn't a typo — it's the existing "collection expression arguments" preview pattern already used in ModuleConfigurationBuilder.cs, consistent with this project's LangVersion=preview.
• The final commit (44744a9aca) is a minimal, well-justified NoWarn for RS1038 with a comment explaining why this internal package intentionally co-locates analyzers and code fixes.

One remaining gap: MP0018 (non-public module) doesn't account for assembly-scan registration

ReportModuleDiagnostics (ModuleAuthoringAnalysis.cs:283-290) reports NonPublicModuleRule for every non-public module except ones in instanceRegisteredModules (i.e. registered via AddModule(instance)/AddModule(factory)):

foreach (var module in moduleSet.Where(module =>
             !IsPublic(module) && !instanceRegistered.Contains(module)))
{
    ReportModuleDiagnostic(context, module, ModuleRegistrationAnalyzer.NonPublicModuleRule);
}

This runs before scannedAssemblies is even computed (that set is built a few lines later, only for the MP0013 check). But ServiceCollectionExtensions.AddModulesFromAssembly (src/ModularPipelines/Extensions/ServiceCollectionExtensions.cs:272) discovers modules via assembly.GetTypes() — not GetExportedTypes() — so it legitimately picks up internal module classes at runtime. An internal module living in an assembly that's registered via AddModulesFromAssembly(...) / AddModulesFromAssemblyContainingType<T>() is fully functional, but MP0018 will still flag it as "module class is not public," since that check only exempts the instance/factory forms, not the assembly-scan form.

This is the same class of false positive that was just fixed for MP0013 (reusable-library / assembly-scan registration), applied to a sibling rule that shares the same scannedAssemblies data but doesn't consult it. Confirmed there's no test for this combination — ModuleSource(...) (the shared test helper) always emits public class BuildModule, so none of the AddModulesFromAssembly* tests exercise a non-public module.

Suggested fix: move the MP0018 loop after scanned is computed and also exempt scanned.Contains(module.ContainingAssembly), then add a test pairing an internal module with AddModulesFromAssembly/AddModulesFromAssemblyContainingType.

Nothing else stood out — this is genuinely the last loose end after a very thorough back-and-forth; everything else checks out against the actual runtime registration/discovery code, not just the test suite.

[thomhurst]

Addressed all fresh review findings in e6369acb74 and 81edd6aa85:

• follow local assembly aliases and normalize closed generic registrations
• trace stored/joined tasks, callable cancellation overloads, reduced extensions, and invoked local functions
• diagnose Task.WaitAll / Task.WaitAny
• honor assembly scans for non-public modules, closing the additional Claude review gap

Validation: analyzer tests 91/91 passed; touched-file info analyzer and whitespace gates passed; ModularPipelines.Analyzers.sln Release build passed with 0 errors (existing warnings only).

All seven inline threads are answered and resolved.

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 81edd6aa85

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Code review (re-review at HEAD 81edd6aa85)

Reviewed the delta since the last round (commits e6369acb74 and 81edd6aa85, ~430 lines across ModuleAuthoringAnalysis.cs and its test file), verified it against the fixes claimed in the PR's last comment, and independently rebuilt ModularPipelines.Analyzers.sln in Release — clean, 0 warnings/0 errors.

All items raised in prior rounds (mine and Codex's) check out as fixed and are backed by new tests:

• MP0018 now honors assembly-scan registration — the non-public-module loop runs after scanned is computed and excludes scanned.Contains(module.ContainingAssembly), closing the gap flagged in the previous review. Covered by Does_Not_Report_NonPublic_Module_Registered_By_Assembly.
• Closed-generic registrations normalized — AddModule<ScaleModule<string>>() now registers via typeArgument.OriginalDefinition, matching the open-generic symbol collected from the declaration. Covered by Does_Not_Report_Closed_Generic_Module_Registration.
• Task.WaitAll/WaitAny now flagged as blocking calls alongside .Wait()/.Result/GetAwaiter().GetResult(). Covered by new DataRows in Reports_Blocking_Call_In_ExecuteAsync.
• Cancellation-flow tracing extended to stored tasks (var pending = Task.Delay(1); await pending;), Task.WhenAll/WhenAny joins, and local assembly variables passed to AddModulesFromAssembly — each has a dedicated regression test, and I traced GetAwaitedInvocations/GetValueAndLocalInitializers by hand against those cases; the recursion terminates correctly (local-reference cycles are guarded by visitedLocals, ConfigureAwait/WhenAll chains unwrap correctly).
• Reduced extension-method overloads now resolve via method.ReducedFrom, and candidate overloads are filtered by compilation.IsSymbolAccessibleWithin(...) before being treated as the "cancellable" counterpart — verified against Reports_Unflowed_Token_For_Reduced_Extension_Overload and Does_Not_Report_Inaccessible_CancellationToken_Overload.
• Local-function handling — diagnostics inside a local function declared in ExecuteAsync now only fire if that local function is actually invoked somewhere in the method body (LocalFunctionsAreInvoked), avoiding false positives on dead/unused helpers while still catching real ones (Reports_Async_Safety_Inside_Invoked_Local_Function vs. Does_Not_Report_Async_Safety_Inside_Unused_Local_Function).

I also checked the GitHub review-thread state directly via the API: all 16 Codex review threads on this PR are marked resolved, and the fixes match what's in the current diff, not just the commit message claims.

Nothing new stood out in this delta — no compile errors, no logic that produces wrong results independent of exotic inputs, and no fresh CLAUDE.md violations (the shared DiagnosticDescriptorFactory/resource-localization pattern and the one-analyzer-per-concern split from earlier rounds are both still intact in this HEAD). This has been a very thorough back-and-forth and the analyzer's behavior now lines up with the actual runtime registration/discovery semantics in ServiceCollectionExtensions, not just the test suite.

[thomhurst]

Follow-up 19d5d6b58b replaces recursive/nested await traversal with an explicit stack, addressing CodeFactor's complexity finding while preserving traversal order and local-initializer handling. Analyzer tests 91/91 passed; touched-file info analyzer, whitespace, and git diff --check gates passed.

@codex review

[thomhurst]

@codex review

[thomhurst]

Addressed the review-body scope concern: the PR description now explicitly discloses that merged stacked PR #3437/#3312 is included and that renumbering shipped analyzer IDs breaks ID-keyed suppressions, with migration guidance.

Latest P2 fixes are in 240b06c194; focused factory tests, generated-doc check, and Release analyzer test-project build all pass.

@codex review

[claude]

Code review (HEAD 240b06c194)

Re-reviewed against the prior rounds of feedback (mine and Codex's) rather than starting over.

Previously flagged, now confirmed resolved:

• The DiagnosticDescriptorFactory/resource-localization bypass and the single 476-line class owning 7 unrelated diagnostics are both gone — the analyzer is now split into ModuleRegistrationAnalyzer, ModuleAsyncSafetyAnalyzer, and DuplicateDependsOnAnalyzer, and all three route their descriptors through DiagnosticDescriptorFactory.Create with Resources.resx entries, consistent with every other analyzer in the project.
• The undisclosed-breaking-change concern (silently renumbering 10 already-shipped, NuGet-published analyzer IDs to the MP#### family under an issue labeled "Breaking: no") is now disclosed: the PR description has a dedicated "Breaking change" section naming the old→new ID mapping, and docs/docs/how-to/analyzers.md documents the full migration table for consumers with ID-keyed suppressions.

Since the last review (81a5dbcb80 → 240b06c194, commit fix(analyzers): close factory and ID gaps):

• Added a !moduleType.IsModule(compilation) guard in ModuleAuthoringAnalysis.cs to stop treating invocations that return a non-module class type as module registrations, with a new regression test (Does_Not_Report_Source_Unavailable_DI_Factory) — verified the guard and test line up correctly.
• Reconciled AnalyzerReleases.Shipped.md/Unshipped.md so the MPDEP002→MP0005 rename chain for the circular-dependency rule is tracked without a dangling intermediate ID. One very minor nit: Shipped.md's "Release 1.0" table now lists MPDEP002 as the ID that shipped, but the ID actually published to NuGet in that release was ConflictingDependsOnAttribute (per the earlier verified NuGet history) — MPDEP002 was itself never released. This is an internal Roslyn release-tracking file (used for ID-consistency checks, not read by consumers), and the real migration guide already lists both ConflictingDependsOnAttribute → MP0005 and MPDEP002 → MP0005, so it's not consumer-impacting — just flagging for accuracy, not blocking.

No new correctness issues or architectural concerns found in this round. This PR has been reviewed extensively (219+ resolved threads across mine and Codex's rounds); nothing outstanding from prior rounds remains unaddressed.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 240b06c194

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thomhurst]

@codex review

[claude]

Code review (HEAD 7da253b0c)

Only one commit landed since the last review (240b06c194): fix(docs): preserve analyzer severities.

Change: the generated analyzer doc pages previously hardcoded every rule's .editorconfig example as dotnet_diagnostic.<id>.severity = warning, regardless of the rule's actual default severity (e.g. MP0001/MP0006, which default to Error). The new getEditorConfigSeverity() helper in docs/scripts/generate-analyzer-docs.mjs maps the rule's real DiagnosticSeverity (Error/Warning/Info/Hidden) to the correct .editorconfig keyword (error/warning/suggestion/silent), and the 13 regenerated .md pages now show the correct severity per rule.

I checked this against how severity is actually populated: it's read straight from the AnalyzerReleases.*.md release-tracking tables, which only ever contain Roslyn's four canonical DiagnosticSeverity names — so the switch's four cases are exhaustive and the throw for an unrecognized value is unreachable in practice, not a latent gap. No design issues with this change: it's a narrowly-scoped, self-contained fix to a generator script, consistent with the existing sentence()/checkGeneratedFiles() helpers already in that file, and doesn't touch analyzer logic.

This PR has now had 200+ resolved review threads across many rounds (mine and Codex's). All previously raised architectural points — routing through DiagnosticDescriptorFactory, splitting the single mega-analyzer into ModuleRegistrationAnalyzer/ModuleAsyncSafetyAnalyzer/DuplicateDependsOnAnalyzer, nameof vs. string-literal resource keys, test coverage for assembly-wide/params-Type[] registration forms, and disclosure of the breaking ID-renumbering change — remain resolved as of this HEAD. Nothing new to flag.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7da253b0cc

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thomhurst]

Addressed the current async-void false negative in 81bae8f: event-shaped methods now require an observed subscription before exemption. Validation: async-void analyzer tests 7/7, analyzer Release build 0 warnings/errors, targeted format clean. @codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 81bae8fba7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Code review (delta since last review, HEAD 7da253b0c → 81bae8fba7)

One commit landed since the last review: 81bae8fba7 ("fix(analyzers): require event subscription"), touching ModuleAuthoringAnalysis.cs (-17/+0 net) and its test file (+20).

What it does: Removes HasEventHandlerSignature, which previously exempted any async-void method matching the (object?, EventArgs) shape from the async-void diagnostic purely by signature, regardless of whether it was ever actually wired up as an event handler. The async-void exemption now requires the method to appear in eventHandlerMethods — populated by CollectEventHandlerMethod, which only adds a method when it's the resolved target of a real += (IEventAssignmentOperation) somewhere in the compilation, via the existing GetDirectDelegateTarget walk (handles conversions, delegate creation, method-group and lambda targets).

Verified correct:

• This closes a genuine false negative: an async void OnCompleted(object? sender, EventArgs e) method that matches the conventional handler shape but is never subscribed anywhere (dead code, or a handler wired up only through a shape the analyzer can't see) would previously be silently exempted from MP00xx async-void reporting. Now it's correctly flagged, since fire-and-forget async void is exactly as dangerous whether or not the signature looks like a handler.
• The one existing test that relied on the old shape-only exemption (Does_Not_Report_Async_Void_Event_Handler_In_Module) was updated to add an actual Completed += OnCompleted; subscription in the constructor, so it still exercises the "legitimately subscribed" path rather than accidentally relying on removed behavior.
• Grepped the rest of the test file for the same (object?, EventArgs eventArgs) shape — the only two occurrences are that updated test and the new Reports_Async_Void_Event_Shaped_Method_When_Not_Subscribed test, so no other test was silently depending on the removed shape-based exemption.
• CollectEventHandlerMethod is registered via RegisterOperationAction(..., OperationKind.EventAssignment) at the compilation level (not scoped to the containing module type), so a subscription anywhere in the compilation — not just inside the declaring module — still correctly exempts the method.

No functional or correctness issues found in this delta. Consistent with the file's established "prefer real usage evidence over shape heuristics" direction from earlier rounds (e.g. the reachability-based exemptions elsewhere in this analyzer). The standing architectural note from prior rounds (hand-rolled reachability/dataflow tracing in ModuleAuthoringAnalysis.cs vs. Roslyn's ControlFlowGraph/AnalyzeDataFlow APIs) still applies but is unrelated to this commit and already tracked across many earlier reviews — not repeating it in full here.

[thomhurst]

@codex review

[claude]

Code review (delta since last review, HEAD 81bae8fba7 → 581df7be1)

One commit landed since the last review: 581df7be1 ("fix(analyzers): ignore event removals"), touching ModuleAuthoringAnalysis.cs and its test file.

What it does: In CollectEventHandlerMethod, the event-subscription exemption for the async-void rule now checks eventAssignment.Adds before recording a method as a legitimate event handler. Previously, a method referenced only via Completed -= OnCompleted; (unsubscribe, never subscribe) was incorrectly added to eventHandlerMethods, exempting it from the async-void diagnostic even though it was never actually wired up as a live handler.

Verified correct:

• This is consistent with the existing GetEventAccessor helper elsewhere in the same file (eventAssignment.Adds ? AddMethod : RemoveMethod), so the fix follows an established pattern rather than introducing a new one.
• The new test Reports_Async_Void_Event_Handler_When_Only_Unsubscribed correctly exercises the fix: a method only ever unsubscribed (Completed -= OnCompleted; in the constructor, no corresponding += anywhere) is now flagged as async-void instead of being silently exempted.
• No other tests rely on the old (incorrect) "any assignment counts as subscription" behavior.

No functional or correctness issues found in this delta.

PR-wide status check: All 224 review threads on this PR are resolved, and the checks that have completed are passing (no new failures introduced by this commit).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 581df7be1a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thomhurst]

Fixed the delegated-factory MP0018 false positive in 144a324. Unresolved DI factory registrations are now tracked separately: they conservatively suppress the non-public diagnostic while unrelated unresolved dynamic Type[] registrations still do not. Added an internal-module delegated-factory regression; focused pair passed 2/2; analyzer Release build passed with 0 warnings/errors.

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 144a324bc4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Code review (delta since last review, HEAD 581df7be1 → 144a324bc4)

One commit landed since the last review: 144a324bc4 ("fix(analyzers): scope delegated-factory suppression"), touching ModuleAuthoringAnalysis.cs and one test method in ModuleAuthoringAnalyzerTests.cs.

What it does: Fixes an MP0018 (NonPublicModuleRule) false positive for modules registered only through a delegated DI factory (e.g. services.AddSingleton<IModule>(_ => factory()) where factory is itself an opaque parameter). Previously TrackDirectImplementationValue only ever fed the shared unresolvedModuleRegistrations bag when it couldn't statically resolve the registered type, and that bag exclusively gates UnregisteredModuleRule (MP0018's sibling), not NonPublicModuleRule. So an internal module registered only via an unresolvable factory was correctly treated as "possibly registered" for the unregistered-module check but still flagged as "should be public" — even though instance/factory registration doesn't require public visibility. The fix adds a new, narrower unresolvedFactoryRegistrations bag, populated only when the unresolved argument was specifically implementationFactory (not implementationType/params-Type[]), and gates the NonPublicModuleRule loop on it.

Verified correct:

• Traced the two distinct tracking paths: TryTrackDirectImplementationType (params-Type[]/implementationType scans) only ever touches unresolvedModuleRegistrations, never the new unresolvedFactoryRegistrations bag. TrackDirectImplementationValue (implementationInstance/implementationFactory args) is the only path that can populate the new bag, and only does so when tracking failed and an implementationFactory argument was present. So the fix is correctly scoped to the delegated-factory case the commit message describes, not unresolved dynamic Type[] registrations.
• Confirmed this doesn't regress the existing Reports_NonPublic_Module_When_Params_Type_Array_Property_Cannot_Be_Resolved test (an internal module reachable only via typeof(Registration).Assembly.GetTypes() still correctly reports MP0018), since that path never touches unresolvedFactoryRegistrations.
• The new/renamed test Does_Not_Report_NonPublic_Module_Registered_By_Delegated_DI_Factory correctly exercises the fix by changing the registered module from public to internal, which is the only way this test could previously have caught the false positive (a public module wouldn't trigger MP0018 regardless).
• Built ModularPipelines.Analyzers.slnx locally at this HEAD: 0 warnings, 0 errors, consistent with the author's validation note.

One architectural note, not blocking: like the pre-existing unresolvedModuleRegistrations gate for UnregisteredModuleRule, this new gate suppresses NonPublicModuleRule for every non-public module in the compilation once a single unresolvable factory registration is seen anywhere, rather than scoping the suppression to the specific module the factory might be registering. That's a real precision trade-off (one opaque delegated-factory registration silently quiets MP0018 project-wide going forward), but it's consistent with the conservative "prefer under-reporting to false positives" philosophy this file already uses for the unregistered-module check, so it's not a new pattern being introduced — just worth keeping in mind if MP0018 false negatives come up in a large solution that uses this registration style.

No functional or correctness issues found in this delta.

[thomhurst]

Fixed the #3819 integration break in e748223: analyzer registration and ConfigureServices recognition now accepts both the v4 root-namespace PipelineBuilderExtensions and the prior namespace. Consumer analyzer suite passes 502/502; analyzer Release build passes with 0 warnings/errors. @codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e7482238fd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Code review (delta since last review, HEAD 144a324bc4 → e7482238fd)

One commit landed since the last review: e7482238fd ("fix(analyzers): recognize root builder APIs"), touching only ModuleAuthoringAnalysis.cs (+5/-2).

What it does: Fixes a #3819 integration break where a v4 API change moved PipelineBuilderExtensions from the ModularPipelines.Extensions namespace to the ModularPipelines root namespace. Three call sites that pattern-match on the fully-qualified containing-type name were updated to accept both the new root-namespace and prior-namespace forms:

• IsModuleRegistrationMethod (module registration recognition, e.g. AddModule/AddModules)
• IsDirectServiceRegistrationMethod (direct AddSingleton/AddScoped/AddTransient recognition)
• IsKnownDelegateInvoker (recognizing ConfigureServices as a delegate invoker for reachability tracing)

Verified correct:

• Grepped the full file for both namespace string literals — all three occurrences of the old ModularPipelines.Extensions.PipelineBuilderExtensions string were updated to also accept ModularPipelines.PipelineBuilderExtensions; no call site was missed.
• The is "A" or "B" or ... / switch pattern additions are syntactically consistent with the existing style at each site and don't change behavior for the pre-existing namespace form (pure additive or branch).
• This is an backward/forward-compatible widening (accept both namespaces) rather than a breaking rename, which is the right call for an analyzer that needs to work across the v3/v4 API surface.

No functional or correctness issues found in this delta.

Minor observation, not blocking: this commit doesn't add a dedicated analyzer test case pinning recognition of the new root-namespace PipelineBuilderExtensions form (the author's validation note cites the downstream consumer suite passing 502/502, not a new unit test in ModuleAuthoringAnalyzerTests.cs). Given the rest of this PR's rigorous "one test per fix" pattern in earlier rounds, a small regression test using the root-namespace form would guard against this specific mapping silently regressing if the namespace list is edited again later — but I'm not blocking on it since the existing tests plus the consumer suite already exercise the behavior.

PR-wide status check: no new issues found; this stays consistent with the architectural notes already tracked from earlier rounds (hand-rolled reachability tracing vs. Roslyn's ControlFlowGraph APIs, and the project-wide (not per-module-scoped) unresolvedFactoryRegistrations suppression) — not repeating those in full here since they're unrelated to this commit.

[thomhurst]

@codex review

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[claude]

Code review (delta since last review, HEAD e7482238fd → 497464d6e0)

One commit landed since the last review: 497464d6e0 ("fix(analyzers): follow fluent registrations"), touching ModuleAuthoringAnalysis.cs (+21/-8) and ModuleAuthoringAnalyzerTests.cs (+86 new tests).

What it does:

• Extracts the inline ModuleRegistration<T> type check into a shared IsModuleRegistrationType helper.
• Teaches IsDirectServiceRegistrationMethod to recognize AddSingleton/AddScoped/AddTransient called directly on ModuleRegistration<TModule> (the fluent handle returned by AddModule<T>()), not just the extension-method forms.
• Teaches IsKnownDelegateInvoker to recognize ModuleRegistration<T>.ConfigureServices(...) as a delegate-invoking call so module registrations inside the callback are still traced.
• Closes a gap where ServiceDescriptor.Singleton<IModule>(_ => factory()) routed through TryTrackServiceDescriptorArguments/TryTrackDirectModuleServiceRegistration didn't feed unresolvedFactoryRegistrations, by threading that bag through and adding ContainsUnresolvedModuleServiceDescriptorFactory to detect the factory-overload case specifically (vs. an unresolved implementationType).

Verified correct:

• Confirmed ModuleRegistration<T> (src/ModularPipelines/Builders/ModuleRegistration.cs) genuinely exposes AddSingleton<TService, TImplementation>(), AddSingleton<TService>(instance), and ConfigureServices(Action<IServiceCollection>) as thin forwarders to the underlying PipelineBuilder — so recognizing these as equivalent to the existing extension-method forms is correct, not a widening that admits false negatives.
• IsKnownDelegateInvoker's existing switch already treats the PipelineBuilderExtensions.ConfigureServices extension method as a delegate invoker; adding the ModuleRegistration<T> instance-method form is consistent with that existing entry.
• ContainsUnresolvedModuleServiceDescriptorFactory's extra Arguments.Any(argument => argument.Parameter?.Name == "implementationFactory") check isn't redundant with IsUnresolvedModuleServiceDescriptorFactory's own factory branch — it's what disambiguates "unresolved because of an opaque factory delegate" from "unresolved because implementationType couldn't be statically determined," which matters because only the former should feed unresolvedFactoryRegistrations.
• All three new tests (Does_Not_Report_Unresolved_ServiceDescriptor_Factory, Does_Not_Report_Fluent_AddSingleton_Registration, Does_Not_Report_Fluent_ConfigureServices_Registration) map 1:1 to the three behavioral changes and use the project's existing VerifyRegistrationCS.VerifyExecutableAnalyzerAsync harness correctly.
• DescendantsAndSelf() on IOperation is already used extensively elsewhere in this file, so the new usage isn't introducing an unresolved API.

No functional or correctness issues found in this delta.

PR-wide status check: no new issues found. This stays consistent with the architectural notes already tracked from earlier rounds (hand-rolled reachability tracing vs. Roslyn's ControlFlowGraph APIs, and the project-wide rather than per-module-scoped unresolvedFactoryRegistrations suppression in ReportModuleDiagnostics) — not repeating those in full here since they're unrelated to this commit.

[thomhurst]

Required pipeline failed on unrelated RunReportTests timing flake: DistributedMasterReconcilesMatchedMetricsPerWorker timed out; all other jobs passed. Triggered one failed-job rerun.