feat(poll): implement Phase 2 cron poller for GitLab event dispatch by ggallen · Pull Request #4100 · fullsend-ai/fullsend

ggallen · 2026-07-11T11:24:37Z

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 · 2026-07-11T11:25:35Z

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

qodo-code-review · 2026-07-11T11:25:43Z

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

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 · 2026-07-11T11:29:24Z

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 · 2026-07-11T11:37:53Z

Review

This is a re-review (prior SHA: 826a05a). Code rebased (18 commits between prior and current SHA); all files re-evaluated. One prior low-severity finding removed (false positive confirmed by code verification).

Prior Finding	Status
High: `missing-authorization` (no linked issue)	⚠ Unchanged
Low: `error-handling` (at-least-once delivery)	⚠ Unchanged
Low: `architectural-alignment` (GitLabClient vs forge.Client)	⚠ Unchanged
Low: `edge-case` (discoverSlashCommands MR fields)	⚠ Unchanged
Low: `edge-case` (NoteAuthorID overload)	⚠ Unchanged
Low: `logic-error` (bot detection in toNormalizedEvent)	✅ Removed — false positive; all three bot detection signals present for mr_event
Low: `schema-divergence` (dispatch.NormalizedEvent)	⚠ Unchanged
Low: `fail-open` (resolveActorRole)	⚠ Unchanged
Low: `bot-filtering-consistency`	⚠ Unchanged
Low: `injection-defense` (YAML concatenation)	⚠ Unchanged
Low: `data-exposure` (CI variable size)	⚠ Unchanged

Findings

High

[missing-authorization] — No linked issue. This PR implements a major new feature (Phase 2 of ADR 0067, 4400+ lines, 16 files, new internal/poll/ package). Non-trivial changes require explicit authorization via a linked issue. The PR body references ADR 0067 but no issue tracks or authorizes this work.
Remediation: Link this PR to an issue that authorizes the implementation of ADR 0067 Phase 2.

Low

[error-handling] internal/poll/poll.go:170 — Dispatches are written to OutputPath before persistDispatchedKeys is called. If key persistence fails, events re-dispatch on the next cycle. The inline comment documents this as intentional at-least-once delivery semantics.
[architectural-alignment] internal/poll/client.go:237 — GitLabClient interface defines 13 GitLab-specific API methods separate from forge.Client. The interface comment and ADR 0067 justify this separation: the methods are GitLab-specific with no forge-neutral equivalent.
[edge-case] internal/poll/events.go:196 — discoverSlashCommands creates mr_note events without MR-specific fields. buildChangeProposalState logs a WARNING and sets ChangeProposal to nil. Routers MUST treat nil as "unknown" and deny fork-sensitive stages by default.
[edge-case] internal/poll/convert.go:75 — NoteAuthorID field overloaded for mr_event: holds the merged-by user's ID rather than a note author. Functionally correct but fragile for refactoring.
[nil-handling] internal/poll/poll.go:160 — When p.router is nil, events are silently consumed and the watermark advances past them. The nil-client guard prevents this scenario today, but if a client is wired before the router, events would be permanently lost.
[logic-error] internal/poll/events.go:96 — filterBotEvents drops all bot-merged MR events (only retains mr_note type with changes-requested marker). The function comment documents this behavior as intentional.
[schema-divergence] internal/dispatch/event.go:1 — dispatch.NormalizedEvent duplicates the existing normevent.Event schema with plain strings instead of typed enums. Code comment documents this as intentional.
[fail-open] internal/poll/convert.go:563 — resolveActorRole returns "none" on API error. Actually fail-closed (least-privileged). Persistent failures logged as WARNING.
[bot-filtering-consistency] internal/poll/events.go:105 — filterBotEvents changes-requested exception only checks NoteAuthorID == botUserID, not the isProjectAccessTokenBot heuristic. The narrowness is intentional (enrolled bot ID only).
[injection-defense] internal/poll/dispatch.go:113 — YAML generated via string concatenation. Currently safe: Stage, EventType, and ResourceKey are regex-validated; EventPayloadB64 is base64-encoded and %q-quoted.
[data-exposure] internal/poll/state.go:146 — CI variable size limit risk (10K char cap). Pruning mechanisms exist but no explicit size guard before UpdateCIVariable.

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

Previous run

Review

This is a re-review (prior SHA: d0cb43d). Code rebased; all files re-evaluated. Two prior medium-severity findings downgraded to low by challenger (both documented as intentional design). Two prior low-severity findings removed (unreachable code, bounded residual).

Prior Finding	Status
High: `missing-authorization` (no linked issue)	⚠ Unchanged
Medium: `error-handling` (double-dispatch window)	⬇ Downgraded to low — intentional at-least-once delivery, documented inline
Medium: `architectural-alignment` (GitLabClient vs forge.Client)	⬇ Downgraded to low — methods are genuinely GitLab-specific with no forge-neutral equivalent
Low: `edge-case` (discoverSlashCommands MR fields)	⚠ Unchanged
Low: `edge-case` (persistFailedKeys count==3)	✅ Removed — bounded residual set, not unbounded growth
Low: `edge-case` (NoteAuthorID overload)	⚠ Unchanged
Low: `edge-case` (nil router no dispatches)	✅ Removed — code path unreachable (nil client check exits first)
Low: `schema-divergence` (dispatch.NormalizedEvent)	⚠ Unchanged
Low: `fail-open` (resolveActorRole)	⚠ Unchanged
Low: `bot-filtering-consistency`	⚠ Unchanged
Low: `injection-defense` (YAML concatenation)	⚠ Unchanged
Low: `data-exposure` (CI variable size)	⚠ Unchanged
Low: `authorization` (buildChangeProposalState)	⚠ Merged into edge-case finding
Low: `docs-staleness` (poll command undocumented)	✅ Removed — command is hidden and incomplete; premature to document

Findings

High

[missing-authorization] — No linked issue. This PR implements a major new feature (Phase 2 of ADR 0067, 4400+ lines, 16 files, new internal/poll/ package). Non-trivial changes require explicit authorization via a linked issue. The PR body references ADR 0067 but no issue tracks or authorizes this work.
Remediation: Link this PR to an issue that authorizes the implementation of ADR 0067 Phase 2.

Low

[error-handling] internal/poll/poll.go:170 — Dispatches are written to OutputPath before persistDispatchedKeys is called. If key persistence fails, events re-dispatch on the next cycle. The inline comment documents this as intentional at-least-once delivery semantics.
[architectural-alignment] internal/poll/client.go:12 — GitLabClient interface defines 13 GitLab-specific API methods separate from forge.Client. The interface comment justifies this: the methods (ListProjectEvents, ListResourceLabelEvents, GetCIVariable, etc.) are GitLab-specific with no forge-neutral equivalent. An architectural discussion, not a clear violation.
[edge-case] internal/poll/events.go:196 — discoverSlashCommands creates mr_note events without MR-specific fields. buildChangeProposalState fails for these, logging a WARNING and setting ChangeProposal to nil. Routers MUST treat nil as "unknown" and deny fork-sensitive stages by default.
[edge-case] internal/poll/convert.go:75 — NoteAuthorID field overloaded for mr_event: holds the merged-by user's ID rather than a note author. Functionally correct but fragile for refactoring.
[logic-error] internal/poll/convert.go:131 — Bot detection in toNormalizedEvent for mr_event only checks event.IsBot, missing botUserID and isProjectAccessTokenBot checks used for other event types. filterBotEvents provides defense-in-depth upstream.
[schema-divergence] internal/dispatch/event.go:1 — dispatch.NormalizedEvent duplicates the existing normevent.Event schema with plain strings instead of typed enums. Code comment documents this as intentional.
[fail-open] internal/poll/convert.go:160 — resolveActorRole returns "none" on API error. Actually fail-closed (least-privileged). Persistent failures logged as WARNING.
[bot-filtering-consistency] internal/poll/events.go:195 — filterBotEvents changes-requested exception only checks NoteAuthorID == botUserID, not the isProjectAccessTokenBot heuristic. The narrowness is intentional (enrolled bot ID only).
[injection-defense] internal/poll/dispatch.go:113 — YAML generated via string concatenation. Currently safe: Stage, EventType, and ResourceKey are regex-validated; EventPayloadB64 is base64-encoded and %q-quoted.
[data-exposure] internal/poll/state.go:146 — CI variable size limit risk (10K char cap). Pruning mechanisms exist but no explicit size guard before UpdateCIVariable.

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

Previous run (2)

Review

This is a re-review (prior SHA: a55b7b5). Code rebased; all files re-evaluated. Prior resolved findings remain fixed. Two new medium-severity findings identified.

Prior Finding	Status
Medium: `unbounded-state` (persistFailedKeys boundary bug)	✅ Fixed — condition `count <= maxEventRetries`
Medium: `bot-filtering-bypass` (discoverSlashCommands bot detection)	✅ Fixed — all three bot detection signals used
Low: `nil-panic` (poll command visible but non-functional)	✅ Fixed — `cmd.Hidden = true` set

Findings

High

[missing-authorization] — No linked issue. This PR implements a major new feature (Phase 2 of ADR 0067, 4400+ lines, 16 files, new internal/poll/ package). Non-trivial changes require explicit authorization via a linked issue. The PR body references ADR 0067 but no issue tracks or authorizes this work.
Remediation: Link this PR to an issue that authorizes the implementation of ADR 0067 Phase 2.

Medium

[error-handling] internal/poll/poll.go:170 — Double-dispatch window. Dispatches are written to OutputPath (line 170) before persistDispatchedKeys is called (line 198). If persistDispatchedKeys fails, the child pipeline YAML has already been written but dispatched keys are not persisted. The next poll cycle will re-dispatch the same events.
Remediation: Persist dispatched keys before writing the output file, or document this as an at-least-once delivery guarantee.
[architectural-alignment] internal/poll/client.go — GitLabClient interface defines 13 GitLab-specific API methods that parallel existing forge.Client methods (e.g., GetIssue, UpdateCIVariable, GetAuthenticatedUser). Per AGENTS.md: "All git forge operations must go through the forge.Client interface." This creates a bifurcated forge abstraction.
Remediation: Add the required methods to forge.Client interface, or document why a separate interface is architecturally necessary and provide a convergence plan.

Low

[edge-case] internal/poll/events.go:192 — discoverSlashCommands creates mr_note events without MR-specific fields (MRSource, MRTarget, SourceBranch, TargetBranch, MRAuthorID, MRAuthorLogin). In fast-poll mode, MR slash commands produce degraded NormalizedEvents with no fork safety metadata.
[schema-divergence] internal/dispatch/event.go — dispatch.NormalizedEvent duplicates the existing normevent.Event schema. Both represent forge-neutral routing events but with different type systems (plain strings vs typed enums). Code comment documents this as intentional.
[edge-case] internal/poll/state.go:207 — persistFailedKeys retains entries at exactly maxEventRetries (count=3) permanently. These accumulate linearly with permanently-failing events.
[edge-case] internal/poll/convert.go:75 — NoteAuthorID field overloaded for mr_event. For merge events, carries the merged-by user's ID. Functionally correct but fragile for refactoring.
[edge-case] internal/poll/poll.go:112 — Nil router silently produces no dispatches. No log message when events dropped due to missing router.
[fail-open] internal/poll/convert.go:160 — resolveActorRole returns "none" on API error. Actually fail-closed (least-privileged). Persistent failures logged as WARNING.
[bot-filtering-consistency] internal/poll/convert.go:62 — Minor bot detection inconsistency for notes passing through the filterBotEvents changes-requested exception.
[injection-defense] internal/poll/dispatch.go:113 — YAML generated via string concatenation. Currently safe — Stage, EventType, and ResourceKey are regex-validated; EventPayloadB64 is base64-encoded and %q-quoted.
[data-exposure] internal/poll/state.go:146 — CI variable size limit risk. Dispatched keys, failed keys, and label state serialized to GitLab CI variables (10K char cap). Pruning mechanisms exist but no explicit size guard.
[docs-staleness] docs/cli/README.md — New fullsend poll command undocumented. Command is intentionally hidden (cmd.Hidden = true) with nil client guard. Documentation may be intentionally deferred.
[authorization] internal/poll/convert.go:106 — buildChangeProposalState failure is non-fatal. Logs warning, continues with nil ChangeProposal. Routers must treat nil as "unknown" and deny fork-sensitive stages by default.

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

Previous run (3)

Review

This is a re-review (prior SHA: 03e61fc). Four files changed since the prior review. Two prior medium-severity findings resolved; one prior low-severity finding resolved.

Prior Finding	Status
Medium: `unbounded-state` (persistFailedKeys boundary bug)	✅ Fixed — condition changed to `count <= maxEventRetries`
Medium: `bot-filtering-bypass` (discoverSlashCommands bot detection)	✅ Fixed — all three bot detection signals now used
Low: `nil-panic` (poll command visible but non-functional)	✅ Fixed — `cmd.Hidden = true` set

Findings

High

[missing-authorization] — No linked issue. This PR implements a major new feature (Phase 2 of ADR 0067, 4400+ lines, 16 files, new internal/poll/ package). Per the intent authorization tier framework, non-trivial changes require explicit authorization via a linked issue. The PR body references ADR 0067 but no issue tracks or authorizes this work.
Remediation: Link this PR to an issue that authorizes the implementation of ADR 0067 Phase 2.

Low

[edge-case] internal/poll/convert.go:75 — NoteAuthorID field overloaded for mr_event. For merge events, carries the merged-by user's ID. Functionally correct but fragile for refactoring.
[fail-open] internal/poll/convert.go:163 — resolveActorRole returns "none" on API error. Actually fail-closed (least-privileged). Persistent failures go unnoticed (only log.Printf WARNING).
[edge-case] internal/poll/poll.go:47 — Nil router silently produces no dispatches. Intentional for Phase 1 stub.
[schema-contract] internal/dispatch/event.go:8 — NormalizedEvent uses plain strings instead of typed enums. Documented as intentional design decision.
[authorization] internal/poll/convert.go:106 — buildChangeProposalState failure is non-fatal. Logs warning, continues with nil ChangeProposal. Routers must treat nil as "unknown" and deny fork-sensitive stages by default.
[bot-filtering-consistency] internal/poll/convert.go:62 — Minor bot detection inconsistency in toNormalizedEvent. For notes passing through the filterBotEvents changes-requested exception, Actor.Kind may be set to "human" because isBot = event.IsBot relies solely on the Notes API Bot field.
[injection-defense] internal/poll/dispatch.go:113 — YAML generated via string concatenation. Currently safe — Stage, EventType, and ResourceKey are regex-validated; EventPayloadB64 is base64-encoded and %q-quoted.
[data-exposure] internal/poll/state.go:146 — CI variable size limit risk. Dispatched keys serialized to GitLab CI variables (10K char cap). persistDispatchedKeys prunes entries older than the watermark.
[docs-staleness] docs/cli/README.md — New fullsend poll command undocumented. Command is intentionally hidden (cmd.Hidden = true) with nil client guard. Documentation may be intentionally deferred.

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

Previous run (4)

Review

This is a re-review — the head SHA is unchanged from the prior review (03e61fc). All prior findings remain at their assessed severities. No code changes to evaluate.

Prior Finding Status

Prior #	Status	Notes
1 (High, missing-authorization)	⚠ Unchanged	No linked issue; PR references ADR 0067 but no issue authorizes this work
2 (Medium, unbounded-state, persistFailedKeys boundary bug)	⚠ Unchanged	Condition `count < maxEventRetries` still drops keys at exactly `maxEventRetries`
3 (Medium, bot-filtering-bypass, bot detection inconsistency)	⚠ Unchanged	`discoverSlashCommands` ignores `evt.Author.Bot` and `botUserID`
4–15 (Low)	⚠ Unchanged	All low-severity findings unchanged

Findings

High

#	Category	File	Description
1	`missing-authorization`	N/A	No linked issue. This PR implements a major new feature (Phase 2 of ADR 0067, 4400+ lines, 16 files). Non-trivial changes require explicit authorization via a linked issue per the intent authorization tier framework. The PR body references ADR 0067 but no issue tracks or authorizes this work. Remediation: Link this PR to an issue that authorizes the implementation of ADR 0067 Phase 2.

Medium

#	Category	File	Description
2	`unbounded-state`	`internal/poll/state.go:210`	`persistFailedKeys` pruning condition boundary bug. The condition `count > 0 && count < maxEventRetries` drops keys at exactly `maxEventRetries` (3). This causes infinite retry oscillation: count reaches 3 → pruned from storage → next poll starts at 0 → fails again → cycle repeats (0→1→2→3(pruned)→0→...). The `minFailedAt` guard prevents watermark advancement, so these events loop forever. Remediation: Change condition to `count > 0 && count <= maxEventRetries` to persist exhausted keys permanently.
3	`bot-filtering-bypass`	`internal/poll/events.go:199`	Bot detection inconsistency in fast-poll mode. `discoverSlashCommands` sets `IsBot` solely from `isProjectAccessTokenBot(evt.Author.Username)`, ignoring both the `evt.Author.Bot` API field and the configured `botUserID`. In contrast, `discoverAllEvents` uses `note.Author.Bot` directly. A bot with a non-PAT-pattern username where only `Author.Bot=true` would bypass bot filtering in fast-poll mode. Remediation: Set `IsBot: isProjectAccessTokenBot(evt.Author.Username)

Low

#	Category	File	Description
4	`nil-panic`	`internal/cli/poll.go:27`	Poll command visible but non-functional. The command passes nil client and router to `poll.New()`. `Run()` returns a descriptive error, so there is no crash risk. However, the command appears in `fullsend --help`. Set `cmd.Hidden = true` until Phase 1 wiring is complete.
5	`injection-defense`	`internal/poll/dispatch.go:113`	YAML generated via string concatenation. Currently safe — Stage, EventType, and ResourceKey are regex-validated; EventPayloadB64 is base64-encoded and `%q`-quoted. Consider migrating to `encoding/yaml` when adding new fields.
6	`data-exposure`	`internal/poll/state.go:146`	CI variable size limit risk. Dispatched keys serialized to GitLab CI variables (10K char cap). `persistDispatchedKeys` prunes by watermark. Burst scenarios within a single poll cycle could theoretically exceed the limit but are unlikely for a cron poller.
7	`edge-case`	`internal/poll/convert.go:116`	`NoteAuthorID` field overloaded for `mr_event`. For merge events, carries the merged-by user's ID. Functionally correct but fragile for refactoring.
8	`fail-open`	`internal/poll/convert.go:198`	`resolveActorRole` returns `"none"` on API error. Actually fail-closed (least-privileged). Persistent failures go unnoticed (no logging).
9	`edge-case`	`internal/poll/poll.go:47`	Nil router silently produces no dispatches. `Run()` checks nil client but not nil router. Intentional for Phase 1 stub.
10	`schema-contract`	`internal/dispatch/event.go:8`	`NormalizedEvent` uses plain strings instead of typed enums. Documented as intentional design decision.
11	`scope-mismatch`	`internal/cli/poll.go:63`	PR title says "implement Phase 2" but Phase 1 wiring is pending. Phase 2 per ADR 0067 refers to the poll logic itself, which is complete with 91.8% coverage.
12	`authorization`	`internal/poll/convert.go:455`	`buildChangeProposalState` failure is non-fatal. Logs warning, continues with nil `ChangeProposal`. Routers MUST treat nil as "unknown" and deny fork-sensitive stages by default.
13	`secrets-handling`	`internal/cli/poll.go:36`	`FULLSEND_FORGE_TOKEN` correctly handled. Read from environment, validated non-empty, currently discarded. No exposure risk.
14	`docs-staleness`	`docs/cli/README.md`	New `fullsend poll` command not documented. CLI command, subcommand, environment variables, and 7 CI state variables are undocumented. Documentation may be intentionally deferred since the command cannot run yet (nil client guard).
15	`pattern-inconsistency`	`internal/poll/client.go`	Godoc comments omit trailing periods. The codebase consistently uses trailing periods on godoc comments. The new `internal/poll/` package deviates from this convention throughout.

Reviewed dimensions: correctness, security, intent & coherence, style & conventions, docs currency. Re-review on unchanged SHA — findings carried forward from prior app-verified review.

Previous run (5)

Review

This is a re-review — the head SHA is unchanged from the prior review (03e61fc). All prior findings remain at their assessed severities. No code changes to evaluate.

Prior Finding Status

Prior #	Status	Notes
1 (High, missing-authorization)	⚠ Unchanged	No linked issue; PR references ADR 0067 but no issue authorizes this work
2 (Medium, unbounded-state, persistFailedKeys boundary bug)	⚠ Unchanged	Condition `count < maxEventRetries` still drops keys at exactly `maxEventRetries`
3 (Medium, bot-filtering-bypass, bot detection inconsistency)	⚠ Unchanged	`discoverSlashCommands` ignores `evt.Author.Bot` and `botUserID`
4–15 (Low)	⚠ Unchanged	All low-severity findings unchanged

Findings

High

#	Category	File	Description
1	`missing-authorization`	N/A	No linked issue. This PR implements a major new feature (Phase 2 of ADR 0067, 4400+ lines, 16 files). Non-trivial changes require explicit authorization via a linked issue per the intent authorization tier framework. The PR body references ADR 0067 but no issue tracks or authorizes this work. Remediation: Link this PR to an issue that authorizes the implementation of ADR 0067 Phase 2.

Medium

#	Category	File	Description
2	`unbounded-state`	`internal/poll/state.go:210`	`persistFailedKeys` pruning condition boundary bug. The condition `count > 0 && count < maxEventRetries` drops keys at exactly `maxEventRetries` (3). This causes infinite retry oscillation: count reaches 3 → pruned from storage → next poll starts at 0 → fails again → cycle repeats (0→1→2→3(pruned)→0→...). The `minFailedAt` guard prevents watermark advancement, so these events loop forever. Remediation: Change condition to `count > 0 && count <= maxEventRetries` to persist exhausted keys permanently.
3	`bot-filtering-bypass`	`internal/poll/events.go:199`	Bot detection inconsistency in fast-poll mode. `discoverSlashCommands` sets `IsBot` solely from `isProjectAccessTokenBot(evt.Author.Username)`, ignoring both the `evt.Author.Bot` API field and the configured `botUserID`. In contrast, `discoverAllEvents` uses `note.Author.Bot` directly. A bot with a non-PAT-pattern username where only `Author.Bot=true` would bypass bot filtering in fast-poll mode. Remediation: Set `IsBot: isProjectAccessTokenBot(evt.Author.Username)

Low

#	Category	File	Description
4	`nil-panic`	`internal/cli/poll.go:27`	Poll command visible but non-functional. The command passes nil client and router to `poll.New()`. `Run()` returns a descriptive error, so there is no crash risk. However, the command appears in `fullsend --help`. Set `cmd.Hidden = true` until Phase 1 wiring is complete.
5	`injection-defense`	`internal/poll/dispatch.go:113`	YAML generated via string concatenation. Currently safe — Stage, EventType, and ResourceKey are regex-validated; EventPayloadB64 is base64-encoded and `%q`-quoted. Consider migrating to `encoding/yaml` when adding new fields.
6	`data-exposure`	`internal/poll/state.go:146`	CI variable size limit risk. Dispatched keys serialized to GitLab CI variables (10K char cap). `persistDispatchedKeys` prunes by watermark. Burst scenarios within a single poll cycle could theoretically exceed the limit but are unlikely for a cron poller.
7	`edge-case`	`internal/poll/convert.go:116`	`NoteAuthorID` field overloaded for `mr_event`. For merge events, carries the merged-by user's ID. Functionally correct but fragile for refactoring.
8	`fail-open`	`internal/poll/convert.go:198`	`resolveActorRole` returns `"none"` on API error. Actually fail-closed (least-privileged). Persistent failures go unnoticed (no logging).
9	`edge-case`	`internal/poll/poll.go:47`	Nil router silently produces no dispatches. `Run()` checks nil client but not nil router. Intentional for Phase 1 stub.
10	`schema-contract`	`internal/dispatch/event.go:8`	`NormalizedEvent` uses plain strings instead of typed enums. Documented as intentional design decision.
11	`scope-mismatch`	`internal/cli/poll.go:63`	PR title says "implement Phase 2" but Phase 1 wiring is pending. Phase 2 per ADR 0067 refers to the poll logic itself, which is complete with 91.8% coverage.
12	`authorization`	`internal/poll/convert.go:455`	`buildChangeProposalState` failure is non-fatal. Logs warning, continues with nil `ChangeProposal`. Routers MUST treat nil as "unknown" and deny fork-sensitive stages by default.
13	`secrets-handling`	`internal/cli/poll.go:36`	`FULLSEND_FORGE_TOKEN` correctly handled. Read from environment, validated non-empty, currently discarded. No exposure risk.
14	`docs-staleness`	`docs/cli/README.md`	New `fullsend poll` command not documented. CLI command, subcommand, environment variables, and 7 CI state variables are undocumented. Documentation may be intentionally deferred since the command cannot run yet (nil client guard).
15	`pattern-inconsistency`	`internal/poll/client.go`	Godoc comments omit trailing periods. The codebase consistently uses trailing periods on godoc comments. The new `internal/poll/` package deviates from this convention throughout.

Reviewed dimensions: correctness (opus), security (opus), intent & coherence, style & conventions, docs currency. Re-review on unchanged SHA — findings carried forward from prior app-verified review.

Previous run (6)

Review

This is a re-review (prior SHA: 1b6a3be). All files re-evaluated against new head SHA 03e61fc. Three prior medium findings downgraded by challenger pass (nil-panic, injection-defense, data-exposure). One prior low finding upgraded to medium (bot-filtering-bypass). One new high finding identified (missing-authorization).

Prior Finding Status

Prior #	Status	Notes
1 (Medium, unbounded-state, persistFailedKeys boundary bug)	⚠ Unchanged	Condition `count < maxEventRetries` still drops keys at exactly `maxEventRetries`
2 (Medium, nil-panic, poll command visible)	⬇ Downgraded to low	No actual panic risk; command returns descriptive error safely
3 (Medium, injection-defense, YAML string concatenation)	⬇ Downgraded to low	All interpolated fields regex-validated; currently safe
4 (Medium, data-exposure, CI variable size limit)	⬇ Downgraded to low	Pruning mechanism exists; burst scenario unrealistic for cron poller
5–13 (Low)	⚠ Unchanged	All low-severity findings unchanged

Findings

High

#	Category	File	Description
1	`missing-authorization`	N/A	No linked issue. This PR implements a major new feature (Phase 2 of ADR 0067, 4400+ lines, 16 files). Non-trivial changes require explicit authorization via a linked issue per the intent authorization tier framework. The PR body references ADR 0067 but no issue tracks or authorizes this work. Remediation: Link this PR to an issue that authorizes the implementation of ADR 0067 Phase 2.

Medium

#	Category	File	Description
2	`unbounded-state`	`internal/poll/state.go:210`	`persistFailedKeys` pruning condition boundary bug. The condition `count > 0 && count < maxEventRetries` drops keys at exactly `maxEventRetries` (3). This causes infinite retry oscillation: count reaches 3 → pruned from storage → next poll starts at 0 → fails again → cycle repeats (0→1→2→3(pruned)→0→...). The `minFailedAt` guard prevents watermark advancement, so these events loop forever. Remediation: Change condition to `count > 0 && count <= maxEventRetries` to persist exhausted keys permanently.
3	`bot-filtering-bypass`	`internal/poll/events.go:199`	Bot detection inconsistency in fast-poll mode. `discoverSlashCommands` sets `IsBot` solely from `isProjectAccessTokenBot(evt.Author.Username)`, ignoring both the `evt.Author.Bot` API field and the configured `botUserID`. In contrast, `discoverAllEvents` uses `note.Author.Bot` directly. A bot with a non-PAT-pattern username where only `Author.Bot=true` would bypass bot filtering in fast-poll mode. Remediation: Set `IsBot: isProjectAccessTokenBot(evt.Author.Username)

Low

#	Category	File	Description
4	`nil-panic`	`internal/cli/poll.go:27`	Poll command visible but non-functional. The command passes nil client and router to `poll.New()`. `Run()` returns a descriptive error, so there is no crash risk. However, the command appears in `fullsend --help`. Set `cmd.Hidden = true` until Phase 1 wiring is complete.
5	`injection-defense`	`internal/poll/dispatch.go:113`	YAML generated via string concatenation. Currently safe — Stage, EventType, and ResourceKey are regex-validated; EventPayloadB64 is base64-encoded and `%q`-quoted. Consider migrating to `encoding/yaml` when adding new fields.
6	`data-exposure`	`internal/poll/state.go:146`	CI variable size limit risk. Dispatched keys serialized to GitLab CI variables (10K char cap). `persistDispatchedKeys` prunes by watermark. Burst scenarios within a single poll cycle could theoretically exceed the limit but are unlikely for a cron poller.
7	`edge-case`	`internal/poll/convert.go:116`	`NoteAuthorID` field overloaded for `mr_event`. For merge events, carries the merged-by user's ID. Functionally correct but fragile for refactoring.
8	`fail-open`	`internal/poll/convert.go:198`	`resolveActorRole` returns `"none"` on API error. Actually fail-closed (least-privileged). Persistent failures go unnoticed (no logging).
9	`edge-case`	`internal/poll/poll.go:47`	Nil router silently produces no dispatches. `Run()` checks nil client but not nil router. Intentional for Phase 1 stub.
10	`schema-contract`	`internal/dispatch/event.go:8`	`NormalizedEvent` uses plain strings instead of typed enums. Documented as intentional design decision.
11	`scope-mismatch`	`internal/cli/poll.go:63`	PR title says "implement Phase 2" but Phase 1 wiring is pending. Phase 2 per ADR 0067 refers to the poll logic itself, which is complete with 91.8% coverage.
12	`authorization`	`internal/poll/convert.go:455`	`buildChangeProposalState` failure is non-fatal. Logs warning, continues with nil `ChangeProposal`. Routers MUST treat nil as "unknown" and deny fork-sensitive stages by default.
13	`secrets-handling`	`internal/cli/poll.go:36`	`FULLSEND_FORGE_TOKEN` correctly handled. Read from environment, validated non-empty, currently discarded. No exposure risk.
14	`docs-staleness`	`docs/cli/README.md`	New `fullsend poll` command not documented. CLI command, subcommand, environment variables, and 7 CI state variables are undocumented. Documentation may be intentionally deferred since the command cannot run yet (nil client guard).
15	`pattern-inconsistency`	`internal/poll/client.go`	Godoc comments omit trailing periods. The codebase consistently uses trailing periods on godoc comments. The new `internal/poll/` package deviates from this convention throughout.

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

Previous run (7)

Review

This is a re-review — the head SHA is unchanged from the prior review (1b6a3be). All prior findings remain at their assessed severities. No code changes to evaluate.

Prior Finding Status

Prior #	Status	Notes
1 (Medium, unbounded-state, persistFailedKeys boundary bug)	⚠ Unchanged	Condition `count < maxEventRetries` still drops keys at exactly `maxEventRetries`
2 (Medium, nil-panic, poll command visible but non-functional)	⚠ Unchanged	`cmd.Hidden = true` not set; command still registered
3 (Medium, injection-defense, YAML string concatenation)	⚠ Unchanged	Fields regex-validated; pattern remains fragile for future additions
4 (Medium, data-exposure, CI variable size limit risk)	⚠ Unchanged	No size check before `UpdateCIVariable`
5–13 (Low)	⚠ Unchanged	All low-severity findings unchanged

Findings

Medium

#	Category	File	Description
1	`unbounded-state`	`internal/poll/state.go:182`	`persistFailedKeys` pruning condition boundary bug. The condition `count > 0 && count < maxEventRetries` drops keys at exactly `maxEventRetries` (3). On the next poll cycle, the event will be retried since its key was pruned from storage. Combined with watermark advancement for exhausted events (poll.go:90–95), exhausted events are silently forgotten once the watermark passes them. Remediation: Change condition to `count > 0 && count <= maxEventRetries` to persist exhausted keys, or add a dead-letter mechanism.
2	`nil-panic`	`internal/cli/poll.go:68`	Poll command accessible but non-functional. The command passes nil client and router to `poll.New()`. `Run()` returns a descriptive error ("poller requires a GitLab client (Phase 1 wiring incomplete)"), but the command is registered and visible to users. Remediation: Set `cmd.Hidden = true` until the GitLab client is wired.
3	`injection-defense`	`internal/poll/dispatch.go:113`	YAML generated via string concatenation. `generateChildPipelineYAML` uses `fmt.Fprintf` instead of a YAML library. Currently safe — Stage, EventType, and ResourceKey are regex-validated; EventPayloadB64 is base64-encoded and %q-quoted. Pattern is fragile for future field additions.
4	`data-exposure`	`internal/poll/state.go:146`	CI variable size limit risk. Dispatched keys map can grow unbounded between pruning cycles. GitLab CI variables are capped at 10,000 characters (documented in client.go). A burst of events between polls could exceed this limit, causing state persistence failure and potential duplicate dispatches. Remediation: Add a size check before `UpdateCIVariable`; prune oldest entries by timestamp if serialized data exceeds ~9,500 characters.

Low

#	Category	File	Description
5	`schema-contract`	`internal/dispatch/event.go:128`	`dispatch.NormalizedEvent` uses plain strings instead of `normevent` typed enums. Code comment (lines 125–127) documents this as intentional: "keeps the dispatch package free of normevent dependencies; the poll layer is the only producer, and child pipelines consume JSON." Downgraded from medium — deliberate design decision, not an oversight.
6	`scope-mismatch`	`internal/cli/poll.go:63`	PR title says "implement Phase 2" but Phase 1 wiring is pending. `poll.New(nil, nil, ...)` with `TODO(phase1)` comments. The `generate-child-pipeline` subcommand IS fully functional. Downgraded from medium — "Phase 2" per ADR 0067 refers to the poll logic itself, which is complete with 91.8% coverage.
7	`edge-case`	`internal/poll/events.go:188`	`discoverSlashCommands` omits MR context for `mr_note` events. Events lack MRSource, MRTarget, SourceBranch, TargetBranch, MRAuthorID, MRAuthorLogin. `buildChangeProposalState` will produce nil `ChangeProposal`. ADR 0067 documents this as an accepted tradeoff; routers deny fork-sensitive stages by default when ChangeProposal is nil. Severity anchored at low per prior review.
8	`fail-open`	`internal/poll/events.go:199`	Bot detection inconsistency between poll modes. `discoverSlashCommands` sets `IsBot` from `isProjectAccessTokenBot(username)` only, ignoring `evt.Author.Bot` API field. Downstream `isBotEvent()` provides defense-in-depth via `botUserID` check. Severity anchored at low per prior review.
9	`edge-case`	`internal/poll/convert.go:427`	`NoteAuthorID` field name misleading for `mr_event`. For merge events, `NoteAuthorID` carries the merged-by user's ID (populated from `mergedBy.ID` in `discoverAllEvents`). Functionally correct but fragile for refactoring.
10	`fail-open`	`internal/poll/convert.go:112`	`resolveActorRole` silently returns `"none"` on API error. This is fail-closed (least-privileged) but persistent failures go unnoticed. Consider adding a log warning on error.
11	`edge-case`	`internal/poll/poll.go:47`	Nil router silently produces no dispatches. `Run()` checks nil client but not nil router. With nil router, `stages` stays nil and all events are skipped without logging. Intentional for Phase 1 stub but behavior should be documented.
12	`docs-staleness`	`docs/cli/README.md`	New `fullsend poll` command not documented. CLI command, subcommand (`generate-child-pipeline`), environment variables (FULLSEND_FORGE_TOKEN, FULLSEND_POLL_MODE, CI_PROJECT_PATH), and 7 CI state variables are undocumented. Also missing from `docs/guides/dev/cli-internals.md` command tree. Since the command cannot run yet (nil client guard), documentation may be intentionally deferred. Severity anchored at low per prior review.
13	`pattern-inconsistency`	`internal/dispatch/event.go:123`	Inconsistent punctuation in godoc comments. Some type comments use trailing periods while others omit them. Existing codebase pattern uses periods consistently.

Reviewed dimensions: correctness, security, intent & coherence, style & conventions, docs currency. Re-review on unchanged SHA — findings carried forward from prior app-verified review.

Previous run (8)

Review

This is a re-review (prior SHA: 5ab6a953). Branch was rebased; all files re-evaluated. Two prior medium findings downgraded (intentional design decisions confirmed by code comments). One prior medium finding resolved but replacement pruning logic has a new boundary bug. Two new medium findings identified.

Prior Finding Status

Prior #	Status	Notes
1 (Medium, schema-contract, dispatch.NormalizedEvent)	⬇ Downgraded to low	Code comment explicitly documents intentional design decision (plain strings for JSON child-pipeline consumption)
2 (Medium, scope-mismatch, Phase 2 title)	⬇ Downgraded to low	PR title "Phase 2" is consistent with ADR 0067 phasing; poll logic is fully functional with 83 tests
3 (Medium, unbounded-state, persistFailedKeys pruning)	✅ Resolved, new bug	Prior issue (no pruning) resolved; new boundary condition bug in replacement pruning logic
4 (Low, edge-case, discoverSlashCommands MR context)	⚠ Unchanged	Fast-poll path still omits MR context; severity anchored at low
5 (Low, docs-staleness, poll command undocumented)	⚠ Unchanged	CLI and env vars still undocumented
6 (Low, fail-open, bot detection inconsistency)	⚠ Unchanged	Severity anchored at low

Findings

Medium

#	Category	File	Description
1	`unbounded-state`	`internal/poll/state.go:182`	`persistFailedKeys` pruning condition boundary bug. The condition `count > 0 && count < maxEventRetries` drops keys at exactly `maxEventRetries` (3). On the next poll cycle, the event will be retried since its key was pruned from storage. Combined with watermark advancement for exhausted events (poll.go:90–95), exhausted events are silently forgotten once the watermark passes them. Remediation: Change condition to `count > 0 && count <= maxEventRetries` to persist exhausted keys, or add a dead-letter mechanism.
2	`nil-panic`	`internal/cli/poll.go:68`	Poll command accessible but non-functional. The command passes nil client and router to `poll.New()`. `Run()` returns a descriptive error ("poller requires a GitLab client (Phase 1 wiring incomplete)"), but the command is registered and visible to users. Remediation: Set `cmd.Hidden = true` until the GitLab client is wired.
3	`injection-defense`	`internal/poll/dispatch.go:113`	YAML generated via string concatenation. `generateChildPipelineYAML` uses `fmt.Fprintf` instead of a YAML library. Currently safe — Stage, EventType, and ResourceKey are regex-validated; EventPayloadB64 is base64-encoded and %q-quoted. Pattern is fragile for future field additions.
4	`data-exposure`	`internal/poll/state.go:146`	CI variable size limit risk. Dispatched keys map can grow unbounded between pruning cycles. GitLab CI variables are capped at 10,000 characters (documented in client.go). A burst of events between polls could exceed this limit, causing state persistence failure and potential duplicate dispatches. Remediation: Add a size check before `UpdateCIVariable`; prune oldest entries by timestamp if serialized data exceeds ~9,500 characters.

Low

#	Category	File	Description
5	`schema-contract`	`internal/dispatch/event.go:128`	`dispatch.NormalizedEvent` uses plain strings instead of `normevent` typed enums. Code comment (lines 125–127) documents this as intentional: "keeps the dispatch package free of normevent dependencies; the poll layer is the only producer, and child pipelines consume JSON." Downgraded from medium — deliberate design decision, not an oversight.
6	`scope-mismatch`	`internal/cli/poll.go:63`	PR title says "implement Phase 2" but Phase 1 wiring is pending. `poll.New(nil, nil, ...)` with `TODO(phase1)` comments. The `generate-child-pipeline` subcommand IS fully functional. Downgraded from medium — "Phase 2" per ADR 0067 refers to the poll logic itself, which is complete with 91.8% coverage.
7	`edge-case`	`internal/poll/events.go:188`	`discoverSlashCommands` omits MR context for `mr_note` events. Events lack MRSource, MRTarget, SourceBranch, TargetBranch, MRAuthorID, MRAuthorLogin. `buildChangeProposalState` will produce nil `ChangeProposal`. ADR 0067 documents this as an accepted tradeoff; routers deny fork-sensitive stages by default when ChangeProposal is nil. Severity anchored at low per prior review.
8	`fail-open`	`internal/poll/events.go:199`	Bot detection inconsistency between poll modes. `discoverSlashCommands` sets `IsBot` from `isProjectAccessTokenBot(username)` only, ignoring `evt.Author.Bot` API field. Fix: `IsBot: evt.Author.Bot \|\| isProjectAccessTokenBot(evt.Author.Username)`. Downstream `isBotEvent()` provides defense-in-depth via `botUserID` check. Severity anchored at low per prior review.
9	`edge-case`	`internal/poll/convert.go:427`	`NoteAuthorID` field name misleading for `mr_event`. For merge events, `NoteAuthorID` carries the merged-by user's ID (populated from `mergedBy.ID` in `discoverAllEvents`). Functionally correct but fragile for refactoring.
10	`fail-open`	`internal/poll/convert.go:112`	`resolveActorRole` silently returns `"none"` on API error. This is fail-closed (least-privileged) but persistent failures go unnoticed. Consider adding a log warning on error.
11	`edge-case`	`internal/poll/poll.go:47`	Nil router silently produces no dispatches. `Run()` checks nil client but not nil router. With nil router, `stages` stays nil and all events are skipped without logging. Intentional for Phase 1 stub but behavior should be documented.
12	`docs-staleness`	`docs/cli/README.md`	New `fullsend poll` command not documented. CLI command, subcommand (`generate-child-pipeline`), environment variables (FULLSEND_FORGE_TOKEN, FULLSEND_POLL_MODE, CI_PROJECT_PATH), and 7 CI state variables are undocumented. Also missing from `docs/guides/dev/cli-internals.md` command tree. Since the command cannot run yet (nil client guard), documentation may be intentionally deferred. Severity anchored at low per prior review.
13	`pattern-inconsistency`	`internal/dispatch/event.go:123`	Inconsistent punctuation in godoc comments. Some type comments use trailing periods while others omit them. Existing codebase pattern uses periods consistently.

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

Previous run (9)

Review

This is a re-review (prior SHA: 34e3e186). Branch was rebased; all files re-evaluated. One prior finding resolved (dispatched keys pruning added). Three new medium-severity findings identified.

Prior Finding Status

Prior #	Status	Notes
1 (Low, edge-case, discoverSlashCommands MR context)	⚠ Unchanged	Fast-poll path still omits MR context; severity anchored at low
2 (Low, docs-staleness, poll command undocumented)	⚠ Unchanged	CLI and env vars still undocumented
3 (Low, fail-open, bot detection inconsistency)	⚠ Unchanged	Severity anchored at low
4 (Low, unbounded-state, dispatched keys growth)	✅ Resolved	`persistDispatchedKeys` now prunes entries older than watermark

Findings

Medium

#	Category	File	Description
1	`schema-contract`	`internal/dispatch/event.go`	NormalizedEvent duplicates canonical normevent.Event. The new `dispatch.NormalizedEvent` type mirrors `internal/normevent/event.go` but uses plain strings where `normevent` uses typed enums (`EntityKind`, `TransitionKind`, `ActorKind`, `ActorRole`, `SourceSystem`), and lacks the `Validate()` method. All existing dispatch consumers (`ghaevent.go`, `core.go`, `auth.go`, `enumerate.go`) use `normevent.Event`. Adding a parallel unvalidated type creates divergence risk. Remediation: Replace with imports from `internal/normevent`, or document architectural justification for a separate type.
2	`scope-mismatch`	`internal/cli/poll.go`	PR title claims Phase 2 but Phase 1 wiring is incomplete. The CLI passes nil client and nil router (line 68). The guard at `poll.go:53` returns an error for nil client. The `generate-child-pipeline` subcommand IS functional. Consider retitling to reflect scaffolding nature or adding a PR body note about phasing strategy.
3	`unbounded-state`	`internal/poll/state.go`	`persistFailedKeys` does not prune despite its doc comment. The comment says "pruning entries that have exceeded the retry budget" but the implementation marshals all keys unconditionally (lines 207-215). Events hitting `maxEventRetries` are skipped in the poll loop but their entries persist in `FULLSEND_FAILED_KEYS_` CI variable indefinitely. Compare with `persistDispatchedKeys` which correctly prunes by watermark. Remediation:* Delete entries where `count >= maxEventRetries` before marshaling, and correct the doc comment.

Low

#	Category	File	Description
4	`edge-case`	`internal/poll/events.go`	`discoverSlashCommands` omits MR context for `mr_note` events. Events lack MRSource, MRTarget, SourceBranch, TargetBranch, MRAuthorID, MRAuthorLogin. `buildChangeProposalState` will produce nil `ChangeProposal`. Nil client guard prevents execution. Severity anchored at low per prior review.
5	`docs-staleness`	`docs/cli/README.md`	New `fullsend poll` command not documented. CLI command, subcommand (`generate-child-pipeline`), and environment variables (FULLSEND_FORGE_TOKEN, FULLSEND_POLL_MODE, CI_PROJECT_PATH, plus 7 CI state variables) are undocumented. Since the command cannot run yet (nil client guard), documentation may be intentionally deferred. Severity anchored at low per prior review.
6	`fail-open`	`internal/poll/events.go`	Bot detection inconsistency between poll modes. `discoverSlashCommands` uses `isProjectAccessTokenBot(username)` while `discoverAllEvents` uses `Author.Bot`. Blast radius limited to fast-poll mode. Severity anchored at low per prior review.

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

Previous run (10)

Review

Verdict: Approve — all three prior low-severity findings unchanged; one new low finding on the delta. No medium or higher findings.

This is a re-review (prior SHA: 5d9cc3bd). Four files changed in this iteration, adding cross-poll deduplication via a FULLSEND_DISPATCHED_KEYS CI variable and associated tests:

File	Change
`internal/poll/state.go`	Added `readDispatchedKeys` and `persistDispatchedKeys` — CI variable-backed set for dispatched event keys
`internal/poll/poll.go`	Added pre-dispatch filter against previously-dispatched keys; merge and persist after dispatch
`internal/poll/poll_test.go`	Added `TestRunIdempotentSecondPoll` — verifies second poll with identical events produces 0 dispatches
`internal/poll/convert_test.go`	Added `TestBuildChangeProposalState_ZeroProjectIDs` — tests error path for zero MRSource

Delta correctness assessment: The deduplication logic is correctly ordered (after bot filtering and within-cycle dedup, before NormalizedEvent conversion). readDispatchedKeys returns empty set on error (fail-open, consistent with at-least-once delivery). Key accumulation correctly merges new keys into the prior set before persisting. TestRunIdempotentSecondPoll exercises the full round-trip: first poll dispatches, state is carried forward, second poll skips the same event. No existing test assertions were weakened or loosened.

Delta security assessment: FULLSEND_DISPATCHED_KEYS stores only event keys (format: note-{ID}, {type}-{IID}, {type}-{IID}-{label}) — no sensitive data. Keys are derived from controlled format strings with integer/string values; no user-controlled content reaches the key format. Variable is persisted with protected: true, consistent with other CI variables.

Prior Finding Status

Prior #	Status	Notes
1 (Low, edge-case, discoverSlashCommands MR context)	⚠ Unchanged	Fast-poll path still omits MR context; severity anchored at low
2 (Low, docs-staleness, poll command undocumented)	⚠ Unchanged	`FULLSEND_DISPATCHED_KEYS` adds one more undocumented CI variable
3 (Low, fail-open, bot detection inconsistency)	⚠ Unchanged	Severity anchored at low

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`, `FULLSEND_DISPATCHED_KEYS`) 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.
4	`unbounded-state`	`internal/poll/state.go`	Dispatched keys set grows without bound. `readDispatchedKeys`/`persistDispatchedKeys` accumulate every dispatched event key but never prune old entries. Compare with `detectNewLabels` (state.go:100-107), which prunes closed issues. GitLab CI variables have a size limit (~10KB); at ~25 bytes per key, the limit is reached after ~400 dispatched events. When hit, `persistDispatchedKeys` fails (WARNING logged), and subsequent `readDispatchedKeys` returns empty set — gracefully reverting to watermark-only dedup. Since the CLI still passes nil client (Phase 1 incomplete), this is latent. Remediation: Add TTL-based pruning (retain keys from last N hours only) before wiring the Phase 1 client.

Reviewed dimensions: correctness, security, intent & coherence, docs currency. Delta: 4 files changed (state.go, poll.go, poll_test.go, convert_test.go). Prior review provenance: app-verified. Findings adjudicated against prior review anchored severities.

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

Previous run (11)

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/

fullsend-ai-review · 2026-07-11T15:43:18Z

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

codecov · 2026-07-11T15:46:49Z

Codecov Report

❌ Patch coverage is 81.99482% with 139 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/poll/poll.go	71.72%	24 Missing and 17 partials ⚠️
internal/cli/poll.go	56.36%	22 Missing and 2 partials ⚠️
internal/poll/events.go	86.06%	14 Missing and 9 partials ⚠️
internal/poll/dispatch.go	79.77%	9 Missing and 9 partials ⚠️
internal/poll/convert.go	91.19%	10 Missing and 7 partials ⚠️
internal/poll/state.go	86.44%	12 Missing and 4 partials ⚠️

📢 Thoughts on this report? Let us know!

fullsend-ai-review · 2026-07-11T15:58:20Z

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

fullsend-ai-review · 2026-07-11T19:51:15Z

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

fullsend-ai-review · 2026-07-12T23:54:16Z

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

fullsend-ai-review · 2026-07-19T23:46:09Z

🤖 Review · Started 11:46 PM UTC
Commit: d0cb43d · View workflow run →

ggallen · 2026-07-19T23:46:13Z

/fs-review

fullsend-ai-review · 2026-07-19T23:47:25Z

🤖 Review · ❌ Terminated · Started 11:47 PM UTC · Ended 12:08 AM UTC
Commit: 3d48dce · View workflow run →

ggallen · 2026-07-20T00:13:23Z

/fs-review

fullsend-ai-review · 2026-07-20T00:17:11Z

🤖 Review · Started 12:16 AM UTC
Commit: 3d48dce · View workflow run →

ggallen · 2026-07-20T00:40:12Z

/fs-review

ggallen · 2026-07-20T00:53:41Z

/fs-review

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: 89.8% on internal/poll/. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Greg Allen <gallen@redhat.com>

ggallen · 2026-07-20T10:41:47Z

/fs-review

fullsend-ai-review · 2026-07-20T10:43:24Z

🤖 Finished Review · ❌ Failure · Started 10:43 AM UTC · Completed 10:59 AM UTC
Commit: 7d0bfa9 · View workflow run →

fullsend-ai-retro · 2026-07-20T11:47:02Z

🤖 Finished Retro · ✅ Success · Started 11:47 AM UTC · Completed 11:59 AM UTC
Commit: feb6548 · View workflow run →

fullsend-ai-retro · 2026-07-20T11:59:35Z

Retro: PR #4100 — feat(poll): implement Phase 2 cron poller

What happened

PR #4100 added 3,909 lines of new Go code across 16 files implementing a GitLab cron-based event poller (ADR-0067 Phase 2). The PR was authored by a human (ggallen) and went through a turbulent 9-day review cycle (Jul 11-20).

Timeline:

Jul 11: PR opened. fullsend-ai-review ran first pass — posted 8 findings, submitted APPROVED. qodo-code-review posted 6 inline findings.
Jul 11-12: Author systematically fixed all findings.
Jul 13: Second review cycle — fullsend-ai-review APPROVED with 4 new low-severity findings. A separate review-squad (3 parallel agents cross-referencing ADR-0067) posted 1 CRITICAL + 3 MEDIUM findings.
Jul 17: Major review-squad pass found 1 CRITICAL + 5 HIGH + 8 MEDIUM issues that the standard review agent missed in 6 prior passes. Author fixed all. Re-verification confirmed 12/14 fixed, 2 partially fixed, then final pass confirmed all addressed — APPROVED.
Jul 17-19: 5 review runs failed with 422 Unprocessable Entity from GitHub API in post-review.sh. The agent itself succeeded every time; the post-script failed when file-level fallback comments (findings outside the diff hunk) were included in the API review submission with invalid line positions.
Jul 19-20: Author triggered /fs-review 8+ times due to recurring failures. One successful run submitted CHANGES_REQUESTED with findings that included repeats of previously-resolved items.
Jul 20: Author merged PR despite 4 outstanding CHANGES_REQUESTED from the review agent, citing the review-squad's prior APPROVED verdict and all substantive findings having been addressed.

By the numbers: 19 review runs total, 5 failures (all 422 post-script errors), 2-4 terminated, verdict sequence of 5 APPROVEDs followed by 4 CHANGES_REQUESTEDs.

Key observations

1. Post-review 422 failures (evidence for #2569): All 5 failures shared the same root cause — the review agent generated findings referencing lines outside the PR's changed diff hunks. The post-review script detected these and marked them as "file-level comments," but they were still included in the createPullRequestReview API call with their original (invalid) positions, causing GitHub to reject the entire review. The single successful late run had zero file-level fallback comments. Issue #2569 describes this exact mechanism and has a fix in progress.

2. Verdict flip-flopping (evidence for #5107, #1500): The agent approved the PR 5 times, then requested changes 4 times. The late CHANGES_REQUESTED included both stale re-raises of previously-resolved findings (NoteAuthorID overloading, nil router, YAML injection safety) and 2 new medium-severity findings. The verdict regression (APPROVED to CHANGES_REQUESTED on objectively improved code) undermined the agent's credibility and led the human to override it.

3. Manual retry loop (evidence for #2138): Because failed runs required full re-dispatch, the user triggered /fs-review 8+ times manually. Post-script-only retry (#2138) would have avoided re-running the entire agent and saved significant tokens.

4. ADR context gap (evidence for #3007): The review-squad, which cross-referenced ADR-0067, found 14 issues (1 CRITICAL, 5 HIGH) that 6 prior standard review agent passes missed entirely. The standard agent lacked design-intent context and only caught surface-level API contract and error-handling issues. This is strong evidence that ADR cross-referencing dramatically improves review quality on architecture-heavy PRs.

5. Autonomy counter-evidence (evidence for #5251): The review agent approved a 3,900-line greenfield PR on first pass, missing CRITICAL correctness bugs later caught by the review-squad. Combined with the verdict flip-flop and human override, this PR demonstrates the agent is not ready for autonomous approval authority on large, architecture-heavy PRs.

Proposals filed

Review agent should self-report coverage gaps on large PRs (in fullsend-ai/agents)

ggallen requested a review from a team as a code owner July 11, 2026 11:24

qodo-code-review Bot reviewed Jul 11, 2026

View reviewed changes

Comment thread internal/poll/client.go

Comment thread internal/cli/poll.go

Comment thread internal/poll/events.go

Comment thread internal/poll/dispatch.go Outdated

Comment thread internal/poll/dispatch.go

Comment thread internal/poll/dispatch.go Outdated

fullsend-ai-retro Bot mentioned this pull request Jul 11, 2026

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

Closed

5 tasks

fullsend-ai-review Bot added requires-manual-review Review requires human judgment component/dispatch Workflow dispatch and triggers go Pull requests that update go code labels Jul 11, 2026

ggallen force-pushed the pr-4099 branch from f0ccd41 to 77c4163 Compare July 11, 2026 15:42

This comment was marked as outdated.

Sign in to view

fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed requires-manual-review Review requires human judgment labels Jul 11, 2026

ggallen force-pushed the pr-4099 branch from 77c4163 to f33be3f Compare July 11, 2026 19:50

This comment was marked as outdated.

Sign in to view

fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed ready-for-merge All reviewers approved — ready to merge labels Jul 11, 2026

ggallen force-pushed the pr-4099 branch 2 times, most recently from 4b59124 to 3af2476 Compare July 12, 2026 23:53

This comment was marked as outdated.

Sign in to view

fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge type/feature New capability request and removed requires-manual-review Review requires human judgment labels Jul 13, 2026

ggallen force-pushed the pr-4099 branch from 3af2476 to 5d9cc3b Compare July 13, 2026 00:15

fullsend-ai-review Bot removed the requires-manual-review Review requires human judgment label Jul 19, 2026

ggallen force-pushed the pr-4099 branch from a55b7b5 to d0cb43d Compare July 19, 2026 23:45

github-actions Bot deployed to site-preview July 19, 2026 23:47 View deployment

This comment was marked as outdated.

Sign in to view

ggallen force-pushed the pr-4099 branch from d0cb43d to 826a05a Compare July 20, 2026 00:13

This comment was marked as outdated.

Sign in to view

ggallen force-pushed the pr-4099 branch from 826a05a to 6f883ca Compare July 20, 2026 00:40

ggallen force-pushed the pr-4099 branch from 6f883ca to feb6548 Compare July 20, 2026 10:41

github-actions Bot deployed to site-preview July 20, 2026 10:43 View deployment

fullsend-ai-review Bot mentioned this pull request Jul 20, 2026

feat(scaffold): add GitLab CI/CD pipeline templates #3193

Merged

3 tasks

ggallen added this pull request to the merge queue Jul 20, 2026

ggallen removed this pull request from the merge queue due to a manual request Jul 20, 2026

ggallen added this pull request to the merge queue Jul 20, 2026

Merged via the queue into main with commit bf05100 Jul 20, 2026
14 checks passed

ggallen deleted the pr-4099 branch July 20, 2026 11:44

fullsend-ai-retro Bot mentioned this pull request Jul 20, 2026

Review agent should self-report coverage gaps on large PRs fullsend-ai/agents#302

Open

Uh oh!

Conversation

ggallen commented Jul 11, 2026

Summary

Test plan

Uh oh!

fullsend-ai-review Bot commented Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qodo-code-review Bot commented Jul 11, 2026

PR Summary by Qodo

Uh oh!

qodo-code-review Bot commented Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fullsend-ai-review Bot commented Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review

Findings

High

Low

Review

Findings

High

Low

Review

Findings

High

Medium

Low

Review

Findings

High

Low

Review

Prior Finding Status

Findings

High

Medium

Low

Review

Prior Finding Status

Findings

High

Medium

Low

Review

Prior Finding Status

Findings

High

Medium

Low

Review

Prior Finding Status

Findings

Medium

Low

Review

Prior Finding Status

Findings

Medium

Low

Review

Prior Finding Status

Findings

Medium

Low

Review

Prior Finding Status

Findings

Low

Review

Findings

fullsend-ai-review Bot commented Jul 11, 2026 •

edited

Loading

qodo-code-review Bot commented Jul 11, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jul 11, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jul 11, 2026 •

edited

Loading

codecov Bot commented Jul 11, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jul 11, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jul 12, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jul 19, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jul 20, 2026 •

edited

Loading

fullsend-ai-retro Bot commented Jul 20, 2026 •

edited

Loading