feat: add Salesforce connector (OAuth App + custom MCP tools)

[yiboyasss]

Summary

• Adds a Salesforce connector using the same static-client OAuth + custom FastMCP pattern already used for GitHub/Linear/Jira/Slack/Zoom/HubSpot: a seeded oauth_providers + public_mcp_apps row pair, and a new tools/mcp/salesforce.py module wrapping the Salesforce REST API (SOQL query, SOSL search, sobject listing/describe, and non-destructive record read/create/update (no delete tool -- this is the only connector here that could otherwise delete shared business records, and HubSpot, the other CRM connector, ships none either) — generic enough to cover any standard or custom object, since Salesforce orgs are highly customizable with potentially thousands of custom object types).
• Salesforce "Connected Apps" require no platform-level review to function — registering one and sharing the consumer key/secret is immediately usable by any Salesforce org via the generic login.salesforce.com/test.salesforce.com entry points (which resolve to the user's actual org during login).
• A real architectural gap this connector required closing, not just a per-provider quirk branch: Salesforce's token response returns a per-org API host (instance_url) instead of using a fixed domain, and every subsequent API call must go through it. No existing connector needed anything beyond the access token itself, so this required:
  • A new user_oauth.instance_url column (migration + model change).
  • Persisting it generically in generic_oauth_callback (initial connect) and refresh_oauth_token_if_needed (Salesforce can return a new instance_url on refresh, e.g. after an org migration).
  • Threading it through _LegacyOAuthTokenResolution and _build_oauth_mcp_stdio_transport_config so launch_config.env_mapping can map a second field ("instance_url", alongside the existing "access_token") into a launch env var.
• The OIDC userinfo endpoint is the one exception: it's documented as the fixed login.salesforce.com host (not instance_url), since Salesforce routes it internally based on the token — salesforce_get_current_user calls that fixed URL directly rather than going through instance_url like every other tool here.

Test plan

• tests/alembic/test_20260818_add_instance_url_to_user_oauth.py — the new column migration (upgrade/idempotent/downgrade)
• tests/alembic/test_20260818_seed_salesforce_mcp_app.py — seed migration insert/idempotency/downgrade, and drift check against builtin_mcp_registry.py
• tests/web/tools/test_salesforce_mcp.py — headers/instance_url validation, array-shaped error handling, and all 8 Salesforce tools (query, search, list_sobjects, describe_sobject, get_current_user, get_record, create_record, update_record)
• tests/web/test_salesforce_oauth.py — instance_url persisted on connect and on refresh; skipped identity lookup (empty userinfo_url) doesn't crash; non-Salesforce providers unaffected
• tests/web/tools/test_oauth_launch_config_static_env.py — new tests for the instance_url env_mapping case (forwarded when provided, omitted when not)
• ruff check / ruff format / mypy / pre-commit hooks all pass
• Regression: existing Zoom/Slack/Intercom/generic-oauth-login tests, plus the WebToolConfig/legacy-oauth-resolution test files, all still pass after the shared auth.py/tools/config.py/UserOAuth changes

[gemini-code-assist]

Code Review

This pull request introduces a built-in Salesforce (OAuth) MCP connector, adding support for per-org API hosts by persisting an instance_url column in the user_oauth table. It implements several Salesforce tools for querying, searching, and managing records, along with database migrations and comprehensive tests. Feedback on the changes highlights a potential path traversal vulnerability in the request path, a runtime crash in the search tool due to an unexpected API response format, and the use of logging.basicConfig() which overrides global configurations. Additionally, it is recommended to use sa.inspect(bind) instead of the legacy Inspector.from_engine(bind) in the migration script.

[yiboyasss]

/gemini review

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[rogercloud]

PR Summary

This PR adds a Salesforce connector following the existing OAuth App + custom FastMCP tool template used by other connectors (e.g. HubSpot, Jira). Because Salesforce, unlike other providers, requires a per-org API host (instance_url) returned at token-exchange time and needed on every subsequent API call, the PR generalizes the OAuth/launch-config plumbing to carry a second provider-specific value alongside the access token, plus a migration adding instance_url to UserOAuth. The core template reuse and migration are sound, but the generalization is only half-wired through the codebase, and the token lifecycle (refresh, reconnection signaling) for this specific provider isn't fully modeled.

Blocking: yes — recommended event: REQUEST_CHANGES

Design-level assessment (Round 0)

Verdict: acceptable-with-reservations. The decision to persist instance_url in the schema and thread it through the OAuth token resolver is the right shape for this provider — a "second generic launch-config field" alongside the access token, matching the template's per-connector customization points elsewhere. However the threading is incomplete (see Major #1), and the refresh-time update path this PR adds is unreachable in production (see Major #2), so the overall provider integration isn't yet functionally complete.

A design-level comparison to Jira's connector was considered during this review — Jira resolves its instance host at runtime via Atlassian's accessible-resources discovery endpoint, and the question was whether Salesforce should do the same instead of persisting instance_url in the schema. This was checked and ruled inapplicable: Salesforce's userinfo endpoint does not expose an instance host the way Atlassian's dedicated discovery endpoint does, so schema-based persistence of instance_url is technically necessary here, not an unjustified divergence from the Jira pattern. No action needed on this point.

Findings

Major (blocking)

1. src/xagent/web/tools/config.py:3264-3271 — hook-path OAuth token resolver silently drops instance_url
The hook-token call site of _build_oauth_mcp_stdio_transport_config omits the instance_url= kwarg entirely, unlike the legacy DB-path call site at config.py:3312-3317 which passes it correctly. Root cause is structural: _ResolvedHookToken (config.py:257-261) and the public ResolvedToken contract (config.py:128-140) are frozen dataclasses with no field to carry instance_url at all. Any embedder using a custom OAuth token-resolver hook gets a Salesforce connector that launches successfully but fails on the first real tool call with ValueError: SALESFORCE_INSTANCE_URL environment variable is missing (src/xagent/web/tools/mcp/salesforce.py:44-54) — every tool except salesforce_get_current_user breaks. No test exercises this path (tests/web/tools/test_oauth_token_resolver_hook.py has zero references to Salesforce/instance_url). This is a regression introduced by this PR's own diff, which added the instance_url parameter but wired only one of its two call sites.
Fix: add an instance_url field to _ResolvedHookToken/ResolvedToken and pass it through at the hook-path call site, symmetric with the DB path.

2. src/xagent/web/api/auth.py:1665-1671 and src/xagent/web/tools/config.py:562-563,687-692 — refresh-time instance_url update logic this PR adds is dead code in production
auth.py:1665-1671 only sets UserOAuth.expires_at when the token response contains expires_in; Salesforce's OAuth token response does not include expires_in (per the PR's own fixture, tests/web/test_salesforce_oauth.py:96-107). config.py:562-563 treats a NULL expires_at as "assume valid, no refresh needed", so refresh_oauth_token_if_needed returns early before ever reaching the refresh POST for any Salesforce grant. The new instance_url-on-refresh code added at config.py:687-692 is therefore only reachable in the one test that manually forges a past expires_at (test_salesforce_oauth.py:235) — never in real usage. Compounding this, no connector under src/xagent/web/tools/mcp/ implements 401/session-expiry-triggered re-authentication, so once a Salesforce org session expires server-side there's no automatic recovery — just a raw RuntimeError on the next tool call requiring manual reconnect. This ships a code path that looks like working refresh support but is silently unreachable, creating false confidence with no test to catch the gap.
Fix: either derive an appropriate expires_at for Salesforce (Salesforce sessions don't have a fixed lifetime, so this may require a different signal), or explicitly document/handle the "no expiry info" case and add a real reconnect-signal path for 401s.

Minor (non-blocking)

3. src/xagent/web/tools/mcp/salesforce.py:126-134 — path/query injection via unencoded path segments beyond the literal ".." check
_request's only guard is if ".." in path: raise ValueError(...) — a literal-substring blocklist with no percent-decoding and no rejection of /, ?, #. Call sites (~241, 272-293, 307, 331, 343-361) interpolate LLM-controlled sobject_type/record_id raw into the URL path. Sibling connectors hubspot.py:262-274 and jira.py:66-73 both percent-encode with quote(value, safe=""). Concretely: record_id="001x?fields=Id" injects query params; sobject_type="Account/001abc" can retarget the endpoint; percent-encoded traversal (%2e%2e%2f) bypasses the ".." check entirely. Not a privilege-boundary crossing, but an endpoint-redirection/parameter-injection primitive on a connector with DELETE capability. Existing test test_request_rejects_path_traversal (tests/web/tools/test_salesforce_mcp.py:69-78) only covers the literal ".." case.
Note: gemini-code-assist flagged the shallower literal-".." gap on this line in the prior review round; that narrower suggestion was silently adopted (code now has the ".." check). This finding is the broader gap gemini didn't originally catch.
Fix: percent-encode each interpolated segment like the sibling connectors do.

4. src/xagent/web/tools/config.py:3038-3045 — env_mapping dispatch silently drops the env var when instance_url is falsy
elif token_type == "instance_url" and instance_url: env[env_key] = instance_url — when instance_url is None (pre-migration row, hand-restored row, or via the hook-path gap in #1), the connector is still built as "launchable" and only fails later with a raw exception, instead of surfacing the existing oauth_token_required-style reconnect signal used elsewhere for missing access tokens. Distinct root cause from #1 (dispatch logic vs. missing kwarg) — fixing one doesn't fix the other.
Fix: when token_type == "instance_url" and the value is missing, surface the same reconnect-required signal used for missing access tokens instead of silently omitting the env var.

5. src/xagent/web/builtin_mcp_registry.py:225-232 — userinfo_url: "" justified by a comment the connector's own code contradicts
The comment claims "no fixed URL this callback's lookup could use" for Salesforce identity, but salesforce.py:19-22,138-158 hardcodes exactly such a URL (https://login.salesforce.com/services/oauth2/userinfo) and uses it successfully via the same Bearer-token scheme the generic callback already uses (auth.py:1613-1617). Consequence: UserOAuth.email/provider_user_id stay NULL for every Salesforce grant (intentional and tested per tests/web/test_salesforce_oauth.py:89-137), so the connected-account UI label never populates for Salesforce, though is_connected itself still works. user_id_path/email_path remain dead config as a result. No test exercises /api/mcp/apps for Salesforce's is_connected/connected_account combination (comparable coverage exists for Meta: tests/web/test_meta_oauth.py:1220).
Fix: either point userinfo_url at the fixed endpoint salesforce.py already uses, or correct the comment and add a Meta-style test asserting the intentional "connected but unlabeled" state.

6. src/xagent/web/api/auth.py:1237-1242 — PKCE code_verifier travels through the browser inside a signed-but-unencrypted JWT state, untested
state_payload embeds code_verifier before create_access_token (HS256-signed, not encrypted — decodable by anyone without the secret). This state goes out as a URL query param to Salesforce's /authorize and returns via the callback, exposing the verifier to browser history, Referer headers, and proxy/access logs — the exact channel PKCE protects against. The inline comment at auth.py:1233-1236 claiming it "never leaves the server unencrypted" is factually incorrect. The repo's own MCP OAuth flow does this correctly (tests/web/api/test_mcp_oauth_flow.py:640-650: verifier stored server-side, encrypted, never on the wire). Severity is tempered because the token exchange still requires the server-held client_secret (~auth.py:1561-1562), so this is defense-in-depth, not directly exploitable alone. No test covers this path (test_salesforce_oauth.py has zero occurrences of code_verifier/code_challenge).
Fix: correct the misleading comment, add test coverage, and consider aligning with the existing encrypted server-side pattern.

7. src/xagent/web/tools/mcp/salesforce.py:162-181 — no pagination beyond first 2000 SOQL rows; no response-size cap the other direction
salesforce_query reports truncated=not result.get("done", True) but drops nextRecordsUrl and provides no continuation tool — a >2000-row result's remainder is permanently unrecoverable. Separately, salesforce_get_record (line 272) has no default/bounded field list, so fields="" or SELECT FIELDS(ALL) can return arbitrarily large payloads into the LLM context.
Fix: add a salesforce_query_more-style continuation tool using nextRecordsUrl, and consider a default field cap for get_record.

8. src/xagent/web/tools/mcp/salesforce.py:295-311 — salesforce_create_record discards the errors array on failure
On a {"success": false, "errors": [...]} response shape, the code returns {"id": None, "success": false} with errors dropped. Note: Salesforce's create endpoint typically returns validation failures via non-2xx status (already surfaced through the exception path in _request_absolute), so this specific shape may not be reachable today — still worth guarding defensively.
Fix: propagate errors in the return payload when present.

9. salesforce.py:272 (salesforce_get_record), salesforce.py:343 (salesforce_delete_record) — no non-empty validation on record_id/sobject_type
No check that record_id/sobject_type are non-empty before building URLs (an empty record_id collapses to a collection-level URL). salesforce_update_record guards empty fields (line 328) but nothing guards empty ids anywhere. Sibling connector hubspot.py:246-274 has exactly this helper (_require_clean_identifier/_url_path_id) that this connector lacks.
Fix: add an identifier-validation helper mirroring HubSpot's, applied to all id/type-bearing calls.

10. src/xagent/web/builtin_mcp_registry.py:222-223 — hardcoded login.salesforce.com, sandbox orgs cannot connect
Hardcodes the production auth host; the registry's own comment names test.salesforce.com as the sandbox alternative, but no config/env var exposes switching — a sandbox-org user has no path to connect except an admin creating a duplicate provider row.
Fix: expose a config/env toggle for the sandbox host, or at minimum document this limitation in example.env.

11. tests/web/test_salesforce_oauth.py:152-205 — test_non_salesforce_callback_does_not_persist_instance_url can't fail for the reason its name suggests
It asserts instance_url is None when the token response lacks the key, but token_data.get("instance_url") also returns None for a missing key regardless of whether the if "instance_url" in token_data: guard exists — deleting that guard wouldn't break this test, so it doesn't actually prove the guard's presence.
Fix: strengthen the test to actually exercise the guard (e.g. assert a sentinel isn't overwritten, or mock the dict-membership check directly).

12. src/xagent/web/tools/mcp/salesforce.py:67-86 — untested dict-shaped error branch in _extract_error_detail
When the JSON error payload is a dict (not a list), the function returns None and falls back to raw response text — genuinely untested. All existing error-path tests in tests/web/tools/test_salesforce_mcp.py mock array-shaped error bodies (realistic for /services/data/* endpoints), but salesforce_get_current_user's OIDC userinfo endpoint actually returns dict-shaped errors ({"error": ..., "error_description": ...}) on failure — that fallback path has zero coverage.
Fix: add a test covering the dict-shaped error response for the userinfo endpoint.

Simplification opportunities

• src/xagent/migrations/versions/20260818_seed_salesforce_mcp_app.py:36: shrink: redundant second sa.table() definition (OAUTH_PROVIDERS_TABLE, only provider_name) duplicating FULL_OAUTH_PROVIDERS_TABLE. The equivalent Jira migration (20260818_seed_jira_mcp_app.py) selects FULL_OAUTH_PROVIDERS_TABLE.c.provider_name directly with no second table object — drop the redundant one here to match.

net: -3 lines possible

Review completeness notes (checked, no action needed)

A few items raised in the prior gemini-code-assist review round were re-verified this round and require no further action:

• sa.inspect(bind) vs. legacy Inspector.from_engine(bind) in 20260818_add_instance_url_to_user_oauth.py: confirmed fixed in a later commit (e3ca2bbef) matching gemini's suggestion, even though the author never replied to that thread — safe to resolve.
• SOSL search response shape (searchRecords dict access): the author's rebuttal is confirmed technically correct — Salesforce's SOSL endpoint does return a dict, matching the existing code/test. No action.
• logging.basicConfig() in salesforce.py: the author's rebuttal is confirmed correct — this module only runs as a standalone subprocess entrypoint, and 19 other connector modules use the identical pattern. No action.

Additionally, two items were considered but downgraded/dropped after verification: the env_mapping if/elif dispatch is fine as-is for 2 fields (a full dict refactor would be premature abstraction) — only worth a light note to log a warning on unmapped token_type values; and the lack of host/scheme validation on SALESFORCE_INSTANCE_URL is a structural trait shared by every OAuth provider row in this system, not Salesforce-specific, so it's downgraded to an informational note rather than a standalone finding.

Blocking status & recommended decision

Blocking: yes

Blocking issues:

• [new] Major #1 — hook-path OAuth token resolver silently drops instance_url (config.py:3264-3271)
• [new] Major #2 — refresh-time instance_url update logic is unreachable dead code in production (auth.py:1665-1671, config.py:562-563,687-692)

Neither issue was raised in the prior gemini-code-assist review round — both are newly identified in this review.

Recommended event: REQUEST_CHANGES

[rogercloud]

PR Review: feat: add Salesforce connector (OAuth App + custom MCP tools) — Re-review (Round 2)

1. PR Summary

This PR adds a Salesforce connector via OAuth App plus a set of custom MCP tools (query, get/create/update/delete record, describe sobject, list sobjects, get current user). Unlike every other existing connector (GitHub/Linear/Jira/Slack/Zoom/HubSpot), Salesforce's OAuth token response returns a per-org instance_url that every subsequent API call must target instead of a fixed base URL. The PR generalizes the OAuth/launch-config plumbing with a data-driven env_mapping mechanism so a provider can carry a second value (instance_url) alongside the access token, rather than special-casing Salesforce in the transport layer. Migrations are correct (single head, idempotent, verified against alembic's own ScriptDirectory.get_heads() and the full migration test suite), and leaving instance_url unencrypted is consistent with the sibling access_token/refresh_token columns already unencrypted in the same table.

Blocking: yes — recommended event: REQUEST_CHANGES

2. Update Summary

Six commits landed in response to the prior review round, fixing 9 of 12 prior findings outright (2 of those 9 with minor test/doc caveats noted below), while one prior finding (dead refresh-time instance_url code, no session-expiry recovery path) remains open and one (a tautological non-regression test) remains open. A fresh design and discovery pass on the current head surfaced 14 new findings — mostly minor/nit — but two of them are MAJOR test-coverage gaps in the very PKCE feature this update round just added and fixed.

3. Round 0 Design Verdict

Acceptable with reservations — one likely-blocking correctness gap. The core architectural choice — carrying instance_url through OAuth/launch-config via a data-driven env_mapping rather than hardcoding Salesforce into the transport layer — is the right shape for a provider whose API origin is per-org rather than fixed. It generalizes cleanly and doesn't special-case Salesforce where it doesn't need to. The unencrypted instance_url column is a correct, deliberate choice, not a security gap: encrypting only the new field while the sibling access_token/refresh_token columns in the same table remain unencrypted would be security theater. A JSON-metadata-blob alternative was considered and correctly rejected — it would still require a schema decision, would risk unfiltered persistence of sensitive token_data keys, and is premature abstraction at N=1. The per-field touch-point cost (~9 places per new field) is real but doesn't yet justify a Mapping-based refactor at N=2 — YAGNI applies. The one correctness gap that keeps this from a clean accept is the dead refresh path / no session-expiry recovery (prior finding #2, still open — see Blocking section).

4. Prior Findings Status

1. FIXED — Hook-path OAuth resolver silently dropped instance_url, asymmetric with the DB-path call site. Fixed by 0574904cd/671f5f667: both call sites now pass instance_url= symmetrically, field added to ResolvedToken/_ResolvedHookToken, new tests added.
2. NOT FIXED — MAJOR, BLOCKING. Refresh-time instance_url update code (config.py:709-716) is unreachable in production: Salesforce's token response has no expires_in (auth.py:1681 only sets expires_at when that key exists), so config.py:584-585's if not oauth_account.expires_at: return True skips the refresh check entirely for every real Salesforce grant. No 401-triggered reconnect signal exists either (salesforce.py tools blanket-catch Exception into success-shaped error JSON). The promised follow-up issue tracking this was not found filed on this repo. This is still open and blocking.
3. FIXED — Path/query injection via unencoded id/type segments. Fixed by 1462508d5: new _url_path_id() (_require_clean_identifier() + quote(..., safe="")) applied at all 5 call sites, with tests reproducing the original injection payloads.
4. FIXED — env_mapping silently dropped the env var on falsy instance_url instead of signaling reconnect. Fixed by 0574904cd/671f5f667: new _OAuthInstanceUrlRequired exception routes to the same oauth_token_required unavailable-config signal used for missing access tokens.
5. FIXED — Self-contradictory userinfo_url: "" comment. Fixed by 3f3a35d58 (doc-only correction); behavior unchanged, existing test covers the resulting state.
6. FIXED — PKCE code_verifier traveled in a signed-but-unencrypted JWT state, with a misleading comment. Fixed by 4e278a02d: verifier now genuinely encrypted via encrypt_value()/decrypt_value() (Fernet), non-Salesforce flows unaffected, comment corrected. The underlying encryption mechanism is correctly implemented. (Note: separately re-flagged by new findings K/L below for lacking real regression coverage — that's a distinct, newly-found test gap.)
7. STILL OPEN — non-blocking. No pagination for salesforce_query (nextRecordsUrl dropped, truncated=True is a dead end) and no field cap for salesforce_get_record. Author agreed but scoped as follow-up; no tracking issue found filed.
8. FIXED — salesforce_create_record discarded the errors array on {success:false, errors:[...]}. Fixed by 1462508d5, refined by 671f5f667.
9. FIXED (code); test coverage PARTIAL. No non-empty validation on record_id/sobject_type. Fixed by 1462508d5 via _require_clean_identifier() at all 5 call sites; dedicated rejection tests exist only for get_record/delete_record, not describe_sobject/create_record/update_record.
10. FIXED (documentation, satisfies the original either/or bar). Hardcoded login.salesforce.com with no sandbox path. Documented in example.env (6b354a792) — meets the "at minimum document" bar (see new finding F for a completeness follow-up).
11. NOT FIXED — minor. test_non_salesforce_callback_does_not_persist_instance_url (tests/web/test_salesforce_oauth.py:152-205) can't catch a regression in the "instance_url" in token_data guard — it only asserts None on a fresh row, true either way. Fix: pre-seed an existing instance_url and assert it's preserved unchanged.
12. FIXED — Untested dict-shaped error branch in _extract_error_detail. New test test_get_current_user_falls_back_to_raw_text_on_dict_shaped_error confirmed to exercise the branch.

5. New Findings

Major

K. Tautological PKCE-encryption test (empirically verified). tests/web/test_generic_oauth_login.py:170-210 — its assertions (encrypted_verifier != code_challenge, and a decrypt round-trip) pass 18/18 even with encryption removed entirely from the source, because a verifier trivially differs from its own derived challenge and decrypt_value passes through non-ciphertext unchanged. The test's docstring claims to prove the verifier isn't left as plaintext; it does not. Suggest asserting state_payload["code_verifier"] != decrypted_verifier, or that the stored value looks like a Fernet token.

L. Zero test coverage of the PKCE callback path (empirically verified). tests/web/test_salesforce_oauth.py:76-87 — removing the line in auth.py that adds code_verifier to the token-exchange POST body still leaves all 21 tests (across both this file and test_generic_oauth_login.py) passing. Combined with K, the entire PKCE feature — the subject of dedicated fix commit 4e278a02d for a prior review finding — is untested end-to-end, even though the encryption mechanism itself is correctly implemented. Suggest asserting mock_post.call_args.kwargs["data"]["code_verifier"] == <original unencrypted verifier>.

Minor

A. src/xagent/web/tools/mcp/salesforce.py _instance_url() (~L45-55), config.py (~L2929-2936), auth.py (~L1680) — no scheme/host validation on instance_url (only non-empty check). Salesforce is the only connector where the entire outbound API origin comes from provider-returned data rather than a hardcoded constant. Every practical attack route already requires privileged access, so this is defense-in-depth, not exploitable — but worth validating https:// + *.salesforce.com/*.force.com suffix at persist time.

B. src/xagent/web/api/auth.py (~L1243-1245) — PKCE gated on literal provider.lower() == "salesforce"; OAuthProvider has no PKCE-capability column. An admin-created row like "salesforce-sandbox" would silently get no PKCE and fail opaquely against a PKCE-enforcing org. Suggest a capability column or at minimum .startswith("salesforce").

D. src/xagent/web/tools/config.py (~L3072-3078, ~L3374) — the env_mapping dispatch if/elif chain has no else/fallback for an unrecognized token_type; a typo'd mapping value would silently produce neither env var nor error. Also the legacy/DB-path's except _OAuthInstanceUrlRequired handler (~L3374) has no dedicated test (only the hook-path equivalent is tested).

E. src/xagent/web/tools/mcp/salesforce.py — no 429/Retry-After handling, unlike jira.py/slack.py/intercom.py. Salesforce enforces hard per-org daily API limits and commonly returns 429/503, making this the connector family where it matters most. Follow-up, not blocking.

F. example.env — the new sandbox-org note is accurate about the env var but could mislead readers into thinking there's no workaround at all; an admin can edit the oauth_providers.auth_url/token_url row directly (same path as finding B), though salesforce.py's USERINFO_URL is a separate hardcoded constant unaffected by that edit.

H. src/xagent/web/api/auth.py (~L1494-1496) — the callback uses non-strict decrypt_value() for the PKCE verifier, which silently returns raw ciphertext on decryption failure (e.g. ENCRYPTION_KEY rotation mid-flight) instead of raising. decrypt_value_strict() already exists and is documented for exactly this case.

I. src/xagent/web/api/auth.py (~L1249) — encrypt_value() doesn't swallow the ValueError get_cipher() raises when ENCRYPTION_KEY is unset in non-development. Since PKCE is exclusive to Salesforce, /api/auth/salesforce/login uniquely 500s on this misconfiguration while every other provider degrades to a plaintext-secret pass-through.

J. src/xagent/web/tools/mcp/salesforce.py _require_clean_identifier/_url_path_id (~L132-162) — byte-identical logic to hubspot.py's equivalents (~L246-275), differing only in docstrings. mcp/utils.py already exists and is already imported by salesforce.py.

M. tests/web/tools/test_salesforce_mcp.py (~L455-471) test_create_record_propagates_errors_on_failure — mocks HTTP 200 with {"success": false, "errors": [...]}, but real Salesforce create failures return non-2xx with a JSON-array body (a different, already-tested path).

Nit

G. .codespellrc — adds createable to the repo-wide ignore-words-list, when the codebase already has precedent for scoped # codespell:ignore <word> inline comments (e.g. checkin). Suggest inline suppression at the 3 salesforce.py usage sites instead.

N. tests/web/tools/test_salesforce_mcp.py (~L53-66) test_request_uses_instance_url_and_headers — uses /services/oauth2/userinfo as its example path, but in production salesforce_get_current_user always calls the fixed-host USERINFO_URL via _request_absolute, never through _request. Assertions are correct for what's tested, but the example path is misleading about actual routing.

6. Simplification Opportunities

• L132-162: shrink Duplicate _require_clean_identifier/_url_path_id vs hubspot.py:246-275. Extract to mcp/utils.py.
• L1: shrink Global codespell ignore for createable. Use inline # codespell:ignore at the 3 salesforce.py usage sites instead.

net: -10 lines possible

7. Blocking Status & Recommended Decision

Blocking: yes

• [prior] Finding #2 — dead refresh-time instance_url code, no session-expiry recovery path. src/xagent/web/api/auth.py:1681, src/xagent/web/tools/config.py:584-585.
• [new] Finding K — tautological PKCE encryption regression test. tests/web/test_generic_oauth_login.py:170-210.
• [new] Finding L — zero test coverage of the PKCE callback token-exchange path. tests/web/test_salesforce_oauth.py:76-87.

Recommended event: REQUEST_CHANGES

[yiboyasss]

Two round-2 findings that don't map to a specific inline thread, addressing here:

#11 (test_non_salesforce_callback_does_not_persist_instance_url can't prove the guard). The suggested fix (pre-seed an existing instance_url, assert it's preserved across reconnect) doesn't fit the actual persistence model: generic_oauth_callback unconditionally DELETEs any existing UserOAuth row for that (user, provider) pair and inserts a fresh one on every callback (src/xagent/web/api/auth.py) -- there's no update-in-place path where a prior value could survive a reconnect, for instance_url or any other field. This is also why the if "instance_url" in token_data guard itself was removed entirely in 4e278a02d rather than kept and tested: token_data.get("instance_url") is already the correct final value on a row that's always freshly created, so there was no branch left for a test to distinguish. Not changing this test further -- the guard it was written to protect no longer exists, by design.

Codespell createable (simplification note, "shrink"). Two of the three occurrences are plain code (s.get("createable") / f.get("createable")) and would take an inline # codespell:ignore createable fine. The third is inside salesforce_create_record's docstring -- user/LLM-facing tool description text -- where an inline suppression comment would literally appear in that string. Rewording to avoid the word loses precision, since createable is the literal Salesforce API field name being referenced. Migrating only the two code occurrences while keeping the global ignore-list entry for the docstring one doesn't shrink anything (adds two comments, removes zero), so leaving the global entry as-is.

[rogercloud]

PR summary

This PR adds a built-in Salesforce OAuth/MCP connector and persists each grant's Salesforce instance_url alongside its OAuth tokens. The callback, refresh, hook/legacy resolver, and stdio launch paths then carry that per-grant origin into a FastMCP process whose tools cover SOQL/SOSL, object metadata, and generic CRUD for standard and custom objects. It also adds the registry/seed migrations, PKCE handling, and the associated production and test wiring.

Blocking: yes — recommended event: REQUEST_CHANGES

Update summary since 671f5f667

Since 671f5f667, four commits landed: d394ecc4 hardens PKCE tests, 57ab5b3c warns on unknown env_mapping token types and adds a legacy test, 7b16bb0f validates Salesforce instance hosts, moves the Salesforce identifier helper into shared utils, and adds tests, and 38bcb49d catches missing-key ValueError during PKCE decrypt. These updates fix the previously reported hook/env and PKCE/decrypt issues, add the host-validation and Salesforce-side helper changes, and preserve error-array propagation; residual dot-segment handling, call-site coverage, and the HubSpot half of the helper migration remain open, while the output-bound and migration-provenance issues below are new confirmed blockers.

Approach verdict

Acceptable with reservations. The per-grant instance_url flow through UserOAuth, callback/refresh, hook and legacy resolution, launch environment mapping, and Salesforce stdio transport is coherent for production orgs, and the generic object tools avoid a per-object registry expansion. The reservations are material at the connector output and migration-ownership boundaries; production/sandbox realm coexistence and an explicit PKCE capability remain deferred to the tracked #1542 follow-up rather than modeled end-to-end here.

Canonical prior-status table

Root	Verified status	Disposition
G01	FIXED	Hook-path resolver now preserves instance_url.
G02	NOT FIXED; DROPPED/DEFERRED	Tracked in open #1540; no duplicate follow-up here.
G03	PARTIAL	Slash/query/encoded traversal and empty/whitespace validation are fixed; literal dot-segment residual is reported below.
G04	FIXED	Missing instance_url now prevents a half-configured launch.
G05	SPLIT	Contradictory comment is FIXED; the identity/connected_account label remains NOT FIXED and is reported below.
G06	FIXED	PKCE verifier is no longer exposed in plaintext state.
G07	NOT FIXED; DROPPED/DEFERRED	SOQL continuation is tracked in open #1541; no duplicate follow-up here.
G08	SPLIT	Omitted-field get_record behavior is DROPPED/DEFERRED to #1541; query/list/describe output capping is confirmed and reported below.
G09	SPLIT	Error propagation is FIXED; the unsupported synthetic false-success candidate is DROPPED; the create-failure test contract is severity-adjusted to minor and reported below.
G10	SPLIT	Production validation is FIXED; call-site coverage is PARTIAL and reported below.
G11	SPLIT	Original hardcoded-host documentation is FIXED; stronger coexistence design is DROPPED/DEFERRED to #1542.
G12	REFACTORED	No residual tautological callback-test issue.
G13	FIXED	Dict-shaped error handling has direct coverage.
G14	DROPPED/DEFERRED	PKCE capability modeling is tracked in #1542.
G15	FIXED	Unknown env_mapping token types now warn.
G16	DROPPED/DEFERRED	Rate-limit handling is tracked in #1543.
G17	FIXED	PKCE callback decryption is strict.
G18	FIXED	Missing encryption-key handling is covered.
G19	PARTIAL	Salesforce now uses shared utils; HubSpot's duplicate implementation remains and is reported below.
G20	DROPPED	The author/API-field explanation is valid; no simplification finding remains.
G21	FIXED	The userinfo test path is corrected.
G22	FIXED	PKCE encryption tests now verify the actual contract.
G23	FIXED	PKCE callback token exchange has regression coverage.
G24	FIXED	The migration inspector concern is resolved.
G25	DROPPED	The author/API response-shape explanation is correct.
G26	DROPPED	The standalone subprocess logging contract is valid.
G27	CONFIRMED [new]	Migration downgrade provenance/data-loss issue; reported below.
G28	CONFIRMED [new]	Structured-error length-cap issue; reported below.

Findings

Major

G08 [new] — Query/list/describe output is not bounded before serialization

Location: src/xagent/web/tools/mcp/salesforce.py:190 — major

This is the residual half of the prior output-bound concern; it intentionally excludes the separately tracked SOQL continuation/pagination and get_record omitted-fields work. salesforce_query, list_sobjects, and describe_sobject still pass full record, object, field, and picklist collections to _success before any connector-level bound. The generic filter can then append a truncation marker to the serialized text, producing oversized or invalid JSON for large/customizable orgs and breaking MCP/LLM consumers. Cap or project the data before serialization with a valid JSON envelope and explicit truncated state, preserving a safe continuation where the endpoint supports one.

G27 [new] — Seed downgrade can delete pre-existing operator rows

Location: src/xagent/migrations/versions/20260818_seed_salesforce_mcp_app.py:143 — major

upgrade() skips insertion when app_id='salesforce' or provider_name='salesforce' already exists, but downgrade() unconditionally deletes any public_mcp_apps row with that app ID. Its provider guard matches only the provider name, display name, and standard auth/token URLs while intentionally ignoring credentials; deleting the app first can also make the remaining-app guard pass, allowing both pre-existing operator rows to be removed. Track migration ownership/provenance, or conservatively preserve rows that predated the upgrade, and add same-ID, standard-endpoint, and combined-collision upgrade/downgrade tests.

Minor

G03 [prior] — Exact dot-segment identifiers still collapse URL paths

Location: src/xagent/web/tools/mcp/utils.py:35 — minor

The shared helper fixes the prior slash/query/encoded-traversal and empty/whitespace cases, but quote(value, safe="") leaves exact . and .. unchanged. Requests normalizes those URL dot-segments, so an identifier of .. can collapse the Salesforce API path, including for DELETE-capable callers. Reject exact ./.. before quoting and add regression and prepared-request tests.

G05-identity [prior] — Connected Salesforce grants still have no account label

Location: src/xagent/web/builtin_mcp_registry.py:236 — minor

The contradictory provider comment is fixed, but the identity behavior is not: userinfo_url remains empty, so callback skips the fixed USERINFO_URL, persists NULL email/provider_user_id, and /api/mcp/apps reports the app as connected without connected_account. Either populate identity from the fixed endpoint or a verified token response, or explicitly make the unlabeled state the tested and documented API/UI contract.

G09-M [prior] — Create failure test does not exercise the real failure contract

Location: tests/web/tools/test_salesforce_mcp.py:517 — minor

The prior errors-array propagation fix is present, but this test uses MockResponse's default HTTP 200 and asserts only errors. It therefore does not exercise the documented single-record create failure path (a non-2xx response with an error array) or prove the create-level status == "error" and message contract. Use a non-2xx mock at the salesforce_create_record boundary and assert the complete error envelope; keep any synthetic HTTP-200 success:false check as optional defensive coverage rather than the endpoint's primary contract.

G10 [prior] — Two identifier-validation call sites lack negative coverage

Location: tests/web/tools/test_salesforce_mcp.py:121 — minor

The production validation fix is present, but call-site tests still omit empty sobject_type for get_record and delete_record. Add both cases, assert an error, and assert that requests.request was not called, so either call site cannot regress while the shared-helper unit test remains green.

G19 [prior] — HubSpot was not migrated to the new shared helper

Location: src/xagent/web/tools/mcp/utils.py:7 — minor

The Salesforce copy was moved into mcp/utils.py, but HubSpot still carries byte-identical private helpers and call sites. That leaves two implementations that can diverge on future validation or encoding fixes. Finish the migration by importing the shared helpers in HubSpot, deleting its private definitions and direct quote import, and retaining provider integration assertions while moving direct helper assertions to shared utils.

G28 [new] — Structured Salesforce error details bypass the length cap

Location: src/xagent/web/tools/mcp/salesforce.py:109 — minor

_extract_error_detail joins every message in a structured error array without applying MAX_ERROR_RESPONSE_TEXT_CHARS, while _request_absolute caps only the raw-text fallback when extraction returns None. Long or numerous structured errors can therefore inflate logs and MCP/LLM-facing error JSON despite the stated bound. Apply the final cap and a clear truncation marker to structured details as well, and add regression coverage for long single- and multi-item arrays.

Blocking status & recommended decision

• [new] src/xagent/web/tools/mcp/salesforce.py:190 — major — query/list/describe results can exceed the connector boundary and become invalid JSON after generic truncation (G08).
• [new] src/xagent/migrations/versions/20260818_seed_salesforce_mcp_app.py:143 — major — downgrade can delete pre-existing colliding app/provider rows (G27).

Blocking: yes — recommended event: REQUEST_CHANGES

[yiboyasss]

Update since 38bcb49

Addressed this round's review (commit 2eac27d):

Major

• G08: salesforce_query/search/list_sobjects/describe_sobject built their JSON string with no size bound, so a large org's result could serialize past the generic output filter's threshold and get hard-truncated into invalid JSON. Added _success_with_capped_list() (mirrors hubspot.py's existing _paged_list/_success_with_capped_dict halving pattern) and applied it at all four call sites — salesforce_search has the identical unbounded-list shape and wasn't cited but was equally exposed, so it got the same fix.
• G27: the seed migration's downgrade() deleted the public_mcp_apps row by app_id unconditionally, without the shape guard its own oauth_providers deletion already has. A pre-existing operator row that happened to occupy app_id="salesforce" before the migration ever ran would be deleted anyway, which could then make the remaining-apps count wrongly read as zero and delete the provider row too. Added the same shape guard used for the provider row, plus a regression test.

Minor

• G03: url_path_id()'s percent-encoding doesn't touch a literal ./.. (always-unreserved characters), and requests/urllib3 normalize dot-segments out of the final URL before sending it — verified directly that a record_id of ".." collapses .../sobjects/Account/.. to .../sobjects/, a different endpoint. Now rejected explicitly in the shared mcp/utils.py helper.
• G19: finished migrating hubspot.py onto the shared mcp/utils.py helpers (deleted its byte-identical private copies) — as a side effect this also closes the G03 dot-segment gap there.
• G28: _extract_error_detail's structured (JSON array) error messages bypassed MAX_ERROR_RESPONSE_TEXT_CHARS; only the raw-text fallback was capped. Applied the cap to both.
• G09-M / G10: added the realistic non-2xx create-failure test and the remaining empty-sobject_type call-site tests for get_record/delete_record.
• G05-identity: took the "documented, tested contract" option rather than populating identity — added a test asserting connected_account is None at the actual get_mcp_servers() response (not just the UserOAuth row), mirroring Meta's equivalent test.

Not fixed, filed as follow-ups (both are pre-existing gaps in already-shipped, unrelated connectors, not introduced by this PR):

• jira.py's _path_segment and intercom.py's inline quote() calls have the same G03 dot-segment gap url_path_id had — jira.py/intercom.py path-segment helpers don't reject exact '.' or '..' identifiers #1558
• Other connectors' seed migrations likely share G27's downgrade() guard gap (confirmed present in 20260818_seed_jira_mcp_app.py) — Connector seed migrations' downgrade() lacks a provenance guard on the public_mcp_apps row #1559

All touched test suites (~280 tests across salesforce/hubspot/oauth/encryption/migrations) pass.

[rogercloud]

Follow-up Review — Salesforce connector (OAuth App + custom MCP tools)

What this PR does

Adds a Salesforce connector built on the existing static-client OAuth + custom FastMCP pattern already used by GitHub, Linear, Jira, Slack, Zoom and HubSpot: a seeded oauth_providers + public_mcp_apps row pair, plus src/xagent/web/tools/mcp/salesforce.py wrapping SOQL query, SOSL search, sobject listing/describe, and full record CRUD.

The genuinely new architectural piece is that Salesforce's token response carries a per-org API host (instance_url) that every subsequent API call must target — something no prior connector in this codebase needed, since all of them required only an access token. That drove a new user_oauth.instance_url column, persistence on connect and refresh, and threading the value through the OAuth resolver / launch-config plumbing shared by every connector. The PR also lands this codebase's first PKCE implementation, gated to the Salesforce provider.

Update since the last round

The last full round was the 28-item G01–G28 table. This pass re-verified every prior finding from scratch against the current code rather than trusting the prior round's claims. Result: the large majority of prior findings are solidly FIXED with real regression tests; 4 items remain open but are explicitly deferred to tracked GitHub issues; 4 author rebuttals were independently checked and hold up. This pass also found one place where a prior round's "FIXED (tested)" claim was inaccurate (the env_mapping else branch — see minor findings), and surfaced 6 findings no prior round raised, one of which is blocking.

Round 0 design verdict — acceptable with reservations

The design is correct on the paths it covers, unusually well tested for a connector of this size, and the shared-plumbing changes are additive and backward compatible. Two reservations:

1. Token lifetime gap. Because Salesforce's token response has no expires_in, expires_at stays NULL and the refresh path is never entered — which means the connector's own headline feature (re-persisting instance_url on refresh) is effectively unreachable in production. Deferred to issue #1540; noted here because it is a design-level, not cosmetic, consequence.

2. Generalization of instance_url (non-blocking observation). A reasonable concern is whether this should have been a generic per-provider slot rather than a Salesforce-shaped field baked into shared OAuth infra. Independent checking softens the specific critique: the proposed alternative — discovering instance_url at runtime via a userinfo "urls map", following the Jira/Intercom precedent — does not actually apply here, because Salesforce returns instance_url in the token-exchange response for free, and the alternative would add a round-trip the current design avoids. The Jira/Intercom comparison remains directionally valid (per-org routing solved one layer down without a schema change is an established pattern here), but it is an observation, not a finding requiring a change.

Prior findings — verified status

FIXED (verified against current code; no action needed)

• Hook-path OAuth resolver now threads instance_url through — ResolvedToken / _ResolvedHookToken gained the field and both call sites in src/xagent/web/tools/config.py pass it. Tested.
• env_mapping no longer silently drops the env var when instance_url is falsy — it raises _OAuthInstanceUrlRequired and surfaces a reconnect signal (src/xagent/web/tools/config.py:3075-3078). Both call sites tested.
• Path/query injection via unencoded path segments closed by shared url_path_id() / require_clean_identifier() in src/xagent/web/tools/mcp/utils.py, which explicitly reject . / .. dot-segments, applied at every CRUD call site. Tested.
• PKCE code_verifier is now Fernet-encrypted (not merely JWT-signed) before entering the OAuth state, and decrypted with decrypt_value_strict in the callback. Tested — and the previously tautological encryption regression test was fixed to actually assert encrypted != decrypted and _is_encrypted(...).
• The PKCE token-exchange path is no longer untested: a real test asserts the decrypted verifier is sent in the token-exchange POST body.
• Query / search / list_sobjects / describe_sobject output is bounded via a shared _success_with_capped_list() halving-loop helper applied at all four call sites. Tested.
• salesforce_create_record preserves the errors array on failure instead of discarding it, and the unrealistic "200-with-errors" mock was replaced by a genuine non-2xx failure test.
• Non-empty validation on record_id / sobject_type added via the shared identifier helper at every CRUD call site, including the previously missing get_record / delete_record empty-string cases.
• Dict-shaped error responses in _extract_error_detail (OIDC userinfo path) now have dedicated coverage.
• Misleading test example path for salesforce_get_current_user routing corrected; routing behavior is accurately tested.
• Structured (array-shaped) Salesforce error details now respect MAX_ERROR_RESPONSE_TEXT_CHARS, matching the raw-text fallback.
• Seed migration downgrade() now guards on the full expected row shape before deleting, mirroring upgrade()'s skip logic, so pre-existing operator rows are safe. Tested; the migration head graph independently resolves to exactly one head with no orphaned branches.
• Migration inspector calls moved off legacy Inspector.from_engine(bind) to sa.inspect(bind) at both call sites.
• The Salesforce identity / userinfo_url gap is now a deliberate, documented and tested contract: connected_account intentionally reports no label for Salesforce. One documentation-precision note for the record — the earlier round's comparison to "Meta's pattern" was imprecise (Meta does populate userinfo_url and reaches None through an unrelated policy-check path). Linear is the accurate analogue: it also has an empty userinfo_url and no eager identity lookup. No action needed.
• hubspot.py finished migrating onto the shared mcp/utils.py identifier helpers (via re-exported aliases to preserve existing test call sites — cosmetic indirection only).
• The redundant second sa.table() definition for oauth_providers in the seed migration was already dropped later in this same PR (3f3a35d58); the migration now matches the sibling Jira seed migration. Not re-raised.

PARTIAL (one minor open item)

• PKCE decrypt error path — test gap. Decryption was correctly hardened to decrypt_value_strict (raises on bad ciphertext rather than silently passing it through) and the missing-encryption-key branch is covered, but no test exercises the corrupted/tampered-ciphertext-with-key-present branch (InvalidToken → error path). src/xagent/web/api/auth.py (PKCE callback decrypt, ~L1600-1620); test file tests/web/test_salesforce_oauth.py. Code is correct; this is coverage only.

DROPPED — author rebuttals independently verified as correct

• test_non_salesforce_callback_does_not_persist_instance_url being tautological: verified the guard branch it originally targeted was genuinely removed (the row is always freshly recreated on each callback; no update/merge path remains), so there is nothing meaningful left to assert. Rebuttal holds.
• .codespellrc global createable ignore entry: createable is a real Salesforce API field name, and one of its three occurrences sits inside an LLM-facing tool docstring where an inline suppression cannot cleanly apply. Rebuttal holds.
• SOSL result.get("searchRecords") response shape: code and tests are internally consistent with a dict-shaped response; this is external API contract not verifiable from the codebase, and nothing here contradicts it. Dropped.
• logging.basicConfig() in salesforce.py: the module is only ever a standalone subprocess entrypoint and is never imported into the main app; five spot-checked sibling connectors use the identical pattern. Rebuttal holds.

Still open, but deferred to tracked issues (not blocking, not re-raised)

• Tokens are never refreshed — no expires_in means expires_at stays NULL, and refresh_oauth_token_if_needed treats NULL as "assume valid", so the refresh path (including instance_url-on-refresh) is unreachable in production; the connector will silently 401 after the org's session timeout with no reconnect signal. src/xagent/web/api/auth.py:1859-1866, src/xagent/web/tools/config.py:582-585,709-714. → #1540
• No SOQL pagination beyond 2000 rows (nextRecordsUrl dropped, only a truncated flag surfaced) and no field cap on get_record. src/xagent/web/tools/mcp/salesforce.py (salesforce_query, ~L216-227). → #1541
• No sandbox-org support / no PKCE capability modeling — login.salesforce.com is hardcoded with no per-user toggle, and PKCE gating uses provider.lower().startswith("salesforce") string matching rather than a capability column. Note: the code did change from == to .startswith since an earlier round, but the structural fix never landed. Real-world risk is low today because provider rows come from a fixed developer-controlled seed set. src/xagent/web/builtin_mcp_registry.py:222-223, src/xagent/web/api/auth.py:1320-1322. → #1542
• No 429 / Retry-After rate-limit handling, unlike sibling connectors jira.py, slack.py and intercom.py which all implement it. src/xagent/web/tools/mcp/salesforce.py. → #1543

New findings this pass

MAJOR — unguarded destructive delete tool, broader blast radius than any peer connector, no explicit product sign-off

salesforce_create_record / salesforce_update_record / salesforce_delete_record (src/xagent/web/tools/mcp/salesforce.py:346-421) accept any sobject_type string with only URL-safety validation via url_path_id — no business-level allowlist, no dry run, no confirmation step.

Verified repo-wide: the HTTP DELETE method appears in only three connector modules — onedrive.py (a personal file), outlook.py (a personal calendar event), and salesforce.py (an arbitrary standard or custom business record in a shared org). Salesforce is the only connector able to delete shared business-of-record data. HubSpot, the other CRM connector in this codebase, deliberately exposes zero delete-capable tools (its tool surface is search / get / create / update / notes / analytics only).

There is also no generic mitigation one layer up: no confirmation, allowlist, or HITL gating layer exists anywhere in the codebase. The closest thing found is a docstring on an unrelated Gmail-send tool claiming it "triggers an interactive user confirmation" — that claim has no backing implementation in this repo.

Recommendation: make this an explicit author/product decision rather than something that ships silently. Either scope salesforce_delete_record (and arguably create/update) behind an explicit sobject-type allowlist or a confirmation gate, or drop the delete tool the way HubSpot does.

MINOR — instance_url validation has real, if narrow, gaps

1. Persist time, no validation at all. src/xagent/web/api/auth.py:1859 and src/xagent/web/tools/config.py:709-714 write the token-response-supplied instance_url straight to the DB with no type or non-empty check — in contrast to the hook-path resolver at src/xagent/web/tools/config.py:2929-2932, which does check isinstance(..., str) and non-empty.
2. Use time, incomplete parsing. _instance_url() in src/xagent/web/tools/mcp/salesforce.py (~L75-104) checks scheme == "https" and a hostname suffix of salesforce.com / force.com, but never checks port, path, query, or userinfo. Since the validated value is used as a raw string prefix (f"{_instance_url()}{path}"), values like https://acme.my.salesforce.com/evil/path or https://user:pw@acme.my.salesforce.com pass validation and their extra components flow silently into every outbound request URL.
3. force.com in the allowlist has no documented justification narrower than "known Salesforce domain". It is considerably broader than the *.my.salesforce.com API-instance pattern and covers Salesforce Sites / Experience Cloud pages that can serve customer-authored content.

The scheme + host check does block the primary SSRF vector (redirecting the Bearer token to a wholly different origin). What is missing is finer hardening: canonicalize to scheme://hostname before use, reject path/query/userinfo, check port, tighten or document the force.com inclusion, and add to the DB-persist path the same type/non-empty check the hook path already performs. Severity minor: defense in depth, not immediately exploitable given the value originates from Salesforce's own TLS-protected token endpoint.

MINOR — an env_mapping branch was reported "FIXED with a test" in a prior round but is untested and effectively dead

src/xagent/web/tools/config.py:3079-3092 logs a warning for an unrecognized env_mapping.token_type. Verified: (1) no test anywhere exercises this branch — the test cited by the prior round (test_legacy_missing_instance_url_retains_unavailable_server) covers a different, pre-existing branch (the instance_url-required exception), as its own docstring states; (2) token_type is strictly internal — every env_mapping entry in builtin_mcp_registry.py is a fixed string literal ("access_token" or "instance_url"), and no external input, user, or admin action can produce a third value. The branch is reachable only via a typo in this repo's own source.

Recommendation: either delete the branch, or add a real test if it is being kept as defensive tooling. What should not persist is a branch a past review recorded as test-covered when it is not. No functional impact — the behavior, if ever reached, is a harmless warning log.

MINOR — example.env sandbox-workaround note is factually wrong and incomplete

The note at example.env:657-666 claims that editing an oauth_providers row's auth_url / token_url to point at test.salesforce.com is "harmless" with respect to salesforce.py's USERINFO_URL, on the grounds that the oauth_providers.userinfo_url column is unused. This conflates two different things: the DB column is indeed unused, but the module-level constant USERINFO_URL (hardcoded to the production login host) is used unconditionally by salesforce_get_current_user on every call via _request_absolute. Following the documented workaround would send a sandbox-issued token to the production userinfo endpoint, which fails — the opposite of "harmless".

The note also omits that a working sandbox setup needs a second public_mcp_apps row with its own app_id / launch_config / env_mapping, not just a second oauth_providers row; public_mcp_apps is never mentioned anywhere in example.env. This was raised previously and is still unfixed.

MINOR — instance_url parameter design in _build_oauth_mcp_stdio_transport_config

At src/xagent/web/tools/config.py:3053-3060, instance_url is an optional keyword parameter defaulting to None, yet both current call sites (hook path and legacy DB path) always pass it explicitly. The default buys nothing today and lets a future call site silently omit it, surfacing as a confusing user-facing "reconnect required" error instead of a call-time failure. Making it required keyword-only is safe given current callers.

Related: the two except _OAuthInstanceUrlRequired handlers (~L3330-3341 and ~L3388-3399) are duplicated boilerplate apart from their log line; a small extracted helper (e.g. _oauth_token_required_response(server, log_msg)) would remove the duplication without merging the two otherwise-distinct call paths.

MINOR (cosmetic) — two small nits in salesforce.py

• The comment at ~L26-30 states Salesforce "keeps every past API version working indefinitely". That is an absolute claim; Salesforce has historically retired old REST API versions. A hedged phrasing ("supported for many years") would be more accurate. No functional impact.
• _extract_error_detail's array-error fallback str(item.get("message") or item) (~L133) emits a Python dict-repr string (e.g. {'message': '', 'errorCode': 'X'}) when an error element has an empty-string message. Cosmetic, bounded by the existing length cap, low real-world likelihood.

Simplification opportunities

src/xagent/web/api/auth.py:L1321: yagni: PKCE gate matches provider names via string prefix (`.startswith("salesforce")`) instead of a capability flag on the provider row; no second provider exists yet to justify pattern-matching at all. Add a `requires_pkce` column (or gate on `auth_url` host) instead — tracked in issue #1542, no code change landed yet.
src/xagent/web/tools/config.py:L3079-3092: delete: unrecognized-`env_mapping`-token_type warning branch guards a typo only this codebase's own developer-authored registry could produce; no test exercises it and no third token_type value is ever registered anywhere. Delete the branch (or assert at registry-load time) instead of carrying it as untested dead code.
net: ~14 lines possible

Blocking status & recommended decision

Blocking: yes — the unguarded destructive delete tool is the sole major finding of this pass; everything else is minor, deferred to a tracked issue, or already fixed.

Recommended event: REQUEST_CHANGES

Blocking issues:

• src/xagent/web/tools/mcp/salesforce.py:404-421 — major — unguarded destructive delete tool with no allowlist/confirmation and broader blast radius than any peer connector — [new]

[yiboyasss]

Update since 3644bf7

Addressed the follow-up review (commits 3644bf7, fbad311).

Blocking (product decision)

• The unguarded destructive salesforce_delete_record tool has been removed entirely, matching HubSpot's approach of shipping no delete-capable tools for this CRM connector. Its tests were removed too.

Minor

• _instance_url() now canonicalizes to exactly scheme://host[:port] instead of returning the input string as-is — it's used as a raw prefix for every outbound request URL, so a value like .../salesforce.com/evil/path or a userinfo-embedded value previously passed the scheme+host check and then rode along into every request unchanged. Dropped force.com from the allowed host suffixes (Salesforce Sites/Experience Cloud, which can serve customer-authored content, and not a host the OAuth token endpoint's instance_url ever actually returns).
• Added the missing regression test for the env_mapping dispatch's unrecognized-token_type warning branch, and corrected the comment claiming it's developer-only: POST /admin/mcp/apps accepts launch_config as an unvalidated free-form dict, so an admin's hand-typed custom OAuth app can reach it too.
• Added the missing PKCE-decrypt regression test for corrupted/foreign-key ciphertext (EncryptionDecodeError), distinct from the already-tested missing-key case (bare ValueError).
• Corrected example.env's sandbox-workaround note: it claimed switching to test.salesforce.com was harmless with respect to salesforce.py's USERINFO_URL, conflating the unused oauth_providers.userinfo_url DB column with the separate hardcoded USERINFO_URL constant salesforce_get_current_user() calls unconditionally — a sandbox-issued token sent there fails outright. Also noted the second public_mcp_apps row a working sandbox setup needs.
• _extract_error_detail now falls back to errorCode before a Python dict-repr when a structured error item's message is empty.
• Hedged a comment overclaiming Salesforce keeps every API version working "indefinitely".

Self-review catch, not from the review round

• The _instance_url() canonicalization above introduced a real bug: urlparse().port is a lazy property that raises a bare ValueError for a non-numeric port, and that access wasn't guarded — a malformed port (e.g. ...salesforce.com:abc) crashed with urllib's own cryptic message instead of this function's intended "not a valid Salesforce host" error. Fixed, with a regression test. Also flattened an over-nested helper in _extract_error_detail and added a trailing-dot-hostname test the canonicalization change needed but didn't get.

Not fixed, no new issues filed this round (both already covered by open issues, or judged out of scope with no practical impact):

• No persist-time validation on instance_url in auth.py/config.py — the authoritative check already happens at use-time in _instance_url() before any outbound request; adding a duplicate check to the generic, connector-agnostic OAuth code would mean importing Salesforce-specific validation into shared infrastructure.
• Making instance_url a required keyword-only parameter in _build_oauth_mcp_stdio_transport_config, and extracting the two except _OAuthInstanceUrlRequired handlers into a shared helper — both are pure robustness/style improvements with no live bug behind them, at the cost of touching several unrelated existing test call sites.
• Explicit default port (:443) not stripped by the new canonicalization, and its hand-rolled logic not consolidated with similar helpers in unrelated subsystems (a2a_agent_tool.py, mcp_oauth.py) — both zero practical impact / out of this connector's scope.

All touched test suites (~320 tests across salesforce/hubspot/oauth/encryption/migrations) pass.

[rogercloud]

This PR adds a Salesforce OAuth/MCP connector on top of the existing provider, app, UserOAuth, and stdio-launch abstractions. It introduces a nullable per-grant instance_url, threads it through callback/refresh and legacy or hook-based resolution, and exposes eight Salesforce tools for identity, query/search, schema browsing, and record read/create/update operations. The current head intentionally omits destructive delete, but the PR description and test plan still describe full CRUD and nine tools.

Approach verdict

Acceptable with reservations (independent Round 0 verdict). The per-grant instance_url sidecar is a coherent extension of the existing OAuth/MCP architecture: it follows the grant through persistence, refresh, resolver paths, launch environment mapping, and the final Salesforce origin check without creating a second authorization system. The material reservations are seed credential ownership and rotation, migration provenance, callback/refresh persistence invariants, concurrent NULL-identity grants, and irrecoverable output truncation. This is an independent design judgment, not a conclusion derived from whether earlier review comments were claimed to be fixed.

Findings

Major — new — Seeded Salesforce credentials are plaintext and freeze environment rotation

Location: src/xagent/migrations/versions/20260818_seed_salesforce_mcp_app.py:63-67; the duplicate registry seed is at src/xagent/web/builtin_mcp_registry.py:212-224.

Both seed paths copy SALESFORCE_CLIENT_ID, SALESFORCE_CLIENT_SECRET, and SALESFORCE_REDIRECT_URI directly into oauth_providers. The migration/registry insertion does not use the encryption path used by admin-created providers, so the client secret can be plaintext at rest. More importantly, the OAuth resolvers prefer any non-empty stored value over the environment, so rotating the Connected App values in SALESFORCE_* leaves the old snapshot authoritative and silently makes the new deployment configuration ineffective.

Specific suggestion: Keep built-in credential and redirect fields empty so the existing environment fallback remains live, while retaining encrypted values only for explicitly admin-managed rows; or add an explicit encrypted built-in ownership/reconciliation and rotation policy. Add tests for both seed paths and an A-to-B environment rotation. Encrypting only the current insert is insufficient because DB-first precedence would still block rotation.

Major — prior, not fixed — Salesforce seed downgrade can delete operator-owned rows

Location: src/xagent/migrations/versions/20260818_seed_salesforce_mcp_app.py:108-130,153-186.

Upgrade skips an existing provider_name='salesforce' or app_id='salesforce', but downgrade deletes an app using only app_id/name/transport/provider_name and a provider using only provider_name/name/auth_url/token_url. An operator-owned row with those canonical stable values but custom scopes, description, visibility, launch configuration, credentials, redirect URI, or other fields is therefore skipped on upgrade and deleted on downgrade; deleting the app can then make the provider deletion eligible too.

History and re-check: This is the same G27 root raised at discussion_r3821738710 and recorded as major in review 4982943190. A later follow-up review 4989514308 marked G27 fixed, and the author said in issuecomment-5358522780 that the same-shape guard addressed it while pointing to #1559. I re-checked the current predicates: that previous FIXED claim was incomplete because the guard is only a partial shape check and does not establish migration ownership. #1559 concerns other connector migrations and does not make this Salesforce downgrade safe.

Specific suggestion: Record durable migration ownership (for example, inserted row IDs or an ownership ledger) and delete only rows proven to belong to this revision. Otherwise make downgrade conservative and preserve ambiguous rows. Add a fixture with pre-existing canonical app/provider identifiers and endpoints but custom scopes, launch configuration, description, credentials, and redirect URI, then assert that the complete rows survive upgrade and downgrade.

Major — new — OAuth callback can report success after replacing a grant with no usable instance_url

Location: src/xagent/web/api/auth.py:1821-1859 (anchor :1859); the transaction commits at :1947 and returns the success page at :1968-1988.

The callback validates only that an access token exists. It deletes the previous UserOAuth row before assigning token_data.get("instance_url"), creates the MCP association, commits, and returns Connected Successfully even when the Salesforce response omits instance_url or supplies an empty value. The later launch builder rejects the missing mapped value, but only after the old usable grant has been destroyed and the user has been told the connection succeeded.

Specific suggestion: Before deleting or replacing the prior grant, derive required fields from the target app's launch_config.env_mapping and reject a missing, empty, or malformed instance_url. Preserve the prior grant on that error, keep the launch-time check as defense in depth, and add missing/empty callback tests that assert no destructive mutation and an error response.

Major — new — Empty Salesforce userinfo configuration permits concurrent NULL-identity grants

Location: src/xagent/web/builtin_mcp_registry.py:236; the delete/reinsert sequence is in src/xagent/web/api/auth.py:1820-1829, and the uniqueness definition is in src/xagent/web/models/user_oauth.py:12-35.

The intentional userinfo_url="" leaves provider_user_id NULL for every Salesforce grant. The existing composite unique constraint does not enforce uniqueness for repeated NULL values, and callback uses an unlocked delete-then-insert sequence. Concurrent PostgreSQL callbacks can therefore leave multiple Salesforce rows for one user/provider; .first() and unordered listing consumers can select an arbitrary token, org host, or account state.

Specific suggestion: If the contract is one Salesforce grant per user, enforce a provider-level unique key and use an atomic upsert/update or row/advisory lock with conflict handling. If multiple org accounts are intended, persist a verified Salesforce account identity and make uniqueness and every selector account-aware. Add a concurrent-connect regression test that asserts the chosen invariant and deterministic resolution.

Major — new — Local output capping returns irrecoverable prefixes

Location: src/xagent/web/tools/mcp/salesforce.py:42-65 (anchor :61); call sites are :250-345.

_success_with_capped_list() repeatedly halves the list until the JSON string fits, then returns only the prefix with truncated=true. Query results (including a page where Salesforce reports done=true), SOSL results, sObject listings, and describe fields have no offset, cursor, projection, or continuation contract to recover the omitted tail. This is distinct from the fixed prior invalid-JSON/oversize root and from tracked #1541's upstream nextRecordsUrl/get-record scope: valid JSON is now produced, but valid data can still be silently and permanently lost.

Specific suggestion: Replace the lossy generic halving contract with endpoint-specific recoverability: stable limit plus opaque cursor/offset/filter for search, listing, and describe; a local page cursor in addition to any upstream query continuation; or an explicit bounded projection/rejection when safe continuation is impossible. Return has_more and a usable continuation token, and test that a follow-up call retrieves omitted records, objects, fields, or hits without overlap or loss.

Minor — prior, confirmed — Refresh and initial persistence still accept malformed instance_url metadata

Location: src/xagent/web/tools/config.py:709-714; the same persistence invariant is present on the newly recreated callback row at src/xagent/web/api/auth.py:1859.

On a 200 refresh response containing an instance_url key, the refresh path assigns any value, flushes it, logs success, and returns True. An empty, non-string, or malformed value can replace a valid stored host; the later _instance_url() check only fails when the connector is used, after refresh has committed. The callback occurrence is the fresh-row/state-corruption facet of this same persistence root; the callback's broader old-grant-loss and false-success ordering is reported separately above.

History and re-check: This is the prior persistence finding at discussion_r3827114691, also recorded in review 4989514308. The author explicitly declined persist-time validation in issuecomment-5365510264, arguing that use-time _instance_url() validation is authoritative. I re-checked that claim: it mitigates outbound origin misuse, but it cannot preserve a valid host after refresh mutation/commit or prevent an initial callback from committing unusable state. The hardened use-time boundary is why this remains minor, and it is not the separately tracked #1540 refresh-recovery root.

Specific suggestion: Validate type, non-empty value, and the connector-appropriate origin before either writer mutates persistence. On refresh, preserve the old valid host and return a controlled failure/reconnect outcome when the response is malformed; add initial-callback and refresh tests for empty, null, non-string, malformed, and invalid-origin values.

Minor — new — Output-cap tests allow a real limit violation

Location: tests/web/tools/test_salesforce_mcp.py:468; the same assertion is at :506, :555, and :624.

Each test parses the production string, reserializes the resulting object with different formatting, and allows 2000 + 200 characters. The actual production helper enforces an exact raw limit, and the concrete two-item fixtures produce raw envelopes of 2121, 2103, 2166, and 2199 characters, so these assertions can pass while the output filter still hard-truncates the response into invalid JSON.

Specific suggestion: Capture the raw return value before json.loads, assert len(raw) <= 2000 (or the configured limit) exactly, then parse it. Apply the same pattern to all four cap tests and retain assertions for valid JSON and the expected truncation signal.

Minor — new — Alembic merge annotations describe a tuple as scalar-only

Location: src/xagent/migrations/versions/0b38b8d46e1c_merge_salesforce_and_jira_mcp_app_.py:13-16 and src/xagent/migrations/versions/c97b6332a895_merge_salesforce_and_linear_mcp_app_.py:13-16.

Both new merge revisions assign a two-parent tuple to down_revision: Union[str, None]. Alembic's runtime graph is valid, but the declared static contract is false and differs from neighboring merge revisions that include Sequence[str].

Specific suggestion: Use Union[str, Sequence[str], None] in both new merge files (and update the migration template separately so future generated merge revisions use the same accurate annotation).

Minor — new, documentation-only — PR scope and test-plan text is stale

Location: PR description:2 and PR description:13. This is not a code bug and has no inline anchor because PR metadata is not part of the diff.

Current HEAD exposes eight tools and intentionally removed salesforce_delete_record, but the summary still says “full record CRUD” and the test plan still says “all 9 Salesforce tools.” The removed delete implementation is not being re-reported; the actionable issue is only that the public scope and coverage claim are stale.

Specific suggestion: Update the PR summary and test plan to say that eight non-destructive tools expose query/search/schema plus read/create/update, with delete intentionally omitted. Synchronize the runtime catalog and frozen seed descriptions if they are intended to be user-facing.

Criteria, no-issue checks, and limitations

• Correctness, security, edge cases, and performance are covered by the five major findings and the persistence minor; in particular, the origin allowlist, path encoding, encrypted PKCE state, bounded HTTP timeouts, and token redaction were checked without identifying an additional issue.
• Maintainability is covered by the seed ownership/rotation findings and the false merge annotations. The existing sidecar threading and shared resolver/launch abstractions otherwise match repository conventions.
• Test quality is covered by the raw-limit assertion gap and the missing regression cases called out in the major findings. No additional resource-lifecycle or injection issue was confirmed.
• This consolidation was static: no local tests, build, lint, formatter, typecheck, or Alembic commands were run. Preflight CI was reported as 14 completed SUCCESS checks with no failures or pending checks.
• The Simplification Lens was unavailable because the review-spark worker exhausted its usage limit; no Simplification opportunities section or invented simplification finding is included.
• Tracked #1540 (refresh recovery), #1541 (upstream continuation/get-record field cap), #1542 (sandbox/PKCE capability), and #1543 (429 handling) are not re-reported. Fixed or dropped roots, including the prior invalid-JSON root and the removed destructive-delete security root, are likewise not re-reported. No qinxuye rebuttal exception applies to this author.

Blocking status & recommended decision

Blocking: yes
Recommended event: REQUEST_CHANGES

Blocking issues:

• src/xagent/migrations/versions/20260818_seed_salesforce_mcp_app.py:63-67 — major — plaintext built-in credentials and stale DB-first rotation behavior [new]
• src/xagent/migrations/versions/20260818_seed_salesforce_mcp_app.py:153-186 — major — downgrade can delete upgrade-skipped operator rows [prior]
• src/xagent/web/api/auth.py:1859 — major — callback commits unusable state and reports success after deleting the prior grant [new]
• src/xagent/web/builtin_mcp_registry.py:236 — major — concurrent NULL-identity callbacks can leave arbitrary token/org selection [new]
• src/xagent/web/tools/mcp/salesforce.py:61 — major — local capping permanently drops valid result data without recovery [new]`

[yiboyasss]

Ran a self-review pass over the last round's diff (fbad311c..8ccae51d) before it could pick up any new review comments, specifically checking for regressions and half-fixes. Found and fixed two additional issues, both pushed in 1302764:

1. downgrade()'s new shape-guard compared JSON columns via SQL equality, which breaks on PostgreSQL. The strengthened guard added last round (matching oauth_scopes/launch_config/default_scopes before deleting a seeded row) used .where(json_column == value). PostgreSQL's plain json column type (which is what these columns actually are — no JSONB variant here) has no = operator, so alembic downgrade past this revision would raise UndefinedFunction on any Postgres-backed deployment, even for the common no-admin-edits case. The existing tests only ran against SQLite, where this is invisible. Rewrote downgrade() to fetch the candidate row via a plain string-keyed SELECT and compare all fields (JSON included) in Python, so no JSON equality ever reaches SQL. Also flagged this pitfall on Connector seed migrations' downgrade() lacks a provenance guard on the public_mcp_apps row #1559, since roughly a dozen other connectors' seed migrations share the same underlying downgrade-guard weakness and any future fix there needs to avoid the same trap.

2. _success_with_capped_list's halving loop could still exceed the configured output limit. Once halving empties items, the loop exits regardless of the response's actual length — if the fixed "cannot be recovered" message text alone pushes the response over an unusually small XAGENT_TOOL_MAX_OUTPUT_LENGTH, the function would return a payload that violates its own size contract. Now drops the message as a last resort in that case. Also simplified the loop from two nested phases into one, and added a direct unit test for the empty-items-plus-oversized-message edge case.

Also corrected a stale comment in the migration that overstated how the seeded row's oauth_scopes/launch_config could diverge via the admin API — admin_mcp.py's _BUILTIN_PROTECTED_FIELDS actually blocks PATCHing those two fields away from the built-in registry's values while the app stays registered as built-in; the real divergence paths are a raw DB edit or the app_id later being dropped from the registry.

Full regression suite (Salesforce tool tests, migration tests, OAuth/token-resolver tests, encryption tests) re-run clean after these fixes.

[rogercloud]

PR summary

This PR adds a Salesforce connector built on the repo's existing static-OAuth + custom-FastMCP pattern: seeded oauth_providers / public_mcp_apps rows (duplicated in src/xagent/web/builtin_mcp_registry.py and an alembic seed migration) plus src/xagent/web/tools/mcp/salesforce.py, which exposes 8 non-destructive tools over the Salesforce REST API (SOQL query, SOSL search, sobject list/describe, record get/create/update — no delete). Because Salesforce serves each org from its own API host, it also adds a nullable user_oauth.instance_url column and threads it from the token exchange and refresh through both token-resolution paths into a second launch_config.env_mapping entry. Additionally it introduces PKCE for this flow (gated on the Salesforce provider name, verifier carried encrypted inside the signed JWT state), hoists two id-validation helpers out of hubspot.py into a shared src/xagent/web/tools/mcp/utils.py, and adds three empty alembic merge revisions to reconcile heads with concurrent connector PRs. Roughly 3,374 insertions / 35 deletions across 22 semantic files.

Blocking: yes — recommended event: REQUEST_CHANGES

Update summary (fbad311c -> c1a80315)

Three author commits plus a main merge. 8ccae51d added the pre-mutation instance_url guard in the OAuth callback and widened the seed migration's downgrade guard from a 3-4 column structural check to a full-row shape comparison. 1302764c moved that shape comparison out of SQL into Python (Postgres json has no = operator), fixed the output-capping halving loop so it can no longer exceed the limit when the message alone overflows, and replaced the four output-cap tests' reserialized-plus-slack assertions with exact raw-length assertions. c20986e1 / c1a80315 merged main and added a third merge revision ae0d1cffeca6. This round genuinely closed 8 prior findings (P15, P17, P18, P27, P33, P34, P42, P44).

Approach verdict

Acceptable with reservations. The instance_url column and the Salesforce module are well-judged and follow the established connector pattern rather than inventing a parallel one, and the test shape matches the actual risk surface: migration idempotency, downgrade-preserves-admin-edits, registry/seed parity, PKCE round-trip, the missing-ENCRYPTION_KEY path, malformed-refresh-preserves-prior-value, and an explicit non-Salesforce non-regression test.

The design-level reservation: the provider-identity predicate is not consistent across the flow (B1 below) — PKCE is granted by prefix while both callback protections are granted by exact equality, so a configuration the repo documents as supported gets the capability without the safeguards. Secondarily, PKCE capability is still name-gated rather than modeled as provider data; that is already tracked in #1542 and is not asked for here.

Blocking findings

B1 (MAJOR) — Inconsistent Salesforce provider matching splits capability from safeguards

src/xagent/web/api/auth.py:1491 gates PKCE on provider.lower().startswith("salesforce"), with a comment stating the prefix is deliberate so a documented salesforce-sandbox row also gets PKCE. Both callback-side branches, however, use exact equality: :2031 (the instance_url presence/type guard) and :2095 (the provider_user_id = token_data.get("id") backfill). example.env:690-701 documents the salesforce-sandbox second-provider-row workaround as the supported sandbox path, so this configuration is reachable by design, not hypothetical.

For such a row, three things follow:

1. The :2031 guard does not fire, so a token response lacking instance_url reaches the unconditional delete-then-recreate at :2191-2201 (gated only on if user_id:, always truthy there) — destroying a previously working grant while the user is told "Connected Successfully". This is prior finding P40, which is therefore PARTIAL: fixed and tested for "salesforce", still reachable for the documented sandbox row.
2. The :2095 backfill does not fire; userinfo_url is empty for Salesforce-family rows so the generic elif userinfo_url and access_token fallback does not fire either, and provider_user_id stays NULL — forfeiting the (user_id, provider, provider_user_id) unique-constraint protection (src/xagent/web/models/user_oauth.py:12-15) against concurrent duplicate grants. This is prior finding P41, also PARTIAL: the general case is genuinely fixed and tested via the token_data["id"] backfill; only the sandbox row remains exposed.
3. PKCE is granted to precisely the row that receives neither of the other two protections.

To scope this fairly: the unlocked delete-then-insert and the NULL-tolerant composite constraint are pre-existing and generic to all providers (verified identical at base). The ask is not to redesign them — only to make the provider predicate consistent:

def _is_salesforce_provider(provider: str) -> bool:
    return provider.lower().startswith("salesforce")

used at all three sites, plus a test that exercises a prefixed provider name.

B2 (MAJOR, prior finding P29 — not fixed) — Irrecoverable truncation in list_sobjects / describe_sobject

src/xagent/web/tools/mcp/salesforce.py:42-101 _success_with_capped_list halves the list until the JSON fits and returns only the prefix with truncated: true. 1302764c fixed the loop's own limit-violation edge case and added a candid docstring, but added no recoverability contract.

Scoping this precisely: salesforce_list_sobjects and salesforce_describe_sobject expose no filter or names_only parameter, so a truncated answer to "what objects exist" / "what fields does this object have" is silently incomplete with no way for the model to iterate toward the missing remainder. That is the blocking part. salesforce_query and salesforce_search are different: the model can narrow them with LIMIT / WHERE / more selective SOSL, and their remaining gap is genuine nextRecordsUrl pagination, correctly carved out to #1541 — not re-litigated here.

Fix: give the two schema-listing tools a field/name filter (or a names_only mode) so a bounded-but-complete answer is reachable, or an explicit continuation contract.

Non-blocking findings (minor)

Correctness / robustness

1. src/xagent/migrations/script.py.mako:16 — prior finding P43 PARTIAL. The Union[str, Sequence[str], None] annotation was correctly applied to all three merge revisions this PR adds, but the template still emits scalar-only Union[str, None], so future generated merges reproduce the inaccuracy. (Not inline-commentable: the file is not part of this diff.)
2. src/xagent/web/tools/mcp/salesforce.py salesforce_get_record — the only tool returning an unbounded shape with no cap. hubspot.py:186 already has _success_with_capped_dict for the dict case, and hoisting it into tools/mcp/utils.py fits the module this PR created. Reachable when fields="" (documented as "return every field") meets Long Text Area content against the tool output limit; the platform filter treats the whole JSON blob as one opaque string leaf and cuts mid-string. Triaged Minor and deferred in an earlier round — recorded here, still minor.
3. src/xagent/web/api/auth.py:2030 / :2230 — salesforce_instance_url = token_data.get("instance_url") is assigned unconditionally in the generic path under a provider-prefixed name, and :2230 persists token_data.get("instance_url") for any provider with no type check (the :2031 guard covers Salesforce only). No currently seeded provider returns the key and there is no runtime provider-registration surface, so this is hardening plus naming, not a live bug.
4. src/xagent/web/tools/mcp/salesforce.py:144-152 — port carries two meanings ("" = no port, None = malformed). Correct as written; raising directly in the except would keep the variable single-purpose. Separately, urlparse(...).port returns 0 for :0 (in range, falsy), so :0 is silently canonicalized away rather than rejected — practical impact nil given the host is already https + *.salesforce.com allowlisted and operator-set.
5. src/xagent/web/tools/config.py:3260 — getattr(oauth_account, "instance_url", None) on a column the model declares unconditionally. No Mock / SimpleNamespace reaches this path and a missing physical column would fail at query time, so the fallback is unreachable; use direct attribute access.
6. src/xagent/web/tools/mcp/salesforce.py:177-202 _extract_error_detail — return "; ".join(messages) if messages else None: the else None arm is dead, since the earlier non-empty-list guard makes messages always non-empty. Nit.

Duplication / polish

7. salesforce.py _success_with_capped_list and hubspot.py:119-183 _paged_list share the same halve-while-too-long core, differing only in cursor vs. message bookkeeping. Both files are touched by this PR, which already hoisted two smaller helpers into utils.py, so the omission is an internal inconsistency. _success_with_capped_dict and google_analytics.py's inline loop are legitimately out of scope.
8. src/xagent/web/tools/config.py:3103-3120 — the token_type if/elif/else could collapse to a values.get(token_type) lookup inside the transport layer. Explicitly not recommending an extra: Mapping[str, str] on ResolvedToken: that is the public hook contract, where named fields give embedders mypy coverage. Optional polish only.

Comments / documentation

9. src/xagent/web/api/auth.py comments (and the PR discussion) describe this as "this codebase's first PKCE implementation". src/xagent/web/models/mcp_oauth.py:237 already has a code_verifier column and src/xagent/web/api/mcp.py already performs full S256 PKCE for the MCP OAuth flow. Different flow, but the claim as written is wrong and will mislead the next reader.
10. example.env:689-700 — the sandbox note is now factually correct (P17 fixed), but roughly 12 of its lines narrate implementation internals ("USERINFO_URL is a module-level constant", "no way to make that one tool sandbox-aware without a code change") rather than operator-actionable guidance. Optional trim. For the record, it is not an outlier in size: 30 lines vs GitHub's 32 and Slack's 26 — the upper end of an existing range.
11. PR description — prior finding P44 is FIXED (the body now says 8 tools / non-destructive; the registry and seed descriptions were never drifted). Still unmentioned: PKCE, and the hubspot.py helper hoist that tightened HubSpot's id validation to reject . / ... Documentation-only.

Tests

12. The guard not isinstance(x, str) or not x at auth.py:2031 has tests for missing and non-string values (both exercising the isinstance clause) but none for an empty-string instance_url — the second clause is untested.
13. tests/web/test_salesforce_oauth.py:242 test_connected_salesforce_server_reports_no_account_label asserts only connected_account is None. MCPServerResponse has no connected flag, so the docstring's "still show the server as connected" half is unasserted. Weak rather than risky — the sibling test_callback_persists_instance_url_and_skips_userinfo_lookup already covers persistence. Optional tightening.

Migration hygiene

14. Three chained empty merge revisions (0b38b8d46e1c -> c97b6332a895 -> ae0d1cffeca6) where a single one at the tip would be equivalent; c97b6332a895's second parent already has 20260818_seed_jira_mcp_app as an ancestor. The graph itself is healthy — exactly one head (ae0d1cffeca6), no dangling parents, per alembic's own history/heads. Worth collapsing before merge. Note this is purely tidiness: revision ids are author-chosen slugs, not commit SHAs, so a squash would not silently break the cross-PR references.

Pre-existing / out of scope — context, explicitly not blocking

• P39 (not fixed, pre-existing). Seeded OAuth credentials are written from os.environ.get(...) in plaintext with no encrypt_value, and _resolve_oauth_secret (src/xagent/web/api/auth.py:141-150) prefers a non-empty stored value over the environment — so rotating SALESFORCE_* env vars has no effect once seeded. Verified identical for GitHub, Jira, Linear, Slack, Zoom and HubSpot, in both their registry entries and seed migrations, all predating this PR. Worth a separate issue against the shared seeding pattern; not a reason to block here.
• Log injection. require_clean_identifier rejects only surrounding whitespace, so an embedded newline in an id reaches logger.error raw (the logged value is the un-encoded parameter, not the percent-encoded one). Same pattern in jira.py and posthog.py; google_analytics.py:377 already solves it with !r plus an explanatory comment. Repo-wide follow-up.
• Sibling dot-segment gaps. jira.py is tracked in #1558, but posthog.py, zoom.py, facebook.py and instagram.py have the same unguarded _path_segment / _graph_path helpers and are not covered by that issue. Suggest widening #1558's scope.

Prior findings checklist

• Fixed (verified against current code): P15 (env_mapping unknown-token_type now has a real test hitting the else branch, misleading comment corrected), P17 (example.env note now correctly distinguishes the unused oauth_providers.userinfo_url column from the hardcoded USERINFO_URL constant, and mentions the second public_mcp_apps row), P18 (decrypt_value_strict plus a genuine foreign-Fernet-key regression test that would fail if reverted), P27 (downgrade now preserves any admin-edited row via full-shape comparison, with fixtures proving survival — the earlier round's rejection is stale; the guard was rewritten twice after fbad311c), P33 (_instance_url() canonicalizes to scheme://host[:port], force.com dropped, every host-confusion input correctly rejected), P34 (both paths — refresh preserves the prior value with a warning on malformed input, callback rejects before mutation; the "declined" label in the history is stale), P42 (tests assert exact raw length, slack removed), P44 (PR body corrected).
• Partial: P40 and P41 (both roll up into B1 — not separate blocking items), P43 (minor item 1).
• Not fixed: P29 (B2); P39 (out of scope, above).
• Declined by author, still open, minor — WAIVED: P35 (instance_url optional-with-default in _build_oauth_mcp_stdio_transport_config though both call sites always pass it; duplicated except _OAuthInstanceUrlRequired handlers). The position that it is pure style with no live bug is reasonable; not re-raised.
• Previously fixed or dropped, no action: P1, P3, P4, P6, P7, P9, P10, P11, P12, P19, P20, P22, P23, P26, P28, P30, P31, P36, P37, P38 fixed; P21 (createable codespell), P24 (SOSL response shape), P25 (logging.basicConfig), P32 (tautological non-Salesforce guard test) dropped on verified-correct author rebuttals.
• Tracked follow-ups, correctly out of scope: #1540 (session-expiry/reconnect design), #1541 (SOQL pagination + get_record field cap), #1542 (sandbox host + PKCE capability modeling), #1543 (429/Retry-After), #1558 (sibling dot-segment), #1559 (sibling downgrade guards).

Simplification opportunities

tests/web/test_salesforce_oauth.py:89: shrink: test_callback_rejects_missing_instance_url_without_touching_prior_grant and test_callback_rejects_non_string_instance_url_without_touching_prior_grant (:127) have ~85% identical bodies, differing only in the token response payload. Merge into one pytest.mark.parametrize with ids=["missing","non-string"] -- matches the parametrize convention this PR already uses for same-mechanism/different-value cases.
net: -30 lines possible

Review limitations

• No linked issue exists (closingIssuesReferences empty, no issue references in the body), so intent was taken from the PR description alone and could not be corroborated against a stated requirement.
• No tests, builds or linters were run locally for this review. CI is green (14/14 checks) — that is CI's signal, not independent verification here.
• The seed migration's downgrade guard and the instance_url column drop are exercised on SQLite only, so the PostgreSQL json comparison path that 1302764c specifically fixed is unverified by CI.
• _instance_url() accepts the inert bare suffix https://.salesforce.com (not registrable, not exploitable, untested).
• Every review thread on this PR is currently marked resolved, including several whose underlying concern is still open (P29, P39, P43); statuses above come from reading the current code, not from thread state.

Blocking status and recommended decision

Blocking: yes — recommended event: REQUEST_CHANGES

• src/xagent/web/api/auth.py:2031 — MAJOR — provider predicate is prefix-based for PKCE (:1491) but exact-equality for the instance_url guard (:2031) and the provider_user_id backfill (:2095), so the documented salesforce-sandbox row gets PKCE while a missing instance_url falls through to the unconditional delete-then-recreate and provider_user_id stays NULL. Use one shared predicate at all three sites, plus a prefixed-name test. [new] root cause; [prior] consequences (P40 / P41 PARTIAL).
• src/xagent/web/tools/mcp/salesforce.py:61 — MAJOR — _success_with_capped_list truncation is irrecoverable for salesforce_list_sobjects and salesforce_describe_sobject, which expose no filter or names_only parameter, so a schema-discovery answer can be silently incomplete with no path to the remainder. Add a name/field filter or an explicit continuation contract. [prior] (P29, not fixed).

[rogercloud]

MAJOR -- this guard uses exact equality on the provider name, but PKCE at src/xagent/web/api/auth.py:1491 is gated on provider.lower().startswith("salesforce"), with a comment saying the prefix is deliberate so a salesforce-sandbox row also gets PKCE. example.env:690-701 documents that second-provider-row workaround as the supported sandbox path, so the mismatch is reachable by design.

For a salesforce-sandbox row:

1. This guard does not fire, so a token response lacking instance_url reaches the unconditional delete-then-recreate at :2191-2201 (gated only on if user_id:, always truthy there) -- destroying a previously working grant while the user sees "Connected Successfully".
2. The provider_user_id = token_data.get("id") backfill at :2095 also does not fire, and since userinfo_url is empty for Salesforce-family rows the generic elif userinfo_url and access_token fallback does not fire either -- so provider_user_id stays NULL and the (user_id, provider, provider_user_id) unique constraint (src/xagent/web/models/user_oauth.py:12-15) stops protecting against concurrent duplicate grants.
3. PKCE is granted to exactly the row that gets neither protection.

This is the same underlying issue as prior findings P40 and P41 rather than a new unrelated one: both are genuinely fixed and tested for the exact name "salesforce", and both remain reachable for the documented prefixed row -- hence PARTIAL, not FIXED.

To be clear about scope: the unlocked delete-then-insert and the NULL-tolerant composite constraint are pre-existing and generic to all providers (identical at base). The ask is only to make the predicate consistent:

def _is_salesforce_provider(provider: str) -> bool:
    return provider.lower().startswith("salesforce")

Use it at :1491, here, and :2095, and add a test that drives the callback with a prefixed provider name.

[rogercloud]

MAJOR (prior finding P29, not fixed) -- this halving loop returns only a prefix of the list with truncated: true. The last round fixed the loop's own limit-violation edge case and added a candid docstring, but there is still no recoverability contract.

The blocking part is narrow: salesforce_list_sobjects and salesforce_describe_sobject expose no filter or names_only parameter, so a truncated answer to "what objects exist" / "what fields does this object have" is silently incomplete and the model has no way to iterate toward the missing remainder.

salesforce_query and salesforce_search are fine here -- the model can narrow them with LIMIT / WHERE / more selective SOSL, and their remaining nextRecordsUrl pagination gap is correctly carved out to #1541.

Fix: add a name/field filter (or a names_only mode) to the two schema-listing tools so a bounded-but-complete answer is reachable, or give them an explicit continuation contract.

[rogercloud]

Minor (hardening + naming). This persists token_data.get("instance_url") for any provider with no type check -- the presence/type guard at :2031 covers Salesforce only. Relatedly, :2030 assigns salesforce_instance_url = token_data.get("instance_url") unconditionally in the generic path, under a provider-specific name.

Not a live bug: no currently seeded provider returns the key and there is no runtime provider-registration surface. But either validate here too, or rename the variable to something provider-neutral so the generic path does not read as Salesforce-specific.

[rogercloud]

Minor. getattr(oauth_account, "instance_url", None) defends against a missing attribute on a column the model declares unconditionally. No Mock / SimpleNamespace reaches this path, and a missing physical column would fail at query time rather than at attribute access -- so the fallback is unreachable. Prefer direct attribute access; the getattr only hides typos from mypy.

[rogercloud]

Minor. port carries two meanings here -- "" for "no port", None for "malformed". It is correct as written, but raising directly inside the except would keep the variable single-purpose and drop the downstream is None check.

Separately: urlparse(...).port returns 0 for a :0 suffix (in range, so falsy rather than raising), so :0 is silently canonicalized away rather than rejected. Practical impact is nil -- the host is already https + *.salesforce.com allowlisted and operator-set -- noting it only so the behavior is deliberate rather than incidental.

[rogercloud]

Nit: return "; ".join(messages) if messages else None -- the else None arm is dead. The earlier if not isinstance(payload, list) or not payload: return None guard guarantees messages is non-empty by the time this runs, so the conditional can be dropped.