[lawyered0]

What's broken

Agreement.template is a Concerto relationship, so the value that validates and that should be stored is resource:org.accordproject.protocol@1.0.0.Template#<identifier>. PR #241 documented two other values for POST /agreements:

• a bare https://…cta URL, which the handler is written to fetch
• the bare ap://name@version#hash URI that POST /templates/archive returns

Validation ran first and rejected every scheme other than resource:, so both documented requests returned 400 and never fetched or looked anything up. Sending the relationship form already worked, including GET and convert/html.

What changed

POST and PUT /agreements accept either form. A bare http://, https://, or ap:// identifier is percent-encoded and saved as resource:org.accordproject.protocol@1.0.0.Template#…. A value that is already a resource: relationship is stored unchanged. Lookup and HttpTemplateRetriever unwrap that stored value, so the fetch and the ap:// template lookup are the same as before. Any other scheme is still a 400.

The README quick start and the archive-upload example now say that the bare URI is accepted and normalized.

How tested

From server/: npx tsc -p . --noEmit, npm test (406 passed), npm run lint.

Against Postgres with this change:

• README quick start (https://templates.accordproject.org/archives/latedeliveryandpenalty@1.0.0.cta) no longer 400s. It reaches the fetch and returns the pre-existing Cicero mismatch 500 (^1.0.0 vs this server's 2.2.0).
• POST /templates/archive, then POST /agreements with the returned ap:// URI, stores the percent-encoded relationship. GET /agreements/:id and GET /agreements/:id/convert/html both return 200.
• A bare http:// URL for a Cicero 2 archive is fetched, stored as the relationship, and converts to HTML.
• An existing resource:…Template#dan2 value is stored unchanged. ftp:// is still 400.

[niallroche]

The fix is correct and the test coverage is good — the invariant templateUriFromReference(normalizeAgreementTemplateReference(bare)) === bare is verified for all three URI forms, the PUT tests confirm Concerto's decoded form round-trips, and the ftp:// rejection is exercised.

One real concern inline (the POST/PUT encoding divergence via mutation), one nit on the constant name. The encoding divergence doesn't block merging — templateUriFromReference and assertTemplateNotInUse both handle it today — but it should be documented to avoid a future silent regression.

---

Generated by Claude Code

[niallroche]

The mutation of body.template here is load-bearing for the POST path, but it creates a hidden encoding divergence between POST and PUT that's easy to miss.

What happens today:

• POST (handlers/agreements.ts): uses req.body directly for the DB insert after validateAgreementBody returns, so Agreement.template is stored as resource:...Template#https%3A%2F%2F... (the encodeURIComponent form from normalizeAgreementTemplateReference).
• PUT (via CRUD): crud.ts does req.body = customResult.data after the custom validator returns, so Agreement.template is stored as resource:...Template#https://... — Concerto's serializer.toJSON decodes percent-encoded identifiers on the way out.

Both forms round-trip correctly through templateUriFromReference and are covered by assertTemplateNotInUse's three-form inArray (#264), so functionally this is fine today.

The fragility: if someone later makes POST consistent by using result.data too, the stored encoding silently changes and assertTemplateNotInUse's coverage needs reassessing. The existing docstring explains the mutation but doesn't flag the encoding divergence.

Suggestion: add a one-line comment next to the POST handler's DB insert (in handlers/agreements.ts) noting that req.body.template is the percent-encoded form from validateAgreementBody, not the Concerto-decoded form in result.data. Alternatively, factor POST to also use result.data.template for the insert so the two paths are consistent — that's a larger change but eliminates the class of issue entirely.

---

Generated by Claude Code

[lawyered0]

Thanks — documented the POST/PUT distinction next to the insert in 9bded76, keeping the existing storage behavior.

Checking this also exposed a frozen-record issue: the no-op test seeded PUT's canonical output rather than a POST-created row. It now captures the POST insert and resends through PUT for both COMPLETED and SUPERSEDED. The guard compares the referenced template URI and skips only Concerto's non-column $class/$identifier properties; actual template changes remain blocked.

A URI containing its own %20/%2F round-trips correctly, but Concerto's encodeURI spelling needed an additional assertTemplateNotInUse candidate. Added that and regression coverage too.

All 136 tests in the three affected suites pass, as does tsc --noEmit. Fresh independent review found no blocking issues.

[niallroche]

Nit: BARE_TEMPLATE_URI reads as if the constant holds a URI pattern, but it's testing the scheme prefix. NORMALIZABLE_TEMPLATE_SCHEME or just BARE_TEMPLATE_SCHEME would be clearer at a glance.

---

Generated by Claude Code

[lawyered0]

Renamed to BARE_TEMPLATE_SCHEME in 9bded76. Thanks.

[niallroche]

The new commit addresses every point from the previous review precisely:

• BARE_TEMPLATE_SCHEME rename done.
• Comment at the POST insert documenting the encodeURIComponent vs Concerto encodeURI divergence is exactly right.
• assertTemplateNotInUse now has 4 forms: bare URI, unencoded relationship, encodeURIComponent (POST path), encodeURI (Concerto/PUT path). The test with an already-percent-escaped URI confirms all four params are populated and distinct.
• assertAgreementRecordMutable now skips $class/$identifier (Concerto virtual properties not stored in the row) and uses templateUriFromReference to compare template values by their decoded URI rather than their wrapper — so a no-op resend of a frozen agreement with the same template in a different encoding is not rejected.
• Tests cover the new paths: encodeURI preservation, frozen-record no-op resend for both COMPLETED and SUPERSEDED, and a negative case confirming a genuinely different template still throws.

The invariant templateUriFromReference(normalizeAgreementTemplateReference(bare)) === bare holds for all three storage forms, and assertTemplateNotInUse covers them all. Good to merge.

---

Generated by Claude Code

[JayDS22]

Approve with one question on the dual POST/PUT spelling. Clean fix for a real documentation-vs-behavior split from #241: Concerto validation ran first, rejected every non-resource: scheme, and the documented http(s) and ap:// forms could never reach the fetch or lookup paths. normalizeAgreementTemplateReference rewrites http(s)/ap:// to a resource: relationship before concertoValidation sees it, resource: passes through, everything else still 400s. The test matrix is the strongest part: real Concerto validation (not the mock) exercised for both POST and PUT, ap:// and https://, a resource-form passthrough, percent-escape preservation, ftp:// rejection, and a COMPLETED/SUPERSEDED no-op resend that actually goes through POST then PUT to catch the frozen-record guard regression.

HIGH: dual spelling of the same identifier in the DB. POST ends up storing resource:...#${encodeURIComponent(ref)} (raw req.body after in-place mutation), PUT stores resource:...#${encodeURI(ref)} (Concerto's serialized form via customResult.data). Same logical template, two canonical representations in the agreement.template column. The PR compensates well: templateUriFromReference decodes both, assertTemplateNotInUse now checks three spellings, and assertAgreementRecordMutable compares via templateUriFromReference. The inline comment at handlers/agreements.ts:81-83 documents the split deliberately. Question: is unifying on one spelling (letting POST also insert customResult.data from validateAgreementBody, mirroring what buildCrudRouter already does on PUT) in scope for this PR, or a tracked follow-up? Every new downstream consumer that reads agreement.template is going to have to know about both forms, and that knowledge is not free.

MEDIUM: in-place mutation of req.body. validateAgreementBody reassigns body.template on the caller's object. For POST that caller is req.body, which lives for the whole middleware chain; anything observing it after validation sees the mutated value. Right now that is what POST wants (insert uses req.body directly), but the function reads as a pure validator and isn't one. A shallow clone ({ ...body, template: normalize(body.template) }) before passing into concertoValidation keeps the function pure and does not change the observable behavior for either call site.

LOW: BARE_TEMPLATE_SCHEME = /^(?:https?|ap):\/\// is case-sensitive. HTTP://, AP:// will fall through to Concerto and 400. URL schemes are case-insensitive per RFC 3986 and the fetch retriever is tolerant, so this is a behavior gap at the validator boundary. Low priority since nothing in the README example uses uppercase, but a /i flag costs nothing.

Question on the mutation-guard skip set. assertAgreementRecordMutable now skips $class and $identifier. Are those the only Concerto virtuals that can appear on an Agreement row at PUT time? If the Agreement concept grows a $timestamp/$namespace/other virtual in a future model revision, the skip set silently gets stale and a legitimate no-op resend starts tripping AgreementRecordImmutableError. Fine for now if Agreement is stable, worth a comment pinning the assumption.

Nits:

• The "two spellings, one logical identifier" explanation is the key invariant readers need. Right now it lives in handlers/agreements.ts:81-83 as an inline on the insertData build. Consider lifting it to the normalizeAgreementTemplateReference JSDoc or to templateUriFromReference, next to the decode half that depends on it.
• server/README.md:200 example block adds the new normalization sentence inside the same code-fence context. Reads cleanly, no change needed.

LGTM on the fix as-landed. The dual-spelling question is the one I'd want a line on in the PR thread before merge, even if the answer is "follow-up, tracked at #...".

[lawyered0]

Thanks, @JayDS22. I’m keeping storage unification as a separate follow-up, with the scope recorded in this PR comment: move POST to the returned serializer data, check the resulting field/date normalization changes, and decide whether to migrate existing rows. Changing the new writer alone would still leave old rows in both spellings, so downstream compatibility remains necessary.

Pushed d70c297 with the dual-writer invariant beside the decoder, an explicit mutation contract on validateAgreementBody, and a comment tying the virtual-property exceptions to the current Agreement concept/default serializer options. The POST regression now also checks data/state stringification.

I tested the shallow-clone proposal before adopting it. Three existing POST regressions failed because the insert retained the bare template URI. Cloning also prevents concertoValidation’s data/state stringification from reaching req.body. That change needs to be coordinated with the POST writer; the current mutation stays intentional.

The uppercase suggestion also needs an end-to-end change. A real probe confirmed that normalizing HTTPS:// lets validation pass, but POST’s case-sensitive matcher selects no retriever, and HttpTemplateRetriever.fetch independently rejects it. I’ve left scheme handling unchanged here. Case-insensitive handling across normalization, dispatch and fetch is another follow-up scope recorded here, without a new issue.

All 136 tests in the three affected suites pass, server tsc --noEmit passes, and fresh independent review found no blockers. DCO and Snyk are green.

[github-actions]

🎉 Thank you for your contribution! 🎉

Dear @lawyered0,

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?

• Your changes will be included in the next release
• Your name will be added to our contributors list
• Feel free to take on another issue or suggest new features

Once again, thank you for being part of our community!

Best regards,
The APAP Team