From 6ca9e1706d9a8b430c76705fe45ff13708c11b8b Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Fri, 23 Oct 2026 16:10:52 +0200 Subject: [PATCH] docs(19.7-06): complete lane A gap-closure plan in pier --- .../19.7-06-SUMMARY.md | 189 ++++++++++++++++++ 1 file changed, 189 insertions(+) create mode 100644 .planning/phases/19.7-generic-mcp-module/19.7-06-SUMMARY.md diff --git a/.planning/phases/19.7-generic-mcp-module/19.7-06-SUMMARY.md b/.planning/phases/19.7-generic-mcp-module/19.7-06-SUMMARY.md new file mode 100644 index 00000000..1ca7acad --- /dev/null +++ b/.planning/phases/19.7-generic-mcp-module/19.7-06-SUMMARY.md @@ -0,0 +1,189 @@ +--- +phase: 19.7-generic-mcp-module +plan: 06 +subsystem: api +status: complete +gap_closure: true +tags: [pier, mcp, sessions, redis, lua, concurrency, dispatch, pact, cooler, gap-closure] + +requires: + - phase: 19.7-02 + provides: modules/pier (catalog, dispatch, identity, sessions, fan-out, stateless path) + - phase: 19.7-05 + provides: the updater pattern (1111828), helpers_test.go fakes, scripts/check-phase19.7.sh +provides: + - session reaper (reap, reapOnce) started at Mount, `expired` and `ephemeral` audit labels, GET stream ends when the record is gone + - unexported reserver (Reserve, Release) on the memory and Redis stores (Lua script), used by serveNew + - lock-free server build in serveNew, lookup rehydration and stateless getServer + - Identity.fingerprint and re-filtering of a cached server when a binding returns with a different identity + - Redis store warm-up at Mount plus a non-blocking kick on every request + - stateless subscriptions/listen bearer re-check and session close on LRU eviction + - refusal of a personal-token subject without TokenID; corrected pact.MCPPersonalTokenResolver contract + - path-value and path-field rules, fixed guard-failure text, UseNumber, Host and forwarding headers, embedded-field refusal, guard-list check + - pact.MCPPrompt.Writes under the read/write ceiling + - cooler KeyPrefix / keyPrefix doc comments separated +affects: [19.7-07 (token-specific events build on session.go/stateless.go), 19.7-08 (Journal sets MCPPrompt.Writes), 19.7-09 (named tests pin every fix), framework v0.5.0] + +actuals: + tokens: 20336 + tasks: 4 + commits: 6 +plan_head_before: d18d8f0dc206392726e225c2f5cc1340d14463c1 +plan_head_after: 95f177b1e0dbc27483d4ece9dc88751cb226f2fd + +tech-stack: + added: [] + patterns: + - "Unexported capability interface next to updater (reserver), checked with a type assertion, so the exported pier.Store does not change" + - "Build outside the router lock, then insert-or-adopt under it" + - "Identity fingerprint decides when a cached per-identity server is re-filtered" + +key-files: + created: + - modules/pier/reaper_smoke_test.go + modified: + - modules/pier/session.go + - modules/pier/pier.go + - modules/pier/audit.go + - modules/pier/store.go + - modules/pier/store_memory.go + - modules/pier/store_redis.go + - modules/pier/stateless.go + - modules/pier/identity.go + - modules/pier/events.go + - modules/pier/dispatch.go + - modules/pier/catalog.go + - modules/pier/filter.go + - modules/pier/backend_guard.go + - modules/pier/export_test.go + - modules/pier/README.md + - modules/pact/mcp.go + - modules/pact/README.md + - modules/cooler/cooler.go + - docs/services/mcp.md + +key-decisions: + - "P-24: a reaper goroutine started at Mount closes every local session whose store record is gone every min(idle_ttl/2, 1m) (floor 1 s); the store TTL stays the single source of truth across instances; stopped through Pier.stopReap (no backpack shutdown hook yet)" + - "P-25: atomic per-binding reservation through an unexported reserver interface; Redis uses one Lua script (prune, count, SET PX, SADD, PEXPIRE) on the existing key layout" + - "P-26: Identity.fingerprint (sorted scopes, both subjects with guard/user/client/token/scopes, ceiling, token kind); a changed fingerprint re-runs the event add/remove diff under the session's or entry's own mutex, with context.WithoutCancel so a client hang-up cannot leave the set filtered by cancelled guard checks" + - "P-27: path fields must be required at boot; at runtime a missing value, an empty, . or .. segment, or / in a single segment is a tool error before dispatch" + - "P-28: pact.MCPPrompt.Writes (additive) puts a prompt under the read/write ceiling" + - "Guard dispatch errors answer the fixed text `pier: the tool could not be run for this caller`; only pact.ErrMCPTokenInvalid and a new errSubjectUnavailable sentinel (backend user unknown or deactivated) keep their own fixed texts" + +patterns-established: + - "Background lifecycle goroutines of pier (reaper, Redis warm-up) share the context whose cancel func is Pier.stopReap" + +requirements-completed: [SC-1, SC-2, SC-3, D-10, D-11, D-13, D-17, D-19, D-20, D-25, D-26, D-27, D-28, D-32] + +duration: 23min +completed: 2026-10-23 +--- + +# Phase 19.7 Plan 06: Lane A gap closure in pier Summary + +This plan closes lane A of the code review in the framework's MCP module. Abandoned stateful sessions are now reaped at the store TTL. The per-binding cap is reserved atomically (memory store under its lock, Redis through a Lua script). No router-wide lock is held across guard or database I/O. A binding that returns with a wider identity is re-filtered on that request. Redis instances subscribe to fan-out when they mount. Stateless listen streams are re-verified, and an evicted stateless server closes its streams. Personal-token subjects must carry a TokenID. Dispatch only hands routes path values the HTTP mux could produce, and guard failures answer with fixed text. Prompts gain a `Writes` ceiling. + +## Performance + +- **Duration:** about 23 min +- **Completed:** 2026-10-23 +- **Tasks:** 4 of 4 +- **Files modified:** 20 (1 created) + +## Commits and findings + +| Commit | Task | Findings | +|--------|------|----------| +| 2bf9dca `fix(pier): reap stateful sessions whose store record expired` | 1 (tracer) | A-CR-01, A-IN-01, A-IN-06 | +| c1c0c77 `fix(pier): reserve session slots atomically and build servers outside router locks` | 2 | A-WR-03, A-WR-07 | +| 2404572 `fix(pier): widen returning identities, warm the Redis store, end stateless listen streams, refuse personal tokens without a TokenID` | 3 | A-WR-04, A-WR-05, A-WR-06, A-WR-09, A-IN-03 | +| 104db99 `fix(pier): refuse path values the mux never delivers and harden dispatch` | 4 | A-WR-01, A-WR-02, A-WR-08, A-IN-04, A-IN-05, A-IN-07, A-IN-08 | +| 211cec1 `fix(pact): add MCPPrompt.Writes so a write-directing prompt sits under the read/write ceiling` | 4 | C-IN-01 (framework part) | +| 95f177b `docs(cooler): give KeyPrefix and keyPrefix their own doc comments` | 4 | A-IN-02 | + +All lane A findings are covered: A-CR-01, A-WR-01..09 and A-IN-01..08. + +## Accomplishments + +- **Reaper (Task 1).** + - `sessionRouter.reap` and `reapOnce` run from `Mount` and close each local copy whose record has expired, logging `expired`. + - A store error keeps the copy and is logged once per pass. + - `serveStream`'s re-check ends the stream and closes the local copy when the record is gone. + - Ephemeral tool calls log the session as `ephemeral`. + - `TestPierReaperSmoke` proves the flow with a real SDK client. New seams: `ReapNow`, `LocalSessions`, `StopReaper`. +- **Reservation and locks (Task 2).** + - `reserver` with memory and Redis `Reserve`/`Release`, plus `lazyStore` forwarders. + - `serveNew` reserves first, then connects, and releases the slot on a connect or initialize failure. + - `serveNew`, `lookup` rehydration and the stateless `getServer` build the server with no `sr.mu`/`sp.mu` held, then insert or adopt. +- **Identity and lifecycle (Task 3).** + - `fingerprint()` is stored on `localSession` and `statelessEntry`; the `widen` helper re-filters on a change. + - `lazyStore.warm` (backoff 1 s to 30 s) starts at Mount, and `kick()` runs from `route`. + - The stateless `ServeHTTP` re-checks the bearer every `recheck` for `subscriptions/listen` POSTs. + - LRU eviction closes the evicted server's sessions. + - `resolvePersonalToken` refuses `TokenID == 0`, and the resolver contract text is rewritten. +- **Dispatch hardening and prompt ceiling (Task 4).** Covers every item listed in the commit table, and `allowsPrompt` honours `decl.Writes`. + +## Verification + +- `go vet ./...` and `go vet -tags summercms_mcp ./...` are clean. +- `go test -count=1 -short ./...` is green, and so is `-race` on `./modules/pier/...`. +- `go test -count=1 ./modules/pier/... ./modules/surf/...` passes with Docker available. +- `cd examples/hello && go test ./...` is green. +- `scripts/check-phase19.7.sh --go`, `--docs` and `--marker` all PASS. +- `summer docs:build --check` reports no problems. +- `go.mod`, `go.sum` and `admin/package-lock.json` are unchanged since 2527498. +- Commit 104db99 was checked on its own in a temporary detached worktree (vet, short tests, docs check), because Task 4 was split into three commits. +- Ad-hoc checks were run and then deleted, since plan 09 owns the named tests: + - 30 concurrent initializes with `max_per_binding: 2` gave exactly 2 OK and 28 429 on both the memory store and miniredis. + - A read grant followed by a read+write grant on the same binding gained the write tool on both the stateful and the stateless path. + - `9007199254740993` survived as a path value and as a query value. + +## Deviations from Plan + +### Auto-fixed Issues + +**1. [Rule 2 - Missing critical] Sentinel for the backend guard's "unknown or deactivated" answer** +- **Found during:** Task 4 (A-WR-08) +- **Issue:** The plan says to keep specific messages for "the provider sentinel errors dispatch.go already distinguishes", but dispatch.go distinguished none. The backend guard's "unknown or deactivated" answer was a plain `fmt.Errorf`. +- **Fix:** Added `errSubjectUnavailable` in `modules/pier/backend_guard.go`, which the backend guard now wraps. Dispatch maps it, and `pact.ErrMCPTokenInvalid`, to fixed texts. Every other error gets the generic fixed text and is logged server-side. +- **Files modified:** modules/pier/backend_guard.go (not in the plan's file list) +- **Commit:** 104db99 + +**2. [Rule 2 - Missing critical] Widening uses a non-cancellable context** +- **Found during:** Task 3 (A-WR-04) +- **Issue:** `catalog.allowed` hides an item on any guard error. If the request context were cancelled mid-recompute, the server would be re-filtered with tools missing and the new fingerprint stored, so the narrower set would stick. +- **Fix:** `widen` calls `allowed` with `context.WithoutCancel(ctx)`. +- **Files modified:** modules/pier/events.go +- **Commit:** 2404572 + +**3. [Rule 3 - Blocking] The ephemeral label survives `sessionPrefix` and the SDK session id** +- **Found during:** Task 1 (A-IN-06) +- **Issue:** An ephemeral call's SDK session id is `""`, the same as a stateless call. A plain label would also have been cut to 8 characters by `sessionPrefix`. +- **Fix:** + - `serveEphemeral` marks the per-request token info's `callInfo.session` as `ephemeral`. + - `toolHandler` overrides `info.session` only with a non-empty SDK session id. + - `sessionPrefix` passes the `ephemeral` label through untruncated. +- **Files modified:** modules/pier/session.go, modules/pier/audit.go +- **Commit:** 2bf9dca + +**4. [Docs rule] Doc updates outside the listed tasks' file sets** +- In Task 3, `docs/services/mcp.md` gained the resolver/TokenID sentence plus one sentence each on the reaper and on widening. The CLAUDE.md rule requires docs to change in the same commit as an exported contract change. +- Task 4 was split into three commits (pier hardening, pact `Writes`, cooler docs) so each is one logical change and carries the commit-type prefix the success criteria name. + +### Tests changed for old behaviour + +None. No existing test relied on the old behaviour: no fixture uses an optional path field, a shared `pat:user:0` binding, an embedded GET/DELETE field or raw dispatch error text. Every regression pin passed unchanged. + +## Notes for later plans + +- Behaviour change for consumers: Mount now starts two background goroutines (the reaper, and for the Redis store a warm-up that dials `redis.url` with a backoff). `route:list` and tests on the Redis store therefore dial in the background. The pier README and Mount's doc comment say so. +- Consumer impact of the new boot errors: no sibling repository (fonoteka.go, sm-*-app, sm-journal-plugin, the user plugin copies) declares a path field with `omitempty`/`omitzero` or an embedded struct on a GET/DELETE input. Plan 09's consumers stage confirms this by building them. + +## Known Stubs + +None. + +## Self-Check: PASSED + +- FOUND: modules/pier/reaper_smoke_test.go +- FOUND: 2bf9dca, c1c0c77, 2404572, 104db99, 211cec1, 95f177b