chore(assistant): trim contacts CLI per #30281 review (drop guardian-only help paragraph + remove contacts merge)#30301
Conversation
There was a problem hiding this comment.
Devin Review found 2 potential issues.
⚠️ 2 issues in files not directly in the diff
⚠️ Shell completions still offer removed get subcommand for contacts (assistant/src/cli/commands/completions.ts:45)
The contacts get command was removed in this PR, but assistant/src/cli/commands/completions.ts:45 still lists "get" in the contacts completions array. Users who Tab-complete assistant contacts will see get as a valid option, but the command no longer exists. This violates the Deprecation Hygiene rule in assistant/src/cli/AGENTS.md which requires updating all string references to removed commands.
⚠️ COMMAND_INVENTORY.md still lists removed get subcommand and getContact operation ID (assistant/src/cli/COMMAND_INVENTORY.md:55)
The contacts get command was removed in this PR, but assistant/src/cli/COMMAND_INVENTORY.md:55 still lists get in the Subcommands column and getContact in the Operation IDs column for the contacts row. This violates the Deprecation Hygiene rule in assistant/src/cli/AGENTS.md: "Update references — search for string references to the old command in docs, skills, fixtures, help text, and comments. Update or remove them." The COMMAND_INVENTORY.md is CI-validated by check-cli-inventory.ts, so this stale entry may also cause CI failures.
View 2 additional findings in Devin Review.
…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.
contacts get)contacts merge)
c332ea1 to
ed84b68
Compare
Follow-up to #30281 addressing your two review threads.
Review threads addressed
assistant/src/cli/commands/contacts.ts:176— "Delete this comment"Drops the four-line
Contact writes are guardian-only and happen through the assistant web UI / Contacts tab / invite flow ...paragraph I had added to the parentcontactscommand's help text. Help stays terse; the available subcommands speak for themselves.assistant/src/cli/commands/contacts.ts:181— "Delete this cli next" (pointing at$ assistant contacts get abc-123)Removes the
assistant contacts get <id>CLI subcommand entirely (~50 lines: Commander definition, addHelpText, thecliIpcCall("getContact", ...)action). Continuing the trim of guardian-only contact CLI surface —list+ the LLM-sidecontact_searchalready cover ID-resolution needs.Changes
assistant/src/cli/commands/contacts.ts:$ assistant contacts get abc-123example line from the parentcontactsaddHelpTextblock.contacts.command("get <id>")subcommand.channelIdargument note in thechannels update-statushelp text to point atassistant contacts list --jsoninstead of the (now removed)assistant contacts get <contactId>.gateway/src/risk/command-registry/commands/assistant.ts:contacts getfrom the allowlist. No risk-classification entry existed (reads weren't classified medium-risk), so this is the only registry change.What does NOT change
getContactcallers in the store and HTTP routes (gateway + assistant inbox views, contact-prompt routing, etc.) — those are not CLI-exposed.contact_search/contact_merge/google_contacts(unchanged).contacts list,contacts merge,contacts prompt,contacts channels …,contacts invites …CLI subcommands (unchanged).Diff
Verification
bun run lint+bun run typecheckclean on bothassistant/andgateway/. CLI inventory check passes (44 commands ↔ 44 inventory rows).bun test contacts-tools.test.ts contacts-write.test.ts→ 11/11 pass.bun test bash-risk-classifier.test.ts→ 10/10 pass.grep "contacts get"clean across the repo afterwards.