feat(datasource intercom): read-only Intercom datasource, phase 1 - #387
feat(datasource intercom): read-only Intercom datasource, phase 1#387christophebrun-forest wants to merge 6 commits into
Conversation
PRD-1112, first lot of the Intercom datasource. Six read-only collections: conversations and tickets on the cursor tier, admins, teams, ticket types and ticket states read whole. Rows, record details, exact counts and the conversation thread work. Server-side filtering (lot 2), writes and actions (lot 3), contacts and companies (lot 4) are refused with a message naming the lot that will answer them, rather than approximated. 254 specs, 100% line coverage, rubocop clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ables (lot 2) (#384) * fix(intercom): page id lookups and report explicit sorts Two findings from the lot 1 review, both places where a cursor collection answers something other than what it was asked for. An `id in [...]` read returned every record it fetched whatever page was asked for, so page 1 and page 2 of a related-record list rendered the same rows. The window is now cut out of the ids before they are read, which also spares the requests the discarded records cost: Intercom reads them one request each. `default_pk_sort?` read a symbol-keyed `false` as an absent key, so an explicit `?sort=-id` was taken for the ascending default the agent injects, and the warning about Intercom silently ignoring a sort never fired. Refs PRD-1118 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…385) * feat(datasource): intercom relations of the reference tier Declares the relations between the collections this datasource already serves, and answers them (lot 2.5, PRD-1145). Publishing a many-to-one is not free: the agent marks it filterable as soon as any column of its target is, so the interface offers `assignee:name` the moment the relation exists. The traversal is therefore the bulk of this change, not the declarations. * eight many-to-one relations towards Admin, Team, TicketState and TicketType, whose targets all shipped with lot 1; * IntercomTeamMembership, synthesized from GET /teams: Intercom carries the membership on both sides and exposes no resource for the pair, while a many-to-many needs a collection to travel through. Read-only, Intercom writing none; * admin_names and team_names replace the arrays of ids on Team and Admin, read only when a projection asks for them; * one label per object on a ticket: state_category and state_external_label are dropped, both being a hop away on the state relation and neither having ever been filterable; * a projection through a relation reads its target once per page; a condition through one is resolved by the target, exactly, and becomes a condition on the foreign key. Over fifteen matching ids, or on a key the endpoint does not filter, it is refused by name rather than approximated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…he relations (lot 4) (#386) * feat(intercom): contacts, companies and contact relations Publishes the two collections phase 2 is walked through, and turns the denormalized contact columns of lot 1 into a relation now that their target exists. Contacts read through the one endpoint of the API that sorts, so the measured table gained a sortable flag and the cursor tier a server-side order; the match-all predicate Tickets already sent is what routes a list view asking for an order through the search. Companies read through the third pagination tier, by offset, which is the one place the window a list view asks for maps onto what Intercom takes. In exchange it is looked up rather than searched, and anything past the two published lookups is refused by name. Contact and company custom attributes are typed from GET /data_attributes and carry api_writable for the lot that writes, though every column here is read-only. Resolving a relation condition is now bounded on the cursor tier: the target is read one record past what a group may hold, rather than walking a whole collection to refuse the fan-out afterwards. contact_email and contact_ids give way to contact_id and the contact relation, one readable label plus navigation, which is the rule lot 2.5 set for the ticket labels. No saved view can rest on them: both were already refused server-side. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
35 new issues
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
| gem 'rspec', '~> 3.0' | ||
| gem 'simplecov', '~> 0.22', require: false | ||
| gem 'webmock', '~> 3.0' | ||
| end |
| end | ||
| end | ||
| end | ||
| end |
| # `list_key` is the key the endpoint puts its records under, and it is not | ||
| # `data` everywhere: `/tickets/search` answers under `tickets` (measured), so | ||
| # a caller names its own and `data` stays the fallback. | ||
| def list_page(path, per_page:, starting_after: nil, params: {}, list_key: 'data', boot: false) |
| # collections that read them never send one, and the parameter is here for | ||
| # the one that does. `{ field:, ascending: }`, translated to Intercom's own | ||
| # spelling on the way out. | ||
| def search_page(path, query:, per_page:, starting_after: nil, list_key: 'data', params: {}, sort: nil) |
| return [0, nil] if wait <= 0 | ||
| return [0, first_warning? ? wait : nil] if wait > @max_wait | ||
|
|
||
| [wait, nil] |
| @limit = limit if limit | ||
| return if remaining.nil? | ||
|
|
||
| @remaining = new_window || @remaining.nil? ? remaining : [remaining, @remaining].min |
| add(union, name, column, definition) | ||
| end | ||
|
|
||
| def add(union, name, column, definition) |
| return entry if entry.name == name | ||
|
|
||
| warn_collision(name, entry.name, column) | ||
| nil |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| gemspec | ||
|
|
||
| gem 'forest_admin_datasource_customizer' | ||
| gem 'forest_admin_datasource_toolkit' |
There was a problem hiding this comment.
🟠 High forest_admin_datasource_intercom/Gemfile:6
Installing the published forest_admin_datasource_intercom gem without this repository's Gemfile fails with LoadError when it requires forest_admin_datasource_toolkit, because Bundler does not install Gemfile-only dependencies for consumers. Declare forest_admin_datasource_toolkit with spec.add_dependency in forest_admin_datasource_intercom.gemspec.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_datasource_intercom/Gemfile around line 6:
Installing the published `forest_admin_datasource_intercom` gem without this repository's `Gemfile` fails with `LoadError` when it requires `forest_admin_datasource_toolkit`, because Bundler does not install Gemfile-only dependencies for consumers. Declare `forest_admin_datasource_toolkit` with `spec.add_dependency` in `forest_admin_datasource_intercom.gemspec`.
| # Rows come back keyed with strings because that is how the agent reads | ||
| # them, while `Aggregation#apply` hands them back keyed with symbols. | ||
| def aggregate(caller, filter, aggregation, limit = nil) | ||
| aggregation.apply(filtered_records(caller, filter), timezone_for(caller), limit) |
There was a problem hiding this comment.
🟡 Medium collections/fetch_all_collection.rb:74
aggregate groups relation paths such as team:name into a single nil group instead of grouping by each related team's name. Unlike list, it passes the un-enriched serialized records directly to Aggregation#apply, so Record.field_value cannot resolve the declared team relation; resolve the relations before applying the aggregation.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/fetch_all_collection.rb around line 74:
`aggregate` groups relation paths such as `team:name` into a single `nil` group instead of grouping by each related team's name. Unlike `list`, it passes the un-enriched serialized records directly to `Aggregation#apply`, so `Record.field_value` cannot resolve the declared `team` relation; resolve the relations before applying the aggregation.
|
|
||
| case tree.operator | ||
| when Operators::EQUAL then [tree.value].compact.map(&:to_s) | ||
| when Operators::IN then Array(tree.value).compact.map(&:to_s) |
There was a problem hiding this comment.
🟡 Medium collections/cursor_collection.rb:300
An id in [...] filter preserves duplicate IDs, so records_by_ids fetches and returns the same record multiple times; aggregate(count) therefore reports duplicates and repeated IDs can consume the 25-read cap before distinct records are read. Deduplicate the normalized IDs before the per-ID fetches.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb around line 300:
An `id in [...]` filter preserves duplicate IDs, so `records_by_ids` fetches and returns the same record multiple times; `aggregate(count)` therefore reports duplicates and repeated IDs can consume the 25-read cap before distinct records are read. Deduplicate the normalized IDs before the per-ID fetches.
| warn_truncated_ids(ids.size) if ids.size > wanted.size | ||
|
|
||
| wanted.each_slice(IDS_PER_READ).flat_map do |chunk| | ||
| client.search_page(search_endpoint.path, per_page: chunk.size, |
There was a problem hiding this comment.
🟡 Medium collections/contact.rb:187
records_by_ids drops the caller's sort, so an id IN [...] contact query returns Intercom's chunk/API order instead of the requested order; pagination can therefore select the wrong contacts, and no ignored-sort warning is emitted. Propagate the accepted sort into each bulk search_page request (or explicitly reject it with the warning) before returning these results.
Also found in 2 other location(s)
packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb:186
When a bare primary-key
id in [...]filter is recognized,fetch_recordsreturns from this line before using itssortargument. A request such as contactsid in [...]sorted by a supported column therefore returns the caller's id order (or endpoint response order), not the requested server sort, and no ignored-sort warning is issued.
packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb:186
A bare primary-key
id IN [...]lookup returns throughrecords_by_idsbeforesortis used. ForIntercomContact, fields such asnameandserver_sortaccepts their sort, but the ID-bulk searches atrecords_by_idsnever receive that sort, so the result is silently returned in Intercom's/default chunk order rather than the requested order.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/contact.rb around line 187:
`records_by_ids` drops the caller's `sort`, so an `id IN [...]` contact query returns Intercom's chunk/API order instead of the requested order; pagination can therefore select the wrong contacts, and no ignored-sort warning is emitted. Propagate the accepted sort into each bulk `search_page` request (or explicitly reject it with the warning) before returning these results.
Also found in 2 other location(s):
- packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb:186 -- When a bare primary-key `id in [...]` filter is recognized, `fetch_records` returns from this line before using its `sort` argument. A request such as contacts `id in [...]` sorted by a supported column therefore returns the caller's id order (or endpoint response order), not the requested server sort, and no ignored-sort warning is issued.
- packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb:186 -- A bare primary-key `id IN [...]` lookup returns through `records_by_ids` before `sort` is used. For `IntercomContact`, fields such as `name` and `email` are advertised sortable and `server_sort` accepts their sort, but the ID-bulk searches at `records_by_ids` never receive that sort, so the result is silently returned in Intercom's/default chunk order rather than the requested order.
| # which is cheaper still. | ||
| def count_records(caller, filter) | ||
| ids = id_lookup(filter) | ||
| return records_by_ids(ids).size if ids |
There was a problem hiding this comment.
🟡 Medium collections/cursor_collection.rb:347
count_records reports at most 25 for an id in [...] filter matching more than MAX_ID_READS existing records, so the advertised exact count is silently incorrect. It delegates to records_by_ids, which truncates the IDs and only logs a warning; count all matching IDs or refuse the oversized count instead of returning the truncated size.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb around line 347:
`count_records` reports at most 25 for an `id in [...]` filter matching more than `MAX_ID_READS` existing records, so the advertised exact count is silently incorrect. It delegates to `records_by_ids`, which truncates the IDs and only logs a warning; count all matching IDs or refuse the oversized count instead of returning the truncated size.
| # `Filter#nest` prefixes the condition tree without prefixing the sort, so | ||
| # the membership is handed the columns of the collection it reaches. | ||
| def sort_clauses(sort) | ||
| known, unknown = Array(sort).partition { |clause| fields.key?(sort_field(clause)) } |
There was a problem hiding this comment.
🟡 Medium collections/fetch_all_collection.rb:184
Sorting by a non-sortable Json field such as team_names, or by a relation field such as team, is silently treated as supported and returns the endpoint's original order. Because these values are nil in the serialized records, sort_clauses never warns or drops the unsupported clause; check that the field is a sortable ColumnSchema before retaining it.
- known, unknown = Array(sort).partition { |clause| fields.key?(sort_field(clause)) }
+ known, unknown = Array(sort).partition do |clause|
+ schema = fields[sort_field(clause)]
+ schema.is_a?(ColumnSchema) && schema.is_sortable
+ end🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/fetch_all_collection.rb around line 184:
Sorting by a non-sortable `Json` field such as `team_names`, or by a relation field such as `team`, is silently treated as supported and returns the endpoint's original order. Because these values are `nil` in the serialized records, `sort_clauses` never warns or drops the unsupported clause; check that the field is a sortable `ColumnSchema` before retaining it.
| and ticket states as Forest collections. | ||
|
|
||
| This is **lot 1: read only**. Rows, record details, exact counts and the conversation thread work; | ||
| server-side filtering, writes, business actions, contacts and companies arrive in the lots after it |
There was a problem hiding this comment.
🟢 Low forest_admin_datasource_intercom/README.md:7
The overview incorrectly tells users that server-side filtering, contacts, and companies are unavailable, even though this release registers those collections and implements filtering. Update the summary to mention only functionality that is actually deferred.
| server-side filtering, writes, business actions, contacts and companies arrive in the lots after it | |
| server-side writes and business actions arrive in the lots after it |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_datasource_intercom/README.md around line 7:
The overview incorrectly tells users that server-side filtering, contacts, and companies are unavailable, even though this release registers those collections and implements filtering. Update the summary to mention only functionality that is actually deferred.
| # Rows come back keyed with strings because that is how the agent reads | ||
| # them, while `Aggregation#apply` hands them back keyed with symbols. | ||
| def aggregate(caller, filter, aggregation, limit = nil) | ||
| aggregation.apply(filtered_records(caller, filter), timezone_for(caller), limit) |
There was a problem hiding this comment.
🟠 High collections/fetch_all_collection.rb:74
A zero-match Count aggregation returns [], causing the value-chart route to dereference result[0]['value'] and raise NoMethodError instead of reporting 0. Aggregation#apply produces no row for the empty dataset and the map preserves that result; return the same zero-count row used by the cursor and offset collections for empty count results.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/fetch_all_collection.rb around line 74:
A zero-match `Count` aggregation returns `[]`, causing the value-chart route to dereference `result[0]['value']` and raise `NoMethodError` instead of reporting `0`. `Aggregation#apply` produces no row for the empty dataset and the `map` preserves that result; return the same zero-count row used by the cursor and offset collections for empty count results.
Review pass on PR #387. Correctness * a relation read slices the fan-out by what the target resolves in one read, and refuses past what it resolves at all, instead of resolving the first 25 rows of a page and leaving the rest nil; * the contacts of an account with a scope or a segment filtered alongside are refused with a message naming that condition, rather than with the table reason telling the operator to read the account's contacts, which is what they asked for; * the `id in [...]` route reports the order it cannot apply, refuses a count larger than a bulk read fetches, and deduplicates its ids; * the cursor walk logs the two defensive stops it took silently; * a date filter naming no day, a time of day or a month, is refused rather than completed from the clock of whichever host ran it; * the offset tier bounds a windowed read by a record budget instead of by the page cap meant for a read with no window of its own; * a projection reaching through two relations is refused by name, like the filter that reaches that deep. Consistency * the four enrichment hooks read the projection through `column_asked?`, so an all-columns read fills the columns it publishes; * `search_fields.yml` fails at load on a column filed as both filterable and refused, and on operators that publish nothing for its type, which is what its own docstring promised; * `active_support` and `active_support/time` are required rather than relied on through an incidental require of the toolkit; * a base_url carrying credentials is masked in every `inspect`, and a cleartext one is reported as carrying the bearer in clear; * `RecordsById` and `AttributeNaming` hold what the paginated tiers and the two introspectors were duplicating, messages included. Docs: nine collections rather than eleven, 19 of 85 table rows measured, free-text search on contacts too, what each relation really costs, and every limit above. 560 examples, 0 failures, 100% line coverage; rubocop clean on 912 files; toolkit, customizer and agent suites green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| raise unless e.status == 404 | ||
|
|
||
| nil | ||
| end |
| !limit.nil? && count > limit | ||
| end | ||
|
|
||
| def read_targets(caller, collection, target, ids, wanted) |
| # caller: a page that looks like the whole answer and is not is the | ||
| # failure this datasource exists to avoid, so there is no route out of | ||
| # this walk that is short and quiet. | ||
| def cut_short?(page, seen_cursors, pages, records:, window:) |
| # truncates, and rendering a local time here would hide the very shift that | ||
| # makes a day-granular date filter wrong. | ||
| def stamp(seconds) | ||
| return nil unless seconds.is_a?(Numeric) && seconds.positive? |
There was a problem hiding this comment.
🟡 Medium collections/base_collection.rb:134
stamp returns nil for the valid Unix-epoch timestamp 0, so dates at 1970-01-01T00:00:00Z are silently serialized without their value. The seconds.positive? check excludes zero; accept non-negative epoch seconds instead.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/base_collection.rb around line 134:
`stamp` returns `nil` for the valid Unix-epoch timestamp `0`, so dates at `1970-01-01T00:00:00Z` are silently serialized without their value. The `seconds.positive?` check excludes zero; accept non-negative epoch seconds instead.
| @remaining -= 1 if @remaining | ||
| return [0, nil] unless exhausted? | ||
|
|
There was a problem hiding this comment.
🟡 Medium forest_admin_datasource_intercom/rate_limiter.rb:97
When observe records remaining: 1, the next acquire sleeps instead of sending the one request still permitted by the window. plan_wait decrements @remaining before checking exhaustion, turning that final slot into zero; check exhaustion first and decrement only when allowing the request.
- @remaining -= 1 if @remaining
- return [0, nil] unless exhausted?
+ if @remaining && @remaining > 0
+ @remaining -= 1
+ return [0, nil]
+ end
+ return [0, nil] unless exhausted?🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/rate_limiter.rb around lines 97-99:
When `observe` records `remaining: 1`, the next `acquire` sleeps instead of sending the one request still permitted by the window. `plan_wait` decrements `@remaining` before checking exhaustion, turning that final slot into zero; check exhaustion first and decrement only when allowing the request.
|
|
||
| entry = union[column] | ||
| return union[column] = attribute_from(name, column, definition) if entry.nil? | ||
| return entry if entry.name == name |
There was a problem hiding this comment.
🟡 Medium schema/ticket_attributes_introspector.rb:93
When two ticket types reuse an attribute name with incompatible column_type values, this returns the first definition and exposes the other ticket's raw value through that schema column, so a Number column can contain a string. Only merge duplicate names when their rendered types match; otherwise omit the incompatible definition.
| return entry if entry.name == name | |
| return entry if entry.name == name && entry.column_type == column_type_for(definition) |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/schema/ticket_attributes_introspector.rb around line 93:
When two ticket types reuse an attribute name with incompatible `column_type` values, this returns the first definition and exposes the other ticket's raw value through that schema column, so a `Number` column can contain a string. Only merge duplicate names when their rendered types match; otherwise omit the incompatible definition.
| lookup = lookup_condition(filter) | ||
| return page_window(looked_up_records(lookup), filter) if lookup | ||
|
|
||
| refuse_condition!(filter.condition_tree) unless filter&.condition_tree.nil? |
There was a problem hiding this comment.
🟠 High collections/offset_collection.rb:120
A nonblank filter.search with no condition returns unrelated companies from listed_records, so a Companies free-text search is presented as if it matched those rows. Because fetch_records only checks condition_tree before the fallback, it bypasses the RecordsById#blank_search? handling used by the cursor tier; reject or otherwise handle nonblank searches before calling listed_records.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/offset_collection.rb around line 120:
A nonblank `filter.search` with no condition returns unrelated companies from `listed_records`, so a Companies free-text search is presented as if it matched those rows. Because `fetch_records` only checks `condition_tree` before the fallback, it bypasses the `RecordsById#blank_search?` handling used by the cursor tier; reject or otherwise handle nonblank searches before calling `listed_records`.
| # for, and `/contacts/search` does not sort on an id anyway. | ||
| def server_sort(filter) | ||
| clauses = Array(filter&.sort) | ||
| return nil if clauses.empty? || default_pk_sort?(clauses) |
There was a problem hiding this comment.
🟡 Medium collections/cursor_collection.rb:350
An explicit ascending id sort is silently ignored: default_pk_sort? treats the parsed ?sort=id clause as the injected default, so server_sort returns nil without calling warn_ignored_sort. Cursor-backed endpoints then return Intercom's native order instead of reporting that the requested order was not applied. Distinguish injected default sorting from an explicit request before suppressing the warning.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb around line 350:
An explicit ascending `id` sort is silently ignored: `default_pk_sort?` treats the parsed `?sort=id` clause as the injected default, so `server_sort` returns `nil` without calling `warn_ignored_sort`. Cursor-backed endpoints then return Intercom's native order instead of reporting that the requested order was not applied. Distinguish injected default sorting from an explicit request before suppressing the warning.
| ForestAdminDatasourceIntercom.logger.warn( | ||
| "[forest_admin_datasource_intercom] base_url #{redacted_url.inspect} is not https, and every request " \ | ||
| 'carries the Intercom access token as a bearer header. Anything on the path can read it and use it ' \ | ||
| 'against the workspace. Use https, or terminate TLS before the network this crosses.' | ||
| ) | ||
| end |
There was a problem hiding this comment.
🔴 Critical forest_admin_datasource_intercom/configuration.rb:131
validate_base_url! accepts non-loopback http:// URLs and only logs a warning, while Client#build_connection sends the reusable workspace bearer token over that connection. Raise ConfigurationError for cleartext non-loopback URLs instead of continuing.
- ForestAdminDatasourceIntercom.logger.warn(
+ raise ConfigurationError,
"[forest_admin_datasource_intercom] base_url #{redacted_url.inspect} is not https, and every request "
'carries the Intercom access token as a bearer header. Anything on the path can read it and use it '
'against the workspace. Use https, or terminate TLS before the network this crosses.'
- )🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/configuration.rb around lines 131-136:
`validate_base_url!` accepts non-loopback `http://` URLs and only logs a warning, while `Client#build_connection` sends the reusable workspace bearer token over that connection. Raise `ConfigurationError` for cleartext non-loopback URLs instead of continuing.
| return count_by_ids(ids) if ids | ||
|
|
||
| lookup = lookup_condition(filter) | ||
| return looked_up_records(lookup).size if lookup |
There was a problem hiding this comment.
🟡 Medium collections/offset_collection.rb:130
count_records returns the first lookup page's .size, so a lookup response with next_cursor reports only that page's count instead of the full total_count. Use the lookup response's total_count (or refuse paginated lookups) so aggregate counts are not silently undercounted.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/offset_collection.rb around line 130:
`count_records` returns the first lookup page's `.size`, so a lookup response with `next_cursor` reports only that page's count instead of the full `total_count`. Use the lookup response's `total_count` (or refuse paginated lookups) so aggregate counts are not silently undercounted.
Second review pass on PR #387. Four defects, each one a place where the package promised something it did not do. * a lookup naming no record is an empty page rather than a 500. Intercom answers `GET /companies?name=` with a 404 where a search endpoint would answer an empty list, and nothing read it: an operator typing an account name that matches nothing broke the page instead of seeing no row. Read the way a record read by its id already was, and only a 404 -- anything else still raises. The related list of an account Intercom no longer answers for reads the same way; * the pinned API version is checked at boot. `Client#me` reads the header Intercom echoes the served version in, and nothing called it: the check the configuration claimed ran was code that never ran, while a workspace serving another version answers payloads of another shape. Called from the datasource on the boot connection, degrading to a warning like the three attribute reads; * the table names every column of the collections it answers for. Seven columns had no row at all -- `state_id` among them, which the README ranks second on the list to probe -- so they fell back on the generic "takes no filter on it", the one refusal an operator cannot act on; and `state_external_label` outlived by two lots the column it refused. A spec now reads the schema against the table in both directions, which is what the reasons are worth; * the probe ships with the gem. `bin/` is excluded from `spec.files`, so the tool the README makes the first step against a customer's workspace reached nobody who had not cloned the repository. It moves to `exe/forest_admin_intercom_probe` -- and with it its guard, RubyGems loading an executable from a stub of its own, where `$PROGRAM_NAME == __FILE__` would have shipped a command that exits without doing anything. Consistency * `ContactIdentity` reads the Contacts path off the table rather than writing it a second time; * the account-contacts route reports an order it cannot apply with the route as the reason and not the column: this is the one collection Intercom does sort; * the attribute families of the table stop being data nothing reads. `Endpoint#attribute_refusal` is validated at load and read by the translator, so a scope filtering on a workspace attribute is refused with the R7 arbitration rather than with the message for a column nobody declared -- those columns being named by the workspace, no row can name them; * the README figures for the measured rows are derived from the file by a spec: 18 of 89, and they cannot drift from it again. 579 examples, 0 failures, 100% line coverage; rubocop clean on 912 files; toolkit, agent and customizer suites green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| end | ||
|
|
||
| print_operators(field, column, result[:operators]) | ||
| end |
| MAX_DEPTH = 2 | ||
| MAX_GROUP_SIZE = 15 | ||
|
|
||
| def self.call(condition_tree, endpoint:, collection:, timezone: nil, attribute_columns: []) |
| # which no row of the table can name: they are discovered at boot. They | ||
| # share one refusal, carried by the endpoint, and passing them here is | ||
| # what lets it be read instead of the generic message. | ||
| def initialize(endpoint:, collection:, timezone: nil, attribute_columns: []) |
| source: spec | ||
| admin_assignee_id: | ||
| field: admin_assignee_id | ||
| type: string |
There was a problem hiding this comment.
🟠 High query/search_fields.yml:76
Conversation filters for admin_assignee_id and team_assignee_id serialize values as JSON strings, so Intercom rejects them with data_invalid because these fields require integers. Change both field types to number. The advertised title filters are also rejected because title is not accepted by POST /conversations/search; move title to refused unless endpoint support is established.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/query/search_fields.yml around line 76:
Conversation filters for `admin_assignee_id` and `team_assignee_id` serialize values as JSON strings, so Intercom rejects them with `data_invalid` because these fields require integers. Change both field types to `number`. The advertised `title` filters are also rejected because `title` is not accepted by `POST /conversations/search`; move `title` to `refused` unless endpoint support is established.
| end | ||
|
|
||
| def write(documents) | ||
| File.write(@options[:out], documents.join("\n")) |
There was a problem hiding this comment.
🟡 Medium exe/forest_admin_intercom_probe:260
--out concatenates the per-endpoint mappings without a YAML document separator, so multiple endpoints emit duplicate root keys and consumers overwrite or ambiguously interpret the evidence. Separate the mappings with --- (and treat the output as a multi-document YAML stream), or write one mapping keyed by endpoint.
- File.write(@options[:out], documents.join("\n"))
+ File.write(@options[:out], documents.join("\n---\n"))🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_datasource_intercom/exe/forest_admin_intercom_probe around line 260:
`--out` concatenates the per-endpoint mappings without a YAML document separator, so multiple endpoints emit duplicate root keys and consumers overwrite or ambiguously interpret the evidence. Separate the mappings with `---` (and treat the output as a multi-document YAML stream), or write one mapping keyed by endpoint.
| read = 0 | ||
|
|
||
| loop do | ||
| answer = read_offset_page(page: page, per_page: per_page) |
There was a problem hiding this comment.
🟡 Medium collections/offset_collection.rb:158
Windowed reads can exceed MAX_COLLECTED_RECORDS by nearly a full page, returning records beyond the intended 7,500-record bound. collect_pages requests the full per_page at line 158 and checks the cap only after appending the response; limit the final request to the remaining record budget.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/offset_collection.rb around line 158:
Windowed reads can exceed `MAX_COLLECTED_RECORDS` by nearly a full page, returning records beyond the intended 7,500-record bound. `collect_pages` requests the full `per_page` at line 158 and checks the cap only after appending the response; limit the final request to the remaining record budget.
1 participant
Integration branch of phase 1 of PRD-1111 — four lots already reviewed and merged here, now going to
maintogether:A new gem,
forest_admin_datasource_intercom, publishing nine read-only collections over the Intercom REST API. Everything outside those four lots — writes on contacts and companies (lot 4b, PRD-1152), actions, tags and segments (lot 5) — is out of scope and refused by name rather than half-served.The rule the whole package is built on
A page that looks filtered, sorted, counted or resolved without being so is the one answer this datasource must not give. Every limit of the API is either translated exactly or refused with a message naming what to change, before the request leaves the process:
pages.nextwhose cursor cannot be read is refused rather than taken for the last page;The single thing that cannot be refused is a sort on the endpoints that ignore one: Intercom accepts it and drops it silently (measured), so those columns are not sortable and a requested order is reported in the log.
POST /contacts/searchis the only endpoint that honours a sort, and the only collection with sortable columns — everywhere else, and on its own read-by-id route, an order asked for is reported rather than dropped.The collections
IntercomConversationGET /conversations,POST /conversations/searchIntercomTicketPOST /tickets/search,GET /tickets/{id}IntercomContactGET /contacts,POST /contacts/search,GET /companies/{id}/contactsIntercomCompanyPOST /companies/listIntercomAdmin,IntercomTeam,IntercomTicketType,IntercomTicketStateIntercomTeamMembershipGET /teamsNineteen relations are declared across the set — assignee, team, state, type, contact, company, owner, plus the two directions of the team membership — and a condition through a relation is resolved by the target itself and rewritten into a foreign-key condition, or refused by name where the endpoint filters no such key.
What a relation costs differs per target, and the README carries the table: the reference tier resolves a whole page for one request,
IntercomContactfor one request per hundred distinct contacts, andIntercomCompanyfor one request per distinct account — Intercom has no bulk read for a company. That last one is therefore the only relation with a ceiling: pastMAX_RELATION_READSdistinct accounts on a single read it is refused by name. Every page size a list view offers sits under it; an export, which batches a thousand rows, does not, and has to leave the column out.The operator table is data, and it says where each row comes from
The table is a committed YAML file, one row per column per endpoint, each carrying its provenance (
measured/spec), and everyfilter_operatorsin the package is derived from it — no collection writes one by hand, so a column cannot advertise a filter the translator would refuse. Every row is validated when the file loads, the script that rewrites it being the reason: an unknown operator or type, a column filed as both filterable and refused, and operators that publish nothing for the column's type all fail at boot rather than producing a schema nobody can explain. That is the mechanism, and it holds whatever the rows say.What the rows say today is mostly
spec: 19 of 85 are measured, and no endpoint has been probed end to end — all three carrymeasured_at: null.bin/probe_search_fieldsis what turns a candidate into a measurement, one search per (field, operator) cell against a real workspace, and running it is the first thing to do on the customer's. The README ranks the rows worth watching; the top of that list isadmin_assignee_id/team_assignee_id, typedstringhere where Intercom documentsInteger— they carry the filter an ops team reaches for first.Several measured points contradict Intercom's own documentation, and are recorded as such:
todayreturn nothing, since the two bounds the toolkit rewrites it into read as "from tomorrow" and "before today";x-ratelimit-limitis 1667 and describes the 10-second window, not the documented 10 000 a minute — the limiter paces on the instantaneous rate from the response headers;/tickets/searchrefusescompany_idwithinvalid_fieldalthough every ticket carries one;per_page=200is refused withinvalid_per_pageinstead of being clamped;statisticsblock, soclosed_atandlast_responderare derived from the parts, which arrive complete in the search response.Privacy
A conversation body is raw personal data. Nothing logs one: logs carry operations, counts, statuses and Intercom's request id. A response that fails to parse is named rather than quoted — a JSON parser opens its message with the characters it choked on, and on a 200 those are the payload.
display_as=plaintexton every conversation read, and the regional host is a first-class configuration parameter.Outside the package
Two changes in
forest_admin_datasource_toolkit, both inUtils::Collectionand both needed by the relation traversal:get_field_schemaraisesValidationErrorinstead ofForestExceptionon an unknown column, an unknown relation or a relation of the wrong type — each names a field the request asked for, which is a 400 the caller can read, not a 500 saying the agent broke. Nothing stops being caught:ValidationErrordescends fromForestException, so everyrescue ForestAdminDatasourceToolkit::Exceptions::ForestExceptionin the repo still catches these three. The observable change is the status —Http::ErrorTranslatormaps a toolkitValidationErroronto 400 — and the toolkit, customizer and agent suites are green on it;list_relationcompacts a related list: a through row whose target the foreign collection no longer answers yielded a nil where every consumer down to the JSON:API serializer reads a row by key.aggregate_relationcounting through those same nil targets is still open, and is mergeable onmainindependently.Verification
bundle exec rubocopclean across the repository, 910 files;version.rbformat, gemspec MFA opt-out,.rubocop.yml,.releaserc.js, and both lists ofbuild.yml.Known, and documented rather than discovered
IntercomContact.GET /companies/{id}/contactsreturns the account's contacts whole and narrows nothing, and/contacts/searchfilters no company field, so there is no request that takes both halves. The refusal names the condition it could not carry alongside the account. Resolving it properly means reading the account's contact ids first and handingid IN [...]plus the rest of the tree to/contacts/search, which that endpoint does answer — and which is not in this lot.company_idcolumn of a contact names the first of its accounts, and thecompanyrelation resolves that same one, whilecompany_id equals Xreturns every contact of X — so an account's related list can show a contact whosecompanypoints elsewhere. The column is the payload's reading, the filter the account endpoint's.The README carries the whole of it: collections, filtering per column with one reason per refusal, relations and what each costs, pagination tiers, rate limiting and configuration.
🤖 Generated with Claude Code
Note
Add read-only Intercom datasource package (phase 1) with 9 collections and query translation
forest_admin_datasource_intercomgem with aDatasource,Configuration, andClientthat handle regional endpoints, API-version verification, rate limiting, and retry policies.IntercomAdmin,IntercomTeam,IntercomTeamMembership,IntercomContact,IntercomCompany,IntercomConversation,IntercomTicket,IntercomTicketState, andIntercomTicketTypeacross three pagination tiers (cursor, offset, fetch-all), each with serializers, relation declarations, and enrichment hooks.ConditionTreeTranslator,FilterValue,OperatorTable,DayBounds,CallerZone, and a YAML-backedSearchFieldsregistry that maps Forest operators to Intercom search syntax.DataAttributesIntrospector,TicketAttributesIntrospector) that discover workspace custom attributes at boot and normalize names, types, and collisions.Collectionvalidation lookups to raiseValidationErrorinstead ofForestException, andlist_relationnow filters out through rows whose foreign target resolves to nil.Collectionfield/relation validation errors changed fromForestExceptiontoValidationErrorin collection.rb; any out-of-tree callers catchingForestExceptionfor missing columns or relations will no longer catch these errors.Macroscope summarized 69ee4f8.