[mforce]

Records the design for MCP server support. Design only — no code, nothing shipped.

#770 was a feasibility spike. This is the design that came out of it, produced by running three independent design candidates in parallel (one per model), cross-judging them on a rubric, and synthesizing a base plus grafts. Lands under docs/plans/ rather than docs/decisions/, because a decision record is the relocated rationale for a rule that also lives compressed in AGENTS.md — this is a design for unshipped work, which is what docs/plans/ is defined to hold.

What the spike got wrong

Four premises did not survive verification against the SDK source at the v2.2.0 tag and against the repo. Each is recorded with evidence in 04-verified-findings.md.

1. ScopeRequests is not the load-bearing setting. Under SessionMode.Stateless the SDK sets it itself:

// StreamableHttpHandler.CreateSessionAsync, v2.2.0 L554-558
if (serveStatelessly)
{
    mcpServerServices = context.RequestServices;
    mcpServerOptions.ScopeRequests = false;
}

So the realistic regression is not someone deleting our ScopeRequests = false line — it is someone changing SessionMode, after which tools silently revert to a fresh root scope. Under StatefulForInitializeClients that is per-request mixed. Two of the three candidates built their whole safety argument on the redundant line, and all three proposed a guard on it that would redden on a mutation leaving the product safe.

2. MCP does not function at all until /mcp is exempted from IdempotencyMiddleware. Not a key collision — a hard 400 on every request including initialize and tools/list, because MCP Streamable HTTP sends everything as POST, /mcp is not in the exempt list, an authenticated call resolves a tenant, and no MCP client knows Cluckwork's Idempotency-Key header contract. This is phase-zero and blocking even for a read-only demo, which the issue sized at 1–3 days without accounting for it.

3. #271 is touched. WithHttpTransport registers, unconditionally and regardless of session mode:

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

The blocker list is not extended — IdleTrackingBackgroundService.StartAsync returns early under Stateless, so the service is registered but inert. But AGENTS.md tells you to re-derive that list by "walking every AddSingleton/AddHostedService under src/", and that walk is structurally blind to a package registration. AGENTS.md already records the list as "twice derived wrongly"; by its own two misses of the same shape mean the METHOD is wrong rule, the walk's scope is the defect. Filed separately.

4. .NET 10 compat is confirmed. ModelContextProtocol.AspNetCore 2.2.0 ships a native lib/net10.0, answering the issue's open decision 5.

The design in one paragraph

Stateless HTTP transport on /mcp inside the existing middleware pipeline, so one tool call is one authenticated request and CredentialEpochMiddleware keeps its per-call read (#364). A scoped McpCallContext is the only way a tool learns who is calling; its factory throws unless an HttpContext is present, the injected TenantContext is reference-equal to the request's, tenant + actor + flock scope are all IsResolved, and UserId != Guid.Empty. An assembly walk forbids a tool from injecting TenantContext/CurrentUserContext/FlockScope/AppDbContext/IServiceProvider/HttpContext at all — so the next tool someone adds inherits the guarantee, including RecordWaterUsage, which audits nothing and genuinely reaches FlockScopeGuard's fail-open branch. #307's claim/execute/publish protocol is extracted into IIdempotentUnit keyed by an explicit operation key, with the middleware as the first adapter (byte-identical key) and the write tool as the second (mcp:tool:<name>), which dissolves the single-route collision. Eight domain-question tools over thirteen routes: the write takes a flock name, derives FarmId/HouseId, and money renders at the boundary rather than crossing as minor units.

Reviewer notes

• 02-guards.md is the part worth reading closely. 28 guards, each with the mutation that makes it red, plus a section on what is deliberately not guarded and why. Three encode traps the candidates fell into: asserting FlockScope.IsUnrestricted instead of IsResolved (defaults to true, so the guard passes while the hole stays open); asserting zero IHostedService descriptors (red on the correct configuration); and asserting the daily-entry row count on replay (the handler upserts, so it passes with idempotency removed — assert the absence of the second audit row).
• The IIdempotentUnit extraction is the largest risk in the whole issue, not the tool code. AtomicIdempotencyProtocolTests and the other Make API write idempotency atomic across replicas #307 suites must pass unedited; an edit to one is a stop-and-review.
• All three candidate sketches were build errors: PerSessionExecutionContext is [Obsolete] with a DiagnosticId at 2.2.0 and Directory.Build.props sets TreatWarningsAsErrors=true. It is only read in the stateful branch, so the fix is deletion.
• BodyReadingEndpointTests starts applying the moment /mcp is mapped — MapMcp maps HandlePostRequestAsync(HttpContext) with no IAcceptsMetadata.RequestType, so it needs ReadsRequestBodyAttribute.

Spin-offs filed

Three findings outlive #770 and are filed separately rather than bundled here:

• The #271 blocker walk cannot see registrations made inside a NuGet package #786 — the Background worker has no single-runner guarantee — double-runs if scaled >1 instance #271 blocker walk cannot see registrations made inside a NuGet package. Method fix to AGENTS.md; no current blocker is missing.
• FlockScopeGuard's fail-open branch grants account-wide flock access to an unresolved actor #787 — FlockScopeGuard's fail-open branch. Its own comment has been requesting this since fix(seed): seeded audit events carry "(unresolved)" as the actor, now visible on record History columns #500. This design makes it unreachable from MCP rather than fixing it, deliberately.
• Build an OAuth 2.1 authorization server (OpenIddict) so MCP clients can authenticate #788 — MCP clients expect OAuth discovery and a refreshable token; Cluckwork's refresh is an HttpOnly cookie. Gates real-world usability, not correctness, and should be decided before the tool work is sized.

Closes #770

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: faf4f34d-d7d4-4f86-a035-edf5167a89bb

---

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.}

[mforce]

@coderabbitai review

Docs-only PR, but the prose makes specific, checkable claims about third-party SDK behaviour and about this repo's invariants. Those claims are the deliverable, so please weight correctness over style:

• The quoted SDK snippets are from modelcontextprotocol/csharp-sdk at tag v2.2.0 (WithHttpTransport, StreamableHttpHandler.CreateSessionAsync, IdleTrackingBackgroundService.StartAsync). Please check they are quoted accurately and that the conclusions drawn from them follow.
• 04-verified-findings.md asserts four premises in spike: assess adding MCP server support to Cluckwork (feasibility, pros/cons, phasing) #770 are wrong. If any of those four is itself wrong, that is the most valuable thing you can find here.
• 02-guards.md claims three specific guard formulations are traps that would pass while a hole stays open (FlockScope.IsUnrestricted vs IsResolved; asserting zero IHostedService descriptors; asserting daily-entry row count on replay rather than the absence of a second audit row). Please sanity-check that reasoning against the real FlockScope, RecordDailyEntryHandler and IdempotencyMiddleware.
• No code is proposed for merge. Design only.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== Changed documentation files =='
git diff --name-only "$(git merge-base HEAD origin/main)" HEAD 2>/dev/null || git diff --name-only HEAD~1 HEAD

echo
echo '== Candidate design documents =='
fd -t f -a '02-guards\.md|04-verified-findings\.md|AGENTS\.md' . \
  -E .git -E node_modules

echo
echo '== Relevant repository symbols =='
rg -n -C 5 --glob '*.cs' \
  'class\s+FlockScope\b|record\s+FlockScope\b|struct\s+FlockScope\b|IsUnrestricted|IsResolved|class\s+RecordDailyEntryHandler\b|RecordDailyEntryHandler\s*\(|class\s+IdempotencyMiddleware\b|IdempotencyMiddleware\s*\(' \
  . -g '!**/bin/**' -g '!**/obj/**' || true

echo
echo '== Package references for ModelContextProtocol =='
rg -n -C 2 'ModelContextProtocol' --glob '*.*proj' --glob 'Directory.Packages.props' --glob 'packages.lock.json' . || true

echo
echo '== SDK v2.2.0: transport registrations =='
curl -fsSL --retry 2 \
  'https://raw.githubusercontent.com/modelcontextprotocol/csharp-sdk/v2.2.0/src/ModelContextProtocol.AspNetCore/Extensions/McpServerBuilderExtensions.cs' \
  | grep -n -A35 -B10 'WithHttpTransport' || true

echo
echo '== SDK v2.2.0: CreateSessionAsync stateless behavior =='
curl -fsSL --retry 2 \
  'https://raw.githubusercontent.com/modelcontextprotocol/csharp-sdk/v2.2.0/src/ModelContextProtocol.AspNetCore/Handlers/StreamableHttpHandler.cs' \
  | grep -n -A35 -B10 'CreateSessionAsync' || true

echo
echo '== SDK v2.2.0: idle tracker start behavior =='
curl -fsSL --retry 2 \
  'https://raw.githubusercontent.com/modelcontextprotocol/csharp-sdk/v2.2.0/src/ModelContextProtocol.AspNetCore/Handlers/IdleTrackingBackgroundService.cs' \
  | grep -n -A45 -B10 'StartAsync' || true

Length of output: 187344

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== Design documents under review =='
for f in \
  docs/plans/770-mcp-server/00-README.md \
  docs/plans/770-mcp-server/01-design.md \
  docs/plans/770-mcp-server/02-guards.md \
  docs/plans/770-mcp-server/03-arena-synthesis.md \
  docs/plans/770-mcp-server/04-verified-findings.md \
  docs/plans/README.md; do
  echo
  echo "----- $f -----"
  cat -n "$f"
done

echo
echo '== Guard-dependent implementation ranges =='
sed -n '1,120p' src/Cluckwork.Infrastructure/Persistence/FlockScope.cs
sed -n '45,190p' src/Cluckwork.Api/Middleware/IdempotencyMiddleware.cs
sed -n '1,260p' src/Cluckwork.Application/Features/DailyEntries/RecordDailyEntry/RecordDailyEntryHandler.cs

echo
echo '== Idempotency exemptions and pipeline ordering =='
rg -n -C 12 'Exempt|excluded|Path|IdempotencyMiddleware|UseMiddleware<.*Idempotency|UseCluckwork.*Idempotency|MapMcp|Map.*Mcp' \
  src/Cluckwork.Api --glob '*.cs'

echo
echo '== Relevant idempotency and daily-entry test assertions =='
rg -n -C 8 'Idempotency|Idempotency-Key|RecordDailyEntry|DailyEntr|AuditEvent|AuditActions' \
  tests --glob '*.cs' -g '!**/bin/**' -g '!**/obj/**'

echo
echo '== csharp-sdk v2.2.0 candidate source paths =='
gh api 'repos/modelcontextprotocol/csharp-sdk/git/trees/v2.2.0?recursive=1' --paginate \
  --jq '.tree[] | select(.path | test("(McpServerBuilderExtensions|StreamableHttpHandler|IdleTrackingBackgroundService)\\.cs$")) | .path'

echo
echo '== csharp-sdk v2.2.0 package target frameworks =='
gh api 'repos/modelcontextprotocol/csharp-sdk/git/trees/v2.2.0?recursive=1' --paginate \
  --jq '.tree[] | select(.path | test("ModelContextProtocol\\.AspNetCore\\.csproj$")) | .path'

Length of output: 202096

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== 04 verified findings =='
cat -n docs/plans/770-mcp-server/04-verified-findings.md

echo
echo '== 02 guard headings and guard sections =='
rg -n '^#{1,3} |FlockScope|IHostedService|daily-entry|DailyEntry|replay|Idempotency' \
  docs/plans/770-mcp-server/02-guards.md
cat -n docs/plans/770-mcp-server/02-guards.md

echo
echo '== 01 design headings and MCP transport/idempotency sections =='
rg -n '^#{1,3} |SessionMode|ScopeRequests|Idempotency|/mcp|IIdempotentUnit|BodyReading|PerSession|IHostedService' \
  docs/plans/770-mcp-server/01-design.md
cat -n docs/plans/770-mcp-server/01-design.md

echo
echo '== Exact csharp-sdk v2.2.0 evidence =='
for path in \
  src/ModelContextProtocol.AspNetCore/HttpMcpServerBuilderExtensions.cs \
  src/ModelContextProtocol.AspNetCore/StreamableHttpHandler.cs \
  src/ModelContextProtocol.AspNetCore/IdleTrackingBackgroundService.cs \
  src/ModelContextProtocol.AspNetCore/ModelContextProtocol.AspNetCore.csproj; do
  echo
  echo "----- $path -----"
  gh api -H 'Accept: application/vnd.github.raw+json' \
    "repos/modelcontextprotocol/csharp-sdk/contents/$path?ref=v2.2.0" \
    | grep -n -E -A40 -B12 \
      'WithHttpTransport|CreateSessionAsync|serveStatelessly|ScopeRequests|StartAsync|SessionMode|TargetFramework|TargetFrameworks|PerSessionExecutionContext'
done

echo
echo '== Exact Cluckwork evidence for MCP blockers and guards =='
sed -n '48,180p' src/Cluckwork.Api/Middleware/IdempotencyMiddleware.cs
sed -n '190,250p' src/Cluckwork.Api/Endpoints/DailyEntries/DailyEntryEndpoints.cs
sed -n '1,240p' src/Cluckwork.Application/Features/DailyEntries/RecordDailyEntry/RecordDailyEntryHandler.cs
sed -n '300,345p' tests/Cluckwork.Api.IntegrationTests/SubmitDailyEntryTests.cs
rg -n -C 8 'BodyReadingEndpointTests|ReadsRequestBodyAttribute|IAcceptsMetadata' tests src/Cluckwork.Api --glob '*.cs'

Length of output: 130093

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== Remaining design documents =='
cat -n docs/plans/770-mcp-server/00-README.md
cat -n docs/plans/770-mcp-server/03-arena-synthesis.md
cat -n docs/plans/README.md

echo
echo '== AGENTS.md rules and current PR changes to AGENTS.md =='
rg -n -C 8 '`#271`|AddSingleton|AddHostedService|twice derived|two misses|blocker' AGENTS.md
git diff --unified=40 "$(git merge-base HEAD origin/main)" HEAD -- AGENTS.md

echo
echo '== Exact SDK claims used in the design =='
for path in \
  src/ModelContextProtocol.AspNetCore/HttpMcpServerBuilderExtensions.cs \
  src/ModelContextProtocol.AspNetCore/McpEndpointRouteBuilderExtensions.cs \
  src/ModelContextProtocol.AspNetCore/HttpServerTransportOptions.cs \
  src/ModelContextProtocol/McpServerOptions.cs \
  src/ModelContextProtocol.AspNetCore/AuthorizationFilterSetup.cs; do
  echo
  echo "----- $path -----"
  gh api -H 'Accept: application/vnd.github.raw+json' \
    "repos/modelcontextprotocol/csharp-sdk/contents/$path?ref=v2.2.0" 2>/dev/null \
    | grep -n -E -A18 -B8 \
      'MapMcp|HandlePostRequestAsync|ScopeRequests|AddAuthorizationFilters|RemoveAt|McpProtocolException|ConfigureSessionOptions|PerSessionExecutionContext|DefaultValue' \
    || true
done

echo
echo '== Package and compiler warning configuration =='
rg -n -C 3 'TreatWarningsAsErrors|ModelContextProtocol.AspNetCore|Version=' \
  Directory.Build.props Directory.Packages.props --glob '*' 2>/dev/null || true

echo
echo '== FlockScopeGuard fail-open implementation =='
fd -t f -a '*FlockScopeGuard*.cs' src
rg -n -C 12 'class\s+FlockScopeGuard\b|CheckAsync\s*\(' src --glob '*.cs'

echo
echo '== Diff statistics =='
git diff --check "$(git merge-base HEAD origin/main)" HEAD
git diff --stat "$(git merge-base HEAD origin/main)" HEAD

Length of output: 52356

---

@mforce I found one documentation consistency issue.

01-design.md states that the AGENTS.md wording “needs amending in this PR.” This PR does not modify AGENTS.md. 00-README.md correctly states that the defect is raised separately. The PR objective identifies that follow-up as #786.

Change the sentence to say that the wording needs amendment in #786, or remove the statement from this design record.

The requested technical checks otherwise pass:

• The v2.2.0 SDK excerpts are accurate.
• SessionMode.Stateless, not ScopeRequests, is the load-bearing setting.
• WithHttpTransport registers the hosted service unconditionally. StartAsync makes it inert in stateless mode.
• An authenticated /mcp POST reaches the missing-Idempotency-Key 400 without an exemption.
• FlockScope.IsUnrestricted defaults to true, while IsResolved defaults to false.
• A repeat RecordDailyEntryHandler call updates the natural-key row. An entry-count assertion would not detect removed idempotency. The absence of a second DailyEntry.Update audit row would detect it.
• MapMcp uses HandlePostRequestAsync(HttpContext) and supplies AcceptsMetadata with no request type. The ReadsRequestBodyAttribute guard is justified.

You are interacting with an AI system.

[mforce]

@coderabbitai Fixed in 7522f4f, and thank you — that was a real inconsistency, not a nit. 01-design.md claimed the AGENTS.md wording "needs amending in this PR" while the PR ships no AGENTS.md change, contradicting 00-README.md one file away. Now reads:

That wording needs amending, tracked separately in #786 — this record ships no AGENTS.md change.

Rather than fix only the line you pointed at, I grepped the whole plan directory for the same ambiguity and found three more, all now corrected in the same commit:

• 02-guards.md row 24 and 01-design.md (tradeoffs) both said "an edit to a Make API write idempotency atomic across replicas #307 test in this PR is a stop-and-review", meaning the future implementing PR. Now says so explicitly — the phrase would otherwise read as a rule about a docs PR that touches no tests.
• An open question asked whether "this PR" should file the FlockScopeGuard issue. It has been filed (FlockScopeGuard's fail-open branch grants account-wide flock access to an unresolved actor #787), so the question is struck through and marked resolved rather than left to rot.

Verification: TenancyDocsFreshnessTests (the tracked-file docs guard) passes; the change is prose-only in docs/plans/.

Noting for the record that your seven technical confirmations line up with what I verified independently against the SDK source at the v2.2.0 tag and against the repo — in particular the FlockScope.IsUnrestricted / IsResolved default asymmetry and the fact that an entry-count assertion cannot detect removed idempotency, which are the two places a plausible-looking wrong fix would have passed silently.

No further changes planned from this round.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@mforce Thank you for the thorough correction.

The revised wording now distinguishes this documentation PR from the future implementation PR. Resolving the FlockScopeGuard follow-up as #787 also keeps the plan current.

🐇 ✓

---

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

You are interacting with an AI system.

[mforce]

Pushed b0a5954 — an independent adversarial review of the synthesized design, folded in. New file: 05-adversarial-review.md.

An independent reviewer on a different model family was handed the design and told to find what is wrong, explicitly barred from re-checking the six facts in 04-verified-findings.md. Four major findings, one minor, no blockers. I checked all five against source before accepting them; all five hold, and four are defects I introduced.

1. The identity bridge screened parameters, not the dependency graph. McpCallContext validates the scope the tool is built in and says nothing about the scope its dependencies use. A tool taking only McpCallContext can inject a helper that opens a scope via IServiceScopeFactory, copy AccountId, and resolve a repository whose fresh FlockScope is unresolved and therefore unrestricted — a #388-narrowed Worker reads every flock. Both original guards passed. The fix is not "add IServiceScopeFactory to the blacklist": a helper one level down reopens it. Guard 7b now walks transitive dependencies with a causal test.

2. The extracted idempotency port had no success/failure contract. Commit eligibility today is an HTTP 2xx (IdempotencyMiddleware.cs:289), with failure releasing the claim so a retry can run (:336). A tool call has no HTTP status, and the SDK converts exceptions into CallToolResult.IsError inside its own dispatch — so /error never sees them. An implementation publishing on "the delegate returned normally" caches a domain failure: record against a depleted flock → FlockNotActive cached → manager reactivates → the identical retry replays the failure forever.

It also proposed a cheaper shape the design never considered — route the write through the existing HTTP endpoint over loopback with the caller's bearer and a tool-namespaced Idempotency-Key, avoiding the #307 extraction entirely. My "Alternatives considered" rejected hashing the JSON-RPC envelope, which is a different idea. Since the extraction is this design's self-declared largest risk, slice 1 should price the loopback shape first and only extract #307 if it loses on evidence. Recorded in the design and on #789.

3. Name-addressed writes had no ambiguity contract. Duplicate flock names are legal — FlockRepository says so and its ThenBy(f => f.Id) tie-break exists because of it. "The model passes a name, not a GUID" was the headline ergonomic win, and it never said what happens when the name is not unique. First match records against an arbitrary flock; the handler cannot catch it, because it receives a valid GUID.

4. No bounded-result contract, and I conflated two mechanisms. PaymentRepository materializes every matching customer group, and the order cap lives in SaleEndpoints, not the repository — so reusing a repository inherits no limit. Worse because it read as covered: I claimed ReadsRequestBodyAttribute handled the body cap. It does not — it classifies binding errors for BodyReadingEndpointTests; the #309 cap reads MaxRequestBodyBytesMetadata. /mcp had no body cap at all, and no rate limit, since #143 is opt-in per endpoint and /mcp is one route carrying every operation.

5. Guard rows 2 and 19 could not go red on their stated mutations. Row 2: deleting the CurrentUserContext.IsResolved clause leaves the test green, because an unresolved actor has UserId == Guid.Empty and row 4's check still throws. Row 19: keying on /mcp does not alias HTTP and MCP, since an HTTP write still hashes its own route. Both fixed. That is the fourth and fifth instance of a trap this design already documented three times — the lesson stands: run the mutation, do not reason about it.

It also refuted something I had suggested investigating: there is no JSON-RPC batch logging defect against #214, because MCP removed batching in protocol revision 2025-06-18. Recorded so nobody reopens it.

Guards went from 28 to 32. TenancyDocsFreshnessTests passes; still docs-only.

@coderabbitai review — this commit landed after your approval, so the approval no longer covers the head SHA. The new content makes falsifiable claims about IdempotencyMiddleware's commit/release semantics, PaymentRepository materialization, and the ReadsRequestBodyAttribute / MaxRequestBodyBytesMetadata distinction. Please check those three specifically.

[coderabbitai]

Your plan includes PR reviews subject to rate limits. More reviews will be available in 15 minutes.

[mforce]

@coderabbitai review

Re-requesting on head d3434db. The branch was rebased onto main after #790 merged, which force-pushed and cleared the review history — the earlier approval covered neither the adversarial-review commit nor the rebase.

Two commits to look at since that approval:

• 4053aca — folds in an independent adversarial review. Four major findings, guards 28→32. Makes falsifiable claims I'd like checked: (a) IdempotencyMiddleware commit eligibility is HTTP 2xx with failure releasing the claim, so an extracted port publishing on "delegate returned normally" would cache a domain failure permanently; (b) PaymentRepository materializes every matching customer group, and the order cap lives in SaleEndpoints rather than the repository, so calling a repository directly inherits no limit; (c) ReadsRequestBodyAttribute does not activate the Bound authentication request bodies and credential inputs before password hashing #309 body cap — that middleware reads MaxRequestBodyBytesMetadata.
• d3434db — refreshes three cross-references that described The #271 blocker walk cannot see registrations made inside a NuGet package #786's AGENTS.md amendment as pending. It merged in docs(agents): walk into package registrations when re-deriving #271 #790, so the claim is now checkable against main.

Also worth a look: 02-guards.md rows 2 and 19 were rewritten because their original mutations could not turn them red. If either replacement mutation still can't, that's the finding I most want.

Docs-only throughout; no source changes.

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"
echo '== Changed files since main =='
git diff --name-only "$base" HEAD

echo
echo '== Documentation changes under review =='
git diff --unified=35 "$base" HEAD -- docs/plans/770-mcp-server

echo
echo '== Maps of relevant implementation files =='
for file in \
  src/Cluckwork.Api/Middleware/IdempotencyMiddleware.cs \
  src/Cluckwork.Infrastructure/Persistence/PaymentRepository.cs \
  src/Cluckwork.Api/Endpoints/Sales/SaleEndpoints.cs; do
  if test -f "$file"; then
    echo "----- $file -----"
    ast-grep outline "$file" --items all || true
  fi
done

echo
echo '== Idempotency middleware: status eligibility and claim release =='
rg -n -C 18 --glob '*.cs' \
  'StatusCode|StatusCodes|IsSuccessStatusCode|2[0-9]{2}|Commit|Publish|Release|Claim|Idempotency' \
  src/Cluckwork.Api/Middleware/IdempotencyMiddleware.cs

echo
echo '== PaymentRepository materialization and sales endpoint limit =='
rg -n -C 14 --glob '*.cs' \
  'class\s+PaymentRepository\b|Task<.*Payment|ToListAsync|ToArrayAsync|Take\s*\(|PageSize|limit|Limit|Max' \
  src/Cluckwork.Infrastructure/Persistence/PaymentRepository.cs \
  src/Cluckwork.Api/Endpoints/Sales/SaleEndpoints.cs

echo
echo '== Request-body metadata attribute versus body-size middleware =='
rg -n -C 14 --glob '*.cs' \
  'ReadsRequestBodyAttribute|MaxRequestBodyBytesMetadata|MaxRequestBodyBytes|BodyReadingEndpointTests|IAcceptsMetadata' \
  src tests

echo
echo '== Guard rows 2 and 19 and their stated mutations =='
rg -n -A16 -B5 '^\|[[:space:]]*(2|19)[[:space:]]*\||Guard 2|Guard 19|CurrentUserContext\.IsResolved|/mcp|alias' \
  docs/plans/770-mcp-server/02-guards.md

echo
echo '== `#786` cross references and main AGENTS.md evidence =='
rg -n -C 4 '`#786`|AGENTS\.md|pending|merged|amend' docs/plans/770-mcp-server AGENTS.md
git show origin/main:AGENTS.md | rg -n -C 5 '`#271`|AddSingleton|AddHostedService|package|registration|blocker' || true

echo
echo '== Documentation consistency checks =='
git diff --check "$base" HEAD
fd -t f -a . docs/plans/770-mcp-server | sort

Length of output: 113131

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== Locate payment repository and body-limit implementation =='
fd -t f -a -i 'PaymentRepository.cs' .
fd -t f -a -i '*Body*Limit*.cs' src tests
fd -t f -a -i '*RequestBody*.cs' src tests

echo
echo '== Payment repository implementation =='
payment_file="$(fd -t f -a -i 'PaymentRepository.cs' . | head -n 1)"
if test -n "$payment_file"; then
  ast-grep outline "$payment_file" --items all || true
  cat -n "$payment_file"
fi

echo
echo '== Sales list implementation after pagination calculation =='
sed -n '242,350p' src/Cluckwork.Api/Endpoints/Sales/SaleEndpoints.cs

echo
echo '== Body cap metadata definitions and middleware reads =='
rg -n -C 12 --glob '*.cs' \
  'class\s+ReadsRequestBodyAttribute|record\s+ReadsRequestBodyAttribute|class\s+MaxRequestBodyBytesMetadata|record\s+MaxRequestBodyBytesMetadata|WithMaxRequestBodyBytes\s*\(|UseCluckworkRequestBodyLimit|MaxRequestBodyBytesMetadata' \
  src tests

echo
echo '== Body-reading test behavior =='
body_test="$(fd -t f -a -i 'BodyReadingEndpointTests.cs' tests | head -n 1)"
if test -n "$body_test"; then
  cat -n "$body_test"
fi

echo
echo '== Exact current guard rows and local document references =='
sed -n '7,65p' docs/plans/770-mcp-server/02-guards.md
rg -n -C 3 '`#786`|PR `#790`|pending|amended|merged' docs/plans/770-mcp-server

echo
echo '== Verify issue `#786` and PR `#790` state =='
gh issue view 786 --repo mforce/cluckwork --json number,state,title,url,closedAt
gh pr view 790 --repo mforce/cluckwork --json number,state,mergedAt,title,url

echo
echo '== Diff quality =='
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"
git diff --check "$base" HEAD

Length of output: 2517

---

🏁 Script executed:

#!/bin/bash
set -u

echo '== Payment repository =='
payment_file="src/Cluckwork.Infrastructure/Repositories/PaymentRepository.cs"
ast-grep outline "$payment_file" --items all || true
cat -n "$payment_file"

echo
echo '== Sales order pagination and repository call =='
sed -n '242,350p' src/Cluckwork.Api/Endpoints/Sales/SaleEndpoints.cs

echo
echo '== Definitions and use of body-reading classification and body-size metadata =='
rg -n -C 12 --glob '*.cs' \
  'ReadsRequestBodyAttribute|MaxRequestBodyBytesMetadata|WithMaxRequestBodyBytes\s*\(|UseCluckworkRequestBodyLimit' \
  src tests || true

echo
echo '== Body-reading test =='
fd -t f -a -i 'BodyReadingEndpointTests.cs' tests -x cat -n {} \; || true

echo
echo '== Guard rows 2 and 19 =='
sed -n '7,52p' docs/plans/770-mcp-server/02-guards.md

echo
echo '== Current `#786` references =='
rg -n -C 3 '`#786`|PR `#790`|pending|amended|merged' docs/plans/770-mcp-server || true

echo
echo '== GitHub state for follow-up =='
gh issue view 786 --repo mforce/cluckwork --json number,state,title,url,closedAt || true
gh pr view 790 --repo mforce/cluckwork --json number,state,mergedAt,title,url || true

echo
echo '== Whitespace check =='
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"
git diff --check "$base" HEAD || true

Length of output: 87100

---

@mforce I found one remaining documentation inconsistency.

docs/plans/770-mcp-server/00-README.md says that 02-guards.md contains 28 guards. The guard table now contains rows through 32.

Update the table entry to say “32 guards” or remove the fixed count.

The requested checks pass:

• IdempotencyMiddleware publishes only for HTTP 2xx responses. A non-2xx response rolls back and releases the claim.
• PaymentRepository.ListCustomerBalancesAsync materializes all matching grouped rows. The sales-order limit is applied in SaleEndpoints.ListSalesOrders, before its repository call.
• ReadsRequestBodyAttribute only classifies body-related binding failures. UseCluckworkRequestBodyLimit reads MaxRequestBodyBytesMetadata.
• Guard 2 now tests the combined actor requirement. Guard 19 now uses the mirrored HTTP route and can create the intended HTTP/MCP key collision.
• The #786 references correctly state that #790 merged the AGENTS.md amendment.

⚠️ Action not completed

Review rate limited.

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.

[mforce]

@coderabbitai Fixed in 968e5b2 — but not the way you suggested, and the difference is the point.

You proposed changing "28 guards" to "32". The table actually has 34 rows: 32 numbered, plus 3b and 7b, which were inserted as sub-rows precisely because they guard the same invariant as their parent from a different angle. So that correction would have shipped a second wrong number a day later.

Removed the count instead. AGENTS.md already records a bare count going stale twice on the #271 blocker list, and its conclusion there was "re-run the walk rather than trusting this sentence" — the same reasoning applies to a guard table that is still growing. The description now points at the table rather than summarizing it.

Thanks for the other five confirmations; each was a claim I would rather have had contradicted than assumed:

• IdempotencyMiddleware publishes only on 2xx, and non-2xx rolls back and releases the claim. That release is what makes the "cached domain failure" scenario real rather than theoretical.
• PaymentRepository.ListCustomerBalancesAsync materializes all grouped rows, and the sales-order limit is applied in SaleEndpoints.ListSalesOrders before its repository call — which is exactly why calling the repository directly from a tool inherits no bound.
• ReadsRequestBodyAttribute classifies binding failures only; UseCluckworkRequestBodyLimit reads MaxRequestBodyBytesMetadata. This was my error and the one I was most concerned had propagated.
• Guards 2 and 19 can now go red on their stated mutations.

Noting your run reported rate limited, so this round completed as comments rather than a formal review. I am not re-triggering for that reason alone — the findings arrived, which is what matters.

An independent adversarial reviewer is separately attacking the fixes themselves right now (round 2, briefed to find what the fixes broke rather than re-review the original). If it finds anything I will push it here before asking for merge.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@mforce, removing the count is the correct correction.

The table has 34 rows: 32 numbered rows plus sub-rows 3b and 7b. A fixed count would become inaccurate again as the guard table changes. Pointing readers to the table avoids that failure mode.

Thanks for identifying the error in the suggested replacement.

---

✏️ Learnings added

Learnt from: mforce
URL: https://github.com/mforce/cluckwork/pull/785

Timestamp: 2026-09-12T18:57:06.884Z
Learning: For `docs/plans/770-mcp-server/02-guards.md`, do not summarize the guard table with a fixed count. The table includes numbered guard rows and sub-rows such as `3b` and `7b`, and it can change during design work. Reference the table directly instead.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

You are interacting with an AI system.

[mforce]

Pushed 650141e — adversarial review round 2, which attacked the round-1 fixes rather than re-reviewing the design. Two major, two minor, all verified and applied.

The headline: two more guards that could not go red — both of them in the fixes for round 1.

Row 7b claimed a guarantee its own walk could not establish. My round-1 fix said the dependency walk closes the secondary-scope route. It does not: a helper registered as sp => new Helper(() => sp.CreateScope()) exposes only a delegate on its constructor, so a constructor walk sees nothing; a static service locator adds no edge; detached background work need not touch the graph at all. Factory registrations already exist in the legitimate graph (CluckworkIdentityServiceCollectionExtensions.cs:25).

The design now specifies a closed registration model and — the part that matters — stops claiming the branch is unreachable. It bounds the hazard, names the residue, and points at #787 as the backstop that makes the residue fail closed rather than silent. This is why slice 6 of #789 is blocked on #787 rather than merely related to it. A fix that reads as complete and is not is worse than the gap it replaced.

Row 31 tested an output bound while claiming a materialization bound. It said "no tool can materialize an unbounded set", but its mutation only removed the reply cap. An implementation can call ListCustomerBalancesAsync(), apply Take(cap), and return correct continuation metadata: every response bounded, the entire farm's balance book materialized, guard green. Split into row 31 (bounded reply) and row 31b (bounded query, asserted on rows consumed from the database). Since PaymentRepository takes no paging arguments, reusing it at all is the defect — a paged balance query is required.

Row 19's mutation was ambiguous — HTTP stores the SHA-256 digest, so keying on the route text leaves the namespaces distinct and the test green. Now stated byte-for-byte.

The loopback alternative over-promised, and the reviewer refuted its own round-1 suggestion — which is what I asked it to do. A tool-prefixed Idempotency-Key is not a server-enforced namespace (HTTP accepts any non-blank key and hashes it, so an ordinary caller can collide), and the replay path carries no discriminator, so a loopback tool cannot honestly report "replayed": true. Neither is fatal; both are now priced in the alternative rather than discovered mid-slice.

Cleared by round 2: revised guard 2, and new guards 29, 30, 32. The idempotency outcome contract is correct as written. For loopback: forwarding the caller's bearer preserves tenant and actor resolution, the second credential-epoch read correctly rejects a credential revoked between hops, only the inner request claims idempotency, no rate-limit double charge, and /error re-entry keeps its existing skips.

The pattern, recorded because it is the transferable part: round 1 found three guards that could not go red; round 2 found two more, in the fixes for round 1. Every one had a mutation adjacent to but not identical with the invariant it claimed. The rule was already written in this repo — run the mutation, do not reason about it — and it was violated by the person writing it into the same document. Twice.

I am not triggering another round on my own initiative. Two consecutive rounds have now yielded documentation defects rather than product defects (there is no product here — nothing has been implemented), which is the repo's stop-and-ask point. Handing that decision to the owner.

[mforce]

Pushed e20cb0a — adversarial review round 3, attacking round 2's fixes. Two major, one minor, all applied. This is the final round; the loop stops here by decision.

The finding worth reading: I contradicted myself across two artifacts on the same day.

Round 2's fix claimed #787 makes the secondary-scope residue "safe rather than silent". That is true for writes and false for reads — a read escape calls FlockRepository.ListAsync, which goes through the EF global query filter reading FlockScope directly, and never touches FlockScopeGuard at all. So #787 changes nothing on that path.

What makes it sharp: this contradicts my own scoping of #787, written hours earlier on that very issue — "FlockScopeGuard is consulted by RecordDailyEntry… The MCP read slices do not reach it." Two documents, one author, opposite claims, and neither of the two reviews that read them separately caught it. The residual read risk is now recorded as open rather than claimed closed.

Three operative statements still said "unreachable". Round 2 added an honesty note saying the branch is not unreachable and left three statements elsewhere — Alternatives considered, Open questions, Deliberately not guarded — still asserting it is. An implementer reading any of those would omit protection the new argument depends on. All three now carry the bounded claim; the superseded wording survives in the review history rather than the operative text.

Row 31b named a mechanism that cannot measure what it requires. My round-2 fix pointed at ReportQueryBoundingTests' SqlCaptureInterceptor and said it "needs no invention". It records (Sql, Parameters) only — so an implementation that fetches every page in a loop and accumulates the whole book emits perfectly paged SQL and passes. Now requires counting rows consumed by wrapping the DbDataReader, with SQL inspection as supporting evidence.

Two counts corrected, and one deliberately removed. I added "9 factory registrations" while fixing round 2; the reviewer counted 10. The discrepancy turns on which lambda parameter names a grep matches — so the number is now absent by design, with instructions to walk ServiceDescriptors from the built collection instead. This repo has been bitten by a bare count twice already; I managed to add a third in the act of fixing something else.

Cleared by round 3: row 7b is implementable; row 19's byte-for-byte mutation is correct (the registered pattern is /api/v1/daily-entries/ and the middleware hashes the actual request path); row 31's reply/query split is sound, and grouping does not force client materialization since PaymentRepository already aggregates in SQL; the loopback qualifications hold.

Three rounds

Round	Target	Guards found unable to go red
1	the synthesized design	3
2	round 1's fixes	2
3	round 2's fixes	1, plus 2 wrong claims added while fixing

Every round found defects in the previous round's fixes, and the yield narrowed without reaching zero — round 3's findings are about claim precision rather than design shape. Stopping is a decision, not an exhaustion.

The transferable lesson now has six instances behind it: a guard's stated mutation must be run, not reasoned about — and a fix written confidently is exactly as likely to need that as the thing it fixed. What remains useful is not more review of this document; it is running these mutations for real when the guards are written.

Ready to merge once checks settle.