safely get oauth tokens by jsendo · Pull Request #22 · PipedreamHQ/pipedream · GitHub
Skip to content

safely get oauth tokens - #22

Merged
jsendo merged 4 commits into
masterfrom
gcal-undefined-auth-check
May 5, 2020
Merged

jsendo merged 4 commits into
masterfrom
gcal-undefined-auth-check

Conversation

@jsendo

@jsendo jsendo commented May 5, 2020

Copy link
Copy Markdown
Contributor

No description provided.

@jsendo
jsendo merged commit 89702c2 into master May 5, 2020
@jsendo
jsendo deleted the gcal-undefined-auth-check branch May 18, 2020 20:32
michelle0927 added a commit that referenced this pull request Aug 5, 2026
* feat(slack_v2): eval-driven fixes for the MCP tool surface

Iterated against the MCP eval suite (pd-connect-eval-monster/evals/slack_v2).
Suite went 23/35 -> 33/35 on Sonnet 5 (pass^2); every change below is tied to a
specific red-to-green flip or a measured payload reduction.

- list-channels: evals #2/#10/#11/#16/#19/#35 all failed the same way — the
  action returned every field of every channel with numPages defaulting to 1, so
  a 62-channel workspace produced ~55k chars, blew the 25k-token MCP ceiling, and
  the model was handed a file path instead of data (#2 burned all 20 turns
  re-calling it). Added an additive `fields` projection, a `cursor` prop, and
  `has_more`/`next_cursor` so truncation is visible instead of silent. Payload
  55k -> 5.9k chars; all six evals pass.  [minor]

- slack_v2.app.mjs: `assistantSearch` called `sdk().apiCall()` directly — the one
  path in the app that bypassed `_withRetries` — while the client is built with
  `rejectRateLimitedCalls: true`, so every 429 rejected instantly. 8 of 12 search
  calls errored in one run (#24/#26/#27). Routed through `_withRetries`. Also adds
  `resolveUserId` (id / email / display name) for the invite fix below.

- delete-message: eval #20 ("post a note then take it back") failed every run with
  cant_delete_message — post-message sends no `as_user` so it posts as the USER,
  while this action defaulted `as_user: false`, which routes to the BOT token. A
  message just posted could never be deleted. Now retries with the other identity
  on cant_delete_message; the default is unchanged, so existing workflows are
  unaffected. Description also gained confirmation guidance for the destructive
  path (#33).  [minor]

- invite-user-to-channel: eval #10 failed with user_not_found — the agent passed
  the email from the prompt. Now resolves user id / email / display name, and the
  channel by name.  [minor]

- get-channel-details, list-members-in-channel, set-channel-topic: every
  AI-optimized action in this app resolves channel NAMES server-side and these did
  not, so agents that read "#seinfeld-general" from a prompt got channel_not_found
  (#19, #22) or a bare internal_error (#10). All three now use resolveChannelId.
  [minor]

- get-channel-history, get-thread-replies: outputs measured at 17k chars average
  (worst 49k) and 25k average respectively — every call burned that much of the
  agent's context. Added the same additive `fields` projection.  [minor]

- get-user-details, get-current-user: two tools answer "who am I" and the agent
  chose the legacy one in 10/10 trials (#1). Descriptions now differentiate them.
  NOTE: this did NOT change routing — the agent still picks get-current-user. The
  actual fix is removing one of the two from the /v3 component allowlist; these
  edits only make the intended split legible.  [patch]

App package.json bumped 0.6.1 -> 0.7.0.

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

* fix(slack_v2): satisfy CI — lint formatting, dependent version bumps, UI-centric copy

- Lint Code Base: 9 errors, all in the files this branch touches and all
  formatting. Broke the `.split().map().filter()` chains across lines
  (newline-per-chained-call), split the `has_more` spread ternary and its object
  literal (multiline-ternary, object-curly-newline), and expanded the
  `users.list` destructure in resolveUserId (object-curly-newline).

- Ensure component commits modify component versions: this branch edits
  slack_v2.app.mjs, so every component importing it needs a version bump.
  Patch-bumped the 48 actions/sources that were not already versioned in the
  previous commit.

- Removed UI-centric wording from two descriptions, which read oddly for an
  agent calling the tool over MCP and contradicted the resolution behaviour
  added in the previous commit:
  - get-channel-details: "by selecting it or providing an ID" -> "specified by
    ID or by name" (it had claimed both "selecting it" and "Accepts a channel
    ID or NAME" in the same sentence).
  - set-channel-topic: "a selected channel" -> "a channel, specified by ID or
    by name".

Verified locally with ESLint 9 against the repo's own rule options for the three
failing rules: 0 violations across components/slack_v2.

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

* fix(slack_v2): restore comma-separated users on invite, fix delete identity fallback

invite-user-to-channel: conversations.invite documents `users` as comma-separated and
the prop is a plain string, so "U01ABCDEF,U02GHIJKL" was a working input. The new
resolveUserId() matches neither the ID regex (no comma) nor the email regex, then
falls through to an exhaustive paged users.list scan that cannot match, and throws.
Adds resolveUserIds() (plural), which splits on commas — or accepts an array —
resolves each token independently, and rejoins. An all-IDs input still makes zero
extra API calls, so the common path is unchanged in cost as well as result.

delete-message: makeRequest() routes to the bot token only on `as_user === false`, so
`!this.as_user` made the retry a no-op whenever as_user arrived nullish — undefined
and true both route to the user token, so it retried as the identity that had just
been refused. Changed to `this.as_user === false`. Attempt 1 still passes the
configured value verbatim.

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

* docs(slack_v2): reattach resolveChannelId's JSDoc to its function

resolveUserId/resolveUserIds were inserted between resolveChannelId's doc comment
and resolveChannelId itself, leaving two stacked doc blocks above resolveUserId and
no doc on resolveChannelId. Comment move only; no code change.

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

* refactor(slack_v2): move the fields projection into common/utils.mjs

Addresses review feedback on #21493: `pickFields` was copy-pasted identically into
get-channel-history, get-thread-replies and list-channels.

Moved to components/slack_v2/common/utils.mjs, alongside the CSV-or-array `fields`
normalizer that was duplicated in the same three places — both halves of one feature,
so splitting them across files would just invite the next divergence.

`projectFields(records, fields)` composes the two and keeps the additive contract:
with no `fields`, it returns the ORIGINAL array, so a caller that omits the prop gets
exactly what these actions have always returned.

Behavior-identical to the three removed copies, verified against them across
undefined / empty array / empty string / array / CSV / CSV-with-spaces / unknown
field / all-unknown inputs, including array identity on the no-fields path.

No version bumps: all three components are already ahead of master on this branch,
so the PR diff still shows a version change for each.

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

* fix(slack_v2): address review feedback on delete-message, resolveUserId, invite-user-to-channel

- delete-message: only retry with the other identity when a bot token exists,
  since otherwise both attempts hit the same token and just repeat the error.
- resolveUserId: scan all users.list pages and require an exact, unambiguous
  name match; throw ConfigurationError instead of silently picking the first.
- invite-user-to-channel: treat already_in_channel as success only for a
  single user; bulk invites are now sent per-user with individual results so
  failures aren't hidden behind a successful summary.

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

* fix(slack_v2): guard invite-user-to-channel against empty resolved user list

resolveUserIds() returns "" when every comma-separated token is blank, and
"".split(",") yields [""] rather than [], which slipped through the
single-user path and sent an empty users value to Slack. Filter empty tokens
and throw ConfigurationError when none remain.

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

* fix(slack_v2): resolve channel name in delete-message, treat self-invite as no-op

- delete-message now calls resolveChannelId() before deleting, like
  every other AI-optimized action in this app. Previously it passed
  `conversation` straight through, so a channel NAME (rather than ID)
  failed with channel_not_found.
- invite-user-to-channel: Slack refuses to let an identity invite
  itself (cant_invite_self) before it would ever reach an
  already_in_channel check. Treat that the same way — a no-op, not an
  error — since to the caller it means the same thing: the user has no
  further action needed on that channel.

Found via eval-driven regression testing on PR #21493.

* chore(slack_v2): revert version bumps on delete-message, invite-user-to-channel

Keep at 0.2.0/0.1.0 rather than bumping per fix.

---------

Co-authored-by: Dylan Sather <Dylan Sather>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Michelle Bergeron <michelle.bergeron@workday.com>
Co-authored-by: michelle0927 <michelle0927@users.noreply.github.com>
dannyroosevelt added a commit that referenced this pull request Sep 10, 2026
…MCP (#21877)

* feat(google_search_console): AI-optimized Search Console actions for MCP + eval-driven fixes

Iterated against the MCP eval suite (evals/google_search_console): 42 evals,
gating run pass^2 on claude-sonnet-5 under the claude_code_default harness was
38/42 consistent; the 4 it flagged were fixed and re-run 2x each - #34 and #39
now pass both trials, #10 and #11 are warnings on passing evals (judge satisfied
every trial; one extra call or a question before acting on one of two trials).

All eight actions carry the top-level ai: "optimized" marker. Worked examples
use example.com rather than the eval fixture property.

- retrieve-site-performance-data: renamed "Query Search Analytics" with an
  agent-oriented description; response gains row_count, has_more,
  next_start_row and returned_totals so the agent knows when to page; adds the
  hour dimension, hourly_all data state and byProperty/byNewsShowcasePanel
  aggregation; advancedDimensionFilters also accepts a bare filter array; a 403
  now lists the properties the account can actually access. All 13 legacy prop
  keys kept; rowLimit default 10 -> 50; response is a superset of before.  [minor]
- submit-url-for-indexing: eval #34 ("request indexing for an ordinary page")
  had the model run the Indexing API tool on a page it does not cover. The
  description now says it is only for JobPosting/BroadcastEvent pages and to
  offer Submit Sitemap / Inspect URLs and wait. Dropped the try/catch that
  rewrote Google's error text. Props and output unchanged. #34 passes 2/2.  [patch]
- list-sites (new): discovery tool - every property the account can see, with
  permission level and property type, plus the account email. #39 (backlinks
  abstain) showed the model calling it reflexively because it said "call FIRST
  on any per-site task"; it now says not to call it when Search Console cannot
  do the task. #39 passes 2/2.
- list-sitemaps (new): sitemaps for a property with warnings/errors/submitted
  URLs as integers and a summary rollup. #33 (submit -> delete -> confirm gone)
  showed the model verifying a deletion by fetching the deleted path, which
  404s; both sitemap tools now say to verify by listing and checking absence.
- submit-sitemap (new): list -> PUT -> GET so previous_last_submitted tells a
  first submission from a resubmission without a 404 catch.
- delete-sitemap (new): destructiveHint, confirmation required for the exact
  URL. #36 (confirm before deleting) once deleted the real fixture when the
  question tool was denied; the description now states that no answer is never
  consent. #36 passes every trial since.
- inspect-urls (new): URL Inspection for 1-10 URLs in one call, 5 in flight,
  per-URL failures land in that row instead of aborting the batch. #39 showed
  referringUrls reading like a backlink report; the description now says the
  Links report has no API and this is a discovery sample.
- compare-search-analytics (new): two date ranges fetched in parallel, joined
  on dimension keys, per-row and total deltas with pct_change (null when the
  previous period is 0); sorted by absolute delta so #22 ("gained or lost")
  surfaces both directions.
- google_search_console.app.mjs: siteUrl propDefinition drops its async
  options() dropdown (dynamic options break MCP; List Sites is the discovery
  path) and carries the property-format rules; adds sitemapUrl, searchType,
  filterDimension, filterOperator, advancedDimensionFilters propDefinitions and
  getUserInfo/listSitemaps/getSitemap/submitSitemap/deleteSitemap/inspectUrl
  methods; removes the now-dead listSiteOptions().
- common/filters.mjs (new): one definition of the two accepted filter shapes,
  shared by both analytics tools. common/compare.mjs (new): pure join/sort/
  totals for the compare tool.

App package.json bumped 1.0.0 -> 1.1.0.

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

* fix(google_search_console): review follow-ups for #21877

- compare: `delta.ctr`/`delta.position` are null when a key is absent from
  one period. A zero placeholder is not a rank: a query new this period at
  position 47 read as a 47-place drop and topped the position_delta sort.
  Null deltas sort last. Add `truncated` so a period that hit the internal
  5000-row cap is visible in the payload, not only in the description.
- retrieve-site-performance-data: guard the diagnostic getSites call in the
  403 handler so a token/scope failure surfaces the original error.
- list-sites: the userinfo lookup is fail-soft (`account_email: null`) so
  the discovery tool cannot fail on a nice-to-have field.
- app: trim siteUrl/sitemapUrl once where the path is built (a trailing
  space became %20 and a 403); reuse SEARCH_CONSOLE_V3 in getSites and
  getSitePerformanceData.
- submit-url-for-indexing: destructiveHint false per the action guidelines
  (send/publish; URL_DELETED is reversed by URL_UPDATED).
- methods: drop the now-unused checkWhoThrewError.

No version changes: none of the versions bumped in this PR are published yet.

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

* refactor(google_search_console): cut tool description size ~35%

The 8 AI-optimized actions carried a fixed 7-section description template
(Purpose / When to use / Returns / Cross-references / Parameter guidance /
Common mistakes / Example) applied whether the tool needed it or not, so the
toolset cost ~10.5k tokens of schema — ~1,300 per tool — in context on every
turn under tool search.

Removed the redundancy, not the content:

- Parameter guidance duplicated the prop descriptions almost verbatim
  (`dimensions` combination rules, `aggregationType`'s BY_PROPERTY 400,
  `rowLimit`'s spill cap, `subdomainFilter`'s legacy name). The prop
  description is the right home — it is read when the model fills the param.
- Cross-references was O(n²): every tool described every other tool. "Copy
  siteUrl verbatim from List Sites, wrong string → 403" appeared in 7 actions
  plus the app propDefinition; "there is no API to request indexing an
  ordinary page" appeared in 4.
- Returns narrated fields the model already receives. Kept only the
  non-obvious semantics (verdict values, tri-state canonical_mismatch,
  null deltas for new/lost keys, per-row error, truncated vs has_more).
- Dropped Purpose (folded into the opening sentence) and shrank the Example
  blocks to the one line that demonstrates the trap.
- inspect-urls now uses the shared siteUrl propDefinition instead of an
  inline copy of it.

Every behaviour the eval suite gates on is preserved verbatim or near it —
the negative-eval blocks (delete-sitemap's confirmation rules, the no-API
facts behind the request-indexing and backlinks abstentions) are deliberately
the least-trimmed part. Checked with a 44-claim inventory mapped to eval ids.

Description chars 32,970 → 19,567 (-41%); with prop descriptions
42,184 → 27,539 (-35%, ~10.5k → ~6.9k tokens).

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

* refactor(google_search_console): restore page-filter and freshness guidance

Two evals needed guidance the first pass had cut:

- #12 "queries for page X only" — the model stopped applying a page filter and
  queried a matching URL-prefix property instead. The prop is named
  `subdomainFilter` and labelled "Filter Value", so the description was
  carrying the weight of a misleading name. Restored, and now also states that
  a prefix property is NOT a substitute (it covers every page under the
  prefix) — which the original description never said.
- #8 "daily clicks for the last 7 days" — the model stopped setting
  `dataState: all` and returned 6 rows for a 7-day window. The original said
  "omitted unless dataState is all" in three places; the rewrite left it only
  on `endDate`. Restored on `startDate` and added an explicit Freshness block.

Both flip back to pass. +797 chars against a 12,606-char saving.

Verified on the tools the model actually receives (`listMcpTools` private
registry): 33,400 -> 20,794 description chars, -38%. Full suite at pass^2,
same suite hash: 39/42 with the old descriptions, 41/42 with these. Restraint
6/6 both ways. Only #11 (Discover queries) still non-pass, unchanged and
pre-existing — the judge passes all 3 criteria, the eval wants a tool call the
model routes around.

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

* fix(google_search_console): address CodeRabbit review on #21877

Two open findings, both backed by repo path instructions in .coderabbit.yaml:

- ConfigurationError for pre-call validation (.coderabbit.yaml:83). Plain
  `Error` is retry-classified as transient, so a config mistake that can never
  succeed would be retried: >10 URLs or an empty list in Inspect URLs, and
  `sitemapUrl` + `sitemapIndex` both set in List Sitemaps. Three sites.
  The 403 handler in Query Search Analytics deliberately stays a plain Error —
  it runs after the API call and can also mean a token/scope problem, and the
  rule scopes ConfigurationError to pre-call validation only.

- Shared filter-value prop moved to the app file (.coderabbit.yaml:36-38).
  `subdomainFilter` (Query Search Analytics) and `filterValue` (Compare Search
  Analytics) were near-duplicate inline definitions of one prop, free to drift
  on the semantics they share. Now one `filterValue` propDefinition; the legacy
  `subdomainFilter` key is preserved on Query Search Analytics for existing
  workflows, and the shared description names it so the legacy key is still
  explained. Not extended to `dataState`/`dimensions`/`rowLimit`, which share
  names across the two actions but not definitions — Compare has no
  `hourly_all`, no `hour`/`searchAppearance`, and its `rowLimit` caps output
  after the join rather than acting as a page size.

Costs ~120 chars (one 431-char shared description in place of a 393 and a 344).

Verified: 12/12 at pass^2 on the evals covering all four changed actions,
including #12 and #13 which depend on the filter prop description.

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

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant