Add ElevenLabs, New Relic, Retool and Twingate connectors - #1884
Conversation
1189e83 to
b97631e
Compare
|
/cubic review |
@aureliensibiril I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 39 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
b97631e to
88295ac
Compare
|
All seven findings were valid and are fixed. Notes on the three where the reasoning matters: P1 Twingate probe / GraphQL errors — correct, and it was inconsistent with my own P1 Retool P2 Twingate rate limit — taken rather than left documented. Pages are now spaced a second apart, waited only between pages, so a large network stays inside the 60-reads-a-minute budget instead of failing at roughly 6,000 users, and a single-page network waits not at all. P2 P2 New Relic transient GraphQL error — agreed. A GraphQL errors array is now retryable rather than terminal; NerdGraph reports execution failures the same way as refusals, and the two cannot be told apart without reading provider text this driver deliberately does not read. The worker attempt budget bounds a genuine refusal anyway. P3 stale P3 dead Verified after the fixes: |
|
/cubic review |
@aureliensibiril I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 14 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="pkg/accessreview/drivers/twingate.go">
<violation number="1" location="pkg/accessreview/drivers/twingate.go:50">
P2: With the existing two-minute source deadline, a 121-page roster cannot complete: this adds 120 one-second waits before page 121 and leaves no time for requests. Increase the Twingate fetch budget or make it driver-specific so rate-limited pagination can finish.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| // pages it needs, which turns what was a hard failure above roughly 6,000 | ||
| // users into a slower sync. It is waited only BETWEEN pages, so the common | ||
| // single-page network pays nothing. | ||
| twingatePageInterval = time.Second |
There was a problem hiding this comment.
P2: With the existing two-minute source deadline, a 121-page roster cannot complete: this adds 120 one-second waits before page 121 and leaves no time for requests. Increase the Twingate fetch budget or make it driver-specific so rate-limited pagination can finish.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/accessreview/drivers/twingate.go, line 50:
<comment>With the existing two-minute source deadline, a 121-page roster cannot complete: this adds 120 one-second waits before page 121 and leaves no time for requests. Increase the Twingate fetch budget or make it driver-specific so rate-limited pagination can finish.</comment>
<file context>
@@ -38,13 +39,16 @@ const (
+ // pages it needs, which turns what was a hard failure above roughly 6,000
+ // users into a slower sync. It is waited only BETWEEN pages, so the common
+ // single-page network pays nothing.
+ twingatePageInterval = time.Second
+
twingateStateActive = "ACTIVE"
</file context>
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 39 files
Requires human review: Auto-approval blocked by 2 unresolved issues from a previous review of this commit.
Fix all with cubic | Re-trigger cubic
88295ac to
9813350
Compare
Four access-review connector pages, one per provider shipping in getprobo/probo#1884, plus their directory cards. Each Collected Fields table is derived from the driver rather than from the vendor's docs, so a row marked Not supported is one the driver genuinely leaves empty. Three facts came out of testing the live APIs and are recorded because a reader hits them: ElevenLabs refuses a bad key with 400 where most APIs answer 401, New Relic runs three regional endpoints and refuses a key presented to the wrong one, and Twingate fails the connection outright on a network name that does not exist. Retool's page carries a plan gate. Reading the roster needs the users:read scope, which is absent from the token dialog on Team and on Business; a Business token holding every scope it can grant is still refused. Retool's pricing page reserves full API scope access for Enterprise, so the page says so and points a customer without it at a CSV source instead. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
|
Second round: four of the five are fixed, one I am pushing back on with evidence. Rejected —
Fixed — Twingate 404 misdiagnosed as a dead token. Right, and it is the second time this was raised, so it should have landed the first time. A 404 now returns Fixed — New Relic billing tier in Fixed — Partly fixed — the two-minute source deadline. The deadline is real ( What the flat one-second sleep did get wrong is adding the wait on top of each round trip. Pacing is now measured from when the previous request started, so the time Twingate spends answering counts toward the interval instead of extending it — which is what actually lets a mid-sized roster finish. The remaining ceiling is documented in the code and on the docs page as roughly ten thousand users. Giving a rate-limited driver a longer budget than the shared default is a change to the review engine, affecting all 69 connectors, so I have left it out of a connector PR and called it out here instead. Happy to do it separately if you would rather it ship together. Verified after the fixes: |
|
/cubic review |
@aureliensibiril I have started the AI code review. It will take a few minutes to complete. |
retryRoundTripper resends the same *http.Request it was given. The first attempt reads that request's body to the end, so every retry after it carries nothing: a provider that answers a recoverable 503 gets an empty POST on the second try and rejects it as a bad request, turning a blip into a failed sync. It went unnoticed because the transport was written for REST drivers, where the request has no body to lose. Every GraphQL driver has one, and each of them wraps this transport. The rewind goes onto a clone rather than the caller's request, since a RoundTripper is not allowed to mutate what it is handed. A request with a body but no GetBody cannot be rewound at all, so it stops retrying instead of sending a truncated one. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
The hook that keeps credentials out of recorded cassettes listed the headers to remove. A credential travels in whatever header its provider chose, so a list of names to drop is fail-open: the first provider to authenticate through a name nobody had thought of writes its key into a cassette verbatim, and this repository is public. That is not hypothetical. Recording an ElevenLabs cassette wrote the live key into it, because ElevenLabs sends the key in xi-api-key, which canonicalizes to Xi-Api-Key and is not the X-Api-Key that Anthropic already had an entry for. Inverted to an allowlist of headers a cassette may keep: content negotiation, the API-version and routing pins that select a response shape, and Range, which is how Heroku asks for a page. A provider needing a new one now gets a test that cannot find its interaction, which stops at CI. The old direction failed by publishing a key. That inversion also retires sanitizeAWSSigningHeaders, whose headers the allowlist now drops by construction, and which never ran anyway: the AWS recorder is replay-only, so no save hook fires for it. replaceCassetteBody is the other half. A sanitizer that rewrites a body has to restate its length everywhere the cassette records it, and the ones here set the interaction's field while leaving the provider's Content-Length header describing a body that is no longer there. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
9813350 to
62cd996
Compare
Four access-review connector pages, one per provider shipping in getprobo/probo#1884, plus their directory cards. Each Collected Fields table is derived from the driver rather than from the vendor's docs, so a row marked Not supported is one the driver genuinely leaves empty. Three facts came out of testing the live APIs and are recorded because a reader hits them: ElevenLabs refuses a bad key with 400 where most APIs answer 401, New Relic runs three regional endpoints and refuses a key presented to the wrong one, and Twingate fails the connection outright on a network name that does not exist. Retool's page carries a plan gate. Reading the roster needs the users:read scope, which is absent from the token dialog on Team and on Business; a Business token holding every scope it can grant is still refused. Retool's pricing page reserves full API scope access for Enterprise, so the page says so and points a customer without it at a CSV source instead. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
|
Rebased onto main (36 commits, now level). One conflict, in Re-verified on the new base after regenerating: getprobo/probo.com#108 is rebased onto v2 as well, so the two still merge together. |
|
/cubic review |
@aureliensibiril I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 39 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="pkg/accessreview/drivers/newrelic_test.go">
<violation number="1" location="pkg/accessreview/drivers/newrelic_test.go:373">
P2: Custom agent: **Avoid Logging Sensitive Information**
During a cassette recording run (when NEW_RELIC_API_KEY is set), the sanitizer only runs on save, so these assertions run against live NerdGraph data and fail by design. testify's assert.Equal then prints the real user's email (including local-part) and full name into the test failure output, leaking PII in test output. Assert on non-identity properties or on the domain only, e.g. assert.Contains(member.Email, "@example.com") and assert.NotEmpty(member.FullName), so the recording workflow cannot dump real identity values.</violation>
</file>
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Fix all with cubic | Re-trigger cubic
| // who belongs to the built-in Admin group. | ||
| member := records[0] | ||
| assert.Equal(t, "1000000001", member.ExternalID) | ||
| assert.Equal(t, "1000000001@example.com", member.Email) |
There was a problem hiding this comment.
P2: Custom agent: Avoid Logging Sensitive Information
During a cassette recording run (when NEW_RELIC_API_KEY is set), the sanitizer only runs on save, so these assertions run against live NerdGraph data and fail by design. testify's assert.Equal then prints the real user's email (including local-part) and full name into the test failure output, leaking PII in test output. Assert on non-identity properties or on the domain only, e.g. assert.Contains(member.Email, "@example.com") and assert.NotEmpty(member.FullName), so the recording workflow cannot dump real identity values.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/accessreview/drivers/newrelic_test.go, line 373:
<comment>During a cassette recording run (when NEW_RELIC_API_KEY is set), the sanitizer only runs on save, so these assertions run against live NerdGraph data and fail by design. testify's assert.Equal then prints the real user's email (including local-part) and full name into the test failure output, leaking PII in test output. Assert on non-identity properties or on the domain only, e.g. assert.Contains(member.Email, "@example.com") and assert.NotEmpty(member.FullName), so the recording workflow cannot dump real identity values.</comment>
<file context>
@@ -0,0 +1,631 @@
+ // who belongs to the built-in Admin group.
+ member := records[0]
+ assert.Equal(t, "1000000001", member.ExternalID)
+ assert.Equal(t, "1000000001@example.com", member.Email)
+ assert.Equal(t, "Member 1000000001", member.FullName)
+ assert.Equal(t, new(true), member.IsAdmin)
</file context>
Four API-key, single-tenant (Pattern 3) access-review connectors. Each key is bound to one workspace, organization or network, so none of them needs a picker or a settings struct beyond what locates the tenant's API. Every claim below about ElevenLabs, New Relic and Twingate was verified against the live API; Retool's roster is taken from its published OpenAPI, because listing users needs a users:read token and Retool only grants that scope on Enterprise. ElevenLabs answers its members endpoint with a bare JSON array — no envelope, no pagination — and includes locked members, so a deactivated seat is reported inactive rather than dropped. It rejects a key with 400 and an authentication_error body, not 401, which the probe would otherwise read as connected: probeElevenLabs adds 400 to the statuses that mean a dead credential. New Relic keeps a user's identity and their authority in different places. The user record carries a `type` that is the billing tier, not a role, while what a user may do comes from their groups — so the driver reads both and reports group membership as the roles, labelling the tier so it cannot be mistaken for one. Admin is matched exactly against the built-in Admin group; a customer may create one called "admin" that grants nothing. Following the group-member cursor is what lets an administrator be recognised at all, so a targeted page that comes back for a different domain or group is an error rather than a short roster: a member missing from a campaign is reviewed by nobody. The key also belongs to one of three data regions and the others answer 403, which no credential can reveal, so the customer names it and every host follows from it. The probe asks for the roster's own entry point, since any live key can answer a trivial query while NerdGraph refuses organization user management with 200 and an errors array. Retool Cloud routes every organization through the shared api.retool.com gateway, so its base URL setting is optional and serves only self-hosted instances. Retool's own docs write that URL as .../api/v2, so the suffix arriving already attached is the likeliest paste there is, and the base is reduced to an origin and a clean path before the prefix is applied. Its roster is the only one of the four that reports per-user two-factor state, so MFA is never unknown. Twingate gives every tenant its own host, so the network name is the whole of what identifies it — and it lands in the host position of every URL, which is why it is validated as a DNS label and stored canonically rather than as typed. A network that does not exist answers 404, the one mistake a credential check cannot catch, so the probe rejects it too. Twingate also refuses a query with 200 and an errors array, so the probe reads that as the refusal it is. isAdmin is reported separately from role because Twingate sets it for ADMIN and DEVOPS alike. Reads are limited to 60 a minute, so pages are spaced a second apart, which keeps a large network inside the budget instead of failing it outright; a single-page network waits not at all. Wherever a provider says there are more members without saying where to resume, the driver errors rather than returning what it has. A short roster reviewed as if it were complete is the worse failure. None of the four declares a KeyFormat: ElevenLabs, Retool and Twingate document no prefix, and New Relic's current keys carry NRAK- but the REST keys it migrated from did not, and its docs still tell holders of those they need no update. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
62cd996 to
58a3842
Compare
|
CI is green on the rebased branch (12/12). Two cubic findings on this run: 1. 2.
The assertions are also the thing that proves the sanitizer worked — weakening them to |
|
/cubic review |
@aureliensibiril I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 39 files
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Re-trigger cubic
The generated environment reference had drifted from probo: the docs:check-reference job compares it against a fresh checkout of getprobo/probo, and the committed file was 736 lines behind. That break predates this branch. It already failed the quality workflow on v2 when the Crisp guide landed, and because deploy.yaml runs the same check, the site could not deploy either. Regenerated against the branch behind getprobo/probo#1884, so the file also carries the twenty PROBOD_CONNECTOR_*_ENDPOINT_* rows the four new connectors register. That makes the check green only once #1884 is on main, which is the intended merge order anyway: the DocumentationURL links added here 404 until the console change ships. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
docs:check-reference regenerates this page from a checkout of getprobo/probo and fails when the committed file differs, and the committed file was 736 lines behind. Regenerating at the parent of probo commit b995080d3 "Connect Crisp by app install, not a pasted code" reproduces the committed file byte for byte, so that commit is the whole cause: it shifted pkg/bootstrap/builder.go by two lines, and every row here carries a builder.go#Lnnn "View usage" link. No variable was added or removed, which is why nothing looked wrong. The break is already on v2, where it failed both the quality workflow and the deploy, so the Crisp guide rewrite never reached the live site. Regenerated against the branch behind getprobo/probo#1884, so the file also carries the twenty PROBOD_CONNECTOR_*_ENDPOINT_* rows the four new connectors register through the registry loop. That makes the check green only once #1884 is on main, which is the intended merge order anyway: the DocumentationURL links added here 404 until the console change ships. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
Four API-key access-review sources, each with the page structure the other connector guides use, plus their directory entries. Retool's users:read scope is Enterprise only. It is absent on Team and on Business, where a token with every scope selected still gets 403, and no other endpoint returns the roster, so the guide says so rather than letting a customer buy Business and find out. Also regenerates the environment reference: the provider registry emits five PROBOD_CONNECTOR_<PROVIDER>_ENDPOINT_* variables per connector, so these four add twenty rows even though none of them needs bootstrap configuration. docs:check-reference compares that file against probo's default branch, so this goes green once getprobo/probo#1884 is on main, which is also when the DocumentationURL links here stop pointing at 404s. Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
1 participant
Four API-key, single-tenant (Pattern 3) access-review connectors, plus two
fixes to shared infrastructure that the work uncovered. Three commits, each
independently buildable.
9522d68e3d4f8631101189e83a0Verified against the live APIs, which corrected four assumptions
Every claim about ElevenLabs, New Relic and Twingate was checked against the
real API rather than the vendor's docs. That changed the design four times:
400, not401.doProbeRequestreads anything but 401/403 as connected, so a dead key would have shipped a
permanently green badge.
probeElevenLabsadds 400 to the rejected statuses.typeis the billing tier, not a role. Authority comesfrom group membership, which is why the driver walks four cursor levels and
joins group members back onto the roster by user id.
jp. Supporting only us/eu would havelocked those customers out entirely.
HELPDESK,ACCESS_REVIEWERandBILLINGbesides the obvious ones), confirmed by introspecting a livenetwork's schema.
Retool is Enterprise-gated
GET /api/v2/usersneeds theusers:readscope, and that scope is absent fromthe token dialog on Team and on Business. A Business token created with
every available scope selected is still refused with
403 missing one of "users:read". Retool's pricing page reserves "full accessto all API scopes" for Enterprise; its internal feature catalogue carries a
BaseEnterpriseApiflag a Business org does not hold.No other endpoint substitutes:
/user_invitesreturns invitations rather thanmembers, and the usage endpoints need a
usagescope that is also not offered.The driver is correct and the docs state the gate. Its cassette is hand-authored
from Retool's published OpenAPI, with a header saying so and why.
Shared fixes
Retries dropped the POST body.
retryRoundTripperresent the same requestafter its body had been read, so a recoverable 503 became an empty POST and a
failed sync. It affected every GraphQL driver, Monday and Railway included. The
rewind goes onto a clone, since a RoundTripper must not mutate its caller's
request.
The cassette credential hook was fail-open. It listed headers to remove, so
the first provider to authenticate through an unlisted name wrote its key into
a cassette verbatim — which is exactly what happened when recording ElevenLabs
(
xi-api-keycanonicalizes toXi-Api-Key, not theX-Api-Keyalreadylisted). Inverted to an allowlist: a missing entry now breaks a test at CI
instead of publishing a key.
Review
Each connector was reviewed by three independent reviewers (a Claude pass, a
review-as-gearnodepass, and an adversarial Codex pass). They found, and thisbranch fixes: silent roster truncation in New Relic's targeted pagination; a
mistyped Twingate network passing the probe as "connected"; a New Relic probe
that was green while the roster query was refused; an inverted
terminal/transient default in the name resolver; and Retool base-URL shapes
(
/api/v2/.,/?) that produced wrong paths. Codex found no host-injectionbypass in Twingate's network validation.
Testing
go build, the connector/accessreview/bootstrap/coredata/console-v1 suites,golangci-lint(0 issues), Relay + console typecheck, and the four migrationsapplied and re-applied against a disposable Postgres. All five cassettes
audited clean of credentials and real identities. New coverage includes
four-level New Relic pagination with the group-member join, Twingate cursor
paging and its missing-cursor guard, the three custom probes, and the retry
body rewind.
Ships with
Documentation for all four in getprobo/probo.com (linked below) — these carry
DocumentationURL, so the docs need to merge together with this PR or thefour links 404.
🤖 Generated with Claude Code
https://claude.ai/code/session_01JFFcusrDZo9AR42g5Te2wS