chore(assistant): remove POST /v1/contacts daemon route and contact_upsert skill tool (follow-up to #30141)#30278
Conversation
…psert skill tool Follow-up to #30141 (Track A: gateway-native contact upsert dual-write). Now that the gateway owns contact writes natively, the daemon-side POST /v1/contacts route and the LLM-facing `contact_upsert` skill tool are dead weight — and the LLM having a direct contact-write path contradicts the architectural decision that contact writes are guardian-only. Changes: * Delete the `upsert_contact` route from `runtime/routes/contact-routes.ts` (handler, helper, species validation, route entry). Gateway is now the sole HTTP write surface for contacts (`POST /v1/contacts` is proxied through the gateway control-plane route, which has its own `validateSpeciesMetadata` copy). * Delete the bundled `contact_upsert` skill tool (`config/bundled-skills/contacts/tools/contact-upsert.ts`), its entry in `TOOLS.json`, and its registration in `bundled-tool-registry.ts`. * Rewrite the `contacts` SKILL.md to encode the guardian-only policy: contact creation flows through the Contacts tab in the assistant web UI or the invite system; the LLM walks the user to one of those paths instead. Invite docs now say "look up the contact with `assistant contacts list`" and stop telling the LLM to fall back to `assistant contacts upsert`. * Remove the now-unused `upsertAssistantContactMetadata` and `validateSpeciesMetadata` exports from `contact-store.ts`. `upsertContact` itself is kept — it remains the underlying write path used by the gateway dual-write, the `assistant contacts upsert` CLI (guardian-only, host-side), invite redemption, and test fixtures. * Rewire `__tests__/contacts-tools.test.ts` to use a tiny `upsertFixture()` helper that goes straight to the store, and drop the dedicated `contact_upsert tool` describe block. Net diff: 7 files, +63/-610. Search, merge, list, and google-contacts tools still available to the LLM. No CLI surface changed. Track B (invite-redemption gateway-native) remains as the next item in this workstream.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a0998ab7e
ℹ️ 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".
| { | ||
| operationId: "merge_contacts", | ||
| endpoint: "contacts/merge", |
There was a problem hiding this comment.
Keep upsert_contact IPC method available for CLI
By deleting upsert_contact from the shared ROUTES table, this commit removes the IPC handler that assistant contacts upsert still calls (cliIpcCall("upsert_contact", ...) in assistant/src/cli/commands/contacts.ts). The IPC server now has no matching method and returns Unknown method: upsert_contact, so the guardian-facing CLI upsert flow fails at runtime. Please keep an IPC route for this method (it can be IPC-only) or migrate the CLI command to the new write path before removing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in #30281 with the second option from your suggestion ("migrate the CLI command to the new write path before removing it") — but going further and removing the orphan subcommand entirely instead of re-routing it through the gateway. Per the architectural decision encoded in this PR, contact writes are guardian-only via the assistant web UI Contacts tab or the invite flow; the CLI never needed a write surface either. Thanks for catching this — exactly right that the previous state would have failed at runtime with Unknown method: upsert_contact.
There was a problem hiding this comment.
🚩 SKILL.md typical workflows still reference creating contacts
Several entries in the "Typical Workflows" section at the bottom of SKILL.md still contain phrases like "Look up or create the contact first" (visible in the truncated diff lines for Telegram invite, email invite, WhatsApp invite, Slack invite, voice invite workflows). While the full text is truncated in the diff, these likely still reference the old assistant contacts upsert command. This is inconsistent with the new guidance at the top of the file that says contact writes are guardian-only and must go through the web UI. The assistant could attempt to use the now-removed upsert tool if it follows these workflow instructions.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Already addressed in this PR — the second push replaced the per-channel --contact-id flag descriptions across the Telegram, voice, email, WhatsApp, and Slack invite sections so they read: "Look up the contact first with assistant contacts list. New contacts must be added via the Contacts tab in the assistant web UI; you cannot create them here." The top-level Every invite must be bound to a contact note was updated in the same pass. The assistant contacts upsert CLI itself is removed in #30281 so even shelling out to it is no longer a path. Grep on the merged state for assistant contacts upsert in SKILL.md returns empty.
| handler: ({ pathParams }: RouteHandlerArgs) => | ||
| handleGetContact(pathParams!.id), | ||
| }, | ||
| { | ||
| operationId: "upsert_contact", | ||
| endpoint: "contacts", | ||
| method: "POST", | ||
| summary: "Create or update a contact", | ||
| description: | ||
| "Create a new contact or update an existing one. Supports upsert by contactId or channel handle.", |
There was a problem hiding this comment.
🔴 CLI assistant contacts upsert command broken: IPC method upsert_contact removed but CLI caller not updated
The upsert_contact route was removed from the shared ROUTES array in contact-routes.ts, which means the daemon's IPC server no longer registers a handler for this method. However, the CLI command assistant contacts upsert at assistant/src/cli/commands/contacts.ts:450 still calls cliIpcCall("upsert_contact", {...}). At runtime, the AssistantIpcServer (assistant/src/ipc/assistant-server.ts:326-332) will fail to find the method in its map and return "Unknown method: upsert_contact", making the CLI subcommand completely non-functional. The CLI command definition (lines 380–474 of contacts.ts) should either be removed or updated to route through the gateway.
(Refers to lines 480-498)
Prompt for agents
The upsert_contact route was removed from the ROUTES array in contact-routes.ts, but the CLI still has an `assistant contacts upsert` subcommand (assistant/src/cli/commands/contacts.ts lines 380-474) that calls cliIpcCall("upsert_contact", ...). Since the IPC server builds its method map from ROUTES (via routeDefinitionsToIpcMethods in assistant/src/ipc/routes/route-adapter.ts), this method no longer exists and the CLI command will fail with "Unknown method: upsert_contact". Either: (1) remove the CLI `upsert` subcommand entirely if contact writes are truly guardian-only via the web UI now, or (2) keep it working by re-routing through the gateway or adding an IPC-only route. The AGENTS.md Dead Code Removal rule also requires removing the now-orphaned CLI command code if it's no longer functional.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed and addressed in #30281, taking option (1) from your suggestion ("remove the CLI upsert subcommand entirely if contact writes are truly guardian-only via the web UI now"). The follow-up deletes the Commander block in assistant/src/cli/commands/contacts.ts, drops the now-stale contacts upsert entries from gateway/src/risk/command-registry/commands/assistant.ts, and rewrites the parent contacts command help text to spell out the guardian-only policy. Good catch — the IPC method map would have rejected the call as you described.
…mmand (#30281) Follow-up to #30278 (which removed the daemon `upsert_contact` IPC route + LLM-facing skill tool). Per the bots, the CLI subcommand `assistant contacts upsert` was left in place and now fails at runtime with "Unknown method: upsert_contact" because the IPC route it called no longer exists. Per the architectural decision encoded in #30278: **contact writes are guardian-only and happen through the assistant web UI Contacts tab or the invite flow.** There is no command-line write path either. Changes: * Delete the `contacts.command("upsert")` block in `assistant/src/cli/commands/contacts.ts` (~99 lines including the Commander definition, --channels JSON parsing, the now-dead `cliIpcCall("upsert_contact", ...)`, and the example-bearing addHelpText block). * Rewrite the top-level `contacts` command's help text to spell out the guardian-only policy, drop the `upsert` example, and keep the remaining list/get/merge/invites examples. * Drop `contacts upsert` from `gateway/src/risk/command-registry/commands/assistant.ts` (both the allowlist and the medium-risk classification entry). No tests asserted the CLI `upsert` subcommand directly. Internal `upsertContact` callers in the store, invite-redemption, attachment metadata processing, and ~10 test fixtures are unchanged \u2014 those are not LLM- or CLI-exposed. Addresses: * chatgpt-codex-connector bot P1 comment on #30278 ("Keep upsert_contact IPC method available for CLI" \u2014 chose the other option: remove the orphan CLI command since contact writes are guardian-only). * devin-ai-integration bot BUG comment on #30278 ("CLI `assistant contacts upsert` command broken"). Co-authored-by: vellum-apollo-bot[bot] <242025090+vellum-apollo-bot[bot]@users.noreply.github.com>
Follow-up to #30141 (Track A: gateway-native contact upsert dual-write).
Now that the gateway owns contact writes natively, the daemon-side
POST /v1/contactsroute and the LLM-facingcontact_upsertskill tool are dead weight — and the LLM having a direct contact-write path contradicts the architectural decision that contact writes are guardian-only.What changes
upsert_contactroute fromruntime/routes/contact-routes.ts(handler, helper, species validation, route entry). The gateway is now the sole HTTP write surface for contacts;POST /v1/contactsis served by the gateway control-plane route which has its ownvalidateSpeciesMetadatacopy.contact_upsertskill tool (config/bundled-skills/contacts/tools/contact-upsert.ts), its entry inTOOLS.json, and its registration inbundled-tool-registry.ts.contactsSKILL.md to encode the guardian-only policy:assistant contacts invites createfor an existing contact.assistant contacts list" and stop telling the LLM to fall back toassistant contacts upsert— instead they tell it to walk the user to the web UI if the contact doesn't exist.upsertAssistantContactMetadataandvalidateSpeciesMetadataexports fromcontact-store.ts.upsertContactitself is kept — it's still the underlying write path used by the gateway dual-write, theassistant contacts upsertCLI (guardian-only, host-side, requires shell access), invite redemption, attachment metadata processing, and test fixtures.__tests__/contacts-tools.test.ts:executeContactUpsert(...)calls with a tinyupsertFixture()helper that goes straight to the store.contact_upsert tooldescribe block entirely.POST /v1/contactshandler from the test server stub.What remains available to the LLM
contact_search,contact_merge,google_contacts(unchanged)assistant contacts listWhat does NOT change
assistant contacts upsertCLI command — still exists, still guardian-only (host shell).upsertContact(invite redemption, attachment-metadata enrichment, CLI, ~10 test fixtures).POST /v1/contactsroute + dual-write (lives ingateway/src/http/routes/contacts-control-plane-proxy.ts).Diff
Verification
bun run tsc --noEmit(assistant/) clean — only pre-existing unrelatedpackages/service-contractszod errors remain.bun test src/__tests__/contacts-tools.test.ts— 10/10 pass.bun test src/__tests__/contacts-write.test.ts src/__tests__/contact-store-user-file.test.ts— 18/18 pass.Next
Track B (invite-redemption gateway-native) is the next item in the Gateway Security Migration workstream after this lands.