Skip to content

feat(nodes): add tool_pipedrive CRM tool node - #1687

Open
joshuadarron wants to merge 14 commits into
developfrom
feat/RR-1675-tool-pipedrive
Open

feat(nodes): add tool_pipedrive CRM tool node#1687
joshuadarron wants to merge 14 commits into
developfrom
feat/RR-1675-tool-pipedrive

Conversation

@joshuadarron

@joshuadarron joshuadarron commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

Exposes the Pipedrive REST API v1 to agents: 255 tools across 24 resource groups (deals, persons, organizations, activities, pipelines, stages, notes, leads, products, fields, files, users, roles, permission sets, teams, goals, filters, webhooks, subscriptions, mailbox, call logs, projects, misc), plus a generic request tool that reaches any remaining v1 endpoint through the same auth, rate-limit and read-only layer.

Full coverage is more tools than an LLM can choose between reliably, so pipedrive.toolGroups controls which groups are published. The default eight core CRM groups publish 108 tools; ["all"] publishes everything. Filtering happens in IInstance._collect_tool_methods(), so a gated tool is invisible to tool.query and refused by tool.invoke alike.

Client details specific to Pipedrive:

  • {"success", "data", "additional_data"} envelope is unwrapped; success: false on an HTTP 200 is raised as an error.
  • x-ratelimit-reset is the seconds remaining in the window, not an epoch timestamp as in the GitHub client this otherwise mirrors.
  • Offset pagination (start/limit, max 500) everywhere except projects (cursor) and /files (max 100).
  • Personal API tokens go in the api_token query parameter; JWT, long or Bearer -prefixed values go in an Authorization header. Because the token rides in the query string, transport-level errors are built from the exception type plus method and path, never str(exc), and the cause chain is suppressed so a traceback cannot re-expose the URL.
  • Company domains are validated down to a single DNS label, and free-form ids are URL-encoded per path segment, so neither can retarget a request at another host or endpoint.
  • Custom fields are grouped under custom_fields on read and written through the extra passthrough that every create and update tool accepts.

Also corrects the stale node/service counts in docs/README-nodes.md (88 -> 118 directories, 118 -> 148 services).

Also in this PR — changes outside the node

These are not Pipedrive code and carry a wider blast radius. Flagged here so they get
focused review rather than being read as part of the node.

Area Change Why it rides along Tests
packages/ai (chat.py, utils/content_blocks.py, agent/_internal/utils.py) ChatBase._chat returns flattened visible text instead of the provider's raw content. Anthropic with extended thinking returns a list of typed blocks, so every caller was stringifying the blocks' repr — the raw reasoning and its base64 signatures landed in the answer. Without it the Pipedrive tools are unusable from a CrewAI agent: the ReAct parser's greedy Action Input: (.*) swallowed the serialized wrapper's trailing ", "type": "text"}] into the tool arguments, so every declared field reported Field required. tests/ai/common/utils/test_content_blocks.py, tests/ai/common/agent/test_extract_text.py, and tests/ai/common/test_chat_return_contract.py (the _chat return contract itself)
nodes/src/nodes/agent_crewai Per-tool typed Pydantic args_schema; supports_stop_words guarded; planning made opt-in; hierarchical manager tool scoping. Same reason — CrewAI's argument filter intersects emitted keys with the schema's property names, so every real parameter was discarded before validation. nodes/test/agent_crewai/test_tool_schema.py, test_llm_contract.py, test_manager_planning.py, test_manager_tool_scoping.py
packages/shared-ui (liveEventSession.ts) In-memory live-edge event session so the project panes keep working when no host binds openEventStream. Needed to watch a pipedrive agent run at all in the extension. liveEventSession.test.ts (18 tests)
nodes/test/conftest.py, cloud_tts, four node test bootstraps Stop nodes/test reaching sys.path, where its node-named subdirectories shadow the real node packages. This branch's new test files changed collection order and turned a latent landmine into 4 collection errors on develop-adjacent code. nodes/test/test_sys_modules_guard.py

Testing

  • Tests added or updated — 150 credential-free unit tests for the node, plus a 70-test env-gated live suite behind PIPEDRIVE_API_TOKEN (record-creating tests additionally behind PIPEDRIVE_ALLOW_WRITES)
  • Tested locally — full non-dynamic node suite green; packages/ai 689 passed
  • ./builder test passes — CI green on Ubuntu, Windows and macOS

Checklist

  • Commit messages follow conventional commits
  • No secrets or credentials included
  • Wiki updated (if applicable)
  • Breaking changes documented (if applicable)

Linked Issue

Closes #1675

Summary by CodeRabbit

Summary

  • New Features

    • Added a Pipedrive CRM agent tool with selectable tool groups, v1 pagination, custom-field passthrough, optional raw-request escape hatch, and read-only enforcement.
    • Added “live-edge” in-memory event rendering for project views when streamed sessions aren’t available.
  • Bug Fixes

    • Improved AI message rendering to show only user-visible text and omit reasoning/signature blocks.
    • Made CrewAI planning opt-in (off by default) and corrected tool/schema handling for hierarchical manager runs.
  • Documentation

    • Updated node catalog entries and refreshed Pipedrive and CrewAI Manager docs (including planning default/behavior).

Exposes the Pipedrive REST API v1 to agents: 256 tools across 24 resource
groups (deals, persons, organizations, activities, pipelines, stages, notes,
leads, products, fields, files, users, roles, permission sets, teams, goals,
filters, webhooks, subscriptions, mailbox, call logs, projects, misc), plus a
generic `request` tool that reaches any remaining v1 endpoint through the same
auth, rate-limit and read-only layer.

Full coverage is more tools than an LLM can choose between reliably, so
`pipedrive.toolGroups` controls which groups are published. The default eight
core CRM groups publish 108 tools; `["all"]` publishes everything. Filtering
happens in `IInstance._collect_tool_methods()`, so a gated tool is invisible to
`tool.query` and refused by `tool.invoke` alike.

Client details specific to Pipedrive:
- `{"success", "data", "additional_data"}` envelope is unwrapped; `success:
  false` on an HTTP 200 is raised as an error.
- `x-ratelimit-reset` is the seconds remaining in the window, not an epoch
  timestamp as in the GitHub client this otherwise mirrors.
- Offset pagination (`start`/`limit`, max 500) everywhere except projects,
  which use cursor pagination.
- Personal API tokens go in the `api_token` query parameter; JWT, long or
  `Bearer `-prefixed values go in an Authorization header.
- Custom fields are grouped under `custom_fields` on read and written through
  the `extra` passthrough that every create and update tool accepts.

Tests: 81 unit tests that need no credentials or network, plus an env-gated
live suite behind PIPEDRIVE_API_TOKEN, with record-creating tests additionally
behind PIPEDRIVE_ALLOW_WRITES.

Also corrects the stale node/service counts in docs/README-nodes.md
(88 -> 118 directories, 118 -> 148 services).

Closes #1675

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The PR adds a Pipedrive CRM tool node, changes CrewAI planning and tool handling, normalizes typed AI content blocks, adds in-memory live event sessions, and improves test import isolation.

Changes

Pipedrive CRM tool node

Layer / File(s) Summary
Registration, configuration, and transport
nodes/src/nodes/tool_pipedrive/...
Adds Pipedrive registration, authentication, domain routing, grouped tool publication, read-only enforcement, retries, pagination, errors, and response normalization.
Resource-grouped API tools
nodes/src/nodes/tool_pipedrive/tools/*
Adds typed operations across CRM records, administration, communication, search, subscriptions, and projects.
Validation and documentation
nodes/test/tool_pipedrive/*, nodes/src/nodes/tool_pipedrive/README.md, CHANGELOG.md, docs/README-nodes.md
Adds client tests, environment-gated lifecycle tests, and catalog documentation.

CrewAI manager and tool bridge

Layer / File(s) Summary
Planning configuration and manager wiring
nodes/src/nodes/agent_crewai/crewai_manager/*, nodes/src/nodes/agent_crewai/services.manager.json
Makes planning opt-in and prevents delegate tools from being inherited by manager tasks.
LLM and tool schema adapter
nodes/src/nodes/agent_crewai/crewai_base.py, nodes/src/nodes/agent_crewai/requirements.txt
Disables native function-calling claims, generates resilient typed schemas, and bounds CrewAI versions.
CrewAI regression coverage
nodes/test/agent_crewai/*
Tests planning wiring, tool isolation, LLM contracts, schema generation, and stop-word loading.

Typed content blocks and live events

Layer / File(s) Summary
Typed content normalization
packages/ai/src/ai/common/utils/*, packages/ai/src/ai/common/chat.py, packages/ai/src/ai/common/agent/_internal/utils.py
Flattens provider text and reasoning blocks and applies the result to chat and agent extraction.
Live event fallback
packages/shared-ui/src/modules/project/hooks/liveEventSession.ts, packages/shared-ui/src/modules/project/ProjectView.tsx
Adds retained in-memory event stores with independent views and uses them when no host event stream exists.
Regression tests and import isolation
packages/ai/tests/*, packages/shared-ui/src/modules/project/hooks/liveEventSession.test.ts, nodes/test/*
Adds content, live-session, and deterministic test-import coverage.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related issues

  • rocketride-org/rocketride-server#1676 — Covers the same Pipedrive CRM tool-node objective, including grouped operations, authentication, read-only behavior, and tests.

Possibly related PRs

Suggested reviewers: jmaionchi, rod-christensen, stepmikhaylov, asclearuc

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The PR also changes CrewAI, AI utility, and shared-ui code unrelated to the Pipedrive node requirements. Split the unrelated support changes into separate PRs or document why they are needed for this issue.
Docstring Coverage ⚠️ Warning Docstring coverage is 15.27% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately describes the main change: adding the tool_pipedrive CRM node.
Linked Issues check ✅ Passed The PR adds the Pipedrive node, manifest, client, grouped tools, docs, and tests, covering the requested CRM API surface.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/RR-1675-tool-pipedrive

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added docs Documentation module:nodes Python pipeline nodes labels Jul 26, 2026
@github-actions

Copy link
Copy Markdown
Contributor
🤖 Internal: Discord sync marker

Auto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete.

joshuadarron and others added 3 commits July 27, 2026 09:22
…icon

The first icon was a green uppercase "P" next to an unrelated dark dot. The
actual Pipedrive mark is a solid green disc carrying a white lowercase "p"
whose counter shows the disc through it.

Redrawn as plain geometry (disc r=128, stem 36x150, bowl r=56, counter r=27)
so it stays crisp at 16px with no gradients, clip paths or embedded raster.

Uses the default nonzero fill rule with the counter arc wound opposite to the
bowl. Under evenodd the stem and bowl would XOR where they overlap and punch a
green notch out of their junction; nonzero merges them solid while the opposing
winding still renders the counter as a hole.

Keeps the viewBox (SVGR runs with dimensions:false, so an icon without one
computes to zero width) and keeps two distinct colours, which makes the
auto-currentcolor svgo plugin treat it as a multicolour brand mark and pass the
authored colours through untinted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…work

The previous version approximated the mark with circles. The real letterform
differs in three ways that are visible even at 16px: the stem carries a spur at
the top left, the bowl is an ellipse rather than a circle, and the counter is an
ellipse tangent to the stem's right edge.

Geometry was measured off the supplied 272x272 artwork rather than eyeballed —
connected-component analysis to isolate the counter, then a least-squares fit of
the bowl ellipse to the glyph's right-hand edge (max residual 2px). Rasterising
the traced primitives and diffing against the source leaves 1.47% of opaque
pixels differing, all of it a sub-pixel outline along the shape boundary.

Still a two-colour multicolour mark, so the auto-currentcolor svgo plugin passes
the authored colours through, and still nonzero winding with the counter wound
opposite the bowl so the stem and bowl merge solid while the counter stays open.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… agent

Full coverage is 256 tools. An operator can already reach that by selecting
groups, and nothing told them the agent's tool choice degrades long before it.

Adds a guard rail that counts published tools rather than groups, because group
sizes are lopsided: deals is 28 tools and permission_sets is 3, so a group count
bounds nothing useful. The threshold is 120, just under the 128-function cap
some providers impose on a single request and above the 107 the default groups
publish.

Crossing it warns and carries on. The tools are still published, in both
validateConfig (so it shows in the config panel while editing) and beginGlobal
(so it shows at pipeline start). Silently dropping tools an operator asked for
surfaces later as "the agent won't do X", which is worse than a noisy panel. The
warning is deliberately not raised from _collect_tool_methods, which runs on
every tool.query and tool.invoke.

"all" is exempt: it is already an explicit opt-in to the whole surface, and
scripted callers that never route through an LLM legitimately want it. Listing
every group by name is not exempt.

Group sizes are derived from the __pipedrive_group__ stamps rather than a
hand-maintained table, so a new tool cannot drift out of sync; a test asserts
every group in ALL_GROUPS has a count. Another test pins the advisory semantics
so the warning is not later "fixed" into a silent truncation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
joshuadarron and others added 4 commits July 28, 2026 22:36
Anthropic with extended thinking returns `content` as a list of typed
blocks, not a string. Every non-streaming path handed that list straight
to callers, who stringified it — so the answer arrived as the blocks'
repr, carrying the raw reasoning and its base64 signatures.

For agents it was worse than cosmetic: the ReAct parser reads output back
with a greedy `Action Input: (.*)`, so the serialized wrapper's trailing
`", "type": "text"}]` landed inside the tool arguments and every declared
field then reported as missing.

Extract the block vocabulary that only the streaming path knew into
`ai.common.utils.flatten_content_blocks` and route chat's invoke, the
stream fallback, and the agent's `extract_text` through it. The streaming
path now shares that vocabulary instead of duplicating it, so the paths
cannot drift; the fallback also forwards recovered reasoning to
`on_reasoning_chunk` rather than dropping it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three defects made the CrewAI nodes fail on runs that should have worked.

Tool arguments were dropped. `_ToolInput` declared a single `input` field,
and CrewAI's argument filter intersects the model's emitted keys with the
schema's property names — so every real parameter was discarded before
validation, which then reported all required fields missing. Generate a
typed model per tool instead: JSON Schema types map to real annotations
(so pydantic coerces "123" -> 123), optional fields accept an explicit
null, and paramless tools get an empty schema. The JSON schema is no
longer appended to the description either; CrewAI renders args_schema into
the prompt itself, and the two copies disagreed.

`HostInvokeLLM` never declared `supports_function_calling()`. CrewAI
defines it on `LLM` but not on `BaseLLM`, and several call sites invoke it
unguarded — converter.py, tool_usage.py, reasoning_handler, and
task_evaluator. The AttributeError was swallowed by converter's broad
except and re-reported as "Failed to convert text into a Pydantic model".
Declare it False: the host channel is text-in / text-out and cannot honour
native tool calls, so every call belongs on the ReAct path. Pin crewai <2
for the same reason — 1.15.x moves the BaseLLM contract.

Hierarchical crews then died in planning. `Crew(planning=True)` runs a
task with `output_pydantic` set and drives it with the host wrapper, which
cannot promise JSON; unparseable plan text dead-ends the converter and
raises "Failed to get the Planning output". Make planning opt-in via a new
**Crew Planning** field, off by default.

Also clear `Task.tools` after construction: CrewAI back-fills them from
the task's agent, and a hierarchical crew resolves the executing agent's
toolset for the manager — which let the manager work a delegate's tools
itself instead of delegating.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pipedrive retired the v1 search routes: `/persons/search` and its siblings
answer 404 "Unknown method ." and `/itemSearch` answers a plain 404, so
every search tool the node published was dead. Non-search v1 endpoints
still route normally, so only the search calls move — a blanket swap would
break the many endpoints with no v2 equivalent.

Adds a v2 base URL (company-domain aware, sharing v1's normalisation) and
a v2 envelope wrapper: v2 replaced offset pagination with an opaque
`next_cursor`, which `paginated` would have missed entirely and reported
as a single page. Search tool schemas now advertise `cursor` instead of
`start`, and `item_search_by_field` follows v2's renames — `field_type` ->
`entity_type` with the "Field" suffix dropped from each value, and
`exact_match` replaced by the three-way `match` mode.

Also turns Tool groups into a multi-select dropdown with per-group tool
counts, so the field no longer requires typing group names from memory.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR #1661 rewired every accumulating pane — Trace, Errors, Status, Tokens,
Flow, Log, Analyze — to read through a DVR session obtained from the
host's `openEventStream`, a prop no host in this repo binds. Hosts that
have not bridged it across their transport (the VS Code webview is one)
passed a null session, `useTaskEvents.ingestLive` dropped every event, and
the panes rendered their empty states mid-run with no error anywhere.

Fall back to an in-memory store over the live events the host already
delivers. It satisfies the same session contract `deliver` expects — a
pure append, each event handed over exactly once, oldest first, across
repeated `play` calls — and is keyed per stream identity so the several
views `SourceSection` opens all share one buffer. Replay of recorded runs
still requires the real stream.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added module:ai AI/ML modules module:ui Chat UI and Dropper UI labels Jul 29, 2026
joshuadarron and others added 2 commits July 29, 2026 14:34
nodes/test is a package whose subdirectories are named after node packages
(text_output/, response/, image_cleanup/, telegram/, tool_git/, ...). Two places
put that directory on sys.path so they could bare-import a test-root module:
conftest (for _sys_modules_guard) and cloud_tts/test_cloud_tts (for
test_contracts). Once nodes/test sits ahead of src/nodes, `import text_output`
resolves to the *test* package, whose __path__ holds no node module — so
`from text_output.instance import Instance` dies with ModuleNotFoundError.

Whether it bites was pure collection order. The affected modules bootstrap with
"insert src/nodes only if absent", which never re-prioritises: previously nothing
seeded src/nodes before cloud_tts (c) inserted nodes/test, so image_cleanup (i)
was the first to insert src/nodes and landed it in front. The new agent_crewai
tests seed src/nodes during collection of agent_crewai (a) instead, so cloud_tts
now stacks nodes/test on top of it and the guard short-circuits in all four later
modules — four collection errors, none of them in code this branch touches.

Import both test-root modules package-relative and drop the sys.path inserts, so
nodes/test never shadows a node package. cloud_tts gains the __init__.py the rest
of the node-named test dirs already have, which is what makes the relative import
resolvable. The four bootstraps now move src/nodes to the front unconditionally
rather than skipping when it is already somewhere on the path, and the guard suite
pins both invariants: nodes/test stays off sys.path, and a node package imported
by name resolves under src/nodes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@joshuadarron

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 19

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/shared-ui/src/modules/project/ProjectView.tsx (1)

486-511: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Close the live-edge store when the project identity changes.

SourceSection closes each session it opens (pane and fetchChapterEvents walker), but ProjectView keeps liveStoresRef.current across renders and only creates new LiveEventStores; it never closes the store when source.id, runKind, or projectId changes. Track the current identity (at least projectId) and close the old store before re-creating it, otherwise LiveEventStore.views can retain detached sessions and backstop trim events that can no longer be read.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/shared-ui/src/modules/project/ProjectView.tsx` around lines 486 -
511, Update the liveStore callback and liveStoresRef lifecycle so stores are
closed when their identity changes, including at least projectId and the
source.id/runKind key. Before replacing or discarding a store in
liveStoresRef.current, call its close method, and ensure the active identity is
tracked so unchanged keys continue reusing their existing store without closing
it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@nodes/src/nodes/agent_crewai/crewai_base.py`:
- Around line 461-466: Update the docstring of _build_crew_tools to remove the
claim that each tool’s JSON Schema is embedded in its description, and document
that the schema is provided through CrewAI’s args_schema instead. Keep the rest
of the method documentation unchanged.
- Around line 363-370: Update the nested _annotation_for function used by
_build_crew_tools to inspect prop.get('type') before _JSON_TYPE_MAP lookup,
returning Any whenever the type value is not a string (including JSON Schema
arrays such as ["string", "null"]); retain the existing mapped annotation
behavior for string types.

In `@nodes/src/nodes/tool_pipedrive/pipedrive_client.py`:
- Around line 175-182: The company domain normalization in the relevant
Pipedrive client helper must parse and validate exactly one Pipedrive subdomain
before returning it. Reject fragments, queries, credentials, ports, URL
delimiters, and any label containing characters outside lowercase letters,
digits, or hyphens; return None for invalid input, while preserving valid bare
subdomains and supported Pipedrive URLs.

In `@nodes/src/nodes/tool_pipedrive/requirements.txt`:
- Line 1: Pin the requests dependency in requirements.txt to the
constraints-backed safe version requests==2.34.2, matching the workspace
constraint and preventing resolution to older versions.

In `@nodes/src/nodes/tool_pipedrive/tools/call_logs.py`:
- Around line 121-124: URL-encode every model-supplied string path segment with
urllib.parse.quote(value, safe='') before interpolation, preferably through a
shared helper in _base.py. Apply this to log_id in call_log_delete,
call_log_get, and call_log_recording_add; lead UUIDs in lead_get and
lead_delete; label UUIDs in lead_label_delete and lead_label_update; and set_id
in permission_set_get and permission_set_assignments_list across the specified
call_logs.py, leads.py, and roles.py sites.

In `@nodes/src/nodes/tool_pipedrive/tools/deals.py`:
- Around line 65-99: The write-key tuples contain fields unavailable through
their corresponding schemas; make each key/schema pair consistent. In
nodes/src/nodes/tool_pipedrive/tools/deals.py:65-99, organizations.py:63-72, and
persons.py:62-72, either expose add_time in _DEAL_WRITE_PROPS, _ORG_WRITE_PROPS,
and _PERSON_WRITE_PROPS or remove it from the matching write-key tuple. In
nodes/src/nodes/tool_pipedrive/tools/leads.py:52-65, either expose origin_id in
_LEAD_WRITE_PROPS or remove it from _LEAD_WRITE_KEYS.
- Around line 576-585: Add a shared _delete_bulk helper in _base.py that calls
_require_write() before validating ids, validates the required non-empty list,
builds the CSV parameters, applies optional extra passthrough, and performs the
DELETE request. Mark ids as required and delegate deal_delete_bulk in
nodes/src/nodes/tool_pipedrive/tools/deals.py#L576-L585, activity_delete_bulk
and activity_type_delete_bulk in
nodes/src/nodes/tool_pipedrive/tools/activities.py#L176-L183 and `#L253-L260`,
organization_delete_bulk in
nodes/src/nodes/tool_pipedrive/tools/organizations.py#L179-L186,
person_delete_bulk in nodes/src/nodes/tool_pipedrive/tools/persons.py#L208-L215,
and stage_delete_bulk in
nodes/src/nodes/tool_pipedrive/tools/pipelines.py#L280-L287 to the helper,
preserving the deals extra passthrough.
- Around line 558-566: Remove the deal_followers_users_list tool and its
associated registration, leaving deal_followers_list as the sole tool for the
/deals/{deal_id}/followers endpoint; do not apply clean_user to follower rows.

In `@nodes/src/nodes/tool_pipedrive/tools/fields.py`:
- Around line 102-144: The declared EXTRA fields are omitted from request
bodies. In nodes/src/nodes/tool_pipedrive/tools/fields.py lines 102-144, update
field_create and field_update; in nodes/src/nodes/tool_pipedrive/tools/misc.py
lines 127-164, update channel_message_receive. After each body_from call and
before self._write, merge args.get('extra') or {} into the body.

In `@nodes/src/nodes/tool_pipedrive/tools/files.py`:
- Around line 63-70: Update file_list in
nodes/src/nodes/tool_pipedrive/tools/files.py (lines 63-70) to use a
Files-specific schema or validation that caps limit at 100 while preserving the
existing pagination behavior. Update nodes/src/nodes/tool_pipedrive/README.md
(lines 77-82) to qualify the general 1–500 limit statement and document the
Files endpoint’s 100-item maximum.

In `@nodes/src/nodes/tool_pipedrive/tools/products.py`:
- Around line 307-312: Replace the response cleaner argument dict with the
already imported passthrough cleaner in product_variation_update at
nodes/src/nodes/tool_pipedrive/tools/products.py:307-312,
project_plan_activity_update at
nodes/src/nodes/tool_pipedrive/tools/projects.py:234-239, and
project_plan_task_update at
nodes/src/nodes/tool_pipedrive/tools/projects.py:256-261. Preserve the existing
PUT requests and bodies.

In `@nodes/src/nodes/tool_pipedrive/tools/projects.py`:
- Around line 55-58: Replace the local _CURSOR_PROPS and _cursor_list
cursor-pagination implementation with the existing PAGING_V2(),
paging_params_v2(), and paginated_v2 helpers already used by the search tools.
Update the affected project-listing flow to preserve the shared clamping, cursor
trimming, count semantics, and paginated envelope behavior.

In `@nodes/test/tool_pipedrive/test_pipedrive.py`:
- Around line 198-203: Update the domain parameter list in
test_company_domain_forms so the case-normalization case passes an uppercase
value directly, rather than calling lower() before base_url_for receives it.
Keep the existing expected normalized URL and other domain-form cases unchanged.
- Around line 388-402: Reorder parametrization and patch injection for all four
tests in nodes/test/tool_pipedrive/test_pipedrive.py: lines 388-402
(test_malformed_retry_after_falls_back_to_backoff), 575-597
(test_writes_are_blocked), 870-886 (test_search_tools_target_v2), and 910-924
(test_non_search_tools_stay_on_v1). Make pytest.mark.parametrize the outermost
decorator and update signatures respectively to (self, bad_value, mock_request,
mock_sleep), (self, tool, args, mock_request), (self, tool, args, path,
mock_request), and (self, tool, args, path, mock_request).

In `@nodes/test/tool_pipedrive/test_tools.py`:
- Around line 422-433: Update test_create_and_delete to resolve a valid deal
field from the /dealFields endpoint before constructing conditions, replacing
the hard-coded field_id '1'. If no suitable field exists, skip the test as the
prerequisite is unavailable; otherwise use the resolved field identifier in the
filter body.
- Around line 413-415: Remove the vacuous base64.b64encode assertion in the
download test, since the preceding downloaded == payload assertion already
verifies the raw response; if validating encoded output is required, compare it
against the expected base64 value returned by file_download instead of asserting
truthiness.
- Around line 270-277: Update test_pipeline_movement_statistics to assert that
pipelines is non-empty before indexing pipelines[0], matching the guard in
test_pipeline_conversion_statistics. Keep the existing movement_statistics
request and assertions unchanged after the guard.
- Around line 27-28: Update the imports in the live test to use the node package
path, matching test_pipedrive.py: import PipedriveAPIError, base_url_for,
base_url_v2_for, call, and call_envelope from tool_pipedrive.pipedrive_client,
and remove the sys.path insertion and bare pipedrive_client import.

In `@packages/shared-ui/src/modules/project/hooks/liveEventSession.ts`:
- Around line 158-170: Update TaskEventSession.getStatus() to scan only events
at or before the current session watermark established by seek(anchor), rather
than the entire buffer tail. Preserve the existing status-event filtering and
null fallback, ensuring useTaskEvents receives the status snapshot corresponding
to the replay position.

---

Outside diff comments:
In `@packages/shared-ui/src/modules/project/ProjectView.tsx`:
- Around line 486-511: Update the liveStore callback and liveStoresRef lifecycle
so stores are closed when their identity changes, including at least projectId
and the source.id/runKind key. Before replacing or discarding a store in
liveStoresRef.current, call its close method, and ensure the active identity is
tracked so unchanged keys continue reusing their existing store without closing
it.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 5f6cbec3-08b8-4c0e-b062-f1d70c93a4ca

📥 Commits

Reviewing files that changed from the base of the PR and between 3688389 and c4fb63f.

⛔ Files ignored due to path filters (1)
  • nodes/src/nodes/tool_pipedrive/pipedrive.svg is excluded by !**/*.svg
📒 Files selected for processing (67)
  • CHANGELOG.md
  • docs/README-nodes.md
  • nodes/src/nodes/agent_crewai/README.md
  • nodes/src/nodes/agent_crewai/crewai_base.py
  • nodes/src/nodes/agent_crewai/crewai_manager/IGlobal.py
  • nodes/src/nodes/agent_crewai/crewai_manager/README.md
  • nodes/src/nodes/agent_crewai/crewai_manager/manager.py
  • nodes/src/nodes/agent_crewai/requirements.txt
  • nodes/src/nodes/agent_crewai/services.manager.json
  • nodes/src/nodes/tool_pipedrive/IGlobal.py
  • nodes/src/nodes/tool_pipedrive/IInstance.py
  • nodes/src/nodes/tool_pipedrive/README.md
  • nodes/src/nodes/tool_pipedrive/__init__.py
  • nodes/src/nodes/tool_pipedrive/pipedrive_client.py
  • nodes/src/nodes/tool_pipedrive/requirements.txt
  • nodes/src/nodes/tool_pipedrive/services.json
  • nodes/src/nodes/tool_pipedrive/tool_groups.py
  • nodes/src/nodes/tool_pipedrive/tools/__init__.py
  • nodes/src/nodes/tool_pipedrive/tools/_base.py
  • nodes/src/nodes/tool_pipedrive/tools/activities.py
  • nodes/src/nodes/tool_pipedrive/tools/call_logs.py
  • nodes/src/nodes/tool_pipedrive/tools/deals.py
  • nodes/src/nodes/tool_pipedrive/tools/fields.py
  • nodes/src/nodes/tool_pipedrive/tools/files.py
  • nodes/src/nodes/tool_pipedrive/tools/filters.py
  • nodes/src/nodes/tool_pipedrive/tools/goals.py
  • nodes/src/nodes/tool_pipedrive/tools/leads.py
  • nodes/src/nodes/tool_pipedrive/tools/mailbox.py
  • nodes/src/nodes/tool_pipedrive/tools/misc.py
  • nodes/src/nodes/tool_pipedrive/tools/notes.py
  • nodes/src/nodes/tool_pipedrive/tools/organizations.py
  • nodes/src/nodes/tool_pipedrive/tools/persons.py
  • nodes/src/nodes/tool_pipedrive/tools/pipelines.py
  • nodes/src/nodes/tool_pipedrive/tools/products.py
  • nodes/src/nodes/tool_pipedrive/tools/projects.py
  • nodes/src/nodes/tool_pipedrive/tools/roles.py
  • nodes/src/nodes/tool_pipedrive/tools/search.py
  • nodes/src/nodes/tool_pipedrive/tools/subscriptions.py
  • nodes/src/nodes/tool_pipedrive/tools/teams.py
  • nodes/src/nodes/tool_pipedrive/tools/users.py
  • nodes/src/nodes/tool_pipedrive/tools/webhooks.py
  • nodes/test/agent_crewai/test_llm_contract.py
  • nodes/test/agent_crewai/test_manager_planning.py
  • nodes/test/agent_crewai/test_manager_tool_scoping.py
  • nodes/test/agent_crewai/test_stop_words.py
  • nodes/test/agent_crewai/test_tool_schema.py
  • nodes/test/cloud_tts/__init__.py
  • nodes/test/cloud_tts/test_cloud_tts.py
  • nodes/test/conftest.py
  • nodes/test/image_cleanup/test_provenance.py
  • nodes/test/local_text_output/test_instance.py
  • nodes/test/response/test_media_metadata.py
  • nodes/test/test_sys_modules_guard.py
  • nodes/test/text_output/test_instance.py
  • nodes/test/tool_pipedrive/__init__.py
  • nodes/test/tool_pipedrive/test_pipedrive.py
  • nodes/test/tool_pipedrive/test_tools.py
  • packages/ai/src/ai/common/agent/_internal/utils.py
  • packages/ai/src/ai/common/chat.py
  • packages/ai/src/ai/common/utils/__init__.py
  • packages/ai/src/ai/common/utils/content_blocks.py
  • packages/ai/tests/ai/common/agent/__init__.py
  • packages/ai/tests/ai/common/agent/test_extract_text.py
  • packages/ai/tests/ai/common/utils/test_content_blocks.py
  • packages/shared-ui/src/modules/project/ProjectView.tsx
  • packages/shared-ui/src/modules/project/hooks/liveEventSession.test.ts
  • packages/shared-ui/src/modules/project/hooks/liveEventSession.ts

Comment thread nodes/src/nodes/agent_crewai/crewai_base.py Outdated
Comment thread nodes/src/nodes/agent_crewai/crewai_base.py
Comment thread nodes/src/nodes/tool_pipedrive/pipedrive_client.py Outdated
Comment thread nodes/src/nodes/tool_pipedrive/requirements.txt Outdated
Comment thread nodes/src/nodes/tool_pipedrive/tools/call_logs.py Outdated
Comment thread nodes/test/tool_pipedrive/test_tools.py Outdated
Comment thread nodes/test/tool_pipedrive/test_tools.py
Comment thread nodes/test/tool_pipedrive/test_tools.py Outdated
Comment thread nodes/test/tool_pipedrive/test_tools.py
Comment thread packages/shared-ui/src/modules/project/hooks/liveEventSession.ts
…e PR

Security
- pipedrive_client._company_host accepted anything up to the first slash, so a
  configured domain of "evil.example#" built https://evil.example#.pipedrive.com
  and Requests sent it — with the api_token or Authorization header attached — to
  evil.example. Cut at the first path/query/fragment delimiter, refuse userinfo
  and ports, lower-case, and require a single DNS label; anything else falls back
  to the generic host.
- Free-form ids (lead uuids, permission-set ids, channel ids, goal ids, note
  comment ids) were f-stringed into request paths unencoded, so a value carrying
  "/", "..", "?" or "#" retargeted the call at another endpoint — for the DELETE
  tools, a destructive one. Added _base.path_segment() and applied it at all 20
  string-id sites.
- Pin requests>=2.34.2 to match constraints.lock, as tool_cognee/tool_github/
  tool_n8n already do.

Correctness
- crewai_base._annotation_for passed prop['type'] straight to dict.get. JSON
  Schema spells a nullable field as ["string", "null"], and an unhashable key
  raises TypeError instead of returning the default — one such property aborted
  the whole _build_crew_tools loop. Non-str types now fall back to Any, which is
  what the docstring already promised.
- deal_followers_users_list GET the same /deals/{id}/followers as
  deal_followers_list and only differed by running follower rows through
  clean_user, emitting a user-shaped object built from mismatched fields.
  Dropped it; resolving followers to users needs a /users/{id} call per follower.
- add_time (deals, organizations, persons) and origin_id (leads) were in the
  write-key tuples but not in the matching schema props, so body_from could never
  populate them. Added the properties.
- product_variation_update and both project-plan updates passed the builtin dict
  as the response cleaner: dict(None) raises TypeError on an empty 204 body.
  Switched to passthrough, as every other write already uses.
- liveEventSession.getStatus scanned from the live edge, contradicting the
  documented "as of the session position" and the caller, which seeks to an
  anchor first — a repositioned view seeded a replayed run with the current
  status. Scan back from the watermark.

Maintainability
- Eight bulk deletes each repeated the ids guard, the CSV build and a trailing
  _require_write(), and left ids optional so a missing argument surfaced as a
  hand-rolled ValueError. Added _base._delete_bulk (write gate first), marked ids
  required in every schema, and delegated all eight.
- projects._cursor_list re-implemented paging_params_v2 and the v2 envelope
  shaping; it now uses the shared helpers, so the cursor contract has one
  implementation instead of two that can drift.
- file_list advertised the shared 1-500 range, but /files documents a maximum
  limit of 100. PAGING/paging_params/_list take a max_limit override and file_list
  passes 100.
- _build_crew_tools' docstring still claimed the JSON Schema is embedded in the
  tool description, which stopped being true when args_schema took over.
- Tests: 'ACME'.lower() evaluated to 'acme' before the call, so the case path was
  never exercised; test_tools imported pipedrive_client bare, giving the module a
  second identity in a run that also collects test_pipedrive; a vacuous
  b64encode assert, a missing empty-pipelines guard, and a hard-coded field_id
  of 1 that fails rather than skips on a fresh sandbox.

Two findings were not applied. `extra` is not dropped by fields/misc — body_from
merges args['extra'] by default (tools/_base.py). And the @patch/parametrize
signatures are already correct: mock injects positionally innermost-first while
pytest passes parametrize values by keyword, so the suggested reorder would bind
the mocks to the parametrized names.

New tests cover the rejected domains, path-segment encoding, non-str schema
types, the write-gate ordering, and the watermark-bounded status.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@joshuadarron

Copy link
Copy Markdown
Collaborator Author

Addressed the CodeRabbit review in 7b3cc53.

Security

  • _company_host accepted anything up to the first slash, so evil.example# built https://evil.example#.pipedrive.com/... and Requests sent it — with the api_token / Authorization attached — to evil.example. Now cuts at the first path/query/fragment delimiter, refuses userinfo and ports, lower-cases, and requires a single DNS label.
  • Added _base.path_segment() and applied it at all 20 sites where a free-form id (lead uuids, permission-set ids, channel ids, goal ids, note comment ids) was f-stringed into a request path. The review listed 6; the same pattern was present in goals.py, misc.py and notes.py too.
  • Pinned requests>=2.34.2 to match constraints.lock.

Correctness

  • crewai_base._annotation_for: "type": ["string", "null"] is unhashable, so dict.get raised TypeError instead of falling through — one nullable property aborted the whole _build_crew_tools loop. Non-str types now return Any, which the docstring already promised.
  • Dropped deal_followers_users_list (same endpoint as deal_followers_list, clean_user over follower rows).
  • Added the missing add_time / origin_id schema properties so those write keys are reachable.
  • dictpassthrough in the three write call sites.
  • liveEventSession.getStatus() now scans back from the watermark, honouring the documented "as of the session position".

Maintainability

  • Added _base._delete_bulk (write gate first, then validation) and delegated all 8 bulk deletes — the review found 6 — with ids marked required in every schema.
  • projects._cursor_list now uses PAGING_V2() / paging_params_v2 / paginated_v2.
  • file_list advertises and clamps to /files' documented 100 limit via a new max_limit override.
  • Fixed the stale _build_crew_tools docstring and the five test findings.

Dropping the duplicate followers tool moved the surface from 256 to 255 tools; services.json labels, tool_groups.py and the README were updated to match.

Not applied — 2 findings

  • fields.py / misc.py extra pass-through: body_from already merges args['extra'] (its extra_key defaults to 'extra'), so the suggested body.update(...) would be a second, redundant merge. Thread
  • @patch / parametrize argument order: mock injects positionally innermost-first, pytest passes parametrize values by keyword. The current signatures are correct and the suggested reorder would bind a mock to bad_value. Thread

New tests cover the rejected domains, path-segment encoding, non-str schema types, the bulk-delete write-gate ordering, and the watermark-bounded status.

@joshuadarron
joshuadarron marked this pull request as ready for review July 30, 2026 01:41

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
nodes/src/nodes/tool_pipedrive/services.json (1)

59-89: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Make tool counts and the warning guard account for the raw request consistently.

The listed groups sum to 254, and the default eight sum to 106. Since request is separately published when allowRawRequest is enabled, the documented default of 108 cannot be reached from these counts; moreover, the guard currently omits that extra published tool.

  • nodes/src/nodes/tool_pipedrive/services.json#L59-L89: Correct the displayed per-group, default, and “All groups” counts after deriving them from registered tools; keep raw-request counting separate from group selection.
  • nodes/src/nodes/tool_pipedrive/README.md#L38-L50: Align the 255/108 claims with the corrected configuration counts and clarify whether request is included.
  • nodes/src/nodes/tool_pipedrive/tool_groups.py#L29-L35: Update the guard contract and implementation so allowRawRequest=True contributes one tool before applying the 120-tool warning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nodes/src/nodes/tool_pipedrive/services.json` around lines 59 - 89, Derive
the Pipedrive group, default, and all-groups tool counts from the registered
tools, then update nodes/src/nodes/tool_pipedrive/services.json lines 59-89
accordingly while keeping the raw request separate from group counts. Align the
documented totals and explicitly state whether request is included in
nodes/src/nodes/tool_pipedrive/README.md lines 38-50. Update the warning guard
contract and implementation in nodes/src/nodes/tool_pipedrive/tool_groups.py
lines 29-35 so allowRawRequest=True adds one tool before applying the 120-tool
threshold.
nodes/src/nodes/agent_crewai/crewai_base.py (2)

297-315: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a PEP 257 docstring to the new override.

supports_function_calling is a new public method in nodes/**/*.py, but currently has only inline comments. Add a short triple-double-quoted docstring while retaining the detailed rationale if useful.

As per path instructions, Python pipeline nodes require PEP 257 docstrings; the repository convention uses triple-double-quoted docstrings.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nodes/src/nodes/agent_crewai/crewai_base.py` around lines 297 - 315, Add a
concise triple-double-quoted PEP 257 docstring immediately inside the
supports_function_calling method, describing that it reports whether native
function calling is supported. Retain the existing detailed inline rationale and
return behavior unchanged.

Sources: Path instructions, Learnings


390-396: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Guard malformed required lists before building the model.

set(required) raises TypeError for malformed schemas such as required: [[]]. This occurs before the create_model fallback and aborts the entire tool-construction loop instead of isolating the invalid descriptor.

Proposed fix
-            required_keys = set(required) if isinstance(required, (list, tuple, set)) else set()
+            required_keys = (
+                {item for item in required if isinstance(item, str)}
+                if isinstance(required, (list, tuple, set))
+                else set()
+            )

Based on the PR objective that malformed descriptors must not prevent valid tools from being included, malformed required entries need the same safe fallback.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nodes/src/nodes/agent_crewai/crewai_base.py` around lines 390 - 396, Update
the required-key handling near input_schema and required_keys to validate that
every required entry is a string before constructing the set; treat malformed
values such as nested lists as an invalid descriptor and return _NoArgsInput,
allowing the surrounding tool-construction loop to continue processing valid
tools.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@nodes/src/nodes/tool_pipedrive/tools/_base.py`:
- Around line 211-214: Update the delete parameter construction in the
bulk-delete method to apply the validated `extra` dictionary before assigning
the CSV value derived from `ids`; ensure the final `params['ids']` always uses
the validated input and cannot be overridden by `args[extra_key]`.
- Around line 281-290: Update path_segment to reject literal "." and ".." values
before URL encoding, while continuing to quote all other values with safe='' so
reserved characters remain encoded. Ensure callers receive a clear validation
error for these dot-segment IDs and preserve existing behavior for valid
identifiers.

---

Outside diff comments:
In `@nodes/src/nodes/agent_crewai/crewai_base.py`:
- Around line 297-315: Add a concise triple-double-quoted PEP 257 docstring
immediately inside the supports_function_calling method, describing that it
reports whether native function calling is supported. Retain the existing
detailed inline rationale and return behavior unchanged.
- Around line 390-396: Update the required-key handling near input_schema and
required_keys to validate that every required entry is a string before
constructing the set; treat malformed values such as nested lists as an invalid
descriptor and return _NoArgsInput, allowing the surrounding tool-construction
loop to continue processing valid tools.

In `@nodes/src/nodes/tool_pipedrive/services.json`:
- Around line 59-89: Derive the Pipedrive group, default, and all-groups tool
counts from the registered tools, then update
nodes/src/nodes/tool_pipedrive/services.json lines 59-89 accordingly while
keeping the raw request separate from group counts. Align the documented totals
and explicitly state whether request is included in
nodes/src/nodes/tool_pipedrive/README.md lines 38-50. Update the warning guard
contract and implementation in nodes/src/nodes/tool_pipedrive/tool_groups.py
lines 29-35 so allowRawRequest=True adds one tool before applying the 120-tool
threshold.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 25ca362e-f6fd-4add-a0b0-c0c997c7680c

📥 Commits

Reviewing files that changed from the base of the PR and between c4fb63f and 7b3cc53.

📒 Files selected for processing (28)
  • nodes/src/nodes/agent_crewai/crewai_base.py
  • nodes/src/nodes/tool_pipedrive/README.md
  • nodes/src/nodes/tool_pipedrive/pipedrive_client.py
  • nodes/src/nodes/tool_pipedrive/requirements.txt
  • nodes/src/nodes/tool_pipedrive/services.json
  • nodes/src/nodes/tool_pipedrive/tool_groups.py
  • nodes/src/nodes/tool_pipedrive/tools/_base.py
  • nodes/src/nodes/tool_pipedrive/tools/activities.py
  • nodes/src/nodes/tool_pipedrive/tools/call_logs.py
  • nodes/src/nodes/tool_pipedrive/tools/deals.py
  • nodes/src/nodes/tool_pipedrive/tools/fields.py
  • nodes/src/nodes/tool_pipedrive/tools/files.py
  • nodes/src/nodes/tool_pipedrive/tools/filters.py
  • nodes/src/nodes/tool_pipedrive/tools/goals.py
  • nodes/src/nodes/tool_pipedrive/tools/leads.py
  • nodes/src/nodes/tool_pipedrive/tools/misc.py
  • nodes/src/nodes/tool_pipedrive/tools/notes.py
  • nodes/src/nodes/tool_pipedrive/tools/organizations.py
  • nodes/src/nodes/tool_pipedrive/tools/persons.py
  • nodes/src/nodes/tool_pipedrive/tools/pipelines.py
  • nodes/src/nodes/tool_pipedrive/tools/products.py
  • nodes/src/nodes/tool_pipedrive/tools/projects.py
  • nodes/src/nodes/tool_pipedrive/tools/roles.py
  • nodes/test/agent_crewai/test_tool_schema.py
  • nodes/test/tool_pipedrive/test_pipedrive.py
  • nodes/test/tool_pipedrive/test_tools.py
  • packages/shared-ui/src/modules/project/hooks/liveEventSession.test.ts
  • packages/shared-ui/src/modules/project/hooks/liveEventSession.ts

Comment thread nodes/src/nodes/tool_pipedrive/tools/_base.py Outdated
Comment thread nodes/src/nodes/tool_pipedrive/tools/_base.py Outdated
Two follow-ups from the second CodeRabbit pass, both on helpers added in the
previous commit.

_delete_bulk merged the caller's `extra` pass-through after assigning the
validated `ids`, so `deal_delete_bulk({'ids': [1], 'extra': {'ids': '2,3'}})`
deleted 2 and 3 instead of 1. Merge `extra` first and let the validated CSV win.
The hand-rolled version this replaced had the same ordering, so the flaw predates
the helper.

path_segment relied on quote() alone, but "." and ".." contain nothing quote
escapes — `/notes/{id}/comments/..` survived encoding intact and addressed the
parent resource. Reject those two segments instead of encoding them.

Tests cover both: a dot-segment id raises before any request is made, and a
pass-through "ids" no longer displaces the validated list while other extra
parameters still reach the query string.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@asclearuc asclearuc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @joshuadarron — this is a large, careful node: 256 tools across 24 resource groups, a clean read-only/write gate, offset + cursor pagination, and a strong credential-free unit suite. You have already fixed every Major/Critical CodeRabbit finding (SSRF on companyDomain, path-segment encoding, the shared bulk-delete helper, the dict-vs-passthrough cleaners, the array-type schema fix), and two of its Major findings were withdrawn as false positives.

Requesting changes for one security defect: the personal API token can leak into error text and logs on a transport-level failure — see the inline comment on pipedrive_client.py:292. Blocking alongside it: the tool_* checklist requires a test proving the token is absent from the raised error even when the underlying requests exception text contains it, and TestErrors in nodes/test/tool_pipedrive/test_pipedrive.py has none yet — please add one with the fix.

Non-blocking, your call: this PR also bundles unrelated agent_crewai / packages/ai changes (the _chat return-contract change in chat.py affects every chat caller and has no direct test) and a shared-ui live-event-session hook — consider splitting them or calling them out in the description so those wider-blast-radius changes get focused review. Please also confirm nodes/src/nodes/constraints.lock was regenerated for the new node (it has no tool_pipedrive references while tool_github is listed; runtime resolves fine, so low risk). One inline nit on notes.py:56.

timeout=timeout,
)
except requests.RequestException as exc:
raise ValueError(f'Pipedrive request failed: {exc}') from exc

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

MUST fix — the CRM API token can leak into error messages and logs.

On the personal-token path, _auth() returns the credential as an api_token query parameter, so requests bakes it into the request URL: https://<host>.pipedrive.com/api/v1/deals?api_token=<SECRET>. When the request fails at transport level (DNS failure, connection refused, TLS error, timeout), requests raises a RequestException whose text contains that full URL. This line stringifies it:

raise ValueError(f'Pipedrive request failed: {exc}') from exc

{exc} copies the token verbatim into the ValueError the agent framework surfaces and logs. The project tool_* checklist requires error messages to be built only from structured fields, never from str(exc).

Build the message from non-secret parts (exception type only), and drop the raw {exc}. Note that from exc chains the token-bearing exception, so a printed traceback can still re-expose it — suppress the chain with from None (below), or if you want to keep the cause detail, scrub the token, e.g. str(exc).replace(token, '***').

Suggested change
raise ValueError(f'Pipedrive request failed: {exc}') from exc
raise ValueError(f'Pipedrive request failed: {type(exc).__name__}') from None

Please also add the secret-leak regression test the checklist calls for: assert the token does not appear in the raised error's message even when the mocked requests call raises an exception whose text contains the token.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in cdc66fa.

except requests.RequestException as exc:
    raise ValueError(
        f'Pipedrive request failed: {type(exc).__name__} on {method.upper()} {path}'
    ) from None

One deviation from your suggestion: I kept method and path in the message. Both are secret-free — path is the API path with no query string, which is where the credential lives — and without them the message can't tell you which call died. Say the word if you'd rather have the type alone.

Verified this was the only leak site in the client:

  • line 308 stringifies resp.text[:200] — the response body, not the URL.
  • _raise_pipedrive_error builds from parsed error / error_info / code fields.
  • the tenacity wrapper uses reraise=True, so no RetryError repr is introduced.

Three tests added to TestErrors, and all three fail against the previous code:

  • test_api_token_is_not_in_the_transport_error_or_its_traceback — mocks requests.request to raise a ConnectionError whose text embeds ...?api_token=<TOKEN>, then asserts the token is in neither str(exc) nor traceback.format_exception(...). The traceback half is what pins from None — you were right that the chained cause re-exposes it, and scrubbing the message alone wouldn't have been enough.
  • test_transport_failure_names_the_failure_and_the_call — guards against the fix regressing into a bare string.
  • the existing test_transport_failure_raises_value_error.

Red check before/after:

# against the old `f'...: {exc}' from exc`
FAILED test_transport_failure_names_the_failure_and_the_call
FAILED test_api_token_is_not_in_the_transport_error_or_its_traceback
E   AssertionError: assert 'pipedrive-p...-placeholder' not in 'Pipedrive r...lutionError)'

# after
3 passed

Reused the existing PERSONAL_TOKEN placeholder rather than a token-shaped literal, so gitleaks stays quiet.

'lead_id',
'project_id',
'user_id',
'add_time',

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit — add_time is unreachable for notes.

add_time is listed in _NOTE_WRITE_KEYS (line 56) but not in _NOTE_WRITE_PROPS (lines 63-74). note_create / note_update publish **_NOTE_WRITE_PROPS as the input schema, and body_from() only copies keys present in args; the framework validates args against that published schema. A key that is not a published property can never be populated, so an agent cannot set add_time on a note (it only works if smuggled through extra).

CodeRabbit raised this same class for deals and organizations, and you fixed those by adding add_time to their *_WRITE_PROPS; notes was missed. Either add 'add_time': STR(...) to _NOTE_WRITE_PROPS, or drop it from _NOTE_WRITE_KEYS.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in cdc66fa — added add_time to _NOTE_WRITE_PROPS with the same wording as deals/organizations/persons.

Good catch that notes was missed. I scanned every *_WRITE_KEYS / *_WRITE_PROPS pair across tools/*.py after fixing it, and notes was the last mismatch:

$ python - <<'PY'   # keys present in *_WRITE_KEYS but absent from the matching *_WRITE_PROPS
...
PY
no mismatches

The personal-token path puts the credential in the `api_token` query parameter,
so requests bakes it into the URL. A transport-level failure (DNS, refused
connection, TLS, timeout) raises a RequestException whose text carries that URL,
and `raise ValueError(f'Pipedrive request failed: {exc}') from exc` copied the
token verbatim into the error the agent framework surfaces and logs. The chained
cause held the same URL, so scrubbing the message alone would still have leaked
it through any printed traceback.

Build the message from the exception type plus method and path — all secret-free,
and the type still distinguishes ConnectTimeout from SSLError from ConnectionError
— and raise `from None`. Verified this was the only such site: the JSON-decode
path stringifies `resp.text[:200]` rather than the URL, `_raise_pipedrive_error`
builds from parsed response fields, and the tenacity wrapper uses `reraise=True`
so no RetryError repr is introduced.

Three tests pin it, and all three fail against the previous code: the token
appears in neither the message nor the formatted traceback (that half is what
pins `from None`), and the message still names the failure and the call.

Also:
- notes' `add_time` was in _NOTE_WRITE_KEYS but not _NOTE_WRITE_PROPS, so
  body_from could never populate it — the same class fixed for deals,
  organizations, persons and leads in 7b3cc53. A scan of every
  *_WRITE_KEYS/*_WRITE_PROPS pair confirms notes was the last one.
- ChatBase._chat had no direct test. flatten_content_blocks and extract_text are
  covered, but nothing pinned the wiring that routes the provider's content
  through them, which is the contract every non-streaming chat caller depends on.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@joshuadarron

Copy link
Copy Markdown
Collaborator Author

@asclearuc — both blocking items are fixed in cdc66fa, details in the two inline threads. Answers to the three non-blocking notes:

1. Bundled agent_crewai / packages/ai / shared-ui changes

Fair point. I've rewritten the PR description with an "Also in this PR — changes outside the node" table naming each one, its blast radius, why it rides along, and where its tests are. Not splitting them: the CrewAI schema fix and the content-block flattening are what make the Pipedrive tools usable from an agent at all — without them every tool call arrives with Field required on every parameter — so they'd land as a stack of dependent PRs rather than independent ones.

2. _chat has no direct test — you were right, now it does

cad91690 added 229 lines covering flatten_content_blocks and extract_text, but nothing pinned ChatBase._chat itself, which is the contract every non-streaming caller depends on. Added packages/ai/tests/ai/common/test_chat_return_contract.py — 6 tests driving _chat against a stub LLM: the return is always str, typed blocks flatten to the visible answer, reasoning never reaches the caller, and the block repr never reaches the caller (the original regression).

3. constraints.lock has no tool_pipedrive references

Expected, and nothing to regenerate. Per .github/workflows/lock-node-deps.yml, the lock is recompiled and auto-committed only on push to develop, the weekly cron, or manual dispatch. On pull_request the job compiles only — deliberately, as a cross-node conflict gate:

#   * push to develop / weekly / manual -> regenerate + auto-commit the lock.
#   * pull_request touching a requirements.txt -> compile only, so a cross-node
#     version conflict fails HERE (at CI) instead of bricking a user's first run.

That gate ("Compile universal node constraints") is green on this PR, and the lock picks the node up when this merges. tool_github appears in it because it was locked from a develop push, not from its own PR.

Local verification on cdc66fa: 2408 passed in the full non-dynamic node suite (the one failure is test_lifecycle_order.py, a develop-side C++ change against my older local engine build — green in CI, which rebuilds), 689 passed in packages/ai, ruff clean.

Re-requesting your review.

@joshuadarron
joshuadarron requested a review from asclearuc July 30, 2026 19:59
…pipedrive

# Conflicts:
#	nodes/src/nodes/agent_crewai/services.manager.json

@coderabbitai coderabbitai Bot left a comment

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.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
nodes/src/nodes/agent_crewai/README.md (1)

10-10: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Document Crew Planning in the Manager configuration table.

The README now refers to the Crew Planning field, but the hand-written Manager field table omits it. Add a row describing its advanced-mode visibility, default false, and JSON-output requirement. Update the hand-written section rather than the generated block.

As per path instructions, generated README content must not be hand-edited.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nodes/src/nodes/agent_crewai/README.md` at line 10, Update the hand-written
CrewAI Manager configuration table in README.md to add a Crew Planning row,
documenting that it is visible in advanced mode, defaults to false, and requires
JSON output; do not modify the generated content block.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@nodes/src/nodes/agent_crewai/README.md`:
- Line 10: Update the hand-written CrewAI Manager configuration table in
README.md to add a Crew Planning row, documenting that it is visible in advanced
mode, defaults to false, and requires JSON output; do not modify the generated
content block.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 9e165474-28f8-43be-8a26-b384ed26cd75

📥 Commits

Reviewing files that changed from the base of the PR and between cdc66fa and b095246.

📒 Files selected for processing (3)
  • nodes/src/nodes/agent_crewai/README.md
  • nodes/src/nodes/agent_crewai/crewai_manager/manager.py
  • nodes/src/nodes/agent_crewai/services.manager.json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Documentation module:ai AI/ML modules module:nodes Python pipeline nodes module:ui Chat UI and Dropper UI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(nodes): tool_pipedrive — full Pipedrive API v1 CRM tool

2 participants