docs(agents): walk into package registrations when re-deriving #271

[mforce]

AGENTS.md tells you to re-derive the #271 single-serving-instance blocker list by walking every AddSingleton/AddHostedService under src/. That walk cannot see a whole class of registration, and the wording currently reads as if it can.

The gap

A NuGet package's own IServiceCollection extension registers services that never appear under src/. Only the one-line call site does, and it looks like any other registration.

Surfaced while designing MCP support (#770). ModelContextProtocol.AspNetCore's WithHttpTransport registers, unconditionally, from one call:

builder.Services.TryAddSingleton<StatefulSessionManager>();
builder.Services.TryAddSingleton<StreamableHttpHandler>();
builder.Services.TryAddSingleton<SseHandler>();
builder.Services.AddHostedService<IdleTrackingBackgroundService>();

Three singletons and a hosted service. AGENTS.md's stated count ("19 AddSingleton call sites and 1 AddHostedService") would stay unchanged while the live registrations changed.

Two of three independent reviewers followed the documented walk correctly and still concluded "HTTP transport adds no AddHostedService". Only the one who read the package source got it right. That is the tell: the method, not the reviewers.

The outcome in #770 was benign — IdleTrackingBackgroundService.StartAsync returns early under SessionMode.Stateless, so it is registered but inert, and the blocker list genuinely did not change. That is luck, not method. AGENTS.md's own rule says two misses of the same shape mean the method is wrong; this is a third shape, and it is the one the current wording cannot catch by construction.

What changes

• AGENTS.md — the walk now explicitly includes what each package Add*/With* extension Program.cs calls registers internally, with the recipe to answer it.
• docs/decisions/271-single-serving-instance.md — the narrative, added to the existing "How this list was derived, because it was twice derived wrongly" section, which is where it belongs.

The recipe, tested

graphify clone https://github.com/modelcontextprotocol/csharp-sdk --branch v2.2.0
cd ~/.graphify/repos/modelcontextprotocol/csharp-sdk
graphify update . --no-cluster          # AST-only, no LLM, no API cost
graphify explain "WithHttpTransport"

Output, verbatim — exactly the four registrations, with line numbers:

Node: .WithHttpTransport()
  Source: src/ModelContextProtocol.AspNetCore/HttpMcpServerBuilderExtensions.cs L27
  --> StatefulSessionManager          ...HttpMcpServerBuilderExtensions.cs:L31
  --> StreamableHttpHandler           ...HttpMcpServerBuilderExtensions.cs:L32
  --> SseHandler                      ...HttpMcpServerBuilderExtensions.cs:L33
  --> IdleTrackingBackgroundService   ...HttpMcpServerBuilderExtensions.cs:L34

graphify merge-graphs also works for cross-repo traversal (verified: 25,319 nodes, nodes namespaced repo:: / csharp-sdk::), but is not needed for the enumeration step and is not part of the documented recipe.

Two caveats kept in the text deliberately

Both are load-bearing and easy to drop on a re-tell:

1. It graphs the package's published source at a tag, not the restored assembly. Read the version from packages.lock.json rather than typing a branch, or the graph faithfully describes code that is not what restores.
2. It needs the package to have public source at all. True of the MCP SDK; not universal.

What this does NOT claim

No current blocker is missing and #271 is not violated. The walk was re-run for #770 and its conclusion held. The claim is narrower: the walk's scope cannot see this class of registration, so that conclusion holding was not guaranteed by the method.

Still open, deliberately not in this PR

The stronger fix is a test that builds the real service collection and asserts IHostedService descriptors and singleton lifetimes against a reviewed inventory — the only form that catches a package upgrade changing registrations under an unchanged call site, which neither prose nor an on-demand graph query will. It has its own design question (what the inventory is keyed by — and per AGENTS.md, not by file:line), so it is named in the decision record and left for a follow-up rather than bolted on here.

Closes #786

Summary by CodeRabbit

• Documentation
  • Expanded deployment guidance to include services registered indirectly by package extensions.
  • Added instructions for reviewing package source at the locked version when verifying single-instance behavior.
  • Documented an identified case where an indirectly registered hosted service remains inactive in stateless mode.
  • Added guidance for validating hosted-service registrations and singleton lifetimes against a reviewed inventory.

[mforce]

@coderabbitai review

Docs-only: an AGENTS.md wording change plus the matching narrative in docs/decisions/271-single-serving-instance.md.

Two things worth your attention specifically:

1. The claim that WithHttpTransport registers three singletons and a hosted service unconditionally, and that IdleTrackingBackgroundService.StartAsync returns early under SessionMode.Stateless — both from modelcontextprotocol/csharp-sdk at tag v2.2.0. If either is wrong the whole rationale collapses.
2. Whether the new AGENTS.md sentence is still readable. That bullet is already dense and I have added to it; if the result is unparseable, say so — a rule nobody finishes reading is not a rule.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 4036d821-ad4f-4dab-9251-5f671cae4a2c

📥 Commits

Reviewing files that changed from the base of the PR and between 7193ebe and 4305f19.

📒 Files selected for processing (2)

• AGENTS.md
• docs/decisions/271-single-serving-instance.md

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The deployment-invariant guidance now includes service registrations made inside package extensions. The decision record documents package-source inspection, version matching, and a proposed service-collection test.

Changes

Package registration coverage

Layer / File(s)	Summary
Document package registration gap AGENTS.md, docs/decisions/271-single-serving-instance.md	The derivation method now covers registrations made by package Add* and With* extensions. The decision record documents the hosted-service example and its stateless behavior.
Add package inspection and validation procedure docs/decisions/271-single-serving-instance.md	The decision record specifies graphify inspection at the packages.lock.json version and proposes assertions over hosted-service descriptors and singleton lifetimes.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Other · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 4305f

This PR clarifies how to inspect package registrations without changing runtime behavior. No merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 7

✅ Passed checks (7 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description clearly explains the scope gap, documents the changes, records verification details, links issue #786, and identifies follow-up work. It omits the template's explicit Checklist section…
Linked Issues check	✅ Passed	The description explicitly addresses issue #786 and aligns the documented changes with its requested scope correction.
Out of Scope Changes check	✅ Passed	The changes are limited to AGENTS.md and the related decision record. The runtime service-collection test is correctly identified as follow-up work rather than added out of scope.
Title check	✅ Passed	The title uses a conventional commit format and clearly states that package registrations are included when re-deriving decision #271.
Linked Issues check	✅ Passed	The PR satisfies the coding and documentation requirement in #786. AGENTS.md expands the blocker walk to include registrations made inside package Add* and With* extensions called by `Program.cs…
Out of Scope Changes check	✅ Passed	The changed files are AGENTS.md and the related #271 decision record. The added guidance, package inspection commands, example, caveats, and follow-up test note all support the scope correction requ…
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch docs/786-package-registration-walk

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- PR diff: AGENTS.md and decision record ---'
git diff --unified=80 'origin/HEAD...HEAD' -- AGENTS.md docs/decisions/271-single-serving-instance.md || \
git diff --unified=80 HEAD^ HEAD -- AGENTS.md docs/decisions/271-single-serving-instance.md

echo '--- Current AGENTS.md relevant section ---'
rg -n -C 12 'single-serving|AddSingleton|AddHostedService|WithHttpTransport|package' AGENTS.md || true

echo '--- Current decision record relevant section ---'
rg -n -C 15 'How this list was derived|WithHttpTransport|IdleTrackingBackgroundService|published source|packages.lock' docs/decisions/271-single-serving-instance.md || true

base='https://raw.githubusercontent.com/modelcontextprotocol/csharp-sdk/v2.2.0'
echo '--- HttpMcpServerBuilderExtensions.cs at v2.2.0 ---'
curl --fail --silent --show-error \
  "$base/src/ModelContextProtocol.AspNetCore/HttpMcpServerBuilderExtensions.cs" | \
  nl -ba | sed -n '20,50p'

echo '--- IdleTrackingBackgroundService.cs at v2.2.0 ---'
curl --fail --silent --show-error \
  "$base/src/ModelContextProtocol.AspNetCore/IdleTrackingBackgroundService.cs" | \
  nl -ba | sed -n '1,220p'

Length of output: 88916

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- ModelContextProtocol.AspNetCore lock entries ---'
rg -n -C 5 '"ModelContextProtocol\.AspNetCore"' -g 'packages.lock.json' .

echo '--- WithHttpTransport call sites in source ---'
rg -n -C 8 '\.WithHttpTransport\s*\(' src

echo '--- PR changed files and whitespace check ---'
git diff --name-status 'origin/HEAD...HEAD' || git diff --name-status HEAD^ HEAD
git diff --check 'origin/HEAD...HEAD' || git diff --check HEAD^ HEAD

echo '--- HTTP transport mode configuration around call site ---'
rg -n -C 12 'HttpServerSessionMode|SessionMode|McpServer' src

Length of output: 956

---

@mforce No actionable findings.

I verified both package-source claims against modelcontextprotocol/csharp-sdk tag v2.2.0:

• WithHttpTransport unconditionally calls TryAddSingleton for StatefulSessionManager, StreamableHttpHandler, and SseHandler. It also calls AddHostedService<IdleTrackingBackgroundService>().
• IdleTrackingBackgroundService.StartAsync returns Task.CompletedTask when SessionMode is HttpServerSessionMode.Stateless.

The revised AGENTS.md bullet is dense, but it remains readable. It gives a clear sequence: walk src/, inspect package extension registrations at the version resolved by packages.lock.json, then classify exclusions deliberately. The explicit statement that src/ cannot expose package-internal registrations is useful and unambiguous.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.