Skip to content

MCP slice 6: write tool — record daily entry #809

Closed

Description

@mforce

Amended 2026-10-02 (#852, PR #1021): the name-resolution contract exists as IFlockLookup.ResolveByNameAsync (found / not found / ambiguous with candidates). See this comment.

Slice 6 of #789. Blocked on #788, #787, and slice 2 (#805). This is the only write in the phase-1 surface.

Why #787 is a genuine prerequisite, not defence-in-depth

RecordDailyEntry consults FlockScopeGuard. Its first line is if (!user.IsResolved) return Result.Success();, which is fail-open.

Slice 2's McpCallContext is the primary control, but it bounds the hazard rather than closing it. A helper opening a secondary DI scope leaves CurrentUserContext unresolved in that scope, which lands exactly on that branch. #787 flips it fail-closed, turning a silent grant into a refusal.

Note the asymmetry: #787 backstops the write path only. A secondary-scope read never calls FlockScopeGuard. It goes through the EF query filter, which reads FlockScope directly, and an unresolved FlockScope is unrestricted. That read residue is recorded as open risk.

Scope

  • Takes a flock name-or-id and grade names-or-ids. Derives FarmId/HouseId from the flock aggregate. The caller's surface drops from four unguessable GUIDs to one flock and some numbers. One of those GUIDs, HouseId, is a phantom id until Phase 2's House model.
  • Calls the unchanged RecordDailyEntryValidator and RecordDailyEntryHandler. No parallel validation path.
  • Required idempotency key, keyed on the tool name so two tools cannot collide.
  • Name resolution means exactly one accessible match, because duplicate flock names are legal.

Guards worth naming

  • Guard 7b (dependency-graph walk plus the causal second-scope test) is the primary control. It has two mutations: direct IServiceScopeFactory injection, and a factory-registered helper hiding it behind a delegate. A constructor walk sees nothing in the second case.
  • Replay must assert the absence of a second DailyEntryUpdate audit row, NOT the entry count. The handler upserts on (account, farm, house, flock, date), so a count assertion passes with idempotency entirely removed.
  • A Worker not assigned to the flock must be refused.

Design

docs/plans/770-mcp-server/02-guards.md rows 7b, 21-23, 25-27, 32, plus the honesty note on what these guards do not close.

Activity

  1. added
    sliceThin vertical work item
    area:apiAPI/endpoint layer
    priority:tier3Real product weight, real cost
    size:LSeveral days; wide blast radius or unresolved scope
    epic-789MCP server support (#789)
    on Sep 13, 2026
  2. added this to the MCP server support milestone on Sep 13, 2026
  3. mforce commented on Sep 14, 2026

    @mforce
    OwnerAuthor

    Before you pick this up: the name-resolution policy this slice needs does not exist yet

    Found on 2026-09-14 while re-planning #514 (modular monolith), verified against main @ 6231b31.
    No defect is claimed in shipped code. This is a premise in the design that will cost this slice
    time if someone discovers it at review instead of at planning.

    docs/plans/770-mcp-server/01-design.md:209 says the tool layer "deliberately does not contain:
    any validation rule, any authorization decision of its own, any tenant or flock-scope predicate.
    Each already exists one layer down and is already guarded."

    For flock name resolution that premise is false. IFlockRepository
    (src/Cluckwork.Application/Features/Flocks/IFlockRepository.cs) exposes ListAsync, SearchAsync,
    GetDisplayNamesAsync and GetByIdForFlockScopedWriteAsync. There is no name lookup. The only
    name-to-flock resolution anywhere in src/ is SimulationDataSeeder.cs:796:

    var existing = await db.Flocks.FirstOrDefaultAsync(f => f.Name == name, ct);

    That is exactly the take-the-first-match behaviour guard 32 forbids and adversarial review finding 3
    identified. So this slice has to invent the ambiguity contract, and the design's own rule says it must
    not live in Mcp/.

    The two halves are not symmetric

    cluckwork_record_daily_entry resolves a flock name-or-id and grade names-or-ids
    (01-design.md:194-195). Only one half is dangerous:

    Names unique? Resolver exists?
    Egg grades Yes: IX_EggGrades_AccountId_FarmId_LowerName is a unique index on lower(Name) per account+farm (docs/schema/public.EggGrades.md:51) No method, but IEggGradeRepository.NameExistsAsync shows the code already relies on the uniqueness. Filtering ListActiveAsync is safe, because ambiguity cannot arise.
    Flocks No: duplicates are legal. FlockRepository says so outright and its ThenBy(f => f.Id) tie-break exists because of it None at all. This is the half that needs the contract.

    Suggested placement

    Put resolution behind a Flocks-owned read contract, a ResolveAsync returning found /
    not-found / ambiguous-with-candidates, rather than inside the tool. Three reasons beyond the
    design's own rule:

    • the ambiguity policy is Flock Management's to state, not the MCP adapter's;
    • the SPA entity picker will want the same policy later;
    • SimulationDataSeeder.cs:796 is an existing caller that should stop taking the first match.

    #514's re-plan carries this as a candidate first contract, but this slice is not blocked. It can
    define the contract itself. The note exists so the placement is a decision rather than a discovery.

  4. mforce commented on Oct 2, 2026

    @mforce
    OwnerAuthor

    Amendment from #852 (PR #1021). The name-resolution contract this tool needs now exists, so Mcp/ does not have to author that policy.

    • IFlockLookup.ResolveByNameAsync(name, ct) returns FlockNameResolution.Found(FlockReference), NotFound, or Ambiguous(candidates ordered by id).
    • It matches the name exactly, across every status, through the filtered Flocks set. The tenant and flock-scope filters (Add discovery guard for flock-scoped EF query filters #613) decide the candidates, so a Worker sees only assigned flocks.
    • "Exactly one accessible match" is Found. Eligibility is a separate check, FlockDetails.CanRecordProductionOn(date) from IFlockLookup.GetForFlockScopedWriteAsync.
    • FlockNameResolutionTests covers tenant isolation, Worker scope and the candidate order.

    Note the read-path residue this issue already records. An unresolved FlockScope is unrestricted, so a secondary DI scope would see every flock in the farm. Resolve by name in the request scope.

  5. mforce commented on Oct 6, 2026

    @mforce
    OwnerAuthor

    Updated in the 2026-10-06 re-evaluation. Still L.

  6. mforce commented on Oct 10, 2026

    @mforce
    OwnerAuthor

    Amendment: OAuth is available, but the loopback option needs an authentication design. This adds to the October 6 amendment. Its contract, name-resolution and idempotency requirements still apply. Source links pin main at bd4e5eca.

    PR #1136 shipped OAuth request checks and scope gating. PR #1144 shipped consent for daily-entries:write. PR #1138 shipped connected-app audit attribution, and PR #1147 added the farm switch. See src/Cluckwork.Api/Modules/Access/OAuth/OAuthEndpoints.cs:65, src/Cluckwork.Application/Modules/Access/Contracts/OAuthScopes.cs:5 and src/Cluckwork.Infrastructure/Repositories/AuditWriter.cs:53.

    Forwarding an OAuth bearer to the existing REST routes fails. Today's scheme selector sends ordinary REST routes to session-JWT authentication: src/Cluckwork.Api/Hosting/CluckworkIdentityServiceCollectionExtensions.cs:185. The record-daily-entry route has no OAuth opt-in: src/Cluckwork.Api/Modules/EggOperations/DailyEntries/DailyEntryEndpoints.cs:16. It therefore returns 401 for that bearer. Adding AcceptOAuthTokens alone would make the route OAuth-only and reject the SPA's session JWTs.

    Price loopback with that constraint included. Choose and record either an HTTP boundary with explicit authentication and scope rules, or a shared idempotency unit around the module-contract call. Preserve the existing #307 suites.

    The write tool, its authentication/idempotency design and its acceptance tests remain unbuilt. Prove role and scope restrictions, Worker flock access, farm-switch refusal, app audit attribution and replay without a second DailyEntryUpdate audit row. The tool records a draft; a person still submits it. Milestone 10 deliberately provides neither this tool nor OAuth access to the ordinary REST route.

  7. mforce commented on Oct 10, 2026

    @mforce
    OwnerAuthor

    Re-check after milestone 10, 2026-10-10.

    • Recommendation: drop loopback. The 2026-10-10 amendment already notes the REST route rejects OAuth bearers. Making it accept them means a second authentication design for one route. Forwarding the caller's token to it also looks like the token passthrough the MCP authorization spec forbids. A shared idempotency unit around IEggOperationsModule.RecordDailyEntryAsync needs neither. Record the choice in this slice.
    • The tool requires daily-entries:write as well as ProductionWrite.
    • A write-only connection is legal. Consent grants exactly the scopes requested, and daily-entries:write does not imply farm:read. Name resolution must therefore not depend on a read tool. IFlockLookup.ResolveByNameAsync inside this tool covers it.
    • Audit rows already carry the connected app (AuditWriter.cs:58). Assert it on the recorded entry's audit row.
  8. mforce commented on Oct 10, 2026

    @mforce
    OwnerAuthor

    Decision recorded on #789, 2026-10-10: loopback is dropped. The write tool calls IEggOperationsModule.RecordDailyEntryAsync through a shared idempotency unit. The #307 suites still pass unedited, and replay must not write a second DailyEntryUpdate audit row.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:apiAPI/endpoint layerepic-789MCP server support (#789)priority:tier3Real product weight, real costsize:LSeveral days; wide blast radius or unresolved scopesliceThin vertical work item

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions