feat(agent): builder tools — triggers, cron, find_triggers (#4919)#4928
feat(agent): builder tools — triggers, cron, find_triggers (#4919)#4928mmabrouk wants to merge 1 commit into
Conversation
…ign)
- Add trigger/cron platform ops to op_catalog.py: create_trigger,
delete_trigger, list_triggers, create_cron, delete_cron, list_crons,
find_triggers; each wired as direct-call tools with self/target ctx
bindings and approval defaults.
- Extend find_triggers backend: new GET /triggers/discover endpoint
(triggers router + service) returning event details and connection
readiness per use-case.
- Extend SDK tools.models with trigger schema types.
- Add DELETE to the direct-call method allowlist in protocol.ts and
direct.ts; add path-parameter substitution in directCallUrl so
DELETE /triggers/{id} receives the id from the assembled body.
- relay.ts: assemble body before constructing URL (needed by path params).
- Tests: 18/18 op_catalog tests pass; 6/6 trigger discovery tests pass;
42/42 TS direct-call unit tests pass.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a ChangesTrigger Discovery Backend
Agent Platform Tools and DELETE Support
Agent Builder Capabilities Design Docs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
🤖 The AI agent says: What to review on this PR:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
docs/design/agent-workflows/projects/agent-builder-capabilities/README.md (1)
209-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify what "already ship" means for delete endpoints.
The design states that remove/pause/resume tools map to "delete and start/stop endpoints that already ship." While the API routes (
DELETE /api/triggers/schedules/{id}etc.) existed in the backend, the agent tool stack did not support theDELETEHTTP method until this PR (added todirect.ts,relay.ts,protocol.ts, and Python SDKToolCall). Consider adding a parenthetical noting that the agent-side DELETE binding is new in this project, to prevent readers from assuming the agent integration was already complete.api/oss/tests/pytest/unit/triggers/test_triggers_discovery.py (1)
58-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a no-match regression test for discovery.
The service has an explicit no-match branch, but this file only covers positive matches and connection-state inference. A case where every candidate scores
0would lock in the expectednote/empty-event behavior and would have caught the current fallback bug.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 384e1e57-ea61-4f15-9312-87cda2bffffe
⛔ Files ignored due to path filters (1)
services/agent/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
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.pydocs/design/agent-workflows/projects/agent-builder-capabilities/README.mddocs/design/agent-workflows/projects/agent-builder-capabilities/status.mdsdks/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
| use_cases: List[str] | ||
| provider: str = TriggerProviderKind.COMPOSIO.value | ||
| 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 not items: | ||
| raise ValueError("use_cases must contain at least one non-empty fragment") | ||
| return items |
There was a problem hiding this comment.
🎯 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.
Suggested fix
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📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use_cases: List[str] | |
| provider: str = TriggerProviderKind.COMPOSIO.value | |
| 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 not items: | |
| raise ValueError("use_cases must contain at least one non-empty fragment") | |
| return items | |
| use_cases: List[str] | |
| 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") | |
| 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.
🗄️ 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."
Source: Coding guidelines
| async def _discover_events_for_use_case( | ||
| self, | ||
| *, | ||
| provider_key: str, | ||
| use_case: str, | ||
| limit_alternatives: int, | ||
| ) -> List[Tuple[int, TriggerCatalogEvent, TriggerCatalogIntegration]]: |
There was a problem hiding this comment.
📐 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."
Source: Coding guidelines
| matches.append( | ||
| ( | ||
| _score_trigger_match( | ||
| use_case=use_case, | ||
| event=event, | ||
| integration=integration, | ||
| ), | ||
| event, | ||
| integration, | ||
| ) | ||
| ) | ||
|
|
||
| return sorted(matches, key=lambda item: item[0], reverse=True) |
There was a problem hiding this comment.
🎯 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.
Suggested fix
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))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| matches.append( | |
| ( | |
| _score_trigger_match( | |
| use_case=use_case, | |
| event=event, | |
| integration=integration, | |
| ), | |
| event, | |
| integration, | |
| ) | |
| ) | |
| return sorted(matches, key=lambda item: item[0], reverse=True) | |
| score = _score_trigger_match( | |
| use_case=use_case, | |
| event=event, | |
| integration=integration, | |
| ) | |
| if score <= 0: | |
| continue | |
| matches.append( | |
| ( | |
| score, | |
| event, | |
| integration, | |
| ) | |
| ) | |
| return sorted(matches, key=lambda item: item[0], reverse=True) |
| _EMPTY_INPUT_SCHEMA: Dict[str, Any] = {"type": "object", "properties": {}} | ||
| _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"], | ||
| } |
There was a problem hiding this comment.
🔒 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.
Proposed fix
-_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,
}Also applies to: 531-539, 568-603
|
🤖 The AI agent says: Superseded by #4931 — same content, rebuilt as a clean GitButler stack. Closing this one. |
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 #4924) 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