From b6a399215a80dee173a720972d52d9edb5b760e2 Mon Sep 17 00:00:00 2001 From: Jan Scheffler Date: Wed, 8 Jul 2026 14:31:40 +0200 Subject: [PATCH 1/2] fix: address peqy review of v1.2.0 (v1.2.1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - deals set-role: never POST an explicit null role type — omit opportunity_contact_role_type_id when unset (Apollo may reject null). - people search: merge `people` + `contacts` result lists instead of keeping only the first non-empty one (no silent drop). - conversations detail formatter: read fields via a dict-or-model helper so the rich sections render for raw dicts too, not only models. - resolve_stage_id: cap the "Available:" list at 15 names (+N more) so a large stage list can't produce a huge error message. - Document that conversations `--query` -> q_keywords is unverified for the (undocumented) conversations/search endpoint. Tests added for each. Bumps to 1.2.1. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 15 +++++++ pyproject.toml | 2 +- src/apollo_cli/commands/conversations.py | 4 ++ src/apollo_cli/commands/deals.py | 33 ++++++++++------ src/apollo_cli/commands/people.py | 6 +-- src/apollo_cli/formatters/conversations.py | 45 ++++++++++++--------- src/apollo_cli/util.py | 13 +++++- tests/test_commands.py | 46 ++++++++++++++++++++++ tests/test_conversations_formatter.py | 18 +++++++++ tests/test_util.py | 25 +++++++++++- uv.lock | 4 +- 11 files changed, 170 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b8d0e7..e1195ef 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/pyproject.toml b/pyproject.toml index 260d395..6d81c96 100644 --- a/pyproject.toml +++ b/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" diff --git a/src/apollo_cli/commands/conversations.py b/src/apollo_cli/commands/conversations.py index 8991405..0d2f0bd 100644 --- a/src/apollo_cli/commands/conversations.py +++ b/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: diff --git a/src/apollo_cli/commands/deals.py b/src/apollo_cli/commands/deals.py index 8357907..6b55b66 100644 --- a/src/apollo_cli/commands/deals.py +++ b/src/apollo_cli/commands/deals.py @@ -86,19 +86,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,7 +155,7 @@ 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 @@ -156,6 +163,6 @@ async def set_role( for r in roles: r["is_primary"] = r["contact_id"] == contact_id - updated = await client.update_opportunity_roles(id, roles) + updated = await client.update_opportunity_roles(id, _clean_roles(roles)) output(updated, ctx=ctx, format_fn=format_deal_detail) diff --git a/src/apollo_cli/commands/people.py b/src/apollo_cli/commands/people.py index 0b8d558..21e0f29 100644 --- a/src/apollo_cli/commands/people.py +++ b/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)) diff --git a/src/apollo_cli/formatters/conversations.py b/src/apollo_cli/formatters/conversations.py index bf88320..096f444 100644 --- a/src/apollo_cli/formatters/conversations.py +++ b/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,38 +49,38 @@ 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 @@ -81,13 +88,13 @@ def format_conversation_detail(data: Any) -> str: 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) @@ -95,10 +102,10 @@ def format_transcript(data: Any) -> str: 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 diff --git a/src/apollo_cli/util.py b/src/apollo_cli/util.py index c3ced1a..0a7b5c8 100644 --- a/src/apollo_cli/util.py +++ b/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. diff --git a/tests/test_commands.py b/tests/test_commands.py index 3c5e3d5..945b2c6 100644 --- a/tests/test_commands.py +++ b/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: diff --git a/tests/test_conversations_formatter.py b/tests/test_conversations_formatter.py index 01a12a9..157e886 100644 --- a/tests/test_conversations_formatter.py +++ b/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 diff --git a/tests/test_util.py b/tests/test_util.py index 188fdda..a0b6003 100644 --- a/tests/test_util.py +++ b/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) diff --git a/uv.lock b/uv.lock index 1665eee..06fe99c 100644 --- a/uv.lock +++ b/uv.lock @@ -472,7 +472,7 @@ wheels = [ [[package]] name = "qodev-apollo-api" -version = "0.3.0" +version = "0.3.2" source = { directory = "../apollo-api" } dependencies = [ { name = "httpx" }, @@ -494,7 +494,7 @@ provides-extras = ["dev"] [[package]] name = "qodev-apollo-cli" -version = "1.2.0" +version = "1.2.1" source = { editable = "." } dependencies = [ { name = "cyclopts" }, From 2c2e6ed57f37144090e853027daa681380775318 Mon Sep 17 00:00:00 2001 From: Jan Scheffler Date: Wed, 8 Jul 2026 14:58:13 +0200 Subject: [PATCH 2/2] fix: satisfy RoleAssignment-typed update_opportunity_roles signature MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit apollo-api 0.3.1 typed update_opportunity_roles' payload as list[RoleAssignment]; set-role builds entries dynamically (conditional keys + pop) so they're plain dicts — cast at the boundary and require qodev-apollo-api>=0.3.1. (Local mypy had missed this against a 0.3.0 venv.) Co-Authored-By: Claude Opus 4.8 (1M context) --- pyproject.toml | 2 +- src/apollo_cli/commands/deals.py | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 6d81c96..f367f05 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -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] diff --git a/src/apollo_cli/commands/deals.py b/src/apollo_cli/commands/deals.py index 6b55b66..bf60764 100644 --- a/src/apollo_cli/commands/deals.py +++ b/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 @@ -163,6 +164,8 @@ async def set_role( for r in roles: r["is_primary"] = r["contact_id"] == contact_id - updated = await client.update_opportunity_roles(id, _clean_roles(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)