feat(crud): slice 3 REST list delegates to service via listService

[JayDS22]

Summary

Extends the buildCrudRouter helper with an optional listService param. When provided, the GET / handler delegates the DB read + count to that service function instead of running its own inline query. The router still owns query-string parsing (parseQueryParams), whereClause construction (defaultWhereClause with SAFE_IDENTIFIER_RX / operator whitelist), and orderClause construction; the service owns the DB select + count.

This gives REST and MCP a single code path for reads, matching the pattern already in place for the single-resource operations (getTemplateById, getAgreementById). Closes the Proposal Core service-layer port on the code side.

What this PR does

• server/services/templateService.ts: new listTemplatesPaged(db, opts) returning { items, total }. Accepts whereClause + orderClause from the caller so REST filter/sort semantics stay uniform. limit clamped 1..100, offset clamped >= 0.
• server/services/agreementService.ts: new listAgreementsPaged with the same shape.
• server/handlers/crud.ts: adds ListService<TRow> type + listService? option on CrudRouterOptions. GET / branches on whether listService is provided; the pre-slice-3 inline path is preserved as the fallback so resources without a service layer (sharedmodels) keep working unchanged.
• server/handlers/templates.ts, agreements.ts: pass listService so their REST list routes now call the service functions.

Stack

Top of a three-PR chain. Depends on #224 (subscriptions/listen) which depends on #223 (JSON-RPC error range).

Sequence:

1. feat(errors): standardize MCP JSON-RPC codes into -32020..-32099 range #223 - JSON-RPC error range (merge first)
2. feat(mcp): port subscriptions/listen SEP-2575 preview upstream from POC #224 - subscriptions/listen SEP-2575 preview
3. This PR - slice 3 REST list unification (merge last, closes Proposal Core)

Unlocks

• MCP resource paths and REST GET / share one implementation for list. Behaviour changes to one propagate to both.
• The { items, total } return shape is the piece Issue [follow-up] Expose paged reads via MCP resource URIs for apap://templates and apap://agreements #217 needs for paged apap://templates and apap://agreements MCP resource URIs.

Not in this slice (follow-ups)

• sharedmodels service layer + listSharedModelsPaged (its service file does not exist yet; sharedmodels stays on the default DB path).
• MCP resource handlers switching from listTemplates (array) to listTemplatesPaged ({items,total}) to expose total in ReadResourceResult. Tracked under [follow-up] Expose paged reads via MCP resource URIs for apap://templates and apap://agreements #217.

Validation

• npm run build: clean
• npm test: 11 suites / 172 tests, all pass. templates.test.ts and agreements.test.ts continue to exercise the list route through the new delegation path.

Related

• Refs Issue [follow-up] Expose paged reads via MCP resource URIs for apap://templates and apap://agreements #217 (MCP resource-URI paging) - this PR delivers the {items,total} primitive; the resource-handler switch is a small follow-up.
• Closes the service-layer port workstream (last inline DB read in buildCrudRouter now delegates).

Author Checklist

• DCO sign-off on every commit
• Rebased on top of feat(mcp): port subscriptions/listen SEP-2575 preview upstream from POC #224 (which is on top of feat(errors): standardize MCP JSON-RPC codes into -32020..-32099 range #223)
• npm test green locally
• No em-dashes in commit message or PR body

[niallroche]

The split is right: router keeps query parsing, whereClause (with SAFE_IDENTIFIER_RX + operator whitelist), and orderClause; the service owns select + count and clamps limit to 1..100 and offset to >= 0. Clamping in the service is the correct home even if the router also does it — defense in depth against a future caller. Nice.

A few things:

1. Pagination needs a deterministic total order — confirm orderClause is never effectively null. In listTemplatesPaged the ordering is applied conditionally (opts.orderClause ? baseQuery.orderBy(...) : baseQuery). If a request can reach the service with no orderClause, the DB returns rows in unspecified order, and limit/offset paging over an unordered set can repeat or skip rows across pages. A list endpoint with offset pagination should always fall back to a stable key (primary key or createdAt). Can you confirm the router always supplies a default order, and if not, default it inside the service so paging is stable by construction?

2. count and select are separate queries — note the read skew. total and items are fetched in two round-trips with no shared snapshot, so under concurrent writes a client can see total: 11 with 10 items (or vice versa). Usually acceptable for list endpoints; just worth a comment, and if you want them consistent, a repeatable read transaction around the pair does it.

3. The REST/MCP divergence cuts against the PR's stated goal. REST now returns { items, total } while the MCP resource path still returns a bare array via listTemplates. Deferring the MCP switch to #217 is reasonable, but since the headline is "unify REST + MCP," this actually widens the gap until the follow-up lands. Please make sure #217 is tracked and the temporary shape difference is documented so a client dev doesn't assume total exists on both.

4. Test the boundaries. I don't see coverage for the clamping/edge cases the service now owns: limit=0 and limit=1000 (→ 100), negative offset, and offset past total (expect empty items, correct total). These are exactly the paths that regress silently.

Nit: the retained inline fallback for sharedmodels means that resource keeps the old path without the service's clamping/consistency. Fine as transitional, but worth a // TODO(#217?) so it doesn't quietly become permanent.

[JayDS22]

Amended ed91aad addressing all four review notes + the nit:

1. Default orderClause fallback for pagination determinism. Both listTemplatesPaged and listAgreementsPaged now default to asc(Template.id) / asc(Agreement.id) when no orderClause is provided, so limit/offset paging is stable by construction. Caller-supplied orderClause is honoured as before. Documented in the service function bodies (line-comment right above the fallback).

2. Read-skew note added in comment. Called out in both services that count and the row-fetch are separate queries without a shared snapshot, so total and items.length can disagree by a row or two under concurrent writes. Kept the current two-query pattern (acceptable for list endpoints), with the repeatable read transaction escape hatch documented for anyone who ever needs strict consistency.

3. REST/MCP shape divergence documented. The apap://templates / apap://agreements MCP resource handlers still call the pre-slice-3 listTemplates / listAgreements (bare array). Slice 3 delivers the { items, total } primitive that #217 needs; the resource-handler switch to listTemplatesPaged / listAgreementsPaged is a small follow-up on that issue. Called out explicitly in the amendment so a client dev seeing the mixed shape has the reason.

4. Boundary tests added. Seven new tests in templateService.test.ts covering limit=0 (clamped to 1), limit=1000 (clamped to 100), negative offset (clamped to 0), in-range pass-through, default orderClause is applied (pagination determinism), caller-provided orderClause is honoured, and total surfaces from the count query. Same clamp logic in listAgreementsPaged is symmetric; happy to add mirror tests there if you'd prefer both sides pinned.

Nit: sharedmodels inline fallback. Added a TODO(#217) block-comment above the fallback branch in crud.ts noting that the fallback path does NOT get the clamping / default-order / read-skew safeguards, and that once a sharedModelService.ts + listSharedModelsPaged land, the fallback branch can be deleted and listService made required on CrudRouterOptions.

npm test: 11 suites / 185 tests, all pass.

[dselman]

Resolve conflicts please

[JayDS22]

Force-pushed d08c47e (was ed91aad). Rewritten against post-#227 main:

• Reset to origin/main; the old branch stacked on feat(errors): standardize MCP JSON-RPC codes into -32020..-32099 range #223 + feat(mcp): port subscriptions/listen SEP-2575 preview upstream from POC #224 work that is either now closed or handled separately (feat(mcp): port subscriptions/listen SEP-2575 preview upstream from POC #224 rewritten in parallel).
• Only the slice-3 delta: listTemplatesPaged / listAgreementsPaged in the service files, ListService type + listService? option in buildCrudRouter, wire-through in templates.ts / agreements.ts.
• Kept main's as any casts on the drizzle-zod schemas (post-feat(mcp): migrate to @modelcontextprotocol/{core,server,express,node,client}@2.0.0 (closes #221) #227 pattern) — no scope creep into that.
• sharedmodels inline fallback preserved with a TODO(#217) marker so it can be removed once sharedModelService.ts lands.
• Confirmed envelope contract: service returns { items, total }; router wraps into main's existing { items, total, page, limit, totalPages }. No wire-observable shape change.

Validation: npm run build clean, npm test 10 suites / 159 tests all pass (unchanged from main baseline).

Not in this PR (deliberate, follow-up):

• Your test(crud): add GET list envelope + GET :id + POST happy-path coverage (takeover of #165) #226 test-coverage gaps (limit-clamp end-to-end, page=0/-1, 23505 -> 400, POST body assertion) — folding into a small test-only PR after this merges to keep the diff focused.

Ready for review.