Skip to content

Implement webhook-driven TaskSpawner system#32

Closed
knechtionscoding wants to merge 1 commit intoprodfrom
feat/webhook-events
Closed

Implement webhook-driven TaskSpawner system#32
knechtionscoding wants to merge 1 commit intoprodfrom
feat/webhook-events

Conversation

@knechtionscoding
Copy link
Copy Markdown
Collaborator

Adds comprehensive webhook support to enable real-time task creation from GitHub and Linear events, replacing polling with immediate webhook responses.

Core Features

API Extensions:

  • Add GitHubWebhook and LinearWebhook types to TaskSpawner.When
  • Comprehensive filtering system (action, author, labels, branch, state, etc.)
  • Generated CRD manifests with new webhook fields

Webhook Processing:

  • HMAC-SHA256 signature validation for GitHub and Linear webhooks
  • Type-safe GitHub event parsing using go-github/v66 SDK for complete field coverage
  • Manual JSON parsing for Linear webhooks with flexible state/label filters
  • OR semantics across filters with case-insensitive author matching

Task Creation:

  • Shared TaskBuilder supporting both WorkItem and webhook inputs
  • Webhook-specific template variables (Event, Action, Sender, Payload.*)
  • Idempotency via delivery IDs to prevent duplicate tasks
  • Webhook source annotations for auditability

Webhook Server:

  • Controller-runtime based HTTP server (kelos-webhook-server)
  • Per-source-type deployments with --source flag (github/linear)
  • MaxConcurrency enforcement with 503 + Retry-After responses
  • Health and readiness endpoints with graceful shutdown

Infrastructure:

  • Complete Helm chart templates with per-source deployments
  • Single Ingress with path-based routing (/webhook/github, /webhook/linear)
  • Per-provider secret configuration via values.yaml
  • Dockerfile and Makefile integration

Quality Assurance:

  • 100+ unit tests covering all webhook components
  • 5 integration tests for end-to-end webhook flow verification
  • Complete examples for GitHub and Linear webhook configurations

Template Variables

Webhook-sourced tasks have access to:

  • {{.Event}} - GitHub event type or Linear resource type
  • {{.Action}} - Webhook action (created, submitted, etc.)
  • {{.Sender}} - Username who triggered the event
  • {{.Payload.field.sub}} - Full access to raw webhook payload

This enables TaskSpawners to react to webhook events in real-time instead of polling APIs, providing much faster response times for AI agent workflows.

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR is related to:

Special notes for your reviewer:

Does this PR introduce a user-facing change?


Adds comprehensive webhook support to enable real-time task creation from
GitHub and Linear events, replacing polling with immediate webhook responses.

## Core Features

**API Extensions:**
- Add GitHubWebhook and LinearWebhook types to TaskSpawner.When
- Comprehensive filtering system (action, author, labels, branch, state, etc.)
- Generated CRD manifests with new webhook fields

**Webhook Processing:**
- HMAC-SHA256 signature validation for GitHub and Linear webhooks
- Type-safe GitHub event parsing using go-github/v66 SDK for complete field coverage
- Manual JSON parsing for Linear webhooks with flexible state/label filters
- OR semantics across filters with case-insensitive author matching

**Task Creation:**
- Shared TaskBuilder supporting both WorkItem and webhook inputs
- Webhook-specific template variables (Event, Action, Sender, Payload.*)
- Idempotency via delivery IDs to prevent duplicate tasks
- Webhook source annotations for auditability

**Webhook Server:**
- Controller-runtime based HTTP server (kelos-webhook-server)
- Per-source-type deployments with --source flag (github/linear)
- MaxConcurrency enforcement with 503 + Retry-After responses
- Health and readiness endpoints with graceful shutdown

**Infrastructure:**
- Complete Helm chart templates with per-source deployments
- Single Ingress with path-based routing (/webhook/github, /webhook/linear)
- Per-provider secret configuration via values.yaml
- Dockerfile and Makefile integration

**Quality Assurance:**
- 100+ unit tests covering all webhook components
- 5 integration tests for end-to-end webhook flow verification
- Complete examples for GitHub and Linear webhook configurations

## Template Variables

Webhook-sourced tasks have access to:
- {{.Event}} - GitHub event type or Linear resource type
- {{.Action}} - Webhook action (created, submitted, etc.)
- {{.Sender}} - Username who triggered the event
- {{.Payload.field.sub}} - Full access to raw webhook payload

This enables TaskSpawners to react to webhook events in real-time instead
of polling APIs, providing much faster response times for AI agent workflows.

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 30, 2026

Greptile Summary

This PR introduces a comprehensive webhook-driven TaskSpawner system, replacing polling with real-time event handling for GitHub and Linear. It adds two new When trigger types (githubWebhook, linearWebhook) to the CRD, a standalone kelos-webhook-server binary, HMAC-SHA256 signature validation, OR-semantics filter matching, delivery-ID-based idempotency, and a refactored TaskBuilder that unifies webhook and legacy WorkItem task creation. Infrastructure includes Helm templates for per-source deployments with a shared Ingress, RBAC, and 100+ unit and integration tests.

Key findings from the review:

  • P0 panic in push event handling: getString(e.HeadCommit.ID)[:8] will panic if the commit SHA is shorter than 8 characters.
  • P1 security: io.ReadAll(r.Body) has no size limit — memory exhaustion risk from oversized payloads.
  • P1 logic bug in draft filter: nil Draft field silently passes the filter, causing unintended task creation when draft: true is configured.
  • P1 logic bug in Linear label filtering: Labels uses case-sensitive matching while ExcludeLabels uses case-insensitive EqualFold.
  • P2: defer r.Body.Close() registered after a potential early-return path.

Confidence Score: 4/5

Hold for the P0 panic and three P1 correctness bugs before merging to production.

One P0 panic and three P1 correctness bugs need fixes. Architecture, signature validation, idempotency, Helm infrastructure, and test coverage are solid. Once the panic guard, body size limit, draft-nil check, and Linear label case-sensitivity are addressed, this should be safe to merge.

internal/webhook/github_filter.go (panic + draft nil bug), internal/webhook/handler.go (body size limit + defer placement), internal/webhook/linear_filter.go (label case-sensitivity inconsistency)

Important Files Changed

Filename Overview
internal/webhook/handler.go Core webhook HTTP handler: no body size limit (memory exhaustion) and defer r.Body.Close() misplaced after potential early return.
internal/webhook/github_filter.go GitHub webhook parsing and filtering: panic risk on short commit SHA; draft filter silently passes when Draft field is nil.
internal/webhook/linear_filter.go Linear webhook parsing and filtering: Labels uses case-sensitive matching while ExcludeLabels uses EqualFold — inconsistent behavior.
internal/webhook/signature.go HMAC-SHA256 signature validation for GitHub and Linear — correctly uses hmac.Equal for constant-time comparison.
internal/taskbuilder/builder.go New shared TaskBuilder unifying WorkItem and webhook task creation via a clean TaskInput interface; no issues found.
cmd/kelos-webhook-server/main.go Webhook server entrypoint with controller-runtime manager, graceful shutdown, health/ready probes, and flag-based configuration.
api/v1alpha1/taskspawner_types.go Adds GitHubWebhook and LinearWebhook types with comprehensive filter structs and proper kubebuilder validation markers.
test/integration/webhook_handler_test.go Integration tests cover task creation, signature rejection, deduplication, 503 at max concurrency, and filter non-match.

Reviews (1): Last reviewed commit: "Implement webhook-driven TaskSpawner sys..." | Re-trigger Greptile

if !hasAllLabels(event.PullRequest.Labels, filter.Labels) {
return false, nil
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 Panic on short commit SHA in push event ID extraction

getString(e.HeadCommit.ID)[:8] will panic with a slice bounds out of range if the commit ID is fewer than 8 characters. While full SHAs are 40 characters, an empty string or any short/synthetic ID (e.g. in tests or forged payloads) will cause an unhandled panic, crashing the request goroutine.

Suggested change
}
idStr := getString(e.HeadCommit.ID)
if len(idStr) > 8 {
idStr = idStr[:8]
}
vars["ID"] = fmt.Sprintf("push-%s", idStr)

Comment on lines +34 to +42
if err != nil {
h.Log.Error(err, "Reading request body")
http.Error(w, "Failed to read body", http.StatusBadRequest)
return
}
defer r.Body.Close()

// Validate signature
if err := h.validateSignature(body, r.Header); err != nil {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 No HTTP body size limit — memory exhaustion risk

io.ReadAll(r.Body) reads the entire request body into memory without any size restriction. A malicious actor can send an arbitrarily large payload to exhaust memory and crash or degrade the server. Wrap the body with http.MaxBytesReader before reading:

Suggested change
if err != nil {
h.Log.Error(err, "Reading request body")
http.Error(w, "Failed to read body", http.StatusBadRequest)
return
}
defer r.Body.Close()
// Validate signature
if err := h.validateSignature(body, r.Header); err != nil {
r.Body = http.MaxBytesReader(w, r.Body, 25*1024*1024) // 25 MB limit
body, err := io.ReadAll(r.Body)
if err != nil {
h.Log.Error(err, "Reading request body")
http.Error(w, "Failed to read body", http.StatusBadRequest)
return
}
defer r.Body.Close()

Comment on lines +155 to +165
}
}

// Event-specific filtering
switch e := data.RawEvent.(type) {
case *github.IssueCommentEvent:
return matchesIssueCommentEvent(filter, e)
case *github.IssuesEvent:
return matchesIssuesEvent(filter, e)
case *github.PullRequestEvent:
return matchesPullRequestEvent(filter, e)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Draft filter silently passes when Draft field is nil

In matchesIssueCommentEvent (and identically in matchesPullRequestEvent and matchesPullRequestReviewEvent), the draft check is:

if event.Issue.Draft != nil && *event.Issue.Draft != *filter.Draft {
    return false, nil
}

When event.Issue.Draft == nil, the inner condition is skipped and the filter passes. A filter with draft: true would incorrectly match events where draft status is unknown/absent, causing unintended task creation.

The guard should require the field to be present and match:

Suggested change
}
}
// Event-specific filtering
switch e := data.RawEvent.(type) {
case *github.IssueCommentEvent:
return matchesIssueCommentEvent(filter, e)
case *github.IssuesEvent:
return matchesIssuesEvent(filter, e)
case *github.PullRequestEvent:
return matchesPullRequestEvent(filter, e)
if filter.Draft != nil {
if event.Issue == nil || event.Issue.PullRequestLinks == nil {
return false, nil
}
if event.Issue.Draft == nil || *event.Issue.Draft != *filter.Draft {
return false, nil
}
}

The same fix should be applied at the equivalent locations in matchesPullRequestEvent and matchesPullRequestReviewEvent.

Comment on lines +72 to +90
}

if len(f.States) > 0 {
if p.Data.State == nil {
return false
}
stateMatched := false
for _, s := range f.States {
if strings.EqualFold(s, p.Data.State.Name) {
stateMatched = true
break
}
}
if !stateMatched {
return false
}
}

if len(f.Labels) > 0 {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Inconsistent case sensitivity between Labels and ExcludeLabels filters

Labels is checked via containsAllLabels, which does an exact case-sensitive map lookup. ExcludeLabels is checked with strings.EqualFold, which is case-insensitive. A filter like labels: ["Bug"] will fail to match a Linear label named "bug", while excludeLabels: ["Bug"] will correctly exclude "bug". Update containsAllLabels to use case-insensitive comparison:

func containsAllLabels(have []string, want []string) bool {
    for _, w := range want {
        found := false
        for _, h := range have {
            if strings.EqualFold(h, w) {
                found = true
                break
            }
        }
        if !found {
            return false
        }
    }
    return true
}

Comment on lines +34 to +38
if err != nil {
h.Log.Error(err, "Reading request body")
http.Error(w, "Failed to read body", http.StatusBadRequest)
return
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 defer r.Body.Close() registered after potential early return

If io.ReadAll returns an error, execution returns before the defer r.Body.Close() is ever registered, leaving the body unclosed. Move the defer before the read:

Suggested change
if err != nil {
h.Log.Error(err, "Reading request body")
http.Error(w, "Failed to read body", http.StatusBadRequest)
return
}
defer r.Body.Close()
body, err := io.ReadAll(r.Body)
if err != nil {
h.Log.Error(err, "Reading request body")
http.Error(w, "Failed to read body", http.StatusBadRequest)
return
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant