Skip to content

feat(nodes): add tool_gohighlevel CRM tool node - #1695

Open
charliegillet wants to merge 3 commits into
developfrom
feat/RR-1676-tool-gohighlevel
Open

feat(nodes): add tool_gohighlevel CRM tool node#1695
charliegillet wants to merge 3 commits into
developfrom
feat/RR-1676-tool-gohighlevel

Conversation

@charliegillet

@charliegillet charliegillet commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Exposes the GoHighLevel (LeadConnector) v2 REST API to agents: 101 tools across 17 resource groups (contacts, notes, tasks, opportunities, pipelines, conversations, messages, calendars, calendar groups, appointments, appointment notes, custom fields, custom values, location tags, locations, businesses, users), plus a generic request tool that reaches any remaining v2 endpoint through the same auth, rate-limit and read-only layer.

Full coverage is more tools than an LLM chooses between reliably, so gohighlevel.toolGroups controls which groups publish. The default ten groups publish 74 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.

Read-only mode hides write tools rather than only refusing them. This differs from tool_pipedrive, which refuses at invoke time. A tool the agent can only fail on is wasted context, so readOnly: true publishes 45 tools instead of 102 and leaks none of the 57 write-stamped tools.

Authentication: Private Integration Token only

No OAuth, deliberately. GoHighLevel refresh tokens are single-use and rotating, verbatim from their OAuth docs: "Once you use a Refresh Token to obtain a new Access Token, the original Refresh Token becomes invalid." Access tokens live 24 hours and a node has nowhere durable to persist the replacement. An OAuth credential would therefore work for ~24h, refresh, discard the new token it cannot store, run another ~24h, then present the invalidated original and die, unattended, with no config-only recovery: re-pasting the burned token does nothing. Adding OAuth needs a durable secret store the node can write on every refresh plus a lock against concurrent runs, which is what HighLevel's own SDK ships MongoDB/Redis session storage for.

Consequence worth naming for whoever supports this: one credential reaches exactly one sub-account. Agency fan-out is refused at the API (/oauth/installedLocations returns 401, /locations/search returns 403, both confirmed live), so N sub-accounts means N node instances. locationId is a required config field because the PIT is opaque and carries no location.

Client details specific to GoHighLevel

Each of these was verified against a live sub-account, not taken from the docs:

  • Version is per operation, never global: 2021-04-15 for calendars and conversations, 2021-07-28 elsewhere. Sending one value globally breaks that whole family with a 4xx that reads like an auth failure. (n8n's connector hardcodes one value and is wrong here.)
  • There is no response envelope. Every endpoint names its own top-level key and the key is not always the resource (/contacts/{id}/appointments returns events), so call() does not unwrap and each tool names its key.
  • GET /opportunities/search requires location_id in snake case and rejects the camelCase form with a 422. Every other endpoint is camelCase.
  • Retry on 429 only. A 403 here means a missing locationId or agency-scope refusal, so retrying it burns budget and hides the real error.
  • Rate-limit budgets are read from headers, never assumed. A trial PIT measured 25 requests per 10s and 10,000/day, against the 100/10s and 200,000/day published for marketplace apps. x-ratelimit-daily-reset is a duration in milliseconds, not an epoch; sleeping on it as a timestamp would produce a ~55-year wait.
  • The 429 carries no Retry-After (triggered deliberately and captured; it arrives on request 26 of a 25-request window), so backoff comes from x-ratelimit-interval-milliseconds.
  • Ids are opaque 20-24 character strings, so id validation is string-based rather than integer-based.
  • Five distinct error body shapes, message sometimes a string and sometimes a list, and a body statusCode that can disagree with the HTTP status (POST /contacts/ answers HTTP 400 carrying statusCode: 422), so status always comes from the response.

Testing

203 unit tests, no credentials and no network. The largest class is TestVersionHeader (86 tests), because a wrong Version is the most expensive silent mistake available on this API.

⚠️ These tests do not run in CI, and neither do any other node tests. I assumed they did and checked before claiming it: _build.yaml has no pytest step, its Test step runs ./builder test (which does not include node pytest), and os-matrix.yml (the workflow that would) is workflow_dispatch plus a monthly cron rather than a per-PR check. Node pytest lives behind builder nodes:test, which no workflow invokes. So the green checks on this PR mean lint, secrets, links, dependency constraints, the shell contract, and a three-OS engine build, not this node's tests. To run them:

./builder nodes:test --pytest-pattern="tool_gohighlevel"

That is a pre-existing repo gap rather than anything this PR introduces, but it is worth a reviewer knowing, and it means the 203 passing tests rest on my local run rather than on CI.

Live suite (test_tools.py, 87 tests) behind GHL_PIT / GHL_LOCATION_ID, with record-creating tests additionally behind GHL_ALLOW_WRITES. Run against a real trial sub-account before this PR went up.

Exercised live, full create/read/update/delete lifecycles: contacts (incl. tags, followers, upsert, duplicate check), contact notes, contact tasks, opportunities (incl. a status transition confirmed by re-read), pipelines, conversations, calendars, appointments, custom fields, custom values, location tags, businesses, users, locations.

The live suite found a real bug that no stubbed test could: appointment_get returned {} for every appointment. GET /calendars/events/appointments/{eventId} answers under appointment, while both published specs declare event. The unwrapper only strips a key the payload carries, so the whole envelope reached the cleaner, which matched none of its read keys and emitted nothing. Fixed, and the fix was then verified end to end against the live API (create calendar, create appointment, read back a populated record, delete both). Every other envelope key in all 101 tools was subsequently audited against the OpenAPI specs.

Not proven, stated plainly

  • Messaging is unproven against the API. message_send, both schedule-cancels, message_transcription_get and message_email_get need a phone or email provider provisioned on the sub-account; a trial has neither.
  • The agency-scoped surface is unproven (locations search, location create/update/delete, companies): refused to a sub-account PIT by design.
  • contact_workflow_add / contact_workflow_remove can never be proven from a sub-account, because GoHighLevel publishes no endpoint that lists workflows, so a valid workflowId cannot be obtained.
  • contact_duplicate_check's 200 body is undocumented in both specs and unattested anywhere; the code reads it tolerantly but the shape is a guess.
  • The node has not been exercised by the real engine: both suites stub rocketlib and ai.common. The three-OS build passes with the node present, but that builds the engine rather than importing this node or registering its tools. The contract suite and the IDE canvas have not been run. This is the main reason it is a draft.
  • Pagination past the first page was not exercised under real volume (one trial account, five seeded contacts).
  • 34 tools on surfaces the live suite could not reach (most of calendars, appointment notes, several message tools) are spec-verified only, and the appointment_get bug shows the specs can be wrong.

Scope, and the Phase 1 framing in the issue

The issue frames Phase 1 as contacts, opportunities, conversations/messages, calendars/appointments. That is 76 of these 101 tools. The other 25 are the id-resolution ring Phase 1 does not work without: contact_update cannot set a custom field without custom_field_list to resolve the field id, and assignedTo needs a user lookup. Breadth is implemented, publication is gated, and the default group set maps onto the issue's Phase 1 list.

186 operations were inventoried; 101 are here. 74 are deferred and 11 are out of scope (9 agency-only with the refusal confirmed live, 2 needing a conversation-provider marketplace app). Suggest tracking the deferred 74 as a checklist on a follow-up issue rather than losing them when this closes #1676.

Also considered: bridging their MCP server

GoHighLevel ships an official MCP server, so it was measured rather than dismissed: 36 tools against 576 raw v2 operations (6.3%), only 19 of which touch the 186 operations above (10.2%). It is sub-account only and gates irreversible writes behind an interactive safety confirmation an unattended pipeline node cannot answer. tool_mcp_client can still be pointed at https://services.leadconnectorhq.com/mcp/ with zero code as a complementary exploratory path, but it does not meet the "no functional limitations" bar in the issue.

Notes for review

  • The request escape hatch has no output_schema, deliberately: it returns whatever the endpoint returns. Flagging in case a permissive schema is wanted anyway.
  • docs/README-nodes.md: the row is appended at the end of the Agent Tools table and the node-count line on line 14 is untouched. It is already wrong on develop and feat(nodes): add tool_pipedrive CRM tool node #1687 also edits it, so touching it buys a conflict for zero accuracy.
  • Overlap with feat(nodes): add tool_pipedrive CRM tool node #1687 (tool_pipedrive) is one CHANGELOG.md bullet. Whichever merges second rebases.
  • Suggested follow-up from the envelope audit: a shared project() helper that raises when a non-empty record projects to nothing. That is the failure mode behind the appointment_get bug, and it is currently indistinguishable from "not found" for about ten read tools. It changes failure semantics across every read tool, so it did not belong in this PR.

Fixes #1676

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added GoHighLevel integration with 101 tools for contacts, opportunities, conversations, calendars, messages, users, and more.
    • Added Private Integration Token authentication and sub-account location support.
    • Added configurable tool groups, read-only mode, pagination, CRUD operations, and an optional generic API request tool.
    • Added support for custom fields, tags, workflows, appointments, pipelines, and message actions.
  • Documentation

    • Added setup, configuration, usage, and troubleshooting guidance.
    • Added the integration to the Agent Tools catalog and unreleased changelog.

Exposes the GoHighLevel (LeadConnector) v2 REST API to agents as 101 tools
across 17 resource groups, plus a generic request tool that reaches any
remaining v2 endpoint through the same auth, rate-limit and read-only layer.

Authentication is a sub-account Private Integration Token only. OAuth is
deliberately absent: GoHighLevel refresh tokens are single-use and rotating,
and a node has nowhere durable to persist the replacement, so an OAuth
credential would work for 24h, refresh, discard the new token, and die
unrecoverably a day later with no config-only recovery.

Full coverage is more tools than an LLM chooses between reliably, so
gohighlevel.toolGroups controls which groups publish; the default ten groups
publish 74 tools. Read-only mode hides every write tool rather than only
refusing it at invoke time, so a read-only agent never sees a tool it cannot
use.

Client details specific to GoHighLevel, each verified against a live
sub-account rather than the published docs:

- Version is a per-operation header, not a global constant: 2021-04-15 for
  calendars and conversations, 2021-07-28 elsewhere.
- There is no response envelope. Every endpoint names its own top-level key,
  and the key is not always the resource, so call() does not unwrap and each
  tool names its key.
- GET /opportunities/search requires location_id in snake case and rejects
  the camelCase form with a 422; every other endpoint is camelCase.
- Retry on 429 only. A 403 means a missing locationId or agency-scope
  refusal, so retrying it burns budget and hides the real error.
- Rate-limit budgets are read from response headers, never assumed: a trial
  token measured 25 requests per 10s and 10,000 per day, against the 100 per
  10s and 200,000 per day published for marketplace apps.
- x-ratelimit-daily-reset is a duration in milliseconds, not an epoch.
- Ids are opaque 20 to 24 character strings, so id validation is string-based.

Tests: 203 unit tests that need no credentials or network, plus an env-gated
live suite behind GHL_PIT and GHL_LOCATION_ID, with record-creating tests
additionally behind GHL_ALLOW_WRITES.

Refs #1676

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

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Added a GoHighLevel API v2 tool node with grouped CRM, messaging, calendar, administrative, and raw-request tools. It includes authentication, pagination, rate-limit handling, read-only controls, configuration, documentation, and offline and live tests.

Changes

GoHighLevel tool node

Layer / File(s) Summary
Node configuration and publication
nodes/src/nodes/tool_gohighlevel/IGlobal.py, nodes/src/nodes/tool_gohighlevel/IInstance.py, nodes/src/nodes/tool_gohighlevel/tool_groups.py, nodes/src/nodes/tool_gohighlevel/services.json
Adds configuration loading, tool-group filtering, read-only enforcement, raw requests, and node metadata.
REST client and shared tool base
nodes/src/nodes/tool_gohighlevel/gohighlevel_client.py, nodes/src/nodes/tool_gohighlevel/tools/_base.py
Adds authenticated requests, structured errors, credential redaction, 429 retries, pagination, response normalization, schemas, and validation.
CRM, conversations, and messages
nodes/src/nodes/tool_gohighlevel/tools/contacts.py, nodes/src/nodes/tool_gohighlevel/tools/opportunities.py, nodes/src/nodes/tool_gohighlevel/tools/conversations.py, nodes/src/nodes/tool_gohighlevel/tools/messages.py
Adds search, CRUD, pagination, relationship operations, messaging, and response shaping.
Calendars and scheduling
nodes/src/nodes/tool_gohighlevel/tools/calendars.py, nodes/src/nodes/tool_gohighlevel/tools/calendar_groups.py, nodes/src/nodes/tool_gohighlevel/tools/appointments.py, nodes/src/nodes/tool_gohighlevel/tools/*notes.py
Adds calendar, appointment, blocked-slot, free-slot, and note operations with endpoint-specific validation.
Sub-account resources
nodes/src/nodes/tool_gohighlevel/tools/businesses.py, nodes/src/nodes/tool_gohighlevel/tools/locations.py, nodes/src/nodes/tool_gohighlevel/tools/users.py, nodes/src/nodes/tool_gohighlevel/tools/custom_*.py, nodes/src/nodes/tool_gohighlevel/tools/location_tags.py, nodes/src/nodes/tool_gohighlevel/tools/contact_tasks.py, nodes/src/nodes/tool_gohighlevel/tools/pipelines.py
Adds business, location, user, pipeline, custom-field/value, tag, and contact-task tools.
Validation and documentation
nodes/test/tool_gohighlevel/*, nodes/src/nodes/tool_gohighlevel/README.md, docs/README-nodes.md, CHANGELOG.md
Adds offline and live tests, node documentation, catalog metadata, and the changelog entry.

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

Possibly related PRs

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

Sequence Diagram(s)

sequenceDiagram
  participant Agent
  participant IInstance
  participant GoHighLevelToolsBase
  participant GoHighLevelAPI
  Agent->>IInstance: invoke published tool
  IInstance->>GoHighLevelToolsBase: validate arguments and access mode
  GoHighLevelToolsBase->>GoHighLevelAPI: send authenticated API request
  GoHighLevelAPI-->>GoHighLevelToolsBase: return response or structured error
  GoHighLevelToolsBase-->>Agent: return normalized result
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 38.37% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the addition of the GoHighLevel CRM tool node.
Linked Issues check ✅ Passed The PR delivers the Phase 1 GoHighLevel node with core CRM tools, configuration, client behavior, documentation, dependencies, and tests.
Out of Scope Changes check ✅ Passed The changes remain within the GoHighLevel CRM integration scope and support the issue's broader resource coverage objective.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/RR-1676-tool-gohighlevel

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 27, 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.

@charliegillet
charliegillet marked this pull request as ready for review July 31, 2026 19:56

@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: 17

🤖 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_gohighlevel/README.md`:
- Line 247: Update the README tool-group headings under “Available tools,”
including appointment_notes, every sibling resource-group heading, and request,
from #### to ### so they use the correct Markdown hierarchy.
- Around line 79-83: Update the pagination count explanation in the README so
contact_list_by_business and lost_reason_list are described as additional tools,
not members of the preceding seven-tool list. Explicitly state that their
documented count field was absent from observed live responses and that they
remain in the null-total list.

In `@nodes/src/nodes/tool_gohighlevel/requirements.txt`:
- Line 1: Update the dependency declaration in requirements.txt to pin requests
to a safe minimum version, such as requests>=2.34.2, while leaving tenacity
unpinned if it is present.

In `@nodes/src/nodes/tool_gohighlevel/services.json`:
- Around line 55-64: Update the items schema for gohighlevel.toolGroups to
include an enum containing all 17 documented group names plus all, matching the
valid values accepted by normalize_groups and preserving the existing default
and optional settings.

In `@nodes/src/nodes/tool_gohighlevel/tools/__init__.py`:
- Around line 58-78: Update the tests around ALL_MIXINS and IInstance to add an
offline assertion that every entry in ALL_MIXINS appears in IInstance.__bases__
before IInstanceBase. Keep the existing whole-surface tool-count test, and use
the declared ALL_MIXINS and IInstance symbols so missing composition entries
fail during testing.

In `@nodes/src/nodes/tool_gohighlevel/tools/_base.py`:
- Around line 627-635: Update the search_after_cursor docstring to state that
the searchAfter value belongs only to styles B and C; remove style E from the
listed styles while leaving the function implementation unchanged.

In `@nodes/src/nodes/tool_gohighlevel/tools/appointments.py`:
- Around line 521-537: Update blocked_slot_create and its input schema to
declare startTime and endTime as required fields, while preserving the existing
calendarId/assignedUserId ownership requirement. Add local validation for both
times before calling _write, matching the required-field handling used by
appointment_create.

In `@nodes/src/nodes/tool_gohighlevel/tools/contacts.py`:
- Around line 297-314: Move the shared response-normalization helpers into
_base.py: add shared _follower_result, _flag_result, total_of, and
upsert_result(payload, key, cleaner) implementations. In
nodes/src/nodes/tool_gohighlevel/tools/contacts.py:297-314 and
opportunities.py:300-318, remove the local follower/flag helpers and import the
shared versions; also replace each local _upsert_result with the shared helper.
In conversations.py:166-172 and messages.py:186-192, replace _total_of with an
import of total_of.

In `@nodes/src/nodes/tool_gohighlevel/tools/messages.py`:
- Around line 223-241: Update _cancel_result so non-dict payloads return an
unconfirmed, non-success result with ok set to false while preserving the
existing status and message defaults; keep the current normalize_success and
HTTP-status handling for dictionary payloads.

In `@nodes/src/nodes/tool_gohighlevel/tools/opportunities.py`:
- Around line 132-135: Update the shared _STATUS_DESC text to describe
lostReasonId as supported only by opportunity_upsert and
opportunity_status_update, while clarifying the create and update tools do not
accept or route that parameter. Keep the existing status descriptions and
lost_reason_list reference intact.

In `@nodes/src/nodes/tool_gohighlevel/tools/users.py`:
- Around line 118-130: Unify the boolean query-parameter handling by moving the
correctly type-guarded _bool_params implementation from
nodes/src/nodes/tool_gohighlevel/tools/pipelines.py:61-74 into
nodes/src/nodes/tool_gohighlevel/tools/_base.py, generalized to accept a single
key. Update nodes/src/nodes/tool_gohighlevel/tools/users.py:118-130 and
pipelines.py:61-74 to import and use the shared helper, ensuring only
isinstance(value, bool) values serialize as lowercase true/false and all other
values are omitted.

In `@nodes/test/tool_gohighlevel/test_gohighlevel.py`:
- Around line 1411-1440: Update _install_stubs to load the real tool_args module
from its source path, following the existing approach in test_tools.py, so the
six normalization and integer-validation tests exercise the shipped helpers
instead of stub_normalize_tool_input and stub_require_int. Preserve the local
stub only for rocketlib and keep the existing test behavior unchanged.

In `@nodes/test/tool_gohighlevel/test_tools.py`:
- Around line 126-138: The live test module leaks scaffolded module state and
the patched request client across the pytest session. In
nodes/test/tool_gohighlevel/test_tools.py lines 126-138, expand the _SCAFFOLD
snapshot/restore coverage to include the imported tool_gohighlevel submodules
and their transitive imports; in lines 184-209, move pacer installation into a
session-scoped fixture and restore the original ghl.requests during teardown.
- Around line 663-675: The lifecycle tests currently assert deletion inside
finally, which can mask failures from the main test body. In
nodes/test/tool_gohighlevel/test_tools.py at lines 663-675, 677-691, 787-797,
799-810, 829-841, 844-855, 871-889, 939-951, and 953-976, keep each delete call
in finally but move its deleted-result assertion after the try/finally block so
the original failure remains reported; apply this to contact_notes_delete,
contact_tasks_delete, location_tags_delete, custom_field_delete,
custom_value_delete, business_delete, calendar_group_delete,
appointment_notes_delete, and the blocked-slot appointment_delete cases.
- Around line 419-445: Update TestListings.test_listing parameterization so each
pytest id includes both the group and tool name, using the existing LISTINGS
group value; preserve the current tool-based identification while adding the
group context to failure output.
- Around line 906-916: Update test_appointment_get_reads_the_right_envelope_key
so note() is called only when appointment_get returns an empty record,
preserving the existing assertion for a successful response. Move the
regression-history explanation into the test docstring and keep note() reserved
for reporting an actual live/code mismatch.
- Around line 1106-1112: The Retry-After lookup in test_no_retry_after_header
must be case-insensitive, so detect the header regardless of capitalization in
LAST_HEADERS. Update test_daily_budget_is_not_exhausted to call
node.location_get({}) before evaluating
ghl.daily_budget_exhausted(LAST_HEADERS), ensuring it uses headers from its own
request.
🪄 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: f25b2a56-13e1-4cff-93d3-824c7379fa1f

📥 Commits

Reviewing files that changed from the base of the PR and between 173048f and 548a40a.

⛔ Files ignored due to path filters (1)
  • nodes/src/nodes/tool_gohighlevel/gohighlevel.svg is excluded by !**/*.svg
📒 Files selected for processing (32)
  • CHANGELOG.md
  • docs/README-nodes.md
  • nodes/src/nodes/tool_gohighlevel/IGlobal.py
  • nodes/src/nodes/tool_gohighlevel/IInstance.py
  • nodes/src/nodes/tool_gohighlevel/README.md
  • nodes/src/nodes/tool_gohighlevel/__init__.py
  • nodes/src/nodes/tool_gohighlevel/gohighlevel_client.py
  • nodes/src/nodes/tool_gohighlevel/requirements.txt
  • nodes/src/nodes/tool_gohighlevel/services.json
  • nodes/src/nodes/tool_gohighlevel/tool_groups.py
  • nodes/src/nodes/tool_gohighlevel/tools/__init__.py
  • nodes/src/nodes/tool_gohighlevel/tools/_base.py
  • nodes/src/nodes/tool_gohighlevel/tools/appointment_notes.py
  • nodes/src/nodes/tool_gohighlevel/tools/appointments.py
  • nodes/src/nodes/tool_gohighlevel/tools/businesses.py
  • nodes/src/nodes/tool_gohighlevel/tools/calendar_groups.py
  • nodes/src/nodes/tool_gohighlevel/tools/calendars.py
  • nodes/src/nodes/tool_gohighlevel/tools/contact_notes.py
  • nodes/src/nodes/tool_gohighlevel/tools/contact_tasks.py
  • nodes/src/nodes/tool_gohighlevel/tools/contacts.py
  • nodes/src/nodes/tool_gohighlevel/tools/conversations.py
  • nodes/src/nodes/tool_gohighlevel/tools/custom_fields.py
  • nodes/src/nodes/tool_gohighlevel/tools/custom_values.py
  • nodes/src/nodes/tool_gohighlevel/tools/location_tags.py
  • nodes/src/nodes/tool_gohighlevel/tools/locations.py
  • nodes/src/nodes/tool_gohighlevel/tools/messages.py
  • nodes/src/nodes/tool_gohighlevel/tools/opportunities.py
  • nodes/src/nodes/tool_gohighlevel/tools/pipelines.py
  • nodes/src/nodes/tool_gohighlevel/tools/users.py
  • nodes/test/tool_gohighlevel/__init__.py
  • nodes/test/tool_gohighlevel/test_gohighlevel.py
  • nodes/test/tool_gohighlevel/test_tools.py

Comment thread nodes/src/nodes/tool_gohighlevel/README.md Outdated
Comment thread nodes/src/nodes/tool_gohighlevel/README.md Outdated
Comment thread nodes/src/nodes/tool_gohighlevel/requirements.txt Outdated
Comment thread nodes/src/nodes/tool_gohighlevel/services.json
Comment thread nodes/src/nodes/tool_gohighlevel/tools/__init__.py
Comment thread nodes/test/tool_gohighlevel/test_tools.py
Comment thread nodes/test/tool_gohighlevel/test_tools.py
Comment thread nodes/test/tool_gohighlevel/test_tools.py Outdated
Comment thread nodes/test/tool_gohighlevel/test_tools.py
Comment thread nodes/test/tool_gohighlevel/test_tools.py
charliegillet and others added 2 commits August 1, 2026 17:50
Source:
- _base.py: shared bool_params (isinstance-guarded), follower_result,
  flag_result, total_of and upsert_result helpers; the per-module copies in
  contacts, opportunities, conversations, messages, users and pipelines are
  gone, which also fixes the truthiness bug in the users copy that serialized
  the string "false" as 'true'
- appointments.py: blocked_slot_create declares and checks startTime/endTime
- messages.py: a non-dict cancel payload now reports ok False, not ok True
- opportunities.py: _STATUS_DESC names the two tools that take lostReasonId
- _base.py: search_after_cursor docstring drops style E, which pages on
  skip/limit and carries no searchAfter

Tests:
- test_gohighlevel.py loads the real tool_args module instead of asserting
  against local stubs, and pins ALL_MIXINS against IInstance.__bases__
- test_tools.py restores the tool_gohighlevel module tree after import and
  installs the request pacer via a session fixture, so the offline suite no
  longer inherits live-suite state; delete assertions moved out of finally
  blocks; note() in the appointment envelope test fires only on a mismatch;
  Retry-After check is case-insensitive; the daily-budget test makes its own
  request; listing test ids carry the group

Docs/config:
- README: tool-group headings h4 -> h3 (MD001), pagination count wording
- requirements.txt: requests>=2.34.2 (credential-leak advisories; this node
  sends a bearer token on every request)
- services.json: toolGroups items constrained by enum

Offline suite: 204 passed. Live suite not run (env-gated).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@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)
docs/README-nodes.md (1)

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

Document the full GoHighLevel tool surface.

Line 124 lists only contacts, opportunities, conversations, and calendars. The node also publishes notes, tasks, messages, pipelines, custom fields, custom values, tags, businesses, locations, and users. Add the remaining groups or state that the list is representative so the catalog does not understate supported operations.

🤖 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 `@docs/README-nodes.md` at line 124, Update the tool_gohighlevel entry in the
nodes documentation to accurately describe the full supported operation surface,
adding notes, tasks, messages, pipelines, custom fields, custom values, tags,
businesses, locations, and users to the existing groups, or explicitly mark the
list as representative.
🤖 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 `@docs/README-nodes.md`:
- Line 124: Update the tool_gohighlevel entry in the nodes documentation to
accurately describe the full supported operation surface, adding notes, tasks,
messages, pipelines, custom fields, custom values, tags, businesses, locations,
and users to the existing groups, or explicitly mark the list as representative.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: e1e93752-ce40-429a-a25c-730c543885ea

📥 Commits

Reviewing files that changed from the base of the PR and between d5178f5 and ee30260.

📒 Files selected for processing (2)
  • CHANGELOG.md
  • docs/README-nodes.md

@joshuadarron joshuadarron 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.

Review

Thanks for the exceptionally candid PR description — the "Not proven, stated plainly" section made this reviewable in a way a tidier write-up would not have been.

I ran things rather than taking them on trust. What I verified independently:

Claim Result
Unit tests pass 204 passed in 1.7s, in a clean venv with only requests/tenacity
Lint ruff check clean
Tool counts ✅ 101 @gohighlevel_tool + request = 102; 57 writes=True → 45 published in read-only. README, services.json and CHANGELOG all agree with the code
Gating really covers tool.invoke rocketlib/filters.py:778 _dispatch_tool calls _collect_tool_methods() for both tool.query and tool.invoke, so the override in IInstance.py:106 genuinely hides and refuses
__tool_meta__ contract (stubbed in the tests) ✅ The real rocketlib.filters.tool_function stamps {'input_schema','description','output_schema'} and returns fn unwrapped, so _declared_schema works against the real framework and not just the stub
The CI-gap claim in your description ✅ Accurate — no workflow under .github/workflows/ references pytest or nodes:test
README.md vs the doc.md in CLAUDE.md README.md is right: 112 nodes use it, 3 use doc.md, and it matches tool_pipedrive
Tool-name literals ✅ AST-scanned all 101 tools — every self._args(args, 'x'), require_id(..., 'x') and tool=/tool_name= label matches its own method name

I also checked request for SSRF: '://' in path is rejected and path.lstrip('/') neutralises a //evil.com prefix, so it cannot escape the base host. Credential hygiene is solid throughout — redaction on every error path, redact_payload on the one tool returning an unprojected body, endGlobal wiping the token, and the token never in the URL or query string.


Why this is REQUEST_CHANGES rather than an approval

The code quality is better than most integration nodes in this repo, and none of what follows is about the code being wrong. Two things are about it being unexercised, and one is a config default that fails in the widening direction.

1. The PR is not a draft, but its description says it should be.

"The node has not been exercised by the real engine: both suites stub rocketlib and ai.commonThis is the main reason it is a draft."

It is currently open with REVIEW_REQUIRED, not draft. That gap is the single largest merge risk here: 10.9k lines that have never been imported through the engine or registered on the canvas. Reading filters.py de-risks the two contracts I could check statically (see the table), but the contract suite and one canvas load would close it properly. Either flip it back to draft or land those runs.

2. messages ships in the default group set while message_send is unproven. Left inline on tool_groups.py.

3. normalize_groups fails open. Left inline on tool_groups.py.

The rest of the inline comments are polish and would not hold a merge on their own.

Not anchorable inline

docs/README-nodes.md:14 still reads 118 node directories → 148 services, now off by one. Your reasoning about buying a conflict with #1687 for zero accuracy is fair and I am not asking you to touch it — but it is worth an issue so it does not sit stale after both land. Same for your suggestion to track the deferred 74 operations as a checklist; that seems clearly right and should not be lost when this closes #1676.

Worth saying plainly

The appointment_get find is the strongest argument in the PR for the live suite existing at all — a tool returning {} with no error, because the unwrapper only strips a key the payload carries and both published specs declare the wrong one, is exactly the class of bug a stubbed test written from the same spec would have confirmed as correct. Your suggested follow-up (a shared project() that raises when a non-empty record projects to nothing) is the right fix and I agree it does not belong in this PR.

Happy to re-review quickly once the engine load is done.

if 'all' in names or '*' in names:
return ALL_GROUPS
selected = names & ALL_GROUPS
return frozenset(selected) if selected else DEFAULT_GROUPS

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.

Fails open: an all-typo toolGroups publishes more than was asked for.

Two spellings of the same operator mistake go in opposite directions:

  • toolGroups: ["contatcs"] — every name unknown → falls back to DEFAULT_GROUPS, i.e. 74 tools across 11 groups, most of them write-capable
  • toolGroups: ["contacts", "opportunites"] — one name good, one typo → silently narrows to contacts alone

The first widens exposure past what the operator asked for, and it is the more likely typo of the two (a single misspelled group). validateConfig warns, but only in the editor — a deployed pipeline reading this config gets nothing at all.

I can see from test_empty_or_wholly_unknown_falls_back_to_the_defaults that this is deliberate rather than an oversight, and the "empty means default" half is clearly right. It is the "configured, matched nothing" half I would separate out:

names = {name.lower() for name in group_names(raw)}
if not names:
    return DEFAULT_GROUPS          # not configured -> defaults
if 'all' in names or '*' in names:
    return ALL_GROUPS
selected = names & ALL_GROUPS
if not selected:
    raise ValueError(...)          # configured, matched nothing -> fail loudly
return frozenset(selected)

beginGlobal already raises for a missing token and a missing locationId, so a hard failure here would be consistent with how the rest of this config is treated. Logging the dropped names at runtime would also cover the partial-typo case, which today narrows silently.

'contacts',
'conversations',
'custom_fields',
'messages',

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.

messages is in the default set, but its write path is unproven against the API.

Your description is explicit that message_send, both schedule-cancels, message_transcription_get and message_email_get could not be exercised — a trial sub-account has no phone or email provider. Every other group in DEFAULT_GROUPS had a live create/read/update/delete lifecycle run against it.

What makes this different from the other 34 spec-verified-only tools is the blast radius of being wrong. A bad read is a 4xx in a log. A bad message_send is a real SMS or email to a real contact, sent by an unattended pipeline, and it cannot be recalled. appointment_get is the existence proof that the specs can be wrong in exactly the silent way that matters.

Two options, either fine by me:

  • drop messages from DEFAULT_GROUPS until the send path has been run against a provisioned sub-account, or
  • split the group so the five read tools stay default and message_send plus the schedule-cancels move to an opt-in group.

Given toolGroups already exists and is well documented, opting in is a low-friction ask for the operator who genuinely wants outbound messaging.

"""
retry_after = _hdr(headers, 'Retry-After')
if retry_after:
return f'retry after {retry_after}s'

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.

Minor: Retry-After is interpolated without checking it is numeric.

RFC 9110 allows either delay-seconds or an HTTP-date, so a date-form header renders as:

retry after Wed, 21 Oct 2026 07:28:00 GMT s

The retry path handles this correctly already — gohighlevel_rate_limit_wait wraps float(retry_after) in a try and falls through on failure. It is only this human-readable hint that assumes seconds. Guarding it keeps the two consistent:

retry_after = _hdr(headers, 'Retry-After')
if retry_after is not None:
    try:
        return f'retry after {float(retry_after):g}s'
    except (TypeError, ValueError):
        pass

Worth noting your own comment two functions down says Retry-After has never actually been observed on a GoHighLevel response, so this is genuinely cosmetic — flagging it only because the header is read in two places that would otherwise disagree about its type.

"description": "Which groups of GoHighLevel tools this node publishes to the agent. This node implements 101 tools across 17 groups, far more than an LLM can choose between reliably, so only the listed groups are exposed. <b>Leave this empty for the recommended set</b>: 74 tools across appointments, calendars, contact_notes, contact_tasks, contacts, conversations, custom_fields, messages, opportunities, pipelines and users, which is everything an agent needs to run lead nurture and appointment booking end to end. Use <b>all</b> to publish all 101. Available groups: appointment_notes, appointments, businesses, calendar_groups, calendars, contact_notes, contact_tasks, contacts, conversations, custom_fields, custom_values, location_tags, locations, messages, opportunities, pipelines, users.",
"items": {
"type": "string",
"enum": ["appointment_notes", "appointments", "businesses", "calendar_groups", "calendars", "contact_notes", "contact_tasks", "contacts", "conversations", "custom_fields", "custom_values", "location_tags", "locations", "messages", "opportunities", "pipelines", "users", "all"]

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.

Three small drifts from tool_pipedrive/services.json, which is the closest sibling and the one an operator is most likely to have seen first:

  1. Bare-string enum. Pipedrive uses [value, label] pairs carrying counts — ["deals", "Deals (27)"]. Labels here would let the picker show both the per-group tool count and which groups are on by default, which is currently information that only exists in the field's prose paragraph and in the README.
  2. No "uniqueItems": true. Pipedrive sets it; nothing here stops an operator listing the same group twice.
  3. default: [] with optional: true. Pipedrive lists its actual defaults, so the editor shows what will be published. Here the field renders empty and the real default set is resolved later by normalize_groups.

Point 3 compounds the fail-open behaviour I flagged on tool_groups.py: the operator sees an empty field, types one group name, misspells it, and silently gets 74 tools instead of the handful they intended — with no visible before-state to notice the difference against.

I read the note in tool_groups.py that the module is the single source of truth for the group names and that services.json deliberately does not repeat the default set. That reasoning holds for the defaults; it does not really apply to uniqueItems or to the labels.

'this one sends what you give it and returns the raw response body. The Authorization and Version '
'headers are added automatically, so do not pass them. Rate-limit retries and read-only enforcement '
'apply here too.'
),

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.

You flagged this yourself in the description, so: I would add a permissive one rather than leave the exemption standing.

test_every_published_tool_has_an_output_schema currently has to special-case request, which means the invariant is enforced for 101 of 102 published tools and the 102nd is enforced by nobody. Something like

output_schema=OBJ(
    'The raw GoHighLevel response body, exactly as the endpoint returned it. '
    'Unlike the typed tools this is not projected through a key allowlist, so the '
    'shape is whatever that endpoint documents.'
),

costs nothing, tells the agent the one thing that actually distinguishes this tool from the other 101 (that the result is not trimmed), and lets the test cover 102/102 with no carve-out.

exist, so losing it silently is worse than losing it loudly. Failing here makes a typo a
hard error on the first invoke of that tool.
"""
if not hasattr(owner, tool):

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.

The guard catches a name that does not exist, but not a name that exists and belongs to a different tool.

self._args(args, 'contact_list') pasted into contact_search would resolve fine, validate the agent's arguments against contact_list's schema, and pass every test in the suite — while silently rejecting searchAfter and accepting startAfterId. Same failure mode the docstring describes, one step subtler.

I checked, and there is no such mismatch today — I AST-scanned all 101 tools for self._args(args, ...), require_id(..., tool), require_text(..., tool) and every tool=/tool_name= keyword, and every literal matches its own method name. So this is purely about pinning it against regression, since the next person adding a group module will copy-paste an existing one.

Roughly:

def test_every_tool_passes_its_own_name_to_args():
    src = Path(inspect.getfile(IInstance)).parent
    for path in sorted(src.rglob('*.py')):
        tree = ast.parse(path.read_text(encoding='utf-8'))
        for cls in (n for n in ast.walk(tree) if isinstance(n, ast.ClassDef)):
            for fn in (n for n in cls.body if isinstance(n, ast.FunctionDef)):
                for call in ast.walk(fn):
                    ...  # assert the literal == fn.name

Happy to hand over the scanner I already wrote if it saves you the fiddly part.

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

Labels

docs Documentation module:nodes Python pipeline nodes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(nodes): tool_gohighlevel — full GoHighLevel API v2 CRM tool

2 participants