Skip to content

fix(mcp): match flock names ignoring case, plain optional schema types, quiet idempotent replays - #1237

Merged
mforce merged 14 commits into
mainfrom
fix/1228-1230-mcp-names-polish
Oct 12, 2026
Merged

mforce merged 14 commits into
mainfrom
fix/1228-1230-mcp-names-polish

Conversation

@mforce

@mforce mforce commented Oct 11, 2026 •

Copy link
Copy Markdown
Owner

MCP clients can now name a flock in any case. Tool schemas no longer offer null for optional parameters. A replayed write no longer logs two database errors.

Closes #1228
Closes #1230

What changed

Flock names ignore case (#1228). FlockRepository.ListByNameAsync is an exact ILIKE through LiteralSearch.ExactPattern, the escaping the flock and customer searches already use, with no wildcards added. PostgreSQL folds the case of both sides, so a name such as İpek still resolves when typed exactly, and %, _ and \ in a name match only themselves. flock:"barn a" now resolves "Barn A". Two flocks that differ only in case get the existing ambiguity refusal with both ids. The flock parameter description no longer says "exact".

Unknown status is not echoed (#1228). cluckwork_list_sales_orders answers an unknown status with the accepted list only, like cluckwork_list_flocks and the REST flock list. The REST sales-order list still echoes it (SaleEndpoints.cs:255). That route is outside this PR.

Plain optional types in tool schemas (#1230 item 1). WithToolsFromAssembly takes no schema options, so AddCluckworkMcp now registers the tools itself with the same reflection walk as the SDK: instance methods get an ActivatorUtilities target, and static methods use the SDK's static McpServerTool.Create overload. It passes AIJsonSchemaCreateOptions.TransformSchemaNode, which removes "null" from every type union and drops a "default": null. grades is now array, its items object, and grade string. Binding still uses each parameter's C# default, so an omitted parameter behaves as before.

A replay logs no errors (#1230 item 2). The claim insert was an EF SaveChanges that raised 23505 on every replay. EF logged it twice at Error, once as Database.Command.CommandError and once as Update.SaveChangesFailed, before IdempotentUnit caught it. EF's CommandError event carries no exception, so a log filter could match it only by SQL text. Instead, the claim is now INSERT ... ON CONFLICT (AccountId, EndpointHash, IdempotencyKeyHash) DO NOTHING. A replay or a lost race returns 0 rows, the unit logs one Debug line, and the existing steal-or-inspect path runs unchanged. Nothing suppresses 23505. REST and MCP replays both use this path.

The insert runs under CreateExecutionStrategy(). SaveChanges was retried by EnableRetryOnFailure, but raw SQL gets no retry of its own. Without the wrapper, a lost acknowledgment on the claim insert became a 500. RetryBoundaryTests.LostAcknowledgmentOnTheClaimInsert_IsRecognisedAsOurOwnClaim_AndTheHandlerStillRuns caught this in CI on the first push. With the wrapper, the replay finds our own row as before.

The insert writes "AccountId" = {scope.AccountId}. Both callers build the scope from the resolved tenant, like the steal, publish and release statements already in this class. BypassAllowList gets the new hash and a justification that names the insert. The FilterFreeSetSites row for the removed db.IdempotencyRecords.Add is now stale, so this PR deletes it.

Where to start

  1. src/Cluckwork.Api/Middleware/IdempotentUnit.cs: the claim insert. This is the only change to shared write behaviour.
  2. src/Cluckwork.Api/Hosting/CluckworkMcp.cs: tool registration and the schema transform.
  3. FlockRepository.cs, LiteralSearch.cs and CommerceTools.cs: a few lines each.

Tests

Each test below fails when its fix is reverted:

Test Mutation that turns it red
FlockNameResolutionTests.Name_MatchesIgnoringCase, NamesDifferingOnlyInCase_AreAmbiguous restore f.Name == name
FlockNameResolutionTests.UnicodeName_TypedExactly_ResolvesUnderAnInvariantCultureCaller lower-case the input in .NET (the first version of this PR)
FlockNameResolutionTests.WildcardCharacters_MatchOnlyThemselves pass the name to ILIKE unescaped
McpCommerceToolsTests.UnknownStatus_IsRefusedWithoutEchoingIt put {status} back in the message
McpEndpointTests.ToolSchemas_AreServedForEveryTool_WithoutNull remove TransformSchemaNode; separately, keep default: null; separately, leave a valid static tool out of registration
IdempotencyReplayTests.Replay_LogsTheClaimConflictOnce_AtDebug restore the EF insert; separately, drop the Debug log

The schema test lists tools through a real client and also checks that every [McpServerTool] method is still served, so the hand-written registration cannot drop a tool. The existing RecordDailyEntryToolTests.OmittedGrades_KeepTheDraftsLines_AndAnEmptyListRemovesThem passes unchanged. Locally, the integration classes matching Idempotency, Mcp, FlockNameResolution and DailyEntr pass (287 tests). The full Application and Domain suites also pass.

Live MCP client run

The full live run was at 25289f9e, through real OAuth consent with the password step-up; the details are in this comment. A case-only duplicate got the ambiguity refusal. Inspector --strict reported no schema findings. Two replays, one over MCP and one over REST, logged no 23505 and no Error line of their own.

After the review fixes, I rebuilt the stack at d9ff13e9 and checked the changed lookup again: flock:"sim house a" still resolves "Sim House A" (reply).

…s, quiet idempotent replays

- ListByNameAsync compares lower(Name), so flock:"barn a" resolves "Barn A" and
  names differing only in case are ambiguous (#1228).
- cluckwork_list_sales_orders no longer echoes an unknown status, like the flock
  list and its REST route.
- Tool schemas advertise optional parameters as their plain type, without a null
  union or a null default; GradeLine.grade is a plain string (#1230).
- The idempotency claim is INSERT ... ON CONFLICT DO NOTHING, so a replay no
  longer raises a unique violation that EF logged twice at Error; the unit logs
  the existing claim once at Debug (#1230).
@mforce

mforce commented Oct 11, 2026

Copy link
Copy Markdown
Owner Author

Live MCP client run

I ran this at head 25289f9e on an isolated stack rebuilt from that head (image created 2026-10-11T18:29Z), behind the TLS proxy at https://localhost:8496. The client is tools/verify/mcp/client.ts, the TypeScript SDK at protocol 2025-11-25.

Connect. I registered a client dynamically, signed in, and allowed consent with the password step-up. Allow with no password stayed on the consent screen. The token came back with scope=farm:read daily-entries:write, and tools/list named all 10 tools. The consent frame is attached.

#1228: names ignore case.

list_daily_entries flock:"sim house a"
  [{"flockName":"Sim House A","date":"2026-10-10"},{"flockName":"Sim House A","date":"2026-10-09"}]
record_daily_entry flock:"barn live 529"   (REST created "Barn Live 529")
  {"flockName":"Barn Live 529","date":"2026-10-10","replayed":false}
REST creates "BARN LIVE 529", then list_daily_entries flock:"barn live 529"
  isError: true
  More than one flock is named 'barn live 529'. Call again with flock set to one of these ids:
  8ed799b1-… (Active), ca97a3de-… (Active).
list_sales_orders status:"Bogus-ECHO-CANARY"
  isError: true
  status must be Draft, Confirmed, Shipped, Invoiced, Cancelled or Voided.

#1230: plain schema types. npx @modelcontextprotocol/inspector --cli … --method tools/list --strict exited 0 and wrote no schema findings to stderr, only Node's url.parse() deprecation notice. The reply has no "null" string anywhere. grades is now {"type":"array","items":{"type":"object","properties":{"grade":{"type":"string",…},"quantity":{"type":"integer"}},"required":["grade","quantity"]}}. I did not re-run Inspector against main on this stack, so the before count is the one #1230 reports.

#1230: replays log no errors. I sent the same record_daily_entry call with the same key twice. The second reply was "replayed":true. Then I sent POST /api/v1/flocks twice with one Idempotency-Key, and both answered 201. From the first replay call to the end of the run, the app logged 55 lines. None contained 23505. Two were at Error, and both are the SDK logging the two deliberate refusals above:

{"@t":"2026-10-11T18:30:26.53Z","@mt":"\"{ToolName}\" threw an unhandled exception.","@l":"Error","@x":"ModelContextProtocol.McpException: More than one flock is named 'barn live 529'. …
{"@t":"2026-10-11T18:30:27.06Z","@mt":"\"{ToolName}\" threw an unhandled exception.","@l":"Error","@x":"ModelContextProtocol.McpException: status must be Draft, Confirmed, …

The new Debug line is below this stack's Production log level. IdempotencyReplayTests.Replay_LogsTheClaimConflictOnce_AtDebug checks it at Verbose.

That SDK logs every McpException refusal at Error is a separate behaviour, which this PR leaves unchanged.

Afterwards I took the stack down with down -v, removed its image, stopped the proxy, and restored .env.sim.

Consent screen at 1280, light: Read farm data and Record daily entries, Acts as you, password step-up

SaveChanges ran under EnableRetryOnFailure, so a lost acknowledgment replayed
the claim and inspection found our own row. Raw SQL gets no retry of its own,
so RetryBoundaryTests saw the transient fault as a 500.
Lower-casing the input in .NET and the stored name in PostgreSQL disagreed
on names such as "İpek": under the invariant culture the exact name stopped
resolving. The lookup is now an exact ILIKE through LiteralSearch, so the
database folds both sides, and '%', '_' and '\' in a name match only
themselves. Review R1 on #1237.
…test

The hand-written registration and its coverage walk both skipped static
methods, which WithToolsFromAssembly registers. A static tool now gets the
SDK's static McpServerTool.Create overload. Review R2 on #1237.
…es-polish

# Conflicts:
#	src/Cluckwork.Api/Hosting/CluckworkMcp.cs
@mforce

mforce commented Oct 11, 2026

Copy link
Copy Markdown
Owner Author

Fixes for the Codex review of 91c19219. The second Codex round, at d9ff13e9, found no defects.

R1 (P2), flock names folded by .NET and PostgreSQL differently. Fixed in 1035eb9d. ListByNameAsync is now an exact EF.Functions.ILike(f.Name, pattern, LiteralSearch.EscapeChar) on the filtered db.Flocks, and PostgreSQL folds the case of both sides. The pattern comes from a new LiteralSearch.ExactPattern, which escapes \, % and _ and adds no wildcards. ContainsPattern now builds on it, so the flock and customer searches escape exactly as before. Two real-Postgres tests cover it:

  • UnicodeName_TypedExactly_ResolvesUnderAnInvariantCultureCaller stores İpek … and resolves it, typed exactly, with CultureInfo.CurrentCulture set to invariant. On the previous code it failed with Assert.IsType() Failure: … Expected: typeof(…FlockNameResolution+Found).
  • WildcardCharacters_MatchOnlyThemselves checks that barn_1 … finds "Barn_1 …" but not "BarnX1 …", and that pen% … finds "Pen% …" but not "Pen 12 …". It fails when the name reaches ILIKE unescaped.

R2 (P3), static tool methods skipped. Fixed in f20f424d. Registration and the coverage walk in ToolSchemas_AreServedForEveryTool_WithoutNull both include BindingFlags.Static. A static method uses McpServerTool.Create(method, target: null, options). I checked this with a temporary valid static tool in FlockTools, since removed:

  • Registered: the tool was served, and the schema and tool-surface tests passed (45).
  • Left out of registration: the coverage test failed.
  • Old flags on both registration and test: the test stayed green, which is the gap the review described.

Merge. d9ff13e9 merges main. CluckworkMcp.cs conflicted with #1238. The builder now chains .AddAuthorizationFilters() and #1238's .WithRequestFilters(filters => filters.AddCallToolFilter(McpToolErrors.Filter)), then calls AddTools(services).

Live check. I rebuilt the isolated stack at d9ff13e9, behind the TLS proxy, and connected through OAuth consent with the password step-up (scope=farm:read). The lower-case name still resolves:

cluckwork_list_daily_entries {"flock":"sim house a","limit":2}
[{"flockName":"Sim House A","date":"2026-10-10"},{"flockName":"Sim House A","date":"2026-10-09"}]

Locally on the merged tree, the MCP, flock-name, idempotency and retry-boundary integration tests pass (265), and so does the full Application project (771).

@mforce
mforce merged commit e8726a7 into main Oct 12, 2026
21 checks passed
@mforce
mforce deleted the fix/1228-1230-mcp-names-polish branch October 12, 2026 00:26
@mforce mforce mentioned this pull request Oct 12, 2026
16 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant