feat(agent): builder tools — triggers, cron, find_triggers (#4919)#4931
feat(agent): builder tools — triggers, cron, find_triggers (#4919)#4931mmabrouk wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a ChangesTrigger Discovery Feature
DELETE Method Support in Agent Tool Stack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🤖 The AI agent says: What to review on this PR:
|
| items = [str(v).strip() for v in value if str(v).strip()] | ||
| if not items: | ||
| raise ValueError("use_cases must contain at least one non-empty fragment") | ||
| return items |
There was a problem hiding this comment.
🤖 The AI agent says: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the discovery request schema strict.
provider accepts any string, and _require_use_cases() stringifies arbitrary list items (123, {}) instead of rejecting them. That turns malformed payloads into meaningless searches instead of a 422.
class TriggerDiscoveryQuery(BaseModel):
"""Request body for ``POST /triggers/discover``."""
use_cases: List[str]
- provider: str = TriggerProviderKind.COMPOSIO.value
+ provider: TriggerProviderKind = TriggerProviderKind.COMPOSIO
limit_alternatives: int = Field(default=3, ge=0)
@field_validator("use_cases", mode="before")
@classmethod
def _require_use_cases(cls, value: Any) -> List[str]:
if not isinstance(value, list):
raise ValueError("use_cases must be a list of non-empty fragments")
- items = [str(v).strip() for v in value if str(v).strip()]
+ if any(not isinstance(v, str) for v in value):
+ raise ValueError("use_cases must be a list of strings")
+ items = [v.strip() for v in value if v.strip()]
if not items:
raise ValueError("use_cases must contain at least one non-empty fragment")
return items
| return items | ||
|
|
||
|
|
||
| TriggerDiscoveryResponse = TriggerCapabilitiesResult |
There was a problem hiding this comment.
🤖 The AI agent says: 📄 Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Wrap /discover in a normal API response envelope.
Aliasing the response to the core DTO makes this endpoint the odd one out in the router: the rest return {count, ...} envelopes, but this one exposes the core result directly. Please define an explicit API response model here and wrap the service DTO at the route boundary. As per coding guidelines, "Define explicit request and response models in models.py, include count plus payload in response envelopes."
| provider_key: str, | ||
| use_case: str, | ||
| limit_alternatives: int, | ||
| ) -> List[Tuple[int, TriggerCatalogEvent, TriggerCatalogIntegration]]: |
There was a problem hiding this comment.
🤖 The AI agent says: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use a named DTO for discovery matches instead of tuples.
Returning List[Tuple[int, TriggerCatalogEvent, TriggerCatalogIntegration]] from a method in core/**/service.py violates the core service contract and leaks positional tuple unpacking through the discovery flow. As per coding guidelines, "Do not return raw dicts or tuples from service methods or clients; define named DTOs in core/{domain}/dtos.py instead." and "Service methods must return typed DTOs (Pydantic BaseModel subclasses), not raw dicts, tuples, or Any."
| ) | ||
| ) | ||
|
|
||
| return sorted(matches, key=lambda item: item[0], reverse=True) |
There was a problem hiding this comment.
🤖 The AI agent says: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Drop zero-score candidates before returning matches.
When _candidate_integrations() / _candidate_events() fall back to unfiltered catalog pages, this helper still appends score-0 events. discover_triggers() then surfaces an arbitrary event instead of hitting its no-match path.
for event in events:
key = (event.integration or integration.key, event.key)
if key in seen:
continue
+ score = _score_trigger_match(
+ use_case=use_case,
+ event=event,
+ integration=integration,
+ )
+ if score <= 0:
+ continue
seen.add(key)
- matches.append(
- (
- _score_trigger_match(
- use_case=use_case,
- event=event,
- integration=integration,
- ),
- event,
- integration,
- )
- )
+ matches.append((score, event, integration))
| } | ||
| }, | ||
| "required": ["id"], | ||
| } |
There was a problem hiding this comment.
🤖 The AI agent says: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Close the no-arg and id-only schemas.
_EMPTY_INPUT_SCHEMA and _TRIGGER_ID_INPUT_SCHEMA currently leave additionalProperties open, so list_connections plus the POST pause_* / resume_* ops expose arbitrary extra body fields to the backend. That makes these tools depend on server-side internals instead of the cataloged request shape.
-_EMPTY_INPUT_SCHEMA: Dict[str, Any] = {"type": "object", "properties": {}}
+_EMPTY_INPUT_SCHEMA: Dict[str, Any] = {
+ "type": "object",
+ "properties": {},
+ "additionalProperties": False,
+}
_TRIGGER_ID_INPUT_SCHEMA: Dict[str, Any] = {
"type": "object",
"properties": {
"id": {
"type": "string",
"description": "The schedule or subscription id returned by the list tools.",
}
},
"required": ["id"],
+ "additionalProperties": False,
}
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
services/agent/tests/unit/tool-direct.test.ts (1)
382-421: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one relay-level test for path-param stripping.
These cases lock down URL construction, but they will not catch the new POST regression because
executeRelayedTool()is the layer that still forwards the original body. A single test that inspects the fetch payload after{id}substitution would cover the contract this PR is trying to add.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 02c0305c-383f-4ef7-9b3e-ddefa6a78701
⛔ Files ignored due to path filters (1)
services/agent/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
api/oss/src/apis/fastapi/triggers/models.pyapi/oss/src/apis/fastapi/triggers/router.pyapi/oss/src/core/triggers/dtos.pyapi/oss/src/core/triggers/service.pyapi/oss/tests/pytest/unit/triggers/test_triggers_discovery.pysdks/python/agenta/sdk/agents/platform/op_catalog.pysdks/python/agenta/sdk/agents/tools/models.pysdks/python/oss/tests/pytest/unit/agents/platform/test_op_catalog.pyservices/agent/src/protocol.tsservices/agent/src/tools/direct.tsservices/agent/src/tools/relay.tsservices/agent/tests/unit/tool-direct.test.ts
mmabrouk
force-pushed
the
feat/agent-skills-4918-v2
branch
from
420f61c to
a568150
Compare
mmabrouk
force-pushed
the
feat/agent-builder-tools-4919-v2
branch
from
2f3b95e to
03a5eb3
Compare
mmabrouk
force-pushed
the
feat/agent-skills-4918-v2
branch
from
a568150 to
60620ae
Compare
mmabrouk
force-pushed
the
feat/agent-builder-tools-4919-v2
branch
from
03a5eb3 to
f7d58c7
Compare
mmabrouk
force-pushed
the
feat/agent-skills-4918-v2
branch
from
60620ae to
4b8d038
Compare
mmabrouk
force-pushed
the
feat/agent-builder-tools-4919-v2
branch
from
f7d58c7 to
19a2f07
Compare
There was a problem hiding this comment.
the tests here are pretty thin for a complicated logic. please update with much more use cases
There was a problem hiding this comment.
🤖 The AI agent says: Expanded the suite substantially in cc2e131. Added direct coverage for the scoring/evidence helpers (_discovery_terms stopword/dedup/short-token handling, _match_signal exact-phrase vs distinct-term counting, zero-score cases, _has_primary_evidence), for _discover_events_for_use_case (score-descending ranking and the zero-score drop), and for discover_triggers end to end (weak single-term -> no-match, no-events -> no-match, mixed match/no-match across multiple use cases, and slug-collision increment in the connection-state path). 22 tests pass.
| min_length=1, description="Model-facing description (SDK-owned)." | ||
| ) | ||
| method: Literal["GET", "POST"] | ||
| method: Literal["GET", "POST", "DELETE"] |
There was a problem hiding this comment.
any reason we dont want to include put?
There was a problem hiding this comment.
🤖 The AI agent says: Intentionally excluded for now. No cataloged op targets a PUT endpoint: the edit endpoints (PUT /triggers/subscriptions/{id}, PUT /triggers/schedules/{id}, the workflow edits) are not exposed as platform ops, lifecycle is POST .../stop/.../start, and deletes are DELETE. The whole direct-call path is GET/POST/DELETE only too — DIRECT_CALL_METHODS in services/agent/src/tools/direct.ts, the callDirect method type, and the body-sent-only-on-POST branch — so widening this Literal alone would be incoherent. When we catalog an edit op we will add PUT to the Literal and the runner allowlist + body handling together. Leaving as-is.
mmabrouk
force-pushed
the
feat/agent-skills-4918-v2
branch
from
4b8d038 to
851f2e0
Compare
mmabrouk
force-pushed
the
feat/agent-builder-tools-4919-v2
branch
from
19a2f07 to
adf0f9e
Compare
…+ strip path params Address PR #4931 review: - triggers discovery: gate primary match on stronger evidence (exact phrase or >=2 distinct terms) and drop zero-score candidates so weak/ambiguous use cases fall to the no-match path instead of surfacing an arbitrary integration. - op_catalog: set additionalProperties:false on the create-schedule/subscription schemas (and their data objects) so the model cannot retarget via references/selector. - direct-call relay: strip substituted path params out of the POST body. - tests: substantially expand trigger-discovery coverage; assert ValidationError. Claude-Session: https://claude.ai/code/session_01LUTGjUDzSX1tiJwqnqACtj
mmabrouk
force-pushed
the
feat/agent-skills-4918-v2
branch
from
851f2e0 to
2340380
Compare
…+ strip path params Address PR #4931 review: - triggers discovery: gate primary match on stronger evidence (exact phrase or >=2 distinct terms) and drop zero-score candidates so weak/ambiguous use cases fall to the no-match path instead of surfacing an arbitrary integration. - op_catalog: set additionalProperties:false on the create-schedule/subscription schemas (and their data objects) so the model cannot retarget via references/selector. - direct-call relay: strip substituted path params out of the POST body. - tests: substantially expand trigger-discovery coverage; assert ValidationError. Claude-Session: https://claude.ai/code/session_01LUTGjUDzSX1tiJwqnqACtj
mmabrouk
force-pushed
the
feat/agent-builder-tools-4919-v2
branch
from
cc2e131 to
c0ccd67
Compare
mmabrouk
force-pushed
the
feat/agent-skills-4918-v2
branch
from
2340380 to
505fb1b
Compare
…+ strip path params Address PR #4931 review: - triggers discovery: gate primary match on stronger evidence (exact phrase or >=2 distinct terms) and drop zero-score candidates so weak/ambiguous use cases fall to the no-match path instead of surfacing an arbitrary integration. - op_catalog: set additionalProperties:false on the create-schedule/subscription schemas (and their data objects) so the model cannot retarget via references/selector. - direct-call relay: strip substituted path params out of the POST body. - tests: substantially expand trigger-discovery coverage; assert ValidationError. Claude-Session: https://claude.ai/code/session_01LUTGjUDzSX1tiJwqnqACtj
mmabrouk
force-pushed
the
feat/agent-builder-tools-4919-v2
branch
from
c0ccd67 to
34f4351
Compare
mmabrouk
force-pushed
the
feat/agent-skills-4918-v2
branch
from
505fb1b to
b7c6154
Compare
…+ strip path params Address PR #4931 review: - triggers discovery: gate primary match on stronger evidence (exact phrase or >=2 distinct terms) and drop zero-score candidates so weak/ambiguous use cases fall to the no-match path instead of surfacing an arbitrary integration. - op_catalog: set additionalProperties:false on the create-schedule/subscription schemas (and their data objects) so the model cannot retarget via references/selector. - direct-call relay: strip substituted path params out of the POST body. - tests: substantially expand trigger-discovery coverage; assert ValidationError. Claude-Session: https://claude.ai/code/session_01LUTGjUDzSX1tiJwqnqACtj
mmabrouk
force-pushed
the
feat/agent-builder-tools-4919-v2
branch
from
34f4351 to
d4b566e
Compare
mmabrouk
force-pushed
the
feat/agent-skills-4918-v2
branch
from
b7c6154 to
9a02b8a
Compare
mmabrouk
force-pushed
the
feat/agent-builder-tools-4919-v2
branch
from
d4b566e to
115d146
Compare
1 participant
Context
An agent building itself needs to set up cron schedules, subscribe to external events, and discover what triggers are available for a given use case. None of those operations existed in the platform op catalog or the API. Without them, the
set-up-triggersskill (PR #4930) names tools the agent cannot call.Design:
docs/design/agent-workflows/projects/agent-builder-capabilities/Changes
SDK —
op_catalog.py: Seven new platform ops. Mutation ops default to approval; read ops default to allow.find_triggersPOST /api/triggers/discovercreate_trigger/create_cronPOST /api/triggers/subscriptions//schedules/delete_trigger/delete_cronDELETE /api/triggers/subscriptions/{id}/schedules/{id}list_triggers/list_cronsGET /api/triggers/subscriptions//schedules/Destinations (
data.references,data.selector) are bound from run context ($ctx) and stripped from the model-facing schema. The model cannot route a trigger to an arbitrary target.API — triggers router and service: New
POST /triggers/discoverendpoint. Acceptsuse_cases(list of keyword fragments),provider, andlimit_alternatives. The service methoddiscover_triggerstokenizes each use-case string, scores every catalog event by keyword overlap, and returns the top match per use case plus alternatives, connection state, and guidance.New DTOs in
dtos.pyunder the# Trigger discoverysection:TriggerCapabilitiesResult,TriggerCapability,DiscoveredTriggerEvent,TriggerDiscoveryConnectionState,TriggerConnectionRequirement,TriggerConnectAffordance,TriggerDiscoveryGuidance.TS runner (
services/agent/):protocol.ts—DELETEadded toDirectCallMethodunion (required for delete ops that use paths like/triggers/{id}).direct.ts—directCallUrlnow substitutes path parameters (e.g.,{trigger_id}) from the assembled body params before the call.relay.ts— Assembles the body before computing the URL so path param substitution has access to all params.Scope / risk
find_triggersdoes keyword matching in-process against the Composio trigger catalog. It is not a semantic search. The quality of matches depends on overlap between the user's words and the event names/descriptions in the catalog. The_DISCOVERY_STOPWORDSset is a first-pass filter; it may need tuning.The
relay.tschange (assemble body beforedirectCallUrl) is a correctness fix, but it is a behavior change to the relay path. Verify that path-param substitution does not incorrectly consume fields intended as request body keys.The
DELETEmethod addition indirect.tsopens delete calls to any direct-call op, not just trigger ops. Confirm the allowlist and path-param logic are correct before merging.The new platform ops (
create_trigger,delete_trigger, etc.) are model-facing. The input schemas, approval defaults, and context bindings are load-bearing for safety. These deserve a careful read.Tests
op_catalog): 18/18 passed (Codex-reported).triggers discovery): 6/6 passed (Codex-reported).tsc --noEmit): clean.tool-direct.test.ts): 42/42 passed (Codex-reported).ruff format/check: clean.How to QA
Prerequisites: local dev stack with the triggers service running (EE edition with Composio configured).
Steps:
POST /api/triggers/discoverwith{"use_cases": ["new GitHub pull request"]}and a valid project API key.capabilitieslist with at least one entry whoseevent.integrationisGITHUBor similar.connectionslist reflects the current connection state for that integration.Automated tests:
Edge cases:
POST /triggers/discoverwithuse_cases: [""](empty string). Expect a 422 validation error from the_require_use_casesvalidator.DELETEdirect call op. Confirm the path parameter is substituted correctly and the body does not include the substituted field as a body key.list_triggers/list_cronsfrom an agent run and confirm the routing context binding ($ctx) fills the destination without any model-supplied value.https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc