feat(gateway): native POST /v1/contacts handler + dual-write to assistant DB#30141
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53b5fc5857
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| role: params.role ?? "contact", | ||
| principalId: params.principalId ?? null, | ||
| updatedAt: now, |
There was a problem hiding this comment.
When POST /v1/contacts resolves an existing contact via channel address (no id provided), this update path overwrites omitted auth fields with defaults (role → "contact", principalId → null). That can silently demote guardian contacts and break principal-based authorization/binding flows for any client that performs partial upserts by channel. This should preserve existing values when those fields are absent, like the id-based update branch does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — channel-match UPDATE now preserves role/principalId when the caller doesn't pass them. Mirrors the !== undefined guards in assistant/src/contacts/contact-store.ts:213-223.
| } as unknown as Record<string, unknown>).catch(() => {}); | ||
|
|
||
| // ── Build response from assistant DB ───────────────────────────── | ||
| const fullContact = await readAssistantContact(contactId); |
There was a problem hiding this comment.
The route treats assistant DB writes as best-effort, but it unconditionally reads from the assistant DB afterward. If the assistant IPC/DB path is down, readAssistantContact() throws here and the request fails instead of returning the documented gateway fallback contact shape. This turns a degraded dependency into a hard 5xx for contact creation/update.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — readAssistantContact is now wrapped in .catch() with a synthetic gateway-shape fallback, so an assistant-DB outage does not 5xx the upsert. The dual-write itself is also wrapped in try/catch with log.warn so a transient failure doesn't fail the request.
| // ── 2. Look up by channel address ───────────────────────────────── | ||
| if (!contactId && params.channels?.length) { | ||
| for (const ch of params.channels) { | ||
| const address = ch.address.toLowerCase(); |
There was a problem hiding this comment.
Channel elements are only validated for status/policy, but this code assumes address is a string. A payload like {"channels":[{}]} reaches this line and throws on toLowerCase(), producing a server error instead of a 400 bad-request. Since this endpoint is public API surface, malformed channel objects should be rejected during validation rather than crashing in storage logic.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — added explicit channel type and address validation at the route level. Empty/missing/whitespace values return 400 before the store ever sees them. Tests added.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| await assistantDbRun( | ||
| `UPDATE contacts | ||
| SET display_name = ?, | ||
| notes = ?, | ||
| role = ?, | ||
| contact_type = ?, | ||
| principal_id = ?, | ||
| updated_at = ? | ||
| WHERE id = ?`, | ||
| [ | ||
| displayName, | ||
| (body.notes as string | null | undefined) ?? null, | ||
| (body.role as string | undefined) ?? "contact", | ||
| (body.contactType as string | undefined) ?? "human", | ||
| (body.principalId as string | null | undefined) ?? null, | ||
| now, | ||
| contactId, | ||
| ], | ||
| ); |
There was a problem hiding this comment.
🔴 Assistant DB dual-write UPDATE unconditionally overwrites fields, causing data loss
When updating an existing contact in the assistant DB, the SQL UPDATE unconditionally sets notes, role, contact_type, and principal_id to default values (null, "contact", "human", null) even when the request body doesn't provide those fields. The assistant's own upsertContact (assistant/src/contacts/contact-store.ts:213-223) only updates fields that are explicitly provided (guarded by !== undefined checks). For example, a request like { "displayName": "Alice" } targeting an existing contact with notes: "Important" and role: "guardian" would reset notes to null, role to "contact", and contactType to "human". Since the response is built from the assistant DB via readAssistantContact (line 609), this data loss is also visible in the API response.
Prompt for agents
The assistant DB UPDATE in handleUpsertContact (lines 468-486) unconditionally sets notes, role, contact_type, and principal_id with default values. This should mirror the assistant's own upsertContact behavior (assistant/src/contacts/contact-store.ts:213-223) which only includes fields in the SET clause when they are explicitly provided (checked with !== undefined).
The fix should build the SET clause dynamically:
1. Always include display_name and updated_at.
2. Only include notes when body.notes !== undefined.
3. Only include role when body.role !== undefined.
4. Only include contact_type when body.contactType !== undefined.
5. Only include principal_id when body.principalId !== undefined.
This requires building the SQL string and bind array conditionally, similar to how the channel update code already does with setParts/setParams pattern (lines 541-560 in the same file).
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in b6a17fa4da. The assistant-DB UPDATE now uses a dynamic SET clause: only the fields the caller actually provided mutate. notes/role/contact_type/principal_id are no longer clobbered to defaults on a partial upsert. Mirrors the same pattern in assistant/src/contacts/contact-store.ts:213-223.
| this.db | ||
| .update(contacts) | ||
| .set({ | ||
| displayName: params.displayName, | ||
| role: params.role ?? "contact", | ||
| principalId: params.principalId ?? null, | ||
| updatedAt: now, | ||
| }) | ||
| .where(eq(contacts.id, contactId)) | ||
| .run(); |
There was a problem hiding this comment.
🔴 Gateway DB channel-address-match UPDATE clobbers existing role and principalId
In ContactStore.upsertContact, the channel-address-match UPDATE path (step 2) defaults role to "contact" and principalId to null when those fields aren't provided, whereas the id-match UPDATE path (step 1, lines 249-255) correctly preserves existing values using params.role ?? existing.role and params.principalId !== undefined ? params.principalId : existing.principalId. This inconsistency means a contact found via channel address — e.g., one with role: "guardian" — would have its role silently reset to "contact" and its principalId cleared to null if the caller omits those fields. The assistant's upsertContact (assistant/src/contacts/contact-store.ts:261-271) is consistent across both paths, only updating fields that are explicitly provided.
Prompt for agents
In ContactStore.upsertContact (contact-store.ts), the channel-address-match UPDATE path (step 2, lines 291-303) should preserve existing values like the id-match path (step 1, lines 246-259) does. Currently it does:
role: params.role ?? "contact",
principalId: params.principalId ?? null,
It should instead read the existing contact first and preserve values:
const existingContact = this.db.select().from(contacts).where(eq(contacts.id, contactId)).get();
// then in .set():
role: params.role ?? existingContact?.role ?? "contact",
principalId: params.principalId !== undefined ? params.principalId : (existingContact?.principalId ?? null),
Alternatively, adopt the same conditional-only-update pattern the assistant uses (only set fields that are explicitly provided !== undefined).
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed — same lesson as the route-level UPDATE. The channel-match path now preserves existing role/principalId unless the caller explicitly provides them.
| async function resolveAssistantUserFileSlug( | ||
| displayName: string, | ||
| ): Promise<string> { | ||
| const slug = | ||
| displayName | ||
| .toLowerCase() | ||
| .replace(/[^a-z0-9]+/g, "-") | ||
| .replace(/^-+|-+$/g, "") | ||
| .slice(0, 100) || "user"; | ||
|
|
||
| const rows = await assistantDbQuery<{ userFile: string | null }>( | ||
| "SELECT user_file AS userFile FROM contacts WHERE user_file LIKE ?", | ||
| [`${slug}%`], | ||
| ); | ||
| const taken = new Set( | ||
| rows.map((r) => r.userFile?.toLowerCase()).filter(Boolean), | ||
| ); | ||
|
|
||
| const base = `${slug}.md`; | ||
| if (!taken.has(base)) return base; | ||
|
|
||
| for (let i = 2; i <= 100; i++) { | ||
| const candidate = `${slug}-${i}.md`; | ||
| if (!taken.has(candidate)) return candidate; | ||
| } | ||
| return `${slug}-${crypto.randomUUID().slice(0, 8)}.md`; | ||
| } |
There was a problem hiding this comment.
🚩 Missing principalId sibling userFile sharing in assistant DB dual-write
The assistant's upsertContact (assistant/src/contacts/contact-store.ts:292-304) has logic to share userFile slugs among sibling contacts with the same principalId. The gateway's resolveAssistantUserFileSlug (contacts-control-plane-proxy.ts:71-97) always generates a fresh slug without checking for siblings. This means contacts created via the gateway for the same principal will get different userFile slugs, potentially fragmenting persona directories. This is in the best-effort dual-write path so it's not critical, but it's a behavioral divergence worth noting.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good catch — fixed in 3a270d530c. resolveAssistantUserFileSlug now takes principalId; if a sibling contact with that principalId already has a user_file, we reuse it. Mirrors assistant/src/contacts/contact-store.ts:288-303.
| if (body.contactType === "assistant") { | ||
| if (!assistantMeta) { | ||
| return Response.json( | ||
| { | ||
| error: { | ||
| code: "BAD_REQUEST", | ||
| message: | ||
| 'assistantMetadata is required when contactType is "assistant"', | ||
| }, | ||
| }, | ||
| { status: 400 }, | ||
| ); | ||
| } | ||
| if (!isAssistantSpecies(assistantMeta.species)) { | ||
| return Response.json( | ||
| { | ||
| error: { | ||
| code: "BAD_REQUEST", | ||
| message: `Invalid species "${assistantMeta.species}". Must be one of: ${VALID_ASSISTANT_SPECIES.join(", ")}`, | ||
| }, | ||
| }, | ||
| { status: 400 }, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🚩 validateSpeciesMetadata not called in gateway implementation
The assistant's handleUpsertContactRoute (assistant/src/runtime/routes/contact-routes.ts:634-643) calls validateSpeciesMetadata() to verify that assistant-type contact metadata conforms to species-specific requirements. The gateway's implementation validates species against the allowed list but does not call validateSpeciesMetadata. If species-specific metadata validation matters (e.g., ensuring required fields for a 'vellum' species), this could allow invalid metadata through the gateway path. Worth checking whether this validation is important for data integrity.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Done — added validateSpeciesMetadata to the route (mirrors assistant/src/contacts/contact-store.ts:1021). Vellum-species metadata must include non-empty assistantId and gatewayUrl strings; otherwise 400. 3 tests added.
| await assistantDbRun( | ||
| `INSERT INTO contact_channels | ||
| (id, contact_id, type, address, is_primary, | ||
| external_user_id, external_chat_id, | ||
| status, policy, interaction_count, created_at, updated_at) | ||
| VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, 0, ?, ?)`, | ||
| [ | ||
| crypto.randomUUID(), | ||
| contactId, | ||
| ch.type, | ||
| address, | ||
| ch.isPrimary ? 1 : 0, | ||
| ch.externalUserId ?? null, | ||
| ch.externalChatId ?? null, | ||
| ch.status ?? "unverified", | ||
| ch.policy ?? "allow", | ||
| now, | ||
| now, | ||
| ], | ||
| ); |
There was a problem hiding this comment.
🚩 Different channel UUIDs generated for gateway DB vs assistant DB
When inserting a new channel, the gateway DB #syncChannels (contact-store.ts:403) generates a crypto.randomUUID(), and separately the assistant DB dual-write (contacts-control-plane-proxy.ts:581) generates another crypto.randomUUID(). This means the same logical channel will have different IDs in the two databases. During the migration period this could cause issues if code expects channel IDs to match across databases (e.g., for the markChannelVerified dual-write which uses the channel ID). This is likely an intentional simplification given the best-effort nature of the dual-write, but worth noting.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Intentional during the transition. The gateway DB is the new source of truth for contacts/channels (see gateway-security-migration.md plan); the assistant-DB rows are best-effort mirrors that exist only so the daemon's current readers don't break mid-migration. Track B and the follow-up that deletes the daemon's POST /v1/contacts handler eliminate the dual-write entirely. After that, only gateway-DB IDs exist.
| this.db | ||
| .update(contacts) |
There was a problem hiding this comment.
Remember, we need to dual write to the assistant db anytime we write to gateway contacts or channels
Edit: oh you did it at the route level, bring it down to the service layer
There was a problem hiding this comment.
Done — upsertContact is now async and owns the assistant-DB dual-write internally. The route handler is a thin validate-then-call wrapper. See b6a17fa4da.
| return { contact, channels, created }; | ||
| } | ||
|
|
||
| #syncChannels( |
There was a problem hiding this comment.
This looks like invalid syntax
There was a problem hiding this comment.
Renamed #syncChannels → private syncChannels. Also folded the assistant-DB channel sync into the same service-layer method.
| // --------------------------------------------------------------------------- | ||
|
|
||
| const VALID_CONTACT_TYPES = ["human", "assistant"] as const; | ||
| const VALID_ASSISTANT_SPECIES = ["vellum", "openclaw"] as const; |
There was a problem hiding this comment.
We can remove support for OpenClaw for now
There was a problem hiding this comment.
Done — VALID_ASSISTANT_SPECIES = ["vellum"] as const. Added a test that rejects species: "openclaw" with 400.
| * 3. Create a new contact with a generated id. | ||
| */ | ||
| async handleUpsertContact(req: Request): Promise<Response> { | ||
| return forward(req, "/v1/contacts"); |
There was a problem hiding this comment.
We should delete the assistant side route on the next pr
There was a problem hiding this comment.
Acknowledged — opening a follow-up PR after this lands to delete the daemon's POST /v1/contacts handler (assistant/src/runtime/routes/contact-routes.ts:677).
…tant DB
- Replace forward() stub with full gateway-native handleUpsertContact:
- Validates displayName, contactType, assistantMetadata, channel enums
- Writes to gateway DB via ContactStore.upsertContact()
- Dual-writes contact + channels + assistant_contact_metadata to assistant
DB via assistantDbRun (best-effort, logged on failure)
- Emits contacts_changed via ipcCallAssistant
- Returns { ok: true, contact } from assistant DB (fallback to gateway data)
- Add ContactStore.upsertContact() — mirrors assistant resolution order:
1. match by id, 2. match by (type, address) on any channel, 3. create new
Skips cross-contact channel conflicts (no-reassignment policy)
- Export SqliteValue type from assistant-db-proxy for typed SQL bindings
- Update tests: remove handleUpsertContact from forwarded-endpoints assertion,
add 4 gateway-native tests (validation + happy path)
- ContactStore.upsertContact is now async; owns gateway-DB write + best-effort assistant-DB dual-write internally. Route handler becomes a thin validate-then-call wrapper. - Channel-match UPDATE preserves existing role/principalId unless the caller explicitly passes them (mirrors the assistant's contact-store guards) — fixes a partial-upsert demotion path. - Assistant-DB UPDATE uses a dynamic SET clause: only the fields present in the input mutate. Prevents notes/role/contact_type/ principal_id from being clobbered to defaults on a partial upsert. - readAssistantContact() failures fall back to a synthetic gateway- shape contact instead of 5xx-ing the request. - Validate channel.type and channel.address are non-empty strings up front (400 instead of throwing on .toLowerCase()). - Validate species metadata at the route: vellum requires non-empty assistantId + gatewayUrl strings (mirrors validateSpeciesMetadata in assistant/src/contacts/contact-store.ts). - Drop 'openclaw' from VALID_ASSISTANT_SPECIES — vellum-only here. - Renamed #syncChannels (private-method syntax) to private syncChannels. Tests: 19 pass (added 7 for new validations). Addresses review feedback from Vargas (dual-write in service layer, syntax, openclaw), Codex (role/principalId preservation, readback fallback, channel & species validation), and Devin (UPDATE clobber).
vellum-apollo-bot
Bot
force-pushed
the
apollo/gateway-native-contact-upsert
branch
from
53b5fc5 to
b6a17fa
Compare
When INSERTing a new contact into the assistant DB whose principalId already has a sibling contact, reuse that sibling's user_file slug instead of generating a fresh one. Mirrors the assistant's slug logic (`assistant/src/contacts/contact-store.ts:288-303`) — every channel for one principal must resolve to the same persona + journal slug. Addresses Devin analysis comment id 3213334488.
…30372) The native handler introduced in #30141 accepted role and principalId from the request body and passed them straight into ContactStore.upsertContact. The route is protected only by generic edge auth, so any authenticated caller could rebind the guardian by POSTing the guardian's contact id plus role:"guardian" + their own principalId — elevating to guardian for every guardian-only flow. Fix: strip role and principalId from the route input AND from ContactStore.upsertContact's params surface (the route is the only caller; guardian binding is owned by guardian-bootstrap, which writes guardian role via raw SQL with its own privileged path). On update, existing role/principalId are preserved. On create, role defaults to "contact" and principalId to null. Test added: privilege-escalation regression asserting POST /v1/contacts with role:"guardian" + principalId in body does not forward those fields to the service layer. Co-authored-by: ApolloBot <apollo@vellum.ai>
1 participant
What
Makes
POST /v1/contactsgateway-native — gateway writes contact data directly instead of forwarding to the assistant daemon. Part of the ATL-311 track: removingupsertContact/upsertContactChannelfrom the daemon.Changes
gateway/src/db/contact-store.ts— newContactStore.upsertContact()params.id(if provided)(type, address)on any provided channelid,displayName,role,principalId)gateway/src/http/routes/contacts-control-plane-proxy.ts— nativehandleUpsertContactdisplayName,contactType ∈ {human,assistant},assistantMetadatarequired for assistant-type contacts, channelstatus/policyenumsContactStore.upsertContact()assistantDbRun(best-effort; logs warn on failure, never fails the request):INSERT OR UPDATE contacts(preservesuserFile,createdAton update)INSERT OR UPDATE assistant_contact_metadatafor assistant-type contactsUPDATE/INSERTintocontact_channelsuserFileslug computed viaresolveAssistantUserFileSlug()(collision-safe, mirrors daemon logic)contacts_changedevent via IPC after write{ ok: true, contact }from assistant DB (falls back to gateway contact shape if assistant DB is unavailable)gateway/src/db/assistant-db-proxy.tsSqliteValuetype for use by callers needing typed SQL bindingsgateway/src/__tests__/contacts-control-plane-proxy.test.tshandleUpsertContactfrom the forwarded-endpoints assertion (it no longer forwards)handleListContactsinsteadContext
This is Track A of the
upsertContactremoval from the assistant daemon:POST /v1/contactsnativelyupsertContactChannelininvite-redemption-service.tsThe daemon's
POST /v1/contactsroute handler (contact-routes.ts:677) becomes dead code after this and will be removed in a follow-up cleanup PR.Tests
12/12 pass (
contacts-control-plane-proxy.test.ts)