fix(contacts): return post-update row from conditional engagement update; accept RFC-3986 Location in suite 30

[jiashuoz]

Summary

Fixes the two deterministic failures found by the first live staging run of the conformance suite (e2a-ops release-pipeline run 30612956986). Must merge before the v1.5.0 tag — the conformance gate fails deterministically on both without it.

Issue B — server bug: conditional engagement update returns the pre-update row

identity.Store.UpdateEngagementIfUnchanged read the row back as the outer SELECT of a data-modifying CTE:

WITH updated AS (UPDATE contact_engagements SET ... WHERE ... AND updated_at = $8 RETURNING id)
SELECT <joined columns> FROM contact_engagements ce ... WHERE ce.id = (SELECT id FROM updated)

In Postgres the outer query of a data-modifying CTE runs on the statement snapshot, which predates the CTE's own writes — so the outer SELECT scans the pre-update version of the row. The UPDATE commits correctly; only the returned row is stale (old stage, old next_action_at, old updated_at). Since the response ETag derives from updated_at, the returned validator can never match the stored row again, breaking every If-Match read-modify-write chain.

Live evidence: tests/e2e-prod/suites/30-contacts-outreach.test.ts:414 — the If-Match update returned 200 with stage "prospect" (expected "nurture") and a stale ETag.

Fix: split into two statements in one transaction — the UPDATE (version predicate unchanged, so the If-Match race stays closed exactly as before) RETURNING id, then a read of the full joined row by primary key, which sees the prior statement's writes. This mirrors UpsertEngagement's write-then-GetEngagement shape, so both paths now have identical read-back semantics. The ErrEngagementPreconditionFailed / ErrEngagementNotFound disambiguation is byte-for-byte unchanged.

Audit of other data-modifying CTEs under internal/ (grep WITH updated|touched|upd|candidates|children|requested):

• identity/store.go GetPrincipalByAPIKey (WITH touched) — outer SELECT reads FROM touched (the RETURNING output) joined to unmodified users. Safe.
• identity/store.go GetMessageWithContent (WITH upd) — outer SELECT reads FROM upd joined to unmodified webhook_deliveries. Safe.
• webhook/store.go + webhookpub/worker.go lease queries (WITH candidates) — read-only SELECT … FOR UPDATE CTE feeding the main UPDATE, whose RETURNING carries post-update values. Safe.
• identity/thread_identity.go (WITH requested), identity/store.go detachThreadChildrenBatchTx (WITH children) — read-only CTEs. Safe.

UpdateEngagementIfUnchanged was the only instance of the stale pattern (outer SELECT over the modified table itself).

Regression test: TestUpdateEngagementIfUnchangedReturnsPostUpdateRow (DB-backed, internal/identity) asserts the accepted conditional update returns the post-update stage/next_action_at, that updated_at advanced past expectedUpdatedAt (the ETag moves), that the returned row equals a subsequent GetEngagement, and that chaining a second conditional update on the returned validator succeeds. Verified it fails against the old implementation with exactly the live symptoms (stage stays "prospect", stale validator, chained update → precondition failed) and passes with the fix. Handler-level pinning in internal/httpapi/engagements_test.go is moot: the handler deps mock UpdateEngagementIfUnchanged outright and the ETag is computed from whatever the store returns, so the store test is the right (and only meaningful) level.

Issue C — test over-strictness: Location-header encoding in suite 30

30-contacts-outreach.test.ts:217-220 required the createContact Location to contain encodeURIComponent(address) (%40), but the server emits a raw @ via Go's url.PathEscape — valid RFC 3986 (pchar allows @ in a path segment). Live failure: Location: /v1/contacts/ctdup-cd2014-6e046f@example.com.

Fixed the test only (no server change): decode the trailing path segment and compare addresses, accepting either spelling. Audited every other encodeURIComponent use in the suite — all others build request URLs (encoding a request path is always legal) except the engagement-Location assertion at line 409, which only checks the /v1/agents/ prefix and is encoding-agnostic.

Client surface checklist

No API surface change — server-internal persistence fix + test relaxation. No spec, SDK, CLI, MCP, or web changes; api/openapi.yaml untouched.

Operational risk

Low. One extra statement inside a short transaction on the conditional-update path only (If-Match requests). No schema change, no migration.

Test evidence

• go test -tags integration ./internal/identity/ ./internal/httpapi/ — ok (118s / 5s)
• New regression test fails on old code, passes on fix (see above)
• tests/e2e-prod: tsc --noEmit clean, npm run test:unit 18/18 pass, npm run test:unit:gates 6/6 OK

🤖 Generated with Claude Code