Skip to content

docs: ADR 0043 — GitLab per-repo support via OIDC/WIF and webhook bridge#2042

Closed
ggallen wants to merge 1 commit into
fullsend-ai:mainfrom
ggallen:worktree-adr-0043-gitlab-per-repo
Closed

docs: ADR 0043 — GitLab per-repo support via OIDC/WIF and webhook bridge#2042
ggallen wants to merge 1 commit into
fullsend-ai:mainfrom
ggallen:worktree-adr-0043-gitlab-per-repo

Conversation

@ggallen

@ggallen ggallen commented Jun 8, 2026

Copy link
Copy Markdown
Member

Note

Supersedes #1816 (GitLab support via webhook bridge — per-org model). This PR redesigns GitLab support around per-repo-only installation based on review feedback from ifireball and waynesun09.

Summary

  • Adds ADR 0043 which supersedes ADR 0028 (GitLab Support Architecture)
  • Redesigns GitLab support around per-repo-only installation, using GitLab OIDC id_tokens with GCP Workload Identity Federation to retrieve a bot project access token from Secret Manager
  • Secretless from the project perspective — no credentials stored as CI/CD variables, no custom token mint
  • Bot project access token with api scope provides REST + GraphQL API access and bot identity
  • Webhook bridge Cloud Function translates GitLab events to Pipeline Trigger API calls (on-premise container deployment recommended for internal GitLab instances)
  • 7-layer security model with WIF attribute conditions, protected CI/CD variables, and explicit post-compromise abuse scenarios
  • Companion implementation plan (docs/plans/gitlab-per-repo-implementation.md) with 6-phase build-out, security-critical path analysis, and verification checklist

Test plan

  • make lint passes (ADR status, numbers, frontmatter, markdown links all clean)
  • Review ADR content for architectural soundness
  • Review implementation plan for completeness and correct dependency ordering
  • Verify security model covers the project's threat priority order (external injection > insider > drift > supply chain)

🤖 Generated with Claude Code

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Review · Started 7:34 PM UTC
Commit: d0ac11b · View workflow run →

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

Site preview

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

Commit: 03a63e55f22f097e3cd490e7a301cb1f0a4f2f9a

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Review · Started 7:37 PM UTC
Commit: d0ac11b · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 8, 2026

Copy link
Copy Markdown

Review

All five prior medium-severity findings have been resolved: the constructor call mismatch is fixed, --project flag added to gcloud secrets versions access, dispatch routing summary now references the complete event routing table, CLI install step 13 includes FULLSEND_GCP_PROJECT_ID, and the dispatch.env comment is corrected.

No medium or higher findings remain. The ADR is architecturally sound — it preserves the forge abstraction (ADR 0005), reuses WIF/Secret Manager infrastructure, maintains the per-repo config layering model (ADR 0033/0035), and follows the project's threat priority order with a thorough 7-layer security model.

Findings

Low

  • [consumer completeness] docs/plans/gitlab-per-repo-implementation.md:851 — Phase 1 method mapping table omits SetOrgSecretRepos and SetOrgVariableRepos from forge.Client. The GitLab LiveClient must implement these (returning ErrNotSupported) to satisfy the interface.

  • [internal consistency] docs/plans/gitlab-per-repo-implementation.md:863MergeChangeProposal appears twice in the Phase 1 method mapping table. Both rows map to MergeRequests.AcceptMergeRequest. Duplicate entry.

  • [internal consistency] docs/plans/gitlab-per-repo-implementation.md:816 — Phase 0 file table lists internal/cli/github.go for GetAppClientID update, but GetAppClientID is not called in that file. The callers are in internal/appsetup/appsetup.go (already listed). The row is a no-op.

  • [scope-alignment] docs/ADRs/0043-gitlab-per-repo-support.md:607 — The roadmap update says "architectural design is complete" but this is only true for the dispatch half. ADR 0043 explicitly defers the agent execution environment (runner executors, OpenShell integration, sandbox provisioning). Consider qualifying the roadmap claim.

  • [frontmatter-consistency] docs/ADRs/0028-gitlab-support.md:9 — New superseded_by frontmatter field not in the ADR template (0000-adr-template.md). This is the first superseded ADR — consider codifying the field in the template to establish the convention.

  • [infrastructure-dependency] docs/ADRs/0043-gitlab-per-repo-support.md:208 — The webhook bridge Cloud Function is new external infrastructure. The ADR covers deployment options, security properties, and bridge downtime risk, but operational concerns (monitoring/alerting, SLO expectations, runbooks) are deferred. Consider filing a follow-up issue for bridge operational readiness.

  • [authorization / privilege-scope] docs/ADRs/0043-gitlab-per-repo-support.md:170 — The bot PAT uses api scope with post-compromise abuse scenarios well-documented in Known Security Limitations. During implementation, verify that Developer-role project access tokens cannot modify protected branch settings or CI/CD variable protection flags via the API.

  • [replay-protection] docs/plans/gitlab-per-repo-implementation.md:1017 — Webhook replay protection uses an in-memory deduplication cache per Cloud Function instance. The document correctly notes the scaling limitation and suggests shared stores for strict exactly-once semantics.

  • [architectural-divergence] docs/ADRs/0043-gitlab-per-repo-support.md:586 — Per-repo-only for GitLab creates user-visible asymmetry with GitHub. The ADR documents this with clear technical justification across Alternatives, Consequences, and the Comparison table.

Previous run

Review

The two medium-severity findings from the prior review (CI_JOB_TOKEN reference in gitlab-implementation.md, stale dual-token references in the implementation plan) have both been fixed.

Three medium-severity documentation accuracy issues remain. None block the architectural decision.

Findings

Medium

  • [internal consistency] docs/plans/gitlab-per-repo-implementation.md:750 — The gitlab.New constructor call in runGitLabPerRepoInstall passes three arguments (token, "", opts.gitlabURL) but the constructor signature defined earlier in the same document accepts only two parameters (func New(token, baseURL string)). This will cause a compile error if implemented verbatim.
    Remediation: Change the call to gitlab.New(token, opts.gitlabURL) to match the two-parameter signature, or update the constructor signature to accept three parameters if the empty string serves a distinct purpose.

  • [architectural-divergence] docs/ADRs/0043-gitlab-per-repo-support.md:147 — Per-repo-only for GitLab creates user-visible asymmetry with GitHub (which supports both per-org and per-repo). The ADR documents this in Options and Consequences sections with clear technical justification (no GitHub Apps equivalent, cross-project credential complexity adds no capability over per-repo). Worth confirming with stakeholders that this asymmetry is the intended long-term posture.

  • [infrastructure-dependency] docs/ADRs/0043-gitlab-per-repo-support.md:145 — The webhook bridge Cloud Function is new external infrastructure. The ADR covers deployment options and security properties but does not address operational concerns: monitoring/alerting for bridge availability, multi-tenancy model (shared vs. per-org bridges), cost attribution, or ownership in distributed adoption.
    Remediation: Consider adding an Operational Model subsection or filing a follow-up issue to track bridge operational readiness.

Low

  • [error handling gap] docs/plans/gitlab-per-repo-implementation.md:626 — The gcloud secrets versions access command in stage pipeline YAML omits --project flag. Depending on credential configuration, the default project may not be set after WIF auth. Adding --project="${FULLSEND_GCP_PROJECT_ID}" would make the command explicit.

  • [internal consistency] docs/ADRs/0043-gitlab-per-repo-support.md:268 — Section 2 dispatch routing summary lists 8 routes but Section 4 (Event routing table) lists 12. The omitted routes (/fs-fix, /fs-retro, /fs-prioritize, needs-info triage, changes-requested marker) are correctly handled in the dispatch script. A note referencing Section 4 for the complete table would improve clarity.

  • [internal consistency] docs/ADRs/0043-gitlab-per-repo-support.md:422 — CLI install step 13 omits FULLSEND_GCP_PROJECT_ID (listed in Security Model Layer 2 as required protected variable) and includes FULLSEND_FORGE/FULLSEND_PER_REPO_INSTALL (not in Layer 2). The implementation plan correctly includes all variables.

  • [internal consistency] docs/plans/gitlab-per-repo-implementation.md:570 — Dispatch script comment says touch dispatch.env makes downstream rules see STAGE="", but an empty file sets no variable — STAGE will be undefined. Functionally harmless (stage rules fail either way) but the comment is inaccurate.

  • [missing-authorization] docs/ADRs/0043-gitlab-per-repo-support.md — This PR adds ~1550 lines without a linked GitHub issue. The PR body references superseding PR docs: add ADR 0043 for GitLab support via webhook bridge #1816, which provides provenance. ADRs can be self-authorizing design documents, but linking to an issue improves traceability.

  • [frontmatter-consistency] docs/ADRs/0028-gitlab-support.md:9 — New superseded_by frontmatter field not in the ADR template (0000-adr-template.md). First superseded ADR in the repo — establishes a new convention. Consider adding to the template.

Previous run (2)

Review

The three high-severity findings from the prior review (bot filter blocking changes-requested marker, Note Hook minimal payload missing fields, fix.yml reading wrong payload path) have all been addressed. The dispatch.yml now correctly checks the changes-requested marker before the bot filter, the Note Hook minimal payload includes all required fields, and fix.yml reads from .merge_request instead of .object_attributes.

Two medium-severity editorial accuracy issues remain.

Findings

Medium

  • [internal consistency] docs/problems/gitlab-implementation.md:3 — The supersession note states ADR-0043 provides "per-repo-only GitLab support via CI_JOB_TOKEN". This is factually incorrect — ADR 0043 explicitly rejects CI_JOB_TOKEN as insufficient (cannot authenticate GraphQL, cannot create MRs, no bot identity). The credential model uses a bot project access token retrieved via OIDC/WIF from GCP Secret Manager. The PR title itself says "via OIDC/WIF and webhook bridge."
    Remediation: Change "via CI_JOB_TOKEN" to "via OIDC/WIF and bot project access token".

  • [internal consistency] docs/plans/gitlab-per-repo-implementation.md:861 — The method mapping table states CreateChangeProposal "Uses mrClient", but the LiveClient struct (line 835) has only a single client *gitlab.Client field. The "What's net-new" section (line 949) also references "Dual-token routing (most methods → client, CreateChangeProposal → mrClient)". These are stale references from a dual-token design that was rejected — ADR 0043 line 452 explicitly states "No dual-token model is needed."
    Remediation: Remove the "Uses mrClient" note from the CreateChangeProposal row. Remove the "Dual-token routing" bullet from "What's net-new". All methods use the single client field.

Low

  • [internal consistency] docs/plans/gitlab-per-repo-implementation.md:682 — Phase 0 moves methods to GitHubExtensions but lacks a systematic caller inventory. Callers are mentioned in prose, and ErrNotSupported callers have a grep-based audit (line 760), but no equivalent audit for the GitHubExtensions migration.

  • [error handling gap] docs/plans/gitlab-per-repo-implementation.md:848GetOrgPlan returns "unknown", nil for GitLab. Zero callers currently. The plan's note to audit callers during implementation is adequate.

Previous run (3)

Review

Findings

High

  • [logic error] docs/plans/gitlab-per-repo-implementation.md:1188 — The dispatch.yml note fallthrough handler filters out bot-authored comments (IS_BOT=$(jq -r '.user.bot // false' ...)) before checking for the <!-- fullsend:changes-requested --> marker. However, the changes-requested marker is written by the review agent using FULLSEND_MR_TOKEN (a project access token), and project access tokens create bot users in GitLab. This means IS_BOT will be true for the review agent's changes-requested note, causing the entire marker-detection block (lines 1196-1207) to be skipped. The fix stage will never be triggered by the review agent's changes-requested note — only by a human manually posting a comment containing the marker.
    Remediation: Move the MR changes-requested marker check before the IS_BOT guard, or restructure the logic so the bot filter only applies to the issue needs-info triage path.

  • [internal consistency] docs/plans/gitlab-per-repo-implementation.md:1006 — The bridge's minimal payload extraction for Note Hook is defined as {object_attributes: {note, noteable_type, noteable_id}, issue: {iid}, merge_request: {iid}}. However, the dispatch.yml code that consumes this payload reads several fields that are not included in the minimal payload: .user.bot (for bot filtering), .issue.labels[]?.title (for needs-info detection), and .merge_request.source_project_id / .merge_request.target_project_id (for fork MR protection). These reads will return empty/null values, silently breaking bot filtering, needs-info detection, and fork MR protection.
    Remediation: Expand the minimal payload for Note Hook to include: user: {bot}, issue: {iid, labels}, merge_request: {iid, source_project_id, target_project_id}.

Medium

  • [internal consistency] docs/plans/gitlab-per-repo-implementation.md:1306 — The fix stage's fork MR protection reads .object_attributes.source_project_id and .object_attributes.target_project_id from the decoded payload. However, the fix stage is triggered by a Note Hook event (MR comment with changes-requested marker), not a Merge Request Hook. In the Note Hook payload, MR metadata lives under .merge_request, not .object_attributes. The dispatch.yml correctly reads .merge_request.source_project_id at line 1201, but fix.yml reads .object_attributes.source_project_id. Fork MR protection in fix.yml is silently defeated.
    Remediation: Change fix.yml to read .merge_request.source_project_id and .merge_request.target_project_id.

Low

  • [internal consistency] docs/plans/gitlab-per-repo-implementation.md:997 — The merge_request dispatch case does not filter fork MRs for the review stage. Review is read-only so this is not a security issue, but the ADR should explicitly document that review runs on fork MRs by design.

  • [trajectory-alignment] docs/ADRs/0043-gitlab-per-repo-support.md:149 — Per-repo-only for GitLab creates user-visible asymmetry with GitHub. Already documented in Options and Consequences, but an explicit callout would help users working across forges.

  • [trajectory-drift] docs/roadmap.md:77 — Roadmap says 'architecture is decided... but implementation is not yet prioritized' while GitLab remains in 'Later'. Not contradictory but could be clearer.

  • [error handling gap] docs/plans/gitlab-per-repo-implementation.md:848GetOrgPlan returns "unknown", nil for GitLab. Zero callers currently. The plan's note to audit callers during implementation is adequate.

  • [abstraction-boundary] docs/ADRs/0043-gitlab-per-repo-support.md:408 — Forge abstraction compliance claim understates CLI changes (GitLab-specific flags, token resolution). The abstraction boundary is preserved but the claim should acknowledge forge-specific CLI logic.

  • [implementation-readiness] docs/plans/gitlab-per-repo-implementation.md:682 — Phase 0 moves methods to GitHubExtensions but lacks a systematic caller inventory. Callers mentioned in prose but a grep-based audit would strengthen the plan.

  • [Cross-reference format inconsistency] docs/ADRs/0043-gitlab-per-repo-support.md:96 — Supersession reference embeds parenthetical status, inconsistent with other supersession references in the PR.

Previous run (4)

Review

Findings

Medium

  • [internal consistency] docs/plans/gitlab-per-repo-implementation.md:1182 — The dispatch.yml pseudocode's note case handler is missing the 'MR comment with changes-requested marker' routing specified in ADR 0043 Section 4 (Event Routing table). The ADR states that a Note Hook on an MR comment containing the <!-- fullsend:changes-requested --> marker from a same-project MR should trigger the fix stage. However, the dispatch.yml note) case only handles /fs-* slash commands and non-command issue comments with needs-info — it has no branch for detecting MR notes with the changes-requested marker. When implemented as written, the fix stage will never auto-trigger on review change requests; it will only trigger via explicit /fs-fix commands.
    Remediation: Add a branch in the note) case handler's *) fallback that checks if the note is on a merge request (noteable_type == 'MergeRequest'), the note body contains <!-- fullsend:changes-requested -->, and source_project_id == target_project_id. If all conditions match, set STAGE="fix".

Low

  • [internal consistency] docs/ADRs/0043-gitlab-per-repo-support.md:435 — The Security Model (Layer 1) states the ref value is 'a string literal in the bridge code — never derived from webhook payload fields.' However, the implementation plan's bridge pseudocode shows Ref: config.DefaultBranch where DefaultBranch is a per-project field read from GCP Secret Manager at runtime, not a string literal. The security property (never from the webhook payload) is preserved, but the ADR's claim of 'string literal' is technically inaccurate.
    Remediation: Update the Security Model Layer 1 description to accurately state that ref is read from a per-project config in Secret Manager (set at install time, never from the webhook payload).

  • [ADR cross-reference format] docs/ADRs/0043-gitlab-per-repo-support.md:97 — Inconsistent ADR cross-reference format. The new ADR uses 'Supersedes ADR 0028 (GitLab Support Architecture, deprecated).' but existing ADRs use different patterns (ADR 0042 omits parenthetical status; ADR 0041 uses bold). Only two prior examples exist, so no strong convention to violate.

  • [Cross-reference formatting] docs/ADRs/0031-reusable-workflows-for-action-installed-distribution.md:32 — The updated cross-reference uses nested parentheses: 'Multi-forge support (ADR 0043, supersedes ADR 0028)'. Mildly awkward; renders fine in Markdown since inner parens are link syntax.

  • [error handling gap] docs/plans/gitlab-per-repo-implementation.md:848GetOrgPlan returns "unknown", nil for GitLab. Zero callers currently exist, but returning a valid-looking string is fragile. The implementation plan already notes to audit callers and consider ErrNotSupported instead.

  • [edge case] docs/plans/gitlab-per-repo-implementation.md:1162 — Label-change detection uses grep -qxF "${LABEL}" where LABEL is user-controlled input pre-filtered to only ready-to-code or ready-for-review. Safe today but fragile if more labels are added without considering the downstream grep.

  • [operational-readiness] docs/ADRs/0043-gitlab-per-repo-support.md:220 — Bridge Cloud Function operational readiness (monitoring, SLOs, runbooks) not addressed in ADR or implementation plan. The Consequences section acknowledges the bridge must be 'deployed, monitored, and maintained' but does not specify how.

Info

  • [Overly Broad Token Scope] docs/ADRs/0043-gitlab-per-repo-support.md:178FULLSEND_MR_TOKEN uses api scope granting full project API access. ADR extensively documents compensating controls (90-day expiry, stage-limited export, protected variable) and migration path to fine-grained project access tokens when available.

  • [Replay Protection Gap] docs/plans/gitlab-per-repo-implementation.md:957 — In-memory TTL dedup cache is per Cloud Function instance. Plan acknowledges limitation and suggests shared store for strict exactly-once semantics.

  • [Pipeline-Internal Marker] docs/plans/gitlab-per-repo-implementation.md:1116WEBHOOK_VALIDATED can be set by anyone with a trigger token. Plan correctly documents this as a pipeline-internal marker, not a security boundary.

  • [CI_DEBUG_TRACE Best-Effort Guard] docs/ADRs/0043-gitlab-per-repo-support.md:460 — ADR honestly documents the runtime guard is best-effort and identifies the install-time check as the stronger control.

  • [Credential Delivery Improvement] docs/ADRs/0043-gitlab-per-repo-support.md:298FULLSEND_MR_TOKEN correctly delivered via printenv to temp file with chmod 600 and --mr-token-file.

  • [Security Model Completeness] docs/ADRs/0043-gitlab-per-repo-support.md:432 — The 7-layer security model is comprehensive and well-documented with threat scenarios and consequences of failure for each layer.

  • [missing-authorization] — This PR adds ~1485 lines without a linked GitHub issue. The PR body mentions superseding PR docs: add ADR 0043 for GitLab support via webhook bridge #1816, which provides provenance.

  • [Status section formatting] docs/ADRs/0028-gitlab-support.md:18 — Status section uses 'Superseded by ADR 0043' inline. This is the first superseded ADR in the repo, so no prior convention exists.

  • [Frontmatter metadata consistency] docs/ADRs/0028-gitlab-support.md:9 — New superseded_by frontmatter field. First superseded ADR in the repo — establishes the convention rather than violating one.

  • [Cross-reference formatting] docs/ADRs/0036-agent-execution-sandbox.md:45 — Uses 'ADR-0043' with hyphen, consistent with ADR-0036's own existing convention of using hyphens throughout.

Previous run (5)

Review

Findings

Medium

  • [logic error] docs/plans/gitlab-per-repo-implementation.md:1180 — In the dispatch.yml template, the bot-filtering logic uses break inside a case pattern handler to skip bot-authored comments. In bash, break exits for/while/until loops, not case statements — there is no enclosing loop here, so break produces a shell error. The dispatch.yml script does NOT use set -euo pipefail, so the error is non-fatal and execution continues past the guard. The bot filter is completely ineffective: bot-authored comments on issues with needs-info will still trigger the triage stage, creating an infinite loop (agent replies → triggers triage → agent replies → ...).
    Remediation: Replace break with STAGE=""; ;; or restructure the control flow — e.g., wrap the NOTEABLE_TYPE check in an else branch: if [ "${IS_BOT}" = "true" ]; then :; else ... fi.

Low

  • [internal consistency] docs/plans/gitlab-per-repo-implementation.md:1346 — The per-repo enforcement pseudocode passes args[0] (e.g., group/project) to forge.DetectForge(), but DetectForge (defined at line 728) expects a URL and calls url.Parse(). url.Parse("group/project") returns an empty hostname, so DetectForge will always error for non-URL arguments. The error is silently discarded (forgeType, _ = ...), making --forge gitlab effectively mandatory despite the plan stating auto-detection.
    Remediation: Update pseudocode to parse the git remote URL or add argument-format heuristics.

  • [scope-coherence] docs/ADRs/0043-gitlab-per-repo-support.md:220 — The webhook bridge Cloud Function is new GCP infrastructure not present in GitHub per-repo (ADR 0033). The ADR documents deployment options, compromise isolation, and failure modes, but operational readiness (runbooks, availability SLOs, cost analysis) is not yet addressed.
    Remediation: Consider filing a follow-up issue to track bridge operational readiness.

  • [edge case] docs/plans/gitlab-per-repo-implementation.md:1152 — The label-change detection uses grep -qE "^${LABEL}$" where LABEL is user-controlled. Currently safe because LABEL is pre-filtered to only ready-to-code or ready-for-review (neither contains regex metacharacters), but the pattern is fragile.
    Remediation: Use grep -qF (fixed-string match) instead of grep -qE.

  • [internal consistency] docs/plans/gitlab-per-repo-implementation.md:1108 — The dispatch.yml template script does not include set -euo pipefail, while stage templates like code.yml (line 1234) do. Without set -e, failures in validation commands won't abort the script. This compounds the break bug above — with set -e, the break error would actually abort the script (accidentally making the bot filter work by aborting entirely).
    Remediation: Add set -euo pipefail at the top of the dispatch.yml script block.

  • [error handling gap] docs/plans/gitlab-per-repo-implementation.md:848GetOrgPlan returns "unknown", nil for GitLab. Currently has zero callers in the codebase, so no immediate risk. The parenthetical guidance to audit callers during implementation is sound.

  • [Overly Broad Token Scope] docs/ADRs/0043-gitlab-per-repo-support.md:178FULLSEND_MR_TOKEN uses api scope granting full project API access. The ADR documents this as a known limitation with compensating controls (90-day expiry, file-descriptor delivery, protected variable, stage-limited usage) and a migration path. No narrower scope is available in GitLab today.

  • [cross-reference-completeness] docs/architecture.md:130 — The Agent Dispatch section remains GitHub-only (workflow_call, dispatch.yml, pull_request_target). ADR 0043 introduces the GitLab dispatch parallel (webhook bridge → Pipeline Trigger API) but architecture.md's dispatch section does not reference it.
    Remediation: Add a reference to ADR 0043 in the Agent Dispatch section of architecture.md.

  • [supersession-clarity] docs/problems/gitlab-implementation.md:3 — The diff adds a supersession note at the top but the opening reference still directs readers to ADR-0028 first. The rewritten line says "superseded architectural decision" which helps, but body references at line ~597 remain unqualified.

Info

  • [missing-authorization] — This PR adds ~1485 lines without a linked GitHub issue. The PR body mentions superseding PR docs: add ADR 0043 for GitLab support via webhook bridge #1816, which provides provenance.

  • [roadmap-alignment] docs/roadmap.md:78 — ADR 0043 status Accepted while roadmap places GitLab under Later. The diff updates roadmap text to say "architecture is decided... but implementation is not yet prioritized", which addresses the potential confusion.

  • [Replay Protection Gap] docs/plans/gitlab-per-repo-implementation.md:957 — In-memory TTL dedup cache is per Cloud Function instance. The plan explicitly acknowledges this and recommends shared stores for strict exactly-once semantics.

  • [Pipeline-Internal Marker] docs/plans/gitlab-per-repo-implementation.md:1116WEBHOOK_VALIDATED can be set by anyone with a trigger token. The plan correctly documents this as a pipeline-internal marker, not a security boundary.

  • [CI_DEBUG_TRACE Best-Effort Guard] docs/ADRs/0043-gitlab-per-repo-support.md:460 — ADR honestly documents the runtime guard is best-effort and identifies the install-time check as the stronger control.

  • [Credential Delivery Improvement] docs/ADRs/0043-gitlab-per-repo-support.md:298FULLSEND_MR_TOKEN correctly delivered via printenv to temp file with chmod 600 and --mr-token-file. Resolved from prior review.

  • [Security Model Completeness] docs/ADRs/0043-gitlab-per-repo-support.md:432 — The 8-layer security model is well-structured and comprehensive, with post-compromise abuse scenarios now explicitly enumerated. No gaps identified.

  • [stale reference] docs/problems/gitlab-implementation.md:597 — Body references to ADR-0028 survive. Supersession note at top provides adequate context for the per-org design document.

  • [documentation-trajectory] docs/plans/gitlab-per-repo-implementation.md — Largest plan document (929 lines). Operational content should migrate to docs/guides/ during implementation.

Previous run (6)

Review

Findings

Medium

  • [internal consistency] docs/ADRs/0043-gitlab-per-repo-support.md:259 — Section 3 (Pipeline architecture) describes MR event routing as opened/update but section 4 (Event routing table) and the dispatch.yml template both use open/update/reopen. Two discrepancies: (1) opened should be open — GitLab MR Hook uses open not opened as the action value, and (2) reopen is omitted from section 3 but present in both section 4 and the dispatch code.
    Remediation: Change section 3 text from opened/update to open/update/reopen to match section 4 and the dispatch template.

Low

  • [edge case] docs/plans/gitlab-per-repo-implementation.md:1166 — The dispatch.yml bot-filtering logic checks .object_attributes.author.type for Bot or Project, but GitLab's Note Hook webhook payload does not include an author.type field inside object_attributes — the author is identified by author_id (numeric). The jq path would return the default User, causing the bot filter to never match. Bot-authored comments would still trigger triage re-dispatch.
    Remediation: Verify actual GitLab Note Hook webhook payload structure. May need to check user.bot at the top level or use author_id comparison.

  • [edge case] docs/plans/gitlab-per-repo-implementation.md:1146 — Issue routing checks .labels[]?.title for ready-to-code/ready-for-review, but GitLab Issue Hook includes ALL current labels, not just newly added ones. Without checking changes.labels, dispatch will trigger the code stage any time an issue event fires if the issue already has ready-to-code — even for unrelated events like assignee changes. The bridge's minimal payload extraction already includes changes.labels, but the dispatch script ignores it.
    Remediation: Check changes.labels to determine if the relevant label was just added, rather than checking whether the label exists on the issue.

  • [internal consistency] docs/ADRs/0043-gitlab-per-repo-support.md:297 — Section 3 triage stage example uses --event-payload "${EVENT_PAYLOAD}" (passing decoded payload as CLI argument), while the code stage example and the full implementation plan templates use --event-payload-file "${EVENT_PAYLOAD_FILE}" (file-based). Inconsistent and potentially problematic for large payloads (shell argument length limits).
    Remediation: Update the triage stage example in section 3 to use --event-payload-file consistently.

  • [error handling gap] docs/plans/gitlab-per-repo-implementation.md:848 — The method mapping specifies GetOrgPlan returns "unknown", nil for GitLab. Callers may branch on specific plan names (e.g., checking plan == "free" to gate features). The implementation plan does not audit GetOrgPlan callers to verify "unknown" is safe.
    Remediation: Audit all callers of GetOrgPlan during implementation. Consider returning ErrNotSupported instead if callers already handle that sentinel.

  • [Overly Broad Token Scope] docs/ADRs/0043-gitlab-per-repo-support.md:178FULLSEND_MR_TOKEN uses api scope granting full project API access. The ADR documents this as a known limitation with compensating controls (90-day expiry, file-descriptor delivery, protected variable, stage-limited usage) and a migration path. No narrower scope is available in GitLab today.

  • [missing-authorization] — This PR adds ADR 0043 (541 lines), a companion implementation plan (919 lines), and updates 7 other files without a linked GitHub issue. The PR body mentions superseding PR docs: add ADR 0043 for GitLab support via webhook bridge #1816, which provides provenance, but non-trivial design decisions benefit from explicit issue-based authorization.

  • [roadmap-inconsistency] docs/roadmap.md:78 — ADR 0043 has status Accepted but the roadmap places GitLab support under Later (not Now or Next). These are not inherently contradictory — Accepted means the design decision is made, Later means implementation is not yet prioritized — but this nuance could confuse readers.
    Remediation: Consider adding a note in the roadmap that the architecture is decided (ADR accepted) but implementation is not yet prioritized.

Info

  • [section-structure] docs/ADRs/0043-gitlab-per-repo-support.md:120 — Options section uses Alternative 1-4 headers. Most ADRs in the codebase use Option 1-4, though the predecessor ADR 0028 and ADR 0033 also use Alternative — so this is consistent with those precedents.

  • [stale reference] docs/problems/gitlab-implementation.md:597 — Body references to ADR-0028 survive in the old document. The supersession note added at the top provides adequate context for readers.

  • [credential-leak-resolved] docs/ADRs/0043-gitlab-per-repo-support.md:298 — Prior finding about ADR Section 3 showing export FULLSEND_MR_TOKEN has been resolved. The code now correctly uses printenv to a temp file with chmod 600 and --mr-token-file.

  • [Replay Protection Gap] docs/plans/gitlab-per-repo-implementation.md:957 — In-memory TTL dedup cache is per Cloud Function instance, not shared across instances. The plan explicitly acknowledges this and recommends Firestore/Memorystore for strict exactly-once semantics. The resource_group concurrency control provides a second layer.

  • [WEBHOOK_VALIDATED boundary] docs/plans/gitlab-per-repo-implementation.md:1116WEBHOOK_VALIDATED can be set by anyone with a trigger token. The plan correctly documents this as a pipeline-internal marker, not a security boundary.

  • [CI_DEBUG_TRACE-acknowledged] docs/ADRs/0043-gitlab-per-repo-support.md:460 — ADR honestly documents the CI_DEBUG_TRACE runtime guard is best-effort and identifies the install-time check as the stronger control. Inherent GitLab platform constraint with no perfect defense.

Previous run (7)

Review

Findings

Medium

  • [internal consistency] docs/plans/gitlab-per-repo-implementation.md:1183 — The code.yml stage pipeline has two separate trap ... EXIT statements (one for FULLSEND_MR_TOKEN_FILE at line 1184 and one for EVENT_PAYLOAD_FILE at line 1190). In bash, a second trap ... EXIT replaces the first — so the FULLSEND_MR_TOKEN_FILE cleanup is silently discarded, leaving the MR creation PAT written to a temp file on disk after the job ends. Since this template will be copied verbatim during Phase 3 scaffold creation, the bug would propagate to production CI/CD pipelines.
    Remediation: Combine both temp file cleanups into a single trap: trap 'rm -f "${FULLSEND_MR_TOKEN_FILE}" "${EVENT_PAYLOAD_FILE}"' EXIT, or define the trap after both mktemp calls.

  • [Overly Broad Token Scope] docs/ADRs/0043-gitlab-per-repo-support.md:175FULLSEND_MR_TOKEN uses api scope granting full project API access. The ADR acknowledges this and provides compensating controls (90-day expiry, stage-limited export, protected variable) and a migration path. However, the Known Security Limitations section does not enumerate specific post-compromise abuse scenarios — a compromised code/fix stage could use the token to modify CI/CD variables (including removing the Protected flag from other secrets), alter webhooks, or change project settings.
    Remediation: Add specific post-compromise abuse scenarios to the Known Security Limitations section to inform threat modeling during implementation.

  • [missing-living-document-update] docs/architecture.md — ADR 0043 has status "Accepted" but docs/architecture.md was not updated. The Agent Identity Provider section (line 130) has an open question: "How will per-role identity work on GitLab and Forgejo, which lack GitHub's app manifest flow?" ADR 0043 partially answers this for GitLab (single CI_JOB_TOKEN + one PAT, no per-role tokens). The Agent Infrastructure section should also reference ADR 0043.
    Remediation: Update docs/architecture.md to annotate the open question at line 130 with a reference to ADR 0043's GitLab credential decision.

Low

  • [internal consistency] docs/ADRs/0043-gitlab-per-repo-support.md — The dispatch routing checks for needs-info label on non-command issue comments (triggering triage) but does not include auto-triage guards present in the GitHub dispatch: bot filtering, feature-label exclusion, and author-association checks. Without these, the triage agent's own comments could re-trigger triage.

  • [event routing gap] docs/ADRs/0043-gitlab-per-repo-support.md:296 — ADR event routing table says MR Hook opened/updated/ready triggers review, but dispatch.yml routes open|update|reopen. "ready" (draft MR marked ready for review) and "reopen" (closed MR reopened) are different GitLab MR actions. Both may be valid triggers, but the ADR and implementation plan should be consistent.

  • [missing event routing] docs/plans/gitlab-per-repo-implementation.md:1098 — The dispatch.yml issues case only checks for label changes (ready-to-code, ready-for-review). New issues and issue edits are not routed to any stage. On GitHub, issues.opened/edited triggers auto-triage. This may be intentional for GitLab (triage via /fs-triage commands instead), but should be documented explicitly.

  • [Environment Variable Credential Leak] docs/plans/gitlab-per-repo-implementation.md:1183 — The implementation plan's code.yml now correctly uses --mr-token-file with a temp file instead of exporting FULLSEND_MR_TOKEN as an environment variable (improvement over prior review). However, ADR Section 3 (Pipeline architecture) still shows export FULLSEND_MR_TOKEN="${FULLSEND_MR_TOKEN}", creating an inconsistency. A developer following the ADR snippet would re-introduce the env var exposure.

  • [replay protection gap] docs/ADRs/0043-gitlab-per-repo-support.md:436 — The in-memory TTL cache for webhook deduplication is per Cloud Function instance and not shared across instances under horizontal scaling. The implementation plan documents this limitation and suggests shared stores for strict exactly-once semantics. The resource_group concurrency control provides a second layer of deduplication.

  • [error handling gap] docs/plans/gitlab-per-repo-implementation.md:1138dispatch.yml exits 0 without writing dispatch.env when no stage matches. Downstream stage jobs use rules: - if: $STAGE == "..." which may not match, but the behavior depends on how GitLab evaluates dotenv-sourced variables in rules:if contexts.

  • [stale-documentation] docs/problems/gitlab-implementation.md — The PR adds a supersession header note but does not update body references to ADR-0028 at lines 39, 597, and 618. These still reference ADR-0028 Open Questions without noting supersession by ADR-0043. The header note provides adequate warning, but the body references could be updated for consistency.

  • [section-structure] docs/ADRs/0043-gitlab-per-repo-support.md:120 — Options section uses "Alternative 1:", "Alternative 2:" headers. The majority pattern across ADRs is "Option 1:", "Option 2:". Usage is mixed in the codebase (ADRs 0028 and 0033 also use "Alternative"), so this is contextually consistent but diverges from the majority.

Info

  • [ADR number collision risk] docs/ADRs/0043-gitlab-per-repo-support.md — Current highest ADR is 0042. ADR 0043 is the next sequential number. Check for concurrent PRs that also add ADR 0043 before merging.
Previous run (8)

Review

Findings

Medium

  • [architectural-contradiction] docs/ADRs/0043-gitlab-per-repo-support.md — ADR 0043 states it "supersedes" ADR 0028, but ADR 0028's status field currently shows "Deprecated" rather than "Superseded by ADR 0043". These are semantically different: deprecated means discouraged, superseded means replaced by a specific successor. The PR does not modify ADR 0028 to update its status.
    Remediation: Update ADR 0028's status from "Deprecated" to "Superseded" and add a superseded_by reference to ADR 0043.

Low

  • [missing-authorization] PR metadata — This PR introduces ~1420 lines of new documentation without a linked GitHub issue. The PR body mentions it supersedes docs: add ADR 0043 for GitLab support via webhook bridge #1816 (a prior PR), which provides provenance, but non-trivial changes benefit from explicit issue-based authorization.

  • [Weak Security Boundary] docs/plans/gitlab-per-repo-implementation.mdWEBHOOK_VALIDATED=true is a pipeline trigger variable anyone with a trigger token can set. The dispatch.yml template correctly documents this limitation in an inline comment, but the error message ("Pipeline not triggered by validated webhook") could be clarified to avoid implying it is a security gate rather than a routing marker.

  • [Secret Exposure via Debug Tracing] docs/ADRs/0043-gitlab-per-repo-support.md — The ADR's Layer 5 section accurately documents that the CI_DEBUG_TRACE runtime guard is best-effort and that variables may be logged before the guard fires. The install-time check is correctly identified as the stronger control. No additional remediation needed — the ADR is transparent about this limitation.

  • [Replay Protection Gap] docs/plans/gitlab-per-repo-implementation.md — The in-memory TTL cache for webhook deduplication won't be shared across Cloud Function instances. Consider documenting this limitation explicitly. The resource_group concurrency control provides a second layer of deduplication for most scenarios.

  • [Environment Variable Credential Leak] docs/plans/gitlab-per-repo-implementation.md — The code.yml template exports FULLSEND_MR_TOKEN as an environment variable visible to agent subprocesses. The plan's inline comment suggests using a file descriptor instead — consider elevating this to a requirement during implementation.

  • [scope-creep] docs/plans/gitlab-per-repo-implementation.md — The PR modifies docs/problems/gitlab-implementation.md with a note that it reflects the earlier per-org design and points to the new plan. This relationship is clarified in the diff.

  • [section-structure] docs/ADRs/0043-gitlab-per-repo-support.md — Options section uses "Alternative 1:", "Alternative 2:" headers. Some recent ADRs use "Option 1:", "Option 2:" pattern, though usage is mixed across the codebase.

Info

  • [ADR number collision risk] docs/ADRs/0043-gitlab-per-repo-support.md — Current highest ADR is 0042. ADR 0043 is the next sequential number. Check for concurrent PRs that also add ADR 0043 before merging.
Previous run (9)

Review

Findings

Medium

  • [architectural-contradiction] docs/ADRs/0043-gitlab-per-repo-support.md — ADR 0043 states it "supersedes" ADR 0028, but ADR 0028's status field currently shows "Deprecated" rather than "Superseded by ADR 0043". These are semantically different: deprecated means discouraged, superseded means replaced by a specific successor. The PR does not modify ADR 0028 to update its status.
    Remediation: Update ADR 0028's status from "Deprecated" to "Superseded" and add a superseded_by reference to ADR 0043.

Low

  • [missing-authorization] PR metadata — This PR introduces ~1420 lines of new documentation without a linked GitHub issue. The PR body mentions it supersedes docs: add ADR 0043 for GitLab support via webhook bridge #1816 (a prior PR), which provides provenance, but non-trivial changes benefit from explicit issue-based authorization.

  • [Weak Security Boundary] docs/plans/gitlab-per-repo-implementation.mdWEBHOOK_VALIDATED=true is a pipeline trigger variable anyone with a trigger token can set. The dispatch.yml template correctly documents this limitation in an inline comment, but the error message ("Pipeline not triggered by validated webhook") could be clarified to avoid implying it is a security gate rather than a routing marker.

  • [Secret Exposure via Debug Tracing] docs/ADRs/0043-gitlab-per-repo-support.md — The ADR's Layer 5 section accurately documents that the CI_DEBUG_TRACE runtime guard is best-effort and that variables may be logged before the guard fires. The install-time check is correctly identified as the stronger control. No additional remediation needed — the ADR is transparent about this limitation.

  • [Replay Protection Gap] docs/plans/gitlab-per-repo-implementation.md — The in-memory TTL cache for webhook deduplication won't be shared across Cloud Function instances. Consider documenting this limitation explicitly. The resource_group concurrency control provides a second layer of deduplication for most scenarios.

  • [Environment Variable Credential Leak] docs/plans/gitlab-per-repo-implementation.md — The code.yml template exports FULLSEND_MR_TOKEN as an environment variable visible to agent subprocesses. The plan's inline comment suggests using a file descriptor instead — consider elevating this to a requirement during implementation.

  • [scope-creep] docs/plans/gitlab-per-repo-implementation.md — The PR modifies docs/problems/gitlab-implementation.md with a note that it reflects the earlier per-org design and points to the new plan. This relationship is clarified in the diff.

  • [section-structure] docs/ADRs/0043-gitlab-per-repo-support.md — Options section uses "Alternative 1:", "Alternative 2:" headers. Some recent ADRs use "Option 1:", "Option 2:" pattern, though usage is mixed across the codebase.

Info

  • [ADR number collision risk] docs/ADRs/0043-gitlab-per-repo-support.md — Current highest ADR is 0042. ADR 0043 is the next sequential number. Check for concurrent PRs that also add ADR 0043 before merging.
Previous run (10)

Review

Findings

Medium

  • [stale-adr-status] docs/ADRs/0028-gitlab-support.md:3 — ADR 0028 is being superseded by ADR 0043 but other documentation files still reference ADR 0028 without noting its superseded status. Affected files: docs/ADRs/0031-reusable-workflows-for-action-installed-distribution.md, docs/ADRs/0036-agent-execution-sandbox.md, docs/plans/agent-execution-environment.md, docs/roadmap.md, and docs/problems/gitlab-implementation.md.
    Remediation: Update references in these files to note ADR 0028 has been superseded by ADR 0043.

  • [inconsistent event routing — label mismatch] docs/plans/gitlab-per-repo-implementation.md:449 — The dispatch.yml template's grep pattern uses ready-to-review but the ADR's event routing table (section 4) uses ready-for-review. Also, ready-to-merge appears in the grep pattern but has no corresponding stage mapping in the ADR or dispatch case statement.
    Remediation: Reconcile the dispatch grep pattern with the ADR event routing table. Change ready-to-review to ready-for-review and remove or define a stage mapping for ready-to-merge.

  • [phantom interface method] docs/plans/gitlab-per-repo-implementation.md:160 — Phase 1 method mapping table lists AddLabels and RemoveLabel as forge.Client methods to implement, but these methods do not exist on the current forge.Client interface and are not proposed as additions in Phase 0.
    Remediation: Either add AddLabels/RemoveLabel to the Phase 0 interface preparation, or remove them from the Phase 1 mapping table.

  • [incomplete ErrNotSupported consumer coverage] docs/plans/gitlab-per-repo-implementation.md:50 — GitLab returns ErrNotSupported for several methods (DispatchWorkflow, org-level secrets/variables, etc.) but no existing callers handle this sentinel. Callers would treat it as a hard failure when used with a GitLab forge client.
    Remediation: Audit callers of methods that may return ErrNotSupported and specify how each should handle it (skip, log warning, or error).

  • [webhook replay / missing idempotency] docs/ADRs/0043-gitlab-per-repo-support.md:121 — The bridge webhook validation uses only a static webhook secret token with no replay protection (no nonce, no timestamp validation, no delivery ID deduplication). An intercepted valid webhook payload could be replayed to trigger redundant agent pipelines.
    Remediation: Add replay protection to the bridge: validate X-Gitlab-Delivery header for deduplication and/or reject events older than a threshold.

Low

  • [missing-authorization] — This PR adds 1354 lines across two new files with no linked issue. ADRs can be self-authorizing design documents, but linking to an issue provides traceability.

  • [incomplete method mapping] docs/plans/gitlab-per-repo-implementation.md:170 — Phase 1 method mapping table omits several forge.Client methods that need GitLab implementations: ListWorkflowRuns, GetWorkflowRun, GetWorkflowRunLogs, GetWorkflowRunAnnotations, CreateFileOnBranch, GetPullRequestHeadSHA, ListPullRequestFiles, ListPullRequestFileDiffs, and others.

  • [CI_DEBUG_TRACE guard bypass] docs/plans/gitlab-per-repo-implementation.md:939 — The runtime CI_DEBUG_TRACE guard is best-effort: GitLab Runner begins trace-level logging before the script runs, so variable values may already be logged before the guard fires exit 1. The ADR acknowledges this limitation but the guard is presented as Layer 5 defense without clarifying it is not fully reliable.

  • [interface breaking change — missing files] docs/ADRs/0043-gitlab-per-repo-support.md:325 — Phase 0 file list omits internal/appsetup/appsetup.go and internal/cli/github.go which call ListOrgInstallations/GetAppClientID and need type-assertion updates.

  • [WEBHOOK_VALIDATED trust boundary] docs/plans/gitlab-per-repo-implementation.md:947WEBHOOK_VALIDATED can be set by anyone with a pipeline trigger token, making it tautological as a security check. It is a pipeline-internal marker, not an independent security boundary.

  • [dependency graph inconsistency] docs/plans/gitlab-per-repo-implementation.md:24 — ADR text says "Phases 1, 2, and 3 can proceed in parallel after Phase 0" but the ASCII diagram shows Phase 3 with no dependency on Phase 0. Phase 3 creates YAML templates (no Go imports), so the diagram may be more accurate than the text.

  • [overly-broad credential scope] docs/ADRs/0043-gitlab-per-repo-support.md:107 — The ADR acknowledges FULLSEND_MR_TOKEN's broad api scope and provides a migration path, but additional compensating controls (shorter PAT expiry, environment isolation) would strengthen the design.

  • [adr-number-collision] docs/ADRs/0043-gitlab-per-repo-support.md — Before merging, check for ADR number collisions with concurrent PRs on main.

Info

  • [command injection via event payload] docs/plans/gitlab-per-repo-implementation.md:970 — Base64-decoded EVENT_PAYLOAD piped through jq and used in shell case statements. Currently safe but fragile; consider writing decoded payload to a temp file during implementation.

  • [credential exposure in FULLSEND_MR_TOKEN export] docs/plans/gitlab-per-repo-implementation.md:1043FULLSEND_MR_TOKEN is exported into the environment visible to agent tool subprocesses. Agent harness isolation should prevent tool-level access to this token.

  • [bridge Secret Manager key derivation] docs/plans/gitlab-per-repo-implementation.md:765 — Project configs keyed by sha256(project_path). Design rationale (avoiding special characters in Secret Manager key names) should be documented.

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Jun 8, 2026
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:37 PM UTC · Completed 7:50 PM UTC
Commit: d0ac11b · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 8, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:59 PM UTC · Completed 8:12 PM UTC
Commit: d0ac11b · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jun 8, 2026
@ggallen ggallen force-pushed the worktree-adr-0043-gitlab-per-repo branch from d2c51d8 to 755fac5 Compare June 8, 2026 20:20
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 8, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:21 PM UTC · Completed 8:35 PM UTC
Commit: d0ac11b · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jun 8, 2026
@ggallen ggallen force-pushed the worktree-adr-0043-gitlab-per-repo branch from 755fac5 to 0147419 Compare June 8, 2026 20:39
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Review · Started 8:41 PM UTC
Commit: d0ac11b · View workflow run →

Comment thread docs/ADRs/0043-gitlab-per-repo-support.md Outdated
Comment thread docs/ADRs/0043-gitlab-per-repo-support.md Outdated
Comment thread docs/ADRs/0043-gitlab-per-repo-support.md Outdated
Comment thread docs/ADRs/0043-gitlab-per-repo-support.md
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Review · Started 8:54 PM UTC
Commit: d0ac11b · View workflow run →

@ggallen ggallen force-pushed the worktree-adr-0043-gitlab-per-repo branch from 5401191 to f298d1b Compare June 8, 2026 20:55
@ggallen

ggallen commented Jun 9, 2026

Copy link
Copy Markdown
Member Author

Alternative not evaluated: merge_request_event + include: project: ref: main

@waynesun09 (#2042 (comment))

Good catch — this is a valid alternative that wasn't evaluated. I've added it as an open question in the ADR under "Native merge_request_event dispatch via include: project:".

The short version: acknowledged as a future optimization, deferred for now. Reasons:

  1. Bridge is still required for issue/comment events (no native CI trigger), so it can't be eliminated — only reduced in scope for MR events.
  2. Two dispatch paths (native for MR, bridge for issue/comment) increases operational complexity vs. the current single path through the bridge for all events.
  3. include: project: visibility caveat — GitLab docs note that nested includes execute as a public user, so the templates project visibility requirements need investigation.
  4. Can be layered on later without changing the credential model or security architecture.

Both merge_request_event and include: project: ref: main are Free tier — no Premium requirement.

@ggallen ggallen changed the title docs: ADR 0043 — GitLab per-repo support via CI_JOB_TOKEN and webhook bridge docs: ADR 0043 — GitLab per-repo support via OIDC/WIF and webhook bridge Jun 9, 2026
@ggallen ggallen force-pushed the worktree-adr-0043-gitlab-per-repo branch from b0989d0 to b7bfe61 Compare June 9, 2026 11:55
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 9, 2026

Copy link
Copy Markdown

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

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 9, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 3:16 PM UTC · Completed 3:30 PM UTC
Commit: ba204cb · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jun 9, 2026
@waynesun09

Copy link
Copy Markdown
Member

Event routing model: scope the bridge, document what's native

Follow-up to my earlier comments on OIDC/WIF credentials (adopted) and merge_request_event + include: project: (deferred as open question).

Now that the credential model is settled (OIDC/WIF, no CI_JOB_TOKEN/FULLSEND_MR_TOKEN), the ADR should be explicit about which events go through the bridge and which don't. GitLab's CI_PIPELINE_SOURCE has no values for issue, note, or review-verdict events — only the MR lifecycle and scheduled pipelines have native CI triggers.

Proposed event routing table

Event type Dispatch path Agents GitLab mechanism
MR open/update/ready/close Native Review, Retro merge_request_event pipeline source
Fix commit → re-review Native Review Push from fix agent triggers merge_request_event
Review verdict → fix Bridge (webhook callback) Fix No native CI trigger for MR review/approval events
Issue open/edit/label Bridge (webhook) Triage, Code No CI_PIPELINE_SOURCE for issue events
Issue/MR comment (/fs-*) Bridge (webhook) All slash commands No CI_PIPELINE_SOURCE for note events
Scheduled rescore Native Prioritize schedule pipeline source

Review→fix loop requires no separate mechanism

The review→fix iteration cycle falls out naturally from the bridge + native MR events:

  1. Review pipeline runs via native merge_request_event, requests changes
  2. Bridge dispatches fix agent with ITERATION=1
  3. Fix agent commits and pushes → push triggers merge_request_event → review runs again natively
  4. If review approves: done. If changes requested again: bridge dispatches fix with ITERATION=2
  5. Bridge enforces iteration cap (reject if ITERATION >= MAX) — same role as fullsend's current fullsend-no-fix label + iteration cap on GitHub

No recursive pipelines, no parent-child nesting tricks. The bridge is the iteration controller, native MR events handle re-review.

The ask

The ADR currently presents the bridge as the dispatch path for all events. With native merge_request_event handling the MR lifecycle (review, retro, fix→re-review), the bridge's actual scope is narrower: issue events, note events, and review-verdict callbacks only. Documenting this event routing table explicitly would clarify the bridge's role and make the security analysis more precise — the high-frequency, security-sensitive MR path flows through GitLab's native pipeline source, not through the bridge.

… bridge

Adds ADR 0043 and a companion implementation plan for GitLab per-repo
installation mode. Uses GitLab OIDC id_tokens with GCP Workload Identity
Federation to retrieve a bot project access token from Secret Manager —
no credentials stored as CI/CD variables, no custom token mint.

Supersedes ADR 0028 (per-org model).

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

fullsend-ai-review Bot commented Jun 9, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 4:55 PM UTC · Completed 5:11 PM UTC
Commit: ba204cb · View workflow run →

7. Verify review pipeline fires on MR open
8. Run `fullsend admin uninstall group/project --forge gitlab`
9. Verify cleanup (webhook removed, project access token revoked, variables deleted)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] consumer completeness

Phase 1 method mapping table omits SetOrgSecretRepos and SetOrgVariableRepos from forge.Client. GitLab LiveClient must implement these (returning ErrNotSupported) to satisfy the interface.

Suggested fix: Add rows for SetOrgSecretRepos and SetOrgVariableRepos returning ErrNotSupported.

- `TriggerPipeline` — record call with variables

## Security-Critical Code Paths

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] internal consistency

MergeChangeProposal appears twice in Phase 1 method mapping table. Both map to MergeRequests.AcceptMergeRequest. Duplicate entry.


### Integration wiring

- `fullsend run --forge gitlab` constructs a GitLab forge client with the bot PAT from `FULLSEND_FORGE_TOKEN` environment variable (retrieved from Secret Manager via OIDC/WIF in the pipeline script)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] internal consistency

Phase 0 file table lists internal/cli/github.go for GetAppClientID update, but GetAppClientID is not called in that file. The callers are in internal/appsetup/appsetup.go (already listed). The row is a no-op.

**Stage pipelines** (`.gitlab/ci/triage.yml`, `code.yml`, etc.):
```yaml
# fullsend-stage: triage

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] infrastructure-dependency

Webhook bridge operational concerns (monitoring, SLO, runbooks) not addressed. ADR covers deployment and security but defers ops. Consider filing a follow-up issue.

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed requires-manual-review Review requires human judgment labels Jun 9, 2026
**Network connectivity**: The bridge must be able to reach the GitLab instance's Pipeline Trigger API endpoint. For gitlab.com (public API), a standard Cloud Function works. For self-hosted GitLab behind a VPN or firewall, the bridge should be deployed where it can already reach GitLab — not the other way around. Setting up VPN peering from GCP to a corporate network solely to solve a CI trigger limitation is disproportionate infrastructure for the problem.

Deployment options for internal GitLab instances:
- **On-premise deployment (preferred)**: Deploy the bridge as a standalone container on infrastructure inside the corporate network (OpenShift, Kubernetes, or any container host). The bridge is a self-contained Go binary with no GCP runtime dependency — it only needs outbound HTTPS to the GitLab API. This is the expected default for internal instances like gitlab.cee.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I understand this same binary is the one that will get deployed as a GCP cloud function. If that is the case it will be great, so we don't have different binaries for different locations. Just a binary, you choose where to run it.

ggallen added a commit that referenced this pull request Jun 15, 2026
Replace the webhook bridge from PR #2042's GitLab design with cron-based
polling. Scheduled pipelines query the GitLab API for new issues, comments,
and label changes. MR events continue via native CI triggers. Eliminates
the external Cloud Function, webhook secrets, and trigger tokens.

Includes companion implementation plan with 6-phase build-out.

Signed-off-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Greg Allen <gallen@redhat.com>
ggallen added a commit that referenced this pull request Jun 15, 2026
Remove all references to PR #2042 and ADR 0043. Present cron-polling
event dispatch as the original GitLab support design. Webhooks appear
only as a considered-and-rejected alternative with clear rationale.

Signed-off-by: Claude <noreply@anthropic.com>
Signed-off-by: Greg Allen <gallen@redhat.com>
@ggallen

ggallen commented Jul 1, 2026

Copy link
Copy Markdown
Member Author

Superseded by #2853.

@ggallen ggallen closed this Jul 1, 2026
@ggallen ggallen deleted the worktree-adr-0043-gitlab-per-repo branch July 1, 2026 01:18
@fullsend-ai-retro

fullsend-ai-retro Bot commented Jul 1, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 1:22 AM UTC · Completed 1:28 AM UTC
Commit: 03a63e5 · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #2042 — ADR 0043 GitLab per-repo support

Outcome: Closed without merge, superseded by #2853.

Timeline

  1. June 8: ggallen opens PR with ADR 0043 (GitLab per-repo support via OIDC/WIF and webhook bridge), co-authored with Claude.
  2. June 8: ralphbean provides 5 inline comments (PAT terminology, WIF usage for bridge, VPN connectivity, downstream pipelines).
  3. June 8–9: waynesun09 proposes two fundamental changes: (a) merge_request_event + include: project: to eliminate the bridge for MR events, and (b) GitLab OIDC id_tokens → GCP WIF → bot PAT from Secret Manager instead of CI_JOB_TOKEN.
  4. June 9: ggallen adopts the OIDC/WIF credential model, removes CI_JOB_TOKEN entirely.
  5. June 9: Review bot requests changes (medium findings), then approves after fixes (only low findings remain). The bot ran 10 review iterations total, with findings ranging from low to high severity across different revisions.
  6. July 1: PR closed, superseded by docs: move experiments from Internals to Contributing section #2853.

Assessment

The workflow worked as designed. Human reviewers provided the high-value architectural feedback (credential model redesign, alternative dispatch mechanisms) that the review bot cannot provide. The review bot contributed legitimate mechanical findings (duplicate table entries, missing interface methods, internal consistency issues in the implementation plan — including two HIGH-severity findings in intermediate revisions). The bot approved the final revision appropriately; the eventual supersession was driven by architectural decisions from human domain experts, not by review gaps.

Existing coverage

All improvement areas I investigated are already covered by open issues:

  • Docs/ADR review quality: #1480, #1659, #1772, #2678
  • Architectural review capabilities: #1906, #2812
  • Retro on superseded PRs: #2176 — this retro itself is an example of wasted work on a closed-without-merge PR with an immediate successor

No new proposals warranted.

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

Labels

ready-for-merge All reviewers approved — ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants