feat(credentials): agent-initiated oauth credential reconnect#5488
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview The authorize route enforces credential-admin access (not just workspace write), validates workspace/type/provider match, blocks Trello/Shopify, and writes The copilot handler validates reconnect up front (agent-visible errors), threads Reviewed by Cursor Bugbot for commit 1aad91b. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
@cursor review |
|
Greptile SummaryThis PR adds agent-initiated OAuth credential reconnect: the
Confidence Score: 5/5Safe to merge — all three layers of the reconnect flow (tool handler, authorize route, draft upsert) are correctly validated and the previously identified issues have been addressed. The reconnect path is validated at two independent checkpoints (tool handler before URL generation, route handler before draft creation). The draft upsert explicitly clears credentialId on the conflict path for plain connects, closing the stale-draft cross-contamination window. The audit log display-name issue noted in previous review threads is fixed by using the credential's actual displayName directly for reconnect drafts, bypassing collision numbering. Thirty-one new unit tests cover authz edge cases, provider mismatches, and display-name behavior. No logic gaps found. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Agent as Copilot Agent
participant Tool as oauth.ts (Tool Handler)
participant Route as /api/auth/oauth2/authorize
participant DB as DB (pendingCredentialDraft)
participant Provider as OAuth Provider
Agent->>Tool: "oauth_get_auth_link({ providerName, credentialId })"
Tool->>Tool: ensureWorkspaceAccess(workspaceId, userId, 'write')
Tool->>Tool: "getCredentialActorContext(credentialId, userId, { workspaceAccess })"
Note over Tool: Validate: admin, workspace, type, provider match
Tool-->>Agent: "{ oauth_url: /authorize?...&credentialId=cred-1 }"
Agent-->>User: Renders "Reconnect Justin's Gmail" chip
User->>Route: "GET /api/auth/oauth2/authorize?credentialId=cred-1"
Route->>Route: checkWorkspaceAccess
Route->>Route: getCredentialActorContext(credentialId) → isAdmin check
Route->>Route: validate workspace / type / provider
Route->>DB: "INSERT draft { credentialId: 'cred-1', displayName: credential.displayName }"
Note over DB: onConflict → SET credentialId explicitly (nulls on plain connect)
Route->>Provider: redirect to OAuth provider
Provider-->>Route: OAuth callback
Route->>DB: handleReconnectCredential → rebind existing credential.id to new account
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Agent as Copilot Agent
participant Tool as oauth.ts (Tool Handler)
participant Route as /api/auth/oauth2/authorize
participant DB as DB (pendingCredentialDraft)
participant Provider as OAuth Provider
Agent->>Tool: "oauth_get_auth_link({ providerName, credentialId })"
Tool->>Tool: ensureWorkspaceAccess(workspaceId, userId, 'write')
Tool->>Tool: "getCredentialActorContext(credentialId, userId, { workspaceAccess })"
Note over Tool: Validate: admin, workspace, type, provider match
Tool-->>Agent: "{ oauth_url: /authorize?...&credentialId=cred-1 }"
Agent-->>User: Renders "Reconnect Justin's Gmail" chip
User->>Route: "GET /api/auth/oauth2/authorize?credentialId=cred-1"
Route->>Route: checkWorkspaceAccess
Route->>Route: getCredentialActorContext(credentialId) → isAdmin check
Route->>Route: validate workspace / type / provider
Route->>DB: "INSERT draft { credentialId: 'cred-1', displayName: credential.displayName }"
Note over DB: onConflict → SET credentialId explicitly (nulls on plain connect)
Route->>Provider: redirect to OAuth provider
Provider-->>Route: OAuth callback
Route->>DB: handleReconnectCredential → rebind existing credential.id to new account
Reviews (3): Last reviewed commit: "fix(credentials): address reconnect revi..." | Re-trigger Greptile |
Greptile SummaryThis PR adds agent-initiated OAuth credential reconnect: the
Confidence Score: 4/5The reconnect authorization chain is well-guarded with redundant credential-admin checks at both the tool-handler and route layers. The main imprecision is in the audit log for reconnects, where the draft display name gets a collision suffix because the credential being reconnected is counted in takenNames. Cross-workspace attacks, non-admin reconnects, type and provider mismatches, and draft cross-contamination are all explicitly checked and tested. The two issues found are the misleading audit log name during reconnects and a redundant workspace-access DB round-trip in the tool handler — neither affects credential data or user-facing behavior. apps/sim/app/api/auth/oauth2/authorize/route.ts — the createConnectDraft call in the reconnect path should skip or narrow collision detection so the draft's displayName matches the existing credential name for correct audit logs. Important Files Changed
Reviews (2): Last reviewed commit: "feat(credentials): agent-initiated oauth..." | Re-trigger Greptile |
91cec7f to
24ef2cf
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 24ef2cf. Configure here.
d1dd5c0 to
1aad91b
Compare
* feat(credentials): agent-initiated oauth credential reconnect * fix(credentials): address reconnect review findings * improvement(credentials): log when connect draft name lookups degrade
* feat(credentials): agent-initiated oauth credential reconnect * fix(credentials): address reconnect review findings * improvement(credentials): log when connect draft name lookups degrade
* feat(credentials): agent-initiated oauth credential reconnect * fix(credentials): address reconnect review findings * improvement(credentials): log when connect draft name lookups degrade
* feat(credentials): agent-initiated oauth credential reconnect * fix(credentials): address reconnect review findings * improvement(credentials): log when connect draft name lookups degrade
* feat(credentials): agent-initiated oauth credential reconnect * fix(credentials): address reconnect review findings * improvement(credentials): log when connect draft name lookups degrade
* feat(credentials): agent-initiated oauth credential reconnect * fix(credentials): address reconnect review findings * improvement(credentials): log when connect draft name lookups degrade
* feat(credentials): agent-initiated oauth credential reconnect * fix(credentials): address reconnect review findings * improvement(credentials): log when connect draft name lookups degrade
* feat(credentials): agent-initiated oauth credential reconnect * fix(credentials): address reconnect review findings * improvement(credentials): log when connect draft name lookups degrade
…ts, fork chat, inline questions (mothership v0.8) (#5410) * feat(scout): add scout agent * fix(contracts): update contracts to include scout agent * feat(copilot): search agent (research+scout merge) + read-only table/KB tool handlers Mirrors mothership dev f90f9b05: - regenerated tool-catalog/tool-schemas mirrors (search trigger replaces research + scout; QueryUserTable / SearchKnowledgeBase entries) - queryUserTableServerTool / searchKnowledgeBaseServerTool: read-only wrappers delegating to the full user_table / knowledge_base handlers with hard operation allowlists (and outputPath export rejection on query_user_table) - display maps: 'search' agent label/title/icon added; research + scout entries retained so historical transcripts keep rendering - Search.id replaces Research.id in LONG_RUNNING_TOOL_IDS (it inherits research's long crawls) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(copilot): run_code compute-only handler; docs lint fix Mirrors mothership dev db60da94: run_code is the compute-only variant of function_execute for the search agent — same sandbox and inputs, no outputs.files / outputTable, so it cannot create or overwrite workspace resources. Wrapper handler hard-rejects the write vectors and delegates to executeFunctionExecute; run_code is deliberately absent from OUTPUT_PATH_TOOLS and the table output post-processor, so the name gating blocks writes even for leaked args. Added to LONG_RUNNING_TOOL_IDS, display title/icon maps, and the regenerated catalog/schema mirrors. Also removes two ineffective biome suppression comments in the docs workflow-preview (the rule doesn't fire in the docs app config). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(copilot): failed tool calls must surface their error in terminal data A failed handler result that carried a defined-but-empty output (the app-tool executor's 'Tool not found' ships output: {}) won the priority race in getToolCallTerminalData, so the resume payload's data — the only thing the model reads — was a bare {} with the error text dropped. The search agent retried run_code 20+ times blind against a stale server because every failure rendered as empty instead of 'Tool not found'. Failed calls now always carry error in their terminal data: merged into object outputs, wrapped alongside non-object outputs, preserved when the output already has an error field. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(chat): render inline question tags from the agent in chat * fix(chat): let inert multi-step questions browse all prompts * improvement(chat): guard question answer formatting against sparse arrays * chore(copilot): drop user_memory from generated contracts and tool display Companion to mothership 8ae32e97 (user_memory tool removed — the feature no longer exists). Regenerates the mothership contract mirrors via generate-mship-contracts.ts, which also picks up the pending telemetry contract additions (gen_ai.agent.name labels, llm.client.context_tokens, llm.client.compactions, llm.request.compaction_trigger, llm.compaction.pause, gen_ai.usage.context_tokens), and removes the user_memory display title. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * improvement(chat): answered question card becomes the user turn; two select types only UI ordering: answering a question card no longer echoes a duplicate user bubble. The combined answer still goes on the wire as a user message, but the chat pairs it back to its card (strict 'Prompt — Answer' match, now uniform for single questions too) and renders the card as the answered recap — the card IS the user turn, and the next assistant message streams below it. The pairing is derived from the transcript, so live and reloaded renders are identical; a dismissed card followed by an unrelated typed message does not match and renders normally. Messages ending with a question card also drop the copy/thumbs actions row — the card is an input surface, not a reactable assistant turn. Question types are now single_select and multi_select only: text is removed (the free-text 'Something else' row covers it) and confirm collapses into single_select with Yes/No options. multi_select rows toggle with a check and the free-text row's arrow submits the step; answers are comma-joined labels plus any typed entry. Agent-supplied catch-all options ('Other', 'Something else', 'None of the above') are stripped at parse — the card always provides its own free-text row; a question left with no real options is invalid. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * improvement(chat): question cards are single_select only Removes multi_select (and its toggle/check UI). The card is one shape: pick one option or type into the always-present 'Something else' row. Catch-all stripping and the transcript pairing/recap behavior are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * improvement(chat): bring back multi_select question cards Re-adds multi_select with a reworked interaction: option rows carry real checkboxes (emcn Checkbox chrome) instead of numbers and arrows, an option-styled Submit row confirms the step, and the "Something else" row reads as a plain option until clicked — then it becomes the focused text box, auto-checks, and can be unchecked without losing the typed text (blur with nothing typed reverts it). single_select behavior, catch-all stripping, and the transcript pairing/recap format are unchanged; multi_select answers are the checked labels comma-joined. * chore(copilot): regenerate mothership contract mirror (chat blob span attrs) * chore(copilot): regenerate mothership contract mirror (chat blob metrics) * feat(secrets): make output of generate api key a secret * feat(cli): add mkdir, mv, cp to mship tool set * feat(fork-chat): add fork chat to mothership * fix(fork-chat): fix messageid handling in fork chat * feat(credentials): agent-initiated oauth credential reconnect (#5488) * feat(credentials): agent-initiated oauth credential reconnect * fix(credentials): address reconnect review findings * improvement(credentials): log when connect draft name lookups degrade * fix(conflicts): remove migration * fix(conflicts): fix conflicts * fix(fork-chat): add migrations back * fix(ci): fix lint * fix(ci): fix bad import * fix(vfs): fix 500 char limit in vfs for skills and custom tools * feat(copilot): gate user skills to explicit slash-attach (#5536) Stop the mothership from adopting a workspace user-skill on its own: - Remove the load_user_skill tool and its three payload callers (chat payload, mothership execute route, inbox executor); delete lib/mothership/skills.ts + its test. Skills no longer autoload as the agent's own instructions. - Rename the workspace "## Skills" inventory to "## Agent Block Skills — NOT FOR YOU" with a one-line guardrail so a skill's description (e.g. "respond like a pirate") is not treated as an instruction. Skills reach the model as behavior only via explicit /-attach. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(lots-of-things): lots of things * feat(subagents): add persistent subagents * fix(copilot): let edit_workflow set knowledge-base tag filters, and stop it clearing them (#5546) * fix(copilot): persist KB tag subblocks as JSON strings from edit_workflow The edit_workflow tool normalizes array-with-id subblocks (via normalizeArrayWithIds) but only re-stringifies the keys listed in JSON_STRING_SUBBLOCK_KEYS. `tagFilters` (knowledge-tag-filters) and `documentTags` (document-tag-entry) were missing, so agent-authored tag filters were stored as raw JSON arrays while those UI components read their value with JSON.parse (expecting a string). The result: an agent edit to a Knowledge block's tag filter persisted correctly but rendered as an empty filter in the editor (JSON.parse on an array throws -> []). - Add `tagFilters` and `documentTags` to JSON_STRING_SUBBLOCK_KEYS so edit_workflow stores them in the same shape the UI writes. - Make both components' parsers tolerate an already-parsed array on read, self-healing values already persisted in the broken (array) shape. Search execution was unaffected (parseTagFilters accepts arrays), so the value was never lost — only the editor render and round-trip were broken. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(copilot): expose KB tag definitions in VFS meta.json Surface each knowledge base's defined tags (displayName -> tagSlot) inline in its meta.json via serializeKBMeta, loaded in one batched query (loadKbTagDefinitions), so the agent can bind a knowledge-tag filter to a real tag slot instead of guessing a tag name it cannot otherwise see. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(copilot): stringify KB tag subblocks on the nested-node edit path The nested-node merge path normalized array-with-id subblocks but never re-serialized the JSON_STRING_SUBBLOCK_KEYS, so editing a block nested in a loop/parallel container still persisted tagFilters/documentTags (and conditions/routes) as raw arrays -- the exact shape the subblock components cannot JSON.parse. Route all four write paths through a single normalizeSubblockValue helper so the normalize and re-stringify steps cannot drift apart again, and extract the duplicated string-or-array read logic into parseJsonArrayValue. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(copilot): tighten subblock serialization helpers Derive KbTagDefinitionSummary from the canonical TagDefinition instead of restating its fields, make parseJsonArrayValue generic so callers drop their `as T[]` casts, and unexport the three builders helpers that no longer have consumers outside the module now that normalizeSubblockValue fronts them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(copilot): stop stripping tagFilters/documentTags from the agent's workflow view sanitizeForCopilot dropped `tagFilters` and `documentTags` from the workflow state the agent reads (workflows/{name}/state.json), while edit_workflow is allowed to write both. The field was therefore write-only: on a follow-up edit the agent read back an absent field, concluded no filter was set, and cleared the user's tag filter. The redaction was introduced for workflow *export* (#1628) and is already enforced there by sanitizeWorkflowForSharing's key list. The duplicate in the copilot-only sanitizeSubBlocks was redundant for export and destructive for the agent. Removes it and pins the contract with a regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(copilot): reject malformed KB tag values instead of clearing the filter `knowledge-tag-filters` and `document-tag-entry` had no arm in the `edit_workflow` input validator, so they fell through to the pass-through default. Any non-array value the agent supplied -- a double-encoded JSON string, an object, an unparseable string -- reached `normalizeSubblockValue`, where `normalizeArrayWithIds` coerces unparseable input to `[]`. The write path then persisted `"[]"` over the tag filter the user had configured. `condition-input` and `router-input` already guard against exactly this and return an actionable error to the model. Extend that arm to cover the two KB subblock types. It keys on subblock type, so the unrelated `tagFilters` short-input on the Algolia block is unaffected. `null`/`undefined` and empty arrays still clear the field, so intentional clears keep working. Also wrap `loadKbTagDefinitions` in try/catch. Tag definitions are an optional meta.json enrichment, but the query ran inside the top-level `Promise.all`, so a transient failure would reject the entire workspace VFS materialize and leave the agent unable to read any file. Now it degrades to a meta.json without tag definitions, matching the sibling materializers. Adds regression tests for both, plus the first tests for `parseJsonArrayValue`, the helper that keeps pre-fix raw-array rows readable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(copilot): collapse duplicate JSON-array parsing in edit-workflow builders `normalizeArrayWithIds` and `normalizeConditionRouterIds` each hand-rolled the same "accept a raw array or the JSON string these subblocks persist" parse. Extract `parseJsonArray`, which returns null when the value is neither, so each caller keeps its own distinct fallback: `[]` for the former, the untouched original value for the latter. Behavior-preserving. An empty array is truthy, so `[]` and `"[]"` still parse through rather than hitting either fallback. `validation.ts` has a third copy, but `builders.ts` already imports from it, so sharing the helper across the two would introduce an import cycle. Left as is. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(copilot): specify tag name and legal operators in KB meta.json `tagDefinitions` exposed `displayName`, but a `tagFilters` entry must carry the key `tagName`. An entry written with `displayName` passes validation and persists, then filters nothing -- a silent failure. Rename the field at the serializer boundary; the DB column is untouched. Also emit the operators legal for each tag's `fieldType`, reusing `getOperatorsForFieldType`. `between` is valid for number and date but not for text or boolean, and the agent has no way to infer that. An unrecognized fieldType yields an empty list rather than throwing. Still unspecified, and deliberately out of scope: a filter entry's value key is `tagValue` (but `value` on documentTags), and `between` needs `valueTo`. Those describe the subblock entry shape, not the knowledge base, so meta.json is the wrong place for them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(copilot): pass a nullish subblock clear through instead of serializing "[]" `validateValueForSubBlockType` accepts null as an explicit clear, but `normalizeSubblockValue` then ran it through `normalizeArrayWithIds`, which coerces any non-array to `[]`, and persisted the string "[]". No data is lost either way -- "[]" and an absent field both mean "no filters". But it left the field present when the caller asked for it to be unset, so `sanitizeForCopilot` showed the agent an empty filter rather than an absent one, contradicting the absent-means-unset invariant the sanitizer documents. It also made Algolia's `if (params.tagFilters)` see a set value, since "[]" is truthy. An explicitly empty array still serializes to "[]" -- clearing with a value is distinct from clearing by omission. Reported by Cursor Bugbot on #5546. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(changes): huge changes * fix(subagents): lanes * fix(mship): transcript stuff * fix(subagents): thinking lanes * fix(superagent): fix superagent tools and checkpoints * fix(scope): scope subagent tools * fix(lint): fix lint * chore(db): regenerate workspace_files.message_id migration as 0260 on staging base * fix(superagent): fix superagent integration tools * improvement(questions): make something else a placeholder * chore(copilot): regenerate mothership contract mirror after staging rebase * feat(mship): add external mcps to mship * fix(ci): fix dev build * fix(stream): show thinking text * fix(ci): force redeploy * fix(mothership): keep chat forks outside workspace storage billing Preserve the product invariant that Mothership chat files are not charged as workspace file storage after the billing storage merge. * fix(uploads): restore listWorkspaceFiles throwOnError option dropped in rebase * fix(subagent-streaming): remove italics * fix(mothership): treat subagent lanes closed by subagent_end as settled so the between-steps thinking indicator isn't suppressed * fix(ui): thinking loader and rool names * fix(ui): add file * fix(thinking): show thinking during subagents * fix(chat): drop dead thinking-channel ternary after lanes skip thinking blocks * fix(thinking): remove thinking text * improvement(function execute): add timeout to function execute and stop showing text in subagents * fix(subagents): hide thinking text * fix(ff): move ff to go * improvement(superagent): nuke superagent * feat(main-agent): superagent into main agent * chore(db): regenerate workspace_files.message_id migration as 0262 on staging base * fix(credentials): restore reconnect params on shared createConnectDraft * fix(migrations): rebase with staging --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Emir Karabeg <emirkarabeg@berkeley.edu> Co-authored-by: Justin Blumencranz <96924014+j15z@users.noreply.github.com> Co-authored-by: Vikhyath Mondreti <vikhyath@simstudio.ai>
Summary
oauth_get_auth_linkaccepts an optionalcredentialId(tool param + prompts land in the companion mothership PR)/api/auth/oauth2/authorizeacceptscredentialId: requires credential-admin access (same check as the draft POST route), rejects cross-workspace / non-oauth / provider-mismatched ids, and threads the id into the pending credential draft so the existinghandleReconnectCredentialpath rebinds the samecredential.idto the fresh account rowcredentialId(or null) on both the insert and conflict paths — a plain connect can never inherit a stale reconnect draft and silently rebind an existing credentiallib/credentials/display-name.tsand applied it to copilot-path connects — agent-created credentials get "Justin's Gmail 2" instead of identical duplicate names; name-lookup failures degrade to the un-numbered default with alogger.warncredentialId(derived from the URL, not from model-emitted tag fields)ensureWorkspaceAccessnow returns the resolvedWorkspaceAccess(previously discarded) so the tool handler reuses it for the credential-admin lookup instead of re-queryingMerge order: this PR merges before the mothership companion — the new tool param crosses the tool-catalog codegen boundary.
Type of Change
Testing
bun run lintclean,bun run check:api-validation:strictpassed, tsc cleancredential.id, swaps the account row, deletes the orphaned account, dependent workflow still runsChecklist
Screenshots/Videos
🤖 Generated with Claude Code
Companion PRs