{{ message }}
Repository navigation
safely get oauth tokens - #22
Merged
Merged
Conversation
This was referenced Jul 24, 2026
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>
This was referenced Sep 10, 2026
Merged
4 tasks done
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
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.

No description provided.