feat(datasource-intercom): tickets and conversations, read-only (lot 1) - #383
Merged
christophebrun-forest merged 9 commits intoSep 1, 2026
Conversation
First PR of lot 1 (PRD-1112): the package skeleton, nothing that talks to Intercom yet. Configuration, the Faraday client and the collections each follow in their own PR, so this one only has to prove the package is wired into rubocop, rspec, coverage and the release before any behaviour rests on it. Registered in the four places a package has to be declared, since a missing entry breaks CI or releases silently rather than loudly: version.rb in the exact format the release sed matches, the gemspec MFA opt-out plus its rubocop excludes, the three spots of .releaserc.js, and both the test matrix and the coverage file list of build.yml. Ships the error hierarchy the rest of the lot leans on: UnsupportedOperatorError descends from the toolkit's ValidationError so a filter Intercom cannot express exactly answers 400 with a message the operator can act on, and APIError carries Intercom's status and parsed body so a smart action can surface its reason instead of an opaque failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Second PR of lot 1 (PRD-1112). Everything a request needs before there is an endpoint to call: where it goes, which API version it asks for, how long it waits, and how it reacts to a refusal. The regional host is a first-class parameter rather than a constant. api.intercom.io does route to the right region, but a workspace under GDPR wants its requests reaching the European host and nothing else, and a base_url also points the client at a mock server or an egress proxy. The API version is pinned. Without the header a request follows the workspace's own default version, which an operator can change on Intercom's side -- and the payloads change shape under us. Intercom echoes the version it served, so the health check compares the two and says so when the pin was not honoured, rather than raising: running against a version we did not ask for still beats not running. Pacing is driven by the response headers, not by a table of budgets. Intercom allocates its quota in 10-second windows -- the measured x-ratelimit-limit is 1667, not the documented 10 000 a minute -- so what matters is the instantaneous rate, and every response says what is left of the current window and when it refills. The limiter waits that reset out; it also counts its own requests down, since several can be in flight before any of them answers. A reset further out than a window is a clock disagreement rather than a window emptying, so the request goes through and the log says why once per window. The 429 retry stays behind the limiter as the backstop for what this process cannot see: the workspace budget is shared with every other private app the customer runs. Only the verbs that change nothing are replayed, plus the 429 on any verb -- Intercom rejects it unprocessed, where a 502 on the way back from a POST it did perform would be replayed into a second reply on a conversation. per_page is bounded before it is sent: Intercom answers invalid_per_page past 150 instead of clamping, so the list view breaks rather than shrinks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Third step of lot 1 (PRD-1112). Forest sends an offset/limit window; Intercom only hands out the page after a cursor and documents that jumping to page N is not supported. The walker bridges the two: it follows cursors until the window is covered, then slices it out. The walk is capped at 50 pages of 150 records -- not for the quota, which is generous, but for what an operator is willing to wait for and because page 200 of a list view answers no real question. Every truncation is logged naming the window it stopped in: a page that looks like the whole answer and is not is the failure this datasource exists to avoid. Records are deduplicated by id inside the walk. Intercom documents that a dataset modified between two paginated requests yields duplicates or missed records, and conversations move constantly -- two rows carrying one id is what a list view renders as two identical lines and a count that never adds up. The missed counterpart is inherent to cursor pagination and gets documented rather than papered over. The client gains the page envelope: records, the cursor the next page advertises, and the exact total_count that will feed Forest's record counter. An advertised next page whose cursor cannot be read is refused rather than taken for the last page -- including the url shape an older API version serves, whose query string is read instead. A `data` that is absent or is not a list is refused too: read as an empty page, it would hand the collection rows built out of envelope keys. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fourth step of lot 1 (PRD-1112). Admins, teams, ticket types and ticket states: the collections that turn an assignee id into a teammate and a state id into a label, which is what the conversation and ticket rows of the next steps need to be readable at all. Their endpoints answer whole -- no pagination parameter, no filter, no sort -- which paradoxically makes this the most capable tier of the datasource. Filtering, sorting, paging and counting them in memory is exact rather than approximate, because the records in hand are every record Intercom holds: a window cut out of them carries the rows a server-side query would have returned. It is the one place where an in-memory pass does not risk the thing refused everywhere else, a result that looks filtered without being filtered, which only arises when what one holds is a page of something larger. So these are the only countable and groupable collections of the lot, and the cost is bandwidth. A condition the tier cannot evaluate is refused rather than applied: `match` answers nil for an operator with no in-memory equivalence and `apply` reads that as "no match", so it would otherwise hand back an empty page an operator cannot tell from a real answer. The schema advertises no such operator -- every advertised one is re-checked against the toolkit rather than trusted -- but a scope, a segment or a customizer can still send one. Two Intercom particulars, both measured rather than assumed: /admins and /teams put their records under their own key where /ticket_types uses the `data` envelope, so the collection names its key and `data` is the fallback; and a team id is a string on the team and a number inside `admin_ids`, so ids are stringified or a filter value from Forest would never match. `fetch_all` also follows a cursor if one is advertised, since no pagination parameter in the specification is not a promise that a large workspace answers in one response. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fifth step of lot 1 (PRD-1112). The collection an ops team actually works in: what is open, assigned to whom, since when, and what the customer wrote. Cursor collections get their own tier, the opposite of the reference one in every respect: what is in hand is a page of something far larger, so nothing may be filtered, sorted or counted in memory without answering a fraction as if it were the whole. Three routes and no fourth -- no condition walks the listing, `id equals X` reads the record through its own endpoint, and anything else is refused with a message naming the lot that will answer it. Every column ships unfilterable and unsortable for that reason; only the primary key advertises operators, and it is answered by the record endpoint rather than by a filter. Counting is the exception that costs nothing: `total_count` is exact on every response, so the record counter is one request. A group-by is refused rather than computed over the pages a walk collected, which would look exact while answering a fraction. The order Intercom silently drops is reported. A sort sent to these endpoints raises nothing and changes nothing -- measured -- so an operator who asked for an order and did not get one learns it here or nowhere. The timeline opens on `source`, not on the parts: the message that started the conversation lives there, and a thread built from the parts alone loses exactly the one nobody opens a conversation without wanting to read. Every entry keeps its `part_type`, an assignment and a reply being different events. Since Intercom returns the parts only when retrieving a single conversation, a record read gets its timeline for free while a list view pays a request per row: bounded to ten, and the rows past it keep a nil that reads as unknown rather than an empty thread. Contact identity is denormalized onto the row by one bulk read per page rather than a lookup per row, and a failure there costs the two columns instead of the page. It stays a pair of columns rather than a relation: the Contacts collection arrives in lot 4, and a relation whose target is missing is a schema the agent refuses to boot on. Two Intercom particulars handled on the way: the records come under `conversations` rather than the `data` envelope, so `list_page` now takes a `list_key` like `fetch_all` does, and dates travel as epoch seconds, read back as ISO8601 in UTC -- where Intercom stores and truncates, and where a local rendering would hide the shift that makes a day-granular date filter wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sixth step of lot 1 (PRD-1112), and the one the measurements on a real
workspace of 81 142 tickets reshaped.
There is no GET /tickets at all, so even an unfiltered list view goes
through POST /tickets/search with a predicate matching everything, and
its records come back under `tickets` rather than the `data` envelope.
The cursor tier grew a `read_page` hook for that: walking a page is the
base's business, which endpoint answers it is the collection's.
The page size is bounded at 25, far below the 150 the API accepts. Not
caution: a ticket carries its whole timeline in the search response and
Intercom offers no field selection, so a page of 150 would move some 23
000 part objects. The figure is provisional until measured against real
response sizes on the customer's workspace.
That same payload is what makes the two derived columns defensible. A
ticket has no `statistics` block -- measured, confirming the
specification -- so neither a closure date nor a last responder exists
as a field, but both are in the parts, which are paid for whether or not
anything asks:
* `closed_at` and `closed_by_name` come from the last transition into
a state of category `resolved`. Matched on the `ticket_state_updated`
prefix rather than the `_by_admin` variant the sample showed, since
this workspace runs workflows and a closure done by automation would
otherwise be invisible; transitions whose target equals the previous
state are ignored, because they exist.
* `last_reply_at`, `last_responder_name` and `last_responder_type`
come from the last `comment` part, notes excluded: an internal note
is a touch, not an answer to the person waiting.
Both ship display-only, and that is not temporary: the search endpoint
filters neither and ignores a sort without reporting it, so a column
advertising either would put in the interface what the read cannot
honour. When a resolved ticket's transition fell past Intercom's
500-part ceiling the value is unknown rather than absent -- detected by
comparing the parts in hand with their total, and reported in a log
since a Date column cannot say it.
The ticket-type attributes are introspected once at boot and published
as the union of every type's, keyed by name the way the payload is. The
id each type gives the same name is kept even though nothing uses it
yet: it is what the filter translation will need, and re-reading it
would cost a second boot round trip. A token without that permission
costs the attribute columns, never the boot. An attribute whose name a
native column already carries is skipped rather than overwriting it.
The state arrives embedded as a whole object, so its labels cost
nothing, and the contact identity denormalization moved to a module both
collections now share.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Seventh and last step of lot 1 (PRD-1112): the README, and the one hardening the review of the logs turned up. A response that fails to parse used to travel into the error message, and a JSON parser opens its message with the characters it choked on. On a 4xx those are Intercom's error text, which is what an operator needs; on a 200 they are the payload -- a conversation body, most of the time. The failure is now named rather than quoted, at both levels: the Faraday parsing error, and a parser raising on its own, which would otherwise have reached the catch-all whose message is the exception's. The rest of the log review came back clean -- operations, counts, statuses and Intercom request ids, never content. The README documents what this datasource refuses and why, since that is the part an operator meets first: no offset pagination, a sort accepted and ignored, no aggregate endpoint, `per_page` refused past 150 and bounded at 25 for tickets, no `GET /tickets` at all, and an envelope key that is not always `data`. Then the two tiers and why they behave differently, the derived ticket columns with the 500-part ceiling that makes a closure date unknown rather than absent, the rate-limit windows, the privacy rules, and the single read a boot performs. Every figure in it was checked against the constants rather than remembered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
23 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 |
| # `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) |
| # One page of a search endpoint. The query is written by the caller rather | ||
| # than translated from a Forest filter -- that translation is lot 2 -- so | ||
| # what goes on the wire is what the caller asked for. | ||
| def search_page(path, query:, per_page:, starting_after: nil, list_key: 'data') |
| # One record from its own endpoint. Raises on a 404 like on any other | ||
| # failure: what a missing record means -- a stale link, a record outside the | ||
| # token's scope, a deletion -- is the caller's to decide, not the client's. | ||
| def fetch_record(path, id, params: {}, boot: false) |
| end | ||
| end | ||
|
|
||
| records |
| cursor = page.next_cursor | ||
| end | ||
|
|
||
| records |
| Client.bounded_per_page(budget) | ||
| end | ||
|
|
||
| def log_truncation(offset:, limit:, pages:, collected:) |
| 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 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| '( cd packages/forest_admin_datasource_snowflake && gem build && gem push forest_admin_datasource_snowflake-*.gem );' + | ||
| '( cd packages/forest_admin_datasource_mambu_payments && gem build && gem push forest_admin_datasource_mambu_payments-*.gem );' + | ||
| '( cd packages/forest_admin_datasource_graphql_hasura && gem build && gem push forest_admin_datasource_graphql_hasura-*.gem );' , | ||
| '( cd packages/forest_admin_datasource_graphql_hasura && gem build && gem push forest_admin_datasource_graphql_hasura-*.gem );' + |
There was a problem hiding this comment.
🟠 High .releaserc.js:49
The release can report success while forest_admin_datasource_graphql_hasura was not updated or published. Because the Hasura and Intercom commands are separated with ;, a failed Hasura sed, build, or push is overwritten by a successful Intercom command and successCmd/prepareCmd exits 0; chain each command with && (or enable fail-fast) so the failure propagates.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @.releaserc.js around line 49:
The release can report success while `forest_admin_datasource_graphql_hasura` was not updated or published. Because the Hasura and Intercom commands are separated with `;`, a failed Hasura `sed`, build, or push is overwritten by a successful Intercom command and `successCmd`/`prepareCmd` exits 0; chain each command with `&&` (or enable fail-fast) so the failure propagates.
| private | ||
|
|
||
| def fetch_records(filter) | ||
| ids = id_lookup(filter) |
There was a problem hiding this comment.
🟡 Medium collections/cursor_collection.rb:107
Primary-key equal/in list requests ignore filter.page, so id in [1, 2, 3] with offset: 1, limit: 1 returns all three records instead of one. The early return from fetch_records bypasses pagination; apply translate_page to the records returned by records_by_ids.
🚀 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 107:
Primary-key `equal`/`in` list requests ignore `filter.page`, so `id in [1, 2, 3]` with `offset: 1, limit: 1` returns all three records instead of one. The early return from `fetch_records` bypasses pagination; apply `translate_page` to the records returned by `records_by_ids`.
| # reason -- together they cut every path from an object this package hands | ||
| # out to the credential. | ||
| def inspect | ||
| "#<#{self.class.name} url=#{url.inspect} api_version=#{@api_version.inspect} access_token=[FILTERED]>" |
There was a problem hiding this comment.
🟠 High forest_admin_datasource_intercom/configuration.rb:70
Configuration#inspect exposes a proxy password whenever base_url contains URI userinfo, so logging or rendering a valid URL such as https://proxy-user:proxy-password@proxy.example/api leaks credentials despite the method's redaction boundary. Because url.inspect is interpolated verbatim, redact the URI userinfo before including the URL.
- "#<#{self.class.name} url=#{url.inspect} api_version=#{@api_version.inspect} access_token=[FILTERED]>"
+ "#<#{self.class.name} url=#{URI.parse(url).tap { |uri| uri.userinfo = '[FILTERED]' if uri.userinfo }.to_s.inspect} api_version=#{@api_version.inspect} access_token=[FILTERED]>"🚀 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 line 70:
`Configuration#inspect` exposes a proxy password whenever `base_url` contains URI userinfo, so logging or rendering a valid URL such as `https://proxy-user:proxy-password@proxy.example/api` leaks credentials despite the method's redaction boundary. Because `url.inspect` is interpolated verbatim, redact the URI userinfo before including the URL.
Comment on lines
+313
to
+321
| message = join_errors(parsed['errors']) | ||
| message = parsed.to_json if message.empty? | ||
|
|
||
| append_request_id(message[0, 500], parsed['request_id']) | ||
| end | ||
|
|
||
| def join_errors(errors) | ||
| Array(errors).filter_map do |error| | ||
| next error unless error.is_a?(Hash) |
There was a problem hiding this comment.
🟠 High forest_admin_datasource_intercom/client.rb:313
Malformed JSON error bodies are copied into APIError#message, exposing conversation or other personal data to the UI and error collectors. join_errors returns non-Hash entries verbatim, and the fallback serializes any hash without usable errors; skip non-Hash entries and use a size-only summary instead of parsed.to_json.
message = join_errors(parsed['errors'])
- message = parsed.to_json if message.empty?
+ message = "(unreadable body, #{parsed.to_json.bytesize} bytes)" if message.empty?
append_request_id(message[0, 500], parsed['request_id'])
@@
Array(errors).filter_map do |error|
- next error unless error.is_a?(Hash)
+ next unless error.is_a?(Hash)
[error['code'], error['message']].compact.join(': ')🚀 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/client.rb around lines 313-321:
Malformed JSON error bodies are copied into `APIError#message`, exposing conversation or other personal data to the UI and error collectors. `join_errors` returns non-Hash entries verbatim, and the fallback serializes any hash without usable errors; skip non-Hash entries and use a size-only summary instead of `parsed.to_json`.
Comment on lines
+119
to
+121
| def blank_search?(filter) | ||
| search = filter.respond_to?(:search) ? filter.search : nil | ||
| search.nil? || search.to_s.strip.empty? |
There was a problem hiding this comment.
🟡 Medium collections/cursor_collection.rb:119
Requests carrying a nonempty filter.search_extended return incorrect results: an id = X request returns X without applying the extended search, while a list request returns an unfiltered page instead of refusing the unsupported search. Both id_lookup and browsing? rely on blank_search?, which checks only filter.search; include filter.search_extended in that check so these requests are rejected.
def blank_search?(filter)
- search = filter.respond_to?(:search) ? filter.search : nil
- search.nil? || search.to_s.strip.empty?
+ search = filter.respond_to?(:search) ? filter.search : nil
+ extended_search = filter.respond_to?(:search_extended) ? filter.search_extended : nil
+ [search, extended_search].all? { |value| value.nil? || value.to_s.strip.empty? }🚀 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 lines 119-121:
Requests carrying a nonempty `filter.search_extended` return incorrect results: an `id = X` request returns `X` without applying the extended search, while a list request returns an unfiltered page instead of refusing the unsupported search. Both `id_lookup` and `browsing?` rely on `blank_search?`, which checks only `filter.search`; include `filter.search_extended` in that check so these requests are rejected.
| spec.add_dependency 'activesupport', '>= 6.1' | ||
| spec.add_dependency 'faraday', '~> 2.0' | ||
| spec.add_dependency 'faraday-retry', '~> 2.0' | ||
| spec.add_dependency 'zeitwerk', '~> 2.3' |
There was a problem hiding this comment.
🟠 High forest_admin_datasource_intercom/forest_admin_datasource_intercom.gemspec:35
Installing the published gem without the development Gemfile causes require 'forest_admin_datasource_intercom' to raise LoadError because the entrypoint unconditionally requires forest_admin_datasource_toolkit. Declare forest_admin_datasource_toolkit as a runtime dependency in the gemspec so RubyGems installs it for consumers.
spec.add_dependency 'zeitwerk', '~> 2.3'
+ spec.add_dependency 'forest_admin_datasource_toolkit'Also found in 1 other location(s)
packages/forest_admin_datasource_intercom/Gemfile:6
forest_admin_datasource_toolkitis added only to this developmentGemfile, not to the gemspec. The published gem's entrypoint unconditionallyrequires it, so an application that installsforest_admin_datasource_intercomfrom RubyGems will not receive the toolkit dependency and fails withLoadErrorwhen loading the datasource.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_datasource_intercom/forest_admin_datasource_intercom.gemspec around line 35:
Installing the published gem without the development `Gemfile` causes `require 'forest_admin_datasource_intercom'` to raise `LoadError` because the entrypoint unconditionally requires `forest_admin_datasource_toolkit`. Declare `forest_admin_datasource_toolkit` as a runtime dependency in the gemspec so RubyGems installs it for consumers.
Also found in 1 other location(s):
- packages/forest_admin_datasource_intercom/Gemfile:6 -- `forest_admin_datasource_toolkit` is added only to this development `Gemfile`, not to the gemspec. The published gem's entrypoint unconditionally `require`s it, so an application that installs `forest_admin_datasource_intercom` from RubyGems will not receive the toolkit dependency and fails with `LoadError` when loading the datasource.
| return if @base_url.nil? | ||
|
|
||
| uri = URI.parse(@base_url) | ||
| return if uri.is_a?(URI::HTTP) && !blank?(uri.host) |
There was a problem hiding this comment.
🟠 High forest_admin_datasource_intercom/configuration.rb:99
validate_base_url! accepts http:// URLs, so Client#build_connection sends the bearer token and conversation data unencrypted to an HTTP endpoint, allowing network observers to steal the credential and read responses. Require URI::HTTPS here, or make HTTP an explicit opt-in for test-only endpoints.
Suggested change
| return if uri.is_a?(URI::HTTP) && !blank?(uri.host) | |
| return if uri.is_a?(URI::HTTPS) && !blank?(uri.host) |
🚀 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 line 99:
`validate_base_url!` accepts `http://` URLs, so `Client#build_connection` sends the bearer token and conversation data unencrypted to an HTTP endpoint, allowing network observers to steal the credential and read responses. Require `URI::HTTPS` here, or make HTTP an explicit opt-in for test-only endpoints.
| wanted = ids.first(MAX_ID_READS) | ||
| warn_truncated_ids(ids.size) if ids.size > wanted.size | ||
|
|
||
| wanted.filter_map do |id| |
There was a problem hiding this comment.
🟡 Medium collections/cursor_collection.rb:151
Primary-key IN list responses preserve the caller-supplied ID order instead of the effective ascending id order, so id IN ['3','1','2'] returns 3,1,2 and silently violates the default ordering. records_by_ids iterates ids directly, while warn_ignored_sort suppresses the warning for this injected default sort; sort the IDs before fetching.
🚀 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 151:
Primary-key `IN` list responses preserve the caller-supplied ID order instead of the effective ascending `id` order, so `id IN ['3','1','2']` returns `3,1,2` and silently violates the default ordering. `records_by_ids` iterates `ids` directly, while `warn_ignored_sort` suppresses the warning for this injected default sort; sort the IDs before fetching.
Found on a real workspace: the ticket list answered 400 in five milliseconds, before any call to Intercom. Forest lists the fields of a request in a comma-separated query parameter, and a workspace names its ticket attributes in free text. One of them is called "ID de l'objet en question (immo, facture, user)": the agent splits that on the commas, gets three fields no collection has, and rejects the projection. Nothing was wrong with the read -- the count, which sends no field list, answered 200 the whole time. The introspector now derives a column name from the workspace's own: the commas and colons that are separators in Forest's protocol become spaces, and the HTML escaping Intercom hands back -- `j'ai` -- is undone, since that is an artefact of where the name was typed rather than part of it. The payload key stays the workspace's name, because that is what `ticket_attributes` is keyed by; only the schema sees the derived one. Two attributes reading as the same column would share an entry and the second's values would be read under the first's name, which is worse than missing them: the second is left out with a log line naming both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| return entry if entry.name == name | ||
|
|
||
| warn_collision(name, entry.name, column) | ||
| nil |
|
|
||
| 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:100
Same-named attributes with different data_types are merged into one Attribute, so the first definition's schema is applied to values from every ticket type; this produces incorrect types and leaves epoch date values unconverted when the retained type is not Date. Only merge the entry when the data_type also matches, otherwise treat it as a collision and omit the conflicting definition.
- return entry if entry.name == name
+ return entry if entry.name == name && entry.data_type == definition['data_type']🚀 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 100:
Same-named attributes with different `data_type`s are merged into one `Attribute`, so the first definition's schema is applied to values from every ticket type; this produces incorrect types and leaves epoch date values unconverted when the retained type is not `Date`. Only merge the entry when the `data_type` also matches, otherwise treat it as a collision and omit the conflicting definition.
| # attribute columns, never the boot of the agent. | ||
| def attributes | ||
| @attributes ||= build | ||
| rescue APIError => e |
There was a problem hiding this comment.
🟡 Medium schema/ticket_attributes_introspector.rb:55
A transient APIError during fetch_all('ticket_types', boot: true) permanently memoizes attributes as [], so the IntercomTicket schema boots without custom attribute columns until restart. Because Client#fetch_all also maps timeouts, 5xx responses, and malformed responses to APIError, this rescues non-authorization failures as if permission were missing; retry introspection or propagate those failures instead of caching the empty result.
🚀 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 55:
A transient `APIError` during `fetch_all('ticket_types', boot: true)` permanently memoizes `attributes` as `[]`, so the `IntercomTicket` schema boots without custom attribute columns until restart. Because `Client#fetch_all` also maps timeouts, 5xx responses, and malformed responses to `APIError`, this rescues non-authorization failures as if permission were missing; retry introspection or propagate those failures instead of caching the empty result.
A lot that writes nothing was publishing editable columns: neither add_column set is_read_only, so the six collections offered a Save and a Delete in the interface, both reaching the update and delete the collections do not implement -- a 500 where the schema should simply not have offered the button. The two tiers already refuse on the read side what Intercom cannot honour; this is the same rule on the write side. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| IN_MEMORY_OPERATORS = [Operators::IN, Operators::EQUAL, Operators::LESS_THAN, Operators::GREATER_THAN, | ||
| Operators::MATCH, Operators::STARTS_WITH, Operators::ENDS_WITH, | ||
| Operators::LONGER_THAN, Operators::SHORTER_THAN, Operators::INCLUDES_ALL, | ||
| Operators::NOT_IN, Operators::NOT_EQUAL, Operators::NOT_CONTAINS].freeze |
There was a problem hiding this comment.
🟠 High collections/fetch_all_collection.rb:39
NOT_CONTAINS filters return every record, including records containing the requested text. Listing NOT_CONTAINS in IN_MEMORY_OPERATORS makes ConditionTreeLeaf#match treat its nested non-native contains result of nil as true; remove it from the advertised native operators until a valid equivalence is provided.
- Operators::NOT_IN, Operators::NOT_EQUAL, Operators::NOT_CONTAINS].freeze
+ Operators::NOT_IN, Operators::NOT_EQUAL].freeze🚀 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 39:
`NOT_CONTAINS` filters return every record, including records containing the requested text. Listing `NOT_CONTAINS` in `IN_MEMORY_OPERATORS` makes `ConditionTreeLeaf#match` treat its nested non-native `contains` result of `nil` as true; remove it from the advertised native operators until a valid equivalence is provided.
Comment on lines
+148
to
+152
| cursor = nil | ||
| pages = 0 | ||
|
|
||
| loop do | ||
| body = get(path, cursor.nil? ? nil : { 'starting_after' => cursor }, boot: boot).body |
There was a problem hiding this comment.
🟡 Medium forest_admin_datasource_intercom/client.rb:148
fetch_all requests a repeated pages.next.starting_after cursor and appends that page's records again, so malformed pagination returns duplicate reference rows before the collection cap stops it. Track already-followed cursors and reject a repeat before issuing another request, as CursorWalker does.
pages = 0
+ seen_cursors = []
loop do
+ refuse_body_shape(path, "'pages.next' repeats a cursor") if seen_cursors.include?(cursor)
+ seen_cursors << cursor
body = get(path, cursor.nil? ? nil : { 'starting_after' => cursor }, boot: boot).body🚀 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/client.rb around lines 148-152:
`fetch_all` requests a repeated `pages.next.starting_after` cursor and appends that page's records again, so malformed pagination returns duplicate reference rows before the collection cap stops it. Track already-followed cursors and reject a repeat before issuing another request, as `CursorWalker` does.
| return records if page.nil? | ||
|
|
||
| offset = page.offset.to_i.clamp(0, nil) | ||
| limit = page.limit.to_i |
There was a problem hiding this comment.
🟠 High collections/base_collection.rb:51
A page with an offset and limit: nil raises NoMethodError instead of returning all records after the offset. page.limit.to_i dereferences nil, so handle a missing limit as unbounded before converting it.
Suggested change
| limit = page.limit.to_i | |
| limit = page.limit&.to_i |
🚀 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 51:
A page with an offset and `limit: nil` raises `NoMethodError` instead of returning all records after the offset. `page.limit.to_i` dereferences `nil`, so handle a missing limit as unbounded before converting it.
| # 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:70
aggregate accepts Sum and Avg for String columns and returns 0 instead of rejecting the invalid aggregation. Aggregation#apply counts truthy strings while adding only Numeric values, so charts for fields such as name, email, or id display a plausible but false result; validate the aggregation against the column type before calling apply.
🚀 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 70:
`aggregate` accepts `Sum` and `Avg` for String columns and returns `0` instead of rejecting the invalid aggregation. `Aggregation#apply` counts truthy strings while adding only Numeric values, so charts for fields such as `name`, `email`, or `id` display a plausible but false result; validate the aggregation against the column type before calling `apply`.
Comment on lines
+84
to
+85
| @remaining -= 1 if @remaining | ||
| return [0, nil] unless exhausted? |
There was a problem hiding this comment.
🟡 Medium forest_admin_datasource_intercom/rate_limiter.rb:84
acquire sleeps when X-RateLimit-Remaining is 1, withholding the final request that Intercom says is still available and unnecessarily delaying operations. plan_wait decrements @remaining before testing exhaustion, turning 1 into 0; check the pre-decrement exhaustion state while still decrementing the local budget for the request being admitted.
- @remaining -= 1 if @remaining
- return [0, nil] unless exhausted?
+ was_exhausted = exhausted?
+ @remaining -= 1 if @remaining
+ return [0, nil] unless was_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 84-85:
`acquire` sleeps when `X-RateLimit-Remaining` is `1`, withholding the final request that Intercom says is still available and unnecessarily delaying operations. `plan_wait` decrements `@remaining` before testing exhaustion, turning `1` into `0`; check the pre-decrement exhaustion state while still decrementing the local budget for the request being admitted.
| # one page would otherwise collect the same pages until a cap cut it | ||
| # short. | ||
| def stop?(page, seen_cursors) | ||
| page.next_cursor.nil? || page.records.empty? || !seen_cursors.add?(page.next_cursor) |
There was a problem hiding this comment.
🟠 High pagination/cursor_walker.rb:100
An empty intermediate page causes walk to return the records collected so far, silently omitting records available through page.next_cursor. Because stop? breaks before the cap check, this truncation is not logged; follow a non-nil cursor even when the current page has no records.
- page.next_cursor.nil? || page.records.empty? || !seen_cursors.add?(page.next_cursor)
+ page.next_cursor.nil? || !seen_cursors.add?(page.next_cursor)🚀 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/pagination/cursor_walker.rb around line 100:
An empty intermediate page causes `walk` to return the records collected so far, silently omitting records available through `page.next_cursor`. Because `stop?` breaks before the cap check, this truncation is not logged; follow a non-nil cursor even when the current page has no records.
| def filtered_records(caller, filter) | ||
| records = fetch_all.map { |entity| serialize(entity) } | ||
| tree = filter&.condition_tree | ||
| return records if tree.nil? |
There was a problem hiding this comment.
🟠 High collections/fetch_all_collection.rb:106
Free-text searches on these collections return and count every endpoint row instead of only matching rows. filtered_records checks only filter.condition_tree, so it ignores filter.search and filter.search_extended when no condition tree exists; apply the search or explicitly reject unsupported searches, as the cursor tier does.
🚀 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 106:
Free-text searches on these collections return and count every endpoint row instead of only matching rows. `filtered_records` checks only `filter.condition_tree`, so it ignores `filter.search` and `filter.search_extended` when no condition tree exists; apply the search or explicitly reject unsupported searches, as the cursor tier does.
| return [0, nil] unless exhausted? | ||
|
|
||
| wait = clamp_wait(@reset_at - @now.call) | ||
| return [0, nil] if wait <= 0 |
There was a problem hiding this comment.
🟡 Medium forest_admin_datasource_intercom/rate_limiter.rb:88
After @reset_at has passed, concurrent acquire calls are all released without a wait until a new response arrives, so a queued burst can exceed the newly reset window's @limit and trigger avoidable 429s. The wait <= 0 branch leaves @remaining at zero/negative; restore the local budget from @limit when the reset is reached before returning.
Suggested change
| return [0, nil] if wait <= 0 | |
| if wait <= 0 | |
| @remaining = @limit | |
| return [0, nil] | |
| 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/rate_limiter.rb around line 88:
After `@reset_at` has passed, concurrent `acquire` calls are all released without a wait until a new response arrives, so a queued burst can exceed the newly reset window's `@limit` and trigger avoidable 429s. The `wait <= 0` branch leaves `@remaining` at zero/negative; restore the local budget from `@limit` when the reset is reached before returning.
Comment on lines
+74
to
+76
| def collect(ticket_type, union) | ||
| type_id = ticket_type['id'].to_s | ||
| definitions(ticket_type).each do |definition| |
There was a problem hiding this comment.
🟠 High schema/ticket_attributes_introspector.rb:74
A null or non-object element in the ticket types array raises at ticket_type['id'], aborting datasource construction instead of returning the intended empty attribute schema. Guard ticket_type before indexing it so definitions can safely handle invalid elements.
def collect(ticket_type, union)
- type_id = ticket_type['id'].to_s
+ return unless ticket_type.is_a?(Hash)
+ type_id = ticket_type['id'].to_s🚀 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 lines 74-76:
A `null` or non-object element in the ticket types array raises at `ticket_type['id']`, aborting datasource construction instead of returning the intended empty attribute schema. Guard `ticket_type` before indexing it so `definitions` can safely handle invalid elements.
christophebrun-forest
merged commit Sep 1, 2026
71eecb7
into
feat/datasource-intercom
33 checks passed
christophebrun-forest
deleted the
feat/prd-1112-tickets-conversations-read
branch
September 1, 2026 15:43
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First lot of the Intercom datasource — PRD-1112, under the epic PRD-1111.
Targets the integration branch
feat/datasource-intercom, notmain: the lots of phase 1 are merged there and go tomaintogether.What this brings
A new
packages/forest_admin_datasource_intercomgem publishing six read-only collections:IntercomConversationGET /conversations,GET /conversations/{id}IntercomTicketPOST /tickets/search,GET /tickets/{id}IntercomAdmin,IntercomTeam,IntercomTicketType,IntercomTicketStateRows, record details, exact counts and the conversation thread work. Server-side filtering (lot 2), writes and actions (lot 3), contacts and companies (lot 4) do not — and say so rather than pretending.
The line that runs through it
A result that looks filtered and is not is worse than an explicit refusal. Every limit of the API is either translated exactly or refused with a message naming what to change:
id equals Xis served, through the record endpoint;pages.nextwhose cursor cannot be read is refused rather than taken for the last page;The one thing that cannot be refused is a sort: Intercom accepts it and ignores it without a word (measured), so no column is sortable and a requested order is reported in the log.
Measured, not assumed
Everything below came from a real workspace — the trial one for the API semantics, then a customer workspace of 81 142 tickets — and several points contradict the documentation:
/tickets/searchanswers undertickets,/adminsunderadmins,/teamsunderteams, not underdata;statisticsblock: neither a closure date nor a last responder exists as a field;closed_atfrom the last transition into aresolvedcategory (matched on theticket_state_updatedprefix, since workflows close tickets through other variants),last_responderfrom the lastcommentpart, notes excluded;x-ratelimit-limitis 1667, describing the 10-second window, not the documented 10 000 a minute — so the limiter paces on the instantaneous rate, driven by the response headers;per_page=200is refused withinvalid_per_pageinstead of being clamped;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.Verification
bundle exec rubocopclean across the repository;version.rbformat, gemspec MFA opt-out,.rubocop.yml,.releaserc.js, and both lists ofbuild.yml.Before this reaches
mainNot blocking this merge, but open on the lot:
conversationsenvelope key, which was deduced fromticketsrather than measured;per_pageagainst real response sizes, to replace the provisional 25;/tickets/searchactually filters on (company_idis refused withinvalid_field— filtering tickets by account is not the given the plan assumed), which is also the table lot 2 needs.🤖 Generated with Claude Code
Note
Add
forest_admin_datasource_intercomgem with read-only tickets and conversations collectionsFetchAllCollectionfor small reference sets with in-memory filtering/sorting/pagination, andCursorCollectionfor cursor-paginated endpoints (conversations, tickets) with read-only, non-sortable fields and limited filter operatorsClientbacked by Faraday, with aRateLimiterdriven by Intercomx-ratelimit-*headers, aThrottlemiddleware, and aRetryPolicyfor transient failures and 429sCursorWalkerto bridge offset/limit requests onto Intercom cursor pagination, with deduplication and configurable page/record caps that log a warning when truncation occursTicketAttributesIntrospectorto discover ticket-attribute columns at boot, with name sanitization, type mapping, and collision detection; API failures degrade to an empty attribute list with a warningMAX_TIMELINE_READS; remaining rows getniltimelines with a warning. Ticket page size uses a conservative cap belowClient::MAX_PER_PAGEMacroscope summarized a51bd3d.