feat(salesforce): add Salesforce CRM integration#271
Conversation
- 7 actions: search_records, get_record, update_record, list_tasks, list_events, get_task_summary, get_event_summary - OAuth 2.0 platform auth, instance_url resolved from context.metadata - 56 pytest unit tests, zero credentials required - Real Salesforce logo icon (512x512) - README with auth setup, actions table, troubleshooting
🔍 Integration Validation ResultsCommit: Changed directories:
✅ Structure Check output✅ Code Check output✅ Tests output✅ README Check output✅ Version Check output |
|
@Shubhank-Jonnada Can you please change the integration tests to match the setup from other integrations that have them now. For instance, they should load the required credentials from your local .env and match an entry in .env.example (empty value) and there are standard patterns for this that the skill should be able to easily mimic and create. |
TheRealAgentK
left a comment
There was a problem hiding this comment.
See comment on main thread re integration tests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6de77be8db
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…etchResponse, live_context - Rewrote test_salesforce_unit.py using importlib.util.spec_from_file_location loader and FetchResponse(status, headers, data) mocks per writing-unit-tests skill - Added test_salesforce_integration.py with live_context Variant 3 (platform OAuth with real aiohttp), require_*_id() skip helpers, and @pytest.mark.destructive guard - Removed old test_salesforce.py (asyncio.run style, superseded) - All 56 unit tests pass; ruff + bandit clean
- Rename SALESFORCE_TOKEN → SALESFORCE_ACCESS_TOKEN to match _ACCESS_TOKEN pattern - Rename SALESFORCE_RECORD/TASK/EVENT_ID → SALESFORCE_TEST_*_ID to match TEST_ prefix pattern - Expand docstring with full env var reference - Add Salesforce block to .env.example (SALESFORCE_ACCESS_TOKEN, SALESFORCE_INSTANCE_URL, TEST_RECORD/TASK/EVENT_ID)
Add _validate_sf_id() that enforces 15/18-char alphanumeric format on record_id, task_id, and event_id before they are interpolated into SOQL WHERE clauses or sobject URL paths — addresses SOQL injection risk flagged by code review on get_task_summary and get_event_summary. - Add TestValidateSfId unit tests (accepts 15/18-char, rejects short/special/empty) - Update existing test fixtures to use valid 15-char IDs (e.g. 003000000000001) - 64 unit tests passing
Done, aligned with the standard pattern env vars renamed to follow the ACCESS_TOKEN / TEST conventionsand added to .env.example @TheRealAgentK |
Review — Salesforce Integration
🔴 Must-Fix1. Missing The integration pins
# Current (1.0.x pattern)
return ActionResult(data={"result": False, "error": str(e)}, cost_usd=0.0)
# Should be
return ActionError(message=str(e))
2. Output schemas still include Once error paths use 3. Unit tests assert the old error pattern All error tests check assert result.type == ResultType.ACTION_ERROR
assert "..." in result.result.message
4. The SDK dependency is 2.0.0, so the integration version in 🟡 Should-Fix5. SOQL injection on
conditions.append(f"OwnerId = '{assigned_to_id}'")It should either be validated with 6. It only does 7. README "Running Tests" section is incorrect The README says # Unit tests
pytest salesforce/ -v
# Integration tests
pytest salesforce/tests/test_salesforce_integration.py -m integration8.
🟢 Looks Good
Summary: The integration is well-written and functional, but it's half-upgraded to SDK 2.0.0 — |
Item 4 is bs - ignore that @Shubhank-Jonnada - I'll make sure the skill gets an update :) Item 5: that looks like a correct finding, but verify please Item 6: I think that's partially wrong - we do have a conftest, but I'll need to double check something Item 8 is incorrect too - will adjust the skills 1,2 3 and 7 look all correct to me. |
…w use ActionError
…SOQL interpolation
|
Resolved these @TheRealAgentK, please check
|
|
Also removed context.py - 100126d - updated validator no longer requires it |
Summary
Adds a brand new Salesforce CRM integration covering searching/updating records and summarising task & event activity.
Actions (7)
search_recordsget_recordupdate_recordlist_taskslist_eventsget_task_summaryget_event_summaryAuth
platformauth type)instance_urlresolved fromcontext.metadata— Salesforce returns this at token time and the platform stores it separately from credentialsTests
pytest -m unit)pytest -m integration)Test plan
pytest salesforce/tests/test_salesforce_unit.py -v— all 56 passvalidate_integration.py salesforce— no errorscheck_code.py salesforce— no errorssearch_records,list_tasks,list_events