Skip to content

chore(assistant): remove broken assistant contacts upsert CLI subcommand (follow-up to #30278)#30281

Merged
dvargasfuertes merged 1 commit into
mainfrom
apollo/atl-311-remove-contacts-upsert-cli
May 11, 2026
Merged

chore(assistant): remove broken assistant contacts upsert CLI subcommand (follow-up to #30278)#30281
dvargasfuertes merged 1 commit into
mainfrom
apollo/atl-311-remove-contacts-upsert-cli

Conversation

@vellum-apollo-bot
Copy link
Copy Markdown
Contributor

@vellum-apollo-bot vellum-apollo-bot Bot commented May 11, 2026

Follow-up to #30278 (which removed the daemon upsert_contact IPC route + LLM-facing skill tool).

Per the bots on #30278, 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. Both codex and Devin flagged it.

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 — so the right move is to remove the orphan CLI command, not to keep an IPC-only route alive.

Changes

  • Delete the contacts.command("upsert") block in assistant/src/cli/commands/contacts.ts (~99 lines: 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).

What does NOT change

  • Internal upsertContact callers in the store, invite redemption, attachment metadata processing, and ~10 test fixtures — those are not LLM- or CLI-exposed and remain the underlying write path used by the gateway dual-write.
  • Gateway-side POST /v1/contacts (lives in gateway/src/http/routes/contacts-control-plane-proxy.ts) — the canonical HTTP write surface.

Diff

 assistant/src/cli/commands/contacts.ts             | 106 +--------------------
 gateway/src/risk/command-registry/commands/assistant.ts |   4 +-
 2 files changed, 6 insertions(+), 104 deletions(-)

Verification

  • bun run lint + bun run typecheck clean on both assistant/ and gateway/.
  • bun test contacts-tools.test.ts contacts-write.test.ts → 11/11 pass.
  • bun test gateway/.../bash-risk-classifier.test.ts → 10/10 pass.
  • No tests asserted the CLI upsert subcommand directly (grep clean).

Closes

Track B (invite-redemption gateway-native) is the next item in the Gateway Security Migration workstream.


Open in Devin Review

…mmand

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").
@linear
Copy link
Copy Markdown

linear Bot commented May 11, 2026

ATL-311

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 0 potential issues.

Open in Devin Review

Comment on lines +173 to +176
Contact writes are guardian-only and happen through the assistant web UI
(Contacts tab) or the invite flow — there is no CLI command for creating
or updating contacts. This CLI exposes read, merge, and invite/channel
management only.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete this comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #30301 — dropped the four-line guardian-only paragraph. Help text is back to just the architectural blurb + Examples.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated #30301 — guardian-only paragraph still gone, that part is unchanged.

$ assistant contacts list
$ assistant contacts get abc-123
$ assistant contacts upsert --display-name "Alice"
$ assistant contacts merge keep-id merge-id
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete this cli next

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #30301 — removed the full contacts.command("get <id>") block (~50 lines), the example line from the parent help, and dropped contacts get from the gateway risk registry allowlist. Also updated the channels update-status arg help to point at assistant contacts list --json for finding channel IDs instead of the (now removed) assistant contacts get <contactId>.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction — you clarified you want to keep get and drop merge (I had inverted the read). #30301 force-pushed to commit ed84b68ea8 with the swap: get restored, merge subcommand removed, contacts merge dropped from gateway risk allowlist + medium-risk classification. LLM-side contact_merge bundled tool + the merge_contacts IPC route are unchanged (still available to the LLM). PR title updated to match.

@dvargasfuertes dvargasfuertes merged commit fde3911 into main May 11, 2026
14 checks passed
@dvargasfuertes dvargasfuertes deleted the apollo/atl-311-remove-contacts-upsert-cli branch May 11, 2026 18:59
vellum-apollo-bot Bot added a commit that referenced this pull request May 12, 2026
…only help paragraph + remove `contacts merge`)

Addresses Vargas's two review threads on #30281:

* line 173-176 "Delete this comment" — drops the four-line
  "Contact writes are guardian-only and happen through the assistant
  web UI..." paragraph from the parent contacts command's help text.
  Help stays terse; available subcommands speak for themselves.
* line 181 "Delete this cli next" — clarified in follow-up that the
  CLI to remove next is `merge` (not `get`); `get` is the read-only
  detail view and stays. `merge` is a destructive write of one
  contact into another; that operation belongs in the guardian web
  UI alongside the rest of the contact-management flows.

Changes:

* `assistant/src/cli/commands/contacts.ts` —
  - drop the explanatory paragraph + `$ assistant contacts merge keep-id merge-id`
    example from the parent `contacts` help text
  - delete the entire `contacts.command("merge <keepId> <mergeId>")` block
    (~57 lines: Commander definition, addHelpText, action with
    cliIpcCall("merge_contacts", ...))
* `gateway/src/risk/command-registry/commands/assistant.ts` —
  drop `contacts merge` from both the allowlist and the medium-risk
  classification list.

What does NOT change:

* LLM-facing skill tool `contact_merge` (bundled under
  `config/bundled-skills/contacts/tools/contact-merge.ts`) remains
  available to the LLM. It calls `cliIpcCall("merge_contacts", ...)`,
  which is still served by `runtime/routes/contact-routes.ts`.
* CLI `get`, `list`, `prompt`, `channels …`, `invites …`
  subcommands all unchanged.

Verification:

* `bun run lint` + `bun run typecheck` clean on both `assistant/`
  and `gateway/`. CLI inventory check passes.
* `bun test contacts-tools.test.ts contacts-write.test.ts` — 11/11
  pass.
* `bun test bash-risk-classifier.test.ts` — 10/10 pass.
dvargasfuertes pushed a commit that referenced this pull request May 12, 2026
…only help paragraph + remove `contacts merge`) (#30301)

Addresses Vargas's two review threads on #30281:

* line 173-176 "Delete this comment" — drops the four-line
  "Contact writes are guardian-only and happen through the assistant
  web UI..." paragraph from the parent contacts command's help text.
  Help stays terse; available subcommands speak for themselves.
* line 181 "Delete this cli next" — clarified in follow-up that the
  CLI to remove next is `merge` (not `get`); `get` is the read-only
  detail view and stays. `merge` is a destructive write of one
  contact into another; that operation belongs in the guardian web
  UI alongside the rest of the contact-management flows.

Changes:

* `assistant/src/cli/commands/contacts.ts` —
  - drop the explanatory paragraph + `$ assistant contacts merge keep-id merge-id`
    example from the parent `contacts` help text
  - delete the entire `contacts.command("merge <keepId> <mergeId>")` block
    (~57 lines: Commander definition, addHelpText, action with
    cliIpcCall("merge_contacts", ...))
* `gateway/src/risk/command-registry/commands/assistant.ts` —
  drop `contacts merge` from both the allowlist and the medium-risk
  classification list.

What does NOT change:

* LLM-facing skill tool `contact_merge` (bundled under
  `config/bundled-skills/contacts/tools/contact-merge.ts`) remains
  available to the LLM. It calls `cliIpcCall("merge_contacts", ...)`,
  which is still served by `runtime/routes/contact-routes.ts`.
* CLI `get`, `list`, `prompt`, `channels …`, `invites …`
  subcommands all unchanged.

Verification:

* `bun run lint` + `bun run typecheck` clean on both `assistant/`
  and `gateway/`. CLI inventory check passes.
* `bun test contacts-tools.test.ts contacts-write.test.ts` — 11/11
  pass.
* `bun test bash-risk-classifier.test.ts` — 10/10 pass.

Co-authored-by: vellum-apollo-bot[bot] <242025090+vellum-apollo-bot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant