feat(poll): implement Phase 2 cron poller for GitLab event dispatch#4099
Closed
ggallen wants to merge 1 commit into
Closed
feat(poll): implement Phase 2 cron poller for GitLab event dispatch#4099ggallen wants to merge 1 commit into
ggallen wants to merge 1 commit into
Conversation
Implements the cron-based poller (ADR 0067 Phase 2) that discovers GitLab events via API polling, converts them to NormalizedEvents, routes through the dispatch core, and triggers child pipelines. New packages and files: - internal/dispatch/event.go: NormalizedEvent type and EventRouter interface - internal/poll/: complete poller with event discovery, bot filtering, deduplication, label state diffing, watermark management, and child pipeline YAML generation - internal/cli/poll.go: `fullsend poll` CLI command Test coverage: 91.8% on internal/poll/. Signed-off-by: Greg Allen <greg@fullsend.ai> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Greg Allen <gallen@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 11:24 AM UTC · Completed 11:32 AM UTC |
Member
Author
|
Recreating PR with renamed branch pr-4099 |
PR Summary by QodoAdd Phase 2 cron poller to route GitLab events into dispatch + child pipelines
AI Description
Diagram
High-Level Assessment
Files changed (16)
|
|
🤖 Finished Retro · ✅ Success · Started 11:26 AM UTC · Completed 11:37 AM UTC |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1. Poller bypasses forge.Client
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+13
to
+35
| // Poller discovers GitLab events and dispatches agent stages. | ||
| type Poller struct { | ||
| client GitLabClient | ||
| router dispatch.EventRouter | ||
| projectPath string | ||
| owner string | ||
| repo string | ||
| botUserID int | ||
| gitlabURL string | ||
| opts Options | ||
| dispatches []Dispatch | ||
| } | ||
|
|
||
| // New creates a Poller for the given project. | ||
| func New(client GitLabClient, router dispatch.EventRouter, projectPath string, opts Options) *Poller { | ||
| owner, repo := splitOwnerRepo(projectPath) | ||
| gitlabURL := opts.GitLabURL | ||
| if gitlabURL == "" { | ||
| gitlabURL = "https://gitlab.com" | ||
| } | ||
| return &Poller{ | ||
| client: client, | ||
| router: router, |
There was a problem hiding this comment.
1. poller bypasses forge.client 📘 Rule violation ⌂ Architecture
The new poller introduces a parallel GitLabClient interface and injects it into Poller, instead of routing forge operations through internal/forge.Client as required. This undermines the project’s forge-agnostic boundary and risks duplicated/fragmented forge behavior across packages.
Agent Prompt
## Issue description
The new poller code performs forge operations via a new `poll.GitLabClient` interface instead of using the required `internal/forge.Client` interface.
## Issue Context
Compliance requires all forge operations to flow through `forge.Client` to keep the codebase forge-agnostic and prevent new direct forge-specific API surfaces from spreading outside `internal/forge`.
## Fix Focus Areas
- internal/poll/client.go[12-28]
- internal/poll/poll.go[13-43]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+60
to
+63
| // TODO(phase1): Replace nil client and router with real | ||
| // implementations once the GitLab forge client exists. | ||
| poller := poll.New(nil, nil, projectPath, opts) | ||
| return poller.Run(cmd.Context()) |
There was a problem hiding this comment.
3. Poll command panics 🐞 Bug ☼ Reliability
newPollCmd constructs poll.New(nil, nil, ...) and calls Run(), which dereferences p.client in readWatermark and will panic. Even if the nil client were fixed, a nil router causes Run() to treat all events as “no stages matched” and still advance the watermark, silently dropping dispatches.
Agent Prompt
## Issue description
The `fullsend poll` command is registered and runnable, but it constructs a poller with a nil `GitLabClient` and nil `EventRouter`, which will crash on first API access. Additionally, nil routing currently results in advancing the watermark without dispatching, which can lose events.
## Issue Context
This PR adds the command to the root CLI, making it easy to invoke in CI/cron as described in the plan docs.
## Fix Focus Areas
- Ensure `fullsend poll` fails fast with a clear error until a real GitLab client/router is available, OR fully wire real implementations.
- Add non-nil validation in `poll.New(...)` or `(*Poller).Run(...)` (return error) so misconfiguration can’t panic or silently advance the watermark.
- internal/cli/poll.go[21-64]
- internal/cli/root.go[42-56]
- internal/poll/poll.go[48-100]
- internal/poll/state.go[13-27]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+57
to
+66
| if added, ok := newLabels[issue.IID]; ok { | ||
| for _, label := range added { | ||
| events = append(events, RoutableEvent{ | ||
| Type: "issue_label", | ||
| IID: issue.IID, | ||
| UpdatedAt: issue.UpdatedAt, | ||
| Labels: []string{label}, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
4. Label state incomplete 🐞 Bug ≡ Correctness
discoverAllEvents emits issue_label events with Labels set to only the newly-added label, but toNormalizedEvent copies event.Labels into NormalizedEvent.state.labels. This makes state.labels incorrect for label_changed events and breaks routing logic that expects the current label set.
Agent Prompt
## Issue description
For `issue_label` events, `RoutableEvent.Labels` is being used for two different meanings:
1) the label that changed (used as `transition.label.name`), and
2) the entity’s current label set (should map to `state.labels`).
Currently, only the changed label is sent, so `state.labels` becomes incomplete.
## Issue Context
The normative NormalizedEvent docs and the GitLab poller plan both state that `state.labels` is the *current label set* at event time.
## Fix Focus Areas
- Change `RoutableEvent` to carry both the changed label (e.g. `LabelName` / `ChangedLabel`) and the full `Labels` state.
- Update `discoverAllEvents` to set `Labels` to the issue’s full label set and set the changed-label field separately.
- Update `toNormalizedEvent` to:
- set `state.labels` from the full labels slice, and
- set `transition.label.name` from the changed-label field.
- internal/poll/events.go[57-66]
- internal/poll/types.go[18-32]
- internal/poll/convert.go[14-69]
- docs/plans/gitlab-cron-polling-implementation.md[730-739]
- docs/normative/normalized-event/v1/README.md[56-63]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+85
to
+96
| func generateChildPipelineYAML(dispatches []Dispatch) string { | ||
| var buf bytes.Buffer | ||
| for i, d := range dispatches { | ||
| fmt.Fprintf(&buf, "agent-%d:\n", i) | ||
| fmt.Fprintf(&buf, " trigger:\n") | ||
| fmt.Fprintf(&buf, " include: .gitlab/ci/fullsend-%s.yml\n", d.Stage) | ||
| fmt.Fprintf(&buf, " strategy: depend\n") | ||
| fmt.Fprintf(&buf, " variables:\n") | ||
| fmt.Fprintf(&buf, " STAGE: %q\n", d.Stage) | ||
| fmt.Fprintf(&buf, " EVENT_TYPE: %q\n", d.EventType) | ||
| fmt.Fprintf(&buf, " EVENT_PAYLOAD_B64: %q\n", d.EventPayloadB64) | ||
| fmt.Fprintf(&buf, " RESOURCE_KEY: %q\n", d.ResourceKey) |
There was a problem hiding this comment.
5. Unvalidated stage in yaml 🐞 Bug ☼ Reliability
generateChildPipelineYAML interpolates Dispatch.Stage into the generated YAML include path without validation, so invalid stage strings can generate malformed child pipeline YAML or unexpected include references. The generate-child-pipeline CLI subcommand reads dispatches from an arbitrary file, making this easy to trigger.
Agent Prompt
## Issue description
Stage names are inserted directly into the generated YAML include path. If a stage contains unexpected characters (e.g., whitespace/newlines or path separators), the produced YAML can become invalid or reference unintended include paths.
## Issue Context
`GenerateChildPipelineFromFile` reads user-supplied JSON and generates YAML, so this should be robust against malformed inputs.
## Fix Focus Areas
- Validate `Dispatch.Stage` against a strict allowlist/pattern (e.g. `^[a-z0-9][a-z0-9-]*$`) before generating YAML; return an error on invalid values.
- (Optional) Consider emitting YAML using a YAML library to avoid hand-rolled formatting risks.
- internal/poll/dispatch.go[83-100]
- internal/cli/poll.go[77-93]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+40
to
+47
| payload := buildEventPayload(event) | ||
| encoded := base64.StdEncoding.EncodeToString(payload) | ||
| return p.appendDispatch(Dispatch{ | ||
| Stage: stage, | ||
| EventType: event.Type, | ||
| EventPayloadB64: encoded, | ||
| ResourceKey: fmt.Sprintf("%s-%d", event.Type, event.IID), | ||
| }) |
There was a problem hiding this comment.
6. Resourcekey too specific 🐞 Bug ☼ Reliability
dispatch() sets ResourceKey to "<eventType>-<iid>", so different event types for the same issue/MR get different keys and can run concurrently even when they should share a per-entity lock. The GitLab cron-poller plan shows RESOURCE_KEY being used for resource_group locking, implying a stable per-entity key (e.g. mr-<iid>) is intended.
Agent Prompt
## Issue description
`ResourceKey` is currently derived from `event.Type`, which fragments locking for the same underlying entity (issue/MR) across event types.
## Issue Context
The planned GitLab CI templates use `RESOURCE_KEY` in `resource_group` to prevent concurrent runs against the same resource.
## Fix Focus Areas
- Derive `ResourceKey` from entity kind + IID (e.g., `issue-<iid>` / `mr-<iid>`) rather than event type.
- Ensure merge/comment/label events for the same MR/issue share the same key.
- internal/poll/dispatch.go[34-48]
- internal/poll/convert.go[206-229]
- docs/plans/gitlab-cron-polling-implementation.md[1262-1275]
- docs/plans/gitlab-cron-polling-implementation.md[1496-1497]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+94
to
+114
| for _, mr := range mrs { | ||
| if !mr.MergedAt.IsZero() && mr.MergedAt.After(since) { | ||
| events = append(events, RoutableEvent{ | ||
| Type: "mr_event", | ||
| IID: mr.IID, | ||
| UpdatedAt: mr.MergedAt, | ||
| NoteAuthorID: mr.MergedByID, | ||
| IsBot: mr.MergedBy.Bot, | ||
| MRSource: mr.SourceProjectID, | ||
| MRTarget: mr.TargetProjectID, | ||
| }) | ||
| } | ||
|
|
||
| notes, err := p.client.ListMergeRequestNotes(ctx, owner, repo, mr.IID) | ||
| if err != nil { | ||
| log.Printf("list notes for MR %d: %v (skipping MR entirely)", mr.IID, err) | ||
| if minSkippedAt.IsZero() || mr.UpdatedAt.Before(minSkippedAt) { | ||
| minSkippedAt = mr.UpdatedAt | ||
| } | ||
| continue | ||
| } |
There was a problem hiding this comment.
7. Mr skip not atomic 🐞 Bug ☼ Reliability
When MR note listing fails, discoverAllEvents logs that it is “skipping MR entirely” but may already have appended an mr_event merge event for the same MR. This can lead to partial processing and repeated merge-event dispatch attempts on subsequent polls.
Agent Prompt
## Issue description
MR discovery is not atomic: merge events can be appended before MR note retrieval succeeds, but on note retrieval failure the code indicates the MR is skipped.
## Issue Context
This impacts retry behavior and can cause duplicate merge dispatches when the notes endpoint is flaky.
## Fix Focus Areas
- Decide intended behavior:
- either treat note retrieval failure as skipping *only* note events (and update log message), OR
- truly skip the MR (don’t append `mr_event` until after successful note retrieval, or buffer events and only append on success).
- Ensure `minSkippedAt`/watermark behavior matches the chosen semantics.
- internal/poll/events.go[94-114]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Review skipped — this PR is already closed. The Posted by fullsend post-review check |
Retro: PR #4099 — Close-and-recreate waste cascadePR: #4099 Timeline
Waste analysisThis close-and-recreate triggered 3 wasted or partially-wasted agent runs on the same commit:
Existing issues covering these patternsAll identified improvements are already tracked by open issues. No novel proposals are warranted:
Qodo review findings (for reference)Qodo completed a 7-finding review on the closed PR before the fullsend review agent finished. Notable finding: forge.Client architectural violation — the new ConclusionThe close-and-recreate pattern is a well-understood waste driver with 6+ open issues tracking solutions. Implementing #3204 (skip retro on unmerged PRs) and #2388 (cancel in-flight review on close) would have prevented all waste observed here. No new proposals filed — the improvement backlog already captures the right fixes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
internal/poll/package with event discovery, bot filtering, deduplication, label state diffing, watermark management, and child pipeline YAML generationinternal/dispatch/event.gowith NormalizedEvent type and EventRouter interfacefullsend pollCLI command (internal/cli/poll.go)Test plan
go test ./internal/poll/ -count=1)internal/poll/(target >85%)go build ./...cleango vetclean🤖 Generated with Claude Code