Unify analyzer diagnostic IDs and documentation

[thomhurst]

Summary

• renumber all core and development analyzer diagnostics into one MP0001-MP0019 family
• attach stable HelpLinkUri values to every rule and verify descriptor metadata
• generate deterministic per-rule documentation from Roslyn release metadata, with docs build drift checks and migration guidance

Validation

• analyzer tests: 68 passed
• development analyzer tests: 30 passed
• changed-file analyzer formatting: passed
• analyzer docs generator check: 19 pages verified
• docs typecheck and production build: passed

Stack

This PR is based on #3433 so its seven new analyzer rules can receive MP0013-MP0019 without duplicating that PR in the review diff.

Closes #3312

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ee30df8560

ℹ️ 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]

Review: MP#### analyzer renumbering (#3437)

Solid, well-executed change overall — renumbering is applied consistently across both analyzer projects, the AnalyzerReleases.*.md rename bookkeeping follows the existing convention correctly (old shipped IDs moved to "Removed Rules" with a "Renamed to" note, matching how ConflictingDependsOnAttribute → MPDEP002 was already handled on main), and generating the docs pages from the release-tracking files instead of hand-writing 19 markdown pages is a good call — it makes the release files the actual source of truth instead of just release-tracking metadata.

Two things worth addressing, both because they touch the exact problem this PR is trying to solve (one clean, collision-free MP#### ID space):

1. The doc generator can silently merge a duplicate ID instead of failing

docs/scripts/generate-analyzer-docs.mjs builds one Map of active rules by folding all four AnalyzerReleases.*.md files together (applyReleaseMetadata, called once per file). If the same MP#### ID ever appeared as a "New Rule" in two different files — e.g. a future PR adds a rule to ModularPipelines.Development.Analyzers and picks an ID that collides with one already used in ModularPipelines.Analyzers — the second file processed just overwrites the first in the map (activeRulesById.set(id, rule)), and the generator happily produces one doc page for whichever entry won. No error, no warning.

This matters here specifically because the whole point of the PR is a single, unshared MP0001-MP0019 numbering family spread across two independently-built projects that can't reference each other's DiagnosticId constants at compile time. The two AnalyzerMetadataTests added in this PR each only check their own project's IDs in isolation (MP0001-10,13-19 in one test, MP0011-12 in the other) — neither test, nor anything else in CI, actually verifies the two sets don't overlap. The doc generator is the one place that sees the full merged set, so it's the natural place to assert uniqueness (e.g. throw if activeRulesById.has(id) is already true for a "New Rules" entry) rather than silently resolving collisions by file-processing order.

2. DocumentationBaseUrl is duplicated instead of reusing the existing factory pattern

The literal "https://thomhurst.github.io/ModularPipelines/docs/analyzers/" is now declared independently in six places: DiagnosticDescriptorFactory.cs, VirtualCommandAnalyzer.cs, VirtualSwitchPropertyAnalyzer.cs, and three test files. Two of those (VirtualCommandAnalyzer.cs and VirtualSwitchPropertyAnalyzer.cs) live in the same ModularPipelines.Development.Analyzers assembly and duplicate not just the constant but the whole descriptor-construction boilerplate (new DiagnosticDescriptor(id, title, format, category, severity, ..., helpLinkUri: DocumentationBaseUrl + id)).

The core ModularPipelines.Analyzers project already solved this exact problem with DiagnosticDescriptorFactory.Create(...), which centralizes the base URL and the help-link construction in one place. ModularPipelines.Development.Analyzers doesn't reuse that pattern (it can't reference the other assembly — analyzers are typically kept isolated to avoid version conflicts when both load into the same Roslyn host), so it re-implements the same thing by hand, twice, with a copy-pasted constant. A small internal factory local to ModularPipelines.Development.Analyzers (mirroring DiagnosticDescriptorFactory) would remove the duplication within that project immediately, and is the kind of thing that's normally shared between sibling analyzer packages via a linked source file (<Compile Include="../Shared/..\.cs" />) rather than a project reference, precisely because of that isolation constraint. Right now, if the docs domain ever moves, someone has to remember to update six independent string literals instead of one or two.

Minor

• AnalyzerMetadataTests.PublicRulesUseUnifiedIdsAndHelpLinks asserts exact array order against Enumerable.Range(1, 10).Concat(Enumerable.Range(13, 7)). It works today, but every future rule addition/removal requires manually keeping the analyzer array's literal order in sync with a hand-maintained numeric range — an order-agnostic check (sort both sides, or assert set membership + "no gaps") would be less fragile to maintain than keeping two representations of the same sequence in lockstep.
• docs/package.json's prebuild/prestart hooks shell out to npm run ... even though the docs site is otherwise yarn-managed (CI runs yarn build, which does trigger the pre/post npm-style hooks under yarn classic, so it works) — using yarn there instead would keep the project on one package-manager invocation instead of implicitly depending on npm being present alongside yarn.

None of these block the PR — the collision-detection gap is the one I'd actually want fixed before this ships, since it's cheap to add and it's exactly the invariant this PR exists to establish.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dc962e8df4

ℹ️ 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

Good follow-through since the last review: the doc generator now throws on a duplicate MP#### ID instead of silently letting file-processing order win (generate-analyzer-docs.mjs), and the DocumentationBaseUrl/descriptor-construction duplication across VirtualCommandAnalyzer.cs and VirtualSwitchPropertyAnalyzer.cs has been eliminated by adding a DiagnosticDescriptorFactory to ModularPipelines.Development.Analyzers mirroring the core project's factory. The docs/package.json hooks were also switched from the previously-flagged npm run to yarn, and the analyzer ID array assertion already uses CollectionAssert.AreEquivalent/IsEquivalentTo, which is order-insensitive, so that minor concern doesn't need further changes.

Three new items surfaced in this pass, one of which is a real functional bug and two of which are documentation-completeness/architecture gaps worth addressing before this ships, since (like last time) they all touch the exact invariant this PR exists to establish — a single reliable MP#### ID space with working docs.

1. HelpLinkUri will 404 for every rule because the docs site is versioned

Both DiagnosticDescriptorFactory implementations bake in https://thomhurst.github.io/ModularPipelines/docs/analyzers/ + id as the permanent, shipped help link. But docs/versions.json contains ["3.x"] and docs/versioned_docs/version-3.x/ exists with no analyzers/ directory and no lastVersion override in docusaurus.config.ts — so with Docusaurus's default versioning behavior, the bare /docs/... path serves the last released version (3.x), and everything under docs/docs/ (where these 20 new pages live) is only reachable at /docs/next/... until the next version cut. Every help link an IDE surfaces today will 404.

This is architecturally the same class of problem the PR already fixed for ID collisions: the fix works in isolation (docs generator + tests are internally consistent) but doesn't account for how the surrounding system (versioned docs deploy) actually serves the content. Two reasonable fixes: point the base URL at .../docs/next/analyzers/ until 3.x is next cut, or add the pages to docs/versioned_docs/version-3.x/analyzers/ as well so the bare path works immediately. A redirect/alias at the Docusaurus level would also work and avoid ever having to remember this again.

2. The ID migration table is missing 5 of the renames this same PR performs

docs/docs/how-to/analyzers.md (the migration table) documents legacy→current ID mappings, but is missing:

• AwaitThis → MP0007
• StatefulModule → MP0008
• MPDEP001 → MP0009
• MPDEP002 → MP0005 (in addition to the existing, still-correct ConflictingDependsOnAttribute → MP0005 row, since MPDEP002 was a separate, previously-unshipped ID for the same rule)
• MPDEP003 → MP0010

All five were live/unshipped IDs on main before this PR per AnalyzerReleases.Unshipped.md. Anyone with a #pragma warning disable MPDEP001 (etc.) or a matching .editorconfig entry gets no migration guidance for exactly the IDs this PR renumbers.

Since the table is now hand-maintained separately from the generator, consider generating this table from the same AnalyzerReleases.*.md "Removed Rules" Renamed to bookkeeping the generator already reads — that would make the same class of omission structurally impossible instead of relying on someone remembering to update prose by hand each time a rule is renumbered.

3. generate-analyzer-docs.mjs mis-parses a ### Changed Rules section (currently dormant)

The heading regex /^###\s+(New|Removed|Changed) Rules$/i opts into parsing Changed Rules sections, and rows there do get turned into a rule via const [id, category, severity, ...notes] = columns;. But Roslyn's standard Changed Rules table is 6 columns (Rule ID | New Category | New Severity | Old Category | Old Severity | Notes), not the 4-column New/Removed shape. The old category/severity/notes all collapse into description, and since renderIndex interpolates that description unescaped into a markdown table cell, the embedded | characters will corrupt the generated index.md table the first time any rule's category or severity changes (a normal, expected maintenance operation for AnalyzerReleases.md files).

None of the four release files currently have a Changed Rules section, so nothing is broken today — but the code path is live, not speculative, so it'll fail silently (wrong output, not a thrown error) the first time it's exercised. Worth branching the column destructuring on section === 'changed' now while the fix is cheap and the failure mode is fresh in mind, rather than leaving a landmine for whoever changes a rule's severity later.

None of these are inherent to the renumbering approach itself — the core design (one shared MP#### space, generated docs as source of truth, a factory to remove boilerplate) is sound and the right call for this problem.

[claude]

Code review

Checked this pass against my previous review comments on this PR. Confirmed fixed:

• Duplicate MP#### ID collision now throws in generate-analyzer-docs.mjs (applyReleaseMetadata) instead of silently letting file-processing order win.
• DocumentationBaseUrl/descriptor-construction duplication in ModularPipelines.Development.Analyzers is gone now that it has its own DiagnosticDescriptorFactory, mirroring the core project's.
• docs/package.json's prebuild/prestart hooks now call yarn ... instead of npm run ....
• The analyzer ID array assertions in both AnalyzerMetadataTests are order-insensitive (CollectionAssert.AreEquivalent / IsEquivalentTo).

Still open from the last review — none block merging, but all three touch the exact invariant this PR exists to establish (one working MP#### ID space with correct docs), so I'd still like them addressed before this ships:

1. HelpLinkUri will 404 for every rule (docs are versioned)

Both DiagnosticDescriptorFactory implementations (src/ModularPipelines.Analyzers/.../DiagnosticDescriptorFactory.cs:11 and src/ModularPipelines.Development.Analyzers/DiagnosticDescriptorFactory.cs:7) still hardcode https://thomhurst.github.io/ModularPipelines/docs/analyzers/ + id. docs/versions.json still only contains ["3.x"], and docs/versioned_docs/version-3.x/ has no analyzers/ directory. With Docusaurus's default versioning, the bare /docs/... path serves the last released version (3.x), so every new page under docs/docs/analyzers/ is only reachable at /docs/next/analyzers/... until the next version cut — every help link an IDE surfaces today 404s. Either point the base URL at .../docs/next/analyzers/ for now, add the pages under docs/versioned_docs/version-3.x/analyzers/ too, or add a Docusaurus-level redirect.

2. The ID migration table is still missing 5 renames this PR performs

docs/docs/how-to/analyzers.md's migration table still doesn't list:

• AwaitThis → MP0007
• StatefulModule → MP0008
• MPDEP001 → MP0009
• MPDEP002 → MP0005 (in addition to the existing ConflictingDependsOnAttribute → MP0005 row — MPDEP002 was a separate, previously-unshipped ID for the same rule per AnalyzerReleases.Unshipped.md on the base branch)
• MPDEP003 → MP0010

All five were live/unshipped IDs on the base branch before this PR. Anyone with #pragma warning disable MPDEP001 or a matching .editorconfig entry gets no migration guidance for exactly the IDs this PR renumbers. Since this table is hand-maintained separately from the generator, consider generating it from the same "Removed Rules" Renamed to bookkeeping the generator already parses, so this class of omission becomes structurally impossible rather than relying on someone remembering to update prose by hand.

3. generate-analyzer-docs.mjs still mis-parses a ### Changed Rules section (dormant, not exercised today)

The heading regex still matches Changed|New|Removed, and Changed Rules rows still get destructured as [id, category, severity, ...notes] — the same 4-column shape as New/Removed. Roslyn's standard Changed Rules table is 6 columns (Rule ID | New Category | New Severity | Old Category | Old Severity | Notes), so the old category/severity/notes columns would collapse into description, and since renderIndex interpolates that unescaped into a markdown table cell, embedded | characters would corrupt the generated index.md table the first time any rule's category or severity changes. None of the four release files have a Changed Rules section today, so nothing is broken yet, but it's a live code path, not speculative — worth branching the column destructuring on section === 'changed' now while it's cheap, rather than leaving a landmine for the first future severity/category change.

None of these are inherent to the renumbering approach — the core design (one shared MP#### space, generated docs as source of truth, a factory to remove descriptor boilerplate) remains sound and the right call for this problem.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7d33f637d7

ℹ️ 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 all current review feedback in 5c98a9e233:

• updated the intentional console suppression to MP0004
• routed analyzer help links through /docs/next/analyzers/
• completed all legacy ID migration rows
• fixed Changed Rules release-table parsing
• kept generated sibling links and shipped development mappings

Validation: analyzer tests 68/68, development analyzer tests 30/30, analyzer solution Release build (0 errors), docs generator check (19 pages), focused format clean.

@codex review

[claude]

Code review

Checked 5c98a9e233 against the three items still open from my last review. All confirmed fixed:

• HelpLinkUri 404s: DiagnosticDescriptorFactory.cs in both ModularPipelines.Analyzers and ModularPipelines.Development.Analyzers now point at .../docs/next/analyzers/, matching where the new pages actually resolve given docs/versions.json still only has 3.x and there's no analyzers/ folder under versioned_docs/version-3.x/. The two test files' local DocumentationBaseUrl constants were updated to match, so AnalyzerMetadataTests still assert against the real URL.
• Migration table gaps: docs/docs/how-to/analyzers.md now includes all five previously-missing rows (AwaitThis→MP0007, StatefulModule→MP0008, MPDEP001→MP0009, MPDEP002→MP0005, MPDEP003→MP0010).
• Changed Rules parsing landmine: generate-analyzer-docs.mjs now branches the column destructuring on section === 'changed' (columns.slice(5) for the 6-column Roslyn Changed Rules shape vs. columns.slice(3) for New/Removed), so a future severity/category change won't corrupt index.md with unescaped | characters.

One small, non-blocking thing I noticed in this same commit: AnalyzerMetadataTests.PublicRulesUseUnifiedIdsAndHelpLinks switched from the order-insensitive CollectionAssert.AreEquivalent (the fix applied in response to my first review's "Minor" note) back to Assert.AreSequenceEqual, which is order-sensitive by default. It still passes today because the rules array happens to be declared in ID order, but it reintroduces the original fragility — a future contributor reordering that array for readability (with no ID change) would fail the test for an unrelated reason. Since MSTest 4.3+'s Assert.AreSequenceEqual supports SequenceOrder.InAnyOrder as an explicit overload, passing that would keep the stronger sequence-comparison API while restoring order-insensitivity, e.g.:

Assert.AreSequenceEqual(
    Enumerable.Range(1, 10).Concat(Enumerable.Range(13, 7)).Select(index => $"MP{index:0000}"),
    rules.Select(rule => rule.Id),
    SequenceOrder.InAnyOrder);

Not blocking — the core design (one shared MP#### space, generated docs as source of truth, factory-based descriptor construction) is sound and all the substantive issues from earlier passes are resolved.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: 5c98a9e233

ℹ️ 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".