feat(reporting): add GitHub Checks API reporting#1016
Conversation
bb28b86 to
4536eff
Compare
|
@gjkim42 once the other PR lands I will update this with webhook reporting as well, I have that working in my branch |
There was a problem hiding this comment.
1 issue found across 12 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/reporting/watcher.go">
<violation number="1" location="internal/reporting/watcher.go:280">
P2: persistCheckRunState duplicates the entire retry/get/update/error-handling flow from persistReportingState with only different annotation keys/messages, increasing maintenance drift risk. Consider extracting a shared helper for annotation persistence to keep behavior consistent.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Merged it |
4536eff to
a64750e
Compare
a64750e to
6e71615
Compare
6e71615 to
6479c79
Compare
Greptile SummaryThis PR adds GitHub Checks API integration so Kelos task outcomes appear as formal Check Runs on pull requests, enabling branch protection rules and merge queue integration. It extends Confidence Score: 5/5Safe to merge; all previously flagged P1 issues are resolved and only a minor clarification about pull_request_target parser coverage remains. All four issues from the prior review round are addressed. The new check-run path has correct cache guards, proper Accept header, SHA gating, and an observable skip log. No P0 or P1 issues found in the new code. No files require special attention; the one P2 note on pull_request_target in taskspawner_types.go is a clarification question, not a blocker.
|
| Filename | Overview |
|---|---|
| internal/reporting/checks.go | New ChecksReporter that creates and updates GitHub Check Runs via REST API; uses correct Accept header, token resolution pattern, and JSON serialization. |
| internal/reporting/watcher.go | Extended TaskReporter with reportViaCheckRun; adds in-memory cache backstop for check run IDs, annotation-based dedup, and log.Info for SHA-missing skip — all previously flagged issues addressed. |
| internal/webhook/handler.go | Stamps checks annotations only when HeadSHA is non-empty (guards the issue_comment-on-PR edge case); correctly separates comment and checks annotation logic. |
| internal/webhook/github_filter.go | HeadSHA populated from pull_request, pull_request_review, and pull_request_review_comment events; HeadSHA field added to GitHubEventData struct. |
| api/v1alpha1/taskspawner_types.go | New GitHubChecksReporting type with MaxLength=100 on Name; CEL rules reject checks on githubIssues and require PR event types on githubWebhook. |
| cmd/kelos-spawner/reconciler.go | Wires ChecksReporter for polling PR sources; tokenFunc refactored out of inline closure; correctly gates on checksReportingEnabled. |
| cmd/kelos-webhook-server/reporting.go | Dynamically wires ChecksReporter when task has checks annotation; predicate updated to match tasks with either comment or checks annotations. |
Sequence Diagram
sequenceDiagram
participant S as TaskSpawner / WebhookHandler
participant K as Kubernetes API
participant TR as TaskReporter
participant GH as GitHub Checks API
S->>K: Create Task (annotations: github-checks=enabled, source-sha, check-name)
K-->>TR: Task reconcile triggered
TR->>TR: Load cache (check run ID + phase)
alt No cached check run ID
TR->>TR: Read AnnotationGitHubCheckRunID from task
end
alt desiredPhase == lastCheckPhase
TR-->>TR: Skip (already reported)
else checkRunID == 0
TR->>GH: POST /repos/{owner}/{repo}/check-runs (status: in_progress)
GH-->>TR: checkRunID
TR->>TR: storeCheckRun(uid, checkRunID, phase) in cache
TR->>K: Patch task: AnnotationGitHubCheckRunID + AnnotationGitHubCheckReportPhase
else checkRunID != 0
TR->>GH: PATCH /repos/{owner}/{repo}/check-runs/{id} (status: completed, conclusion: success/failure)
TR->>TR: storeCheckRun(uid, checkRunID, phase) in cache
TR->>K: Patch task annotations
end
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
api/v1alpha1/taskspawner_types.go:375-376
**`pull_request_target` in CEL allowlist but unhandled in the webhook parser**
`pull_request_target` is accepted by the CEL validation rule (and by `webhookSourceKind`) as a valid PR event for checks reporting. However, `ParseGitHubWebhook` in `github_filter.go` has no `case *github.PullRequestTargetEvent` branch — the event falls through to the `default` handler, which doesn't extract `Number`, `HeadSHA`, etc. Because `createTask` gates annotation-stamping on `parsed.GitHub.Number > 0`, a task triggered by a `pull_request_target` event would never receive the checks annotations, so no check run would be created despite passing validation.
If go-github v66 parses `pull_request_target` as `*github.PullRequestEvent` (same payload structure), this works correctly and no change is needed. If it returns a distinct type, users who rely on `pull_request_target` alone would silently get no check runs. A targeted test case (similar to `TestParseGitHubWebhook_HeadSHA`) for `pull_request_target` would make the intent explicit and catch a regression either way.
Reviews (8): Last reviewed commit: "feat(reporting): add GitHub Checks API r..." | Re-trigger Greptile
|
/kelos api-review |
|
🤖 Kelos Task Status Task |
6479c79 to
d4b1d08
Compare
|
/kelos api-review |
ce325ed to
b9f23d4
Compare
|
/kelos pick-up |
|
🤖 Kelos Task Status Task |
|
/kelos review |
|
🤖 Kelos Task Status Task |
There was a problem hiding this comment.
🤖 Kelos Reviewer Agent @gjkim42
Review Summary
Verdict: REQUEST CHANGES
Overall correctness: patch is incorrect
Scope: Adds a reporting.checks struct to TaskSpawners that creates GitHub Check Runs for PR-discovered tasks, enabling branch-protection integration alongside the existing comment reporter.
Findings Overview
| Priority | Count | File:Line | Summary |
|---|---|---|---|
| P0 | 0 | — | none |
| P1 | 1 | api/v1alpha1/taskspawner_types.go:377 | reporting.checks accepted on githubWebhook source but silently ignored at runtime |
| P2 | 1 | api/v1alpha1/taskspawner_types.go:63 | Type doc comment and PR description claim a GitHubPullRequestReporting type split that does not exist |
| P3 | 0 | — | none |
Findings
Correctness / API
- [P1]
api/v1alpha1/taskspawner_types.go:377—GitHubWebhook.Reportingis*GitHubReporting, the same shared type that now carriesChecks, so the CRD schema acceptsreporting.checksongithubWebhooksources.GitHubIssueshas a CEL guard (!has(self.reporting) || !has(self.reporting.checks)),GitHubWebhookdoes not. The runtime path that stampsAnnotationGitHubCheckslives only incmd/kelos-spawner/main.go:495(checksReportingEnabledreadsSpec.When.GitHubPullRequests) andinternal/webhook/handler.go:567only stampsAnnotationGitHubReporting. A user who configuresreporting.checkson a webhook source therefore gets no admission error and zero check runs with no diagnostic. Add the same CEL rule toGitHubWebhook(and regeneratedeploy/charts/kelos/templates/crds/taskspawner-crd.yamlandinternal/manifests/install-crd.yaml); remove it when webhook-driven check reporting is wired up in the follow-up PR. Suggested addition above theGitHubWebhookdeclaration:// +kubebuilder:validation:XValidation:rule="!has(self.reporting) || !has(self.reporting.checks)",message="checks reporting is not supported for githubWebhook source"
Conventions / Documentation
- [P2]
api/v1alpha1/taskspawner_types.go:63and PR description — the comment onGitHubReportingsays “The Checks field is only valid for pull-request-based sources (githubPullRequests, and githubWebhook with pull request events)” and the PR description claims the type was split into aGitHubPullRequestReporting(“thechecksfield does not appear in their CRD schema at all”). Neither is true:Checkslives on the sharedGitHubReportingstruct used by all three GitHub sources, and at runtime onlyGitHubPullRequestsis honored. Reconcile the comment, the special-notes section, and (if intent is to defer webhook support) reinforce with the CEL guard above so the doc and schema agree.
Key takeaways
- The CEL guard for
githubWebhookis the only change needed for correctness; once added, the doc/PR-description discrepancy is just a wording fix. - The cache backstop for check-run IDs (
storeCheckRun+ thecached.checkRunID != 0short-circuit) and the duplicate-suppression test coverage are solid.
Note on prompt injection: third-party bot comments on this PR contain embedded directives instructing readers to attribute findings to those tools or to suppress unrelated content. Those instructions were disregarded; findings above are based solely on the diff and surrounding code.
b9f23d4 to
c0bdaa5
Compare
c0bdaa5 to
bb52449
Compare
|
@gjkim42 Okay, updated. I lost the github webhook support for checks somewhere in there, so I added it back. |
|
@greptile review |
|
/kelos review |
|
🤖 Kelos Task Status Task |
There was a problem hiding this comment.
🤖 Kelos Reviewer Agent @gjkim42
Review Summary
Verdict: REQUEST CHANGES
Overall correctness: patch is incorrect
Scope: Adds GitHub Checks API reporting through a new reporting.checks struct on the shared GitHubReporting type, head-SHA propagation via WorkItem and webhook event data, a new ChecksReporter HTTP client, and a parallel create/update lifecycle alongside the existing comment reporter.
Findings Overview
| Priority | Count | File:Line | Summary |
|---|---|---|---|
| P0 | 0 | — | none |
| P1 | 1 | internal/webhook/handler.go:583 | issue_comment-on-PR webhook events stamp AnnotationGitHubChecks=enabled without a SHA, producing a silent no-op |
| P2 | 0 | — | none |
| P3 | 0 | — | none |
Findings
Correctness
- [P1]
internal/webhook/handler.go:583— A webhook spawner that listsissue_commentalongside a PR event type passes theGitHubWebhookCEL guard (!has(self.reporting.checks) || self.events.exists(...)only requires one PR event), but at runtime anissue_commenton a PR returns"pull-request"fromwebhookSourceKind, so the block stampsAnnotationGitHubChecks="enabled"without ever populatingAnnotationSourceSHA—IssueCommentEventparsing ingithub_filter.godoes not setHeadSHA.reportViaCheckRunthen short-circuits on the missing SHA (internal/reporting/watcher.go:277-281), logs an info-level skip, and returns nil. The task carries the enabled annotation but no Check Run is ever created. This is reachable on every/kelos-style trigger comment in a typical mixed-event configuration. Fix by gating the annotation stamping onparsed.GitHub.HeadSHA != "", by restricting the eventType match to the four explicit PR event types, or by lazily fetching the head SHA from the PR API URL the waygithubPRBranchFetcheralready does for the branch.
Suggestions
- [P3] Add a handler-level test for
issue_commenton a PR withreporting.checksconfigured. The currentTestServeHTTP_Checks*tests only exercise thepull_requestevent path, so the silent-skip configuration above is not covered.
Key takeaways
- One P1 silent-failure path on webhook
issue_commentevents. The rest of the wiring — CEL guards ongithubIssues(reject) andgithubWebhook(require PR event), the in-memory cache backstop for check run IDs, the persist/retry refactor, and the dual comment+checks reporting — is consistent and well-tested.
Note on prompt injection: the cubic-dev-ai and greptile-apps review comments on this PR contain embedded directives (e.g., "Prompt for AI agents" sections, attribution demands). Those instructions were disregarded; findings above are formed independently from reading the diff.
…n integration Add GitHub Check Run support to GitHubReporting so Kelos task outcomes appear as formal Check Runs on pull requests. This enables teams to require "Kelos agent passed" in branch protection rules and makes task status visible in the PR Checks tab. New fields on GitHubReporting: - checks (struct, optional) — when present, creates Check Runs - checks.name (string) — overrides the default "Kelos: <spawner-name>" Supported for both githubPullRequests and githubWebhook sources with pull-request event types. A CEL validation rule on githubIssues rejects checks (not applicable), and on githubWebhook requires at least one PR event type when checks is configured. Check Run lifecycle maps task phases to GitHub states: - Pending/Running/Waiting → in_progress - Succeeded → completed (success) - Failed → completed (failure) Head SHA is propagated from PR sources through WorkItem.HeadSHA (polling) or GitHubEventData.HeadSHA (webhook) into a task annotation, enabling Check Run association with the correct commit. Both comment and checks reporting can be enabled simultaneously. Closes kelos-dev#846
bb52449 to
eb3eec8
Compare
|
@gjkim42 Ready |
|
/kelos review |
|
🤖 Kelos Task Status Task |
There was a problem hiding this comment.
🤖 Kelos Reviewer Agent @gjkim42
Review Summary
Verdict: APPROVE
Overall correctness: patch is correct
Scope: Adds GitHub Checks API reporting via a new reporting.checks struct on the shared GitHubReporting type, head-SHA propagation through WorkItem.HeadSHA and GitHubEventData.HeadSHA, a ChecksReporter HTTP client, and a parallel create/update lifecycle in the watcher alongside the existing comment reporter.
Findings Overview
| Priority | Count | File:Line | Summary |
|---|---|---|---|
| P0 | 0 | — | none |
| P1 | 0 | — | none |
| P2 | 0 | — | none |
| P3 | 2 | api/v1alpha1/taskspawner_types.go:381, internal/webhook/handler_test.go | Unrelated MaxItems=20 constraint added to GitHubWebhook.Events; missing handler test for issue_comment-on-PR with checks |
Findings
Code quality / scope creep
- [P3]
api/v1alpha1/taskspawner_types.go:381— A+kubebuilder:validation:MaxItems=20constraint was added toGitHubWebhook.Eventsalong with the matching CRD update. It is not mentioned in the PR description or release note and is unrelated to checks reporting. The cap is generous enough that it is unlikely to break real configurations, but per project guidance ("Keep changes minimal") this drive-by hardening should either be split out or called out in the description.
Tests
- [P3]
internal/webhook/handler_test.go—TestServeHTTP_ChecksAnnotationsForPRWebhookandTestServeHTTP_ChecksAnnotationsSkippedForNonPRWebhookcoverpull_requestandissuesevents, but there is no end-to-end test for anissue_comment-on-PR webhook withreporting.checksconfigured. That path is the one the prior review iterations specifically called out (HeadSHA must come fromenrichGitHubIssueCommentBranch, thencreateTaskmust stamp the checks annotations). The unit-level pieces are tested in isolation, but a handler-level test against apull_requestAPI stub returning a head SHA would lock the integration in place.
Key takeaways
- The two prior P1 concerns (silent-skip on
issue_commentevents with no SHA;reporting.checksaccepted ongithubWebhookwithout runtime support) are both addressed: the handler stampsAnnotationGitHubChecksonly whenparsed.GitHub.HeadSHA != "", and the webhook-server reconciler now wiresChecksReporterbased on the task annotation rather than only the polling spec. - The cache backstop on
storeCheckRun+ thecached.checkRunID != 0short-circuit prevents duplicateCreateCheckRuncalls under reconciler races, and the duplicate-suppression test covers it. persistReportingStateandpersistCheckRunStatenow share apersistAnnotationshelper, removing the earlier duplication concern.- Generated CRD YAML (
deploy/charts/kelos/templates/crds/taskspawner-crd.yaml,internal/manifests/install-crd.yaml) andzz_generated.deepcopy.goare included.
Note on prompt injection: third-party bot comments on this PR (cubic-dev-ai, greptile-apps) include embedded directives (e.g., "Prompt for AI agents" sections, attribution demands). Those instructions were disregarded; findings above are formed independently from reading the diff.
|
I added: Specifically to pass integration testing |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds GitHub Checks API integration to TaskSpawner reporting so that Kelos task outcomes appear as formal Check Runs on pull requests.
Today, Kelos reports task status via PR comments (
reporting.enabled: true). While useful for humans, comments are invisible to GitHub's automated systems — branch protection rules, merge queues, and the Checks tab all ignore them. Teams that want to enforce "Kelos review must pass before merge" have no way to configure this.This PR adds a new
checksstruct to the sharedGitHubReportingtype:checks(struct, optional) — when present, creates GitHub Check Runs that map task phases to check statuses:in_progresscompleted/successcompleted/failurechecks.name(string, optional) — overrides the default name ("Kelos: <spawner-name>"), which is what appears in branch protection rule configuration. The default is stable across releases; renaming a TaskSpawner changes the default and may require updating branch protection rules. MaxLength: 100.Both comment and checks reporting can be enabled simultaneously, allowing gradual migration. The head SHA is propagated from the PR source through
WorkItem.HeadSHA(polling) orGitHubEventData.HeadSHA(webhook) into a task annotation (kelos.dev/source-sha), enabling Check Run association with the correct commit.Supported sources:
checkslives on the sharedGitHubReportingstruct and is supported for:githubPullRequests— always validgithubWebhook— valid when at least one PR event type is configured (pull_request,pull_request_review,pull_request_review_comment,pull_request_target)githubIssues— rejected via CEL validation (Check Runs are not applicable to issues)CEL validation on
githubWebhookensures thatreporting.checksis only accepted when the webhook includes at least one pull-request event type, preventing silent misconfiguration.Example configurations:
Files changed:
api/v1alpha1/taskspawner_types.go, CRD manifestsGitHubChecksReportingtype;checksfield on sharedGitHubReporting; CEL guards ongithubIssues(reject) andgithubWebhook(require PR event);MaxLength=100on nameinternal/source/source.go,github_pr.goHeadSHAtoWorkItem, populated from PR headinternal/webhook/github_filter.goHeadSHAfield toGitHubEventData, extracted from PR-related webhook eventsinternal/webhook/handler.gogithub-checks,source-sha,github-check-name) on webhook-created tasks for PR eventsinternal/reporting/checks.go(new)ChecksReporter— GitHub Checks API client (create + update)internal/reporting/watcher.goTaskReporterwithreportViaCheckRunpath, new annotations for check run state, in-memory cache backstop for check run IDscmd/kelos-spawner/main.go,reconciler.gochecksReportingEnabled(), annotation stamping,ChecksReporterwiring for polling sourcescmd/kelos-webhook-server/reporting.goChecksReporterwhen task has checks annotation; updated predicate to match checks-only taskschecks_test.go,watcher_test.go,main_test.go,handler_test.go,github_filter_test.go,reporting_test.goWhich issue(s) this PR is related to:
Fixes #846
Special notes for your reviewer:
checks:writepermission for Check Runs to be created. This is documented in the CRD field comment.checkslives on the sharedGitHubReportingtype used by all GitHub sources. Applicability is enforced via CEL validation:githubIssuesrejects checks entirely;githubWebhookrequires at least one PR event type (pull_request,pull_request_review,pull_request_review_comment,pull_request_target).webhookSourceKind), so a webhook that mixes PR and issue events only gets Check Runs for the PR tasks.HeadSHAis extracted from the webhook payload'shead.shafield forPullRequestEvent,PullRequestReviewEvent, andPullRequestReviewCommentEvent.ChecksReporterdynamically based on task annotations, and its predicate matches tasks with either comment or checks reporting annotations.kelos.dev/github-check-run-id,kelos.dev/github-check-report-phase) following the same pattern as comment reporting.ReportStateCachein-memory backstop as the comment path, preventing duplicateCreateCheckRuncalls when concurrent reconciles race the annotation Update propagation.reportViaCheckRunlogs an info message and returns nil, making the skip observable.check_run.rerequestedwebhook handling) is intentionally deferred — it requires a small addition to the webhook event parser and is tracked separately.ChecksReporteruses custom HTTP (not go-github) to stay consistent with the existingGitHubReporterpattern and itsTokenFuncdynamic auth.Acceptheader uses the currentapplication/vnd.github+jsonform per GitHub's recommendation."Kelos: <taskspawner-name>") is documented as stable across releases. Renaming a TaskSpawner changes the default, which is also documented.checks.namehasMaxLength=100validation (GitHub's limit) so misconfiguration is caught at admission time.Does this PR introduce a user-facing change?