fix: address peqy review of v1.2.0 (v1.2.1)

CHANGELOG.md

@@ -6,6 +6,21 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 
 ## [Unreleased]
 
+## [1.2.1] - 2026-07-08
+
+Follow-ups from code review of the v1.2.0 changes.
+
+### Fixed
+
+- **`deals set-role` no longer sends an explicit `null` role type.** Adding a contact without `--role-type` omitted the `opportunity_contact_role_type_id` key entirely instead of posting `null`, which Apollo may reject.
+- **`people search` no longer drops results.** When Apollo returns both `people` and `contacts` (matched CRM records), both are now shown — previously only the first non-empty list was kept.
+- **Conversation detail view is robust to raw dicts.** The participants/deals/summary/transcript sections read fields via a dict-or-model helper, so they render whether `conversations get` returns models or plain dicts (previously the sections silently rendered empty for dicts).
+- **`--stage-name` errors are bounded.** An unknown stage name lists at most 15 available names (`… (+N more)`) instead of dumping the entire list.
+
+### Note
+
+- `conversations search --query` maps to Apollo's universal `q_keywords` param. The conversations search endpoint is undocumented and keyword filtering hasn't been confirmed server-side; if a search returns everything unfiltered, that param is the thing to revisit.
+
 ## [1.2.0] - 2026-07-08
 
 Usage-driven UX improvements — every item here closes a gap where users had been dropping to raw curl or doing extra lookups.

pyproject.toml

@@ -1,6 +1,6 @@
 [project]
 name = "qodev-apollo-cli"
-version = "1.2.0"
+version = "1.2.1"
 description = "Agent-friendly CLI for the Apollo API"
 readme = "README.md"
 requires-python = ">=3.11"
@@ -20,7 +20,7 @@ classifiers = [
 dependencies = [
     "cyclopts>=3.0",
     "rich>=13.0",
-    "qodev-apollo-api>=0.3.0",
+    "qodev-apollo-api>=0.3.1",
 ]
 
 [project.optional-dependencies]

src/apollo_cli/commands/conversations.py

@@ -25,6 +25,10 @@ async def search(
     """Search recorded conversations."""
     filters: dict = {}
     if query:
+        # `q_keywords` is Apollo's universal keyword-search param (contacts/accounts/deals/
+        # people all use it). The conversations/search endpoint is undocumented and we
+        # haven't confirmed it honours the filter server-side — if a search returns
+        # everything unfiltered, this is the param to revisit.
         filters["q_keywords"] = query
 
     async with ctx.client() as client:

src/apollo_cli/commands/deals.py

@@ -2,9 +2,10 @@
 
 from __future__ import annotations
 
-from typing import Annotated, Any
+from typing import Annotated, Any, cast
 
 from cyclopts import App, Parameter
+from qodev_apollo_api import RoleAssignment
 
 from apollo_cli.context import ctx
 from apollo_cli.formatters.deals import format_deal_detail, format_deal_list
@@ -86,19 +87,26 @@ async def role_types() -> None:
 
 
 def _existing_roles(deal: Any) -> list[dict]:
-    """Flatten a deal's current opportunity_contact_roles into update_roles entries."""
+    """Flatten a deal's current opportunity_contact_roles into update_roles entries.
+
+    ``opportunity_contact_role_type_id`` is only included when the existing role
+    actually has one — we never send an explicit ``null`` (see ``_clean_roles``).
+    """
     roles: list[dict] = []
     for r in getattr(deal, "opportunity_contact_roles", []) or []:
-        role_type_id = None
-        if r.role:
-            role_type_id = r.role[0].opportunity_contact_role_type_id
-        roles.append(
-            {
-                "contact_id": r.contact_id,
-                "opportunity_contact_role_type_id": role_type_id,
-                "is_primary": bool(r.is_primary),
-            }
-        )
+        entry: dict = {"contact_id": r.contact_id, "is_primary": bool(r.is_primary)}
+        if r.role and r.role[0].opportunity_contact_role_type_id:
+            entry["opportunity_contact_role_type_id"] = r.role[0].opportunity_contact_role_type_id
+        roles.append(entry)
+    return roles
+
+
+def _clean_roles(roles: list[dict]) -> list[dict]:
+    """Drop any ``opportunity_contact_role_type_id`` that is ``None`` so we never POST an
+    explicit null (Apollo may reject roles without a role type — omit the key instead)."""
+    for r in roles:
+        if r.get("opportunity_contact_role_type_id") is None:
+            r.pop("opportunity_contact_role_type_id", None)
     return roles
 
 
@@ -148,14 +156,16 @@ async def set_role(
 
         entry = next((r for r in roles if r["contact_id"] == contact_id), None)
         if entry is None:
-            entry = {"contact_id": contact_id, "opportunity_contact_role_type_id": None, "is_primary": False}
+            entry = {"contact_id": contact_id, "is_primary": False}
             roles.append(entry)
         if role_type_id is not None:
             entry["opportunity_contact_role_type_id"] = role_type_id
         if primary:
             for r in roles:
                 r["is_primary"] = r["contact_id"] == contact_id
 
-        updated = await client.update_opportunity_roles(id, roles)
+        # Entries are built dynamically (conditional keys, pop), so they're plain dicts;
+        # cast to the client's RoleAssignment TypedDict at the boundary.
+        updated = await client.update_opportunity_roles(id, cast("list[RoleAssignment]", _clean_roles(roles)))
 
     output(updated, ctx=ctx, format_fn=format_deal_detail)

src/apollo_cli/commands/people.py

@@ -51,9 +51,9 @@ async def search(
     async with ctx.client() as client:
         result = await client.search_people(**filters)
 
-    # search_people returns the raw Apollo dict; people live under "people" (and
-    # sometimes "contacts" for matched CRM records).
-    items = result.get("people") or result.get("contacts") or []
+    # search_people returns the raw Apollo dict. Results live under "people"; matched
+    # CRM records come back under "contacts". Merge both so we never silently drop half.
+    items = [*(result.get("people") or []), *(result.get("contacts") or [])]
     pagination = result.get("pagination", {})
     total = pagination.get("total_entries", len(items))
 

src/apollo_cli/formatters/conversations.py

@@ -6,6 +6,13 @@
 
 from apollo_cli.formatters.generic import detail_table, list_table
 
+
+def _get(item: Any, key: str) -> Any:
+    """Read ``key`` from a Pydantic model (attr) or a dict — so the rich detail view
+    renders whether ``get_conversation()`` returns models or raw dicts."""
+    return item.get(key) if isinstance(item, dict) else getattr(item, key, None)
+
+
 CONVERSATION_LIST_COLUMNS = [
     ("ID", "id"),
     ("Topic", "topic"),
@@ -42,63 +49,63 @@ def format_conversation_list(items: list[Any], *, total: int = 0, page: int = 1)
 
 def format_conversation_detail(data: Any) -> str:
     """Format a single conversation (with transcript & summary) as a markdown detail view."""
-    topic = getattr(data, "topic", None) or "Conversation"
+    topic = _get(data, "topic") or "Conversation"
     md = detail_table(data, CONVERSATION_DETAIL_FIELDS, title=f"Conversation: {topic}")
 
     # Participants (richer than the participant_names list in the metadata table)
-    participants = getattr(data, "participants_info", None) or []
+    participants = _get(data, "participants_info") or []
     if participants:
         md += "\n\n## Participants\n"
         for p in participants:
-            name = getattr(p, "name", None) or "Unknown"
-            title = getattr(p, "title", None)
-            account = getattr(p, "account_name", None)
-            internal = getattr(p, "is_internal_participant", None)
+            name = _get(p, "name") or "Unknown"
+            title = _get(p, "title")
+            account = _get(p, "account_name")
+            internal = _get(p, "is_internal_participant")
             suffix = ", ".join(x for x in [title, account] if x)
             tag = " (internal)" if internal else ""
             md += f"\n- **{name}**{tag}" + (f" — {suffix}" if suffix else "")
 
     # Associated deals
-    deals = getattr(data, "deals", None) or []
+    deals = _get(data, "deals") or []
     if deals:
         md += "\n\n## Deals\n"
         for d in deals:
-            name = getattr(d, "name", None) or getattr(d, "id", "Unknown")
-            account = getattr(d, "account_name", None)
+            name = _get(d, "name") or _get(d, "id") or "Unknown"
+            account = _get(d, "account_name")
             md += f"\n- {name}" + (f" ({account})" if account else "")
 
     # AI-generated call summary (detail endpoint only)
-    summary = getattr(data, "call_summary", None)
+    summary = _get(data, "call_summary")
     if summary:
         md += _format_summary(summary)
 
     # Transcript (detail endpoint only)
-    if getattr(data, "transcript", None):
+    if _get(data, "transcript"):
         md += "\n\n" + format_transcript(data)
 
     return md
 
 
 def format_transcript(data: Any) -> str:
     """Render just the transcript of a conversation as `**Speaker:** sentence` lines."""
-    segments = getattr(data, "transcript", None) or []
+    segments = _get(data, "transcript") or []
     if not segments:
         return "## Transcript\n\n_No transcript available._"
     lines = ["## Transcript", ""]
     for seg in segments:
-        speaker = getattr(seg, "participant_name", None) or "Unknown"
-        sentence = getattr(seg, "spoken_sentence", None) or ""
+        speaker = _get(seg, "participant_name") or "Unknown"
+        sentence = _get(seg, "spoken_sentence") or ""
         lines.append(f"- **{speaker}:** {sentence}")
     return "\n".join(lines)
 
 
 def _format_summary(summary: Any) -> str:
     """Render the AI call summary (outcome, pain points, objections, next steps)."""
     md = "\n\n## Call Summary\n"
-    outcome = getattr(summary, "outcome", None)
+    outcome = _get(summary, "outcome")
     if outcome:
         md += f"\n**Outcome:** {outcome}\n"
-    pricing = getattr(summary, "pricing_discussion", None)
+    pricing = _get(summary, "pricing_discussion")
     if pricing:
         md += f"\n**Pricing discussion:** {pricing}\n"
 
@@ -107,11 +114,11 @@ def _format_summary(summary: Any) -> str:
         ("Objections", "objections", "text"),
         ("Next Steps", "next_steps", "step"),
     ):
-        items = getattr(summary, attr, None) or []
+        items = _get(summary, attr) or []
         if items:
             md += f"\n### {label}\n"
             for item in items:
-                text = getattr(item, field, None) or ""
-                who = getattr(item, "participant_name", None)
+                text = _get(item, field) or ""
+                who = _get(item, "participant_name")
                 md += f"\n- {text}" + (f" — _{who}_" if who else "")
     return md

src/apollo_cli/util.py

@@ -20,11 +20,20 @@ def resolve_stage_id(name: str, stages: list[Any], *, kind: str = "stage") -> st
     target = name.strip().lower()
     match = next((s for s in stages if (_field(s, "name") or "").lower() == target), None)
     if match is None:
-        available = ", ".join(sorted(n for s in stages if (n := _field(s, "name"))))
-        raise ValueError(f"No {kind} named {name!r}. Available: {available or '(none)'}")
+        names = sorted(n for s in stages if (n := _field(s, "name")))
+        raise ValueError(f"No {kind} named {name!r}. Available: {_preview(names)}")
     return _field(match, "id")
 
 
+def _preview(names: list[str], limit: int = 15) -> str:
+    """Render a name list for an error message, capped so it can't get huge."""
+    if not names:
+        return "(none)"
+    if len(names) <= limit:
+        return ", ".join(names)
+    return f"{', '.join(names[:limit])}, … (+{len(names) - limit} more)"
+
+
 def parse_comma_list(raw: str) -> list[str]:
     """Parse a comma-separated CLI argument into a list of stripped, non-empty tokens.
 

tests/test_commands.py

@@ -375,6 +375,28 @@ async def test_people_search_domain_and_pagination(self, capsys) -> None:
         assert data["items"][0]["name"] == "Jane"
         assert data["total"] == 1
 
+    @pytest.mark.asyncio
+    async def test_people_search_merges_people_and_contacts(self, capsys) -> None:
+        """When Apollo returns both `people` and `contacts`, neither half is dropped."""
+        mock_client = MagicMock()
+        mock_client.search_people = AsyncMock(
+            return_value={
+                "people": [{"id": "p1", "name": "Jane"}],
+                "contacts": [{"id": "c1", "name": "John"}],
+                "pagination": {"total_entries": 2},
+            }
+        )
+
+        _ctx.ctx.configure(json_mode=True, api_key="test-key", limit=25, page=1)
+
+        with patch.object(_ctx.ctx, "client", return_value=MockAsyncContextManager(mock_client)):
+            from apollo_cli.commands.people import search
+
+            await search(keywords="jane")
+
+        data = json.loads(capsys.readouterr().out)
+        assert {p["name"] for p in data["items"]} == {"Jane", "John"}
+
 
 class TestStageNameFilter:
     @pytest.mark.asyncio
@@ -454,6 +476,30 @@ async def test_set_role_read_modify_write_primary(self, sample_deal: dict, capsy
         assert by_contact["c-new"]["is_primary"] is True
         assert by_contact["c-old"]["is_primary"] is False  # demoted
         assert by_contact["c-old"]["opportunity_contact_role_type_id"] == "rt-x"  # preserved
+        # New contact without --role-type: the key is omitted, never sent as an explicit null.
+        assert "opportunity_contact_role_type_id" not in by_contact["c-new"]
+
+    @pytest.mark.asyncio
+    async def test_set_role_never_sends_null_role_type(self, capsys) -> None:
+        """An existing role stored without a role type is sent without the key (no null)."""
+        from qodev_apollo_api.models import Deal
+
+        deal = Deal.model_validate(
+            {"id": "d1", "opportunity_contact_roles": [{"id": "r1", "contact_id": "c-old", "is_primary": True}]}
+        )
+        mock_client = MagicMock()
+        mock_client.get_deal = AsyncMock(return_value=deal)
+        mock_client.update_opportunity_roles = AsyncMock(return_value=deal)
+
+        _ctx.ctx.configure(json_mode=True, api_key="test-key", limit=25, page=1)
+
+        with patch.object(_ctx.ctx, "client", return_value=MockAsyncContextManager(mock_client)):
+            from apollo_cli.commands.deals import set_role
+
+            await set_role("d1", contact_id="c-new")
+
+        _, roles = mock_client.update_opportunity_roles.call_args.args
+        assert all("opportunity_contact_role_type_id" not in r for r in roles)
 
 
 class TestCustomFieldsCommand:

tests/test_conversations_formatter.py

@@ -68,3 +68,21 @@ def test_detail_formatter_minimal_conversation() -> None:
     assert "Conversation: Quick sync" in md
     assert "## Transcript" not in md
     assert "## Call Summary" not in md
+
+
+def test_detail_formatter_handles_raw_dict() -> None:
+    """The rich detail view renders from a raw dict too, not only Pydantic models."""
+    data = {
+        "id": "c1",
+        "topic": "Dict sync",
+        "participants_info": [{"name": "Jane", "title": "VP", "is_internal_participant": True}],
+        "deals": [{"id": "d1", "name": "Enterprise Deal", "account_name": "Acme"}],
+        "call_summary": {"outcome": "Good", "next_steps": [{"step": "Follow up"}]},
+        "transcript": [{"participant_name": "Jane", "spoken_sentence": "Hello."}],
+    }
+    md = format_conversation_detail(data)
+    assert "Conversation: Dict sync" in md
+    assert "## Participants" in md and "Jane" in md and "(internal)" in md
+    assert "## Deals" in md and "Enterprise Deal" in md
+    assert "## Call Summary" in md and "Good" in md and "Follow up" in md
+    assert "## Transcript" in md and "**Jane:** Hello." in md

tests/test_util.py

@@ -4,7 +4,7 @@
 
 import pytest
 
-from apollo_cli.util import parse_comma_list
+from apollo_cli.util import parse_comma_list, resolve_stage_id
 
 
 class TestParseCommaList:
@@ -31,3 +31,26 @@ def test_raises_when_input_has_content_but_no_usable_tokens(self, raw: str) -> N
         input (tested above) is fine because it maps cleanly to "flag not provided"."""
         with pytest.raises(ValueError, match="only separators"):
             parse_comma_list(raw)
+
+
+class TestResolveStageId:
+    def test_matches_case_insensitively(self) -> None:
+        stages = [{"id": "s1", "name": "Negotiation"}, {"id": "s2", "name": "Won"}]
+        assert resolve_stage_id("negotiation", stages) == "s1"
+
+    def test_works_with_model_like_objects(self) -> None:
+        class S:
+            def __init__(self, id, name):
+                self.id, self.name = id, name
+
+        assert resolve_stage_id("Won", [S("s1", "Neg"), S("s2", "Won")]) == "s2"
+
+    def test_unknown_name_lists_available(self) -> None:
+        stages = [{"id": "s1", "name": "Won"}, {"id": "s2", "name": "Lost"}]
+        with pytest.raises(ValueError, match=r"No stage named 'Nope'\. Available: Lost, Won"):
+            resolve_stage_id("Nope", stages)
+
+    def test_available_list_is_capped(self) -> None:
+        stages = [{"id": str(i), "name": f"Stage{i:02d}"} for i in range(30)]
+        with pytest.raises(ValueError, match=r"\(\+15 more\)"):
+            resolve_stage_id("missing", stages)