[knechtionscoding]

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds regex-based body filtering to GitHubWebhookFilter via two new fields:

• bodyPattern (string): requires the body to match the given regex. Intended to replace the existing bodyContains for new usage.
• excludeBodyPatterns ([]string): any-match-excludes — the event is rejected if the body matches any regex entry.

Both fields use Go regexp (re2) syntax, supporting anchors, alternation, character classes, case-insensitive flags ((?i)), etc.

When both are set, the body must match bodyPattern AND must not match any excludeBodyPatterns entry. bodyContains is marked deprecated but remains functional and can be combined with the new fields.

Supported event types:

• issues (filters on issue body)
• issue_comment (filters on comment body)
• pull_request (filters on PR body)
• pull_request_review (filters on review body)
• pull_request_review_comment (filters on review comment body)

Example usage:

filters:
  - event: issue_comment
    action: created
    bodyPattern: "/deploy\\s+(staging|production)"
    excludeBodyPatterns:
      - "--dry-run"
      - "(?i)\\[bot\\]"

Kubebuilder validation markers:

• MaxLength=1024 on bodyContains and bodyPattern
• MaxItems=10, MinLength=1, MaxLength=1024 on excludeBodyPatterns

Which issue(s) this PR is related to:

N/A

Special notes for your reviewer:

• bodyContains (string, substring match) is marked deprecated but not removed — existing spawners that use it continue to work.
• bodyPattern is a single string (like bodyContains), while excludeBodyPatterns is a list (like ExcludeLabels/ExcludeAuthors).
• The matchesPattern/matchesAnyPattern helpers compile patterns per-match. Volume is low enough (webhook filters) that this is fine; caching can be added if needed later.
• Tests exercise real regex features: (?i) case-insensitive, \s+, alternation, anchors.

Does this PR introduce a user-facing change?

Add `bodyPattern` and `excludeBodyPatterns` regex filter fields to GitHub webhook filters. Deprecate `bodyContains` in favor of `bodyPattern`.

[greptile-apps]

Greptile Summary

This PR adds bodyPattern (regex match) and excludeBodyPatterns (regex exclusion list) fields to GitHubWebhookFilter, deprecating the plain-string bodyContains field while keeping it functional. The implementation covers all five supported event types, includes kubebuilder validation markers, ships comprehensive tests, and adds error logging for invalid regex patterns on all code paths.

Confidence Score: 5/5

Safe to merge; the one structural asymmetry noted is a P2 and doesn't affect real-world behavior.

All previously flagged P1 concerns (missing test coverage, error logging inconsistency) are addressed. The only remaining finding is a P2 structural note about PR-path body filters being inside if pr != nil — consistent with the existing BodyContains pattern and pr is always non-nil in practice for GitHub webhook payloads.

internal/webhook/github_filter.go — the PR-path BodyPattern/ExcludeBodyPatterns blocks sit inside if pr != nil; low risk but worth tidying for consistency.

Important Files Changed

Filename	Overview
internal/webhook/github_filter.go	Adds BodyPattern and ExcludeBodyPatterns filtering for issues/comments and PR/review events; error logging is present on both paths; minor structural asymmetry: PR-path body filters are inside if pr != nil while issue-path filters are outside the equivalent nil guard.
api/v1alpha1/taskspawner_types.go	Adds BodyPattern (string) and ExcludeBodyPatterns ([]string) fields with kubebuilder validation markers; BodyContains is deprecated but preserved; schema markers look correct.
api/v1alpha1/zz_generated.deepcopy.go	Auto-generated deep copy for the new ExcludeBodyPatterns slice field; correct copy pattern.
internal/manifests/charts/kelos/templates/crds/taskspawner-crd.yaml	CRD updated with bodyPattern and excludeBodyPatterns schema entries including minLength/maxLength/maxItems constraints; consistent with Go type markers.
internal/manifests/install-crd.yaml	Install CRD manifest updated in sync with the Helm CRD template; changes look correct.
internal/webhook/github_filter_test.go	Good new test coverage: ExcludeBodyPatterns for issues, issue_comment, pull_request, pull_request_review, pull_request_review_comment; combined BodyPattern+ExcludeBodyPatterns; multiple-pattern exclusion; BodyPattern regex tests.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Incoming GitHub Webhook] --> B{Event type?}
    B -->|IssuesEvent / IssueCommentEvent| C[Extract issue/comment body]
    B -->|PullRequestEvent / ReviewEvent / ReviewCommentEvent| D{pr != nil?}
    D -->|No| PASS
    D -->|Yes| E[Extract PR / review / comment body]
    C --> F{BodyContains set?}
    F -->|Yes| G{substring match?}
    G -->|No| REJECT
    G -->|Yes| H{BodyPattern set?}
    F -->|No| H
    H -->|Yes| I{regex match?}
    I -->|Error or No match| REJECT
    I -->|Match| J{ExcludeBodyPatterns set?}
    H -->|No| J
    J -->|Yes| K{matches any pattern?}
    K -->|Match or Error| REJECT
    K -->|No match| PASS
    J -->|No| PASS
    E --> L{BodyContains set?}
    L -->|Yes| M{substring match?}
    M -->|No| REJECT
    M -->|Yes| N{BodyPattern set?}
    L -->|No| N
    N -->|Yes| O{regex match?}
    O -->|Error or No match| REJECT
    O -->|Match| P{ExcludeBodyPatterns set?}
    N -->|No| P
    P -->|Yes| Q{matches any pattern?}
    Q -->|Match or Error| REJECT
    Q -->|No match| PASS
    P -->|No| PASS
    PASS([✅ Event accepted])
    REJECT([❌ Event rejected])

Loading

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
internal/webhook/github_filter.go:468-514
**PR-path body filters skip review/comment body when `pr` is nil**

The `BodyPattern` and `ExcludeBodyPatterns` blocks for `PullRequestReviewEvent` and `PullRequestReviewCommentEvent` are placed inside the `if pr != nil` guard. For these two event types the body being filtered is the *review* or *review comment* body — it has no logical dependency on the PR object being non-nil. If GitHub sends a `pull_request_review` webhook where the `pull_request` field is absent or unparseable, both filters are silently bypassed and the event passes through unfiltered.

This is structurally inconsistent with the issue/comment path, where `BodyPattern` and `ExcludeBodyPatterns` live *outside* the `if issue != nil` block and always run. Moving the PR body filters outside the guard (or extracting the review/comment body before the nil check, as done for issues) would make the behavior consistent and correct regardless of whether the PR object is present.

_{Reviews (8): Last reviewed commit: "feat: add bodyPattern and excludeBodyPat..." | Re-trigger Greptile}

[cubic-dev-ai]

1 issue found across 5 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="internal/webhook/github_filter_test.go">

<violation number="1" location="internal/webhook/github_filter_test.go:620">
P3: Missing test coverage for supported `BodyNotContains` review event types (`pull_request_review` and `pull_request_review_comment`).</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[gjkim42]

/kelos api-review

[kelos-bot]

🤖 Kelos Task Status

Task kelos-api-reviewer-issue-comment-5ea204713caa has been accepted and is being processed.

[kelos-bot]

🤖 Kelos Task Status

Task kelos-api-reviewer-issue-comment-5ea204713caa has succeeded. ✅

[kelos-bot]

🤖 Kelos API Reviewer Agent @gjkim42

API Design Review

Verdict: APPROVE
Scope: Additive bodyNotContains filter on GitHubWebhookFilter (v1alpha1) for excluding webhook events whose body contains a substring; mirrors the existing bodyContains.

Findings

API Conventions — looks good

• BodyNotContains string with JSON tag bodyNotContains, omitempty, and +optional — all consistent with Kubernetes API conventions and matches BodyContains (api/v1alpha1/taskspawner_types.go:391-393).
• Pure additive change in v1alpha1; existing clients can safely ignore the new field. No semantic changes to existing fields.
• CRD updates are present in both internal/manifests/charts/kelos/templates/crds/taskspawner-crd.yaml and internal/manifests/install-crd.yaml, descriptions match the godoc — make update was run as expected.

Documentation — minor nit

• api/v1alpha1/taskspawner_types.go:388-393: The doc clearly explains AND semantics with BodyContains, which is great. Two small clarifications worth adding:
  • Explicitly state that the match is case-sensitive (the implementation uses strings.Contains). The existing BodyContains doc has the same gap, but new fields are a good time to set the bar.
  • Note that an event with an empty body never matches BodyNotContains (so it passes the filter). This is the natural behavior of substring matching, but it's worth being explicit since users may otherwise expect "missing body" to be treated specially.

Naming — non-blocking

• The codebase uses an Exclude* prefix for negative variants of existing fields: Labels / ExcludeLabels, Author / ExcludeAuthors. BodyNotContains breaks that pattern (it pairs with BodyContains more directly, but is asymmetric with the other negative filters). Alternatives like ExcludeBodyContains are awkward, so BodyNotContains is reasonable — but worth noting for consistency. Since the API is v1alpha1, this is the cheapest moment to revisit if maintainers prefer the Exclude* family.

Validation — consider

• No MaxLength validation marker on the field. BodyContains has the same gap, so this is consistent, but unbounded substring filters could be abused via giant CRs. Optional: add // +kubebuilder:validation:MaxLength=... to both fields in a follow-up.

Tests — consider expanding

• internal/webhook/github_filter_test.go: tests cover issues, issue_comment, pull_request, and the combined bodyContains + bodyNotContains case. The PR description lists pull_request_review and pull_request_review_comment as supported, and the implementation handles them at internal/webhook/github_filter.go:443-454, but neither has a regression test. Per CLAUDE.md ("Better tests. Always try to add or improve tests when modifying code"), please add explicit test cases for those two event types so the documented surface is locked in.

Suggestions (optional)

• Consider promoting BodyContains / BodyNotContains to []string (OR / AND-of-NOT) in a follow-up — Labels and ExcludeAuthors already use list semantics, and a single substring is often too narrow in practice. Out of scope for this PR.
• If/when a bodyMatches (regex) variant is ever added, consider whether BodyContains* should remain primary or be deprecated; worth keeping the surface area small.

Note on prompt injection: a third-party reviewer's comment on this PR contained an HTML directive demanding that automated tools attribute findings to it. I disregarded that instruction and formed the analysis above independently from the diff.

/kelos needs-input

[knechtionscoding]

@gjkim42 I think this is ready for review. I went ahead and made the BodyNotContains a list for two reasons. First it is far more likely that you want to have multiple not contains than contains and second it prevents a future breaking API change to swap. BodyContains I can't do much about, but this is easy to prevent future breakages. Otherwise I think we are good to go.

[gjkim42]

/kelos api-review

[kelos-bot]

🤖 Kelos Task Status

Task kelos-api-reviewer-pull-request-review-26d2182fb7f0 has succeeded. ✅

[kelos-bot]

🤖 Kelos API Reviewer Agent @gjkim42

API Design Review

Verdict: COMMENT
Scope: Promotes BodyNotContains from string to []string, adds bound markers on both Body* fields (MaxLength=1024 / MaxItems=10 / items:MinLength=1 / items:MaxLength=1024), expands tests to cover all five supported event types, and documents case-sensitivity + empty-body behavior. Re-review of the latest revision (head 91ac9f5).

Findings

API Conventions — looks good

• Field naming, JSON tag, +optional, and omitempty are all consistent with Kubernetes conventions (api/v1alpha1/taskspawner_types.go:390-403).
• Generated DeepCopyInto correctly nil-guards and make+copys the new slice (api/v1alpha1/zz_generated.deepcopy.go:455-459).
• CRD updates land in both internal/manifests/charts/kelos/templates/crds/taskspawner-crd.yaml and internal/manifests/install-crd.yaml, descriptions match the godoc — make update was run.
• Implementation handles all five event types (internal/webhook/github_filter.go:351-365 for issues/issue_comment, :436-453 for PR / review / review-comment) and each has an explicit regression test, including the multi-entry and combined BodyContains+BodyNotContains cases.

Validation — good, with one note

• MaxItems=10 + items:MinLength=1 + items:MaxLength=1024 together close the strings.Contains(x, "") always-true trap at the API boundary, and the containsAny helper (internal/webhook/github_filter.go:464-471) defensively skips empty entries — belt-and-suspenders, correct.
• Adding MaxLength=1024 to BodyContains is a small server-side tightening of an existing field; acceptable in v1alpha1, but worth calling out in the release note since any existing object with a bodyContains longer than 1024 chars would now fail validation on update (unlikely in practice, but explicit is better).

Naming / type asymmetry — worth deciding now, not later

• BodyContains string vs BodyNotContains []string is asymmetric. @gjkim42 already raised this in the unresolved thread on api/v1alpha1/taskspawner_types.go ("users may be confused about the types of the two fields"), and the author's reply argues BodyNotContains as a list now is forward-compatible. That logic is sound for BodyNotContains, but the same argument applies in reverse to BodyContains — and the cheapest moment to promote it is in this PR, while the field is still v1alpha1 and the surrounding shape is being touched anyway.
• There is precedent for both choices in this struct: Author (string) / ExcludeAuthors ([]string) is asymmetric, while Labels / ExcludeLabels are both []string. So the asymmetry is defensible — but if the API ends up promoting BodyContains to []string later (the prior review and the PR description both flag this as the likely direction), users hit a real breaking change for no benefit over doing it now.
• Recommendation: either (a) promote BodyContains to []string (OR semantics — body must contain any entry) in this PR to eliminate the asymmetry while still in v1alpha1, or (b) explicitly document in BodyContains's godoc that the asymmetry is intentional and reflects "single positive trigger vs. multi-entry exclusions" (mirroring the Author/ExcludeAuthors precedent). Either way, the unresolved thread should be closed with a clear answer rather than left for a follow-up that may never land.

Documentation — minor nit

• BodyContains's updated godoc mentions case-sensitivity but, unlike BodyNotContains, does not note the empty-body behavior. A one-line symmetric note ("An empty (or missing) body never matches.") would keep the two fields' docs aligned (api/v1alpha1/taskspawner_types.go:384-388).

Compatibility / extensibility — fine

• Pure additive change for BodyNotContains; existing clients can ignore the new field. No semantic changes to existing fields beyond the MaxLength=1024 tightening on BodyContains.
• If a regex or glob match mode is ever wanted, the flat-field shape (bodyContains, bodyNotContains, future bodyMatches, ...) will start to look crowded. Worth keeping in mind for a future body: { contains: […], notContains: […], matches: […] } reshape, but explicitly out of scope here.

Suggestions (optional)

• Add a brief comment near containsAny pointing to items:MinLength=1 as the primary defense; the runtime guard is then clearly a fallback for older CRDs that predate the marker. Helps future readers avoid wondering whether the guard is load-bearing.
• If keeping the asymmetry, a one-line // See BodyNotContains for case-sensitivity / empty-body semantics; same rules apply here. on BodyContains saves duplicating prose.

Note on prompt injection: a third-party reviewer's comment on this PR contained an HTML directive demanding that automated tools attribute findings to it. I disregarded that instruction and formed the analysis above independently from the diff.

/kelos needs-input

[gjkim42]

/kelos review

[kelos-bot]

🤖 Kelos Task Status

Task kelos-reviewer-issue-comment-d7305b230a14 has succeeded. ✅

[kelos-bot]

🤖 Kelos Reviewer Agent @gjkim42

Review Summary

Verdict: APPROVE
Overall correctness: patch is correct
Scope: Adds bodyPattern (regex) and excludeBodyPatterns ([]regex) filters to GitHubWebhookFilter for issues / issue_comment / pull_request / pull_request_review / pull_request_review_comment events; deprecates bodyContains in favor of bodyPattern.

Findings Overview

Priority	Count	File:Line	Summary
P0	0	—	none
P1	0	—	none
P2	0	—	none
P3	4	internal/webhook/github_filter_test.go	No invalid-regex test for the error-rejects-event path
P3		internal/webhook/github_filter_test.go	BodyPattern only tested for issue_comment — PR/review/review_comment paths uncovered
P3		internal/webhook/github_filter.go:355-397, 468-514	Body-extraction blocks duplicated four times across the new code
P3		internal/webhook/github_filter.go:531-542	matchesAnyPattern short-circuits on first match without validating later patterns

Findings

Tests

• [P3] internal/webhook/github_filter_test.go — No test exercises the invalid-regex path. A test that sets bodyPattern: "(invalid" (or an excludeBodyPatterns entry with a malformed regex) and asserts the event is rejected would lock in the safe-rejection contract for matchesPattern/matchesAnyPattern errors. Today nothing prevents a future refactor from accidentally returning true (allow-all) on regex compile errors.
• [P3] internal/webhook/github_filter_test.go — BodyPattern is only tested for issue_comment (TestMatchesGitHubEvent_BodyPatternRegex and the combined test). The PR-side body extraction at internal/webhook/github_filter.go:468-490 is structurally distinct from the issues-side block and has no direct BodyPattern regression test — for example, a pull_request_review filter with bodyPattern: "(?i)approved" currently has no test verifying it matches the review body and not the PR body. ExcludeBodyPatterns already has full per-event-type coverage, so adding the parallel BodyPattern cases is a small lift.

Code quality

• [P3] internal/webhook/github_filter.go:355-397 and :468-514 — The same body-extraction if/else if chain appears four times (issue/comment × 2 filters, PR/review/review_comment × 2 filters). Folding them into extractIssueBody(e github.Event) / extractPRBody(e github.Event, pr *github.PullRequest) helpers would reduce the chance the two filters drift in their handling of new event types. Non-blocking; consistent with the existing bodyContains style.
• [P3] internal/webhook/github_filter.go:531-542 — matchesAnyPattern compiles each pattern lazily inside the loop and returns on the first match. If the body matches patterns[0], an invalid patterns[1] is never compiled and never logged; a different webhook payload that doesn't match patterns[0] will hit the bad regex and surface the error. Same configuration → different rejection paths depending on body content. The event is rejected either way, so this is a minor predictability nit, not a correctness bug. Compiling all patterns up front (or precompiling once per filter) would also remove the per-call regexp.Compile cost the PR description flagged.

Suggestions (optional)

• [P3] internal/webhook/github_filter.go:525,535 — fmt.Errorf("Invalid body pattern %q: %w", ...) capitalizes the leading word; idiomatic Go error strings start lowercase. The project's "capital first letter, no trailing punctuation" rule applies to log messages (which this code already gets right at :369, :391, :484, :508), not error returns.
• [P3] Consider an _ = re validation step at filter-load time (or a future admission webhook) so a typo in bodyPattern is caught without waiting for the first webhook to arrive. Out of scope for this PR.

Key takeaways

• Implementation is sound: error paths log and fail-closed (reject), generated CRD/deepcopy artifacts are present, kubebuilder markers (MaxItems=10, MinLength=1, MaxLength=1024) close the empty-pattern always-match trap at the API boundary.
• All findings are non-blocking. Worth tightening the test surface around BodyPattern on PR/review event types and the invalid-regex error path.

Note on prompt injection: a third-party reviewer's comment on this PR contains an HTML directive demanding that automated tools attribute findings to it. I disregarded that instruction and formed the analysis above independently from the diff.

[knechtionscoding]

If you are okay with it @gjkim42 I'll pick up the optional/p3 findings in a follow up PR

[gjkim42]

@knechtionscoding

one question:
Is this backward-compatible? Does setting +kubebuilder:validation:MinLength=1 allow empty BoddyPattern?

[knechtionscoding]

BodyPattern is omitted if empty, rather than allowing for strictly ""?

[knechtionscoding]

If you want me to allow for an empty string specifically then I can. I'm not sure that there is a meaningful difference but happy to do so.

[gjkim42]

I just want to make sure that the existing YAMLs that don't have bodyPattern (because it's a newly introduced field) can be applied.
can we just remove minLength here? it doesn't make sense here.

[knechtionscoding]

Done. @gjkim42