Make context.Tools the canonical tool API - #3574
Conversation
Teach the discoverable tool surface across README, templates, generated references, and current docs while retaining context.X() as the pre-C# 14 compatibility fallback and naming it in MPG0008.
|
Too many files changed for review. ( Bypass the limit by tagging |
|
Head
@codex review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Make context.Tools the canonical tool API (#3574)
Verified this is a doc/example-migration PR on top of already-shipped context.Tools.X functionality (the C# 14 extension-member generator already exists on main, this PR doesn't add new runtime behavior beyond the MPG0008 message tweak). Checked the generator changes, template changes, and a broad sample of the ~100 changed markdown files.
Bug: broken compile-time examples in two distributed-mode docs
docs/docs/distributed/getting-started.md (lines ~111-117) and docs/docs/distributed/github-actions.md (lines ~76-82) had these lines removed:
-using ModularPipelines.Distributed.Extensions;
-using ModularPipelines.Distributed.Redis.Extensions;These two using statements are unrelated to the tool-accessor migration this PR is doing everywhere else (stripping using ModularPipelines.<Tool>.Extensions; because context.Tools.X no longer needs it). They back the calls used later in the very same snippets:
builder.AddDistributedMode(...)is defined inModularPipelines.Distributed.Extensions(src/ModularPipelines/Distributed/Extensions/DistributedPipelineBuilderExtensions.cs)builder.AddRedisDistributedCoordinator(...)is defined inModularPipelines.Distributed.Redis.Extensions(src/ModularPipelines.Distributed.Redis/Extensions/RedisDistributedExtensions.cs)
Neither namespace is a global/implicit using in those projects. With the using lines gone, both snippets no longer compile (CS1061) if copy-pasted as-is — PipelineBuilder won't expose AddDistributedMode/AddRedisDistributedCoordinator without the import. This looks like collateral damage from a search/replace that matched using ModularPipelines.Distributed* broadly rather than the intended using ModularPipelines.<Tool>.Extensions pattern.
It also explains why this slipped past the PR's own validation: the "documentation snippets Release build" only compiles test/ModularPipelines.DocumentationSnippets/CurrentApiSnippets.cs, which doesn't include the distributed-mode examples, so there's no compiled coverage over these two files.
Fix: restore the two removed using lines in both files (they're independent of the context.Tools change and should stay).
Everything else checked out
- Spot-checked ~15 of the
mp-packages/*andmp-packages/cli/*doc diffs plusREADME.md,fundamentals.md,sub-modules.md,logging.md,source-generator-diagnostics.md,azure-example.md,dotnet-test-build-publish.md— theusing Xxx.Extensions;removals there correctly correspond to the tool actually used in each snippet. MarkdownDocumentationGenerator/DependencyRegistrationGeneratorchanges correctly regenerate docs withcontext.Tools.Xas primary andcontext.X()documented as the compatibility fallback, matching the updatedMarkdownDocumentationGeneratorTestsexpectations.MPG0008diagnostic message format string change (GeneratorDiagnostics.cs) — traced the new{2}placeholder back toToolProperty.Name(the short accessor name, e.g."Git") vs.{0}which is the fully-qualifiedMethodName; the resulting message correctly reads...or call the compatibility accessor context.Git(), matching the new unit test assertion.- Template modules (
BuildModule.cs,PublishModule.cs,RestoreModule.cs,TestModule.cs) correctly drop the now-unneededModularPipelines.DotNet.Extensionsimport and switch tocontext.Tools.DotNet.
No architectural concerns — this is a mechanical, well-scoped rename/doc pass. The one finding above is a straightforward regression to fix before merge, not a design issue.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ba132e233
ℹ️ 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".
|
Addressed both review findings: restored required distributed builder extension imports in all affected snippets, and scoped the MPG0008 documentation to locally declared integration accessors. Source-generator integration tests pass (15/15); documentation snippets build with 0 errors. Branch updated from main. @codex review |
There was a problem hiding this comment.
Follow-up review: Make context.Tools the canonical tool API (#3574)
Re-reviewed after the "fix tool API review findings" commit (351f0ab). The regression flagged in the prior pass is resolved:
docs/docs/distributed/getting-started.mdanddocs/docs/distributed/github-actions.mdboth haveusing ModularPipelines.Distributed.Extensions;andusing ModularPipelines.Distributed.Redis.Extensions;restored, so thebuilder.AddDistributedMode(...)/builder.AddRedisDistributedCoordinator(...)snippets compile again.
Re-checked the rest of the diff (all 110 files):
- Spot-checked another batch of
mp-packages/*docs (docker.md,git.md,node.md, plus the earlier sample) — eachusing Xxx.Extensions;removal correctly corresponds to the tool actually used in that file's snippet, andcontext.Tools.X/context.Tools.Node.Npmstyle multi-accessor cases are handled correctly (e.g.node.md'sNpm/Npx/Nvm/Pnpmlist). MarkdownDocumentationGeneratorandDependencyRegistrationGeneratorchanges are consistent:context.Tools.Xis now the primary documented surface,context.X()is documented as the compatibility fallback, andBuildInvocationcorrectly emitscontext.Tools.{prefix}.{...}for both the flat and sub-domain invocation paths.MPG0008diagnostic now names the exact fallback accessor (context.Git()etc.) via the addedfirstProperty.Nameargument, andModularPipelinesIntegrationGeneratorTestsasserts on the new message text — good test coverage for what was previously just a diagnostic string change.- Template modules (
BuildModule,PublishModule,RestoreModule,TestModule) consistently drop theModularPipelines.DotNet.Extensionsimport and usecontext.Tools.DotNet.
No further issues found. This is a mechanical, well-scoped rename/doc migration with no architectural concerns — approving from a code-review standpoint (the previously blocking regression is fixed).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57422c02fd
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
Review: Make context.Tools the canonical tool API (#3574)
Reviewed the two source-generator code changes (GeneratorDiagnostics.cs, ModularPipelinesIntegrationGenerator.cs), the options-generator template changes that drive the ~100 regenerated markdown files, the templates package, and a broad sample of the changed docs.
Correctness
- The
MPG0008message change threadsEscapeIdentifier(firstProperty.Name)through as the new{2}placeholder to name the exact pre-C#14 compatibility accessor, including the@class-style escaping for keyword-shaped tool method names. The newKeyword_Tool_Accessor_On_Older_Language_Version_Is_Escaped_In_Diagnostictest exercises this directly and the existingcontext.Git()test continues to pass with the added assertion — good coverage for what could easily have been an off-by-one betweenName(bare symbol name) andMethodName(full display string used elsewhere in the same diagnostic). - Checked the regression flagged in the earlier
claudereview (missingusing ModularPipelines.Distributed.Extensions;/using ModularPipelines.Distributed.Redis.Extensions;indocs/docs/distributed/getting-started.mdandgithub-actions.mdafter the.Extensionsimport sweep) — confirmed both files have the imports restored on the current head, so that fix stuck. - Spot-checked that
docs/versioned_docs/version-3.x/**and the currentdocs/docs/migrating-to-v3.md/examples/fsharp-interactive.md/examples/single-file-csharp.mdintentionally retain the oldcontext.X()call style, consistent with the PR's stated goal of preserving v3-migration and F# compatibility examples rather than an incomplete sweep. - The
MarkdownDocumentationGenerator/DependencyRegistrationGeneratorchanges and their test updates are consistent with each other and with the generateddocs/docs/mp-packages/**output (verifiedterraform.mdend-to-end: generator template →context.Tools.Terraformexample + compatibility-fallback sentence → matches committed file).
Design note (non-blocking)
This PR is a pure surface-migration/documentation change — it doesn't introduce new abstractions, so there isn't much to critique architecturally. The one thing worth flagging for a future PR rather than this one: MPG0008's message is now doing three jobs (explain the C#14 gap, name the language version, and name the fallback accessor) via three positional format args threaded through two separate call sites (ModularPipelinesIntegrationGenerator.cs and the doc in source-generator-diagnostics.md). That's fine at the current size, but if a fourth variant is ever needed, consider extracting a small DiagnosticMessageBuilder-style helper so the format-string/positional-arg contract isn't duplicated between the generator and its documentation.
Verdict
No bugs found in this pass. Prior review findings (broken distributed-mode doc imports) are confirmed resolved. Approving from a code-quality standpoint — deferring to CI (currently pending) for the full build/test/docs-site validation the PR description already ran locally.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56d8ded3d9
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Code review
Following up on the earlier claude reviews on this PR — the first two rounds of feedback (missing using statements in the distributed-mode docs, and the MPG0008 escaping fix) were both confirmed addressed on the code as of commit e33f7fca. However, one more commit landed after that review (1009f98b0b8266b11121b9985090bcf60e4275a4, "docs(generator): name fallback namespace") that hasn't been reviewed yet, and it reintroduces the same class of bug that was already fixed once in this PR.
Bug: generated CLI reference docs are now out of sync with the generator template
1009f98b changes the fallback sentence in MarkdownDocumentationGenerator.cs from:
The
context.X()extension method remains available as a compatibility fallback for projects older than C# 14.
to:
For projects older than C# 14, import
{Namespace}.Extensionsand use thecontext.X()extension method as a compatibility fallback.
MarkdownDocumentationGeneratorTests.cs was updated to assert the new wording, and the PR description's "Generated fallback-doc follow-up" section claims "Generated CLI reference pages now name the required {tool.TargetNamespace}.Extensions import before recommending the pre-C# 14 context.X() fallback."
That claim doesn't hold for the committed output, though — the ~35 generated pages under docs/docs/mp-packages/cli/*.md were never regenerated, so they still contain the old sentence with no mention of the required namespace import. Confirmed by sampling several files at the current head (4b650c661c):
docs/docs/mp-packages/cli/terraform.md#L16docs/docs/mp-packages/cli/helm.md#L16docs/docs/mp-packages/cli/vault.md#L16docs/docs/mp-packages/cli/snyk.md#L16docs/docs/mp-packages/cli/kubectl.md#L16
All five still read "The context.X() extension method remains available as a compatibility fallback..." with no import instruction — same as before 1009f98b. This is the exact same failure mode as the earlier-fixed regression (generator/template changed, generated output not refreshed to match), just on a different file set. Since the Generate CLI Options workflow only runs on a weekly schedule or manual dispatch (not on this PR), nothing else will catch or fix this automatically before merge.
Suggested fix: regenerate the affected docs (dotnet run the ModularPipelines.OptionsGenerator for the CLI tools under docs/docs/mp-packages/cli/, or run the generator for all tools) and commit the refreshed markdown alongside the template/test change, so the shipped docs actually match what the PR description says they do.
Everything else
No new issues beyond the above — the rest of the migration (README, examples, source-generator diagnostic changes, templates) was already reviewed in the prior rounds and those findings were addressed.
|
Addressed the generated-doc review: synchronized all 37 CLI reference pages with the generator wording and verified every page uses its declared namespace's .Extensions import. MarkdownDocumentationGeneratorTests: 23/23. @codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Review: Make context.Tools the canonical tool API (#3574)
Re-verified at the current head (410379134). This is a large but mechanical surface migration: it promotes the already-shipped context.Tools.X (C# 14 extension-member) API to canonical status across README, templates, current guides, examples, and the ~100 generated CLI reference docs, while keeping context.X() documented as a pre-C#14 compatibility fallback. It does not introduce new runtime behavior beyond the MPG0008 diagnostic message improvement.
Checked against prior review history
Several rounds of claude/codex review already ran on this PR and flagged real regressions. All of them are confirmed resolved on the current head:
- The stripped
using ModularPipelines.Distributed.Extensions;/using ModularPipelines.Distributed.Redis.Extensions;imports indocs/docs/distributed/getting-started.mdandgithub-actions.mdare restored — those snippets compile again. MPG0008's compatibility-accessor name is now escaped viaEscapeIdentifier(firstProperty.Name)inModularPipelinesIntegrationGenerator.cs, so a keyword-shaped accessor like@classrenders ascontext.@class()instead of the invalidcontext.class(). Covered by the newKeyword_Tool_Accessor_On_Older_Language_Version_Is_Escaped_In_Diagnostictest.fundamentals.md's MPG0008 callout is now correctly scoped to "a project [that] declares an integration accessor in its own compilation" — it no longer implies the warning fires for ordinary consumers of a precompiled tool package.- The generated CLI reference pages (spot-checked
terraform.md,helm.md,az.md,vault.md,snyk.md,kubectl.md) now match the currentMarkdownDocumentationGeneratortemplate exactly, including the "import{Namespace}.Extensions" instruction that a previous commit had temporarily desynced from the committed docs.
Own verification
- Traced
GeneratorDiagnostics.cs'sUnsupportedToolsLanguageVersionformat string ({0}/{1}/{2}) against its two call sites — argument count and meaning line up correctly (MethodName, language version, escapedName). DependencyRegistrationGenerator/MarkdownDocumentationGeneratorchanges are consistent with each other and with the regenerateddocs/docs/mp-packages/**output I diffed against the template logic directly.- Confirmed the intentionally-preserved old-style
context.X()examples (migrating-to-v3.md,examples/fsharp-interactive.md,examples/single-file-csharp.md,versioned_docs/version-3.x/**) are exactly the ones called out in the PR description as deliberately retained (v3 migration history and F# compatibility), not stragglers from an incomplete sweep. - Template modules (
BuildModule,PublishModule,RestoreModule,TestModule) correctly drop the now-unneededModularPipelines.DotNet.Extensionsimport and switch tocontext.Tools.DotNet.
Non-blocking note
No architectural concerns — this PR doesn't introduce new abstractions, so there isn't much to critique there. The one thing worth watching for a future PR: MPG0008's message now threads three positional format args (language-gap explanation, language version, fallback accessor name) through two independently-maintained places — the generator's format string and its prose description in source-generator-diagnostics.md. That's fine at the current size, but as this PR's own history shows (the generated-docs-desync regression that was caught and fixed mid-review), generator-output-vs-committed-doc drift is the recurring failure mode here. If more diagnostic variants get added, a shared source of truth (or a CI check that regenerates and diffs the CLI docs on every PR touching the generator, not just the weekly scheduled job) would close that gap structurally rather than relying on review to catch it each time.
Verdict
No new issues found. All previously-flagged regressions are fixed on the current head. Approving from a code-quality standpoint.
There was a problem hiding this comment.
Code review
Reviewed the full diff (100 files: README/template/doc-page renames plus the source-generator, options-generator, and template/test code). Ran independent passes for CLAUDE.md compliance, doc/generator consistency, and bug-hunting on the non-markdown files. No issues found.
What was checked:
- Generator ↔ generated-doc consistency:
MarkdownDocumentationGenerator.cs's new intro sentence andcontext.Tools.{Prefix}invocation builders were diffed against all 37 regenerateddocs/docs/mp-packages/cli/*.mdpages — the committed output matches exactly what the updated generator now emits, so regeneration is a no-op. This also satisfies the root CLAUDE.md rule that generated files shouldn't be hand-edited independently of their generator. - MPG0008 diagnostic: the format string gained a third
{2}placeholder for the escaped compatibility-accessor name (GeneratorDiagnostics.cs). Confirmed the single call site inModularPipelinesIntegrationGenerator.cs#L302-L311passesfirstProperty.Name(notMethodName) throughEscapeIdentifier, which correctly producescontext.@class()for keyword-named accessors — matches the newKeyword_Tool_Accessor_On_Older_Language_Version_Is_Escaped_In_Diagnostictest. - Removed
using ...Extensions;in templates/snippets: verifiedcontext.Tools.Xis served by the extension member emitted intoModularPipelines.Context(already imported), so dropping the tool-specific.Extensionsimport inBuildModule.cs/PublishModule.cs/RestoreModule.cs/TestModule.cs/CurrentApiSnippets.csdoesn't break compilation. BuildInvocation/GetNavigationSegmentsprefix slicing: all branches now consistently emitcontext.Tools.{Prefix}., so theinvocation[prefix.Length..]slice in the test helper can't throw or desync from the generator's actual output.
One non-blocking, forward-looking note (not a defect in this PR — just flagging for a possible follow-up): the linked issue #3486 also floated "consider shipping pre-generated extension(IToolsContext) members in each tool package so Tools.X works without the consumer-side generator." This PR intentionally scopes to the docs/messaging fix and keeps context.Tools.X dependent on the consumer project both running the source generator and being on C# 14+ (with context.X() as the documented fallback otherwise). That's a reasonable scope cut, but the dual-API surface (canonical + fallback) will need an explicit deprecation story eventually if context.X() is ever meant to go away in v4 — worth a tracking issue if that's still the plan, rather than letting the two idioms coexist indefinitely.
Nice work keeping this mechanical rename fully in sync between the generator and its generated output, and for adding the keyword-escaping test coverage.
Review: #3574 — Make
|
1 participant
Closes #3486
Summary
context.Tools.Xcanonical across README, templates, current guides, examples, and all tool-package referencescontext.X()as the documented compatibility fallback for pre-C# 14 projectsMPG0008name the exact fallback accessor, including escaped C# keyword identifiersValidation
ModularPipelinesIntegrationGeneratorTests: 16/16MarkdownDocumentationGeneratorTests: 23/23context.Tools.DotNetand version token was replacedgit diff --checkpassedGenerated fallback-doc follow-up
{tool.TargetNamespace}.Extensionsimport before recommending the pre-C# 14context.X()fallback.MarkdownDocumentationGeneratorTests: 23/23 passed; scoped whitespace formatter clean.