feat(poll): implement Phase 2 cron poller for GitLab event dispatch

[ggallen]

Summary

• 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
• Adds internal/poll/ package with event discovery, bot filtering, deduplication, label state diffing, watermark management, and child pipeline YAML generation
• Adds internal/dispatch/event.go with NormalizedEvent type and EventRouter interface
• Adds fullsend poll CLI command (internal/cli/poll.go)

Test plan

• All 83 tests pass (go test ./internal/poll/ -count=1)
• Coverage: 91.8% on internal/poll/ (target >85%)
• go build ./... clean
• go vet clean
• Pre-commit hooks pass (gofmt, secrets, etc.)

🤖 Generated with Claude Code

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:25 AM UTC · Completed 11:38 AM UTC
Commit: f0ccd41 · View workflow run →

[qodo-code-review]

PR Summary by Qodo

Add Phase 2 cron poller to dispatch GitLab events into child pipelines

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Add cron-driven GitLab event poller that discovers events, normalizes them, and routes stages.
• Persist watermark/label state via CI variables and emit dispatches for child pipeline triggering.
• Introduce fullsend poll CLI (plus child-pipeline YAML generator) and comprehensive poller tests.

Diagram

graph TD
  A["fullsend CLI"] --> B["Poller.Run"] --> C{{"GitLab API"}} --> D["RoutableEvent"] --> E["NormalizedEvent"] --> F["EventRouter"] --> G["dispatches.json"] --> H["child-pipeline.yml"]

  subgraph Legend
    direction LR
    _cli["CLI"] ~~~ _svc["Service/Logic"] ~~~ _ext{{"External"}} ~~~ _file["File output"]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use GitLab webhooks instead of cron polling

• ➕ Near-real-time dispatch with less API load
• ➕ Avoids watermark/label-state persistence complexity
• ➕ More reliable event ordering/delivery semantics (with retries)
• ➖ Requires inbound connectivity and webhook secret management
• ➖ Harder to run purely inside GitLab CI without external service
• ➖ May need additional infra for durable queueing

2. Persist watermark/state in a dedicated store (e.g., Redis/S3)

• ➕ Decouples state from CI variable limits/permissions
• ➕ Better suited for multi-project or high-frequency polling
• ➕ More flexibility for schema evolution/versioning
• ➖ Introduces infra dependency and operational overhead
• ➖ Less self-contained for GitLab-only deployments

3. Route directly from RoutableEvent instead of NormalizedEvent

• ➕ Less conversion/mapping code; fewer structs
• ➕ Potentially easier to align with GitLab-specific nuances
• ➖ Couples dispatch core to forge-specific semantics
• ➖ Makes future forge support harder; conflicts with the normalized-event ADR direction

Recommendation: For ADR 0067 Phase 2, the PR’s approach (poll -> NormalizedEvent -> EventRouter -> dispatch artifacts) is the right long-term shape because it keeps routing forge-neutral and enables adding other forges later. If/when a webhook-capable deployment target exists, consider migrating to webhooks to reduce polling/state complexity; until then, CI-variable watermark/state is a pragmatic self-contained persistence mechanism.

Files changed (16) +3951 / -0

Enhancement (10) +1274 / -0

poll.goAdd 'fullsend poll' command and child-pipeline generator subcommand +95/-0

Add 'fullsend poll' command and child-pipeline generator subcommand

• Introduces a new Cobra command for polling GitLab and writing dispatch output, plus a 'generate-child-pipeline' subcommand that converts dispatches JSON into GitLab CI YAML. Includes flag/env handling for forge selection, project path, polling mode, GitLab URL, and output paths; poller wiring is intentionally stubbed pending Phase 1 GitLab client/router implementations.

internal/cli/poll.go

root.goRegister poll command on root CLI +1/-0

Register poll command on root CLI

• Adds the new poll command to the root CLI command tree so it is available as 'fullsend poll'.

internal/cli/root.go

event.goDefine forge-neutral NormalizedEvent model and EventRouter interface +92/-0

Define forge-neutral NormalizedEvent model and EventRouter interface

• Adds the dispatch-domain 'NormalizedEvent' schema (entity/transition/actor/state/source) used as a routing input and for CEL trigger evaluation. Introduces 'EventRouter' as the interface the poller uses to map normalized events to stage names.

internal/dispatch/event.go

client.goIntroduce GitLabClient interface and API DTO types for polling +96/-0

Introduce GitLabClient interface and API DTO types for polling

• Defines the GitLab API surface area required by the cron poller, including list/get operations and CI variable/emoji interactions. Adds minimal structs representing issues, merge requests, notes, project events, and label events.

internal/poll/client.go

convert.goConvert discovered events into NormalizedEvent for routing +267/-0

Convert discovered events into NormalizedEvent for routing

• Implements conversion from 'RoutableEvent' into 'dispatch.NormalizedEvent', including transition mapping (label/comment/merge), actor role resolution via access levels, label-author resolution via label events, and best-effort entity-author detection. Also provides utilities for slash-command extraction and GitLab URL/entity-kind/raw-type mapping.

internal/poll/convert.go

dispatch.goCreate dispatch records, write dispatch JSON, and generate child pipeline YAML +117/-0

Create dispatch records, write dispatch JSON, and generate child pipeline YAML

• Implements dispatch record accumulation and JSON emission, including base64-encoded per-event payloads and a stable resource key. Adds YAML generation that emits one GitLab child-pipeline trigger job per dispatch and a helper to generate YAML from a dispatches file (used by the CLI subcommand).

internal/poll/dispatch.go

events.goDiscover routable GitLab events, filter bots, and deduplicate +221/-0

Discover routable GitLab events, filter bots, and deduplicate

• Implements full discovery (issues + notes + label diffs + MR merges + MR notes) and fast discovery (Events API for '/fs-' notes only). Adds bot filtering rules (dropping bot events except enrolled bot changes-requested markers), fork detection helper, routable label filtering, and key-based deduplication.

internal/poll/events.go

poll.goImplement Poller.Run loop with watermarking, routing, and state persistence +188/-0

Implement Poller.Run loop with watermarking, routing, and state persistence

• Adds the core poll cycle: read watermark, discover events (fast/full), filter bots, deduplicate, convert to NormalizedEvent, route via EventRouter, and build dispatch outputs. Advances watermark conservatively based on failures/skips, reacts to slash commands with an emoji, and persists label state with rollback for failed label events.

internal/poll/poll.go

state.goManage watermark and label-state persistence in CI variables +138/-0

Manage watermark and label-state persistence in CI variables

• Implements reading/updating the poll watermark (with separate fast/full variable names) and a label-state diff engine that detects newly added routable labels. Persists label state to a CI variable, prunes closed issues, and degrades gracefully on corrupt stored state.

internal/poll/state.go

types.goDefine poller Options, event/dispatch types, and dedup keying +59/-0

Define poller Options, event/dispatch types, and dedup keying

• Introduces core poller configuration, the 'RoutableEvent' intermediary type, label state tracking type, dispatch record struct, and label author struct. Provides a 'Key()' method for deterministic deduplication based on note IDs or label sets.

internal/poll/types.go

Tests (6) +2677 / -0

convert_test.goAdd unit tests for NormalizedEvent conversion and helpers +600/-0

Add unit tests for NormalizedEvent conversion and helpers

• Covers conversion behavior across issue notes, label changes, MR notes, and merge events, including bot/human actor classification and change proposal state population. Adds targeted tests for helper functions (command extraction, entity URL/kind mapping, role resolution, label-author resolution, and entity-author detection).

internal/poll/convert_test.go

dispatch_test.goAdd tests for dispatch JSON output and child-pipeline YAML generation +445/-0

Add tests for dispatch JSON output and child-pipeline YAML generation

• Validates dispatch accumulation, empty/non-empty JSON writing behavior, payload encoding, and YAML generation for single/multiple/empty dispatch sets. Includes file-based integration-style tests for 'GenerateChildPipelineFromFile' error cases and outputs.

internal/poll/dispatch_test.go

events_test.goAdd tests for event discovery, bot filtering, and deduplication +579/-0

Add tests for event discovery, bot filtering, and deduplication

• Covers issue note discovery, label-change event creation, MR merge/note discovery, and failure handling (note/MR API errors affecting minSkippedAt and rollback). Adds tests for fast slash-command discovery, bot username detection, bot-event filtering rules, fork detection, routable label filtering, and deduplication keys.

internal/poll/events_test.go

mock_test.goAdd configurable GitLabClient mock for poller tests +173/-0

Add configurable GitLabClient mock for poller tests

• Implements a deterministic mock GitLab client with configurable data and error injection for all poller-required APIs. Records side effects like CI variable updates and emoji reactions to support behavior assertions in tests.

internal/poll/mock_test.go

poll_test.goAdd end-to-end poll-cycle tests with routing and failure scenarios +456/-0

Add end-to-end poll-cycle tests with routing and failure scenarios

• Tests poller construction defaults, watermark advancement behavior, fast vs full mode, dispatch output creation, multiple stages, no-stage routing, router/conversion failures, and label-state rollback semantics. Uses the mock client and stub router to validate side effects (CI var writes, emoji reactions, dispatch JSON).

internal/poll/poll_test.go

state_test.goAdd tests for watermark and label-state persistence/diffing logic +424/-0

Add tests for watermark and label-state persistence/diffing logic

• Covers first-run defaults, timestamp parsing and error propagation, fast/full watermark variable selection, new-label detection behavior, corrupt state recovery, pruning of closed issues, previous-state snapshotting, persistence JSON correctness, and helper set conversion.

internal/poll/state_test.go

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 54 rules

✅ Skills: writing-user-docs, writing-adrs

1. Poll command nil client ✓ Resolved 🐞 Bug ≡ Correctness

Description

fullsend poll constructs poll.New(nil, nil, ...) and immediately calls Run, which dereferences
p.client and will panic. Because the command is registered on the root CLI, any invocation of
fullsend poll will crash instead of failing gracefully.

Code

internal/cli/poll.go[R60-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())

Relevance

⭐⭐⭐ High

Team often accepts CLI fail-fast validation to prevent runtime crashes (e.g., stricter flag/env
checks in PR #215).

PR-#215

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The CLI constructs the poller with a nil client and calls Run(). Run() immediately reads the
watermark, and readWatermark() dereferences p.client via GetCIVariable, which will panic when
the client is nil. The command is reachable because it is added to the root command.

internal/cli/poll.go[43-64]
internal/cli/root.go[41-56]
internal/poll/poll.go[45-52]
internal/poll/state.go[13-22]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`newPollCmd` builds a `poll.Poller` with a nil `GitLabClient` and calls `Run()`. `Run()` calls `readWatermark()`, which unconditionally calls `p.client.GetCIVariable(...)`, causing a nil-pointer panic.

### Issue Context
The poll command is already registered in the root command tree, so this is a user-facing crash path.

### Fix Focus Areas
- internal/cli/poll.go[24-64]
- internal/cli/root.go[41-56]
- internal/poll/poll.go[45-55]
- internal/poll/state.go[13-22]

### Expected fix
- Add an explicit runtime guard before any `p.client` usage (preferably at the start of `(*Poller).Run`) that returns a clear error like `"poller requires a GitLab client"`.
- Optionally also guard in the CLI (if Phase 1 wiring isn’t ready) and return a user-friendly error instead of constructing the poller.
- Add a unit test to ensure `Run()` returns an error (not panic) when `client == nil`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. poll.GitLabClient bypasses forge.Client ✗ Dismissed 📘 Rule violation ⌂ Architecture

Description

The new poller introduces a poll.GitLabClient interface and wires poller logic against it instead
of routing forge operations through internal/forge.Client. This violates the requirement to keep
forge operations encapsulated behind forge.Client, increasing divergence and making cross-forge
support harder to enforce consistently.

Code

internal/poll/client.go[R12-28]

+// GitLabClient defines the GitLab API surface the poller requires.
+// The interface is satisfied by the GitLab forge client (Phase 1).
+type GitLabClient interface {
+	ListIssuesUpdatedSince(ctx context.Context, owner, repo string, since time.Time) ([]Issue, error)
+	ListMergeRequestsUpdatedSince(ctx context.Context, owner, repo string, since time.Time) ([]MergeRequest, error)
+	ListProjectEvents(ctx context.Context, owner, repo string, targetType string, after time.Time) ([]ProjectEvent, error)
+	ListIssueNotes(ctx context.Context, owner, repo string, issueIID int) ([]Note, error)
+	ListMergeRequestNotes(ctx context.Context, owner, repo string, mrIID int) ([]Note, error)
+	ListResourceLabelEvents(ctx context.Context, owner, repo string, issueIID int) ([]ResourceLabelEvent, error)
+	GetCIVariable(ctx context.Context, owner, repo, name string) (string, error)
+	UpdateCIVariable(ctx context.Context, owner, repo, name, value string, protected bool) error
+	GetAuthenticatedUser(ctx context.Context) (string, error)
+	CreateNoteAwardEmoji(ctx context.Context, owner, repo string, noteableIID, noteID int, emoji string) error
+	GetIssue(ctx context.Context, owner, repo string, issueIID int) (*Issue, error)
+	GetMemberAccessLevel(ctx context.Context, owner, repo string, userID int) (int, error)
+	GetProjectPath(ctx context.Context, projectID int) (string, error)
+}

Relevance

⭐⭐ Medium

Repo recently expanded forge.Client for GitLab support (PR #3194), but no clear precedent rejecting
extra GitLab-specific interfaces.

PR-#3194

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1062052 requires forge operations to use internal/forge.Client. The PR adds a new
poll.GitLabClient interface for GitLab API calls and the Poller struct consumes it directly
(including forge actions like creating award emoji), rather than depending on forge.Client.

Rule 1062052: Route all git forge operations through forge.Client
internal/poll/client.go[12-28]
internal/poll/poll.go[13-16]
internal/poll/poll.go[101-115]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The poller defines and depends on a new `poll.GitLabClient` interface for GitLab API operations, which bypasses the required `internal/forge.Client` abstraction.

## Issue Context
Compliance requires all git forge operations (GitHub/GitLab/etc.) to flow through `forge.Client` so the rest of the codebase stays forge-agnostic and behavior remains consistent across implementations.

## Fix Focus Areas
- internal/poll/client.go[12-28]
- internal/poll/poll.go[13-16]
- internal/poll/poll.go[101-115]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Incorrect labels in events ✓ Resolved 🐞 Bug ≡ Correctness

Description

issue_label events only store the newly-added label (and MR events store no labels), but
toNormalizedEvent uses event.Labels as the entity’s full label state. This makes
NormalizedEvent.state.labels incorrect/missing and will break any routing logic that evaluates
label state.

Code

internal/poll/events.go[R57-129]

+		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},
+				})
+			}
+		}
+
+		for _, note := range notes {
+			if note.CreatedAt.Before(since) {
+				continue
+			}
+			events = append(events, RoutableEvent{
+				Type:         "issue_note",
+				IID:          issue.IID,
+				UpdatedAt:    note.CreatedAt,
+				NoteBody:     note.Body,
+				NoteID:       note.ID,
+				NoteAuthorID: note.Author.ID,
+				IsBot:        note.Author.Bot,
+				Labels:       issue.Labels,
+			})
+		}
+	}
+
+	mrs, err := p.client.ListMergeRequestsUpdatedSince(ctx, owner, repo, since)
+	if err != nil {
+		log.Printf("list merge requests: %v (continuing with issue events only)", err)
+		if minSkippedAt.IsZero() || since.Before(minSkippedAt) {
+			minSkippedAt = since
+		}
+		return events, updatedLabelState, minSkippedAt, nil
+	}
+
+	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
+		}
+		for _, note := range notes {
+			if note.CreatedAt.Before(since) {
+				continue
+			}
+			events = append(events, RoutableEvent{
+				Type:         "mr_note",
+				IID:          mr.IID,
+				UpdatedAt:    note.CreatedAt,
+				NoteBody:     note.Body,
+				NoteID:       note.ID,
+				NoteAuthorID: note.Author.ID,
+				IsBot:        note.Author.Bot,
+				MRSource:     mr.SourceProjectID,
+				MRTarget:     mr.TargetProjectID,
+			})

Relevance

⭐⭐ Medium

No historical evidence on poller label-state semantics; internal/poll is new so team acceptance is
unclear.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

discoverAllEvents emits issue_label events with Labels: []string{label} (only the added label)
while MR events omit labels, but toNormalizedEvent copies event.Labels into
NormalizedEvent.State.Labels. This contradicts the dispatch.State contract that Labels
represent the entity’s state at event time.

internal/poll/events.go[57-66]
internal/poll/events.go[94-130]
internal/poll/convert.go[12-32]
internal/poll/client.go[40-55]
internal/dispatch/event.go[64-68]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The poller’s `RoutableEvent.Labels` is being overloaded:
- For `issue_label`, it holds only the changed label.
- For MR events/notes, it is omitted entirely.
But `toNormalizedEvent` sets `NormalizedEvent.State.Labels = event.Labels`, and `dispatch.State` is documented as “entity’s state at event time”. This means normalized events don’t reliably contain the full label set.

### Issue Context
Label-based CEL/routing rules generally need the complete label state (e.g., “has label X and not Y”), not just the label delta.

### Fix Focus Areas
- internal/poll/types.go[18-33]
- internal/poll/events.go[57-66]
- internal/poll/events.go[94-130]
- internal/poll/convert.go[14-42]
- internal/poll/client.go[40-55]

### Expected fix
- Extend `RoutableEvent` to separate **label delta** from **label state**, e.g.:
 - `Labels []string` = full current labels at event time
 - `ChangedLabel string` (or `LabelDelta struct{ Name, Action string }`) = which label changed
- When emitting `issue_label` events, set `Labels` to the issue’s full label list, and set `ChangedLabel` to the added label.
- When emitting MR events (`mr_note`, `mr_event`), populate `Labels: mr.Labels`.
- Update `toNormalizedEvent` to:
 - use full `event.Labels` for `State.Labels`
 - use `ChangedLabel` for `Transition.Label.Name` and label-author resolution
- Add/adjust tests to assert `State.Labels` contains the full label set for `issue_label` and MR events.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Unsafe stage YAML include ✓ Resolved 🐞 Bug ⛨ Security

Description

Child pipeline YAML generation interpolates stage into an unquoted YAML scalar for the trigger
include path. If a stage contains whitespace, :, or newlines, the generated YAML can become
invalid or structurally altered.

Code

internal/poll/dispatch.go[R85-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)

Relevance

⭐⭐⭐ High

Security hardening around interpolation/escaping is commonly accepted (e.g., URL/path escaping +
safer request building in PR #215).

PR-#215

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The include path line is generated by interpolating d.Stage directly into YAML without
quoting/escaping, while other values are quoted, making this the primary YAML-structure risk
surface.

internal/poll/dispatch.go[83-99]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`generateChildPipelineYAML` writes:
`include: .gitlab/ci/fullsend-%s.yml` using raw `%s` interpolation. Because this is unquoted YAML, stage strings containing special characters can break parsing or change the resulting YAML structure.

### Issue Context
Even if stage names are *intended* to be trusted, this is a sharp edge: a misconfigured stage name can silently generate invalid YAML.

### Fix Focus Areas
- internal/poll/dispatch.go[83-101]
- internal/poll/dispatch.go[103-117]

### Expected fix
- Validate stage names against a strict allowlist (e.g. `^[a-z0-9_-]+$`) before emitting YAML.
- Quote the include path as a YAML string (e.g., use `%q` on the full path) or, preferably, generate YAML via a YAML library to avoid manual formatting.
- Consider changing `generateChildPipelineYAML` to return `(string, error)` and propagate validation errors through `GenerateChildPipelineFromFile`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Unstable RESOURCE_KEY per entity ✓ Resolved 🐞 Bug ☼ Reliability

Description

Dispatch.ResourceKey is derived from event.Type and IID, so the same issue/MR can produce
different keys across event types. This undermines the documented use of RESOURCE_KEY for
resource_group concurrency control and can allow concurrent agent runs for the same entity.

Code

internal/poll/dispatch.go[R36-47]

+func (p *Poller) dispatch(ctx context.Context, owner, repo, stage string, event RoutableEvent) error {
+	_ = ctx   // reserved for future use
+	_ = owner // included in signature for routing context
+	_ = repo
+	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),
+	})

Relevance

⭐⭐ Medium

No historical evidence found about RESOURCE_KEY stability/resource_group conventions; new
internal/poll code has no prior review patterns.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The poller sets ResourceKey to <event type>-<iid>, while the repo’s GitLab polling plan
explicitly uses RESOURCE_KEY as the stable identifier for resource_group locking and shows MR
keys that don’t include the event type.

internal/poll/dispatch.go[34-47]
docs/plans/gitlab-cron-polling-implementation.md[1262-1275]
docs/plans/gitlab-cron-polling-implementation.md[1488-1497]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ResourceKey` is currently `fmt.Sprintf("%s-%d", event.Type, event.IID)`. For a single issue, an `issue_note` and an `issue_label` event will get different `RESOURCE_KEY` values, which defeats entity-level serialization.

### Issue Context
The repo’s GitLab polling plan uses `RESOURCE_KEY` to drive `resource_group: "fullsend-<stage>-${RESOURCE_KEY}"` so jobs for the same entity don’t overlap.

### Fix Focus Areas
- internal/poll/dispatch.go[34-47]
- docs/plans/gitlab-cron-polling-implementation.md[1262-1275]
- docs/plans/gitlab-cron-polling-implementation.md[1488-1497]

### Expected fix
- Compute a stable key per entity, independent of event type, e.g.:
 - `issue-<iid>` for issue events
 - `mr-<iid>` for MR events
- Optionally include `projectPath` if cross-project collisions are possible in downstream aggregation.
- Add/adjust tests to assert the same entity yields the same `ResourceKey` across multiple event types.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Panic on payload marshal ✓ Resolved 🐞 Bug ☼ Reliability

Description

buildEventPayload panics on JSON marshal error instead of returning an error to the poll loop.
This makes the poller fragile to future payload changes (or unexpected values) by crashing the poll
job rather than reporting/handling the failure.

Code

internal/poll/dispatch.go[R76-79]

+	data, err := json.Marshal(m)
+	if err != nil {
+		panic(fmt.Sprintf("buildEventPayload: marshal failed: %v", err))
+	}

Relevance

⭐⭐⭐ High

Team has accepted robustness changes to avoid fragile failures and improve error handling (e.g.,
hardening external-call error paths in PR #215).

PR-#215

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code explicitly panics on json.Marshal failure, and dispatch()/Run() do not recover from
panics, so any future marshal error will crash the poll process instead of being handled like other
dispatch failures.

internal/poll/dispatch.go[50-80]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`buildEventPayload` calls `json.Marshal` and panics if it fails. Panics in the poller will crash the job and skip normal error handling.

### Issue Context
Marshal failures are unlikely today with the current map contents, but this pattern is brittle if the payload evolves.

### Fix Focus Areas
- internal/poll/dispatch.go[50-81]
- internal/poll/dispatch.go[34-48]

### Expected fix
- Change `buildEventPayload` to return `([]byte, error)`.
- Update `dispatch()` to propagate the error via its existing `error` return.
- Optionally add a small unit test that forces a marshal error (e.g., by factoring payload construction to allow injection of an unsupported type) or at least ensures `dispatch()` handles the error path without panicking.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[fullsend-ai-review]

Review

Verdict: Approve — two prior low-severity findings resolved; three remaining findings are low severity.

This is a re-review (prior SHA: 3af24766). The only change is in internal/poll/convert_test.go (blob SHA d460680b vs prior 92197ab1). All other 15 PR files have identical blob SHAs. Two of five prior findings were resolved:

Prior #	Status	Resolution
1 (Low, edge-case, discoverSlashCommands MR context)	⚠ Unchanged	Fast-poll path still omits MR context; severity anchored at low per prior review
2 (Low, test-adequacy, isEntityAuthor MR positive case)	✅ Resolved	TestIsEntityAuthor_MREvent now tests positive case (MRAuthorID==actorID → true), negative case (actorID=99 → false), and zero-value guard (MRAuthorID=0 → false). All three logical branches of event.MRAuthorID != 0 && event.MRAuthorID == actorID are exercised.
3 (Low, test-adequacy, mapRawAction untested)	✅ Resolved	TestMapRawAction table test covers all four explicit switch cases (issue_label→labeled, issue_note→commented, mr_note→commented, mr_event→merged) plus the default case (unknown→"").
4 (Low, docs-staleness, poll command undocumented)	⚠ Unchanged	docs/cli/README.md does not list the poll command; no docs/cli/poll.md exists
5 (Low, fail-open, bot detection inconsistency)	⚠ Unchanged	Severity anchored at low per prior review

No assertions were weakened — TestIsEntityAuthor_IssueNote remains intact, TestMapRawAction is entirely new, and TestIsEntityAuthor_MREvent strictly adds coverage. No new findings identified in the delta.

---

Findings

Low

#	Category	File	Description
1	edge-case	internal/poll/events.go	discoverSlashCommands omits MR context for mr_note events. Events created in fast-poll mode lack MRSource, MRTarget, SourceBranch, TargetBranch, MRAuthorID, and MRAuthorLogin. buildChangeProposalState will log a warning and produce nil ChangeProposal. The fast-poll path cannot run yet (nil client guard at poll.go:49-51), and this is documented as an intentional trade-off in ADR 0067 (lightweight polling avoids per-note MR detail fetches). Severity anchored at low per prior review.
2	docs-staleness	docs/cli/README.md	New fullsend poll command not documented. docs/cli/README.md does not list the poll command, and no docs/cli/poll.md exists. Environment variables (FULLSEND_FORGE_TOKEN, CI_PROJECT_PATH, FULLSEND_POLL_MODE) and CI state variables (FULLSEND_LAST_POLL_AT_FAST, FULLSEND_LAST_POLL_AT_FULL, FULLSEND_LABEL_STATE) are also undocumented. Since the command is a Phase 2 stub with nil client guard, documentation may be intentionally deferred.
3	fail-open	internal/poll/events.go	Bot detection inconsistency between poll modes. discoverSlashCommands uses isProjectAccessTokenBot(username) while discoverAllEvents uses Author.Bot. The evt.Author.Bot field is available in fast-poll but unused. Blast radius is limited to slash commands in fast-poll mode. Severity anchored at low per prior review.

---

Reviewed dimensions: correctness (opus), security (opus), intent & coherence, docs currency. Findings adjudicated against prior review provenance: app-verified. Delta: 1 file changed (convert_test.go), 2 test functions added/updated.

---

Labels: PR implements GitLab cron-polling feature under internal/poll/ and internal/dispatch/

Previous run

Review

Verdict: Approve — both medium-severity findings from the prior review are resolved; remaining findings are low severity.

This is a re-review (prior SHA: f33be3fb). Seven of nine prior findings were resolved in this commit:

Prior #	Status	Resolution
1 (Medium, api-contract, actor.id numeric)	✅ Resolved	actorLogin propagated via NoteAuthorLogin / MergedByLogin; ne.Actor.ID now uses username string
2 (Medium, api-contract, mapRawType non-standard)	✅ Resolved	mapRawType returns "issues" / "merge_request"; new mapRawAction populates RawAction
3 (Low, docs-staleness)	⚠ Unchanged	Poll command still not documented
4 (Low, error-handling, buildChangeProposalState)	✅ Resolved	Error now logged with log.Printf("WARNING: ...")
5 (Low, edge-case, isEntityAuthor MR)	✅ Resolved	Now handles mr_note/mr_event via event.MRAuthorID == actorID
6 (Low, missing-fields, event payload)	✅ Resolved	SourceBranch, TargetBranch, MRAuthorID, IsBot now included in buildEventPayload
7 (Low, yaml-injection, EventType/ResourceKey)	✅ Resolved	validYAMLField regex validates both before YAML interpolation
8 (Low, fail-open, bot detection)	⚠ Unchanged	Acknowledged as intentional tradeoff; severity-anchored at low
9 (Low, data-exposure, NoteBody not truncated)	✅ Resolved	buildEventPayload now applies truncate(event.NoteBody, 4096)

The implementation is well-structured: watermark-based polling with at-least-once delivery, label state diffing, deny-by-default fork detection, and fail-closed authorization (resolveActorRole defaults to "none"). Test suite is thorough (83 tests, 91.8% coverage). The two prior medium findings — actor.id using numeric strings and mapRawType returning non-standard values — are both correctly resolved in this commit.

---

Findings

Low

#	Category	File	Description
1	edge-case	internal/poll/events.go	discoverSlashCommands omits MR context for mr_note events. Events created in fast-poll mode lack MRSource, MRTarget, SourceBranch, TargetBranch, MRAuthorID, and MRAuthorLogin. buildChangeProposalState will log a warning and produce nil ChangeProposal. The fast-poll path cannot run yet (nil client guard at poll.go:49-51), and this is documented as an intentional trade-off in ADR 0067 (lightweight polling avoids per-note MR detail fetches). Severity anchored at low per prior review.
2	test-adequacy	internal/poll/convert_test.go	Missing positive-case assertion for isEntityAuthor on MR events. TestIsEntityAuthor_MREvent only tests the negative case (MRAuthorID=0). TestToNormalizedEvent_MREventMerge sets up matching IDs (MRAuthorID=10, NoteAuthorID=10) but does not assert ne.Actor.IsEntityAuthor. The positive path IS tested for issues (TestToNormalizedEvent_IssueNote line 68-69).
3	test-adequacy	internal/poll/convert.go	No test for mapRawAction. The new function (lines 212-224) has zero direct or indirect test coverage — no TestMapRawAction table test and no Source.RawAction assertions in integration tests. The function is trivial (10-line switch) but was a prior review finding that drove the fix.
4	docs-staleness	docs/cli/README.md	New fullsend poll command not documented. docs/cli/README.md does not list the poll command, and no docs/cli/poll.md exists. Environment variables (FULLSEND_FORGE_TOKEN, CI_PROJECT_PATH, FULLSEND_POLL_MODE) and CI state variables (FULLSEND_LAST_POLL_AT_FAST, FULLSEND_LAST_POLL_AT_FULL, FULLSEND_LABEL_STATE) are also undocumented. Since the command is a Phase 2 stub with nil client guard, documentation may be intentionally deferred.
5	fail-open	internal/poll/events.go	Bot detection inconsistency between poll modes. discoverSlashCommands uses isProjectAccessTokenBot(username) while discoverAllEvents uses Author.Bot. The evt.Author.Bot field is available in fast-poll but unused. Blast radius is limited to slash commands in fast-poll mode. Severity anchored at low per prior review.

---

Reviewed dimensions: correctness (opus), security (opus), intent & coherence, style & conventions, docs currency. Findings adjudicated by challenger pass. Prior review provenance: app-verified.

---

Labels: PR adds GitLab polling infrastructure under internal/poll/ and dispatch event types under internal/dispatch/

---

Labels: PR implements a new feature (GitLab cron polling); type/feature label aligns with feat() PR title prefix

Previous run (2)

Review

Verdict: Comment — two medium-severity API contract findings worth addressing; none blocking.

This is a re-review (prior SHA: 77c41631). Three of five prior low-severity findings were resolved in this commit:

Prior #	Status	Resolution
1 (Low, api-contract, nil Labels)	✅ Resolved	Labels: []string{} initialization in discoverSlashCommands
2 (Low, api-contract, byte truncation)	✅ Resolved	truncate() now uses []rune for Unicode-correct code-point truncation
3 (Low, correctness, bot detection)	⚠ Unchanged	Acknowledged as intentional tradeoff; severity-anchored at low
4 (Low, dead-code, isForkMR)	✅ Resolved	Function removed
5 (Low, docs-currency)	⚠ Unchanged	Poll command still not documented (merged into finding #3 below)

The implementation is well-structured: watermark-based polling with at-least-once delivery, label state diffing, deny-by-default fork detection, and fail-closed authorization (resolveActorRole defaults to "none"). Test suite is thorough (83 tests, 91.8% coverage). Two new medium-severity API contract findings were identified on code unchanged since the prior review, relating to NormalizedEvent values that don't match the normative schema examples.

---

Findings

Medium

#	Category	File	Description
1	api-contract	internal/poll/convert.go	actor.id uses numeric string instead of username. strconv.Itoa(authorID) produces values like "42", but the normative schema defines actor.id as "Forge login or service account identifier" and all examples (including gitlab-mr-merged.json) use username strings (e.g., "dev-user", "maintainer-user"). The same issue applies to ChangeProposalState.AuthorID. UserRef.Username is already available from the GitLab API response but is not propagated through RoutableEvent. Fix: Propagate username through RoutableEvent and use it for actor.id and author_id.
2	api-contract	internal/poll/convert.go	mapRawType returns non-standard values. Returns "issue_event" and "merge_request_event", but normative examples show "merge_request" and the adapter README states raw_type carries "the GitLab object type (e.g. merge_request, note)". Additionally, Source.RawAction is never populated — normative examples show raw_action: "merged", raw_action: "opened". CEL triggers written against the normative examples will not match. Fix: Return "issue" / "merge_request" and populate RawAction.

Low

#	Category	File	Description
3	docs-staleness	docs/cli/README.md	New fullsend poll command not documented. docs/cli/README.md does not list the poll command, and no docs/cli/poll.md exists. The CLI docs follow a per-command markdown file pattern. Environment variables (FULLSEND_FORGE_TOKEN, CI_PROJECT_PATH, FULLSEND_POLL_MODE) are also undocumented. Since the command is a Phase 1 stub, documentation may be intentionally deferred.
4	error-handling	internal/poll/convert.go	buildChangeProposalState errors silently swallowed. When GetProjectPath fails for mr_event/mr_note, the NormalizedEvent is emitted without state.change_proposal and no warning is logged. Fail-closed (stages won't match), but operators have no visibility into missing data. Add log.Printf on error.
5	edge-case	internal/poll/convert.go	isEntityAuthor always returns false for MR events. MRAuthorID is available on RoutableEvent (populated in discoverAllEvents) but not compared with actorID. This means actor.is_entity_author is always false for MR comments and merges.
6	missing-fields	internal/poll/dispatch.go	buildEventPayload omits SourceBranch, TargetBranch, MRAuthorID, IsBot. These fields are on RoutableEvent but not in the serialized event payload passed to child pipelines.
7	yaml-injection	internal/poll/dispatch.go	EventType and ResourceKey not regex-validated in generateChildPipelineYAML (only Stage is). Currently these are constructed from hardcoded strings and integers (not exploitable), but GenerateChildPipelineFromFile reads from disk. Defense-in-depth: add allowlist validation matching the validStage pattern.
8	fail-open	internal/poll/events.go	Bot detection inconsistency between poll modes. discoverSlashCommands uses isProjectAccessTokenBot(username) while discoverAllEvents uses Author.Bot. The evt.Author.Bot field is available but unused. (Severity anchored at low per prior review.)
9	data-exposure	internal/poll/dispatch.go	NoteBody in event payload is not truncated. Unlike the NormalizedEvent comment body (truncated to 4096 code points), the raw event payload passes NoteBody at full length.

---

Reviewed dimensions: correctness (opus), security (opus), intent & coherence, style & conventions, docs currency. Findings adjudicated by challenger pass. Prior review provenance: app-verified.

---

Labels: PR adds GitLab polling infrastructure under internal/poll/ and dispatch event types under internal/dispatch/

Previous run (3)

Review

Verdict: Approve — all prior findings resolved; remaining findings are low severity.

This is a re-review (prior SHA: f0ccd41). All six findings from the prior review have been addressed:

Prior #	Status	Resolution
1 (Medium, api-contract)	✅ Resolved	buildChangeProposalState now populates HeadRef, BaseRef, AuthorID from event.SourceBranch, event.TargetBranch, event.MRAuthorID
2 (Medium, error-handling)	✅ Resolved	buildEventPayload returns ([]byte, error) — no more panic()
3 (Medium, injection-defense)	✅ Resolved	validStage regex ^[a-z][a-z0-9-]*$ validates stage names before path interpolation
4 (Low, correctness)	✅ Resolved	Run() has nil guard: if p.client == nil { return error }
5 (Low, api-contract)	✅ Resolved	truncate(event.NoteBody, 4096) and truncate(instruction, 4096) now applied
6 (Low, pattern-inconsistency)	✅ Resolved	--forge flag marked required via cmd.MarkFlagRequired("forge")

The implementation is well-structured: watermark-based polling with at-least-once delivery, label state diffing, deny-by-default fork detection via IsFork field, and fail-closed authorization (resolveActorRole defaults to "none"). Test suite is thorough (83 tests, 91.8% coverage). Five low-severity findings remain for follow-up.

---

Findings

Low

#	Category	File	Description
1	api-contract	internal/poll/events.go	discoverSlashCommands omits MR context and Labels for all events. The fast-poll path creates RoutableEvent without MRSource, MRTarget, SourceBranch, TargetBranch, MRAuthorID, or Labels. For mr_note events, buildChangeProposalState will fail silently (MRSource=0). For all events, Labels is nil — Go marshals nil slices as JSON null, which violates the NormalizedEvent schema's state.labels type requirement ("type": "array"). The fast-poll path cannot run yet (nil client guard), so this is latent. ADR 0067 documents the intentional omission of MR context in fast-poll, with deny-by-default handling via isForkMR. The nil-Labels schema violation should still be fixed (e.g., initialize to []string{}).
2	api-contract	internal/poll/convert.go	truncate operates on bytes, not Unicode code points. len(s) counts bytes; the schema's maxLength: 4096 refers to code points. Byte truncation is conservative (never exceeds the limit), but s[:maxLen] can split multi-byte UTF-8 characters, producing invalid UTF-8. Use []rune conversion for correct truncation.
3	correctness	internal/poll/events.go	Bot detection inconsistency between poll modes. discoverSlashCommands uses isProjectAccessTokenBot(username) while discoverAllEvents uses Author.Bot. The GitLab Events API may not reliably populate the Bot field, making this an intentional tradeoff documented in the design. Blast radius is limited to slash commands in fast-poll mode.
4	dead-code	internal/poll/events.go	isForkMR defined but never called. The function correctly handles zero-value deny-by-default (returns true when MRSource==0), but no call site exists in Phase 2. The inline IsFork computation in buildChangeProposalState uses event.MRSource != event.MRTarget without the zero-value guard. Expected for phased implementation; wire in Phase 3.
5	docs-currency	docs/cli/README.md	New fullsend poll command not documented. docs/cli/README.md does not list the poll command, and no docs/cli/poll.md exists. The existing CLI docs follow a per-command markdown file pattern. Documentation may be intentionally deferred to a later phase.

---

Reviewed dimensions: correctness (opus), security (opus), intent & coherence, style & conventions, docs currency. Findings adjudicated by challenger pass. Prior review provenance: app-verified.

---

Labels: PR adds GitLab polling infrastructure under internal/poll/ and dispatch event types under internal/dispatch/

Previous run (4)

Review

Verdict: Approve — all prior findings resolved; remaining findings are low severity.

This is a re-review (prior SHA: f0ccd41). All six findings from the prior review have been addressed:

Prior #	Status	Resolution
1 (Medium, api-contract)	✅ Resolved	buildChangeProposalState now populates HeadRef, BaseRef, AuthorID from event.SourceBranch, event.TargetBranch, event.MRAuthorID
2 (Medium, error-handling)	✅ Resolved	buildEventPayload returns ([]byte, error) — no more panic()
3 (Medium, injection-defense)	✅ Resolved	validStage regex ^[a-z][a-z0-9-]*$ validates stage names before path interpolation
4 (Low, correctness)	✅ Resolved	Run() has nil guard: if p.client == nil { return error }
5 (Low, api-contract)	✅ Resolved	truncate(event.NoteBody, 4096) and truncate(instruction, 4096) now applied
6 (Low, pattern-inconsistency)	✅ Resolved	--forge flag marked required via cmd.MarkFlagRequired("forge")

The implementation is well-structured: watermark-based polling with at-least-once delivery, label state diffing, deny-by-default fork detection via IsFork field, and fail-closed authorization (resolveActorRole defaults to "none"). Test suite is thorough (83 tests, 91.8% coverage). Five low-severity findings remain for follow-up.

---

Findings

Low

#	Category	File	Description
1	api-contract	internal/poll/events.go	discoverSlashCommands omits MR context and Labels for all events. The fast-poll path creates RoutableEvent without MRSource, MRTarget, SourceBranch, TargetBranch, MRAuthorID, or Labels. For mr_note events, buildChangeProposalState will fail silently (MRSource=0). For all events, Labels is nil — Go marshals nil slices as JSON null, which violates the NormalizedEvent schema's state.labels type requirement ("type": "array"). The fast-poll path cannot run yet (nil client guard), so this is latent. ADR 0067 documents the intentional omission of MR context in fast-poll, with deny-by-default handling via isForkMR. The nil-Labels schema violation should still be fixed (e.g., initialize to []string{}).
2	api-contract	internal/poll/convert.go	truncate operates on bytes, not Unicode code points. len(s) counts bytes; the schema's maxLength: 4096 refers to code points. Byte truncation is conservative (never exceeds the limit), but s[:maxLen] can split multi-byte UTF-8 characters, producing invalid UTF-8. Use []rune conversion for correct truncation.
3	correctness	internal/poll/events.go	Bot detection inconsistency between poll modes. discoverSlashCommands uses isProjectAccessTokenBot(username) while discoverAllEvents uses Author.Bot. The GitLab Events API may not reliably populate the Bot field, making this an intentional tradeoff documented in the design. Blast radius is limited to slash commands in fast-poll mode.
4	dead-code	internal/poll/events.go	isForkMR defined but never called. The function correctly handles zero-value deny-by-default (returns true when MRSource==0), but no call site exists in Phase 2. The inline IsFork computation in buildChangeProposalState uses event.MRSource != event.MRTarget without the zero-value guard. Expected for phased implementation; wire in Phase 3.
5	docs-currency	docs/cli/README.md	New fullsend poll command not documented. docs/cli/README.md does not list the poll command, and no docs/cli/poll.md exists. The existing CLI docs follow a per-command markdown file pattern. Documentation may be intentionally deferred to a later phase.

---

Reviewed dimensions: correctness (opus), security (opus), intent & coherence, style & conventions, docs currency. Findings adjudicated by challenger pass. Prior review provenance: app-verified.

---

Labels: PR adds GitLab polling infrastructure under internal/poll/ and dispatch event types under internal/dispatch/

Previous run (5)

Review

Verdict: Comment — medium-severity findings worth noting; none blocking.

This PR implements the Phase 2 cron poller for GitLab event dispatch per ADR 0067. The design is sound: watermark-based polling with at-least-once delivery, label state diffing, deny-by-default fork detection, and fail-closed authorization. The test suite is thorough (83 tests, 91.8% coverage). Three medium-severity findings are worth addressing.

---

Findings

Medium

#	Category	File	Description
1	api-contract	internal/poll/convert.go	buildChangeProposalState omits required schema fields. The NormalizedEvent schema requires head_ref, base_ref, and author_id in state.change_proposal (lines 306–307). The function only sets ID, HeadRepo, BaseRepo, and IsFork. MR events will produce schema-invalid NormalizedEvents that fail downstream validation. The MergeRequest struct already has SourceBranch, TargetBranch, and AuthorID fields — they need to flow through RoutableEvent to buildChangeProposalState.
2	error-handling-idiom	internal/poll/dispatch.go	buildEventPayload uses panic() instead of returning an error. While json.Marshal on a struct of primitives is unlikely to fail, panic in a CLI tool that runs in CI produces unstructured stack traces instead of clean errors. Replace with error return and propagate through the dispatch chain.
3	injection-defense	internal/poll/dispatch.go	Stage name not validated before path interpolation. generateChildPipelineYAML interpolates d.Stage into include: .gitlab/ci/fullsend-%s.yml using %s. Stage currently comes from the router (internal constants), but a defense-in-depth validation (e.g., ^[a-z]+$) would prevent path traversal if the Stage provenance changes.

Low

#	Category	File	Description
4	correctness	internal/cli/poll.go	CLI passes nil client and router to poll.New(). The TODO comment acknowledges this as incomplete wiring for Phase 1 integration. Run() will panic on the first client call. Consider adding a nil-guard at the top of Run().
5	api-contract	internal/poll/convert.go	Comment body and instruction are not truncated to the schema's maxLength: 4096. The schema description says adapters should truncate at the transport layer.
6	pattern-inconsistency	internal/cli/poll.go	--forge flag is not marked required with MarkFlagRequired, unlike the established pattern in other CLI commands (19+ usages across the codebase).

---

Reviewed dimensions: correctness (opus), security (opus), intent & coherence, style & conventions, docs currency. Findings adjudicated by challenger pass.

---

Labels: PR adds GitLab polling infrastructure under internal/poll/ and dispatch event types under internal/dispatch/

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 3:43 PM UTC · Ended 3:58 PM UTC
Commit: 2941769 · View workflow run →

[codecov]

Codecov Report

❌ Patch coverage is 84.53453% with 103 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/poll.go	55.55%	22 Missing and 2 partials ⚠️
internal/poll/poll.go	78.30%	13 Missing and 10 partials ⚠️
internal/poll/events.go	87.66%	12 Missing and 7 partials ⚠️
internal/poll/dispatch.go	79.77%	9 Missing and 9 partials ⚠️
internal/poll/convert.go	91.05%	10 Missing and 7 partials ⚠️
internal/poll/state.go	96.96%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:43 PM UTC · Completed 3:58 PM UTC
Commit: 77c4163 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:51 PM UTC · Completed 8:05 PM UTC
Commit: f33be3f · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:54 PM UTC · Completed 12:09 AM UTC
Commit: 3af2476 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:16 AM UTC · Completed 12:24 AM UTC
Commit: 5d9cc3b · View workflow run →

[waynesun09]

Review squad pass (3 agents: Claude, Claude, Gemini) cross-referencing this PR against ADR-0067. Findings not already covered by the 31 existing review comments — posted inline (1 CRITICAL, 3 MEDIUM). YAML-injection mitigation and prompt-injection surface were independently verified as correctly implemented per the ADR and are not flagged.

[waynesun09]

[CRITICAL] Watermark has a stationary fixed point that causes perpetual re-dispatch of the same event every poll cycle

Run() sets the new watermark as maxUpdatedAt.Add(-30 * time.Second), where maxUpdatedAt is the timestamp of the most recently processed event. On the next poll cycle, event discovery includes anything with CreatedAt/UpdatedAt not before the watermark (see events.go — note.CreatedAt.Before(since) and the MR-merge equivalent mr.MergedAt.After(since)). Because the persisted watermark is always exactly 30s before the last event's own timestamp, that same event's timestamp T always satisfies T >= T-30s, so it is re-discovered, re-converted, re-routed, and re-dispatched on every subsequent poll cycle — not just once. If no new activity occurs on the project after the last event, the watermark stays frozen at T-30s forever and the same slash command / MR-merge / MR-note event keeps re-triggering its agent stage on every single poll cycle (every 5–60 min per the ADR schedule) until a genuinely newer event more than 30s later arrives. deduplicate() only dedupes within a single Run() invocation (in-memory map) — there is no persisted set of already-processed event keys to prevent cross-cycle re-dispatch.

Suggestion: Persist a small set of recently-dispatched event keys (similar to the label-state CI variable) and skip re-dispatch for keys already recorded, independent of watermark position. Alternatively/additionally, advance the watermark using a value that isn't self-referential when idle (e.g., only subtract the 30s overlap from event timestamps strictly newer than the previous watermark, and otherwise leave the watermark at now() - overlap when no events were found). Add a test that calls Run() twice in succession with no new activity between calls and asserts the second call produces zero dispatches.

[waynesun09]

[MEDIUM] Fork-MR protection for unknown project IDs relies entirely on downstream (not-in-this-PR) behavior, with no explicit deny-by-default check or test here

The ADR states fork protection is "deny-by-default when unknown," and specifically calls out the fast-poll path as not fetching MRSource/MRTarget. When these are 0 (fast-poll mr_note events), buildChangeProposalState calls GetProjectPath(ctx, 0), which errors, leaving ne.State.ChangeProposal as nil (only a warning is logged) while the NormalizedEvent is still emitted and passed to router.Route(). Whether this actually blocks the fix/code stage depends entirely on the (not-in-this-PR) harness CEL trigger correctly treating a missing state.change_proposal as non-matching rather than erroring/matching by default. A prior review round flagged that no explicit fork-check (isForkMR) existed in the poll package itself; that helper has since been removed from the diff entirely rather than wired in, and there's no test in this PR exercising buildChangeProposalState with MRSource == 0 to assert the nil-ChangeProposal fail path.

Suggestion: Either add an explicit, testable deny-by-default check in the poll package (reinstate isForkMR and wire it into the stage-filtering path), or add a unit test asserting a zero-value MRSource/MRTarget event produces a nil ChangeProposal, plus a link to the specific CEL trigger test (in the dispatch-core PR) verifying a missing change_proposal is treated as non-match for fix/code stages — so the "deny-by-default" claim is verifiable rather than assumed.

[waynesun09]

[MEDIUM] premature-decision — "shared dispatch core, not duplicated" is structurally correct but currently unverifiable — no implementation exists anywhere in the repo yet

Poller correctly depends on the dispatch.EventRouter interface and calls p.router.Route(&normalizedEvent) rather than hardcoding any event→stage mapping itself (verified: no stage-name string literals appear anywhere in internal/poll/*.go). However, a repo-wide search confirms fullsend dispatch does not exist as a CLI command, and no implementation of ADR-0054 authorization or ADR-0061 CEL trigger evaluation exists in the codebase at all (pre- or post- this PR). internal/cli/poll.go:62 wires poll.New(nil, nil, ...) unconditionally — there is no existing dispatch path for this PR to reuse, so the ADR's claim that the dispatch core "is shared... not duplicated" can't actually be verified by this PR; it's an assumption about a component that doesn't exist yet. The only thing pinning the expected EventRouter contract is a hand-written stub in poll_test.go.

Suggestion: Not a blocker for this PR (correctly scoped as poll-driver-only), but track explicitly as a follow-up gate: when the real dispatch core lands, validate it against dispatch.EventRouter as defined here, including a contract/integration test that exercises Poller.Run() against the real router (not just fixtures), to confirm the "not duplicated" claim actually holds once both sides exist.

[waynesun09]

[MEDIUM] premature-decision — bot detection via username heuristic instead of the API's authoritative Bot field

isProjectAccessTokenBot (used by the fast-poll path via the GitLab Events API) classifies an author as a bot by pattern-matching the username (strings.HasPrefix(username, "project_") && strings.Contains(username, "_bot_")), rather than using the Bot bool field already present on the same UserRef struct. Every other event-discovery path in this PR (discoverAllEvents, using the Notes API) uses the authoritative Bot field directly. There's no comment explaining why the Events API path can't do the same, which suggests this is an untested assumption about GitLab's Events API response shape. If the Events API does populate bot reliably, this heuristic is unnecessary and can misclassify a human user whose username happens to match the pattern (silently dropping their event), or fail to recognize a differently-named bot account.

Suggestion: Use the Bot field for consistency with the other paths, unless there's a verified/documented reason the Events API omits or misreports it for project access token users — in which case add a comment citing that GitLab behavior and a test asserting the fallback triggers only when Bot is false/absent.