[auto] #86 feat: webhook triggers — GitHub/Slack events start agent workflows - #97
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a webhook subsystem: new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP as HTTPServer
participant Deduper as Deduplicator
participant Resolver as WorkspaceResolver
participant Matcher as TriggerMatcher
participant Dispatcher as Dispatcher
participant Workflow as WorkflowRunner
participant Agent as AgentSender
Client->>HTTP: POST /v1/webhooks/generic {body, headers}
HTTP->>HTTP: enforce 1MB body cap
HTTP->>Deduper: compute/lookup dedup key (headers/body)
Deduper-->>HTTP: cached response? / none
alt cached
HTTP-->>Client: 200 cached response
else not cached
HTTP->>HTTP: parse JSON -> source,event,workspace,payload
HTTP->>Resolver: resolve_action_workspace(workspace_hint)
Resolver-->>HTTP: workspace
HTTP->>Matcher: match_triggers(source,event)
Matcher-->>HTTP: matched_rules
alt no matches
HTTP->>WebhookLog: log_event(matched=false)
Deduper->>Deduper: save response under dedup key
HTTP-->>Client: 200 {"matched":false,"triggers_fired":0}
else matches
HTTP->>Dispatcher: acquire WEBHOOK_DISPATCH_LOCK
loop per matched_rule
alt rule.workflow
Dispatcher->>Workflow: run(workspace, workflow, payload)
Workflow-->>Dispatcher: workflow result
else rule.agent
Dispatcher->>Agent: expand_template & send(workspace, agent, prompt)
Agent-->>Dispatcher: send result
end
end
Dispatcher->>WebhookLog: log_event(matched=true)
Deduper->>Deduper: save response under dedup key
HTTP-->>Client: 200 {"matched":true,"triggers_fired":n,"dispatched":[...]}
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/up.rs (1)
154-167:⚠️ Potential issue | 🟠 MajorInconsistent profile-awareness between
run()andrun_all().The single-workspace
run()function doesn't checkagent_uses_profilebefore blocking or incrementing, unlikerun_all(). In a mixed-runtime workspace (e.g., claude-code + codex agents) with a Claude profile limit:
- Line 155: The capacity check blocks all agents when the Claude limit is reached, even codex agents that don't use the Claude profile.
- Line 334: The increment applies unconditionally, incorrectly counting codex agents against the Claude profile.
🐛 Proposed fix to add profile compatibility check
For the capacity check (around line 154):
if let Some(limit) = &profile_limit + && global.as_ref().is_some_and(|g| agent_uses_profile(&config, g, agent, &limit.profile_name)) && active_for_profile >= limit.max_concurrent {For the post-launch increment (around line 333):
launched.push((agent.name.clone(), session, runtime_name)); - if profile_limit.is_some() { + if let Some(limit) = &profile_limit + && global.as_ref().is_some_and(|g| agent_uses_profile(&config, g, agent, &limit.profile_name)) + { active_for_profile += 1; }Also applies to: 331-335
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/up.rs` around lines 154 - 167, The run() path is incorrectly applying a profile limit to agents that may not use that profile; update the capacity check and the post-launch increment to first verify the agent actually uses the profile (use the same agent_uses_profile predicate used by run_all()) before comparing against limit.max_concurrent or incrementing active_for_profile; specifically, wrap the check that currently sets refused_by_limit and the block that increments the profile counter (references: run(), profile_limit, active_for_profile, limit.max_concurrent, refused_by_limit, agent.name, and the post-launch increment near the existing code around lines ~331-335) so they only run when agent_uses_profile(agent, &limit.profile_name) is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/cli/up.rs`:
- Around line 154-167: The run() path is incorrectly applying a profile limit to
agents that may not use that profile; update the capacity check and the
post-launch increment to first verify the agent actually uses the profile (use
the same agent_uses_profile predicate used by run_all()) before comparing
against limit.max_concurrent or incrementing active_for_profile; specifically,
wrap the check that currently sets refused_by_limit and the block that
increments the profile counter (references: run(), profile_limit,
active_for_profile, limit.max_concurrent, refused_by_limit, agent.name, and the
post-launch increment near the existing code around lines ~331-335) so they only
run when agent_uses_profile(agent, &limit.profile_name) is true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 50a21b65-bee7-48d3-add6-6e59df2f4b3e
📒 Files selected for processing (2)
src/cli/serve.rssrc/cli/up.rs
Add WebhookConfig struct to TuttiConfig with [[webhook]] TOML array parsing (source, events, workflow, agent, prompt fields). Add POST /v1/webhooks/generic endpoint in serve.rs that accepts JSON payloads, enforces a 1MB size limit, matches against configured triggers by source and event type, and dispatches via the existing workflow run or agent send infrastructure. Includes unit tests for config deserialization. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/serve.rs`:
- Around line 766-818: The webhook handler currently loops over matched triggers
(matched -> wh) and unconditionally calls super::run::run or super::send::run,
causing duplicate non-idempotent executions; add idempotency/deduplication by
deriving or reading a delivery key (e.g., a header or fingerprint from
source/event_type/wh) and performing an atomic check-and-set against a durable
idempotency store before dispatching each trigger (check store for key -> if
present skip and don't push to dispatched; if absent insert key and then call
super::run::run or super::send::run). Ensure the check-and-set is atomic to
prevent races and update the dispatched array and triggers_fired count to
reflect only newly executed actions; reference the loop over matched,
wh.workflow/wh.agent, dispatched, and the calls to super::run::run and
super::send::run when implementing.
- Around line 709-723: The current Content-Length check is bypassable because
read_json_body(request) still consumes the whole stream; change route_webhook to
read the request body through a bounded reader limited to WEBHOOK_MAX_BODY_BYTES
+ 1 (e.g., use a Take wrapper on the Request body or otherwise read at most that
many bytes), read into a buffer, reject with an error if the actual bytes read
exceed WEBHOOK_MAX_BODY_BYTES, and then parse the JSON from that buffer instead
of calling read_json_body; update references to Request and read_json_body
accordingly so parsing uses the bounded buffer and not the unbounded stream.
- Around line 769-800: The issue is that with_project_root() calls
std::env::set_current_dir() which mutates global CWD and races when HTTP
requests are handled concurrently by
start_control_http_server()/route_webhook(), causing run::run and send::run to
execute in the wrong workspace; fix by removing dependence on process-global CWD
in the webhook dispatch paths: either (preferred) modify run::run and send::run
(and any helpers they call) to accept an explicit project_root/Path parameter
and use that for all file/path operations instead of relying on
set_current_dir(), then call those new APIs from route_webhook with
target.project_root; or if changing APIs is not feasible, serialize CWD changes
with a global Mutex around with_project_root() so only one request can set CWD
at a time (ensuring the mutex guards the entire call into run::run or
send::run).
In `@src/cli/up.rs`:
- Around line 1340-1373: The profile concurrency gate is still applied globally
and blocks agents with incompatible runtimes; update the checks in run() (both
spots currently around lines 154-167 and 333-335) and in run_all() (around lines
1541-1549) to only consider agents that actually use the target profile by
invoking agent_uses_profile(config, global, agent, profile_name) when
computing/inspecting concurrent counts; replace or augment any existing calls
that use count_running_agents_for_profile or raw max_concurrent comparisons so
they filter agents with agent_uses_profile before enforcing max_concurrent,
ensuring incompatible runtimes are not counted against the profile limit.
In `@src/config/mod.rs`:
- Around line 315-331: TuttiConfig::validate() currently skips checking
WebhookConfig entries; update validate to iterate over self.webhooks and
enforce: WebhookConfig.source is non-empty, exactly one of
WebhookConfig.workflow or WebhookConfig.agent is present (reject both or
neither), and if workflow is set it exists in self.workflows (or the workflows
collection used in config), and if agent is set it exists in self.agents (or the
agents collection); return a clear Err for the first invalid webhook mentioning
the offending source/index and the missing/unknown name so invalid webhooks fail
at startup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b45af228-4b66-4018-ac65-9fdfbbec474a
📒 Files selected for processing (8)
src/automation/mod.rssrc/budget/mod.rssrc/cli/doctor.rssrc/cli/handoff.rssrc/cli/serve.rssrc/cli/up.rssrc/cli/watch.rssrc/config/mod.rs
✅ Files skipped from review due to trivial changes (3)
- src/cli/doctor.rs
- src/budget/mod.rs
- src/automation/mod.rs
- Enforce webhook body size cap while reading via take() instead of only checking Content-Length header (serve.rs) - Add webhook deduplication using X-GitHub-Delivery header, Idempotency-Key, or payload hash to prevent duplicate dispatches from retries (serve.rs) - Serialize webhook dispatches with a mutex to prevent concurrent cwd races from with_project_root (serve.rs) - Only enforce profile max_concurrent limit against agents that actually use the profile's runtime (up.rs) - Add webhook config validation for workflow and agent references (config/mod.rs) - Extract webhook matching, template expansion, and event logging into src/webhook.rs module Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/cli/serve.rs (2)
709-712:⚠️ Potential issue | 🔴 CriticalThe CWD mutex still leaves cross-thread races open.
This guard is only taken in
route_webhook()./v1/actions/*and the recovery tick still callwith_project_root()without the same mutex, so an action/recovery thread can still interleave with a webhook dispatch and run commands in the wrong workspace. The serialization needs to live inwith_project_root()itself, or these paths need to stop mutating process-global CWD entirely.Also applies to: 757-761
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/serve.rs` around lines 709 - 712, The current Mutex (WEBHOOK_DISPATCH_LOCK) only guards route_webhook(), leaving with_project_root() calls from /v1/actions/* and the recovery tick unprotected and allowing CWD races; move the serialization into with_project_root() itself by acquiring a global static Mutex (or reuse/rename WEBHOOK_DISPATCH_LOCK to something like PROJECT_ROOT_CWD_LOCK) at the start of with_project_root() so every caller that mutates the process CWD is serialized, and remove or stop using the ad-hoc lock in route_webhook(); alternatively, change with_project_root() and all callers to avoid mutating the global CWD entirely if you prefer not to use a global lock.
735-738:⚠️ Potential issue | 🔴 CriticalWebhook idempotency is still non-atomic.
idempotency_lookup()runs before the critical section, and the key is only saved after every matched trigger succeeds. Two identical deliveries can both miss the cache before either saves, and a retry after trigger 1 succeeds / trigger 2 fails will replay trigger 1. The check-and-set needs to happen inside the same atomic section, with per-trigger or in-progress state if partial failures should be retry-safe.Also applies to: 757-761, 763-830
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/serve.rs` around lines 735 - 738, The idempotency check (webhook_dedup_key + idempotency_lookup) occurs outside the critical section so two concurrent deliveries can both miss the cache; change the flow to an atomic claim pattern: implement a new idempotency_claim (or update idempotency_lookup to support claim semantics) that does an atomic check-and-set (or DB transaction/row-level lock) to insert an "in-progress" marker for the dedup_key before processing triggers, and update that marker to the final response on success or a failure/cleared state on error; update all callsites (the webhook handling block around webhook_dedup_key/idempotency_lookup and the other ranges noted) to use this claim API so concurrent deliveries cannot both proceed.
🧹 Nitpick comments (1)
src/webhook.rs (1)
6-22: Keep these helpers off the public API surface.These are internal helpers, but the module currently exposes
pub fnsignatures returning bareVec/String/(). Either lower the visibility boundary here or wrap them in a public API that returnsResult<_, TuttiError>so the module stays aligned with the repo’s public-function rule.As per coding guidelines, "All public functions must return
Result<T, TuttiError>".Also applies to: 27-45, 65-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/webhook.rs` around lines 6 - 22, The function match_triggers is an internal helper but is currently public; either make it non-public (remove the pub) or change its signature to return Result<Vec<&'a WebhookConfig>, TuttiError> and map any internal failure into a TuttiError so it conforms to the rule "All public functions must return Result<T, TuttiError>"; update any call sites accordingly and apply the same visibility/signature fix to the other internal helpers noted (the functions around 27-45 and 65-90) so they are either private or return Result with TuttiError, referencing the WebhookConfig and TuttiError types when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config/mod.rs`:
- Around line 677-703: The webhook validation loop in src/config/mod.rs
currently allows a webhook with both workflow set and prompt set which will be
ignored at runtime in route_webhook; update the validation in the for (i, wh) in
self.webhooks.iter().enumerate() block to reject webhooks where
wh.workflow.is_some() && wh.prompt.is_some() (or alternatively wire wh.prompt
through to route_webhook), by returning a TuttiError::ConfigValidation
explaining that webhook[{i}] cannot specify both 'workflow' and 'prompt' (or
that 'prompt' is unsupported for workflow webhooks) so the config fails fast
instead of silently dropping the prompt.
---
Duplicate comments:
In `@src/cli/serve.rs`:
- Around line 709-712: The current Mutex (WEBHOOK_DISPATCH_LOCK) only guards
route_webhook(), leaving with_project_root() calls from /v1/actions/* and the
recovery tick unprotected and allowing CWD races; move the serialization into
with_project_root() itself by acquiring a global static Mutex (or reuse/rename
WEBHOOK_DISPATCH_LOCK to something like PROJECT_ROOT_CWD_LOCK) at the start of
with_project_root() so every caller that mutates the process CWD is serialized,
and remove or stop using the ad-hoc lock in route_webhook(); alternatively,
change with_project_root() and all callers to avoid mutating the global CWD
entirely if you prefer not to use a global lock.
- Around line 735-738: The idempotency check (webhook_dedup_key +
idempotency_lookup) occurs outside the critical section so two concurrent
deliveries can both miss the cache; change the flow to an atomic claim pattern:
implement a new idempotency_claim (or update idempotency_lookup to support claim
semantics) that does an atomic check-and-set (or DB transaction/row-level lock)
to insert an "in-progress" marker for the dedup_key before processing triggers,
and update that marker to the final response on success or a failure/cleared
state on error; update all callsites (the webhook handling block around
webhook_dedup_key/idempotency_lookup and the other ranges noted) to use this
claim API so concurrent deliveries cannot both proceed.
---
Nitpick comments:
In `@src/webhook.rs`:
- Around line 6-22: The function match_triggers is an internal helper but is
currently public; either make it non-public (remove the pub) or change its
signature to return Result<Vec<&'a WebhookConfig>, TuttiError> and map any
internal failure into a TuttiError so it conforms to the rule "All public
functions must return Result<T, TuttiError>"; update any call sites accordingly
and apply the same visibility/signature fix to the other internal helpers noted
(the functions around 27-45 and 65-90) so they are either private or return
Result with TuttiError, referencing the WebhookConfig and TuttiError types when
making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 086b0a61-0e9c-440b-b995-3a4613111f37
📒 Files selected for processing (5)
src/cli/serve.rssrc/cli/up.rssrc/config/mod.rssrc/main.rssrc/webhook.rs
✅ Files skipped from review due to trivial changes (1)
- src/main.rs
Prompt is only consumed for direct agent dispatch; a workflow webhook with prompt silently drops it. Fail at config validation instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/serve.rs`:
- Around line 910-916: The current fallback dedup key uses Rust's process-seeded
DefaultHasher (serialized, hasher, hasher.finish(),
format!("webhook:hash:...")), which won't be stable across restarts; replace it
with a stable digest (e.g., SHA-256) by importing a stable hash implementation
(sha2::Sha256 and Digest) and computing the hash over body bytes (or serialized)
to produce a hex string, then use a stable key namespace like
"webhook:sha256:<hex>"; add the sha2 (and hex if needed) crate to Cargo.toml and
update the imports to use Digest instead of DefaultHasher so the dedup key
survives process restarts.
- Around line 823-877: The code currently ignores wh.agent when wh.workflow is
Some because the branch is "if let Some(workflow) { ... } else if let
Some(agent) { ... }"; change the behavior to dispatch both when both are present
by replacing the "else if let Some(agent)" with a separate "if let Some(agent)"
block (or extract the agent-send logic into a helper and call it from both
places) so both super::run::run (for workflow) and super::send::run (for agent)
are invoked, ensure webhook::log_event and pushed entries to dispatched remain
correct (add both "dispatched_workflow" and "dispatched_send" entries when
applicable), and keep error propagation with the existing ? returns;
alternatively, if you prefer mutual exclusivity instead, add validation in
validate_webhooks() to reject configs where both workflow and agent are set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 82023e51-b0f4-45de-a76e-74529656d2d9
📒 Files selected for processing (3)
src/cli/serve.rssrc/config/mod.rssrc/main.rs
✅ Files skipped from review due to trivial changes (1)
- src/main.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/config/mod.rs
- Reject webhooks that specify both workflow and agent in validation - Replace DefaultHasher with FNV-1a for deterministic dedup keys across server restarts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/cli/serve.rs (1)
940-953: Minor: Error message mentions "webhook" but function is shared.
read_json_bodyis called for both action endpoints (line 622) and webhooks (line 776), but the error message specifically says "webhook payload too large". Consider a generic message.Suggested fix
if body.len() > WEBHOOK_MAX_BODY_BYTES { return Err(TuttiError::ConfigValidation(format!( - "webhook payload too large (>{WEBHOOK_MAX_BODY_BYTES} bytes)" + "request payload too large (>{WEBHOOK_MAX_BODY_BYTES} bytes)" ))); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/serve.rs` around lines 940 - 953, The error string in read_json_body is specific to "webhook" but the function is reused for action endpoints too; update the TuttiError::ConfigValidation message in read_json_body to a generic phrase like "payload too large" that does not mention webhook so it correctly reflects all callers (function: read_json_body, constant: WEBHOOK_MAX_BODY_BYTES).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/serve.rs`:
- Around line 848-864: The webhook prompt expansion currently calls
webhook::expand_template(raw_prompt, &payload) and injects the resulting prompt
into super::send::run which allows untrusted payload fields to become executable
instructions; update this flow to either (a) perform escaping/quoting of
interpolated values inside webhook::expand_template (escape control characters,
newlines, template markers and shell/command-like tokens), (b) add an allowlist
mechanism in webhook::expand_template so only approved JSON paths from payload
are substituted (reject or replace others with a safe placeholder), and (c)
surface an operator-facing configuration/documentation flag on the webhook
trigger to explicitly enable raw interpolation; ensure the sanitized/allowlisted
prompt variable (the one passed to super::send::run) is used instead of the raw
expansion and keep the behavior when wh.agent is None unchanged.
---
Nitpick comments:
In `@src/cli/serve.rs`:
- Around line 940-953: The error string in read_json_body is specific to
"webhook" but the function is reused for action endpoints too; update the
TuttiError::ConfigValidation message in read_json_body to a generic phrase like
"payload too large" that does not mention webhook so it correctly reflects all
callers (function: read_json_body, constant: WEBHOOK_MAX_BODY_BYTES).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4d0e15a1-6d44-4d1a-ba39-5aa93350e325
📒 Files selected for processing (2)
src/cli/serve.rssrc/config/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/config/mod.rs
Webhook payloads are untrusted; interpolated values are inserted verbatim. Document this so operators configure appropriate agent permissions for webhook-triggered workflows. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated SDLC cycle for #86.
Summary by CodeRabbit
New Features
Improvements
Tests