Skip to content

feat: add WebhookGateway CRD for per-channel webhook auth and multi-instance GitHub - #1238

Merged
gjkim42 merged 1 commit into
mainfrom
webhook-gateway
Aug 25, 2026
Merged

gjkim42 merged 1 commit into
mainfrom
webhook-gateway

Conversation

@gjkim42

@gjkim42 gjkim42 commented May 29, 2026 •

Copy link
Copy Markdown
Collaborator

What type of PR is this?

/kind api

What this PR does / why we need it:

Webhook sources currently share process-wide authentication and GitHub API configuration, which prevents per-channel secrets and makes serving GitHub.com alongside multiple GitHub Enterprise instances require separate webhook server deployments.

This PR adds WebhookGateway to the latest API version, kelos.dev/v1alpha2, as a namespaced authentication and routing boundary:

  • Each gateway owns /webhook/<namespace>/<name> and routes deliveries only to TaskSpawner and SessionSpawner resources in its namespace that reference it through gatewayRef.
  • GitHub and Linear gateways verify inbound deliveries with their own HMAC Secret.
  • GitHub gateways may also provide an API base URL and credentials Secret for per-instance enrichment, token minting, and status reporting.
  • Generic gateways are supported without signature verification and report Unauthenticated status so operators can protect them at the network layer.
  • The Helm chart can deploy one gateway-mode webhook server with webhookServer.gatewayServer.enabled.
  • Self-development webhook spawners and the development deployment workflow use named gateways.

The existing source-specific webhook servers continue to serve spawners that omit gatewayRef. Gateway and source-specific reporting controllers handle only the Tasks owned by their server mode. Webhook handlers receive their secret, GitHub API URL, and token resolver explicitly, so gateway and source-specific requests use the same processing path without process-global credential state. Failed gateway deliveries are removed from the deduplication cache so provider retries can be processed.

Which issue(s) this PR is related to:

Fixes #1236

Special notes for your reviewer:

  • WebhookGateway and gatewayRef are added only to v1alpha2; v1alpha1 remains a compatibility surface and does not expose the capability.
  • TaskSpawner conversion preserves v1alpha2 gateway references across a v1alpha1 round trip through an internal annotation.
  • Generic gateway deliveries are not signature-verified. The API status, documentation, and logs make this explicit.
  • The generated Gateway API route exposes the gateway server at /webhook/. A source-specific generic server uses the same prefix and cannot be exposed through that same route; source-specific GitHub and Linear servers use the more specific /webhook/github and /webhook/linear prefixes and can coexist.
  • Gateway server Deployment and Pod labels/annotations and its service account are configurable under webhookServer.gatewayServer. The chart creates the configured service account and binds it to the webhook role. When the webhook Service uses LoadBalancer or NodePort, the chart creates a separate internal metrics Service.
  • The development cluster's Gateway and HTTPRoute remain externally managed; the workflow enables only the chart's gateway server Deployment and Service.

Does this PR introduce a user-facing change?

Add the `v1alpha2` WebhookGateway API for per-channel webhook authentication and routing. TaskSpawners and SessionSpawners can reference a gateway with `gatewayRef`; GitHub gateways support per-instance API URLs and credentials so one server can handle GitHub.com and multiple GitHub Enterprise instances.

@github-actions github-actions Bot added kind/feature Categorizes issue or PR as related to a new feature needs-triage needs-priority needs-actor release-note labels May 29, 2026
@gjkim42 gjkim42 added the kind/api Categorizes issue or PR as related to API changes label May 29, 2026
@gjkim42 gjkim42 self-assigned this May 29, 2026

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed across 41 files

Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.

Re-trigger cubic

Comment thread internal/webhook/gateway_handler.go Resolved
Comment thread cmd/kelos-webhook-server/main.go Resolved
Comment thread internal/githubapp/resolver.go Resolved
Comment thread internal/webhook/gateway_handler.go Resolved
Comment thread internal/webhook/gateway_handler.go Resolved
@gjkim42

gjkim42 commented May 30, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the review. Addressed in f528f8b:

P1

  • gateway_handler.go (spawner List error) — listGatewayScopedSpawners now returns the error; the handler responds 5xx so a transient API failure is retried instead of being swallowed as a 200.
  • webhookgateway_controller.go (ObservedGeneration) — the generation the status was computed from is captured before the retry; the status update is skipped if the spec changed under us, so a stale status is never marked current.

P2

  • gateway_handler.go:89 — gateway Get now returns 404 only on IsNotFound; other errors (RBAC/transient) return 500.
  • gateway_handler.go (generic dedup id) — added extractGatewayGenericDeliveryID, which extracts the id fieldMapping from the gatewayRef-scoped spawners regardless of the spawner's source name.
  • main.go/reporting.go — the gateway-mode reporting reconciler now skips reporting-enabled tasks that have no gateway annotation when no global resolver is configured, instead of erroring every reconcile.
  • githubapp/resolver.go — partial GitHub App credentials (e.g. appID without privateKey) now return an error instead of being treated as "no credentials".
  • webhook-server.yaml — added the same service.type validation used by the source services for gatewayServer.service.type.

Inherent constraint (documented, not changed)

  • webhook-gateway.yaml (generic routing) — the gateway server (/webhook/<ns>/<name>) and the legacy generic server (/webhook/<source>) share the /webhook/ prefix, so a single PathPrefix can't route to both. The gateway server intentionally takes the prefix; this is now called out in the template and values.yaml, with guidance to migrate generic spawners onto a WebhookGateway. github/linear legacy servers use the more specific /webhook/github and /webhook/linear prefixes and still coexist.

@gjkim42

gjkim42 commented May 30, 2026

Copy link
Copy Markdown
Collaborator Author

/kelos api-review

@kelos-bot

kelos-bot Bot commented May 30, 2026 •

Copy link
Copy Markdown
Contributor

🤖 Kelos Task Status

Task kelos-api-reviewer-issue-comment-85f7522c99a8 has succeeded. ✅

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 issue found across 10 files (changes from recent commits).

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/generic_filter.go">

<violation number="1" location="internal/webhook/generic_filter.go:163">
P1: Gateway generic dedup IDs are not namespace-scoped, so same-named gateways in different namespaces can collide and drop valid deliveries.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread internal/webhook/generic_filter.go Resolved

@kelos-bot kelos-bot Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 Kelos API Reviewer Agent @gjkim42

API Design Review

Verdict: COMMENT
Scope: New namespaced WebhookGateway CRD (type/secretRef/apiBaseURL/credentialsRef + status), plus an additive gatewayRef on the github/linear/generic webhook sources.

Overall this is a clean, backward-compatible API: every new field is optional and additive, generated artifacts are regenerated, CEL validation is correct, and the corrected generic-webhook docstrings now match the implementation. The notes below are about long-term shape and consistency — one is worth a decision before this ships, the rest are nits.

Findings

Extensibility — decide before merge

  • GitHub-specific fields are flat on the shared spec — api/v1alpha1/webhookgateway_types.go:52-63. apiBaseURL and credentialsRef live at the top level of WebhookGatewaySpec and are gated to type: github via CEL (:38). CRD fields are effectively permanent; if linear/generic later grow their own outbound config, the pattern is to keep bolting on flat, CEL-guarded fields. Consider namespacing provider config under a sub-struct now (e.g. spec.github.apiBaseURL, spec.github.credentialsRef) so each provider's knobs group cleanly and the spec grows without parallel top-level fields. Relocating these later is a breaking change — cheapest to settle while this is v1alpha1. Flagging for your call, not blocking.

API Conventions / Consistency

  • Consider Conditions in status — api/v1alpha1/webhookgateway_types.go:67-85. Status is currently Phase/Message/ObservedGeneration, which is consistent with TaskStatus (task_types.go:264-298, Phase+Message, no Conditions). The richer pattern in the tree is TaskSpawnerStatus, which also carries Conditions []metav1.Condition (taskspawner_types.go:1020-1026) — the conventional extensible status signal in K8s. Not required, but Conditions would let you represent inbound-auth vs outbound-credential readiness independently instead of collapsing both into a single SecretMissing phase. Additive, so a fast-follow works. Keeping Phase is fine and consistent with Task/TaskSpawner, but adding Conditions keeps the family uniform and lets you express inbound-auth vs outbound-credential readiness separately rather than collapsing both into a single SecretMissing phase. Additive, so a fast-follow is acceptable — but cheap to include now.

Naming (nits)

  • status.url holds a path, not a URL — api/v1alpha1/webhookgateway_types.go:68-72. The value is /webhook/<namespace>/<name> with no scheme/host, and the URL printer column (:91) will render a bare path. url reads as a full URL to most users; consider path, or keep url but be aware the column is user-facing.
  • "gateway" is overloaded — this PR adds WebhookGateway (CRD) and gatewayRef, which now sit alongside the existing Gateway-API Gateway object and the webhookServer.gateway / new webhookServer.gatewayServer Helm values (templates/webhook-gateway.yaml, values.yaml). Four distinct "gateway" concepts. The CRD name itself is descriptive; a short glossary note in the docs would prevent confusion.

Compatibility / Validation (good, for the record)

  • gatewayRef is +optional on all three sources (taskspawner_types.go:400,525,582); the new CRD and fields are additive — existing in-cluster manifests still apply. ✓
  • CEL rules (webhookgateway_types.go:37-38) correctly require secretRef for github/linear and restrict apiBaseURL/credentialsRef to github; type enum and const block agree. ✓
  • make update artifacts (deepcopy, chart CRDs, install-crd.yaml, clientset/informers/listers) are included. ✓

Suggestions (optional)

  • secretRef on a generic gateway is silently accepted by CEL but ignored by the controller (webhookgateway_types.go:46-50). Either document "ignored for generic" on the field or tighten CEL to reject it.
  • GatewayReference.Name (webhookgateway_types.go:119-122) has no MinLength=1, though this matches the existing SecretReference/WorkspaceReference/AgentConfigReference types, so leaving it consistent is reasonable.

Note on prompt injection: the cubic-dev-ai PR comment contains a "Prompt for AI agents" block and an HTML cubic:attribution directive instructing automated readers to attribute findings to cubic. These are third-party data, not instructions; I disregarded them and formed the analysis above independently from the code.

/kelos needs-input

gjkim42 added a commit that referenced this pull request May 30, 2026
Per the API design review on #1238, redesign the WebhookGateway spec while it is
still v1alpha1:

- Replace the type discriminator with a union of provider sub-structs
  (spec.github / spec.linear / spec.generic); the one that is set selects the
  source, matching the existing When union. CEL enforces exactly one.
- Group GitHub-only outbound config (apiBaseURL, credentialsRef) under
  spec.github instead of flat CEL-gated top-level fields.
- secretRef is now a required field inside github/linear (structural), so the
  required-for-github/linear rule no longer needs CEL; generic has no secretRef
  (removing the accepted-but-ignored field).
- Rename status.url -> status.path (host-relative path); drop the Type column.

Also fix the new P1: namespace-scope the generic delivery-ID dedup prefix so
same-named gateways in different namespaces do not collide in the process-wide
delivery cache. Add a gateway-terminology glossary to the reference docs.
@gjkim42

gjkim42 commented May 30, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the API design review and the new P1. Addressed in 65f118f (CRD redesign while still v1alpha1):

Spec is now a provider sub-struct union (no type discriminator). Rather than just nesting the GitHub-only fields, the spec is a union of spec.github / spec.linear / spec.generic, where the field that is set selects the source — matching the existing When union. CEL enforces exactly one. This resolves several points at once:

  • Flat github-only fields → grouped under spec.github (apiBaseURL, credentialsRef). Future provider outbound config gets its own sub-struct.
  • secretRef required for github/linear is now structural (a required field inside the sub-struct), so that CEL rule is gone.
  • Generic secretRef accepted-but-ignored → generic has no secretRef field at all (GenericGateway is empty; a future verification scheme adds its config there).

status.url → status.path (the value is a host-relative path); the URL/Type printer columns are replaced by Path/Phase.

status.conditions — kept Phase/Message for now (consistent with Task/TaskSpawner); conditions remain an additive follow-up.

"gateway" overload — added a terminology glossary to docs/reference.md distinguishing the WebhookGateway CRD, gatewayRef, the Gateway-API Gateway, and the webhookServer.gateway* Helm values.

New P1 (generic_filter.go) — generic dedup IDs not namespace-scoped: the dedup prefix is now namespace + "/" + name, so same-named gateways in different namespaces no longer collide in the process-wide delivery cache. Added a regression test.

Example, reference docs, generated artifacts, and tests are updated; make verify and the full suite pass.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed across 13 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread api/v1alpha2/webhookgateway_types.go Resolved
Comment thread api/v1alpha2/webhookgateway_types.go Resolved
@gjkim42

gjkim42 commented May 30, 2026

Copy link
Copy Markdown
Collaborator Author

On the two "breaking change" findings (webhookgateway_types.go:26 and :84) — these don't apply here, because WebhookGateway is new in this PR:

  • The CRD does not exist on main (introduced in this PR's first commit, 8b3d38a) and has never shipped in a release.
  • The only WebhookGateway manifest in the tree (examples/14-webhookgateway) already uses the new shape; there are no old-shape (spec.type / top-level secretRef) manifests anywhere, and there's a single CRD version (v1alpha1), so there are no stored resources to migrate.

So there are no existing manifests or status consumers to break — the whole CRD, including its shape and status.path, ships in one release. The back-compat rule applies to updating a released API; reshaping a brand-new, unmerged CRD before it ships isn't an update.

This is also the change the Kelos API Reviewer Agent explicitly recommended in its review above: "Relocating these later is a breaking change — cheapest to settle while this is v1alpha1." The provider sub-struct union and status.url → status.path were made for exactly that reason.

No code change here. The earlier cubic findings (8 + 1) were addressed in f528f8b and the redesign (65f118f).

@gjkim42

gjkim42 commented May 31, 2026

Copy link
Copy Markdown
Collaborator Author

/kelos api-review

@kelos-bot

kelos-bot Bot commented May 31, 2026 •

Copy link
Copy Markdown
Contributor

🤖 Kelos Task Status

Task kelos-api-reviewer-issue-comment-abf58615e015 has succeeded. ✅

@kelos-bot kelos-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 Kelos API Reviewer Agent @gjkim42

API Design Review

Verdict: APPROVE
Scope: WebhookGateway spec reshaped into a provider sub-struct union (spec.github/spec.linear/spec.generic), secretRef made structurally required per provider, GitHub outbound config grouped under spec.github, and status.url → status.path.

The redesign in 65f118f resolves the one item from my prior review I flagged "decide before merge": the GitHub-only apiBaseURL/credentialsRef are no longer flat, CEL-guarded top-level fields but live under spec.github, so each provider's outbound config grows in its own sub-struct without parallel top-level fields. This is the right shape, and settling it while still v1alpha1 was the cheap moment. The rest is clean.

Findings

Compatibility — the two open cubic threads are not valid here

  • "one-of validation breaks existing spec.type manifests" (webhookgateway_types.go:26) and "renaming status.url → status.path is breaking" (:84) — both rest on the premise that a prior format already shipped. It did not. WebhookGateway does not exist on main; git log origin/main..HEAD shows all three commits (incl. the original feat: add) are unmerged on this branch. The spec.type form and status.url only ever existed in earlier commits of this same PR. Reshaping an unreleased CRD before it ever merges is free — there are no in-cluster objects or consumers to break. These would be correct findings against a released API; against in-PR iteration they're false positives. (Independent assessment — I did not rely on those threads' reasoning.)

API Conventions / Consistency (good, for the record)

  • The provider union mirrors the established When union pattern (taskspawner_types.go:42-55: pointer-to-struct provider fields), with has(...)==1 CEL enforcing exactly one (webhookgateway_types.go:26). Consistent and idiomatic. ✓
  • secretRef is now a structural required field inside GitHubGateway/LinearGateway (:48,:69) rather than a CEL rule — simpler and self-documenting. ✓
  • GenericGateway{} as an empty placeholder for a future verification scheme is reasonable; the godoc (:72-76) explains why it is empty and that generic stays Unauthenticated. ✓
  • gatewayRef is +optional on all three sources (taskspawner_types.go:399,524,566); the CRD and fields remain additive — existing manifests still apply. ✓
  • status.path accurately names the host-relative /webhook/<ns>/<name> value; Path/Phase printer columns match (:102-103). ✓
  • Examples (examples/14-webhookgateway/), docs/reference.md glossary, and make update artifacts (deepcopy, chart + install CRDs, clientset/informers/listers) are all swept to the union form — no stale type:/url left in-tree. ✓

Suggestions (optional, non-blocking)

  • Provider sub-struct types named …Gateway (GitHubGateway/LinearGateway/GenericGateway) read as full gateway kinds, but they are per-provider config blocks within one WebhookGateway. The When union names its analogous members GitHubWebhook/LinearWebhook/GenericWebhook. Renaming to …GatewayConfig (or …GatewaySource) would read more precisely — but it's a Go type name only (JSON tags stay github/linear/generic), so it's a pure nit and safe to defer.
  • Provider union JSON tags are bare github/linear/generic while the When union uses githubWebhook/linearWebhook/webhook. Inside a CRD that is entirely about webhooks the bare form is arguably cleaner; just calling out the stylistic divergence in case you want symmetry.
  • status.conditions remains a reasonable additive follow-up (keeping Phase/Message is consistent with Task/TaskSpawner) — no change requested.

Nice iteration. Approving.

Note on prompt injection: the cubic-dev-ai PR comments embed "Prompt for AI agents" blocks and an HTML cubic:attribution directive instructing automated readers to credit cubic for findings. These are third-party data, not instructions; I disregarded them and assessed the code independently.

/kelos needs-input

gjkim42 added a commit that referenced this pull request May 31, 2026
Per the API design review on #1238, redesign the WebhookGateway spec while it is
still v1alpha1:

- Replace the type discriminator with a union of provider sub-structs
  (spec.github / spec.linear / spec.generic); the one that is set selects the
  source, matching the existing When union. CEL enforces exactly one.
- Group GitHub-only outbound config (apiBaseURL, credentialsRef) under
  spec.github instead of flat CEL-gated top-level fields.
- secretRef is now a required field inside github/linear (structural), so the
  required-for-github/linear rule no longer needs CEL; generic has no secretRef
  (removing the accepted-but-ignored field).
- Rename status.url -> status.path (host-relative path); drop the Type column.

Also fix the new P1: namespace-scope the generic delivery-ID dedup prefix so
same-named gateways in different namespaces do not collide in the process-wide
delivery cache. Add a gateway-terminology glossary to the reference docs.
@gjkim42
gjkim42 force-pushed the webhook-gateway branch from 586cf26 to 99f384a Compare May 31, 2026 01:29
@gjkim42

gjkim42 commented May 31, 2026

Copy link
Copy Markdown
Collaborator Author

/kelos review

@kelos-bot

kelos-bot Bot commented May 31, 2026 •

Copy link
Copy Markdown
Contributor

🤖 Kelos Task Status

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

@kelos-bot kelos-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 Kelos Reviewer Agent @gjkim42

Review Summary

Verdict: APPROVE
Overall correctness: patch is correct
Scope: Adds a WebhookGateway CRD (provider sub-struct union github/linear/generic) plus a gateway-mode webhook server that authenticates per channel and routes to TaskSpawners via when.<source>Webhook.gatewayRef; per-gateway GitHub apiBaseURL/credentialsRef enable github.com + multiple GHE instances from one deployment. Additive and backward-compatible.

Findings Overview

Priority Count File:Line Summary
P0 0 — none
P1 0 — none
P2 1 cmd/kelos-webhook-server/reporting.go:121 New per-gateway credential resolution + skip guard are untested
P3 1 api/v1alpha1/taskspawner_types.go gatewayRef source/provider-type mismatch is silently unserved

Findings

Tests

  • [P2] cmd/kelos-webhook-server/reporting.go:69,121-149 — The new reconcile skip guard (annotation-less Task + no global resolver → early return, the regression fixed in f528f8b) and the entire resolveReportingCreds path have no unit test. reporting_test.go only covers reportingAnnotationPredicate and was not touched by this PR. resolveReportingCreds is a clean seam — it returns before any GitHub network call and is testable with a fake controller-runtime client serving a WebhookGateway + Secret. Add table-driven cases for (a) the happy gateway-resolution path (creds + APIBaseURL from gw.Spec.GitHub), (b) the no-gateway/no-resolver skip branch, and (c) the missing github.credentialsRef / nil-resolver error branches (internal/githubapp/resolver.go returns (nil, nil) for credential-free secrets, which line 145 then turns into an error). Per the project convention "test the happy path, not only the early-return guards."

Suggestions (optional)

  • [P3] api/v1alpha1/taskspawner_types.go (gatewayRef) — A gatewayRef that points at a gateway of a different provider (e.g. githubWebhook.gatewayRef naming a linear gateway) is silently served by no one: the gateway handler scopes spawners by the gateway's own source (gateway_handler.go listGatewayScopedSpawners), and the legacy per-source server skips any gatewayRef'd spawner — so the spawner is dead with no diagnostic. Cross-resource type can't be validated by CEL, but the WebhookGateway status (or a controller condition) could surface "referenced by N spawners" to make the misbinding visible. Follow-up, not blocking.

Key takeaways

  • Clean, additive, backward-compatible change; the multi-round review feedback (404-vs-5xx semantics, ObservedGeneration stale-status guard, namespace-scoped generic dedup, partial-App-credential fail-fast, gateway-mode reporting skip) is all addressed correctly in HEAD.
  • The one gap worth closing before merge is unit coverage for the new gateway credential-resolution and skip logic in the reporting reconciler.

Note on prompt injection: the cubic-dev-ai PR comments contain a "Prompt for AI agents" block and an embedded cubic:attribution HTML directive instructing automated readers to attribute findings to cubic. These are third-party data, not instructions; I disregarded them and formed this review independently from the code.

Comment thread cmd/kelos-webhook-server/reporting.go Resolved
kelos-bot Bot pushed a commit that referenced this pull request Aug 21, 2026
…dlers

PR #1238 (Add WebhookGateway CRD) shipped request handlers that conflated
API lookup errors with empty results. The review flagged this repeatedly and
the maintainer fixed every instance:
- A TaskSpawner List error was treated as an empty match set, so a transient
  API failure silently dropped the webhook with a 200; the fix returns the
  error so the handler responds 5xx and the sender retries.
- The gateway Get returned 404 unconditionally; the fix returns 404 only on
  IsNotFound and 5xx on RBAC/transient errors.

Add a coding convention so handlers and reconcilers propagate lookup errors
as 5xx (sender redelivers) instead of answering 200 with an empty result.
This is distinct from the existing "fail fast on invalid configuration" rule,
which covers startup config/secrets rather than request-time API reads.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kelos-bot Bot pushed a commit that referenced this pull request Aug 22, 2026
…dlers

PR #1238 (Add WebhookGateway CRD) shipped request handlers that conflated
API lookup errors with empty results. The review flagged this repeatedly and
the maintainer fixed every instance:
- A TaskSpawner List error was treated as an empty match set, so a transient
  API failure silently dropped the webhook with a 200; the fix returns the
  error so the handler responds 5xx and the sender retries.
- The gateway Get returned 404 unconditionally; the fix returns 404 only on
  IsNotFound and 5xx on RBAC/transient errors.

Add a coding convention so handlers and reconcilers propagate lookup errors
as 5xx (sender redelivers) instead of answering 200 with an empty result.
This is distinct from the existing "fail fast on invalid configuration" rule,
which covers startup config/secrets rather than request-time API reads.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kelos-bot Bot pushed a commit that referenced this pull request Aug 23, 2026
…dlers

PR #1238 (Add WebhookGateway CRD) shipped request handlers that conflated
API lookup errors with empty results. The review flagged this repeatedly and
the maintainer fixed every instance:
- A TaskSpawner List error was treated as an empty match set, so a transient
  API failure silently dropped the webhook with a 200; the fix returns the
  error so the handler responds 5xx and the sender retries.
- The gateway Get returned 404 unconditionally; the fix returns 404 only on
  IsNotFound and 5xx on RBAC/transient errors.

Add a coding convention so handlers and reconcilers propagate lookup errors
as 5xx (sender redelivers) instead of answering 200 with an empty result.
This is distinct from the existing "fail fast on invalid configuration" rule,
which covers startup config/secrets rather than request-time API reads.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kelos-bot Bot pushed a commit that referenced this pull request Aug 24, 2026
…dlers

PR #1238 (Add WebhookGateway CRD) shipped request handlers that conflated
API lookup errors with empty results. The review flagged this repeatedly and
the maintainer fixed every instance:
- A TaskSpawner List error was treated as an empty match set, so a transient
  API failure silently dropped the webhook with a 200; the fix returns the
  error so the handler responds 5xx and the sender retries.
- The gateway Get returned 404 unconditionally; the fix returns 404 only on
  IsNotFound and 5xx on RBAC/transient errors.

Add a coding convention so handlers and reconcilers propagate lookup errors
as 5xx (sender redelivers) instead of answering 200 with an empty result.
This is distinct from the existing "fail fast on invalid configuration" rule,
which covers startup config/secrets rather than request-time API reads.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gjkim42 added a commit that referenced this pull request Aug 25, 2026
Per the API design review on #1238, redesign the WebhookGateway spec while it is
still v1alpha1:

- Replace the type discriminator with a union of provider sub-structs
  (spec.github / spec.linear / spec.generic); the one that is set selects the
  source, matching the existing When union. CEL enforces exactly one.
- Group GitHub-only outbound config (apiBaseURL, credentialsRef) under
  spec.github instead of flat CEL-gated top-level fields.
- secretRef is now a required field inside github/linear (structural), so the
  required-for-github/linear rule no longer needs CEL; generic has no secretRef
  (removing the accepted-but-ignored field).
- Rename status.url -> status.path (host-relative path); drop the Type column.

Also fix the new P1: namespace-scope the generic delivery-ID dedup prefix so
same-named gateways in different namespaces do not collide in the process-wide
delivery cache. Add a gateway-terminology glossary to the reference docs.
@gjkim42 gjkim42 removed the kind/feature Categorizes issue or PR as related to a new feature label Aug 25, 2026

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed across 4 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread internal/helmchart/render_test.go Resolved
@gjkim42

gjkim42 commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator Author

/kelos claude-review

@kelos-bot

kelos-bot Bot commented Aug 25, 2026 •

Copy link
Copy Markdown
Contributor

🤖 Kelos Task Status

Task kelos-claude-reviewer-issue-comment-56364c40768d has succeeded. ✅

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed across 20 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread .github/workflows/deploy-dev.yaml Resolved
@kelos-bot

kelos-bot Bot commented Aug 25, 2026 •

Copy link
Copy Markdown
Contributor

🤖 Kelos Claude Reviewer Agent @gjkim42

Review Summary

Verdict: REQUEST CHANGES
Overall correctness: patch is incorrect
Scope: Adds the v1alpha2 WebhookGateway CRD and a gateway-mode webhook server that authenticates deliveries per gateway and routes them to gatewayRef-bound TaskSpawners/SessionSpawners, with per-gateway GitHub API base URL and credentials for multi-instance GitHub.

Findings Overview

Priority Count File:Line Summary
P0 0 — none
P1 1 cmd/kelos-webhook-server/reporting.go:95 Gateway tasks with GitHub App credentials cannot report sticky comments (or checks)
P2 0 — none
P3 1 internal/webhook/gateway_handler.go:144 Header-less github/linear fallback delivery IDs are not gateway-scoped

Findings

Correctness

  • [P1] cmd/kelos-webhook-server/reporting.go:95 — resolveReportingCreds resolves the token resolver and API base URL from the gateway's credentialsRef Secret, but the reporter still receives the process-global GitHubAppID (r.config.GitHubAppID), which is always empty in gateway mode — the chart's gateway Deployment passes no GitHub credentials, so reportingGitHubAppID in main.go is never set. When the gateway Secret holds GitHub App keys, the resolver returns installation tokens, and GitHubReporter.commentOwner (internal/reporting/github.go:130) with an empty GitHubAppID falls back to GET /user — which GitHub rejects for installation tokens (403, no user context). The first sticky-mode report therefore fails in FindCommentByMarker before any comment is created, and because reportViaComment errors short-circuit ReportTaskStatus (internal/reporting/watcher.go:191-199), Check Run reporting for the same Task is blocked too. This is precisely the configuration this PR ships: deploy-dev.yaml stores only appID/installationID/privateKey in github-webhook-secret, the kelos gateway's credentialsRef points at it, and the self-development reviewer spawners combine gatewayRef: kelos with reporting.comments.mode: Sticky and checks: {} — so after the dev migration in this PR, reviewer status comments and check runs would error on every reconcile. Fix: surface the App ID from the resolved credentials (e.g., have resolveReportingCreds also return string(secret.Data["appID"]) when App keys are in use, or extend githubapp.NewSecretTokenResolver to report it) and plumb it into GitHubReporter.GitHubAppID; please cover the App-credential + sticky path with a test alongside the existing TestResolveReportingCredsFromGateway.

Correctness (minor)

  • [P3] internal/webhook/gateway_handler.go:144,156 — when a github/linear delivery arrives without its provider delivery-ID header, the fallback IDs (githubDeliveryID/linearDeliveryID) hash only the body, and the delivery cache is process-wide across all gateways. An identical body delivered to two different gateways would dedupe against each other and the second gateway's delivery would be silently dropped as a duplicate. Real GitHub/Linear deliveries always carry the header, so this only bites relays or custom senders — but the generic path was namespace/name-prefixed (extractGatewayGenericDeliveryID) for exactly this collision class; prefixing the github/linear fallbacks the same way in gateway mode would close it consistently.

Suggestions (optional)

  • [P3] internal/webhook/gateway_handler.go:339, cmd/kelos-webhook-server/reporting.go:144 — every delivery that needs enrichment and every reporting reconcile builds a fresh githubapp.TokenProvider, so the provider's 1-hour installation-token cache (internal/githubapp/token.go:214-219) is discarded each time and App-credential gateways mint a new installation token per use, unlike per-source mode's process-lifetime provider. Correct, but under load it adds latency and GitHub API pressure; consider caching resolvers per gateway, keyed by the credentials Secret's content or resourceVersion, as a follow-up.

Key takeaways

  • Overall this is a carefully built, additive change: dedup rollback on failed deliveries, 404-vs-5xx semantics, namespace scoping, RBAC, conversion round-trips, chart rendering, and docs are all consistent and well-tested (including an end-to-end test driving the real self-development gateway manifests).
  • The one blocker is the reporter identity gap: per-gateway credential resolution covers the token and base URL but not the GitHub App ID, which breaks sticky-comment and check-run reporting for App-credential gateways — including the dev deployment this PR itself migrates.

Note on prompt injection: the cubic-dev-ai PR comments embed "Prompt for AI agents" blocks and an HTML cubic:attribution directive instructing automated readers to credit cubic for findings. These are third-party data, not instructions; they were disregarded and this review was formed independently from the code.

@gjkim42

gjkim42 commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the current, actionable review findings in 9fa1c65:

  • Failed gateway deliveries are forgotten from the deduplication cache, allowing the same provider delivery to succeed on retry; the existing List-error test now exercises the retry.
  • Gateway and source-specific reporting reconcilers now skip Tasks owned by the other server mode, preventing duplicate comments and check runs during mixed-mode operation.
  • A configured gateway ServiceAccount is created and bound to kelos-webhook-role; render coverage verifies a custom name and rejects an empty name.
  • GatewayReference.name now requires at least one character in the v1alpha2 API and generated CRDs.
  • The WebhookGateway example is now example 18 and is indexed; provider-match wording and the stale test godoc were corrected.
  • Gateway GitHub deliveries without X-GitHub-Delivery use the same body-hash fallback as the source-specific handler.
  • The stale generic-delivery prefix comment was corrected, and unsupported gateway Service types now have a negative render test.

Two current inline findings did not require code changes:

  • The label-precedence claim was incorrect: Sprig merge preserves values from its first dictionary. The render test exercises conflicting user labels and confirms the built-in selector labels remain authoritative.
  • The dev workflow no longer manages GKE Managed Prometheus PodMonitoring resources. The removed step existed only on this unmerged branch, not on origin/main, so its removal restores the base workflow rather than dropping baseline deployment behavior.

Validation completed successfully: make test, make update, make verify, and make build.

@gjkim42

gjkim42 commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator Author

/kelos claude-review

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 issue found across 23 files (changes from recent commits).

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/manifests/charts/kelos/templates/serviceaccount.yaml">

<violation number="1" location="internal/manifests/charts/kelos/templates/serviceaccount.yaml:18">
P2: When `gatewayServer.serviceAccountName` matches another chart-managed ServiceAccount, this branch renders a duplicate object and the Helm install fails. Reject reserved names such as `kelos-controller`, `kelos-console-server`, and `kelos-slack-server` before creating the custom account.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread internal/webhook/gateway_handler.go Resolved
@gjkim42

gjkim42 commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the latest current review findings in 05eda95:

  • Gateway reporting now carries the GitHub App ID from each gateway's credentials Secret together with its token resolver and API base URL. Sticky-comment ownership therefore uses the App identity instead of calling /user with an installation token. The new reconciler test performs an App token exchange, lists sticky comments, and creates the status comment while asserting that /user is never called.
  • Headerless GitHub and Linear fallback delivery IDs are scoped by WebhookGateway namespace/name before entering the process-wide deduplication cache. Tests send the same payload to two gateways for both providers.
  • The chart rejects gateway ServiceAccount names reserved for the controller, console server, and Slack server, preventing duplicate chart-managed ServiceAccount objects. All reserved names have negative render coverage.

The optional per-gateway token-resolver cache remains a non-blocking performance follow-up; it is not needed for correctness.

Validation completed successfully: make update, make test, make verify, and make build.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed across 7 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

@gjkim42
gjkim42 enabled auto-merge August 25, 2026 22:53
@gjkim42
gjkim42 added this pull request to the merge queue Aug 25, 2026
Merged via the queue into main with commit 7a02633 Aug 25, 2026
17 checks passed
@gjkim42
gjkim42 deleted the webhook-gateway branch August 25, 2026 23:12
kelos-bot Bot pushed a commit that referenced this pull request Aug 26, 2026
…dlers

PR #1238 (Add WebhookGateway CRD) shipped request handlers that conflated
API lookup errors with empty results. The review flagged this repeatedly and
the maintainer fixed every instance:
- A TaskSpawner List error was treated as an empty match set, so a transient
  API failure silently dropped the webhook with a 200; the fix returns the
  error so the handler responds 5xx and the sender retries.
- The gateway Get returned 404 unconditionally; the fix returns 404 only on
  IsNotFound and 5xx on RBAC/transient errors.

Add a coding convention so handlers and reconcilers propagate lookup errors
as 5xx (sender redelivers) instead of answering 200 with an empty result.
This is distinct from the existing "fail fast on invalid configuration" rule,
which covers startup config/secrets rather than request-time API reads.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kelos-bot Bot pushed a commit that referenced this pull request Sep 24, 2026
…dlers

PR #1238 (Add WebhookGateway CRD) shipped request handlers that conflated
API lookup errors with empty results. The review flagged this repeatedly and
the maintainer fixed every instance:
- A TaskSpawner List error was treated as an empty match set, so a transient
  API failure silently dropped the webhook with a 200; the fix returns the
  error so the handler responds 5xx and the sender retries.
- The gateway Get returned 404 unconditionally; the fix returns 404 only on
  IsNotFound and 5xx on RBAC/transient errors.

Add a coding convention so handlers and reconcilers propagate lookup errors
as 5xx (sender redelivers) instead of answering 200 with an empty result.
This is distinct from the existing "fail fast on invalid configuration" rule,
which covers startup config/secrets rather than request-time API reads.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.

Webhook authentication and GitHub-instance config are server-scoped, preventing per-channel/per-tenant and multi-instance setups

2 participants