Skip to content

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

Merged
ggallen merged 1 commit into
fullsend-ai:mainfrom
ggallen:gitlab/cicd-templates
Jul 23, 2026
Merged

feat(scaffold): add GitLab CI/CD pipeline templates#3193
ggallen merged 1 commit into
fullsend-ai:mainfrom
ggallen:gitlab/cicd-templates

Conversation

@ggallen

@ggallen ggallen commented Jul 6, 2026

Copy link
Copy Markdown
Member

Summary

  • Add internal/scaffold/fullsend-repo-gitlab/ with complete GitLab CI/CD pipeline templates for per-repo installation
  • Stage templates call fullsend run <agent> with the same flags as GitHub's action.yml — harness-resolved images via Podman/OpenShell, no hardcoded container images
  • Dispatch template constructs NormalizedEvent v1 (ADR 0061) from GitLab CI variables, with shell routing annotated with equivalent CEL trigger expressions for future migration (feat(dispatch): port triage to harness CEL trigger #2896-2901)
  • MR author authorization via Members API with GitLab access_level → NormalizedEvent role mapping (fail-closed)
  • Fork MR protection in code/fix stages via IS_FORK variable from dispatch (fail-closed on deleted source projects)
  • WIF/variable credential modes, CI_DEBUG_TRACE guards, and bot actor detection across all stage templates

Related Issue

Part of GitLab forge support — ADR 0067 Phase 3.

Changes

  • internal/scaffold/fullsend-repo-gitlab/: Root pipeline, dispatch, poll, and 6 agent stage templates
  • internal/scaffold/scaffold.go: New embed directive, GitLabPerRepoFile() and WalkGitLabPerRepo() functions
  • internal/scaffold/scaffold_gitlab_test.go: Tests covering file existence, content validation, fork protection, credential validation, fullsend run flag correctness, and regression guards
  • docs/plans/gitlab-cron-polling-implementation.md: Updated Phase 3 examples to match real fullsend run flags
  • AGENTS.md: Document GitLab dispatch template and NormalizedEvent usage

Design Notes

  • Same execution model as GitHub: Stage templates call fullsend run <agent> --fullsend-dir .fullsend --target-repo . --forge gitlab ... — identical to what action.yml does. fullsend run resolves the harness file, reads the image: field, and creates the sandbox container via Podman.
  • No hardcoded agent images: All stage templates use ghcr.io/fullsend-ai/fullsend-sandbox:latest as the CI runner image. The agent-specific sandbox image comes from the harness file at runtime.
  • CEL-annotated shell routing: Each routing check in dispatch has a comment with the equivalent CEL expression, making the future swap to fullsend dispatch --input-driver json mechanical when built-in harness triggers land (feat(dispatch): port triage to harness CEL trigger #2896-2901)
  • STAGE allowlist gate: Defense-in-depth case check before the heredoc that generates child pipeline YAML

Testing

  • go test ./internal/scaffold/... passes (all GitLab + GitHub scaffold tests)
  • make lint passes
  • NormalizedEvent JSON matches normative examples

🤖 Generated with Claude Code

@ggallen
ggallen requested a review from a team as a code owner July 6, 2026 23:57
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 6, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 11:58 PM UTC · Completed 12:10 AM UTC
Commit: 54cfb94 · View workflow run →

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Add embedded GitLab per-repo CI/CD scaffold templates

✨ Enhancement ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

AI Description

• Add a GitLab per-repo scaffold with root pipeline, MR dispatch, and scheduled polling.
• Provide stage templates (triage/code/review/fix/retro/prioritize) supporting WIF or variable
 creds.
• Embed and expose GitLab scaffold files via new Go helper functions for install-time rendering.
Diagram

graph TD
  A["GitLab CI trigger"] --> B["Root .gitlab-ci.yml"]
  B --> C{"Source?"}
  C -->|"merge_request_event"| D["MR dispatch job"] --> E["MR child pipeline"] --> F["Stage template job"] --> G["fullsend CLI run"]
  C -->|"schedule (protected)"| H["Poll events job"] --> I["Generate child YAML"] --> J["Poller child pipeline"] --> F
  H --> K[("GCP Secret Manager")]

  subgraph Legend
    direction LR
    _p["Pipeline/job"] ~~~ _d{"Decision"} ~~~ _db[("Secret store")]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Use centralized GitLab CI templates (include:project) instead of per-repo scaffold
  • ➕ Single source of truth for pipeline logic across repositories
  • ➕ Easier patching of security fixes without reinstalling scaffold content
  • ➖ Requires hosting and access controls for a shared templates project
  • ➖ Harder to keep templates and per-repo customization in sync without additional tooling
2. Single pipeline with dynamic rules only (avoid child pipelines)
  • ➕ Simpler mental model and fewer generated artifacts
  • ➕ Fewer moving parts (no YAML generation step)
  • ➖ Hard to prevent non-matching stage jobs from appearing without STAGE in root context
  • ➖ More brittle rule logic; can increase noise and accidental job execution risk
3. Refactor stage templates to share a common script via YAML anchors/includes
  • ➕ Reduces duplication of WIF credential setup and CI_DEBUG_TRACE guard blocks
  • ➕ Lowers risk of stage drift (e.g., one stage missing a security guard)
  • ➖ YAML anchors can be harder to maintain/debug across multiple included files
  • ➖ May constrain future stage-specific deviations

Recommendation: Keep the PR’s child-pipeline approach: it cleanly separates root workflow routing from stage execution and avoids rule-evaluation pitfalls when STAGE is unset. If follow-up work is planned, consider extracting the repeated credential/guard logic into a shared include/anchor to reduce duplication while preserving the current security posture.

Files changed (16) +651 / -0

Enhancement (1) +27 / -0
scaffold.goEmbed and expose GitLab per-repo scaffold file tree +27/-0

Embed and expose GitLab per-repo scaffold file tree

• Embeds the new fullsend-repo-gitlab directory and adds helper functions to read a single scaffold file and to walk the embedded tree. Enables install-time rendering/iteration over the GitLab template set.

internal/scaffold/scaffold.go

Other (15) +624 / -0
config.yamlAdd GitLab forge config for Fullsend scaffold +4/-0

Add GitLab forge config for Fullsend scaffold

• Introduces a minimal .fullsend configuration file that declares the forge as GitLab for per-repo installs.

internal/scaffold/fullsend-repo-gitlab/.fullsend/config.yaml

.gitkeepAdd placeholder for customized agents directory +0/-0

Add placeholder for customized agents directory

• Adds a .gitkeep to ensure the customized agents directory exists in the scaffold tree.

internal/scaffold/fullsend-repo-gitlab/.fullsend/customized/agents/.gitkeep

.gitkeepAdd placeholder for customized harness directory +0/-0

Add placeholder for customized harness directory

• Adds a .gitkeep to ensure the customized harness directory exists in the scaffold tree.

internal/scaffold/fullsend-repo-gitlab/.fullsend/customized/harness/.gitkeep

.gitkeepAdd placeholder for customized policies directory +0/-0

Add placeholder for customized policies directory

• Adds a .gitkeep to ensure the customized policies directory exists in the scaffold tree.

internal/scaffold/fullsend-repo-gitlab/.fullsend/customized/policies/.gitkeep

.gitkeepAdd placeholder for customized scripts directory +0/-0

Add placeholder for customized scripts directory

• Adds a .gitkeep to ensure the customized scripts directory exists in the scaffold tree.

internal/scaffold/fullsend-repo-gitlab/.fullsend/customized/scripts/.gitkeep

.gitkeepAdd placeholder for customized skills directory +0/-0

Add placeholder for customized skills directory

• Adds a .gitkeep to ensure the customized skills directory exists in the scaffold tree.

internal/scaffold/fullsend-repo-gitlab/.fullsend/customized/skills/.gitkeep

.gitlab-ci.ymlAdd root GitLab pipeline routing MR vs scheduled poller runs +28/-0

Add root GitLab pipeline routing MR vs scheduled poller runs

• Defines the root pipeline stages and workflow rules to route merge request events to MR dispatch and scheduled runs (protected refs only) to the cron-poller path. Avoids including stage templates at the root to prevent non-matching jobs from appearing/executing.

internal/scaffold/fullsend-repo-gitlab/.gitlab-ci.yml

fullsend-code.ymlAdd GitLab stage template for code agent +63/-0

Add GitLab stage template for code agent

• Adds the code stage job using the fullsend-code image, with WIF credential retrieval support and CI_DEBUG_TRACE protection. Decodes the event payload, prepares the workspace, and runs the code stage with resource_group locking.

internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-code.yml

fullsend-dispatch.ymlAdd MR-only dispatch job that generates a child pipeline per MR state +79/-0

Add MR-only dispatch job that generates a child pipeline per MR state

• Implements an MR dispatch job that queries GitLab’s MR API to infer lifecycle state, selects the appropriate stage (review/retro/no-op), and writes a child pipeline YAML with STAGE/RESOURCE_KEY variables. Triggers the generated child pipeline via a dependent bridge job.

internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml

fullsend-fix.ymlAdd fix stage template with fork-MR protection +72/-0

Add fix stage template with fork-MR protection

• Adds the fix stage job using the fullsend-code image with WIF support and CI_DEBUG_TRACE guard. Includes explicit fork-MR detection to skip running fix when source/target projects differ, preventing unsafe writes to the target project.

internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-fix.yml

fullsend-poll.ymlAdd scheduled poller pipeline that generates and triggers child pipelines +107/-0

Add scheduled poller pipeline that generates and triggers child pipelines

• Adds scheduled polling for GitLab events (protected refs only), including WIF token exchange + Secret Manager retrieval in WIF mode. Generates a child pipeline YAML from poll results (or a no-op when empty) and triggers it with depend strategy.

internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-poll.yml

fullsend-prioritize.ymlAdd prioritize stage template +63/-0

Add prioritize stage template

• Adds the prioritize stage job with CI_DEBUG_TRACE protection, optional WIF-based credential retrieval, event payload decoding, workspace preparation, and fullsend stage execution.

internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-prioritize.yml

fullsend-retro.ymlAdd retro stage template +63/-0

Add retro stage template

• Adds the retro stage job with the standard security guards, optional WIF credential retrieval, event payload decoding, workspace preparation, and fullsend stage execution under a stage-specific resource group.

internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-retro.yml

fullsend-review.ymlAdd review stage template with dual event-source handling +82/-0

Add review stage template with dual event-source handling

• Adds the review stage job and supports both native MR pipelines (constructing payload from CI variables + MR API state) and polled events (base64 payload decode). Uses CI_DEBUG_TRACE guard, optional WIF credential retrieval, then runs the review stage.

internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-review.yml

fullsend-triage.ymlAdd triage stage template +63/-0

Add triage stage template

• Adds the triage stage job with CI_DEBUG_TRACE protection, optional WIF credential retrieval, event payload decoding, workspace preparation, and fullsend stage execution.

internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-triage.yml

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

Site preview

Preview: https://16f57603-site.fullsend-ai.workers.dev

Commit: 698aacf5f6dd6a780b23a67cc816473171da4d9e

@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@qodo-code-review

qodo-code-review Bot commented Jul 7, 2026

Copy link
Copy Markdown

Code Review by Qodo

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

Context used
✅ Compliance rules (platform): 54 rules

Grey Divider


Action required

1. GitLabPerRepoFile lacks tests 📘 Rule violation ▣ Testability
Description
New exported Go logic was added (GitLabPerRepoFile/WalkGitLabPerRepo) without any corresponding
unit tests updated or added in this PR. This violates the requirement that new/modified Go logic be
covered by tests, increasing regression risk for scaffold embedding/iteration behavior.
Code

internal/scaffold/scaffold.go[R243-265]

+// GitLabPerRepoFile returns the content of a file from the GitLab per-repo scaffold.
+// The path is relative to the fullsend-repo-gitlab root (e.g., ".gitlab-ci.yml").
+func GitLabPerRepoFile(path string) ([]byte, error) {
+	return gitlabContent.ReadFile("fullsend-repo-gitlab/" + path)
+}
+
+// WalkGitLabPerRepo calls fn for each file in the GitLab per-repo scaffold.
+func WalkGitLabPerRepo(fn func(path string, content []byte) error) error {
+	return fs.WalkDir(gitlabContent, "fullsend-repo-gitlab", func(path string, d fs.DirEntry, err error) error {
+		if err != nil {
+			return err
+		}
+		if d.IsDir() {
+			return nil
+		}
+		relPath := path[len("fullsend-repo-gitlab/"):]
+		data, readErr := gitlabContent.ReadFile(path)
+		if readErr != nil {
+			return fmt.Errorf("reading %s: %w", path, readErr)
+		}
+		return fn(relPath, data)
+	})
+}
Relevance

⭐⭐⭐ High

Team commonly adds tests for new scaffold/Go logic changes (new funcs shipped with tests in PR
#2261; more tests accepted in #2545).

PR-#2261
PR-#2545

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 1062049 requires tests for new or modified Go logic. The PR adds new exported
functions in internal/scaffold/scaffold.go, and there are no references to these functions in any
_test.go files in internal/scaffold/, indicating no tests were added/updated to cover this new
behavior.

Rule 1062049: Require tests for new or modified Go logic
internal/scaffold/scaffold.go[243-265]
internal/scaffold/scaffold_test.go[52-103]

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

## Issue description
New exported functions `GitLabPerRepoFile` and `WalkGitLabPerRepo` were introduced without tests, violating the repo requirement to add/adjust tests when Go logic changes.

## Issue Context
These functions are responsible for reading and walking the embedded `fullsend-repo-gitlab` scaffold tree. Regressions here can silently break GitLab template installation/iteration.

## Fix Focus Areas
- internal/scaffold/scaffold.go[243-265]
- internal/scaffold/scaffold_test.go[52-103]
- internal/scaffold/scaffold_gitlab_test.go[1-200]

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


2. Child pipeline missing stages ✓ Resolved 🐞 Bug ≡ Correctness
Description
The dispatch job generates mr-dispatch-pipeline.yml without any stages: list, but the included
stage templates define jobs with stage: agent, which will make the child pipeline invalid when
created. This prevents MR-dispatched agents from running at all.
Code

internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml[R52-62]

+      # Generate child pipeline YAML — STAGE is a pipeline-level
+      # variable so it is available during rules evaluation.
+      cat > mr-dispatch-pipeline.yml <<YAML
+      variables:
+        STAGE: "${STAGE}"
+        RESOURCE_KEY: "${RESOURCE_KEY}"
+        EVENT_TYPE: "merge_request_event"
+
+      include:
+        - local: .gitlab/ci/fullsend-${STAGE}.yml
+      YAML
Relevance

⭐⭐ Medium

No historical evidence for GitLab child-pipeline stage validation; only GitHub workflow robustness
patterns seen (PR #390).

PR-#390

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The generated child pipeline YAML omits stages: entirely, while included stage templates
explicitly set stage: agent, creating a stage name that is not declared within the child pipeline
config.

internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml[52-62]
internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-review.yml[4-6]
internal/scaffold/fullsend-repo-gitlab/.gitlab-ci.yml[15-20]

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

## Issue description
`mr-dispatch-pipeline.yml` is generated with only `variables:` and `include:`, but the included templates define jobs with `stage: agent`. Child pipeline configs must define the stages they use (or avoid custom stages), otherwise GitLab CI will reject the config.

## Issue Context
This breaks the MR dispatch path because the child pipeline created from `mr-dispatch-pipeline.yml` will fail config validation before any agent job can start.

## Fix Focus Areas
- internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml[52-62]
- internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-review.yml[4-6]

## Suggested fix
Update the heredoc that writes `mr-dispatch-pipeline.yml` to include a `stages:` section that contains `agent`, e.g.:

```yaml
stages:
 - agent
```

Alternatively (and more robust if stage templates are ever used as standalone child pipeline configs), add `stages: [agent]` (or equivalent) to each `fullsend-*.yml` stage template and keep the generated child pipeline minimal.

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



Remediation recommended

3. Retro dispatch missing payload ✓ Resolved 🐞 Bug ≡ Correctness
Description
In the merged-MR fallback, dispatch sets STAGE=retro but generates mr-dispatch-pipeline.yml
without EVENT_PAYLOAD_B64, while fullsend-retro.yml unconditionally decodes EVENT_PAYLOAD_B64
under set -u. This makes the retro fallback path fail at runtime with an unbound variable error.
Code

internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml[R26-62]

+      case "${MR_STATE}" in
+        merged)
+          # Race-condition fallback: merge_request_event does not fire on
+          # MR merge, but a pipeline triggered just before merge may query
+          # state and find "merged". Primary retro path is the cron-poller.
+          STAGE=retro
+          RESOURCE_KEY="mr-${CI_MERGE_REQUEST_IID}"
+          ;;
+        opened)
+          # GitLab returns "opened" for both new and reopened MRs —
+          # there is no separate "reopened" state in the MR API response.
+          STAGE=review
+          RESOURCE_KEY="mr-${CI_MERGE_REQUEST_IID}"
+          ;;
+        closed)
+          echo "MR closed without merge — no dispatch"
+          echo 'no-op: { script: ["echo MR closed — no dispatch"], rules: [{ when: always }] }' > mr-dispatch-pipeline.yml
+          exit 0
+          ;;
+        *)
+          echo "Unhandled MR state: ${MR_STATE}"
+          echo 'no-op: { script: ["echo Unhandled MR state"], rules: [{ when: always }] }' > mr-dispatch-pipeline.yml
+          exit 0
+          ;;
+      esac
+
+      # Generate child pipeline YAML — STAGE is a pipeline-level
+      # variable so it is available during rules evaluation.
+      cat > mr-dispatch-pipeline.yml <<YAML
+      variables:
+        STAGE: "${STAGE}"
+        RESOURCE_KEY: "${RESOURCE_KEY}"
+        EVENT_TYPE: "merge_request_event"
+
+      include:
+        - local: .gitlab/ci/fullsend-${STAGE}.yml
+      YAML
Relevance

⭐⭐ Medium

No GitLab scaffold history; team often accepts set -u unbound-var guards in CI scripts (e.g., PR
#2456).

PR-#2456

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The merged-MR branch selects retro but the generated child pipeline sets only
STAGE/RESOURCE_KEY/EVENT_TYPE; the retro stage script always runs `echo "${EVENT_PAYLOAD_B64}" |
base64 -d ..., which fails if the variable is unset under set -u`.

internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml[26-62]
internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-retro.yml[43-47]

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 dispatch job's `merged)` fallback routes to the `retro` stage but does not provide the event payload variable (`EVENT_PAYLOAD_B64`) that the `retro` stage template requires. With `set -u`, this causes an immediate job failure.

## Issue Context
This is explicitly described as a race-condition fallback, but the current implementation will reliably fail whenever that fallback triggers.

## Fix Focus Areas
- internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml[26-62]
- internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-retro.yml[43-47]

## Suggested fix
Choose one:
1) Make the `merged)` branch produce a no-op child pipeline (since retro is “primary via cron-poller”), avoiding a broken fallback.
2) Or make `fullsend-retro.yml` handle native `merge_request_event` the same way as `fullsend-review.yml`: build an event payload file from GitLab MR CI variables (and/or query MR details) instead of requiring `EVENT_PAYLOAD_B64`.
3) Or in the `merged)` branch, construct an appropriate payload JSON, base64-encode it, and inject it into `mr-dispatch-pipeline.yml` as `EVENT_PAYLOAD_B64` so the retro template can decode it safely.

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


Grey Divider

Qodo Logo

Comment thread internal/scaffold/scaffold.go
Comment thread internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml Outdated
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 7, 2026

Copy link
Copy Markdown

Review

This is a re-review. The prior review (SHA 99bced6) had 1 medium and 7 low findings.

Prior finding Status
[protected-path] medium — images/runner/Containerfile Unchanged — human approval required
[documentation-implementation-inconsistency] low — plan doc parent_pipeline ✅ Resolved — parent_pipeline rule removed from plan
[edge-case] low — MR_AUTHOR_ID check (agent.yml) Unchanged — fail-closed
[defense-in-depth] low — convert.go IsFork guard Unchanged — defense-in-depth
[defense-in-depth] low — dispatch.go fork detection Unchanged — fail-closed
[edge-case] low — is_entity_author hardcoded (dispatch.yml) Unchanged — correct for MR-only path
[injection] low — heredoc unquoted across templates Unchanged — inputs constrained
[authorization] low — Members API CI_PROJECT_ID fallback + chain Unchanged — fail-closed

Findings

Medium

  • [protected-path] images/README.md, images/runner/Containerfile — This PR modifies files under protected paths. images/runner/Containerfile adds PyYAML 6.0.2 as a dependency for GitLab scaffold config parsing (+5/−1 lines). images/README.md updates the dependency list to include pyyaml (+2/−2 lines). Human approval is always required for protected-path changes, regardless of context.

Low

  • [edge-case] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:112 — The authorization gate checks MR_AUTHOR_ID for non-retro/non-prioritize stages and skips (exit 0) when unset. Issue events have MRAuthorID=0 (omitted from child pipeline YAML by the conditional at dispatch.go:148), so issue-triggered code/fix stages will always exit 0 (fail-closed). The LIMITATION comment at line 107 acknowledges this — generalizing to ACTOR_ID is deferred until non-MR event routing is enabled.

  • [defense-in-depth] internal/poll/convert.go:293 — The IsFork guard in buildChangeProposalState (event.MRSource == 0 || ...) is defense-in-depth: when either project ID is zero, GetProjectPath(ctx, 0) errors first, so the guard is unreachable in that case. The fail-closed form is a correct security improvement.

  • [defense-in-depth] internal/poll/dispatch.go:64 — Fork detection in dispatch() uses fail-closed logic: for MR events, isFork defaults to true, only set to the actual comparison when both MRSource and MRTarget are non-zero. Issue events correctly default to isFork=false. Well-covered by 5 test cases.

  • [edge-case] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml:144 — The dispatch template hardcodes actor.is_entity_author to true in the NormalizedEvent. Correct for MR-triggered pipelines where the actor is always the MR author. Would break if comment-triggered dispatch were added via this path, but that uses the cron-poller instead.

  • [injection] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml, fullsend-agent.yml, fullsend-poll.yml — Unquoted heredocs (<<YAML, <<CRED) allow shell expansion across all three scaffold templates. Currently safe: in dispatch, STAGE is constrained by case-statement allowlist (review|retro), RESOURCE_KEY is integer-derived, IS_FORK is true/false, MR_AUTHOR_ID is integer from API. In agent/poll, WIF variables are sourced from CI/CD protected variables set at install time.

  • [authorization] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:127 — The Members API authorization call uses ${CI_MERGE_REQUEST_PROJECT_ID:-${CI_PROJECT_ID}} for the target project. In the cron-poller child pipeline path, CI_MERGE_REQUEST_PROJECT_ID is not set, so the fallback to CI_PROJECT_ID is used. Currently correct — CI_PROJECT_ID in a GitLab child pipeline always refers to the project that owns the pipeline configuration. Authorization chain is defense-in-depth sound and fail-closed at every step.

  • [stale-doc] docs/problems/gitlab-implementation.md:246 — The dispatch workflow example describes scanning .gitlab/ci/*.yml for # fullsend-stage: markers with per-stage template files. The actual implementation uses a single generic fullsend-agent.yml template parameterized by $STAGE. This is a historical problem document superseded by ADR 0067 — low impact, but future readers may mistake the described approach for the current architecture.

Previous run

Review

This is a re-review. The prior review (SHA d93783c) had 1 medium and 7 low findings.

Prior finding Status
[protected-path] medium — images/runner/Containerfile Unchanged — human approval required
[edge-case] low — MR_AUTHOR_ID check (agent.yml) Unchanged — fail-closed
[defense-in-depth] low — convert.go IsFork guard Unchanged — defense-in-depth
[defense-in-depth] low — dispatch.go fork detection Unchanged — fail-closed
[edge-case] low — is_entity_author hardcoded (dispatch.yml) Unchanged — correct for MR-only path
[injection] low — heredoc unquoted across templates Unchanged — inputs constrained
[authorization] low — Members API CI_PROJECT_ID fallback + chain Unchanged — fail-closed
[stale-doc] low — images/README.md pyyaml ✅ Resolved — README already updated in PR

Findings

Medium

  • [protected-path] images/README.md, images/runner/Containerfile — This PR modifies files under protected paths. images/runner/Containerfile adds PyYAML 6.0.2 as a dependency for GitLab scaffold config parsing (+5/−1 lines). images/README.md updates the dependency list to include pyyaml (+2/−2 lines). Human approval is always required for protected-path changes, regardless of context.

Low

  • [documentation-implementation-inconsistency] docs/plans/gitlab-cron-polling-implementation.md:1228 — The plan document's .gitlab-ci.yml pseudocode includes a parent_pipeline workflow rule that the actual scaffold at internal/scaffold/fullsend-repo-gitlab/.gitlab-ci.yml intentionally omits. The test TestGitLabRootPipelineContent explicitly asserts NotContains for parent_pipeline. The plan was partially updated in this PR (per-stage templates replaced with generic agent template) but this stale rule was missed.

  • [edge-case] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:112 — The authorization gate checks MR_AUTHOR_ID for non-retro/non-prioritize stages and skips (exit 0) when unset. Issue events have MRAuthorID=0 (omitted from child pipeline YAML by the conditional at dispatch.go:155), so issue-triggered code/fix stages will always exit 0 (fail-closed). The LIMITATION comment at line 107 acknowledges this — generalizing to ACTOR_ID is deferred until non-MR event routing is enabled.

  • [defense-in-depth] internal/poll/convert.go:295 — The IsFork guard in buildChangeProposalState (event.MRSource == 0 || ...) is defense-in-depth: when either project ID is zero, GetProjectPath(ctx, 0) errors first, so the guard is unreachable in that case. The fail-closed form is a correct security improvement.

  • [defense-in-depth] internal/poll/dispatch.go:73 — Fork detection in dispatch() uses fail-closed logic: for MR events, isFork defaults to true, only set to the actual comparison when both MRSource and MRTarget are non-zero. Issue events correctly default to isFork=false. Well-covered by 5 test cases.

  • [edge-case] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml:144 — The dispatch template hardcodes actor.is_entity_author to true in the NormalizedEvent. Correct for MR-triggered pipelines where the actor is always the MR author. Would break if comment-triggered dispatch were added via this path, but that uses the cron-poller instead.

  • [injection] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml, fullsend-agent.yml, fullsend-poll.yml — Unquoted heredocs (<<YAML, <<CRED) allow shell expansion across all three scaffold templates. Currently safe: in dispatch, STAGE is constrained by case-statement allowlist (review|retro), RESOURCE_KEY is integer-derived, IS_FORK is true/false, MR_AUTHOR_ID is integer from API. In agent/poll, WIF variables are sourced from CI/CD protected variables set at install time.

  • [authorization] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:127 — The Members API authorization call uses ${CI_MERGE_REQUEST_PROJECT_ID:-${CI_PROJECT_ID}} for the target project. In the cron-poller child pipeline path, CI_MERGE_REQUEST_PROJECT_ID is not set, so the fallback to CI_PROJECT_ID is used. Currently correct — CI_PROJECT_ID in a GitLab child pipeline always refers to the project that owns the pipeline configuration. Authorization chain is defense-in-depth sound and fail-closed at every step.

Previous run (2)

Review

This is a re-review. The prior review (SHA fb87d08) had 1 medium and 6 low findings.

Prior finding Status
[protected-path] medium — AGENTS.md, images/runner/Containerfile Updated — AGENTS.md no longer in PR; Containerfile unchanged
[edge-case] low — MR_AUTHOR_ID check (agent.yml) Unchanged — fail-closed
[defense-in-depth] low — convert.go IsFork guard Unchanged — defense-in-depth
[defense-in-depth] low — dispatch.go fork detection Unchanged — fail-closed
[edge-case] low — is_entity_author hardcoded (dispatch.yml) Unchanged — correct for MR-only path
[injection] low — heredoc unquoted across templates Unchanged — inputs constrained
[authorization] low — Members API CI_PROJECT_ID fallback + chain Unchanged — fail-closed

Findings

Medium

  • [protected-path] images/runner/Containerfile — This PR modifies a file under a protected path. images/runner/Containerfile adds PyYAML 6.0.2 as a dependency for GitLab scaffold config parsing (+5/−1 lines). Human approval is always required for protected-path changes, regardless of context.

Low

  • [edge-case] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:112 — The authorization gate checks MR_AUTHOR_ID for non-retro/non-prioritize stages and skips (exit 0) when unset. Issue events have MRAuthorID=0 (omitted from child pipeline YAML by the conditional at dispatch.go:455), so issue-triggered code/fix stages will always exit 0 (fail-closed). The LIMITATION comment at line 112 acknowledges this — generalizing to ACTOR_ID is deferred until non-MR event routing is enabled.

  • [defense-in-depth] internal/poll/convert.go:295 — The IsFork guard in buildChangeProposalState (event.MRSource == 0 || ...) is defense-in-depth: when either project ID is zero, GetProjectPath(ctx, 0) errors first, so the guard is unreachable in that case. The fail-closed form is a correct security improvement.

  • [defense-in-depth] internal/poll/dispatch.go:73 — Fork detection in dispatch() uses fail-closed logic: for MR events, isFork defaults to true, only set to the actual comparison when both MRSource and MRTarget are non-zero. Issue events correctly default to isFork=false. Well-covered by 5 test cases.

  • [edge-case] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml:144 — The dispatch template hardcodes actor.is_entity_author to true in the NormalizedEvent. Correct for MR-triggered pipelines where the actor is always the MR author. Would break if comment-triggered dispatch were added via this path, but that uses the cron-poller instead.

  • [injection] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml, fullsend-agent.yml, fullsend-poll.yml — Unquoted heredocs (<<YAML, <<CRED) allow shell expansion across all three scaffold templates. Currently safe: in dispatch, STAGE is constrained by case-statement allowlist (review|retro), RESOURCE_KEY is integer-derived, IS_FORK is true/false, MR_AUTHOR_ID is integer from API. In agent/poll, WIF variables are sourced from CI/CD protected variables set at install time.

  • [authorization] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:127 — The Members API authorization call uses ${CI_MERGE_REQUEST_PROJECT_ID:-${CI_PROJECT_ID}} for the target project. In the cron-poller child pipeline path, CI_MERGE_REQUEST_PROJECT_ID is not set, so the fallback to CI_PROJECT_ID is used. Currently correct — CI_PROJECT_ID in a GitLab child pipeline always refers to the project that owns the pipeline configuration. Authorization chain is defense-in-depth sound and fail-closed at every step.

  • [stale-doc] images/README.md — The runner image dependency list does not mention PyYAML, which was added in this PR. The README lists other pip packages (pre-commit, gitlint, jsonschema) but not the new pyyaml package. The Containerfile is the source of truth; the README is a shorthand summary.

Previous run (3)

Review

This is a re-review. The prior review (SHA abc700b) had 1 medium and 9 low findings.

Prior finding Status
[protected-path] medium — AGENTS.md, images/runner/Containerfile Unchanged — human approval required
[edge-case] low — MR_AUTHOR_ID check (agent.yml) Unchanged — fail-closed
[defense-in-depth] low — convert.go IsFork guard Unchanged — defense-in-depth
[defense-in-depth] low — dispatch.go fork detection Unchanged — fail-closed
[edge-case] low — is_entity_author hardcoded (dispatch.yml) Unchanged — correct for MR-only path
[injection] low — heredoc unquoted across templates Unchanged — inputs constrained
[authorization] low — Members API CI_PROJECT_ID fallback + chain Unchanged — fail-closed
[scope-creep] low — internal/poll/ modifications Dropped — not re-raised
[architectural-coherence] low — shell authorization gate Dropped — tracked by #5416
[naming-consistency] low — GitLabPerRepoFile naming Dropped — correct naming confirmed

Findings

Medium

  • [protected-path] AGENTS.md, images/runner/Containerfile — This PR modifies files under protected paths. AGENTS.md documents the new GitLab scaffold execution model (+2/−1 lines). images/runner/Containerfile adds PyYAML as a dependency for GitLab scaffold config parsing (+5/−1 lines). Human approval is always required for protected-path changes, regardless of context.

Low

  • [edge-case] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:126 — The authorization gate checks MR_AUTHOR_ID for non-retro/non-prioritize stages and skips (exit 0) when unset. Issue events have MRAuthorID=0 (omitted from child pipeline YAML by the conditional at dispatch.go:132), so issue-triggered code/fix stages will always exit 0 (fail-closed). The LIMITATION comment at line 118 acknowledges this — generalizing to ACTOR_ID is deferred until non-MR event routing is enabled.

  • [defense-in-depth] internal/poll/convert.go:295 — The IsFork guard in buildChangeProposalState (event.MRSource == 0 || ...) is defense-in-depth: when either project ID is zero, GetProjectPath(ctx, 0) errors first, so the guard is unreachable in that case. The fail-closed form is a correct security improvement.

  • [defense-in-depth] internal/poll/dispatch.go:73 — Fork detection in dispatch() uses fail-closed logic: for MR events, isFork defaults to true, only set to the actual comparison when both MRSource and MRTarget are non-zero. Issue events correctly default to isFork=false. Well-covered by 5 test cases.

  • [edge-case] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml:168 — The dispatch template hardcodes actor.is_entity_author to true in the NormalizedEvent. Correct for MR-triggered pipelines where the actor is always the MR author. Would break if comment-triggered dispatch were added via this path, but that uses the cron-poller instead.

  • [injection] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml:216, fullsend-agent.yml, fullsend-poll.yml — Unquoted heredocs (<<YAML, <<CRED) allow shell expansion across all three scaffold templates. Currently safe: in dispatch, STAGE is constrained by case-statement allowlist (review|retro), RESOURCE_KEY is integer-derived, IS_FORK is true/false, MR_AUTHOR_ID is integer from API. In agent/poll, WIF variables are sourced from CI/CD protected variables set at install time.

  • [authorization] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:127 — The Members API authorization call uses ${CI_MERGE_REQUEST_PROJECT_ID:-${CI_PROJECT_ID}} for the target project. In the cron-poller child pipeline path, CI_MERGE_REQUEST_PROJECT_ID is not set, so the fallback to CI_PROJECT_ID is used. Currently correct — CI_PROJECT_ID in a GitLab child pipeline always refers to the project that owns the pipeline configuration. Authorization chain is defense-in-depth sound and fail-closed at every step.


Labels: PR modifies docs/ADRs and docs/plans alongside scaffold and Go code — component/docs applies per repo convention of labeling doc changes alongside code component labels.

Previous run (4)

Review

This is a re-review. The prior review (SHA ac1f222) had 1 medium and 9 low findings.

Prior finding Status
[protected-path] medium — AGENTS.md, images/runner/Containerfile Unchanged — human approval required
[edge-case] low — MR_AUTHOR_ID check (agent.yml) Unchanged — fail-closed
[defense-in-depth] low — convert.go IsFork guard Unchanged — defense-in-depth
[defense-in-depth] low — dispatch.go fork detection Unchanged — fail-closed
[edge-case] low — MR event type prefix check Dropped — not re-raised by correctness review
[injection] low — heredoc unquoted across templates Unchanged — inputs constrained
[authorization] low — Members API CI_PROJECT_ID fallback Merged with authorization chain
[authorization] low — authorization chain Merged with Members API fallback
[scope-creep] low — internal/poll/ modifications Unchanged — coupling justified
[architectural-coherence] low — shell authorization gate Unchanged — tracked by #5416

Findings

Medium

  • [protected-path] AGENTS.md, images/runner/Containerfile — This PR modifies files under protected paths. AGENTS.md documents the new GitLab scaffold execution model (+2/−1 lines). images/runner/Containerfile adds PyYAML as a dependency for GitLab scaffold config parsing (+5/−1 lines). Human approval is always required for protected-path changes, regardless of context.

Low

  • [edge-case] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:126 — The authorization gate checks MR_AUTHOR_ID for non-retro/non-prioritize stages and skips (exit 0) when unset. Issue events have MRAuthorID=0, so issue-triggered code/fix stages will always exit 0 (fail-closed). The LIMITATION comment at line 118 acknowledges this — generalizing to ACTOR_ID is deferred until non-MR event routing is enabled.

  • [defense-in-depth] internal/poll/convert.go:293 — The IsFork guard in buildChangeProposalState (event.MRSource == 0 || ...) is defense-in-depth: when either project ID is zero, GetProjectPath(ctx, 0) errors first, so the guard is unreachable in that case. The fail-closed form is a security improvement.

  • [defense-in-depth] internal/poll/dispatch.go:63 — Fork detection in dispatch() uses fail-closed logic: for MR events, isFork defaults to true, only set to the actual comparison when both MRSource and MRTarget are non-zero. Issue events correctly default to isFork=false. Well-covered by 5 test cases.

  • [edge-case] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml:175 — The dispatch template hardcodes actor.is_entity_author to true in the NormalizedEvent. Correct for MR-triggered pipelines where the actor is always the MR author. Would break if comment-triggered dispatch were added via this path, but that uses the cron-poller instead.

  • [injection] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml:232, fullsend-agent.yml, fullsend-poll.yml — Unquoted heredocs (<<YAML, <<CRED) allow shell expansion across all three scaffold templates. Currently safe: in dispatch, STAGE is constrained by case-statement allowlist (review|retro), RESOURCE_KEY is integer-derived, IS_FORK is true/false, MR_AUTHOR_ID is integer from API. In agent/poll, WIF variables are sourced from CI/CD protected variables set at install time.

  • [authorization] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:127 — The Members API authorization call uses ${CI_MERGE_REQUEST_PROJECT_ID:-${CI_PROJECT_ID}} for the target project. In the cron-poller child pipeline path, CI_MERGE_REQUEST_PROJECT_ID is not set, so the fallback to CI_PROJECT_ID is used. Currently correct — CI_PROJECT_ID in a GitLab child pipeline always refers to the project that owns the pipeline configuration. Authorization chain is defense-in-depth sound and fail-closed at every step.

  • [scope-creep] internal/poll/dispatch.go — This PR modifies internal/poll/ files (dispatch.go, types.go, convert.go, dispatch_test.go) alongside the Phase 3 scaffold templates. These changes add MRAuthorID, IsFork, and IID to the Dispatch struct and the child pipeline YAML generation — directly required by the scaffold agent template's authorization gate, fork protection, and --status-number flag. The coupling is justified; the implementation plan's phase boundaries are a planning tool, not a hard scope gate.

  • [architectural-coherence] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml — Authorization gate (kill switch, role enablement, Members API check) is implemented in shell rather than Go code. Issue Consolidate kill switch and role enablement checks into fullsend run #5416 tracks consolidation into fullsend run so both forges share Go-based authorization. The current shell implementation is correct and acceptable as documented follow-up work.

  • [naming-consistency] internal/scaffold/scaffold.go — Function name GitLabPerRepoFile uses mixed-case 'GitLab' while the existing GitHub embed variable uses a single lowercase word (content). This follows Go exported naming conventions for proper nouns and is correct — the deviation from the existing naming pattern is intentional for brand accuracy.

Previous run (5)

Review

This is a re-review. The prior review (SHA 65a3fcf) had 1 medium and 8 low findings.

Prior finding Status
[protected-path] medium — AGENTS.md, images/runner/Containerfile Unchanged — human approval required
[edge-case] low — MR_AUTHOR_ID check (agent.yml) Unchanged — fail-closed
[defense-in-depth] low — convert.go IsFork guard Unchanged — defense-in-depth
[defense-in-depth] low — dispatch.go fork detection Unchanged — fail-closed
[edge-case] low — MR event type prefix check Unchanged — convention correct
[injection] low — heredoc unquoted across templates Unchanged — inputs constrained
[authorization] low — Members API CI_PROJECT_ID fallback Unchanged — correct for child pipelines
[authorization] low — authorization chain Unchanged — fail-closed
[inconsistency] low — Plan doc --gitlab-url default ✅ Resolved — now uses ${CI_SERVER_URL} matching actual template

Findings

Medium

  • [protected-path] AGENTS.md, images/runner/Containerfile — This PR modifies files under protected paths. AGENTS.md documents the new GitLab scaffold execution model (+2/−1 lines). images/runner/Containerfile adds PyYAML as a dependency for GitLab scaffold config parsing (+5/−1 lines). Human approval is always required for protected-path changes, regardless of context.

Low

  • [edge-case] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:126 — The authorization gate checks MR_AUTHOR_ID for non-retro/non-prioritize stages and skips (exit 0) when unset. Issue events have MRAuthorID=0, so issue-triggered code/fix stages will always exit 0 (fail-closed). The LIMITATION comment at line 118 acknowledges this — generalizing to ACTOR_ID is deferred until non-MR event routing is enabled.

  • [defense-in-depth] internal/poll/convert.go:293 — The IsFork guard in buildChangeProposalState (event.MRSource == 0 || ...) is defense-in-depth: when either project ID is zero, GetProjectPath(ctx, 0) errors first, so the guard is unreachable in that case. The fail-closed form is a security improvement.

  • [defense-in-depth] internal/poll/dispatch.go:63 — Fork detection in dispatch() uses fail-closed logic: for MR events, isFork defaults to true, only set to the actual comparison when both MRSource and MRTarget are non-zero. Issue events correctly default to isFork=false. Well-covered by 5 test cases.

  • [edge-case] internal/poll/dispatch.go:63 — The MR event type prefix check (strings.HasPrefix(event.Type, "mr_")) relies on the naming convention that all MR-related event types start with mr_. Correct for the current set (mr_event, mr_note) and consistent with resourceKey().

  • [injection] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml:232, fullsend-agent.yml:36, fullsend-poll.yml — Unquoted heredocs (<<YAML, <<CRED) allow shell expansion across all three scaffold templates. Currently safe: in dispatch, STAGE is constrained by case-statement allowlist (review|retro), RESOURCE_KEY is integer-derived, IS_FORK is true/false, MR_AUTHOR_ID is integer from API. In agent/poll, WIF variables are sourced from CI/CD protected variables set at install time.

  • [authorization] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:127 — The Members API authorization call uses ${CI_MERGE_REQUEST_PROJECT_ID:-${CI_PROJECT_ID}} for the target project. In the cron-poller child pipeline path, CI_MERGE_REQUEST_PROJECT_ID is not set, so the fallback to CI_PROJECT_ID is used. Currently correct — CI_PROJECT_ID in a GitLab child pipeline always refers to the project that owns the pipeline configuration.

  • [authorization] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml — Authorization chain is defense-in-depth sound and fail-closed at every step: credential mode validation, forge token check, config read from default branch (no fallback), kill switch, role enablement, Members API with default AUTHOR_ACCESS=0, MR_AUTHOR_ID check, fork protection with IS_FORK:-true default.

  • [scope-creep] internal/poll/dispatch.go — This PR modifies internal/poll/ files (dispatch.go, types.go, convert.go, dispatch_test.go) alongside the Phase 3 scaffold templates. These changes add MRAuthorID, IsFork, and IID to the Dispatch struct and the child pipeline YAML generation — directly required by the scaffold agent template's authorization gate, fork protection, and --status-number flag. The coupling is justified; the implementation plan's phase boundaries are a planning tool, not a hard scope gate.

  • [architectural-coherence] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml — Authorization gate (kill switch, role enablement, Members API check) is implemented in shell rather than Go code. Issue Consolidate kill switch and role enablement checks into fullsend run #5416 tracks consolidation into fullsend run so both forges share Go-based authorization. The current shell implementation is correct and acceptable as documented follow-up work.

Previous run (6)

Review

This is a re-review. The prior review (SHA b650621) had 1 medium and 8 low findings.

Prior finding Status
[protected-path] medium — AGENTS.md, images/runner/Containerfile Unchanged — human approval required
[edge-case] low — MR_AUTHOR_ID check (agent.yml) Unchanged — fail-closed
[logic-error] low — convert.go IsFork guard Unchanged — defense-in-depth
[defense-in-depth] low — dispatch.go fork detection Unchanged — fail-closed
[edge-case] low — MR event type prefix check Unchanged — convention correct
[injection] low — heredoc unquoted across templates Unchanged — inputs constrained
[authorization] low — Members API CI_PROJECT_ID fallback Unchanged — correct for child pipelines
[authorization] low — authorization chain Unchanged — fail-closed
[inconsistency] low — Plan doc --gitlab-url default ✅ Resolved — now uses ${CI_SERVER_URL} matching actual template

Findings

Medium

  • [protected-path] AGENTS.md, images/runner/Containerfile — This PR modifies files under protected paths. AGENTS.md documents the new GitLab scaffold execution model (+2/−1 lines). images/runner/Containerfile adds PyYAML as a dependency for GitLab scaffold config parsing (+5/−1 lines). Human approval is always required for protected-path changes, regardless of context.

Low

  • [edge-case] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:126 — The authorization gate checks MR_AUTHOR_ID for non-retro/non-prioritize stages and skips (exit 0) when unset. Issue events have MRAuthorID=0, so issue-triggered code/fix stages will always exit 0 (fail-closed). The LIMITATION comment at line 118 acknowledges this — generalizing to ACTOR_ID is deferred until non-MR event routing is enabled.

  • [defense-in-depth] internal/poll/convert.go:293 — The IsFork guard in buildChangeProposalState (event.MRSource == 0 || ...) is defense-in-depth: when either project ID is zero, GetProjectPath(ctx, 0) errors first, so the guard is unreachable in that case. The change from event.MRSource != event.MRTarget to the fail-closed form is a security improvement.

  • [defense-in-depth] internal/poll/dispatch.go:58 — Fork detection in dispatch() uses fail-closed logic: for MR events, isFork defaults to true, only set to the actual comparison when both MRSource and MRTarget are non-zero. Issue events correctly default to isFork=false. Well-covered by 5+ test cases.

  • [edge-case] internal/poll/dispatch.go:63 — The MR event type prefix check (strings.HasPrefix(event.Type, "mr_")) relies on the naming convention that all MR-related event types start with mr_. Correct for the current set (mr_event, mr_note) and consistent with resourceKey().

  • [injection] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml:232, fullsend-agent.yml:36, fullsend-poll.yml — Unquoted heredocs (<<YAML, <<CRED) allow shell expansion across all three scaffold templates. Currently safe: in dispatch, STAGE is constrained by case-statement allowlist (review|retro), RESOURCE_KEY is integer-derived, IS_FORK is true/false, MR_AUTHOR_ID is integer from API. In agent/poll, WIF variables are sourced from CI/CD protected variables set at install time.

  • [authorization] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:127 — The Members API authorization call uses ${CI_MERGE_REQUEST_PROJECT_ID:-${CI_PROJECT_ID}} for the target project. In the cron-poller child pipeline path, CI_MERGE_REQUEST_PROJECT_ID is not set, so the fallback to CI_PROJECT_ID is used. Currently correct — CI_PROJECT_ID in a GitLab child pipeline always refers to the project that owns the pipeline configuration.

  • [authorization] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml — Authorization chain is defense-in-depth sound and fail-closed at every step: credential mode validation, forge token check, config read from default branch (no fallback), kill switch, role enablement, Members API with default AUTHOR_ACCESS=0, MR_AUTHOR_ID check, fork protection with IS_FORK:-true default.

Previous run (7)

Review

This is a re-review. The prior review (SHA cd86eae) had 1 medium and 8 low findings.

Prior finding Status
[protected-path] medium — AGENTS.md, images/runner/Containerfile Unchanged — human approval required
[logic-error] low — convert.go IsFork guard unreachable Unchanged — defense-in-depth
[edge-case] low — MR event type prefix check Unchanged — convention correct
[defense-in-depth] low — dispatch.go fork detection Unchanged — fail-closed
[injection] low — heredoc unquoted across templates Unchanged — inputs constrained
[authorization] low — Members API CI_PROJECT_ID fallback Unchanged — correct for child pipelines
[authorization] low — authorization chain Unchanged — fail-closed
[inconsistency] low — Plan doc pseudocode without :- ✅ Resolved — credential mode pseudocode replaced

Findings

Medium

  • [protected-path] AGENTS.md, images/runner/Containerfile — This PR modifies files under protected paths. AGENTS.md documents the new GitLab scaffold execution model (+2/−1 lines). images/runner/Containerfile adds PyYAML as a dependency for GitLab scaffold config parsing (+5/−1 lines). Human approval is always required for protected-path changes, regardless of context.

Low

  • [edge-case] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:126 — The authorization gate checks MR_AUTHOR_ID for non-retro/non-prioritize stages and skips (exit 0) when unset. Issue events have MRAuthorID=0, so issue-triggered code/fix stages will always exit 0 (fail-closed). The LIMITATION comment at line 118 acknowledges this — generalizing to ACTOR_ID is deferred until non-MR event routing is enabled.

  • [logic-error] internal/poll/convert.go:293 — The IsFork guard in buildChangeProposalState (event.MRSource == 0 || ...) is defense-in-depth: when either project ID is zero, GetProjectPath(ctx, 0) errors first, so the guard is unreachable in that case. The change from event.MRSource != event.MRTarget to the fail-closed form is a security improvement.

  • [defense-in-depth] internal/poll/dispatch.go:58 — Fork detection in dispatch() uses fail-closed logic: for MR events, isFork defaults to true, only set to the actual comparison when both MRSource and MRTarget are non-zero. Issue events correctly default to isFork=false. IsFork is a bool — there is no way to distinguish "not applicable" from "definitely not a fork" in the Dispatch struct. Currently acceptable because fork protection only restricts, never grants permissions. Well-covered by 5+ test cases.

  • [edge-case] internal/poll/dispatch.go:63 — The MR event type prefix check (strings.HasPrefix(event.Type, "mr_")) relies on the naming convention that all MR-related event types start with mr_. Correct for the current set (mr_event, mr_note) and consistent with resourceKey().

  • [injection] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml:143, fullsend-agent.yml, fullsend-poll.yml — Unquoted heredocs (<<YAML, <<CRED) allow shell expansion across all three scaffold templates. Currently safe: in dispatch, STAGE is constrained by case-statement allowlist (review|retro), RESOURCE_KEY is integer-derived, IS_FORK is true/false, MR_AUTHOR_ID is integer from API. In agent/poll, WIF variables are sourced from CI/CD protected variables set at install time.

  • [authorization] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:127 — The Members API authorization call uses ${CI_MERGE_REQUEST_PROJECT_ID:-${CI_PROJECT_ID}} for the target project. In the cron-poller child pipeline path, CI_MERGE_REQUEST_PROJECT_ID is not set, so the fallback to CI_PROJECT_ID is used. Currently correct — CI_PROJECT_ID in a GitLab child pipeline always refers to the project that owns the pipeline configuration.

  • [authorization] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml — Authorization chain is defense-in-depth sound and fail-closed at every step: credential mode validation, forge token check, config read from default branch (no fallback), kill switch, role enablement, Members API with default AUTHOR_ACCESS=0, MR_AUTHOR_ID check, fork protection with IS_FORK:-true default.

  • [inconsistency] docs/plans/gitlab-cron-polling-implementation.md — The plan document was partially updated in this PR but still contains stale references: poll template pseudocode defaults --gitlab-url to https://gitlab.com while the actual scaffold template uses ${CI_SERVER_URL}.


Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

  • AGENTS.md
  • images/runner/Containerfile
Previous run (8)

Review

This is a re-review. The prior review (SHA d7da5f1) had 1 medium and 8 low findings.

Prior finding Status
[protected-path] medium — AGENTS.md, images/runner/Containerfile Unchanged — human approval required
[logic-error] low — Plan doc pseudocode without :- ✅ Resolved — credential mode pseudocode replaced with placeholder comment
[authorization] low — Members API CI_MERGE_REQUEST_PROJECT_ID fallback Unchanged — CI_PROJECT_ID correct for child pipelines
[logic-error] low — convert.go IsFork guard unreachable Unchanged — defense-in-depth
[edge-case] low — MR event type prefix check Unchanged — convention correct for current types
[injection] low — heredoc <<YAML unquoted Merged with WIF credential heredoc — same class of unquoted heredocs across templates
[injection] low — WIF credential heredoc Merged with YAML heredoc
[authorization] low — authorization chain Unchanged — fail-closed
[authorization] low — dispatch.go fork detection Merged with new IsFork bool semantics observation

Findings

Medium

  • [protected-path] AGENTS.md, images/runner/Containerfile — This PR modifies files under protected paths. AGENTS.md documents the new GitLab scaffold execution model (+2/−1 lines). images/runner/Containerfile adds PyYAML as a dependency for GitLab scaffold config parsing (+5/−1 lines). Human approval is always required for protected-path changes, regardless of context.

Low

  • [logic-error] internal/poll/convert.go:293 — The IsFork guard in buildChangeProposalState (event.MRSource == 0 || ...) is defense-in-depth: when either project ID is zero, GetProjectPath(ctx, 0) errors first, so the guard is unreachable in that case. The change from event.MRSource != event.MRTarget to the fail-closed form is a security improvement — previously two zero values would compare equal and incorrectly report IsFork=false.

  • [edge-case] internal/poll/dispatch.go:63 — The MR event type prefix check (strings.HasPrefix(event.Type, "mr_")) relies on the naming convention that all MR-related event types start with mr_. Correct for the current set (mr_event, mr_note) and consistent with resourceKey().

  • [defense-in-depth] internal/poll/dispatch.go:58 — Fork detection in dispatch() uses fail-closed logic: for MR events, isFork defaults to true, only set to the actual comparison when both MRSource and MRTarget are non-zero. Issue events correctly default to isFork=false. IsFork is a bool — there is no way to distinguish "not applicable" (issue events) from "definitely not a fork" in the Dispatch struct. Currently acceptable because fork protection only restricts (code/fix stages), never grants permissions. Well-covered by 5+ test cases.

  • [injection] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml:143, fullsend-agent.yml:34, fullsend-poll.yml — Unquoted heredocs (<<YAML, <<CRED) allow shell expansion across all three scaffold templates. Currently safe: in dispatch, STAGE is constrained by case-statement allowlist (review|retro), RESOURCE_KEY is integer-derived, IS_FORK is true/false, MR_AUTHOR_ID is integer from API, STATUS_IID is integer. In agent/poll, FULLSEND_WIF_PROVIDER, OIDC_TOKEN_FILE, and FULLSEND_SA are sourced from CI/CD protected variables set at install time (not user-controlled at runtime).

  • [authorization] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:127 — The Members API authorization call uses ${CI_MERGE_REQUEST_PROJECT_ID:-${CI_PROJECT_ID}} for the target project. In the cron-poller child pipeline path, CI_MERGE_REQUEST_PROJECT_ID is not set, so the fallback to CI_PROJECT_ID is used. Currently correct — CI_PROJECT_ID in a GitLab child pipeline always refers to the project that owns the pipeline configuration (the enrolled project).

  • [authorization] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml — Authorization chain is defense-in-depth sound and fail-closed at every step: credential mode validation, forge token check, config read from default branch (no fallback), kill switch, role enablement, Members API with default AUTHOR_ACCESS=0, MR_AUTHOR_ID check, fork protection with IS_FORK:-true default.


Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

  • AGENTS.md
  • images/runner/Containerfile
Previous run (9)

Review

This is a re-review. The prior review (SHA 8af11ad) had 1 medium and 12 low findings.

Prior finding Status
[protected-path] medium — AGENTS.md, images/runner/Containerfile Unchanged — human approval required
[authorization] low — Members API CI_MERGE_REQUEST_PROJECT_ID fallback Unchanged — CI_PROJECT_ID correct for child pipelines
[logic-error] low — convert.go IsFork guard unreachable Unchanged — defense-in-depth
[code-organization] low — Fork detection divergence Unchanged — distinct purposes
[edge-case] low — MR event type prefix check Unchanged — convention correct for current types
[injection] low — heredoc <<YAML unquoted Unchanged — safe (STAGE allowlist)
[injection] low — WIF credential heredoc Unchanged — install-time variables
[authorization] low — authorization chain Unchanged — fail-closed
[authorization] low — dispatch.go fork detection Unchanged — fail-closed
[naming-conventions] low — gitlabContent naming ⬇ Downgraded to info — defensible naming pattern
[authorization-coherence] low — MR_AUTHOR_ID limitation Dropped — documented limitation, Phase 2 concern
[docs-currency] low — ADR 0028 stale references ⬇ Downgraded to info — Deprecated ADR, immutability rule applies
[docs-currency] low — gitlab-implementation.md stale refs ⬇ Downgraded to info — historical problem document
[phase-dependency-coherence] low — fullsend poll CLI Dropped — Phase 2 concern, contained by embed-only

Findings

Medium

  • [protected-path] AGENTS.md, images/runner/Containerfile — This PR modifies files under protected paths. AGENTS.md documents the new GitLab scaffold execution model (+2/−1 lines). images/runner/Containerfile adds PyYAML as a dependency for GitLab scaffold config parsing (+5/−1 lines). Human approval is always required for protected-path changes, regardless of context.

Low

  • [logic-error] docs/plans/gitlab-cron-polling-implementation.md — The plan's poll template pseudocode uses ${FULLSEND_CREDENTIAL_MODE} without the :- default expansion operator. Under set -euo pipefail, referencing an unset variable without :- triggers an unbound variable error. The actual scaffold template (fullsend-poll.yml) correctly uses ${FULLSEND_CREDENTIAL_MODE:-}. The plan also omits the elif clause rejecting invalid credential modes and the post-retrieval FULLSEND_FORGE_TOKEN emptiness check. Since this is a plan document (not executable code) and the actual templates are correct, the risk is limited to misleading future implementers.

  • [authorization] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:127 — The Members API authorization call uses ${CI_MERGE_REQUEST_PROJECT_ID:-${CI_PROJECT_ID}} for the target project. In the cron-poller child pipeline path, CI_MERGE_REQUEST_PROJECT_ID is not set, so the fallback to CI_PROJECT_ID is used. Currently correct — CI_PROJECT_ID in a GitLab child pipeline always refers to the project that owns the pipeline configuration (the enrolled project).

  • [logic-error] internal/poll/convert.go:293 — The IsFork guard in buildChangeProposalState (event.MRSource == 0 || ...) is defense-in-depth: when either project ID is zero, GetProjectPath(ctx, 0) errors first, so the guard is unreachable in that case. The change from event.MRSource != event.MRTarget to the fail-closed form is a security improvement — previously two zero values would compare equal and incorrectly report IsFork=false.

  • [edge-case] internal/poll/dispatch.go:63 — The MR event type prefix check (strings.HasPrefix(event.Type, "mr_")) relies on the naming convention that all MR-related event types start with mr_. Correct for the current set (mr_event, mr_note) and consistent with resourceKey().

  • [injection] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml:143 — Heredoc uses <<YAML (unquoted, allows shell expansion). Currently safe: STAGE constrained by case-statement allowlist (review|retro), RESOURCE_KEY derived from CI_MERGE_REQUEST_IID (integer), IS_FORK constrained to true/false, MR_AUTHOR_ID is integer from API, STATUS_IID is CI_MERGE_REQUEST_IID (integer).

  • [injection] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:34 — WIF credential JSON heredoc (<<CRED) is unquoted, allowing shell expansion of FULLSEND_WIF_PROVIDER, OIDC_TOKEN_FILE, and FULLSEND_SA. These variables are sourced from CI/CD protected variables set at install time (not user-controlled at runtime). Same pattern in fullsend-poll.yml.

  • [authorization] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml — Authorization chain is defense-in-depth sound and fail-closed at every step: credential mode validation, forge token check, config read from default branch (no fallback), kill switch, role enablement, Members API with default AUTHOR_ACCESS=0, MR_AUTHOR_ID check, fork protection with IS_FORK:-true default.

  • [authorization] internal/poll/dispatch.go:63 — Fork detection in dispatch() uses fail-closed logic: for MR events, isFork defaults to true, only set to the actual comparison when both MRSource and MRTarget are non-zero. Issue events correctly default to isFork=false. Well-covered by 5 test cases.


Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

  • AGENTS.md
  • images/runner/Containerfile
Previous run (10)

Review

This is a re-review. The prior review (SHA 474a19e) had 1 medium and 8 low findings.

Prior finding Status
[protected-path] medium — AGENTS.md, images/runner/Containerfile Unchanged — human approval required
[authorization] low — Members API CI_MERGE_REQUEST_PROJECT_ID fallback Unchanged — CI_PROJECT_ID correct for child pipelines
[logic-error] low — convert.go IsFork guard unreachable Unchanged — defense-in-depth
[code-organization] low — Fork detection divergence Unchanged — distinct purposes
[edge-case] low — MR event type prefix check Unchanged — convention correct for current types
[injection] low — heredoc <<YAML unquoted Unchanged — safe (STAGE allowlist)
[injection] low — WIF credential heredoc Unchanged — install-time variables
[authorization] low — authorization chain Unchanged — fail-closed
[authorization] low — dispatch.go fork detection Unchanged — fail-closed

Findings

Medium

  • [protected-path] AGENTS.md, images/runner/Containerfile — This PR modifies files under protected paths. AGENTS.md documents the new GitLab scaffold execution model (+2/−1 lines). images/runner/Containerfile adds PyYAML as a dependency for GitLab scaffold config parsing (+5/−1 lines). Human approval is always required for protected-path changes, regardless of context.

Low

  • [authorization] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:127 — The Members API authorization call uses ${CI_MERGE_REQUEST_PROJECT_ID:-${CI_PROJECT_ID}} for the target project. In the cron-poller child pipeline path, CI_MERGE_REQUEST_PROJECT_ID is not set, so the fallback to CI_PROJECT_ID is used. Currently correct — CI_PROJECT_ID in a GitLab child pipeline always refers to the project that owns the pipeline configuration (the enrolled project).

  • [logic-error] internal/poll/convert.go:293 — The IsFork guard in buildChangeProposalState (event.MRSource == 0 || ...) is defense-in-depth: when either project ID is zero, GetProjectPath(ctx, 0) errors first, so the guard is unreachable in that case. The dispatch path (dispatch.go) correctly handles zero IDs fail-closed with dedicated test coverage.

  • [code-organization] internal/poll/dispatch.go, internal/poll/convert.go — Fork detection logic exists in both dispatch() and buildChangeProposalState() with different patterns. The implementations serve different purposes (dispatch writes child pipeline variables; convert populates NormalizedEvent state) with different preconditions, so the divergence is defensible.

  • [edge-case] internal/poll/dispatch.go:63 — The MR event type prefix check (strings.HasPrefix(event.Type, "mr_")) relies on the naming convention that all MR-related event types start with mr_. Correct for the current set (mr_event, mr_note) and consistent with resourceKey().

  • [injection] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml:143 — Heredoc uses <<YAML (unquoted, allows shell expansion). Currently safe: STAGE constrained by case-statement allowlist (review|retro), RESOURCE_KEY derived from CI_MERGE_REQUEST_IID (integer), IS_FORK constrained to true/false, MR_AUTHOR_ID is integer from API, STATUS_IID is CI_MERGE_REQUEST_IID (integer).

  • [injection] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:34 — WIF credential JSON heredoc (<<CRED) is unquoted, allowing shell expansion of FULLSEND_WIF_PROVIDER, OIDC_TOKEN_FILE, and FULLSEND_SA. These variables are sourced from CI/CD protected variables set at install time (not user-controlled at runtime). Same pattern in fullsend-poll.yml.

  • [authorization] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml — Authorization chain is defense-in-depth sound and fail-closed at every step: credential mode validation, forge token check, config read from default branch (no fallback), kill switch, role enablement, Members API with default AUTHOR_ACCESS=0, MR_AUTHOR_ID check, fork protection with IS_FORK:-true default.

  • [authorization] internal/poll/dispatch.go:63 — Fork detection in dispatch() uses fail-closed logic: for MR events, isFork defaults to true, only set to the actual comparison when both MRSource and MRTarget are non-zero. Issue events correctly default to isFork=false. Well-covered by 5 test cases.

  • [naming-conventions] internal/scaffold/scaffold.go:15 — New gitlabContent embed.FS variable uses lowerCamelCase while the existing GitHub embed variable uses a single lowercase word (content). Minor naming asymmetry — defensible given that content was named when only one scaffold existed.

  • [authorization-coherence] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml — Authorization gate uses MR_AUTHOR_ID, which is only populated for MR events. Issue events will hit the fail-closed skip. This is an intentional, documented limitation (see LIMITATION comment in agent template) — generalizing to ACTOR_ID/ACTOR_ROLE is deferred until non-MR event routing is enabled.

  • [docs-currency] docs/ADRs/0028-gitlab-support.md:90 — References old per-stage GitLab CI template files (triage.yml, code.yml, etc.) that have been replaced by fullsend-agent.yml. ADR-0028 is marked Deprecated and the AGENTS.md immutability rule applies — minor annotations are acceptable but not required.

  • [docs-currency] docs/problems/gitlab-implementation.md:245 — References old per-stage template architecture. This is a historical problem document (speculative/planning) with a note that the webhook approach is superseded by ADR 0067. Low impact.

  • [phase-dependency-coherence] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-poll.yml — Poll template calls fullsend poll CLI subcommand which is not yet implemented (Phase 2 work). The scaffold is embed-only and not deployed until fullsend admin install --forge gitlab is updated (Phase 4), so the broken state is contained.


Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

  • AGENTS.md
  • images/runner/Containerfile
Previous run (11)

Review

This is a re-review. The prior review (SHA e79d234) had 1 medium and 10 low findings.

Prior finding Status
[protected-path] medium — AGENTS.md, images/runner/Containerfile Unchanged — human approval required
[authorization] low — Members API CI_MERGE_REQUEST_PROJECT_ID fallback Unchanged — CI_PROJECT_ID correct for child pipelines
[architectural-coherence] low — Kill switch/role duplication ⬇ Downgraded to info — tracked via #5416
[logic-error] low — convert.go IsFork guard unreachable Unchanged — defense-in-depth
[code-organization] low — Fork detection divergence Unchanged — distinct purposes
[logic-error] low — actor.is_entity_author hardcoded Dropped — low impact, unchanged, not re-raised
[edge-case] low — MR_AUTHOR_ID empty, cron-polled events Dropped — Phase 2 concern, well-documented
[injection] low — heredoc <<YAML unquoted Unchanged — safe (STAGE allowlist)
[injection] low — WIF credential heredoc Unchanged — install-time variables
[authorization] low — authorization chain Unchanged — fail-closed
[authorization] low — dispatch.go fork detection Unchanged — fail-closed

Findings

Medium

  • [protected-path] AGENTS.md, images/runner/Containerfile — This PR modifies files under protected paths. AGENTS.md documents the new GitLab scaffold execution model (+2/−1 lines). images/runner/Containerfile adds PyYAML as a dependency for GitLab scaffold config parsing (+5/−1 lines). Human approval is always required for protected-path changes, regardless of context.

Low

  • [authorization] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:130 — The Members API authorization call uses ${CI_MERGE_REQUEST_PROJECT_ID:-${CI_PROJECT_ID}} for the target project. In the cron-poller child pipeline path, CI_MERGE_REQUEST_PROJECT_ID is not set, so the fallback to CI_PROJECT_ID is used. Currently correct — CI_PROJECT_ID in a GitLab child pipeline always refers to the project that owns the pipeline configuration (the enrolled project). Cross-project polling is explicitly out of scope per ADR 0067.

  • [logic-error] internal/poll/convert.go:293 — The IsFork guard in buildChangeProposalState (event.MRSource == 0 || ...) is defense-in-depth: when either project ID is zero, GetProjectPath(ctx, 0) errors first, so the guard is unreachable in that case. The dispatch path (dispatch.go) correctly handles zero IDs fail-closed with dedicated test coverage.

  • [code-organization] internal/poll/dispatch.go, internal/poll/convert.go — Fork detection logic exists in both dispatch() and buildChangeProposalState() with different patterns. The implementations serve different purposes (dispatch writes child pipeline variables; convert populates NormalizedEvent state) with different preconditions, so the divergence is defensible.

  • [edge-case] internal/poll/dispatch.go:64 — The MR event type prefix check (strings.HasPrefix(event.Type, "mr_")) relies on the naming convention that all MR-related event types start with mr_. Correct for the current set (mr_event, mr_note) and consistent with resourceKey(). If a future event type breaks this convention, fork detection would silently produce wrong results.

  • [injection] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-dispatch.yml:184 — Heredoc uses <<YAML (unquoted, allows shell expansion). Currently safe: STAGE constrained by case-statement allowlist (review|retro), RESOURCE_KEY derived from CI_MERGE_REQUEST_IID (integer), IS_FORK constrained to true/false, MR_AUTHOR_ID is integer from API, STATUS_IID is CI_MERGE_REQUEST_IID (integer).

  • [injection] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml:37 — WIF credential JSON heredoc (<<CRED) is unquoted, allowing shell expansion of FULLSEND_WIF_PROVIDER, OIDC_TOKEN_FILE, and FULLSEND_SA. These variables are sourced from CI/CD protected variables set at install time (not user-controlled at runtime). Same pattern in fullsend-poll.yml.

  • [authorization] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/fullsend-agent.yml — Authorization chain is defense-in-depth sound and fail-closed at every step: credential mode validation, forge token check, config read from default branch (no fallback), kill switch, role enablement, Members API with default AUTHOR_ACCESS=0, MR_AUTHOR_ID check, fork protection with IS_FORK:-true default.

  • [authorization] internal/poll/dispatch.go:63 — Fork detection in dispatch() uses fail-closed logic: for MR events, isFork defaults to true, only set to the actual comparison when both MRSource and MRTarget are non-zero. Issue events correctly default to isFork=false. Well-covered by 5 test cases.


Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

  • AGENTS.md
  • images/runner/Containerfile

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added component/install CLI install and app setup component/dispatch Workflow dispatch and triggers labels Jul 7, 2026
@ggallen
ggallen force-pushed the gitlab/cicd-templates branch from 54cfb94 to e696ab1 Compare July 7, 2026 11:28
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 7, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 11:29 AM UTC · Ended 11:36 AM UTC
Commit: e8381e3 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 7, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 11:37 AM UTC · Completed 11:51 AM UTC
Commit: 9e26a35 · View workflow run →

@ifireball ifireball left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think doing this is a bit premature - lets get CEL triggers in place first, then we can base the GL workflows on those rahter then implement the same kind of fixed-role workflows we have in GH

Also it would be really nice to have #1982 in place and GL drivers for those tests so that Green CI == it really works.

@fullsend-ai-review
fullsend-ai-review Bot dismissed their stale review July 7, 2026 11:51

Superseded by updated review

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Jul 7, 2026
@ggallen
ggallen force-pushed the gitlab/cicd-templates branch from 9e26a35 to 5d8ae65 Compare July 7, 2026 11:54
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 7, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 11:55 AM UTC · Completed 12:09 PM UTC
Commit: 5d8ae65 · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review 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 7, 2026
@ggallen
ggallen force-pushed the gitlab/cicd-templates branch from 5d8ae65 to a4f953b Compare July 7, 2026 12:14
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 7, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 12:15 PM UTC · Completed 12:26 PM UTC
Commit: a4f953b · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 22, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:01 PM UTC · Completed 10:21 PM UTC
Commit: cd86eae · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 23, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 4:00 AM UTC · Completed 4:19 AM UTC
Commit: b650621 · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 23, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 4:27 AM UTC · Completed 4:45 AM UTC
Commit: 65a3fcf · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

@ifireball ifireball left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping R-C flag, I can't review all this RN and I don't see any BT prooving that this really works, so I just have to trust that it does.

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 23, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 6:05 AM UTC · Completed 6:21 AM UTC
Commit: ac1f222 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 23, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 6:40 AM UTC · Completed 6:55 AM UTC
Commit: abc700b · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 23, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:02 AM UTC · Completed 7:20 AM UTC
Commit: fb87d00 · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 23, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 11:40 AM UTC · Completed 11:58 AM UTC
Commit: d93783c · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 23, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 12:04 PM UTC · Completed 12:21 PM UTC
Commit: 99bced6 · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

Signed-off-by: Greg Allen <gallen@redhat.com>
Signed-off-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Greg Allen <gallen@redhat.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 23, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 12:27 PM UTC · Completed 12:47 PM UTC
Commit: 698aacf · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the review comment for full details.

Comment thread internal/poll/convert.go
Comment thread internal/poll/dispatch.go
@fullsend-ai-retro

fullsend-ai-retro Bot commented Jul 23, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 1:10 PM UTC · Completed 1:32 PM UTC
Commit: 698aacf · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #3193 — GitLab CI/CD pipeline templates

Timeline: Opened Jul 6 by @ggallen (human-authored). @ifireball requested changes Jul 7 ("premature — get CEL triggers in place first"). PR paused 12 days. Revived Jul 19 with extensive rework. @waynesun09 conducted 6 rounds of multi-agent review squad analysis (Claude + Grok + Gemini) from Jul 20–22, surfacing 4 critical + 8 high + ~28 medium findings. @ifireball approved Jul 23 noting "I can't review all this RN and I don't see any BT proving that this really works." Merged Jul 23 after 17 days.

Review agent performance: 52 review runs (35 success, 7 failure, 10 cancelled) across 52 unique commit SHAs from rapid amend+force-push iteration. The bot's final review found only 1 medium (protected-path) + 7 low findings — all low-severity noise. Meanwhile, @waynesun09's human-driven multi-model squad found critical platform-breaking issues the bot completely missed:

  • Runner image ENTRYPOINT incompatible with GitLab Docker executor (every job would fail at container start)
  • yq not installed in runner image (every agent job dies at kill-switch check)
  • Members API not in CI job token allowlist (authorization always fails)
  • .source_project.path_with_namespace doesn't exist in MR REST response (every MR gets IS_FORK=true)
  • Fork protection defaults fail-open (${IS_FORK:-false})
  • Protected variable model conflicts with MR pipeline execution on unprotected refs
  • Issue events incorrectly marked as fork MRs, making ready-to-code path unreachable

Root cause of review gap: The review agent's sub-agents (correctness, security, style, intent-coherence) are entirely platform-agnostic. None have GitLab CI-specific knowledge, and none cross-reference script tool dependencies against the container image Containerfile. The security sub-agent has hardcoded GHA-specific guidance (command injection, permissions scoping) but nothing equivalent for GitLab. The agent architecture has no extensibility point for domain-specific knowledge injection beyond what AGENTS.md/CLAUDE.md happen to contain.

Review run failures (7/52): Three distinct failure modes: (1) Permission denied during sandbox cleanup — 4 runs failed with Error: clearing local repo /tmp/agent-review-XXXX/website/.gitignore: permission denied; (2) GitHub API 422 submitting review against stale SHA — 2 runs; (3) Token mint 502 response parsed as token — 1 run.

Evidence for existing issues:

Autonomy assessment: The review agent is not ready for increased autonomy on this class of change (new platform integration scaffolding). Its review was ineffective — zero of the 12 critical+high findings were detected. Human review was essential. The agent's approval should carry no weight for changes introducing CI/CD templates for new forge platforms until platform-specific review capabilities are added.

Proposals filed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/ci CI pipelines and checks component/dispatch Workflow dispatch and triggers component/docs User-facing documentation component/install CLI install and app setup component/runner Agent runner behavior and lifecycle go Pull requests that update go code requires-manual-review Review requires human judgment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants