Skip to content

Add ADR-0028 for GitLab support architecture#601

Merged
ggallen merged 27 commits into
fullsend-ai:mainfrom
ggallen:docs/adr-026-gitlab-support-clean
May 6, 2026
Merged

Add ADR-0028 for GitLab support architecture#601
ggallen merged 27 commits into
fullsend-ai:mainfrom
ggallen:docs/adr-026-gitlab-support-clean

Conversation

@ggallen

@ggallen ggallen commented May 1, 2026

Copy link
Copy Markdown
Member

Summary

Proposes comprehensive architecture for extending fullsend to support GitLab alongside GitHub.

Key Design Decisions

  • Directory structure: .gitlab/ci/*.yml parallel to .github/workflows/
  • Authentication: Project Access Tokens for per-role authentication (instead of GitHub Apps)
  • Security: Protected branch + workflow rules for shim security (no pull_request_target equivalent)
  • Dispatch: Pipeline trigger API for cross-repo dispatch (instead of workflow_dispatch)
  • Discovery: Same # fullsend-stage: marker pattern for stage discovery

Contents

The ADR covers:

  • CI/CD pipeline architecture mapping GitHub Actions concepts to GitLab CI
  • Authentication model using Project Access Tokens
  • Security considerations and mitigations for shim pipelines
  • Implementation phases and required CLI changes
  • Event mapping between GitHub and GitLab webhooks
  • Alternatives considered and trade-offs

Status

Proposed - seeking feedback on the design before implementation.

Related

  • ADR-0005: Forge abstraction layer (foundational design this builds on)
  • ADR-0007: Per-role GitHub Apps (authentication pattern to replicate)
  • ADR-0008: workflow_dispatch (cross-repo pattern to replicate)
  • ADR-0009: pull_request_target (security model to adapt)

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented May 1, 2026

Copy link
Copy Markdown

Site preview

Preview: https://133649fa-site.fullsend-ai.workers.dev

Commit: fd5c65cda4a92d403552faabe51548ae53272806

@fullsend-ai-review

fullsend-ai-review Bot commented May 1, 2026

Copy link
Copy Markdown

Review: #601

Head SHA: 9c54514
Timestamp: 2026-05-06T00:00:00Z
Outcome: approve

Summary

This is a well-crafted documentation PR that adds ADR-0028 (GitLab Support Architecture) and a companion problem document for implementation details. The ADR correctly identifies the key architectural gaps between GitHub and GitLab (no pull_request_target, webhook/trigger API incompatibility, Project Access Tokens vs GitHub Apps) and proposes sound mitigations. All cross-references to existing ADRs (0005, 0007, 0008, 0009) are valid, and claims about the existing forge.Client interface methods (ListOrgInstallations, GetAppClientID, DispatchWorkflow) are accurate against the current codebase. The security model is thorough — protected CI/CD variables as defense-in-depth, hardcoded ref=main, constant-time token comparison, and webhook authenticity validation are all appropriate controls. Open questions (webhook translation architecture, forge interface design details, agent execution environment) are clearly scoped and deferred appropriately.

Findings

Medium

  • [style/conventions] README.md:36 — The new "GitLab Implementation" entry is inserted between "Testing the Agents" and "Operational Observability", which breaks the loosely alphabetical ordering of the problem docs list. "GitLab Implementation" (G) should appear before entries starting with I, O, P, S, and T.
    Remediation: Move the entry higher in the list, after "Forge Abstraction" or before "Intent Representation".

Info

  • [correctness] docs/problems/gitlab-implementation.md — The dispatch pipeline YAML snippets contain duplicated validation logic (SOURCE_PROJECT regex, enrollment check) across the shim security and cross-repo dispatch sections. The document acknowledges this with a note about extracting to a shared script during implementation, which is appropriate for a design doc. No action needed now, but implementers should take note.

  • [correctness] docs/ADRs/0028-gitlab-support.md:160 — The ADR notes GitHub App private keys "never expire" but doesn't mention that installation tokens have 1-hour TTL with auto-refresh. This is accurate but could be clearer to prevent misunderstanding by readers unfamiliar with the GitHub Apps model. The comparison point is token rotation burden: GitLab PATs require manual renewal annually vs GitHub's automatic refresh.

Footer

Outcome: approve
This review applies to SHA 9c54514ffeec8dc699135040c54c12abb08af677. Any push to the PR head clears this review and requires a new evaluation.

Previous run

Review: #601

Head SHA: 3a0b2fe
Timestamp: 2026-05-06T00:00:00Z
Outcome: approve

Summary

This PR adds ADR-0028 (GitLab Support Architecture) and a companion problem document (docs/problems/gitlab-implementation.md), proposing a comprehensive design for extending fullsend to support GitLab as a second forge. The change is well-scoped to documentation only (no executable code changes), follows repo conventions for ADR and problem doc structure, and demonstrates thorough security analysis. The design correctly identifies the key architectural gap (webhook-to-trigger API incompatibility) and documents it as an open question rather than hand-waving past it. No critical or high findings.

Findings

Critical

None.

High

None.

Medium

None.

Low

  • [style] docs/problems/gitlab-implementation.md — The validation logic (SOURCE_PROJECT format check, enrollment lookup, webhook token comparison) is intentionally duplicated between the shim security snippet and the dispatch workflow snippet. The document acknowledges this and suggests extracting to a shared script during implementation, which is the right call. Just flagging to ensure the TODO is tracked when implementation begins.

Info

  • [correctness] docs/ADRs/0028-gitlab-support.md — The ADR correctly identifies that ListOrgInstallations and GetAppClientID are GitHub-specific methods on the current forge.Client interface. The proposed migration strategy (deprecation → extension interfaces → removal) is sound and consistent with ADR-0005's abstraction boundary.

  • [correctness] docs/problems/gitlab-implementation.md — The timing side-channel mitigation for webhook token comparison (hash-then-compare) is honestly documented as not truly constant-time but limiting information leakage to hash bytes rather than token bytes. For a design doc this is appropriate; implementation should use crypto/subtle.ConstantTimeCompare in Go.

  • [intent alignment] The PR has no linked issue, which is fine for design exploration documents per CLAUDE.md conventions. The scope is appropriate — two new documents plus a README link.

  • [injection defense] PR body, commit messages, and document content inspected for prompt injection patterns and non-rendering Unicode. None found. The "🤖 Generated with Claude Code" footer is standard.

Footer

Outcome: approve
This review applies to SHA 3a0b2fe2b59db518b732c10d0e3505b1607acacc. Any push to the PR head clears this review and requires a new evaluation.

Previous run (2)

Review: #601

Head SHA: 4c61f37
Timestamp: 2026-05-06T00:00:00Z
Outcome: comment-only

Summary

This is a well-structured documentation PR that adds ADR-0028 for GitLab support architecture alongside a companion problem document. The design is thorough, security-conscious (protected variables, hardcoded ref=main, webhook token validation), and properly references existing ADRs. The claims about the current forge.Client interface (GitHub-specific methods like ListOrgInstallations, GetAppClientID, DispatchWorkflow) are verified accurate against the codebase. No critical or high findings — the items below are style and accuracy observations worth considering.

Findings

Medium

  • [Style/conventions] docs/problems/gitlab-implementation.md — The file is titled "GitLab Support Implementation Details" and reads largely as prescriptive implementation specification (concrete YAML configs, Go interface signatures, CLI command changes). CLAUDE.md guidance says problem documents in docs/problems/ should "present multiple options with trade-offs, not prescribe single solutions." While the doc does explore options for the webhook translation architecture, most sections present a single decided approach. Consider either (a) reframing the document with more explicit trade-off exploration per section, or (b) moving it to a different location (e.g., an appendix to the ADR, or a docs/guides/ entry) that better matches its prescriptive nature.

  • [Correctness] docs/ADRs/0028-gitlab-support.md:198-222 — The "Forge Abstraction Compliance" section claims the GitLab implementation complies with ADR-0005's promise that "Adding a new forge requires implementing forge.Client — no changes to layers, CLI, or app setup code." However, the companion implementation doc explicitly lists modifications to internal/appsetup/, internal/layers/workflows.go, internal/layers/enrollment.go, internal/config/config.go, and internal/forge/forge.go (the interface itself). The ADR softens this as "limited to calling new forge-neutral interface methods," but adding new methods to the interface is changing the interface, and callers in layers/CLI/appsetup are being modified. Consider acknowledging this as a controlled deviation from ADR-0005's original promise rather than claiming compliance, and updating ADR-0005's consequence statement if the interface evolution is accepted.

Low

  • [Style/conventions] docs/ADRs/0028-gitlab-support.md:1 — The frontmatter title uses "28. GitLab Support Architecture" without leading zeros. The ADR template (0000-adr-template.md) and all existing ADRs use four-digit format (e.g., "0027. Allowed and Disallowed Tools for Agents"). Should be "0028. GitLab Support Architecture" for consistency.

  • [Correctness] docs/problems/gitlab-implementation.md:362-441 — The dispatch pipeline YAML contains nested heredocs (bash <<'BASH' containing cat > file <<'EOF') with significant indentation inside a YAML block scalar. While this is illustrative code in a design document, the indentation shown would likely cause parse errors in actual GitLab CI. A brief note that the indentation is simplified for readability would prevent confusion during implementation.

  • [Correctness] docs/problems/gitlab-implementation.md:463-464 — The "Cross-Repo Dispatch Mechanism" section notes that validation logic is duplicated between the shim security snippet and the dispatch snippet, with a comment to extract into a shared script during implementation. This is fine for a design doc but worth tracking — the duplication between the two YAML snippets could cause confusion about which is authoritative.

Info

  • [Style/conventions] README.md — The new GitLab Implementation link is inserted between "Testing the Agents" and "Operational Observability". The existing problem doc links don't follow strict alphabetical order but do follow a rough logical grouping. The GitLab Implementation doc feels more infrastructure-adjacent and might sit better near "Agent Infrastructure" or at the end of the list.

  • [Correctness] Lint could not be verified — make lint failed due to network access restrictions in the sandbox (cannot fetch pre-commit hooks). This is an environment limitation, not a PR issue.

Footer

Outcome: comment-only
This review applies to SHA 4c61f37d2f45403521ade9ff5dfa6fc7f0f6b7b2. Any push to the PR head clears this review and requires a new evaluation.

Previous run (3)

Review: #601

Head SHA: da14129
Timestamp: 2026-05-05T00:00:00Z
Outcome: approve

Summary

This PR adds ADR-0028 and a companion problem document for GitLab support architecture. The change is documentation-only — no executable code is modified. The ADR number (0028) is the next available, all cross-referenced ADRs (0005, 0007, 0008, 0009) exist, and the README link is correctly placed. The design documents are thorough, with well-reasoned security analysis covering protected branches, protected CI/CD variables, webhook validation with timing-safe comparison, YAML injection prevention via base64 encoding, and an explicit rejection of multi-project pipelines. Open questions are honestly documented with clear trade-off analysis. The PR follows CLAUDE.md conventions (problem doc in docs/problems/, linked from README, ADR in docs/ADRs/).

Findings

Critical

(none)

High

(none)

Medium

(none)

Low

  • [correctness] docs/problems/gitlab-implementation.md — The webhook token validation logic (SOURCE_PROJECT format check, enrollment check, token comparison) is duplicated between the "Shim Pipeline Security" and "Cross-Repo Dispatch Mechanism" sections. The document acknowledges this and recommends extraction during implementation, but this is worth tracking to prevent maintenance drift in the actual implementation.

  • [correctness] docs/problems/gitlab-implementation.md — The detectForge function is shown under a // NOTE: comment explaining it should live in internal/forge/detect.go per ADR-0005's boundary rule, but the code block header says internal/cli/admin.go. This is clearly illustrative, but implementers should note the intended location is internal/forge/detect.go.

Info

  • [style] docs/problems/gitlab-implementation.md — The timing-safe token comparison approach (hash-then-compare) is well-documented with an honest caveat that bash [ ] comparison of hash bytes is not truly constant-time. For a design doc this is appropriate; implementation should consider using openssl dgst with a proper HMAC comparison or a language with constant-time comparison primitives.

  • [style] docs/ADRs/0028-gitlab-support.md — The "Alternatives" section precedes the "Decision" section, which is a good pattern for ADRs — it shows what was considered before presenting the chosen approach.

Footer

Outcome: approve
This review applies to SHA da141294a5bc192c06141e675e53f34ecc923ddb. Any push to the PR head clears this review and requires a new evaluation.

Previous run (4)

Review: #601

Head SHA: 1ab346d
Timestamp: 2026-05-05T00:00:00Z
Outcome: comment-only

Summary

This is a well-structured documentation PR adding ADR-0028 (GitLab support architecture) and a companion problem document (docs/problems/gitlab-implementation.md). The documents demonstrate thorough security thinking — the webhook-based architecture, protected variable defense-in-depth, and YAML injection prevention are all well-reasoned. The forge interface evolution proposal aligns with ADR-0005's abstraction layer design. Two minor issues are worth noting: the PR title references "ADR-0027" but the file is correctly numbered 0028 (since ADR-0027 already exists), and the HasSuffix pattern in the illustrative forge detection code could match attacker-controlled domains.

Findings

Medium

  • [correctness] PR title — PR title says "Add ADR-0027" but the ADR file is 0028-gitlab-support.md with content title "28. GitLab Support Architecture". ADR-0027 (0027-allowed-and-disallowed-tools-for-agents.md) already exists on main. The file numbering is correct; the PR title is misleading.
    Remediation: Update PR title to "Add ADR-0028 for GitLab support architecture".

Low

  • [correctness] docs/problems/gitlab-implementation.md — The detectForge illustrative code uses strings.HasSuffix(host, ".github.com") which the inline comment already flags as insecure ("attacker-controlled domains like evil.github.com"). The comment recommends an allowlist but the code still shows the HasSuffix pattern. Since this is explicitly marked as illustrative and the comment warns against it, this is low severity, but the code example could be made safer to avoid copy-paste risk.
    Remediation: Consider showing the allowlist approach in the code example instead of the HasSuffix pattern, since the comment already recommends it.

  • [style/conventions] docs/problems/gitlab-implementation.md — The dispatch pipeline YAML snippets contain duplicated validation logic (SOURCE_PROJECT format check, enrollment check) across the shim security section and cross-repo dispatch section. A comment notes this ("should be extracted into a shared script") but the duplication could cause confusion about which snippet is canonical.
    Remediation: No action needed — the inline comment already calls this out as a consolidation TODO for implementation.

Info

  • [intent alignment] The PR body and ADR status are both "Proposed", consistent with seeking design feedback before implementation. No scope concerns — the change is documentation-only with no code modifications.

  • [injection defense] No prompt injection patterns, non-rendering Unicode, or bidirectional overrides detected in PR body, commit messages, or document content.

Footer

Outcome: comment-only
This review applies to SHA 1ab346db48345ba9970d161abdd3f20f1eb7a412. Any push to the PR head clears this review and requires a new evaluation.

Previous run (5)

Review: #601

Head SHA: 96a05fe
Timestamp: 2026-05-05T00:00:00Z
Outcome: request-changes

Summary

This PR adds a well-structured ADR and companion problem document for GitLab support architecture. The documents are thorough, with strong security analysis (webhook-based dispatch, protected variables, constant-time token comparison) and honest treatment of open questions. However, the PR has a critical ADR numbering collision: it creates 0027-gitlab-support.md but 0027-allowed-and-disallowed-tools-for-agents.md already exists on the target branch. This must be resolved before merge. Additionally, the ADR title claims "27." which conflicts with the existing ADR 0027. The document content itself is high quality.

Findings

Critical

  • [Correctness] docs/ADRs/0027-gitlab-support.md:1ADR number collision. ADR 0027 already exists as 0027-allowed-and-disallowed-tools-for-agents.md on the target branch. This PR creates a second 0027-gitlab-support.md, producing two ADRs with the same number. The ADR title inside the file also says "27. GitLab Support Architecture" which conflicts.
    Remediation: Renumber to 0028 (filename, title in frontmatter, title in heading, and all internal cross-references in both files). The companion problem doc (gitlab-implementation.md) references "ADR-0027" in multiple places — update those too.

Medium

  • [Correctness] docs/problems/gitlab-implementation.md:329 — The SOURCE_PROJECT regex ^[a-zA-Z0-9_-]+/[a-zA-Z0-9_-]+$ does not match GitLab project paths containing dots or nested groups (e.g., my.org/my.project or group/subgroup/project). The document notes this at lines 325-328 and 455-457 ("this regex should be expanded during implementation"), but the dispatch validation snippet (line 329) and the generate-config snippet (line 455) use the restrictive regex without a clear marker that this is intentionally incomplete. Since this is illustrative code, this is acceptable, but an inline # ILLUSTRATIVE marker or TODO would prevent copy-paste bugs during implementation.
    Remediation: Add a # TODO(implementation): comment directly on the regex line in both snippets to prevent verbatim copy-paste.

  • [Correctness] docs/problems/gitlab-implementation.md:492-501 — The envsubst pattern writes to .gitlab-ci-child.yml then reads from the same file in the next command. While this works because envsubst reads fully before mv replaces, the heredoc at line 492 uses single-quoted 'EOF' which prevents $pipeline_file expansion — then envsubst is used to substitute it. This is correct but fragile: if someone removes the envsubst step thinking the heredoc handles it, $pipeline_file will appear literally in the child config. A comment explaining the two-step intentional design would help.
    Remediation: Add a brief comment above the heredoc explaining that single-quoting is intentional and envsubst handles the substitution.

  • [Style/conventions] docs/problems/gitlab-implementation.md — The document is 610 lines, which is substantial for a problem document. The repo convention (CLAUDE.md) says problem docs should "present multiple options with trade-offs, not prescribe single solutions." This document leans more toward a detailed implementation specification than a problem exploration. Much of the content (YAML snippets, exact variable names, API paths) reads as implementation spec rather than problem analysis.
    Remediation: Consider whether some implementation-specific detail (exact YAML configs, variable naming schemes) belongs in implementation PRs rather than the problem doc. This is a style observation, not blocking.

Low

  • [Correctness] docs/problems/gitlab-implementation.md:355-358 — The "constant-time comparison" via SHA256 hashing is a good defense against timing attacks on the raw token, but the final [ "$WEBHOOK_HASH" != "$EXPECTED_HASH" ] comparison in bash test is not guaranteed constant-time by POSIX. In practice, the threat is minimal since the attacker only learns hash bytes (not token bytes), so this is defense-in-depth rather than a true constant-time primitive. The comment on line 353 ("Use constant-time comparison") slightly overstates what the code achieves.
    Remediation: Adjust the comment to "Use hash comparison to mitigate timing side-channel attacks" for precision.

  • [Intent alignment] README.md — The new entry is inserted in the middle of the existing problem document list, between "Agent Infrastructure" and "Autonomy Spectrum". The existing list appears to be in a curated order (not strictly alphabetical — "Intent Representation" comes first). Inserting "GitLab Implementation" here breaks the existing ordering convention.
    Remediation: Append the new entry at the end of the problem doc list, or confirm the intended ordering.

Info

  • [Correctness] The ADR's claims about the existing forge.Client interface are verified accurate: ListOrgInstallations, GetAppClientID, and DispatchWorkflow exist as described, and the interface uses forge-neutral vocabulary (ChangeProposal) as claimed.

  • [Correctness] The companion problem doc correctly identifies that CreateWebhook would be a no-op for GitHub (line 641), which is consistent with the webhook-based architecture being GitLab-specific.

  • [Content security] No secrets, API keys, or sensitive infrastructure identifiers were found in the added files.

  • [Injection defense] PR body and commit messages contain no prompt injection patterns or non-rendering Unicode.

Footer

Outcome: request-changes
This review applies to SHA 96a05fe90b41294d772ca55354ccc6e4b0bb601e. Any push to the PR head clears this review and requires a new evaluation.

Previous run (6)

Review: #601

Head SHA: aaf10c9
Timestamp: 2026-05-05T00:00:00Z
Outcome: comment-only

Summary

This is a well-structured documentation PR adding ADR-0027 (GitLab Support Architecture) and a companion problem document (docs/problems/gitlab-implementation.md). The design is thorough, with extensive security analysis including defense-in-depth via protected CI/CD variables, webhook authentication, and YAML injection prevention. The documents correctly identify the key architectural tension (webhook-to-trigger API incompatibility) and present it as an open question with trade-offs rather than prescribing a solution. Two findings are noted below — both non-blocking but worth addressing to avoid misleading future implementors.

Findings

Medium

  • [Correctness] docs/problems/gitlab-implementation.md:354-358 — The illustrative bash code claims to use "constant-time comparison" for webhook token validation by comparing SHA256 hashes, but [ "$WEBHOOK_HASH" != "$EXPECTED_HASH" ] is a standard shell string comparison that short-circuits on the first differing byte. While hashing prevents direct token byte leakage via timing, the comparison itself is not constant-time, making the security claim misleading. Future implementors may copy this pattern believing it provides timing-attack resistance when it does not.
    Remediation: Either note that true constant-time comparison requires a purpose-built tool (e.g., openssl cms or a small helper binary), or soften the claim to "timing-resistant" rather than "constant-time" and document the residual risk.

Low

  • [Style/conventions] docs/problems/gitlab-implementation.md:397-399 — The dispatch workflow section notes that validation logic is duplicated from the shim security snippet and recommends extracting to a shared script during implementation. This is good documentation hygiene, but the two code blocks have subtle differences (the shim section includes webhook token validation; the dispatch section includes stage matching). Consider adding a brief note clarifying which validation steps belong to each phase to avoid confusion during implementation.

Info

  • [Correctness] docs/ADRs/0027-gitlab-support.md:156-158 — The ADR states GitHub App private keys "never expire" while noting GitLab PAT max 1-year expiry. While GitHub App private keys don't have built-in expiry, GitHub App installation tokens expire after 1 hour and require refresh. The parenthetical clarification partially addresses this but could be clearer to avoid creating a false equivalence in the limitations comparison.

Footer

Outcome: comment-only
This review applies to SHA aaf10c90fd83e5adee4745e6b8b1237ae644cfd8. Any push to the PR head clears this review and requires a new evaluation.

Previous run (7)

Review: #601

Head SHA: 491cae4
Timestamp: 2026-05-05T00:00:00Z
Outcome: approve

Summary

This documentation-only PR adds ADR-0027 (GitLab Support Architecture) and a companion implementation problem document, with a README index entry. The ADR is well-structured, correctly numbered (follows ADR-0026), and follows established conventions. The architectural proposal is thorough: it accurately characterizes the existing forge.Client interface (verified against internal/forge/forge.go), honestly identifies where ADR-0005's promise of zero-change extension doesn't fully hold, and proposes a sound migration strategy via forge-neutral interface methods and extension interfaces. Security analysis is strong — the webhook-based architecture avoids the pull_request_target gap, and the document openly acknowledges unresolved questions (webhook-to-trigger translation) rather than papering over them. No blocking findings.

Findings

Info

  • [style] docs/problems/gitlab-implementation.md — The filename follows the problem doc convention, but the document is titled "Implementation Details" rather than framing a problem space with open questions like other problem docs (e.g., security-threat-model.md, agent-architecture.md). The document does have an "Open questions" section, which aligns with the convention. This is a minor stylistic observation — the ADR/implementation-details split is a reasonable organizational choice given the volume of material.

  • [correctness] docs/problems/gitlab-implementation.md:340 — The "constant-time comparison" via SHA256 hashing before bash [ ] string comparison is a pragmatic approach for shell scripts, though [ ] itself is not guaranteed constant-time. The security benefit comes from comparing hashes (which leak information about the hash, not the token). The document's inline comments correctly describe the intent. No remediation needed for a design document.

  • [correctness] docs/ADRs/0027-gitlab-support.md — The ADR accurately reflects the current state of forge.Client in internal/forge/forge.go. The three GitHub-specific methods cited (ListOrgInstallations, GetAppClientID, DispatchWorkflow) all exist at the referenced interface. The proposed forge-neutral replacements (CreateRoleCredential, TriggerPipeline, CreateWebhook) are clearly marked as illustrative.

  • [intent-alignment] — PR title says "Add ADR-0027" but the PR also adds a problem document (docs/problems/gitlab-implementation.md) and a README entry. This is appropriate scope for an ADR that needs a companion implementation document, and matches the pattern described in CLAUDE.md ("Problem-oriented documents that can evolve independently, with ADRs spun off later when decisions crystallize").

Footer

Outcome: approve
This review applies to SHA 491cae45399adacf81de4257cb8b3248fc26927d. Any push to the PR head clears this review and requires a new evaluation.

Previous run (8)

Review: #601

Head SHA: af0b94a
Timestamp: 2026-05-05T00:00:00Z
Outcome: request-changes

Summary

This PR adds a well-structured ADR-0027 and companion problem document proposing GitLab support architecture. The design is thorough — the security analysis of webhook-vs-shim trade-offs, the forge-neutral interface evolution, and the honest acknowledgment of open questions are all strong. However, the new problem document docs/problems/gitlab-implementation.md is missing the ## Open questions section that every other problem document in the repo has, as required by CLAUDE.md. Additionally, CLAUDE.md requires new problem documents to be linked from README.md, which was not done. These are structural compliance issues that should be resolved before merge.

Findings

High

  • [style/conventions] docs/problems/gitlab-implementation.md — Missing required ## Open questions section. CLAUDE.md states: "Each problem document has an 'Open questions' section — this is where unresolved issues live." All 18 existing problem documents follow this convention. The document discusses open questions inline (e.g., webhook-to-trigger translation in the shim security section), but these should be consolidated into a dedicated section at the end of the document.
    Remediation: Add an ## Open questions section at the end of gitlab-implementation.md. Move or cross-reference the inline open questions (webhook translation architecture, interface design scope) into this section.

  • [style/conventions] README.md — New problem document not linked. CLAUDE.md states: "When adding new problem areas, create a new file in docs/problems/ and link it from README.md." The PR adds docs/problems/gitlab-implementation.md but does not add a corresponding entry to the problem document list in README.md (lines 17-36).
    Remediation: Add an entry like [GitLab Implementation](docs/problems/gitlab-implementation.md) — GitLab CI/CD mapping, authentication model, and webhook-based security architecture to the problem document list in README.md.

Medium

  • [correctness] docs/problems/gitlab-implementation.md:302 — Illustrative webhook token comparison uses string equality (!=) which is vulnerable to timing side-channels. While the surrounding comments clearly note this must be constant-time in production, the code sample will likely be copy-pasted. Consider using the constant-time pattern directly in the illustrative code (e.g., comparing SHA256 digests as shown in the comment) so the sample itself is safe to reuse.
    Remediation: Replace the string equality check with the constant-time comparison pattern already described in the comments (openssl dgst -sha256 comparison).

Low

  • [correctness] docs/ADRs/0027-gitlab-support.md:145 — The ADR notes that Project Access Token expiration "requires rotation" and that GitHub App keys "don't expire." GitHub App private keys don't expire, but installation tokens do (1-hour TTL). The comparison is slightly misleading — the real difference is that GitHub App key rotation is optional while GitLab PAT rotation is mandatory. Consider clarifying.

Info

  • [style/conventions] docs/problems/gitlab-implementation.md:373 — The implementation note about extracting duplicated validation logic into a shared script is good practice guidance. Flagging for visibility — this should be tracked as a follow-up during implementation.

  • [intent alignment] The PR is appropriately scoped as a design proposal. No code changes, no modifications to existing files, clean ADR numbering (0027 is the next available), and correct placement of both documents. Referenced ADRs (0005, 0007, 0008, 0009) all exist and are accurately characterized.

  • [injection defense] PR body, commit messages, and document content inspected. No prompt injection patterns, non-rendering Unicode, or bidirectional overrides detected.

Footer

Outcome: request-changes
This review applies to SHA af0b94a309447fd5b82974fbf98154d3a8b7d10a. Any push to the PR head clears this review and requires a new evaluation.

Previous run (9)

Review: #601

Head SHA: 604160dbc155717b4da26cc0c0f807ccc874da11
Timestamp: 2026-05-05T00:00:00Z
Outcome: comment-only

Summary

This ADR proposes a comprehensive architecture for extending fullsend to support GitLab alongside GitHub. The design is well-researched: cross-references to ADRs 0005, 0007, 0008, and 0009 are accurate (verified against source), the forge.Client interface analysis correctly identifies existing GitHub-specific methods that need generalization, and the security analysis is thorough (protected branch requirements, token scoping, webhook authenticity, constant-time comparison callouts). No injection patterns, non-rendering Unicode, or security concerns were found in the PR content or the ADR text. The findings below are non-blocking observations about document structure and minor design notes for the implementation phase.

Findings

Medium

  • [style/conventions] docs/ADRs/0027-gitlab-support.md — At 676 lines, this ADR is roughly 13–20x the typical ADR length (30–50 lines), a point the ADR itself raises in its "Open Questions" section. The implementation details (Sections 4–7: shim pipeline security, cross-repo dispatch, event mapping, state machine primitives), code snippets, and YAML templates read more like a design document than a decision record. Per CLAUDE.md, problem-oriented documents in docs/problems/ are the appropriate home for material that "can evolve independently." Consider splitting: keep the ADR focused on the architectural decision (context, options, decision rationale, consequences) and move implementation details to docs/problems/gitlab-implementation.md, linked from the ADR.

  • [correctness] docs/ADRs/0027-gitlab-support.md:308–321 — The generate-config job in Section 5 duplicates the SOURCE_PROJECT format validation and enrollment check from Section 4's dispatch snippet. The ADR acknowledges this with a NOTE comment, but duplicated validation logic in a single pipeline definition creates a maintenance risk: if one section is updated during implementation, the other may fall out of sync. Consider noting that these should be extracted into a shared script or function during implementation.

Low

  • [correctness] docs/ADRs/0027-gitlab-support.md:595–600 — The illustrative detectForge function uses strings.HasSuffix(host, ".github.com") for subdomain matching. This would match attacker-controlled subdomains (e.g., evil.github.com). Since this code is explicitly marked as illustrative and the ADR notes it should live in internal/forge/detect.go, this is not blocking, but the implementation should use an allowlist or DNS-based validation rather than suffix matching.

  • [style/conventions] docs/ADRs/0027-gitlab-support.md:358–369 — The envsubst pattern for generating the child pipeline config (writing a heredoc with $pipeline_file, then running envsubst to substitute, then mv-ing the temp file) is more complex than necessary. A simpler approach would use cat with shell variable expansion or sed. Minor readability concern for illustrative code.

Info

  • [intent-alignment] No linked issue for this PR. The change is a "Proposed" ADR for design exploration, which aligns with the repo's stated purpose. No scope concern.

  • [correctness] The ADR correctly identifies that ADR-0005's promise ("Adding a new forge requires implementing forge.Client — no changes to layers, CLI, or app setup code") needs qualification. The current forge.Client interface (verified in internal/forge/forge.go) contains GitHub-specific methods (ListOrgInstallations, GetAppClientID) and the proposed forge-neutral additions (CreateRoleCredential, TriggerPipeline, CreateWebhook) are a reasonable evolution path. The extension interface pattern for backward compatibility is well-designed.

  • [platform-security] The ADR's security model is sound. The webhook-based approach for GitLab correctly avoids exposing FULLSEND_DISPATCH_TOKEN to enrolled repos. The open question about webhook-to-trigger translation (Options 1–3) honestly presents the security/infrastructure trade-off without prematurely choosing. The constant-time comparison requirement for token validation is prominently called out.

Footer

Outcome: comment-only
This review applies to SHA 604160dbc155717b4da26cc0c0f807ccc874da11. Any push to the PR head clears this review and requires a new evaluation.

Previous run (10)

Review: #601

Head SHA: 22019d9
Timestamp: 2026-05-05T00:00:00Z
Outcome: comment-only

Summary

This ADR proposes a comprehensive architecture for GitLab support, covering CI/CD mapping, authentication via Project Access Tokens, webhook-based security (replacing pull_request_target), and forge-neutral interface evolution. The design is thorough and honest about trade-offs — it correctly identifies the webhook-to-trigger translation gap as an open question rather than papering over it. The ADR accurately references ADR-0005, 0007, 0008, 0009, and 0026, and the proposed forge-neutral interface methods (CreateRoleCredential, TriggerPipeline, CreateWebhook) align well with ADR-0005's vocabulary conventions. No critical or high findings; the items below are observations for the author to consider.

Findings

Medium

  • [Correctness] docs/ADRs/0027-gitlab-support.md:242-247 — The config lookup uses PROJECT_NAME="${SOURCE_PROJECT##*/}" which extracts only the last path component. For nested GitLab groups (group/subgroup/project), two projects with the same name in different groups would collide in the config.yaml lookup (yq ".repos.\"$PROJECT_NAME\".enabled"). The webhook token validation correctly uses full-path SHA256 hashing, but the enrollment check uses only the basename. Consider using the full project path (with appropriate escaping for yq) or documenting this as a known limitation for nested groups.

  • [Style/conventions] docs/ADRs/0027-gitlab-support.md — The ADR is 668 lines, which the Open Questions section itself flags as significantly exceeding typical ADR length (30-50 lines). Per CLAUDE.md, problem-oriented documents in docs/problems/ are the appropriate home for implementation details that evolve independently. Consider extracting Sections 4-7 (shim security, dispatch, event mapping, implementation phases) into docs/problems/gitlab-implementation.md and keeping the ADR focused on the decision rationale, high-level architecture, and consequences.

Low

  • [Correctness] docs/ADRs/0027-gitlab-support.md:351-357 — The child pipeline config generation writes a heredoc with literal $pipeline_file, then uses envsubst to substitute it. This works but is fragile — if future maintainers add other variables to the template, envsubst '$pipeline_file' would silently skip them. A brief comment explaining why the restricted substitution is intentional would help.

  • [Correctness] docs/ADRs/0027-gitlab-support.md:283 — The dispatch token exposure discussion notes that FULLSEND_DISPATCH_TOKEN should be a protected variable, but then acknowledges this breaks the webhook translation pattern (option 1). Since the ADR's preferred approach is webhooks (not option 1), the protected-variable mitigation is actually compatible with the chosen architecture. Clarifying this would strengthen the security argument.

Info

  • [Intent alignment] The ADR transparently addresses the tension with ADR-0005's promise ("no changes to layers, CLI, or app setup code") by proposing forge-neutral interface methods that push forge-specific logic into Client implementations. The proposed changes to appsetup, layers, and CLI are minimal and appropriate — they replace GitHub-specific calls with forge-neutral ones rather than adding conditional logic. This is a good architectural approach.

  • [Correctness] The illustrative bash snippets correctly note that Alpine's /bin/sh (ash) doesn't support ${!VAR} indirect expansion or [[ regex ]], and prescribe bash. The apk add bash + heredoc approach is correct for the illustrative purpose.

Footer

Outcome: comment-only
This review applies to SHA 22019d90753321ad3aa88e7fcc8d5a79548d293b. Any push to the PR head clears this review and requires a new evaluation.

Previous run (11)

Review: #601

Head SHA: ebb934f
Timestamp: 2026-05-05T00:00:00Z
Outcome: comment-only

Summary

This ADR proposes a well-researched architecture for GitLab support, correctly identifying the key challenges (no pull_request_target equivalent, webhook-to-trigger translation gap, Project Access Token limitations vs GitHub Apps). The security analysis is thorough, the forge interface evolution strategy is sound, and claims about existing ADRs and the forge.Client interface were verified against the source. No blocking issues found. The main observations are structural: the document is ~15x longer than typical ADRs in this repo and contains an internal inconsistency in webhook token naming.

Findings

Medium

  • [style-conventions] docs/ADRs/0027-gitlab-support.md — At 668 lines, this ADR is significantly larger than any other ADR in the repo (most are 30-80 lines). The document itself raises this concern in "Open Questions > ADR Scope and Structure." Implementation details (Sections 4-9, Implementation Notes, CLI Changes) read as design documentation rather than a decision record. Consider extracting implementation details to docs/problems/gitlab-implementation.md per CLAUDE.md's guidance that problem-oriented documents handle evolving design, leaving the ADR focused on the decision (webhook-based dispatch, Project Access Tokens, parallel template architecture) and high-level consequences.

Low

  • [correctness] docs/ADRs/0027-gitlab-support.md:258 — The enrollment setup section references underscore-sanitized variable names (WEBHOOK_TOKEN_myorg__myrepo, WEBHOOK_TOKEN_my_H_org__my_H_repo) while the dispatch validation section (lines 218-221) and security properties section (line 264) describe SHA256 hashing (WEBHOOK_TOKEN_<sha256(project_path)>). These are contradictory approaches in the same document. Recommend removing the underscore-sanitization examples from line 258 since the SHA256 approach was explicitly chosen to avoid collision issues.

Info

  • [correctness] docs/ADRs/0027-gitlab-support.md:207 — The simplified SOURCE_PROJECT regex (^[a-zA-Z0-9_-]+/[a-zA-Z0-9_-]+$) excludes dots and nested groups, which are valid in GitLab project paths. The document notes this (lines 204-206, 314-316), but since this is security-critical input validation in illustrative code that implementers may copy directly, consider using the production regex inline and noting the simplified version was considered rather than the reverse.

Footer

Outcome: comment-only
This review applies to SHA ebb934f68fca4dc1bbd1a8b84b58389453b34ad2. Any push to the PR head clears this review and requires a new evaluation.

Previous run (12)

Review: #601

Head SHA: 98bd17e
Timestamp: 2026-05-04T00:00:00Z
Outcome: comment-only

Summary

This ADR proposes a comprehensive GitLab support architecture that correctly maps GitHub primitives to GitLab equivalents. The security analysis is thorough — particularly the honest assessment of the pull_request_target gap and the dispatch token exposure risk. The document is well-structured and internally consistent with the existing ADR corpus (ADR-0005, 0007, 0008, 0009). No critical or high issues were identified. Several medium and low findings are noted below for consideration before acceptance.

Findings

Medium

  • [correctness] docs/ADRs/0027-gitlab-support.md:167-173 — The webhook-to-trigger API incompatibility section concludes that an intermediary is required, then recommends option 1 (GitLab CI/CD webhook integration), which uses CI_PIPELINE_SOURCE == "webhook" in the enrolled repo. But this option doesn't actually solve the incompatibility described — it runs a pipeline in the enrolled repo (on unprotected MR branches), which the ADR itself identifies as reintroducing the security concern. The recommendation to use workflow:rules with CI_COMMIT_REF_PROTECTED == "true" would prevent the job from running on MR branches entirely, making it unable to react to MR events — which is its primary purpose. This tension is acknowledged but not resolved. Consider documenting this as an explicit open question rather than a recommendation.

  • [correctness] docs/ADRs/0027-gitlab-support.md:256-259 — The enrollment setup says the webhook URL is https://gitlab.com/api/v4/projects/<fullsend-project-id>/trigger/pipeline, but Section 4 lines 166-167 established that webhook payloads and the trigger API are not wire-compatible ("pointing a webhook URL directly at the trigger endpoint results in a malformed request"). These two statements contradict each other. The webhook URL should either be clarified as requiring the intermediary, or this line should be updated to reflect the actual webhook target.

  • [correctness] docs/ADRs/0027-gitlab-support.md:500-553 — The forge-neutral interface additions propose CreateRoleCredential, TriggerPipeline, and CreateWebhook as additions to forge.Client. However, the current forge.Client interface (in internal/forge/forge.go) already has specific methods like DispatchWorkflow, ListOrgInstallations, and GetAppClientID. The ADR mentions deprecating these but doesn't address backward compatibility or migration strategy. The existing interface has ~40 methods; adding new ones while deprecating old ones needs a concrete plan to avoid interface bloat. Consider referencing this as follow-up work more explicitly.

Low

  • [style] docs/ADRs/0027-gitlab-support.md — At 633 lines, this ADR is significantly larger than all existing ADRs in the repo (most are 30-50 lines). The Implementation Notes section (lines 498-607) reads more like a design document than an architectural decision record. Consider whether the implementation details (CLI changes, package layout, Go code snippets) belong in a separate design doc under docs/problems/ with the ADR referencing it, per the repo's convention that problem documents explore options while ADRs record decisions.

  • [style] docs/ADRs/0027-gitlab-support.md:4-5 — The relates_to frontmatter references agent-infrastructure and agent-architecture. Verify these match actual filenames in docs/problems/ (the template says "filename without .md from docs/problems/").

  • [correctness] docs/ADRs/0027-gitlab-support.md:219-222 — The variable name sanitization scheme (/__, -_H_) acknowledges a collision risk with pathological project names but dismisses it as an admin responsibility. For a security-critical token lookup, collision-resistant encoding should be a requirement, not a recommendation. The SHA256 alternative mentioned is stronger — consider making it the primary recommendation.

Info

  • [intent-alignment] No linked issue for this PR. The change is consistent with ADR-0005's stated goal of multi-forge support and the repo's purpose as a design exploration. No scope concerns.

  • [injection-defense] PR body, commit messages, and ADR content are clean. No prompt injection patterns, suspicious Unicode, or hidden instructions detected.

Footer

Outcome: comment-only
This review applies to SHA 98bd17e9e94c7215e7ba3edc2dea48db4cfa1d8b. Any push to the PR head clears this review and requires a new evaluation.

Previous run (13)

Review: #601

Head SHA: 2f6e5a4
Timestamp: 2026-05-04T00:00:00Z
Outcome: comment-only

Summary

This PR adds a well-structured ADR proposing GitLab support architecture. The design is thorough, correctly identifies the key mapping challenges between GitHub and GitLab primitives, and proposes reasonable solutions. No critical or high issues found. Two medium findings relate to a webhook-to-pipeline API incompatibility that undermines a core assumption of the proposed architecture, and a potential gap in the dispatch token security model. These are design-level concerns appropriate for feedback on a "Proposed" ADR.

Findings

Medium

  • [Correctness] docs/ADRs/0027-gitlab-support.md:159-168 — The ADR assumes GitLab webhooks can POST directly to the pipeline trigger API endpoint (/api/v4/projects/:id/trigger/pipeline). However, GitLab webhooks send a JSON event payload (e.g., merge request event object), while the pipeline trigger API expects form-encoded parameters (token, ref, and variables[KEY]=value). These APIs are not wire-compatible — pointing a webhook URL at the trigger endpoint will result in a malformed request. This undermines the core argument in lines 164-168 that "No external hosting is required — the webhook targets GitLab's own API." An intermediary would be needed to translate webhook payloads to trigger API calls, which partially reintroduces the "hosted webhook receiver" that ADR-0009 rejected for GitHub.
    Remediation: Acknowledge this gap and evaluate alternatives: (a) a lightweight GitLab serverless function or CI/CD integration that translates webhooks to trigger API calls, (b) using GitLab's native MR pipeline triggers (though these run MR code, conflicting with the security model), or (c) a minimal bridge service. Update the ADR-0009 comparison to account for the middleware requirement.

  • [Correctness] docs/ADRs/0027-gitlab-support.md:276 — The FULLSEND_DISPATCH_TOKEN (pipeline trigger token) is stored as a group-level CI/CD variable visible to all enrolled repos. Any CI/CD job in any enrolled repo can read this token. A malicious MR in an enrolled repo could exfiltrate this token during a pipeline run (e.g., curl it to an external endpoint) and then use it to trigger arbitrary pipelines in .fullsend with crafted variables. While the CI_COMMIT_REF_PROTECTED == "true" guard in dispatch.yml would prevent execution on unprotected branches, this relies on the workflow:rules being correctly configured. The GitHub equivalent (ADR-0008) has the same exposure pattern with FULLSEND_DISPATCH_TOKEN as an org-level Actions secret, but GitHub's workflow_dispatch only triggers workflows already committed to the target ref — GitLab's trigger API is more permissive in what variables it accepts.
    Remediation: Document this attack surface explicitly. Consider whether the trigger token should be a project-level variable in .fullsend only (not group-level), with enrolled repos using a different mechanism (e.g., the webhook secret token) to authenticate dispatch requests.

Low

  • [Correctness] docs/ADRs/0027-gitlab-support.md:173-247,279-388 — Sections 4 and 5 both present YAML for dispatch.yml with different architectures. Section 4 shows a single-job structure focused on webhook token validation. Section 5 shows a two-job generate-config + trigger-stage pattern. While the prose explains their relationship (lines 268-269), the duplication with divergent structure may confuse implementers about the target architecture. The validation logic is duplicated with a comment referencing Section 4, but Section 5 omits the webhook token validation step entirely.
    Remediation: Consider consolidating into a single complete pipeline definition, or more clearly mark Section 4's snippet as a security-focused excerpt of the full pipeline shown in Section 5.

  • [Style] docs/ADRs/0027-gitlab-support.md:196-201 — The SOURCE_PROJECT regex validation (^[a-zA-Z0-9_-]+/[a-zA-Z0-9_-]+$) is noted as needing expansion for dots and nested groups, but the NOTE comment about this is easy to miss in implementation. The same limitation appears in Section 5 (line 309). Since GitLab supports deeply nested groups (e.g., group/subgroup/subsubgroup/project), the two-segment assumption is a significant functional limitation.
    Remediation: Consider defining the expanded regex in the ADR itself (even as illustrative) to prevent implementers from shipping the restrictive version.

Info

  • [Style] docs/ADRs/0027-gitlab-support.md — The ADR follows the template structure correctly (frontmatter, Context, Options, Decision, Consequences). Number 0027 is sequential after 0026. Cross-references to ADRs 0005, 0007, 0008, 0009 are accurate. The forge-neutral vocabulary (ChangeProposal, RoleCredential) is consistent with ADR-0005's conventions.

  • [Intent alignment] The ADR correctly identifies that the current forge.Client interface (35 methods in internal/forge/forge.go) contains GitHub-specific methods (ListOrgInstallations, GetAppClientID, DispatchWorkflow) that would need generalization. The proposed forge-neutral additions (CreateRoleCredential, TriggerPipeline, CreateWebhook) are directionally sound and consistent with ADR-0005's design intent.

Footer

Outcome: comment-only
This review applies to SHA 2f6e5a4b2146c107ccab3cbb7756f51fda03206b. Any push to the PR head clears this review and requires a new evaluation.

Previous run (14)

Review: #601

Head SHA: 82114c0
Timestamp: 2026-05-04T00:00:00Z
Outcome: comment-only

Summary

Well-structured ADR proposing GitLab support architecture that demonstrates strong security awareness (webhook validation, protected branch requirements, payload encoding). The design closely follows ADR-0005's forge abstraction principles and proposes reasonable forge-neutral interface additions. Three technical issues in the illustrative YAML snippets are worth addressing before the ADR is accepted, though none are blocking for a "Proposed" status document.

Findings

Medium

  • [Correctness] docs/ADRs/0027-gitlab-support.md:209-211 — The webhook token variable name sanitization scheme preserves hyphens (foo-bar/baz → WEBHOOK_TOKEN_foo-bar__baz), but GitLab CI/CD variable names must match [A-Za-z_][A-Za-z0-9_]* — hyphens are not valid characters. The sanitization needs to also replace - with _ (or another valid character), with a collision-resistance analysis for the expanded scheme (e.g., foo-bar vs foo_bar would collide).
    Remediation: Update the sanitization scheme to replace both / with __ and - with a distinct valid sequence (e.g., _H_ for hyphen), then update the collision analysis.

  • [Correctness] docs/ADRs/0027-gitlab-support.md:291-355 — Section 5's generate-config job uses [[ ! "" =~ ... ]] bash syntax in a - | script block, but the image is alpine:latest which uses ash (not bash) as /bin/sh. Section 4 correctly wraps its script in bash <<'BASH' to handle bashisms. This inconsistency means the illustrative code in Section 5 would fail if used as-is.
    Remediation: Either wrap the Section 5 script in bash <<'BASH' (matching Section 4's pattern) or add apk add --no-cache bash and use a bash shebang.

  • [Correctness] docs/ADRs/0027-gitlab-support.md:345envsubst is called without a variable restriction argument, so it substitutes ALL environment variables in the template, not just . CI-injected variables like , ``, etc. would be unintentionally substituted if they appear in any included YAML.
    Remediation: Use envsubst '\' to restrict substitution to only the intended variable.

Low

  • [Style/conventions] docs/ADRs/0027-gitlab-support.md:274-379 — The validation logic (SOURCE_PROJECT format check, enrollment check) is duplicated between Section 4 and Section 5, with inline comments noting the duplication. While the ADR explains both sections describe the same dispatch.yml pipeline, the two snippets could drift during future edits. Consider presenting the full pipeline once and referencing specific portions in the security discussion.

Info

  • [Correctness] docs/ADRs/0027-gitlab-support.md:196-201 — The SOURCE_PROJECT regex limitation (no dots or nested groups) is well-documented with a clear NOTE about what to expand during implementation. No action needed now.

  • [Correctness] docs/ADRs/0027-gitlab-support.md:220-229 — The constant-time comparison requirement is clearly documented with a SECURITY comment, and the illustrative code intentionally uses != for clarity. Good documentation practice.

Footer

Outcome: comment-only
This review applies to SHA 82114c03905970419d1a8b06b641003b1577510d. Any push to the PR head clears this review and requires a new evaluation.

Previous run (15)

Review: #601

Head SHA: 410a472c15efc076598a86485abcd0709ef71e37
Timestamp: 2026-05-04T00:00:00Z
Outcome: comment-only

Summary

This is a well-structured design ADR proposing GitLab support architecture. The document demonstrates strong security awareness — it identifies timing-safe comparison gaps, variable name collisions, regex validation limitations, and YAML injection risks, annotating each with NOTE: comments for implementors. No critical or high findings. The medium findings below are design-level observations worth addressing before the ADR moves to "Accepted" status.

Findings

Medium

  • [Correctness] docs/ADRs/0027-gitlab-support.md:211 — Webhook token variable name collision: the tr '/' '_' | tr '-' '_' sanitization maps distinct project paths to the same variable name (e.g., foo-bar/baz and foo_bar/baz both become WEBHOOK_TOKEN_foo_bar_baz). The ADR notes this but leaves it unresolved. Since this is a security-critical lookup (token validation), the collision-resistant encoding should be decided in this ADR rather than deferred to implementation.
    Remediation: Specify the encoding scheme (e.g., double-underscore for /, percent-encoding for -, or SHA256 hash of the project path) as part of the decision.

  • [Correctness] docs/ADRs/0027-gitlab-support.md:224 — Timing-safe token comparison: the ADR notes the != string comparison is not timing-safe but doesn't commit to a solution. For a security architecture ADR, the mitigation should be prescribed, not optional.
    Remediation: State that implementations MUST use constant-time comparison (e.g., openssl dgst -sha256 HMAC comparison) and add this to the security properties list.

  • [Correctness] docs/ADRs/0027-gitlab-support.md:331 — The include: local: heredoc writes $pipeline_file literally (via <<'EOF') then uses sed to substitute it. This works but is fragile — if a future stage filename contains @ or regex metacharacters, the sed substitution breaks silently. The ADR's own NOTE acknowledges this partially.
    Remediation: Consider using envsubst or a more robust templating approach in the design, or document the filename constraints explicitly.

Low

  • [Style/conventions] docs/ADRs/0027-gitlab-support.md:1 — Frontmatter title uses unpadded numbering ("27. GitLab Support Architecture") while ADR-0025 uses padded format ("0025."). ADR-0026 also uses unpadded, so there's an existing inconsistency in the repo, but this is worth standardizing.
    Remediation: Pick one convention and apply it consistently. If unpadded is the new standard, update ADR-0025's frontmatter for consistency.

  • [Intent alignment] docs/ADRs/0027-gitlab-support.md — The ADR proposes forge-neutral interface methods (CreateRoleCredential, TriggerPipeline, CreateWebhook) but the current forge.Client uses forge-neutral vocabulary (ChangeProposal not "pull request"). The proposed method names like CreateWebhook are already forge-neutral, which is good. However, CreateRoleCredential might benefit from aligning with the existing naming pattern (e.g., documenting what the forge-neutral noun for "credential" is, similar to how ChangeProposal replaces "PR").
    Remediation: Consider whether the forge-neutral vocabulary convention from ADR-0005 should extend to these new methods (e.g., RoleCredential as the canonical term).

Info

  • [Correctness] The ADR contains duplicate validation logic between Section 4 (security-focused snippet) and Section 5 (full dispatch pipeline). The ADR acknowledges this duplication explicitly with cross-references. This is acceptable for a design document but implementors should ensure only one copy exists in the actual code.

  • [Platform security] The webhook-based approach for GitLab is architecturally sound and correctly avoids the pull_request_target gap. The justification for why webhooks work for GitLab but were rejected for GitHub (ADR-0009) is well-reasoned — GitLab's built-in pipeline trigger API serves as the webhook receiver, maintaining compute-platform agnosticism.

  • [Injection defense] No prompt injection patterns, non-rendering Unicode, or bidirectional overrides detected in the PR body, commit messages, or diff content.

Footer

Outcome: comment-only
This review applies to SHA 410a472c15efc076598a86485abcd0709ef71e37. Any push to the PR head clears this review and requires a new evaluation.

@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.

@ralphbean ralphbean 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.

ADR-0026 Review: GitLab Support Architecture

The direction is sound — GitLab support is well-motivated (issue #322), builds on the forge abstraction (ADR-0005), and the ADR covers a lot of ground. However, there are critical gaps in the security model and event-handling architecture that need to be addressed before this can be accepted.

Key concerns

  1. Security model relies on workflow:rules + CI_COMMIT_REF_PROTECTED, but GitLab MR pipelines run the source branch's CI config. An attacker can remove these checks from their branch. The real mitigation is protected CI/CD variables (not just masked), which GitLab only exposes to pipelines on protected branches. The ADR says "masked" but never says "protected."

  2. GitLab CI cannot natively trigger on issue events, issue comments, or MR comments. CI_PIPELINE_SOURCE has 15 valid values — none for issues or notes. The event mapping table says "Webhook → shim pipeline" but doesn't explain how, because no native mechanism exists. This has significant architectural implications (external webhook receiver needed?) that conflict with compute-platform agnosticism.

  3. Sandbox/compute architecture is missing. How does OpenShell run on GitLab CI runners? Which executor types are compatible? This should be discussed or explicitly scoped out with a reference to a future ADR.

  4. Forge abstraction needs discussion. ADR-0005 promises "no changes to layers, CLI, or app setup code" for new forges, but this ADR lists modifications to admin.go, workflows.go, enrollment.go, appsetup/, and config.go. The ADR should propose forge-neutral interface additions and flag existing GitHub-specific methods (ListOrgInstallations, GetAppClientID) that need generalization.

Comment thread docs/ADRs/0028-gitlab-support.md
Comment thread docs/ADRs/0026-gitlab-support.md Outdated
Comment thread docs/ADRs/0028-gitlab-support.md
Comment thread docs/ADRs/0028-gitlab-support.md
Comment thread docs/ADRs/0027-gitlab-support.md Outdated
Comment thread docs/ADRs/XXXX-gitlab-support.md Outdated
Comment thread docs/ADRs/0027-gitlab-support.md Outdated

@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.

@ggallen ggallen force-pushed the docs/adr-026-gitlab-support-clean branch from d135b32 to d158a0a Compare May 4, 2026 20:05

@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.

@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.

@ggallen ggallen changed the title Add ADR-0026 for GitLab support architecture Add ADR-0027 for GitLab support architecture May 4, 2026

@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.

@ggallen ggallen requested a review from ralphbean May 4, 2026 21:06

@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.

@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.

@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.

@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.

@ggallen

ggallen commented May 4, 2026

Copy link
Copy Markdown
Member Author

/review

@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.

@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.

@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.

@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.

@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.

@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.

@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.

@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.

@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.

@rh-hemartin

Copy link
Copy Markdown
Member

I don't see Multi-Pipelines mentioned: https://docs.gitlab.com/ci/pipelines/downstream_pipelines/?tab=Multi-project+pipeline.

For the security concern to be true the attacker needs to modify the target repo dispatch to target a .fullsend ref that is not main, where this attacker introduced a branch that exfiltrates secrets. If the secrets are just scoped to the main branch, then the attacker can't exfiltrate anything even if it lands a branch in the .fullsend repository.

ggallen added a commit to ggallen/fullsend that referenced this pull request May 5, 2026
Address security concerns raised by rh-hemartin in PR fullsend-ai#601:

1. **Protected CI/CD Variables (Defense-in-Depth)**
   Add new required security control: all secrets in .fullsend project MUST
   be marked as protected variables. This restricts them to pipelines running
   on protected branches only.

   Threat model: GitLab's trigger API accepts a 'ref' parameter with no
   documented restrictions - anyone with the trigger token can specify any
   branch. Protected variables defend against:
   - Webhook intermediary compromise/bugs triggering on ref=malicious-branch
   - Insider threats (write access to .fullsend but can't bypass main protection)
   - Misconfiguration of trigger API calls

   Even if dispatch pipeline runs on unprotected branch, secrets won't be exposed.

2. **Webhook Intermediary Security Requirements**
   Add explicit requirements for translation service:
   - MUST hardcode ref=main when calling .fullsend trigger API
   - MUST NOT derive ref from webhook payload
   - MUST validate webhook secret token
   - MUST use least-privilege credentials

   Rationale: Intermediary controls which branch dispatch runs on. If it accepts
   attacker-controlled ref values, insider with .fullsend write could exfiltrate
   secrets via malicious branch (mitigated by protected variables).

3. **Multi-Project Pipelines Consideration**
   Document why GitLab's multi-project pipeline feature is not suitable:
   - Would run MR code with potential secret access (violates security model)
   - GitLab warns against passing masked variables to multi-project pipelines
   - Parent-child pipelines within .fullsend provide better security guarantees

4. **Update Dispatch Token Security Section**
   Correct statement that protected variables don't apply to chosen architecture.
   Protected variables are defense-in-depth control that applies to ALL
   architectures and provides critical protection against ref manipulation.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@ggallen

ggallen commented May 5, 2026

Copy link
Copy Markdown
Member Author

Thanks for raising these points! You're absolutely right on both counts. I've investigated and updated the implementation doc:

Multi-Project Pipelines

Good catch - I hadn't explicitly considered GitLab's multi-project pipelines. I've now documented why they're not suitable for fullsend's security model:

  • Would trigger FROM .fullsend TO enrolled repos, potentially running MR code with secret access
  • GitLab explicitly warns: "Do not use this method to pass masked variables to a multi-project pipeline"
  • The chosen parent-child pipeline approach (all within .fullsend protected main branch) provides better security guarantees

See the new Multi-Project Pipelines section in the implementation doc.

Protected Variables Defense-in-Depth

Your security analysis is spot-on. The threat is real but requires insider-level access or intermediary compromise:

Attack vector: The webhook translation intermediary calls GitLab's trigger API with ref parameter. The pipeline trigger API docs confirm that anyone with a trigger token can specify any ref with no documented restrictions. So if:

  1. The intermediary has a bug or is compromised to accept attacker-controlled refs, OR
  2. An insider with .fullsend write access creates a malicious branch

Then the dispatch pipeline could run on an unprotected branch.

Your point about protected variables is correct: GitLab's protected variables are only exposed to pipelines running on protected branches. This provides critical defense-in-depth.

I've updated the doc to make protected variables a required security control (not optional):

Thanks for the careful review - this strengthens the security model significantly.

@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.

ggallen and others added 14 commits May 6, 2026 11:25
Medium findings addressed:

1. GitLab CI/CD variable naming constraint: Variable names must match
   [A-Za-z_][A-Za-z0-9_]* (no hyphens allowed). Updated sanitization
   scheme to replace both / → __ and - → _H_. Added collision analysis
   noting pathological cases (e.g., "foo_H_bar/baz" collides with
   "foo-bar/baz") and recommending SHA256 hash for collision-free
   guarantee if needed. Updated examples throughout.

2. Bash syntax in alpine image: Section 5 used [[ regex ]] bashism but
   alpine:latest defaults to ash (/bin/sh). Wrapped script in
   bash <<'BASH' heredoc (matching Section 4 pattern) and added bash to
   apk install.

3. envsubst variable restriction: envsubst without arguments substitutes
   ALL environment variables. Added '$pipeline_file' argument to
   restrict substitution to only the intended variable, preventing
   unintended substitution of CI-injected vars like $CI_PROJECT_PATH.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…sure)

Medium findings addressed:

1. Webhook-to-trigger API incompatibility: GitLab webhooks send JSON
   payloads but the trigger API expects form-encoded parameters. They
   are not wire-compatible - direct webhook→trigger doesn't work.
   Added section documenting this gap and three options for translation:
   - GitLab CI/CD webhook integration (keeps everything in GitLab CI/CD)
   - GitLab serverless functions (requires Premium/Ultimate tier)
   - Minimal bridge service (reintroduces hosted receiver concern)
   Recommended option 1 for initial implementation with documented
   residual risk and comparison to ADR-0009.

2. Dispatch token exposure risk: FULLSEND_DISPATCH_TOKEN is group-level
   and visible to all enrolled repos. Malicious MR could exfiltrate and
   use to trigger arbitrary pipelines. Added security consideration
   section documenting attack surface, comparing to GitHub (ADR-0008),
   and proposing mitigations (protected variables, webhook secret auth).

Low findings addressed:

- SOURCE_PROJECT regex: Added expanded regex for production use
  (^[a-zA-Z0-9._-]+(/[a-zA-Z0-9._-]+)+$) supporting dots and nested
  groups, with note about bracket notation for yq queries.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Medium findings:
- Clarify webhook-to-trigger incompatibility: option 1 (CI/CD webhook
  integration) cannot enforce protected-branch-only without blocking MR
  reactions entirely. Document as open question instead of recommending
  a non-working solution.
- Fix enrollment setup contradiction: webhook URL requires translation
  intermediary (serverless/bridge), cannot point directly to trigger API.
- Add backward compatibility strategy for forge.Client interface: deprecation
  phase, extension interfaces for forge-specific methods, breaking change
  timeline.

Low findings:
- Use SHA256 hashing as primary recommendation for collision-free webhook
  token variable naming (not character sanitization).
- Add Open Questions section acknowledging ADR length (650+ lines) and
  suggesting implementation details move to separate design doc.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Low finding: enrollment section referenced underscore-sanitized variable
names (WEBHOOK_TOKEN_myorg__myrepo) while dispatch and security sections
use SHA256 hashing (WEBHOOK_TOKEN_<sha256(project_path)>). Remove the
contradictory examples to maintain consistency with the chosen SHA256
approach for collision-free token identification.

Medium finding (acknowledged): ADR length (668 lines) already documented
in Open Questions section. No action needed beyond existing acknowledgment.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Medium finding - config lookup collision:
Fix enrollment check to use full project path (group/subgroup/project)
instead of basename extraction which caused collisions. Convert slashes
to dots for config.yaml key lookup (repos."group.subgroup.project".enabled).
This maintains consistency with webhook token validation which uses full
path SHA256 hashing.

Low finding - dispatch token clarity:
Clarify that the webhook-based architecture (chosen approach) avoids
dispatch token exposure to enrolled repos entirely. Webhook secret tokens
authenticate enrolled repos, and dispatch token remains internal to
.fullsend. Move the exposure concern to "alternative architecture note"
since it only applies to non-chosen approaches (in-repo shim workflows).

Low finding - envsubst restriction:
Already fixed in previous commit (line 354 uses envsubst '$pipeline_file').

Medium finding - ADR length:
Already acknowledged in "Open Questions > ADR Scope and Structure" section.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Medium finding - duplicated validation logic:
Add implementation note to Section 5 stating that validation logic
(SOURCE_PROJECT format check, enrollment check, token validation) appears
in both Section 4 and Section 5 snippets for clarity in the design
document, but should be extracted into a shared script or bash function
during implementation to avoid maintenance drift.

Low finding - detectForge subdomain matching:
Add security note to detectForge function that HasSuffix for subdomain
matching is illustrative only. Production implementations should use an
allowlist (github.com, ghe.example.com) or DNS-based validation to
prevent attacker-controlled domains like evil.github.com from being
detected as GitHub.

Medium finding - ADR length:
Already acknowledged in "Open Questions > ADR Scope and Structure" section.

Low finding - envsubst complexity:
Minor readability concern for illustrative code, no action needed.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Address review feedback about ADR length (676 lines → 217 lines, 68% reduction).
Extract implementation details to docs/problems/gitlab-implementation.md per
CLAUDE.md guidance that problem-oriented documents handle evolving design while
ADRs record decisions.

Moved to implementation doc:
- Shim pipeline security (webhook architecture, validation snippets)
- Cross-repo dispatch mechanism (child pipelines, YAML templates)
- Stage markers, event mapping, state machine primitives
- Implementation phases (5-phase rollout plan)
- Forge interface evolution (CreateRoleCredential, TriggerPipeline, etc.)
- CLI changes (forge detection, install command, config schema)
- Security considerations (protected branches, token scoping)

ADR now focuses on:
- Context (why GitLab support is needed)
- Options (rejected alternatives and rationale)
- Decision (high-level architecture: directory structure, CI/CD patterns,
  authentication model)
- Consequences (positive/negative/risks/mitigations)
- Open questions (webhook translation architecture)

The implementation doc is structured for iterative evolution as GitLab support
progresses from design to production. Both documents cross-reference each other.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
High findings:
- Add required "## Open questions" section to gitlab-implementation.md per
  CLAUDE.md convention (all 18 existing problem documents include this).
  Consolidated inline open questions: webhook-to-trigger translation
  architecture and forge interface design details.
- Add gitlab-implementation.md to README.md problem document list (inserted
  after Agent Infrastructure, before Autonomy Spectrum).

Medium finding:
- Fix timing side-channel vulnerability in webhook token comparison code
  sample. Replace string equality (vulnerable to timing attacks) with
  constant-time comparison using SHA256 hashes. Changed from:
    if [ "$WEBHOOK_TOKEN" != "$EXPECTED_TOKEN" ]
  to:
    WEBHOOK_HASH=$(echo -n "$WEBHOOK_TOKEN" | openssl dgst -sha256 -hex | cut -d' ' -f2)
    EXPECTED_HASH=$(echo -n "$EXPECTED_TOKEN" | openssl dgst -sha256 -hex | cut -d' ' -f2)
    if [ "$WEBHOOK_HASH" != "$EXPECTED_HASH" ]

Low finding:
- Clarify GitHub App key expiration comparison. Changed from "GitHub App
  keys don't expire" to "GitHub App private keys never expire, though
  installation tokens have 1-hour TTL and auto-refresh". Makes clear that
  GitLab PAT rotation is mandatory while GitHub App key rotation is optional.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Address security concerns raised by rh-hemartin in PR fullsend-ai#601:

1. **Protected CI/CD Variables (Defense-in-Depth)**
   Add new required security control: all secrets in .fullsend project MUST
   be marked as protected variables. This restricts them to pipelines running
   on protected branches only.

   Threat model: GitLab's trigger API accepts a 'ref' parameter with no
   documented restrictions - anyone with the trigger token can specify any
   branch. Protected variables defend against:
   - Webhook intermediary compromise/bugs triggering on ref=malicious-branch
   - Insider threats (write access to .fullsend but can't bypass main protection)
   - Misconfiguration of trigger API calls

   Even if dispatch pipeline runs on unprotected branch, secrets won't be exposed.

2. **Webhook Intermediary Security Requirements**
   Add explicit requirements for translation service:
   - MUST hardcode ref=main when calling .fullsend trigger API
   - MUST NOT derive ref from webhook payload
   - MUST validate webhook secret token
   - MUST use least-privilege credentials

   Rationale: Intermediary controls which branch dispatch runs on. If it accepts
   attacker-controlled ref values, insider with .fullsend write could exfiltrate
   secrets via malicious branch (mitigated by protected variables).

3. **Multi-Project Pipelines Consideration**
   Document why GitLab's multi-project pipeline feature is not suitable:
   - Would run MR code with potential secret access (violates security model)
   - GitLab warns against passing masked variables to multi-project pipelines
   - Parent-child pipelines within .fullsend provide better security guarantees

4. **Update Dispatch Token Security Section**
   Correct statement that protected variables don't apply to chosen architecture.
   Protected variables are defense-in-depth control that applies to ALL
   architectures and provides critical protection against ref manipulation.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add section to gitlab-implementation.md clarifying that agent execution
environment (runner configuration, executor types, isolation model, OpenShell
integration) is beyond the scope of this document. These details should be
documented in Agent Infrastructure design docs or implementation PRs.

Addresses review finding about sandbox architecture discussion.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Rename 0027-gitlab-support.md to 0028-gitlab-support.md (ADR 0027 already
  exists on main as allowed-and-disallowed-tools-for-agents.md)
- Update all references to ADR-0027 in both ADR and gitlab-implementation.md
- Add TODO(implementation) markers on SOURCE_PROJECT regex lines to prevent
  copy-paste bugs during implementation
- Clarify hash comparison comment - it mitigates timing attacks but bash [ ]
  is not truly constant-time (compares hash bytes, not token bytes)
- Add comment explaining two-step heredoc + envsubst pattern to prevent
  accidental variable expansion
- Move GitLab Implementation entry to end of README problem doc list to
  preserve existing curated ordering

Addresses review findings from fullsend-ai-review.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Replace HasSuffix subdomain matching with explicit allowlist of known forge
domains to prevent subdomain spoofing (e.g., evil.github.com). The allowlist
approach is more secure and was already recommended in the original comment.

Addresses low finding from review: showing the safer pattern directly in the
code example prevents copy-paste security issues during implementation.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Resolves four critical issues identified by ralphbean:

1. Security Model: Add CRITICAL SECURITY REQUIREMENT emphasizing that all
   CI/CD variables must be marked as "protected" in GitLab, not just "masked".
   This is the primary defense-in-depth control preventing secret exfiltration
   via non-protected branch execution.

2. Event Handling: Add section 4 documenting GitLab's lack of native issue/
   comment event support and the required webhook-to-trigger translation
   intermediary. Clarifies architectural gap and three implementation options.

3. Sandbox/Compute: Add section 5 explicitly scoping out agent execution
   environment details (runner types, OpenShell integration) to future ADR.

4. Forge Abstraction: Add section 6 documenting compliance strategy with
   ADR-0005 promise via forge-neutral interface methods (CreateRoleCredential,
   TriggerPipeline) that minimize layer/CLI/appsetup changes.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Addresses 6 moderate-severity review findings:

1. Fix broken ADR cross-references (0005-forge-abstraction.md →
   0005-forge-abstraction-layer.md, 0009-pull-request-target.md →
   0009-pull-request-target-in-shim-workflows.md)

2. Remove invalid CI_PIPELINE_SOURCE == "webhook" reference - GitLab
   has no "webhook" pipeline source value. Clarify that there's no
   native mechanism to trigger pipelines from webhooks.

3. Correct FULLSEND_DISPATCH_TOKEN storage: project-level (not
   group-level) since webhook architecture means enrolled repos
   never see this token.

4. Fix GitLab labels API endpoint: PUT /projects/:id/merge_requests/:iid
   with labels/add_labels/remove_labels params (not dedicated /labels
   endpoint).

5. Clarify event filtering must use EVENT_PAYLOAD_B64, not
   CI_MERGE_REQUEST_EVENT_TYPE (unavailable in trigger API pipelines).

6. Expand pull_request_review.submitted mapping to acknowledge need
   for multiple GitLab event types (approvals + Notes).

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@ggallen ggallen force-pushed the docs/adr-026-gitlab-support-clean branch from 3a0b2fe to 9c54514 Compare May 6, 2026 15:25
@ggallen ggallen requested a review from ralphbean May 6, 2026 15:26

@ralphbean ralphbean 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.

Thorough ADR and implementation doc for GitLab support. The webhook-based dispatch architecture is well-motivated, the defense-in-depth security model is sound, and the forge-neutral interface evolution respects ADR-0005.

This has been through extensive review (25+ commits addressing feedback). One remaining inconsistency noted in existing thread (dispatch token scope in ADR line 148 still says group-level while impl doc correctly says project-level), otherwise LGTM.

+1'd existing comments on timing-safe comparison, ADR template section, and webhook translation language strength.

Change FULLSEND_DISPATCH_TOKEN from group-level to project-level
to match the implementation doc. The webhook-based architecture
stores this token at project-level in .fullsend, scoped to .fullsend
only—enrolled repos never see this token.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@ggallen

ggallen commented May 6, 2026

Copy link
Copy Markdown
Member Author

@ralphbean Fixed in 38c13f8 - changed FULLSEND_DISPATCH_TOKEN from group-level to project-level to match the implementation doc. The webhook-based architecture stores this token at project-level in .fullsend, scoped to .fullsend only—enrolled repos never see this token.

@ggallen ggallen added this pull request to the merge queue May 6, 2026
Merged via the queue into fullsend-ai:main with commit d3a1c88 May 6, 2026
9 checks passed
@ggallen ggallen deleted the docs/adr-026-gitlab-support-clean branch May 6, 2026 16:21
ggallen added a commit to ggallen/fullsend that referenced this pull request May 9, 2026
Addresses the open question from GitLab implementation (PR fullsend-ai#601) about how
agents execute on GitLab runners vs GitHub Actions.

Key decisions:
- Shared container image (ghcr.io/fullsend-ai/agent-sandbox) for both GitHub
  Actions and GitLab CI
- OpenShell as PID 1 with nested sandboxes for individual agents
- Docker and Kubernetes executors supported (shell executor excluded)
- Privileged container requirement for OpenShell network namespace manipulation
- Resource limits and timeout enforcement per platform
- Image signing with Sigstore for supply chain integrity

The ADR explores four options:
1. Shared container image, Docker-first (chosen)
2. Platform-specific images (rejected: maintenance burden, inconsistent security)
3. Kubernetes-native with CRDs (rejected: poor fit for ephemeral task execution)
4. Minimal sandbox + dynamic tools (rejected: violates zero-trust execution)

Implementation details moved to docs/problems/agent-execution-environment.md:
- Container image build pipeline and Dockerfile structure
- OpenShell gateway configuration and sandbox creation flow
- L7 policy and provider configuration examples
- Platform-specific considerations (GitHub Actions VMs, GitLab Docker/K8s executors)
- Host-side REST server lifecycle in containerized environments
- Image signing, verification, upgrade, and rollback procedures

Open questions documented:
- Rootless OpenShell support (user namespaces, eBPF alternatives)
- Image build and distribution strategy (public vs per-org registry)
- Builder services for Docker-in-Docker use cases (external Kaniko vs prohibit)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
ggallen added a commit to ggallen/fullsend that referenced this pull request May 9, 2026
Lint fixes:
- Remove gitlab-implementation from ADR frontmatter relates_to (doesn't exist yet)
- Update references to ADR-0028 and gitlab-implementation.md to note they are pending in PR fullsend-ai#601

Review findings (Medium):
- Add builder stage to Dockerfile example (was missing, would fail to build)
- Remove Docker socket mount from GitLab runner registration (security violation - bypasses sandbox isolation)
- Update all references to pending GitLab support work (PR fullsend-ai#601 instead of ADR-0028)

Review findings (Low):
- Remove placeholder OpenShell issue link, describe as upstream feature request instead
- Replace non-existent docker/run@v1 action with standard docker run command
- Add comment explaining docker login in GitLab job (for pulling additional images, not the base image)

All changes address findings from fullsend-ai-review comment-only review.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
ggallen added a commit to ggallen/fullsend that referenced this pull request May 9, 2026
Replace markdown link to non-existent ADR-0028 file with plain text noting
it is pending in PR fullsend-ai#601. This prevents broken link if this PR merges before
fullsend-ai#601.

Addresses review finding about broken link in References section.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
ggallen added a commit to ggallen/fullsend that referenced this pull request May 9, 2026
**Medium:**
- Add docs/plans/ section to README for proper categorization
- Update stale ADR-0028 reference in implementation doc (was "pending in PR fullsend-ai#601")

**Low:**
- Update ADR context to reflect GitLab support is decided (ADR-0028), not "under development"
- Update all references from "PR fullsend-ai#601" to ADR-0028 throughout

**Other:**
- Remove agent-execution-environment from ADR frontmatter relates_to (now in plans, not problems)
- Fix agent-infrastructure.md link path in implementation doc references

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants