feat(poll): wire up event router for GitLab cron-polling dispatch

[ggallen]

Summary

• Adds HarnessRouter implementing dispatch.EventRouter with routing rules from ADR 0067: slash commands (/fs-X), label triggers (ready-to-code → code, ready-for-review → review), merge → retro, changes-requested → fix, needs-info → triage
• Exports RoleLevel/HasRole on the dispatch package as the single source of truth for role hierarchy comparisons (ADR 0054)
• Wires the router into fullsend poll --forge gitlab so discovered events actually dispatch to agent stages (previously the router was always nil)
• Resolves bot user ID via GetAuthenticatedUserID() for proper event filtering
• Adds --fullsend-dir flag (default .fullsend) to locate config.yaml for agent discovery
• Custom/additional agents defined in config.yaml are dispatchable via /fs-{name} slash commands

Test plan

• 18 unit tests covering all routing rules, authorization guards, fork protection, disabled agents, case-insensitive matching, and edge cases
• go build ./... compiles cleanly
• go test ./internal/dispatch/... passes
• go test ./internal/poll/... passes (existing tests unaffected)
• go vet clean
• Integration test with a real GitLab project (manual)

🤖 Generated with Claude Code

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 2:56 PM UTC · Ended 3:00 PM UTC
Commit: 3d4c1e5 · View workflow run →

[github-actions]

Site preview

Preview: https://40357556-site.fullsend-ai.workers.dev

Commit: ada69a87e950685ad5db7a9f40a9886c870aa694

[qodo-code-review]

PR Summary by Qodo

Wire event routing into GitLab poller dispatch (HarnessRouter + config agents)

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Add HarnessRouter to map GitLab events to agent stages per ADR 0067 routing table
• Wire router + authenticated bot user ID into fullsend poll --forge gitlab so events dispatch
• Load dispatchable agent names from scaffold defaults and .fullsend/config.yaml (incl. disabled
 agents)

Diagram

graph TD
  A["fullsend poll"] --> B["buildRouter"] --> R["HarnessRouter"] --> P["Poller"] --> S["Stage dispatch"]
  C[".fullsend config"] --> B
  D["Scaffold harnesses"] --> B
  P --> G["GitLab API"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Data-driven routing table in config

• ➕ Lets orgs customize label/event → stage mappings without code changes
• ➕ Avoids code churn when routing rules evolve
• ➖ Adds config validation burden and misconfiguration risk
• ➖ Harder to keep consistent defaults across forges

2. Use existing CEL trigger evaluation for routing

• ➕ Single mechanism for routing and harness triggers
• ➕ Potentially simplifies future routing expansion
• ➖ Overkill for a small fixed ADR mapping
• ➖ Increases coupling to trigger syntax and security hardening needs

Recommendation: Keeping routing as an explicit, tested router (this PR) is the best near-term choice: it makes GitLab polling actually dispatch while enforcing clear authorization and fork-safety defaults. If/when routing needs customization, evolve toward a data-driven routing table while preserving these hardcoded defaults as the baseline.

Files changed (4) +565 / -6

Enhancement (3) +223 / -6

poll.goWire router + bot user lookup into GitLab poll command +56/-6

Wire router + bot user lookup into GitLab poll command

• Resolves the authenticated GitLab user ID for bot-event filtering and constructs a real event router instead of passing nil. Adds '--fullsend-dir' and builds the valid-agent set from scaffold harnesses plus enabled config agents (explicit disables respected).

internal/cli/poll.go

router.goAdd HarnessRouter implementing ADR 0067 event-to-stage routing +150/-0

Add HarnessRouter implementing ADR 0067 event-to-stage routing

• Introduces 'dispatch.HarnessRouter' to route normalized events to stages via slash commands and fixed mappings (labels, merge, changes-requested marker, needs-info triage). Enforces role-based guards and blocks fork-sensitive routing when MR metadata indicates a fork or is missing.

internal/dispatch/router.go

poll.goExpose authenticated GitLab user ID for bot filtering +17/-0

Expose authenticated GitLab user ID for bot filtering

• Adds 'GetAuthenticatedUserID()' which calls '/user' and decodes the authenticated user’s numeric ID, enabling accurate bot filtering in polling mode.

internal/forge/gitlab/poll.go

Tests (1) +342 / -0

router_test.goUnit tests for HarnessRouter routing rules and guards +342/-0

Unit tests for HarnessRouter routing rules and guards

• Adds tests for slash-command routing (including custom agents), role-based authorization, label triggers, merge→retro, changes-requested→fix fork protection, and needs-info triage gating.

internal/dispatch/router_test.go

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:02 PM UTC · Completed 3:19 PM UTC
Commit: 4372576 · View workflow run →

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Compliance rules (platform): 54 rules

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

1. Slash command stage casing ✓ Resolved 🐞 Bug ≡ Correctness

Description

HarnessRouter accepts agent names case-insensitively but returns the stage using the original casing
from the /fs- command (e.g., /fs-TRIAGE → "TRIAGE"). This can break downstream GitLab execution
because stage/role checks and child-pipeline generation expect lowercase stage identifiers.

Code

internal/dispatch/router.go[R102-112]

+	stage := strings.TrimPrefix(cmd, "/fs-")
+	if stage == "" {
+		return nil, nil
+	}
+
+	if !r.validAgents[strings.ToLower(stage)] {
+		return nil, nil
+	}
+
+	return []string{stage}, nil
+}

Relevance

⭐⭐⭐ High

Repo enforces lowercase stage identifiers elsewhere; normalizing returned stage to lowercase aligns
with existing stage validation.

PR-#390

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The router lowercases stage only for membership checking, then returns the original stage string;
GitLab’s agent pipeline compares roles against the exact stage string and the poll YAML generator
enforces a lowercase stage regex, so mixed-case stages can cause skips/errors.

internal/dispatch/router.go[97-112]
internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml[85-104]
internal/poll/dispatch.go[15-16]
internal/poll/dispatch.go[122-131]

Agent prompt

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

## Issue description
`routeSlashCommand` validates stage membership with a lowercase key but returns the original `stage` string (original casing). This creates inconsistent stage identifiers (e.g. `TRIAGE`) that downstream GitLab logic treats as different from `triage`.

## Issue Context
- Router membership check is case-insensitive, but the returned stage is not normalized.
- GitLab CI template logic and poll YAML generation enforce lowercase stage conventions.

## Fix Focus Areas
- internal/dispatch/router.go[97-112]

### Suggested fix
Normalize once and return the normalized stage:
- `stage := strings.ToLower(strings.TrimPrefix(cmd, "/fs-"))`
- Use `stage` consistently for lookup and return.
- Add/adjust a unit test to cover `/fs-TRIAGE` routing to `triage`.

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

---

2. Forged marker triggers fix ✓ Resolved 🐞 Bug ⛨ Security

Description

The changes-requested marker route triggers the fix stage based solely on marker presence, without
verifying the comment is authored by the bot (or otherwise trusted). A human commenter can include
the marker string and cause a fix-stage dispatch even if they couldn’t issue a /fs-fix slash
command.

Code

internal/dispatch/router.go[R71-81]

+	// Changes-requested marker on MR notes → fix stage.
+	if event.Entity.Kind == "change_proposal" &&
+		strings.Contains(event.Transition.Comment.Body, changesRequestedMarker) {
+		if event.State.ChangeProposal == nil || event.State.ChangeProposal.IsFork {
+			return nil, nil
+		}
+		if !r.validAgents["fix"] {
+			return nil, nil
+		}
+		return []string{"fix"}, nil
+	}

Relevance

⭐⭐ Medium

Security hardening is often welcomed, but no close precedent on bot-authored marker enforcement in
routing.

PR-#390
PR-#2346

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The router’s marker branch has no actor trust/role guard, and the poller’s bot filtering explicitly
allows all non-bot events through (including human notes that contain the marker).

internal/dispatch/router.go[71-81]
internal/poll/events.go[220-236]

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 `changesRequestedMarker` routing path dispatches `fix` when the marker is present, but does not verify that the actor is the bot (or at least a trusted/authorized actor). This allows any human-authored MR comment containing the marker to trigger `fix`.

## Issue Context
- The poller only *preserves* bot-authored marker notes as a special case, but it does not prevent a human note containing the marker from reaching the router.
- The comment in code states the marker is a bot signal.

## Fix Focus Areas
- internal/dispatch/router.go[71-81]
- internal/poll/events.go[220-236]

### Suggested fix
In `routeComment` (marker branch), add a trust check such as:
- `if event.Actor.Kind != "bot" { return nil, nil }`
Optionally also require `hasRole(event.Actor.Role, "write")` for defense-in-depth.
Add a unit test asserting a human actor with the marker does **not** route to `fix`.

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

---

3. Underscore stage name mismatch ✗ Dismissed 🐞 Bug ≡ Correctness

Description

buildRouter registers config-derived agent names (which may include underscores) as routable
slash-command targets, but the poll child-pipeline YAML generator rejects stage names containing
underscores. This means a valid config agent name like "my_agent" can be routed and dispatched, then
later fail during child pipeline generation.

Code

internal/cli/poll.go[R106-120]

+	configAgents := cfg.AgentEntries()
+
+	// Collect unique agent names: scaffold defaults + config entries.
+	// Config entries with enabled: false suppress the agent.
+	nameSet := make(map[string]bool)
+	for _, name := range scaffoldNames {
+		if !config.IsAgentExplicitlyDisabled(configAgents, name) {
+			nameSet[strings.ToLower(name)] = true
+		}
+	}
+	for _, entry := range configAgents {
+		if entry.IsEnabled() {
+			nameSet[strings.ToLower(entry.DerivedName())] = true
+		}
+	}

Relevance

⭐⭐ Medium

Potential cross-component naming constraint, but no clear precedent on underscore-stage
incompatibility handling.

PR-#390

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Config/scaffold naming regexes allow underscores, while the child pipeline generator’s stage regex
does not; buildRouter adds config-derived names directly, so routing can produce a stage value the
generator later rejects.

internal/cli/poll.go[93-128]
internal/config/config.go[15-18]
internal/scaffold/baseurl.go[18-21]
internal/poll/dispatch.go[15-16]
internal/poll/dispatch.go[122-131]

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 router is built from scaffold + config agent names without enforcing the stage-name grammar used later when generating GitLab child pipeline YAML. Config/scaffold allow `_`, but the poll YAML generator currently rejects `_` in `Dispatch.Stage`.

## Issue Context
- Config agent names allow underscores.
- Scaffold harness names also allow underscores.
- Poll child pipeline stage validation allows only lowercase letters, digits, and hyphens.

## Fix Focus Areas
- internal/cli/poll.go[106-120]

### Suggested fix options (pick one policy)
1) Preferred: Broaden `validStage` in `internal/poll/dispatch.go` to `^[a-z][a-z0-9_-]*$` to match scaffold/config naming.
2) Alternatively: Validate and reject (with a clear error) agent names containing `_` when building the router (or when dispatching), so you fail early with an actionable message.

Add a unit test covering a config agent name with `_` and the expected behavior under the chosen policy.

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

---

4. buildRouter() lacks CLI tests 📘 Rule violation ▣ Testability

Description

New routing/config merge logic was added in buildRouter but this PR does not add or update any
internal/cli/*_test.go coverage for it, risking regressions in fullsend poll behavior. This
violates the requirement to include tests for new or modified Go logic.

Code

internal/cli/poll.go[R93-128]

+// buildRouter constructs a HarnessRouter from the merged set of
+// scaffold default agents and config-registered agents.
+func buildRouter(fullsendDir string) (*dispatch.HarnessRouter, error) {
+	cfg, err := config.LoadConfig(fullsendDir, config.LoadOpts{MissingOK: true})
+	if err != nil {
+		return nil, fmt.Errorf("load config: %w", err)
+	}
+
+	scaffoldNames, err := scaffold.HarnessNames()
+	if err != nil {
+		return nil, fmt.Errorf("list scaffold harnesses: %w", err)
+	}
+
+	configAgents := cfg.AgentEntries()
+
+	// Collect unique agent names: scaffold defaults + config entries.
+	// Config entries with enabled: false suppress the agent.
+	nameSet := make(map[string]bool)
+	for _, name := range scaffoldNames {
+		if !config.IsAgentExplicitlyDisabled(configAgents, name) {
+			nameSet[strings.ToLower(name)] = true
+		}
+	}
+	for _, entry := range configAgents {
+		if entry.IsEnabled() {
+			nameSet[strings.ToLower(entry.DerivedName())] = true
+		}
+	}
+
+	names := make([]string, 0, len(nameSet))
+	for name := range nameSet {
+		names = append(names, name)
+	}
+
+	return dispatch.NewHarnessRouter(names), nil
+}

Relevance

⭐ Low

Similar “add tests for new CLI logic” requests were rejected as nonessential coverage work.

PR-#2860
PR-#1627
PR-#1573

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1062049 requires tests for new/modified Go logic. The PR introduces buildRouter()
with configuration-driven routing behavior in internal/cli/poll.go, but provides no corresponding
internal/cli test updates to exercise this logic.

Rule 1062049: Require tests for new or modified Go logic
internal/cli/poll.go[93-128]

Agent prompt

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

## Issue description
`internal/cli/poll.go` adds non-trivial logic in `buildRouter()` (merging scaffold agent names with config-defined agents, handling disabled agents, and normalizing names), but there are no accompanying tests in the `internal/cli` package updated/added in this PR to validate the behavior.

## Issue Context
The compliance rule requires tests for new or modified Go logic. `buildRouter()` affects which agents can be dispatched and could break routing if config parsing/merging changes.

## Fix Focus Areas
- internal/cli/poll.go[93-128]
- internal/cli/poll_test.go[1-200]

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

---

[codecov]

Codecov Report

❌ Patch coverage is 84.24242% with 26 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/poll.go	48.00%	10 Missing and 3 partials ⚠️
internal/forge/gitlab/poll.go	61.90%	4 Missing and 4 partials ⚠️
internal/dispatch/router.go	94.93%	3 Missing and 1 partial ⚠️
internal/poll/events.go	95.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Review

Findings

Low

• [missing-authorization] internal/cli/poll.go — Non-trivial PR (13 changed files, 800+ new lines) lacks a linked issue. The PR title uses feat(poll): prefix indicating this is a feature-level change. Without a linked issue, there is no authorization trail showing this work was approved.

• [authorization-bypass-via-entity-author] internal/dispatch/router.go — Entity authors bypass the triage role check on needs-info issues. Intentional per ADR 0067 §"needs-info re-triage" and documented in a code comment.

• [missing-authorization-check] internal/dispatch/router.go — routeMerge dispatches the "retro" stage without an actor role check. Intentional: only users with Maintainer+ access can merge (forge-level gate), and retro is read-only. Documented in the method's doc comment.

• [architectural-coherence] internal/dispatch/router.go — ADR 0067 prescribes CEL trigger expressions for routing; this PR implements hard-coded Go routing rules as an interim. The deviation is properly documented in the HarnessRouter doc comment and acknowledged in docs/contributing/go-code.md.

• [design-direction] internal/dispatch/event.go — RoleLevel/HasRole are exported from the dispatch package. When the dispatch core is built (feat(dispatch): port triage to harness CEL trigger #2896–feat(dispatch): port prioritize to harness CEL trigger #2901), authorization should be centralized there per ADR 0061. Not blocking — just a note for future direction.

---

Prior review findings resolved: The silent-no-op-on-unknown-command-prefix finding is resolved — routeSlashCommand now explicitly checks strings.HasPrefix(cmd, "/fs-") before calling TrimPrefix.

Previous run

Review

Findings

Low

• [silent-no-op-on-unknown-command-prefix] internal/dispatch/router.go — routeSlashCommand applies strings.TrimPrefix(cmd, "/fs-") without verifying the command actually starts with /fs-. If a NormalizedEvent is produced with a Command field that lacks the prefix, TrimPrefix returns the string unchanged and the full command string is looked up in validAgents. Today the only producer (poll layer) guarantees the prefix, but the router has no contract enforcement. Adding if !strings.HasPrefix(cmd, "/fs-") { return nil, nil } before the TrimPrefix call would provide defense-in-depth.

• [authorization-bypass-via-entity-author] internal/dispatch/router.go — Entity authors bypass the triage role check on needs-info issues. Intentional per ADR 0067 §"needs-info re-triage" and documented in a code comment.

• [missing-authorization-check] internal/dispatch/router.go — routeMerge dispatches the "retro" stage without an actor role check. Intentional: only users with Maintainer+ access can merge (forge-level gate), and retro is read-only. Documented in the method's doc comment.

• [architectural-coherence] internal/dispatch/router.go — ADR 0067 prescribes CEL trigger expressions for routing; this PR implements hard-coded Go routing rules as an interim. The deviation is properly documented in the HarnessRouter doc comment and acknowledged in docs/contributing/go-code.md.

• [design-direction] internal/dispatch/event.go — RoleLevel/HasRole are exported from the dispatch package. When the dispatch core is built (feat(dispatch): port triage to harness CEL trigger #2896–feat(dispatch): port prioritize to harness CEL trigger #2901), authorization should be centralized there per ADR 0061. Not blocking — just a note for future direction.

---

Prior review findings resolved: The latent-fork-bypass finding is resolved — routeLabel now includes the isForkOrUnknown check for change_proposal entities.

Previous run (2)

Review

Findings

Low

• [latent-fork-bypass] internal/dispatch/router.go — routeLabel does not enforce fork-MR blocking for change_proposal entities. Currently unreachable because the poll layer only produces issue_label events (which map to work_item entities), but the router accepts any Entity.Kind without checking. If MR label event production is added upstream, the router would dispatch code/review stages on fork MRs without the isForkOrUnknown gate that protects slash commands and the changes-requested path. Adding a one-line guard (if event.Entity.Kind == "change_proposal" && isForkOrUnknown(event.State) { return nil, nil }) would provide defense-in-depth consistent with the existing pattern.

• [authorization-bypass-via-entity-author] internal/dispatch/router.go — Entity authors bypass the triage role check on needs-info issues. Intentional per ADR 0067 §"needs-info re-triage" and documented in a code comment.

• [missing-authorization-check] internal/dispatch/router.go — routeMerge dispatches the "retro" stage without an actor role check. Intentional: only users with Maintainer+ access can merge (forge-level gate), and retro is read-only. Documented in the method's doc comment.

• [architectural-coherence] internal/dispatch/router.go — ADR 0067 prescribes CEL trigger expressions for routing; this PR implements hard-coded Go routing rules as an interim. The deviation is properly documented in the HarnessRouter doc comment and acknowledged in docs/contributing/go-code.md.

• [design-direction] internal/dispatch/event.go — RoleLevel/HasRole are exported from the dispatch package. When the dispatch core is built (feat(dispatch): port triage to harness CEL trigger #2896–feat(dispatch): port prioritize to harness CEL trigger #2901), authorization should be centralized there per ADR 0061. Not blocking — just a note for future direction.

---

Prior review findings resolved: The test-adequacy finding is resolved — TestDiscoverSlashCommands_MRFetchError now verifies the GetMergeRequest failure path (event is skipped, minSkippedAt is set correctly).

Previous run (3)

Review

Findings

Low

• [test-adequacy] internal/poll/events_test.go — The discoverSlashCommands function now calls GetMergeRequest for mr_note events and skips the event on failure (continue). There is no test verifying this failure path — that a GetMergeRequest error during slash command discovery correctly skips the event without error. The mock infrastructure (mrErr map in mockClient) is already in place.

• [authorization-bypass-via-entity-author] internal/dispatch/router.go — Entity authors bypass the triage role check on needs-info issues. Intentional per ADR 0067 §"needs-info re-triage" and documented in a code comment.

• [missing-authorization-check] internal/dispatch/router.go — routeMerge dispatches the "retro" stage without an actor role check. Intentional: only users with Maintainer+ access can merge (forge-level gate), and retro is read-only. Documented in the method's doc comment.

• [architectural-coherence] internal/dispatch/router.go — ADR 0067 prescribes CEL trigger expressions for routing; this PR implements hard-coded Go routing rules as an interim. The deviation is properly documented in the HarnessRouter doc comment and acknowledged in docs/contributing/go-code.md.

• [design-direction] internal/dispatch/event.go — RoleLevel/HasRole are exported from the dispatch package. When the dispatch core is built (feat(dispatch): port triage to harness CEL trigger #2896–feat(dispatch): port prioritize to harness CEL trigger #2901), authorization should be centralized there per ADR 0061. Not blocking — just a note for future direction.

Previous run (4)

Review

Findings

Low

• [architectural-coherence] internal/dispatch/router.go — ADR 0067 section "Event routing" prescribes CEL trigger expressions (ADR 0061) evaluated in the dispatch core, not hard-coded Go routing rules in the input driver. This PR implements HarnessRouter with hard-coded routing instead. The code documents this as interim (lines 13–17), and docs/contributing/go-code.md notes this as the current pattern until built-in harness triggers land (feat(dispatch): port triage to harness CEL trigger #2896–feat(dispatch): port prioritize to harness CEL trigger #2901). The deviation is properly documented and acknowledged.

• [authorization-bypass-via-entity-author] internal/dispatch/router.go — Entity authors bypass the triage role check on needs-info issues. Intentional per ADR 0067 §"needs-info re-triage" and documented in a code comment.

• [missing-authorization-check] internal/dispatch/router.go — routeMerge dispatches the "retro" stage without an actor role check. Intentional: only users with Maintainer+ access can merge (forge-level gate), and retro is read-only. Documented in the method's doc comment.

• [design-direction] internal/dispatch/event.go — RoleLevel/HasRole are exported from the dispatch package. When the dispatch core is built (feat(dispatch): port triage to harness CEL trigger #2896–feat(dispatch): port prioritize to harness CEL trigger #2901), authorization should be centralized there per ADR 0061. These utility functions can be called by the dispatch core, so this is not blocking — just a note for future direction.

• [missing-documentation] docs/cli/README.md — The fullsend poll command is not in the CLI reference. However, the command is explicitly cmd.Hidden = true, so this is expected for an internal command used by GitLab CI/CD pipelines.

---

Prior review findings resolved: The fail-open finding (HasRole now fails closed on unrecognized required roles via the reqLevel == 0 guard). The error-handling-gap finding (GetAuthenticatedUserID now returns an error when the API returns zero ID). The test-inadequate finding (TestBuildRouter_WithConfigAgents now verifies both custom agent routing and disabled agent filtering).

Previous run (5)

Review

Findings

Medium

• [architectural-coherence] internal/dispatch/router.go — ADR 0067 section "Event routing" prescribes CEL trigger expressions (ADR 0061) evaluated in the dispatch core, not hard-coded Go routing rules in the input driver. This PR implements HarnessRouter with hard-coded routing instead. The code documents this as interim (lines 13–17), and docs/contributing/go-code.md notes this as the current pattern until built-in harness triggers land (feat(dispatch): port triage to harness CEL trigger #2896–2901). The deviation is properly documented and acknowledged.

Low

• [fail-open] internal/dispatch/event.go — RoleLevel returns 0 for unknown role strings. Since HasRole compares RoleLevel(actorRole) >= RoleLevel(required), passing an unrecognized required role (e.g., a typo) would yield 0 >= 0 == true, silently passing all actors. Current callers use hardcoded valid literals ("write", "triage"), so this is not exploitable today. Consider returning -1 from the default case so unrecognized required roles always deny.

• [error-handling-gap] internal/forge/gitlab/poll.go — GetAuthenticatedUserID does not validate that the decoded user ID is non-zero. If /user returns id:0, botUserID would be 0 and the poller's bot detection would skip the user-ID check.

• [test-inadequate] internal/cli/poll_test.go — TestBuildRouter_WithConfigAgents creates a config with code disabled and my-custom-agent added, but only asserts the router is non-nil. It does not verify that the disabled agent is excluded from routing or that the custom agent is included.

• [missing-authorization] — This non-trivial PR (10 files, ~865 lines) has no linked issue. It implements Phase 5 of the GitLab cron-polling implementation plan (ADR 0067). Previous GitLab phases followed the same ADR/plan reference pattern.

• [design-direction] internal/dispatch/event.go — RoleLevel/HasRole are exported from the dispatch package. When the dispatch core is built (feat(dispatch): port triage to harness CEL trigger #2896–2901), authorization should be centralized there per ADR 0061. These utility functions can be called by the dispatch core, so this is not blocking — just a note for future direction.

• [authorization-bypass-via-entity-author] internal/dispatch/router.go — Entity authors bypass the triage role check on needs-info issues. Intentional per ADR 0067 §"needs-info re-triage" and documented in a code comment.

• [missing-authorization-check] internal/dispatch/router.go — routeMerge dispatches the "retro" stage without an actor role check. Intentional: only users with Maintainer+ access can merge (forge-level gate), and retro is read-only. Documented in the method's doc comment.

• [missing-documentation] docs/cli/README.md — The fullsend poll command is not in the CLI reference. However, the command is explicitly cmd.Hidden = true, so this is expected for an internal command used by GitLab CI/CD pipelines.

Previous run (6)

Review

Findings

Medium

• [architectural-coherence] internal/dispatch/router.go — ADR 0067 section "Event routing" prescribes CEL trigger expressions (ADR 0061) evaluated in the dispatch core, not hard-coded Go routing rules in the input driver. This PR implements HarnessRouter with hard-coded routing instead. The code documents this as interim ("This is an interim implementation using Go routing rules... this hard-coded router will be replaced once the CEL evaluation engine is available in the dispatch core"), and the Go code contributing guide notes this as the current pattern until built-in harness triggers land (feat(dispatch): port triage to harness CEL trigger #2896–2901). Tracking this deviation with an ADR or issue would formalize the interim status.

Low

• [fail-open] internal/dispatch/event.go — RoleLevel returns 0 for unknown role strings. Since HasRole compares RoleLevel(actorRole) >= RoleLevel(required), passing an unrecognized required role (e.g., a typo) would yield 0 >= 0 == true, silently passing all actors. Current callers use hardcoded valid literals ("write", "triage"), so this is not exploitable today. Consider returning -1 from the default case so unrecognized required roles always deny.

• [missing-authorization] — This non-trivial PR (8 files, new router implementation + 18 tests) has no linked issue. Linking to an authorizing issue or referencing the implementation plan in the PR description would clarify the authorized scope.

• [design-direction] internal/dispatch/event.go — RoleLevel/HasRole are exported from the dispatch package. When the dispatch core is built (feat(dispatch): port triage to harness CEL trigger #2896–2901), authorization should be centralized there per ADR 0061. These utility functions can be called by the dispatch core, so this is not blocking — just a note for future direction.

• [authorization-bypass-via-entity-author] internal/dispatch/router.go:85 — Entity authors bypass the triage role check on needs-info issues. Intentional per ADR 0067 ("Reporter+ or issue author") and documented in a code comment.

• [missing-authorization-check] internal/dispatch/router.go:140 — routeMerge dispatches the "retro" stage without an actor role check, unlike routeLabel and routeSlashCommand. Intentional: only users with Maintainer+ access can merge (forge-level gate), and retro is read-only. Documented in the method's doc comment.

---

Prior review findings resolved: The interface-coherence finding (GetAuthenticatedUserID not in poll.GitLabClient interface) is now resolved — the method is declared in the interface, with a matching mock implementation. The fail-open finding (GetAuthenticatedUserID error ignored) is resolved — errors are now fatal. The logic-error finding (slash command casing) is resolved — stage names are lowercased via strings.ToLower.

Previous run (7)

Review

Findings

High

• [architectural-coherence] internal/dispatch/router.go — ADR 0067 section "Event routing" explicitly states: "Routing is performed by harness CEL trigger expressions (ADR 0061) evaluated in the dispatch core, not by the gitlab-poll input driver." This PR implements HarnessRouter with hard-coded Go routing rules (slash commands, labels, merge, changes-requested) instead of using CEL trigger evaluation from the dispatch core. The routing table reimplements the exact event-to-stage mappings that ADR 0067 says should be expressed as CEL triggers on harness files.
  Remediation: Either implement CEL trigger evaluation per ADR 0061/0067, or write an ADR documenting why hard-coded routing is necessary as an interim step.

Medium

• [interface-coherence] internal/forge/gitlab/poll.go — GetAuthenticatedUserID is a new method on PollClient but is not declared in the poll.GitLabClient interface. The existing GetAuthenticatedUser method IS in the interface. The compile-time interface check (var _ poll.GitLabClient = (*PollClient)(nil)) still passes because Go allows extra methods on concrete types, but the new method cannot be called through the interface, creating API surface inconsistency and making it harder to mock in tests.
  Remediation: Add GetAuthenticatedUserID(ctx context.Context) (int, error) to the poll.GitLabClient interface in internal/poll/client.go.

Low

• [authorization-bypass-via-entity-author] internal/dispatch/router.go:56 — Entity authors bypass the triage role check on needs-info issues. This is intentional per ADR 0067 ("Reporter+ or issue author") and the code has a comment explaining it.

• [missing-authorization-check] internal/dispatch/router.go:104 — routeMerge dispatches the "retro" stage for any merge event without an actor role check. Low severity because retro is read-only by design and only users who actually merged can produce this event type.

Previous run (8)

Review

Findings

Medium

• [logic-error] internal/dispatch/router.go:96 — routeSlashCommand returns the stage name with original casing from the user's slash command (e.g., /fs-TRIAGE yields "TRIAGE"). The validAgents check lowercases for lookup but the return value preserves original case. Downstream, generateChildPipelineYAML validates stage names with ^[a-z][a-z0-9-]*$, which rejects uppercase. Mixed-case commands pass the router but fail pipeline generation.
  Remediation: Lowercase the stage before returning: return []string{strings.ToLower(stage)}, nil.

• [missing authorization check] internal/dispatch/router.go:99 — routeLabel dispatches code and review stages for label events without any actor role check. The equivalent GitHub dispatch workflow checks is_event_actor_authorized for ready-to-code. While GitLab's permission model restricts label application to Developer+, an explicit HasRole check provides defense-in-depth.
  Remediation: Add a HasRole check in routeLabel before dispatching ready-to-code (require write).

• [fail-open] internal/cli/poll.go:59 — When GetAuthenticatedUserID fails, execution continues with botUserID=0, disabling the primary bot-filtering check. Secondary heuristics (Author.Bot, isProjectAccessTokenBot) remain active, but the primary filter is lost. This could cause the bot to re-dispatch on its own comments.
  Remediation: Consider making this error fatal, or document why degraded filtering is acceptable.

• [missing-authorization] — This non-trivial PR (new router implementation, new forge method, new CLI flag) has no linked issue. Non-trivial feature additions require explicit authorization via a linked issue.
  Remediation: Create and link an issue describing the intent.

• [architectural-coherence] internal/dispatch/router.go — ADR 0067 states routing should be performed by "harness CEL trigger expressions evaluated in the dispatch core." This PR introduces hard-coded Go routing rules, deviating from the prescribed architecture. If this is an interim step, document the deviation.
  Remediation: Either implement CEL trigger evaluation or write an ADR documenting the deviation.

• [naming-coherence] internal/forge/gitlab/poll.go — GetAuthenticatedUserID diverges from the existing poll.GitLabClient interface method GetAuthenticatedUser(ctx) (string, error). The new method returns int (user ID) vs string (username), creating naming inconsistency in the API surface.
  Remediation: Add GetAuthenticatedUserID to the poll.GitLabClient interface.

Low

• [insufficient-input-validation] internal/dispatch/router.go:87 — Stage name from slash commands is only validated by the validAgents allowlist. Consider explicit pattern validation as defense-in-depth.

• [authorization-bypass-via-entity-author] internal/dispatch/router.go:70 — Entity authors bypass the triage role check on needs-info issues. This appears intentional per ADR 0067 but should be documented in a code comment.

• [scope-creep] internal/dispatch/event.go — RoleLevel/HasRole utility functions placed in event.go alongside type definitions. Minor placement concern.

• [logging-idiom] internal/cli/poll.go:61 — log.Printf is unique in internal/cli/; all other warnings use fmt.Fprintf(os.Stderr, ...).

• [flag-description-consistency] internal/cli/poll.go:88 — --fullsend-dir description differs from the established convention used by other commands ("base directory containing the .fullsend layout").

• [flag-requirement-consistency] internal/cli/poll.go:88 — --fullsend-dir has a default value (.fullsend) unlike other commands where it is marked required.

• [package-cohesion] internal/dispatch/router.go — HarnessRouter adds routing logic to a package that previously contained only types and interfaces. The EventRouter interface was already in this package, so the placement is defensible.

---

Labels: PR adds event routing logic in the dispatch package with Go code changes

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:26 PM UTC · Completed 3:46 PM UTC
Commit: ead38ad · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:51 PM UTC · Completed 4:07 PM UTC
Commit: 5a6fa14 · View workflow run →

Superseded by updated review

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 5:55 PM UTC · Ended 6:05 PM UTC
Commit: ebd8029 · View workflow run →

[waynesun09]

4-agent review squad verification pass on round-2's fixes (fast-poll GetMergeRequest for MRSource/MRTarget, isForkOrUnknown extraction) — both confirmed correct and complete by all 4 agents, no regressions, consistent at both call sites.

1 HIGH + 1 MEDIUM survived verification and are posted inline below. Note: agents initially split on the HIGH finding's severity (2/4 independently found the same mechanism; a third assessed it as test-coverage-only). I traced poll.go's watermark logic directly myself before posting to resolve the disagreement — the finding is real, not just a coverage gap.

Posting as COMMENT only, per standing convention (no approve/request-changes).

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 8:25 PM UTC · Ended 8:30 PM UTC
Commit: 9c0c71e · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:31 PM UTC · Completed 8:44 PM UTC
Commit: 9abf915 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 9:12 PM UTC · Ended 9:12 PM UTC
Commit: c5b1850 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:14 PM UTC · Completed 9:31 PM UTC
Commit: 62f5226 · View workflow run →

[waynesun09]

4-agent review squad, round 4, at current head 9abf9153. Round-3's HIGH finding (GetMergeRequest failure causing permanent watermark loss of MR slash commands) is confirmed fixed — all 4 agents independently traced the minSkippedAt fix end-to-end through poll.go's watermark logic, including the specific edge case that mattered (the zero-events time.Now() fallback correctly gets overridden by the minSkippedAt clamp). The regression test asserts the exact timestamp value, not just presence. The previously-flagged GetMergeRequest HTTP client tests are confirmed adequate and consistent with the file's existing conventions.

A fresh full pass over the 13-file scope surfaced no new MEDIUM+ issues. Two optional LOW/style notes (a pre-existing, not-this-PR's-fault gap in Run()-level end-to-end test coverage for watermark holdback, and a minor opportunity to dedupe the "track earliest timestamp" pattern into the existing trackFailure-style helper) were left out per the medium-and-above posting threshold — neither is blocking.

Approving.

[waynesun09]

Round 4 review squad: round-3's HIGH finding (watermark loss of MR slash commands) confirmed fixed by all 4 agents, no new MEDIUM+ issues found. Approving.

[waynesun09]

Round 4 review squad: round-3's HIGH finding confirmed fixed by all 4 agents, no new MEDIUM+ issues found. Approving.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:35 PM UTC · Completed 9:47 PM UTC
Commit: ada69a8 · View workflow run →

[fullsend-ai-review]

Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:

• internal/dispatch/router.go:109: [low] authorization-bypass-via-entity-author

Entity authors bypass the triage role check on needs-info issues. Intentional per ADR 0067 and documented in a code comment.

• internal/dispatch/router.go:151: [low] missing-authorization-check

routeMerge dispatches the retro stage without an actor role check. Intentional: only users with Maintainer+ access can merge (forge-level gate), and retro is read-only. Documented in the methods doc comment.

• internal/dispatch/router.go (file-level): Line 374 · [low] architectural-coherence

ADR 0067 prescribes CEL trigger expressions for routing; this PR implements hard-coded Go routing rules as an interim. The deviation is properly documented in the HarnessRouter doc comment and acknowledged in docs/contributing/go-code.md.

• internal/dispatch/event.go (file-level): Line 190 · [low] design-direction

RoleLevel/HasRole are exported from the dispatch package. When the dispatch core is built (#2896-#2901), authorization should be centralized there per ADR 0061. Not blocking - just a note for future direction.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 10:20 PM UTC · Completed 10:39 PM UTC
Commit: ada69a8 · View workflow run →

[fullsend-ai-retro]

Retro: PR #5534 — feat(poll): wire up event router for GitLab cron-polling dispatch

A human-authored PR (+1203/−16, 13 files) implementing Phase 5 of GitLab cron-polling support. Reviewed over ~7.5 hours with 16 review agent runs, 4 fix agent runs, and 84 source-repo workflow dispatches before merging.

Timeline

• 14:55 — PR opened
• 15:00–15:25 — Bot + Qodo initial review finds ~10 issues (casing bug, forged marker, missing role check, fail-open on GetAuthenticatedUserID, style nits). Author fixes all rapidly.
• 15:45–18:47 — Three more review cycles. Bot approves at 18:47 with only LOW/informational findings remaining.
• 19:34 — waynesun09's 4-agent review squad (2 Claude, 1 Grok, 1 Gemini) round 1 finds 3 issues the bot missed: (1) HIGH: fork protection missing for slash commands — routeSlashCommand had no fork-MR check despite the contract in event.go stating routers MUST deny by default for forks; (2) MEDIUM: buildRouter reimplemented config.MergedAgents helper; (3) MEDIUM: incorrect GitLab merge-permission claim the bot itself had recommended.
• 19:57 — Squad round 2: MEDIUM duplicated fork-check predicate.
• 20:23 — Squad round 3: HIGH: GetMergeRequest failure causes permanent watermark loss — a direct consequence of the fork-protection fix, where discoverSlashCommands returned no minSkippedAt.
• 20:29–21:23 — Author fixes all. Squad round 4 confirms everything resolved. Final approval.
• 21:47 — Bot final review (LOW findings only). Approved.
• 22:18 — Merged.

Key Findings

Review quality gap: The bot missed 3 HIGH findings (all within the diff) and approved the PR 4 times while HIGH bugs remained. All 3 HIGH findings required tracing invariants across 2–4 files in the same diff — fork-protection contract in event.go vs. routing code in router.go, and watermark flow across events.go/poll.go. The bot's correctness sub-agent (Opus) had the right instructions ("trace the full path," "consumer completeness") but did not execute them on this 13-file change.

Finding noise: The bot re-raised the same intentional-by-design findings (entity-author bypass per ADR 0067, merge role check, CEL vs Go routing) ~8 times each across review cycles, producing ~37 noise comment instances that the author had to repeatedly dismiss.

CI volume: 40 of 62 fullsend.yaml runs were triggered by pull_request_review: submitted events that matched no dispatch route — pure waste. The existing cancel-in-progress: false on the dispatch shim is correct for event semantics but means these no-op runs all completed.

Fix regression undetected: 2 of the 3 HIGH findings were direct consequences of fixes applied during the review session. The bot re-reviewed post-fix and approved without detecting the regressions.

Evidence Supporting Existing Issues (proposals skipped as duplicates)

• #1525 (cross-file impact analysis): Third documented occurrence of waynesun09's review squad catching HIGH findings the bot missed. PR feat(poll): wire up event router for GitLab cron-polling dispatch #5534 adds 3 more HIGH misses — strongest single-PR evidence yet.
• agents #270 (side-effect analysis on fix commits): The watermark-loss bug was a direct regression from the fork-protection fix. Same pattern as PR feat(repos)!: add repos diff and repos sync CLI commands #4079 (documented in Composite action needs retry logic for OpenShell and CLI downloads to handle transient CDN failures #270). Note: the severity threshold in Composite action needs retry logic for OpenShell and CLI downloads to handle transient CDN failures #270 ("HIGH/CRITICAL findings") would not have triggered here since the bot rated the original finding as MEDIUM — broadening to all fix commits would close this gap.
• agents #380 (symmetric defensive patterns): Fork check was present in routeComment but absent in routeSlashCommand — identical pattern to the issue's original finding.
• #898 (security-critical findings on large PRs): Fork protection gap is a security vulnerability on a 13-file, 1200-line architectural PR. Same failure class.
• #4682, #2959, #1013 (dismissed finding dedup): ~37 noise instances from re-raised findings across 16 review cycles. Author dismissed the same ADR-0067 finding 4+ times.
• #1125, #4984 (pull_request_review trigger filtering): 40 no-op dispatch runs consumed CI minutes. Same waste pattern as originally documented in Filter pull_request_review trigger to only fire on changes_requested #1125.
• #4780, #3005 (premature approval): Bot approved 4 times with HIGH bugs remaining. The bot never rated any finding above MEDIUM despite 3 being genuine HIGH-severity issues.

What Went Well

• The bot's initial review (15:00–15:25) found 10 genuine issues including a security-relevant forged-marker bug and a fail-open pattern. These were all fixed before the human reviewer engaged.
• The author responded to every finding promptly, with fixes typically within 10–15 minutes.
• The multi-reviewer approach (bot for fast initial pass + human squad for depth) ultimately caught all issues before merge.
• The challenger sub-agent correctly did not false-positive any of the bot's genuine findings.