feat(services): port triggerAgreement, unify REST + MCP trigger path (slice 2c) - #216
Merged
niallroche merged 10 commits intoJul 21, 2026
Conversation
niallroche
self-requested a review
July 21, 2026 11:45
niallroche
reviewed
Jul 21, 2026
niallroche
left a comment
niallroche
left a comment
Contributor
There was a problem hiding this comment.
The REST + MCP unification is clean and preserving the legacy { isError: true } shape at HTTP 200 is the right backward-compat call. One thing worth tightening:
In triggerAgreement, the persist step spreads the whole row:
await db.update(Agreement).set({ ...agreement, state: triggerResult.state })...
That writes every column back — including the primary key — on what is really a single-field state transition. It's unnecessary write surface and opens a concurrent-update clobber (any column that changed between the read and this write gets overwritten with the stale value). A targeted set({ state: triggerResult.state }) is safer and says exactly what it does. Minor — not a blocker for the slice.
…slice 1)
Slice 1 of the shared-service-layer port from apap-mcp-poc. Adds the
first service module upstream and rewires all four MCP template call
sites in mcp.ts to call it directly instead of going through the
internal `makeApiRequest(${API_BASE_URL}/templates...)` HTTP loop.
Files:
- server/db/client.ts (new, 8 lines) - Database type export (a
PostgresJsDatabase<typeof schema> alias) so services can type the db
handle they take as their first argument.
- server/services/templateService.ts (new, 69 lines) - six functions
(listTemplates, getTemplateById, getTemplateByUri, createTemplate,
updateTemplate, deleteTemplate) taking `db` as the first arg. Throws
TemplateNotFoundError / TemplateDuplicateError from the existing
ServiceError hierarchy introduced in accordproject#184. No transport-specific
imports.
- server/services/templateService.test.ts (new, 211 lines, jest) - 13
unit tests over a fluent Drizzle query-builder mock. Covers the
success and not-found paths for all six functions plus the 23505
unique-violation and generic re-throw branches on createTemplate.
- server/handlers/mcp.ts - getServer signature changes from `()` to
`(db: Database)`; both callers pass `res.locals.db`. Four call sites
now use templateService instead of makeApiRequest:
* getTemplates (module fn) uses listTemplates
* apap://templates/{templateId} list callback uses listTemplates
* apap://templates/{templateId} read callback uses getTemplateById
* getTemplate tool uses getTemplateById
Same strict-numeric guard as accordproject#208 (regex /^\d+$/) is applied to the
templateId path segment before calling getTemplateById, so a malformed
resource URI resolves to TemplateNotFoundError instead of NaN. Existing
serviceErrorToResourceError / serviceErrorToCallToolResult mappers
convert the typed errors to protocol-appropriate responses.
Not in slice 1:
- agreementService port (slice 2, larger surface with trigger/convert)
- REST /templates route rewire to use the service (slice 3)
- subscriptionRegistry port (separate slice)
Validation:
- npm test in server/: 9/9 suites pass (121 tests, 13 new)
- npx tsc --noEmit clean
- No changes to the REST /templates route, so accordproject#187 SQL injection guard
and accordproject#208 :id middleware are untouched
Signed-off-by: Jay Guwalani <guwalanijj@gmail.com>
… review) Niall flagged that the un-paged `listTemplates(db)` was a regression on the `apap://templates` MCP resource path: `parseQueryParams` used to clamp list reads to ≤100 rows, and the service function returns the whole table on every call. That's a token-budget + scalability regression, and it means the per-resource `ttlMs` / `cacheScope` hints from accordproject#201 now annotate an unbounded payload. Add `{ limit?: number; offset?: number }` opts to `listTemplates` directly on the primitive, matching the ≤100 cap the REST layer already applied. Callers with no opts (the current MCP resource path) get the same effective bound as before. Slice 3 REST unification will pass `limit` / `offset` through from `parseQueryParams` and inherit the bounded primitive rather than having to re-add paging in memory. Also updates the fluent Drizzle mock in templateService.test.ts to handle both `.limit(N)` and `.limit(N).offset(M)` chains, and adds four tests pinning the clamp behavior (limit>100 → 100, limit<1 → 1, offset<0 → 0, no-opts → 100/0). Validation: - npm test: 9/9 suites pass, 126 tests total (4 new) - npx tsc --noEmit clean Signed-off-by: Jay Guwalani <guwalanijj@gmail.com>
…eement lookups (slice 2) Slice 2 of the shared-service-layer port. Stacked on top of accordproject#211 (slice 1) — this branch is `jay/feat/service-layer-slice-2-agreements` off `jay/feat/service-layer-slice-1-templates`. The diff will collapse to just these three files once accordproject#211 merges. Scope choice: only the CRUD lookup half is ported (`listAgreements`, `getAgreementById`). The POC's `convertAgreement` and `triggerAgreement` are stubs that do not run the real @accordproject/template-engine, so porting them as-is would regress the RI's existing `TemplateArchiveProcessor`-based convert + trigger implementation in server/handlers/agreements.ts. That deserves its own slice (2b) that wraps the real engine rather than replacing it with a POC stub. Files: - server/services/agreementService.ts (new, 25 lines) - two functions (listAgreements, getAgreementById) taking `db` as the first arg. Throws AgreementNotFoundError from the existing ServiceError hierarchy. - server/services/agreementService.test.ts (new, 85 lines, jest) - 4 unit tests over the same fluent Drizzle mock pattern established in templateService.test.ts. Covers the success and not-found paths for both functions. - server/handlers/mcp.ts - four agreement call sites now use the service instead of makeApiRequest: * getAgreement module fn uses getAgreementById * getAgreements module fn uses listAgreements * apap://agreements/{agreementId} list callback uses listAgreements * getAgreement tool uses getAgreementById Same strict-numeric guard as accordproject#208 (/^\d+$/) applied to the agreementId path segment. Also preserves the accordproject#128 fix (agreement's own `uri` field must not overwrite the MCP resource URI). Not in this slice: - convert-agreement-to-format tool (still uses makeApiRequest until slice 2b wraps the real template engine as a service) - trigger-agreement tool (same reason) - REST /agreements route rewire (slice 3 territory, once slice 1 lands) Validation: - npm test in server/: 10/10 suites pass (125 tests, 4 new) - npx tsc --noEmit clean - No changes to REST route, template engine wrap, or convert/trigger logic; those paths remain unchanged Signed-off-by: Jay Guwalani <guwalanijj@gmail.com>
review) Same pattern as the listTemplates fix in accordproject#211. `listAgreements(db)` was returning the entire Agreement table on every call; the `apap://agreements` MCP resource previously came bounded by ≤100 through `parseQueryParams`. Add `{ limit?, offset? }` opts on the primitive with the same clamping (limit 1..100, offset ≥0, default 100/0) so slice 3 REST unification inherits the bounded surface instead of re-adding paging in memory. Also updates the mock in agreementService.test.ts to mirror the templateService one (fluent `.limit().offset()` + top-level `then`), and adds four clamp tests. The two existing `listAgreements` tests drop their bespoke `mockReturnValue({ from: ... })` wiring in favor of the shared `_setReturn` helper. Validation: - npm test: 10/10 suites pass, 134 tests total (4 new) - npx tsc --noEmit clean Signed-off-by: Jay Guwalani <guwalanijj@gmail.com>
JayDS22
force-pushed
the
jay/feat/service-layer-slice-2c-trigger
branch
from
July 21, 2026 14:41
f1c26ec to
50fca59
Compare
…Uri (@niallroche follow-up) Slice 1's templateService exposes both `getTemplateById` and `getTemplateByUri`. Slice 2's agreementService only exposed `getAgreementById`. That asymmetric surface would confuse anyone reading the two files side-by-side, and the URI-lookup shape is what slice 3 will want when the REST resource-URI form of the agreement route lands. Adds `getAgreementByUri(db, uri)` that mirrors the template version: `db.select().from(Agreement).where(eq(Agreement.uri, uri)).limit(1)`, throws `AgreementNotFoundError` on empty. Two tests (found + not-found). Validation: - npm test: 10/10 suites pass, 136 tests total (2 new) - npx tsc --noEmit clean Signed-off-by: Jay Guwalani <guwalanijj@gmail.com>
…T + MCP (slice 2b) Slice 2b of the shared-service-layer port. Stacked on top of accordproject#213 (slice 2). Adds `convertAgreement` to `agreementService.ts` so both the REST `/agreements/:id/convert/:format` route and the MCP `convert-agreement-to-format` tool call the same function. Eliminates one more internal HTTP loop from `handlers/mcp.ts`. Scope reduction from the originally planned slice 2b: `triggerAgreement` extraction is deferred to slice 2c. When the trigger logic runs behind a service-layer import chain, jest cannot propagate `--experimental-vm-modules` through the dynamic imports `@accordproject/template-engine` performs, and the existing trigger tests fail with `ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`. The old inline trigger path (unchanged in this slice) works because template-engine loads from the same module context as the test file. Solving that needs its own investigation and does not belong in this slice. Files: - server/services/agreementService.ts * Adds `resolveAgreementRuntime` (private helper): loads agreement, resolves associated template by hash or URI, reconstructs the Cicero template archive. Mirrors the inline resolveAgreement in handlers/agreements.ts. * Adds `convertAgreement(db, agreementId, format)`: uses resolveAgreementRuntime + TemplateArchiveProcessor.draft. Throws AgreementNotFoundError or AgreementConversionError on failure. * templatebuilder is imported from ../handlers/templatebuilder. It is technically under handlers/ in the current tree but is a transport-agnostic utility (no Express or MCP SDK imports). Moving it to a proper utility directory is a follow-up refactor. - server/handlers/agreements.ts * REST /:id/convert/:format now calls convertAgreement service. Adds the same strict-numeric :id guard as accordproject#208. * Trigger route unchanged (slice 2c territory). * Inline resolveAgreement helper kept because trigger still uses it. Duplication with resolveAgreementRuntime is intentional for slice 2b and gets removed in slice 2c. - server/handlers/mcp.ts * MCP `convert-agreement-to-format` tool now calls convertAgreement service directly. Adds the same strict-numeric guard. * Module fn `draftAgreement` deleted (was the HTTP-loop wrapper). * Trigger tool and its module fn `triggerAgreement` unchanged. Validation: - npm test in server/: 10/10 suites pass, 125 tests total - npx tsc --noEmit clean - Trigger path completely untouched; existing trigger tests (including the ones that exercise template-engine dynamic imports) all still pass Signed-off-by: Jay Guwalani <guwalanijj@gmail.com>
…iew) Niall asked for focused service-level coverage of `convertAgreement` alongside the indirect coverage the REST route tests already provide. Four new tests over a bespoke `.limit()`-per-call mock (since resolveAgreementRuntime does two independent selects): - resolves + returns drafted text on the happy path - throws AgreementConversionError when the referenced template is missing from the DB - throws AgreementNotFoundError when the agreement itself is missing - wraps template-engine draft failures as AgreementConversionError with the right typed `code` `templateFromDatabase` and `TemplateArchiveProcessor` are mocked at the module boundary so the service can be exercised without a real Postgres or a real .cta archive. Validation: - npm test: 10/10 suites pass, 138 tests total (4 new) - npx tsc --noEmit clean Signed-off-by: Jay Guwalani <guwalanijj@gmail.com>
…(slice 2c)
Slice 2c of the shared-service-layer port, closes the runtime-half
extraction started in slice 2b. `triggerAgreement` now lives in
`agreementService.ts` alongside `convertAgreement`, and both the REST
`/agreements/:id/trigger` route and the MCP `trigger-agreement` tool
call the same function. Two more internal HTTP loops removed from
`handlers/mcp.ts`, and the inline `resolveAgreement` helper in
`handlers/agreements.ts` is deleted (both routes now use the service's
`resolveAgreementRuntime`).
The slice-2c blocker documented at the end of the 2026-07-18 session
turned out to be a false alarm: it was caused by running
`npx jest handlers/agreements.test.ts` in the debug loop without the
`--experimental-vm-modules` flag. `npm test` sets the flag via the
`node --experimental-vm-modules node_modules/.bin/jest` prefix in the
`test` script and all 125 tests pass with trigger extracted.
Files:
- server/services/agreementService.ts
* `triggerAgreement(db, agreementId, requestBody)` uses
resolveAgreementRuntime + TemplateArchiveProcessor. Throws
InvalidPayloadError on `$class` mismatch, ValidationError on
Concerto validation failure, AgreementTriggerError on execution
failure. Wraps init + trigger in a single try so runtime
failures surface uniformly.
* resolveAgreementRuntime template-not-found path now throws plain
Error so globalErrorHandler renders `{ error: message }` at 500,
preserving the wire shape the REST test suite already asserts.
- server/handlers/agreements.ts
* REST /:id/trigger now calls the service; catches the three
"recoverable" typed error families and maps to the legacy
`{ isError: true, errorMessage, errorDetails }` at HTTP 200 for
backward compatibility. Not-found and generic errors bubble to
globalErrorHandler.
* Delete the inline resolveAgreement helper (now unused).
* Drop templateFromDatabase, TemplateArchiveProcessor, and the
duplicate error-import block.
- server/handlers/mcp.ts
* MCP `trigger-agreement` tool calls the service directly. Parses
the payload from the JSON string that the MCP tool contract
expects. Adds the same strict-numeric guard as slice 1/2/2b.
* Delete the module-level `triggerAgreement` HTTP-loop wrapper.
* Import triggerAgreement as `triggerAgreementService` to avoid
name collision with the MCP tool.
Validation:
- npm test: 10/10 suites pass, 125 tests total
- npx tsc --noEmit clean
Signed-off-by: Jay Guwalani <guwalanijj@gmail.com>
…iew)
Niall flagged that triggerAgreement's persist step spread the whole
agreement row back:
set({ ...agreement, state: triggerResult.state })
That writes every column, including the PK, on what is really a
single-field state transition. Two problems:
1. Unnecessary write surface — every column round-trips through the
update path for no reason.
2. Concurrent-clobber risk — any column that changed between the read
(top of triggerAgreement) and this write gets overwritten with the
stale in-memory value.
Change to a targeted set({ state: triggerResult.state }). Same
behavior for the state field, none of the collateral write, no clobber
risk.
Also fixes a stale assertion on convertAgreement's template-not-found
test — resolveAgreementRuntime throws a plain Error on this branch
(preserved from slice 2c's REST-wire-shape fix), so the test now
asserts against the plain Error message rather than
AgreementConversionError. Same coverage of the failure case, correct
error type.
Validation:
- npm test: 10/10 suites pass, 138 tests total
- npx tsc --noEmit clean
Signed-off-by: Jay Guwalani <guwalanijj@gmail.com>
…niallroche follow-up) Advisory note from Niall's review: two triggers arriving concurrently on a never-initialised agreement both see `state == null` and each runs `processor.init` + `processor.trigger` independently, then the targeted `.set({ state })` is last-write-wins. Inherited from the inline REST behaviour pre-slice-2c, not made worse in slice 2c, and rare in practice because a single client rarely fires concurrent triggers on the same agreement. Correct fix is a row-level lock or optimistic update at the DB layer, tracked as a follow-up. Document the assumption inline so a future reader does not have to re-derive it. No behaviour change. Signed-off-by: Jay Guwalani <guwalanijj@gmail.com>
JayDS22
force-pushed
the
jay/feat/service-layer-slice-2c-trigger
branch
from
July 21, 2026 15:03
50fca59 to
d12e3aa
Compare
niallroche
self-requested a review
July 21, 2026 21:08
niallroche
approved these changes
Jul 21, 2026
🎉 Thank you for your contribution! 🎉Dear @JayDS22, Your pull request has been successfully merged into the project! We greatly appreciate your efforts and the time you've dedicated to improving our repository. What happens next?
Once again, thank you for being part of our community! Best regards, |
JayDS22
added a commit
to JayDS22/apap
that referenced
this pull request
Jul 22, 2026
…ygiene wins (Jul 21) Updates the rolling roadmap for the Jul 21 milestone: the full service-layer chain (accordproject#211 templateService, accordproject#213 agreementService CRUD, accordproject#214 convertAgreement, accordproject#216 triggerAgreement) shipped upstream, plus the CI hygiene set (accordproject#210 crypto stub, accordproject#212 test-swallow, accordproject#215 server install) landed Jul 20-21. - Status header: bump to W8 day 1 / Jul 21, restate the merge count from eight to fifteen, headline the service-layer completion. - Workstream 1 (Proposal Core): reframe as "service-layer port complete upstream" with slice 3 REST unification as the remaining piece. - W7 row: flip Active -> Done, add the seven Jul 20-21 merges to the activity list. - Contributions table: add rows for accordproject#210, accordproject#211, accordproject#212, accordproject#213, accordproject#214, accordproject#215, accordproject#216, plus issue accordproject#217 (MCP paged reads follow-up), plus a combined row for the peer-review APPROVEs on Satvik's accordproject#192 + accordproject#203. Signed-off-by: Jay Guwalani <guwalanijj@gmail.com>
This was referenced Jul 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Slice 2c of the shared-service-layer port. Closes the runtime-half extraction that slice 2b started with convert.
triggerAgreementnow lives inagreementService.tsalongsideconvertAgreement, and both the REST/agreements/:id/triggerroute and the MCPtrigger-agreementtool call the same function. Two more internalmakeApiRequestHTTP loops are gone fromhandlers/mcp.ts, and the inlineresolveAgreementhelper inhandlers/agreements.tsis deleted (both REST routes now use the service's privateresolveAgreementRuntime).Stack
Stacked on top of #214 (slice 2b), which is stacked on #213 (slice 2), which is stacked on #211 (slice 1). Base is
main; the diff will collapse to the three files below once the earlier slices land.About the "slice 2c blocker" from 2026-07-18
I closed slice 2b with a note that trigger extraction was blocked by a jest+VM dynamic-import issue and needed its own investigation. That turned out to be a false alarm. In yesterday's debug loop I was running
npx jest handlers/agreements.test.tson the failing tests, which strips the--experimental-vm-modulesflag that thetestscript sets vianode --experimental-vm-modules node_modules/.bin/jest. The moment I switched back tonpm test, all 125 tests passed with trigger fully extracted. No code-side workaround needed.What this PR does
server/services/agreementService.tstriggerAgreement(db, agreementId, requestBody). UsesresolveAgreementRuntime+TemplateArchiveProcessor. ThrowsInvalidPayloadErroron$classmismatch,ValidationErroron Concerto validation failure,AgreementTriggerErroron execution failure. Wraps init + trigger in a single try so runtime failures surface uniformly.resolveAgreementRuntimetemplate-not-found path now throws a plainErrorsoglobalErrorHandlerrenders{ error: message }at 500 — this preserves the exact wire shape the REST test suite already asserts. Typed error usage would have changed the response body.server/handlers/agreements.ts/:id/triggercalls the service. Catches the three "recoverable" typed error families and maps them to the legacy{ isError: true, errorMessage, errorDetails }shape at HTTP 200 for backward compatibility:InvalidPayloadError-> full message on both fieldsValidationError-> hardcoded top-line + first concerto error message (viaerr.details.errors[0].message)AgreementTriggerError->err.upstreamMessageunwrapped (strips the typed"Failed to trigger agreement N: "prefix so existing clients see the raw business error unchanged)resolveAgreementhelper (unused now).templateFromDatabase,TemplateArchiveProcessor, and the duplicate error-import block.server/handlers/mcp.tstrigger-agreementtool calls the service directly. Parses the JSON string payload that the MCP tool contract expects. Adds the same strict-numeric guard as slice 1/2/2b.triggerAgreementHTTP-loop wrapper.triggerAgreementServiceto avoid a name collision with the MCP tool name.Validation
npm testinserver/: 10/10 suites pass, 125 tests totalnpx tsc --noEmitclean{ isError, errorMessage, errorDetails }clients see exactly the same body stringsAuthor Checklist
services/)