Skip to content

docs: add ADR 0043 for GitLab support via webhook bridge#1816

Open
ggallen wants to merge 1 commit into
fullsend-ai:mainfrom
ggallen:docs/adr-0043-gitlab-support
Open

docs: add ADR 0043 for GitLab support via webhook bridge#1816
ggallen wants to merge 1 commit into
fullsend-ai:mainfrom
ggallen:docs/adr-0043-gitlab-support

Conversation

@ggallen
Copy link
Copy Markdown
Contributor

@ggallen ggallen commented Jun 2, 2026

Add ADR 0043 documenting the webhook bridge architecture for GitLab support, replacing the broader ADR 0028 approach with a focused, incrementally deployable design.

Key decisions:

  • Webhook bridge Cloud Function translates GitLab webhooks to pipeline triggers with hardcoded ref=main for security
  • OIDC via Workload Identity Federation for bridge-to-mint auth
  • Project Access Tokens for per-role GitLab credentials
  • Defense-in-depth: protected CI/CD variables, per-project webhook secrets, payload validation

Also adds companion implementation document (docs/problems/gitlab-support.md) with phased rollout plan and updates ADR 0028 status to reference its successor.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 2, 2026

Site preview

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

Commit: 83c40100b5abaf68d8638f9d1c61cc3016b333dc

@fullsend-ai-review
Copy link
Copy Markdown

fullsend-ai-review Bot commented Jun 2, 2026

Review

No findings.

Previous run

Review

Findings

Low

  • [stale-forward-reference] docs/ADRs/0007-per-role-github-apps.md:39 — ADR 0007 states "GitLab and Forgejo will need equivalent per-role identity mechanisms when their forge implementations are built." ADR 0043 (added in this PR) now defines the GitLab mechanism: Project Access Tokens with per-role permissions (triage → Reporter, code → Developer, review → Developer, fix → Developer, fullsend → Maintainer). The forward-looking statement is now partially stale — GitLab's mechanism is decided, only Forgejo remains open.
    Remediation: Update to note that GitLab's per-role identity is defined in ADR 0043 (Project Access Tokens), while Forgejo remains an open question.

Info

  • [architectural-coherence] docs/ADRs/0043-gitlab-support-via-webhook-bridge.md — Prior finding resolved: Alternative 3 rejection now explicitly states "(storage mechanism, not PATs themselves)" and explains that the rejection targets CI/CD variable storage, not PATs. The ambiguity from the prior review is addressed.

  • [architectural-coherence] docs/ADRs/0043-gitlab-support-via-webhook-bridge.md — Prior finding resolved: Alternative 4 now has explicit decision criteria ("will be prototyped if two or more organizations report that webhook bridge deployment is blocked by network constraints") and a clear drop criterion ("If no such reports arise during initial adopter rollout, this alternative will be dropped"). The ambiguity from the prior review is addressed.

  • [edge-case] docs/problems/gitlab-support.md — Timing side-channel inconsistency between bridge Go code (crypto/subtle.ConstantTimeCompare) and pipeline bash validation (openssl dgst with !=) remains cosmetic — CI pipeline timing variance dwarfs comparison timing differences.

Previous run (2)

Review

Findings

Medium

  • [error-handling] internal/cli/run.go:141 — The ResolveOpts passed to resolve.ResolveHarness omits the TraceID field. The ResolveOpts struct has a TraceID field that is propagated to audit log entries (verified at resolve.go:117), and the test suite at resolve_test.go:270 validates it. Omitting it means audit log entries for remote resource fetches will lack trace correlation, making incident investigation harder.
    Remediation: Pass a trace ID (e.g., from FULLSEND_TRACE_ID env var or a generated UUID) to ResolveOpts.TraceID.

Low

  • [scope-coherence] internal/cli/run.go — This PR bundles Go code changes (URL resolution wiring, --offline flag, HasURLReferences, ValidateFilesExist defense-in-depth), scaffold changes (post-triage deferred label, model pin updates, review agent/skill definition updates), and a Phase 2 implementation plan (docs/plans/universal-harness-access-phase2.md) alongside the primary ADR 0043 documentation. These are independent concerns that could be separate PRs for clearer review history.

  • [architectural-coherence] docs/ADRs/0043-gitlab-support-via-webhook-bridge.md — Alternative 3 (PATs stored as CI/CD variables) is rejected for "diverging from the token mint model," but the Decision section adopts PATs stored in Secret Manager. The rejection rationale could confuse readers into thinking PATs themselves were rejected, rather than the storage mechanism. Clarifying that the rejection is about storage/distribution (CI/CD variables vs. Secret Manager + mint) would reduce ambiguity.

  • [architectural-coherence] docs/ADRs/0043-gitlab-support-via-webhook-bridge.md — Alternative 4 (pull-based model) is deferred with specific criteria ("Recommended for: multiple self-hosted GitLab instances behind VPNs") but the open-ended "may be dropped entirely" phrasing leaves implementation intent ambiguous. Adding explicit decision criteria (e.g., "will be revisited if N+ organizations report webhook bridge deployment blocked by network constraints") would clarify commitment level.

  • [comment-placement] internal/scaffold/fullsend-repo/scripts/post-triage.sh:93 — The explanatory comment for the deferred label mechanism (referencing Race condition: multiple labels applied simultaneously can prevent code agent from running #1752) is separated from the DEFERRED_LABEL="" declaration by the is_control_label function. Moving the comment immediately before the declaration would follow the codebase pattern of co-locating variable documentation with its declaration.

Info

  • [edge-case] docs/problems/gitlab-support.md — Prior finding resolved: MinimalPayload now includes a Changes field with MinimalChanges and MinimalLabelChanges types, so determine-stage can correctly diff labels via .changes.labels.current and .changes.labels.previous.

  • [edge-case] docs/problems/gitlab-support.md — Prior finding resolved: getAudience now iterates the full aud array checking for the expected audience rather than returning only the first element.

  • [edge-case] docs/problems/gitlab-support.md — Timing side-channel inconsistency between bridge Go code (crypto/subtle.ConstantTimeCompare) and pipeline bash validation (openssl dgst with !=) remains cosmetic — CI pipeline timing variance dwarfs comparison timing differences.

Previous run (3)

Review

Findings

Medium

  • [logic-error] docs/problems/gitlab-support.md — The MinimalPayload struct in the bridge function (Phase 2) does not include a changes field — it has Issue, MergeRequest, Note, Action, and Labels. However, the determine-stage script (Phase 4) relies on .changes.labels.current and .changes.labels.previous to diff newly added labels when the action is labeled. Since the bridge strips the changes field during minimal payload extraction and base64-encodes only the MinimalPayload, the jq expression (.changes.labels.current // []) will always resolve to [], producing an empty label diff. The label-driven dispatch paths (ready-to-code → code stage, ready-for-review → review stage) would silently fail if implemented as designed.
    Remediation: Add a Changes field to MinimalPayload (e.g., a nested struct capturing changes.labels.current and changes.labels.previous) so the bridge preserves the label change data that determine-stage needs. Alternatively, extract the triggering label in the bridge and pass it as a dedicated pipeline variable.

Low

  • [edge-case] docs/problems/gitlab-support.md — The getAudience function handles []interface{} but returns only the first element (aud[0]). Per RFC 7519 §4.1.3, when the aud claim is an array, the relying party should check whether its identifier is among the values, not just extract the first one. If a GitLab OIDC token has multiple audiences (e.g., ["other-service", "fullsend-mint"]), the mint would extract the wrong audience and reject a valid token.
    Remediation: Iterate over all elements in the aud array and check if the expected audience (fullsend-mint) is present, rather than returning only the first element.

  • [scope-coherence] docs/ADRs/0043-gitlab-support-via-webhook-bridge.md — Alternative 3 (store PATs as CI/CD variables, bypassing the mint) is explicitly rejected in the Decision section for diverging from ADR 0029's mint model. However, the Credential model section (line 208) and the Open Questions section both re-open this as a "viable alternative." This creates architectural ambiguity — readers cannot tell whether bypassing the mint is rejected or conditionally supported.
    Remediation: Clarify whether mint-bypass for GitLab PATs is a rejected alternative or a supported design fork. If it remains viable for specific deployment profiles, mark it as "Deferred" rather than "Rejected" in the Options section and explain the criteria for choosing it.

  • [architectural-coherence] docs/ADRs/0043-gitlab-support-via-webhook-bridge.md — Alternative 4 (pull-based scheduled pipelines) is marked "Deferred for further evaluation" with a note that it "warrants a proof-of-concept alongside the webhook bridge." The Decision section does not clarify whether this is a parallel track or a deferred fallback. ADRs record decisions; an unresolved parallel track creates implementation ambiguity.
    Remediation: Move Alternative 4 to an Open Questions section if it is truly deferred pending webhook bridge experience, or add decision criteria (e.g., "Recommended for: multiple self-hosted instances behind VPNs; not recommended for: GitLab.com or single-instance deployments").

  • [architectural-coherence] docs/ADRs/0043-gitlab-support-via-webhook-bridge.md — The PAT limitation section (line 210) acknowledges that PATs cannot be scoped down at mint time, calling it "a security regression compared to GitHub's model." However, the Risks section does not include PAT compromise blast radius. Unlike GitHub installation tokens (1-hour TTL, repo-scoped), a compromised GitLab PAT grants persistent role-level project access until rotation (up to 1 year).
    Remediation: Add a risk entry: "PAT compromise blast radius: compromised PATs grant persistent role-level access until rotation. Mitigated by 1-year max expiry, rotation automation, and Secret Manager storage — but the window is larger than GitHub's 1-hour installation token TTL."

Info

  • [edge-case] docs/problems/gitlab-support.md — The pipeline-side webhook token validation uses openssl dgst hash comparison with bash != (not constant-time), while the bridge-side Go code uses crypto/subtle.ConstantTimeCompare. In a CI pipeline context, timing side-channels are not practically exploitable (seconds of variance from container startup and package installation dwarf nanosecond comparison differences), and the bridge already performs constant-time validation before triggering the pipeline. This is defense-in-depth, and the inconsistency is cosmetic.

  • [cross-reference-style] docs/ADRs/0043-gitlab-support-via-webhook-bridge.md — The supersession note reads "Supersedes ADR 0028 (Deprecated)." The "(Deprecated)" label is redundant since ADR 0028's status is tracked in its own frontmatter and Status section.

Previous run (4)

Review

Findings

Medium

  • [logic-error] docs/problems/gitlab-support.md — The validate-webhook job in the dispatch pipeline references ${WEBHOOK_TOKEN} for hash comparison (the received webhook token), but this variable is never passed to the pipeline. The bridge function (Phase 2) sends only EVENT_TYPE, SOURCE_PROJECT, and EVENT_PAYLOAD_B64 as pipeline variables via the trigger API. The WEBHOOK_TOKEN_<hash> CI/CD variables are the expected tokens, not the received token. Since WEBHOOK_TOKEN is undefined/empty in the pipeline context, the hash comparison will always fail, causing every legitimate dispatch to be rejected.
    Remediation: Either (a) add WEBHOOK_TOKEN to the bridge's trigger API call as a pipeline variable (variables[WEBHOOK_TOKEN]={token}), or (b) remove the in-pipeline token re-validation entirely since the bridge already validates the token via constant-time comparison before triggering the pipeline. Option (b) is cleaner — the bridge is the trust boundary, and passing the raw webhook token through a pipeline variable widens its exposure.

  • [edge-case] docs/problems/gitlab-support.md — When determine-stage finds no matching stage, it exits 0 without creating stage.env. The job declares artifacts: reports: dotenv: stage.env. In GitLab CI 17.0+, a missing dotenv artifact file causes a warning but may not fail the job — however, this behavior has changed across GitLab versions and is not guaranteed to remain stable.
    Remediation: Always write the dotenv file. When no stage matches, write an empty file (touch stage.env) or write a sentinel value (STAGE=none) and adjust downstream rules accordingly.

Low

  • [logic-error] docs/problems/gitlab-support.md — The determine-stage script extracts the triggering label using .labels[-1].name (the last element of the full labels array), but the Event Mapping section states "The bridge extracts the triggering label from this [changes] field." Taking the last element of the static labels array is not equivalent to checking which label was just added — if the issue already has other labels, the wrong label may be selected.

  • [edge-case] docs/problems/gitlab-support.md — The normalizeClaims function reads the aud claim via getString, which requires the value to be a string. Per RFC 7519, the aud claim may be either a single string or an array of strings. GitLab OIDC tokens with multiple audiences would cause the type assertion to fail, rejecting valid tokens.

  • [naming-convention] README.md — In the supersession notice for GitLab Implementation, the link text uses a filename ([gitlab-support.md]) instead of a human-readable title. This is inconsistent with the README's established convention where all other problem document links use descriptive titles.
    Remediation: Change the link text from gitlab-support.md to GitLab Support to match the convention.

Previous run (5)

Review

Findings

Medium

  • [logic-error] docs/problems/gitlab-support.md — The trigger-stage CI job declares needs: [generate-child-config] but relies on the $STAGE dotenv variable produced by determine-stage. In GitLab CI, dotenv report artifacts are only propagated to jobs that directly list the producing job in their needs: array — transitive propagation through dependency chains does not occur. As written, $STAGE will be empty in trigger-stage, causing the rule condition $STAGE to always evaluate as false and the job to be skipped. Since this example guides implementation of the dispatch pipeline, the bug would be carried into production code.
    Remediation: Add determine-stage to the needs array of trigger-stage: needs: [determine-stage, generate-child-config].

  • [naming-convention] docs/problems/gitlab-support.md:1 — This file has the identical title ("# GitLab Support Implementation Details") as the existing docs/problems/gitlab-implementation.md. While the PR adds a supersession notice to the old file and differentiates them in README.md, having two files with the same H1 title creates confusion in search results, editor tabs, and navigation. Anchored from prior review.
    Remediation: Differentiate the new file's title — e.g., "GitLab Support via Webhook Bridge" or "GitLab Support Implementation Plan (ADR 0043)" — to distinguish it from the superseded document.

Low

  • [logic-error] docs/problems/gitlab-support.md — The determine-stage script extracts the triggering label using .labels[-1].name (the last element of the full labels array), but the Event Mapping section states "The bridge extracts the triggering label from this [changes] field." Taking the last element of the static labels array is not equivalent to checking which label was just added — if the issue already has other labels, the wrong label may be selected.

  • [stale-doc] docs/ADRs/0031-reusable-workflows-for-action-installed-distribution.md:144 — References ADR 0028 as the multi-forge support decision without noting its supersession by ADR 0043. The forward-looking statement remains factually correct (GitLab does need its own distribution mechanism), but readers following the link will land on a superseded ADR without context.

  • [stale-doc] docs/plans/agent-execution-environment.md:925 — References section lists ADR-0028 without noting its superseded status. Readers consulting this plan for GitLab-related execution environment work would benefit from knowing ADR 0043 is the current decision.

Previous run (6)

Review

Findings

Medium

  • [missing-doc] README.md — New problem document docs/problems/gitlab-support.md is not linked from README.md. CLAUDE.md explicitly requires: "When adding new problem areas, create a new file in docs/problems/ and link it from README.md." The existing gitlab-implementation.md entry is a separate document.
    Remediation: Add an entry to the README.md problem doc list for docs/problems/gitlab-support.md.

  • [missing-doc] docs/architecture.md — AGENTS.md requires "When status is Accepted, update docs/architecture.md and related problem docs in the same PR." ADR 0043 is marked Accepted but docs/architecture.md is not updated to reference it or the webhook bridge architecture.
    Remediation: Update docs/architecture.md to reference ADR 0043 in the forge abstraction or GitLab-related sections.

  • [frontmatter-consistency] docs/ADRs/0028-gitlab-support.md:3 — The frontmatter status field remains Deprecated but the body Status section now reads "Deprecated — superseded by ADR 0043." The ADR template defines three valid values: Accepted, Deprecated, Superseded. Since ADR 0028 is being superseded by a newer ADR, Superseded is the semantically correct frontmatter value.
    Remediation: Change the frontmatter status from Deprecated to Superseded.

  • [naming-convention] docs/problems/gitlab-support.md:1 — This file has the identical title ("# GitLab Support Implementation Details") as the existing docs/problems/gitlab-implementation.md, which references the now-deprecated ADR 0028. After merge, readers will find two GitLab implementation documents with the same title and no cross-reference between them.
    Remediation: Either (a) add a deprecation notice to gitlab-implementation.md pointing to gitlab-support.md, or (b) differentiate the new file's title, or (c) replace gitlab-implementation.md if the new doc fully supersedes it.

  • [stale-doc] docs/roadmap.md:78 — References ADR-0028 as "the architectural groundwork" for GitLab support and links only to gitlab-implementation.md. After this PR, ADR 0028 is deprecated/superseded by ADR 0043. Readers following the roadmap will land on a deprecated ADR without knowing the current approach.
    Remediation: Update the roadmap's GitLab section to reference ADR 0043 and add a link to the new gitlab-support.md problem doc.

Low

  • [logic-error] docs/problems/gitlab-support.md:603 — The normalizeClaims code example uses unchecked type assertions (e.g., rawClaims["project_path"].(string)) that will panic if the claim is missing. Since this guides implementation of a security-critical component (the token mint), the example should demonstrate safe patterns.

  • [logic-error] docs/problems/gitlab-support.md:603 — The normalizeClaims code example uses strings.Contains(issuer, "gitlab") to distinguish forges. Self-hosted GitLab instances with non-"gitlab" domains (e.g., code.example.com) would silently fall through to the GitHub path. The doc already mentions ALLOWED_ISSUERS configuration but the code example doesn't use it.

  • [cross-referencing] docs/ADRs/0043-gitlab-support-via-webhook-bridge.md:4 — The relates_to frontmatter lists agent-infrastructure and agent-architecture but omits gitlab-support (the companion problem doc added in this PR). Per the ADR template, relates_to should reference problem doc filenames.

  • [stale-doc] docs/problems/gitlab-implementation.md:3 — Opening line says "see ADR-0028" for the architectural decision without noting that ADR 0028 is now superseded by ADR 0043.

  • [stale-doc] docs/ADRs/0036-agent-execution-sandbox.md:24 — References ADR-0028 as the GitLab support design without noting its superseded status.

Info

  • [inconsistent-validation] docs/problems/gitlab-support.md — The bridge function's Go code (line 496) uses SHA256 pre-hashing before subtle.ConstantTimeCompare for webhook token validation, while the dispatch pipeline's bash validation (line 754) uses openssl dgst hash comparison with == (not constant-time). The two validation implementations have inconsistent security properties.
Previous run (7)

Review

Findings

Medium

  • [logic-error] docs/problems/gitlab-support.md — The validate-webhook job in the dispatch pipeline references ${WEBHOOK_TOKEN} for hash comparison (the received webhook token), but this variable is never passed to the pipeline. The bridge function (Phase 2) sends only EVENT_TYPE, SOURCE_PROJECT, and EVENT_PAYLOAD_B64 as pipeline variables via the trigger API. The WEBHOOK_TOKEN_<hash> CI/CD variables are the expected tokens, not the received token. Since WEBHOOK_TOKEN is undefined/empty in the pipeline context, the hash comparison will always fail, causing every legitimate dispatch to be rejected.
    Remediation: Either (a) add WEBHOOK_TOKEN to the bridge's trigger API call as a pipeline variable (variables[WEBHOOK_TOKEN]={token}), or (b) remove the in-pipeline token re-validation entirely since the bridge already validates the token via constant-time comparison before triggering the pipeline. Option (b) is cleaner — the bridge is the trust boundary, and passing the raw webhook token through a pipeline variable widens its exposure.

  • [edge-case] docs/problems/gitlab-support.md — When determine-stage finds no matching stage, it exits 0 without creating stage.env. The job declares artifacts: reports: dotenv: stage.env. In GitLab CI 17.0+, a missing dotenv artifact file causes a warning but may not fail the job — however, this behavior has changed across GitLab versions and is not guaranteed to remain stable.
    Remediation: Always write the dotenv file. When no stage matches, write an empty file (touch stage.env) or write a sentinel value (STAGE=none) and adjust downstream rules accordingly.

Low

  • [logic-error] docs/problems/gitlab-support.md — The determine-stage script extracts the triggering label using .changes.labels.current[-1].name — the last element of the full current-labels array after the change. This does not reliably identify which label was just added; if an issue already has labels, the last element of current may not be the newly added label. The Event Mapping section states the bridge extracts the triggering label from the changes field, implying a diff-based approach.

  • [edge-case] docs/problems/gitlab-support.md — The normalizeClaims function reads the aud claim via getString, which requires the value to be a string. Per RFC 7519, the aud claim may be either a single string or an array of strings. GitLab OIDC tokens with multiple audiences would cause the type assertion to fail, rejecting valid tokens.

  • [naming-convention] README.md — In the supersession notice for GitLab Implementation, the link text uses a filename ([gitlab-support.md]) instead of a human-readable title. This is inconsistent with the README's established convention where all other problem document links use descriptive titles.
    Remediation: Change the link text from gitlab-support.md to GitLab Support to match the convention.

Previous run (8)

Review

Findings

Medium

  • [logic-error] docs/problems/gitlab-support.md — The trigger-stage CI job declares needs: [generate-child-config] but relies on the $STAGE dotenv variable produced by determine-stage. In GitLab CI, dotenv report artifacts are only propagated to jobs that directly list the producing job in their needs: array — transitive propagation through dependency chains does not occur. As written, $STAGE will be empty in trigger-stage, causing the rule condition $STAGE to always evaluate as false and the job to be skipped. Since this example guides implementation of the dispatch pipeline, the bug would be carried into production code.
    Remediation: Add determine-stage to the needs array of trigger-stage: needs: [determine-stage, generate-child-config].

  • [naming-convention] docs/problems/gitlab-support.md:1 — This file has the identical title ("# GitLab Support Implementation Details") as the existing docs/problems/gitlab-implementation.md. While the PR adds a supersession notice to the old file and differentiates them in README.md, having two files with the same H1 title creates confusion in search results, editor tabs, and navigation. Anchored from prior review.
    Remediation: Differentiate the new file's title — e.g., "GitLab Support via Webhook Bridge" or "GitLab Support Implementation Plan (ADR 0043)" — to distinguish it from the superseded document.

Low

  • [logic-error] docs/problems/gitlab-support.md — The determine-stage script extracts the triggering label using .labels[-1].name (the last element of the full labels array), but the Event Mapping section states "The bridge extracts the triggering label from this [changes] field." Taking the last element of the static labels array is not equivalent to checking which label was just added — if the issue already has other labels, the wrong label may be selected.

  • [stale-doc] docs/ADRs/0031-reusable-workflows-for-action-installed-distribution.md:144 — References ADR 0028 as the multi-forge support decision without noting its supersession by ADR 0043. The forward-looking statement remains factually correct (GitLab does need its own distribution mechanism), but readers following the link will land on a superseded ADR without context.

  • [stale-doc] docs/plans/agent-execution-environment.md:925 — References section lists ADR-0028 without noting its superseded status. Readers consulting this plan for GitLab-related execution environment work would benefit from knowing ADR 0043 is the current decision.

Previous run (9)

Review

Findings

Medium

  • [logic-error] docs/problems/gitlab-support.md — The trigger-stage CI job declares needs: [generate-child-config] but relies on the $STAGE dotenv variable produced by determine-stage. In GitLab CI, dotenv report artifacts are only propagated to jobs that directly list the producing job in their needs: array — transitive propagation through dependency chains does not occur. As written, $STAGE will be empty in trigger-stage, causing the rule condition $STAGE to always evaluate as false and the job to be skipped. Since this example guides implementation of the dispatch pipeline, the bug would be carried into production code.
    Remediation: Add determine-stage to the needs array of trigger-stage: needs: [determine-stage, generate-child-config].

  • [naming-convention] docs/problems/gitlab-support.md:1 — This file has the identical title ("# GitLab Support Implementation Details") as the existing docs/problems/gitlab-implementation.md. While the PR adds a supersession notice to the old file and differentiates them in README.md, having two files with the same H1 title creates confusion in search results, editor tabs, and navigation. Anchored from prior review.
    Remediation: Differentiate the new file's title — e.g., "GitLab Support via Webhook Bridge" or "GitLab Support Implementation Plan (ADR 0043)" — to distinguish it from the superseded document.

Low

  • [logic-error] docs/problems/gitlab-support.md — The determine-stage script extracts the triggering label using .labels[-1].name (the last element of the full labels array), but the Event Mapping section states "The bridge extracts the triggering label from this [changes] field." Taking the last element of the static labels array is not equivalent to checking which label was just added — if the issue already has other labels, the wrong label may be selected.

  • [stale-doc] docs/ADRs/0031-reusable-workflows-for-action-installed-distribution.md:144 — References ADR 0028 as the multi-forge support decision without noting its supersession by ADR 0043. The forward-looking statement remains factually correct (GitLab does need its own distribution mechanism), but readers following the link will land on a superseded ADR without context.

  • [stale-doc] docs/plans/agent-execution-environment.md:925 — References section lists ADR-0028 without noting its superseded status. Readers consulting this plan for GitLab-related execution environment work would benefit from knowing ADR 0043 is the current decision.

Previous run (10)

Review

Findings

Medium

  • [missing-doc] README.md — New problem document docs/problems/gitlab-support.md is not linked from README.md. CLAUDE.md explicitly requires: "When adding new problem areas, create a new file in docs/problems/ and link it from README.md." The existing gitlab-implementation.md entry is a separate document.
    Remediation: Add an entry to the README.md problem doc list for docs/problems/gitlab-support.md.

  • [missing-doc] docs/architecture.md — AGENTS.md requires "When status is Accepted, update docs/architecture.md and related problem docs in the same PR." ADR 0043 is marked Accepted but docs/architecture.md is not updated to reference it or the webhook bridge architecture.
    Remediation: Update docs/architecture.md to reference ADR 0043 in the forge abstraction or GitLab-related sections.

  • [frontmatter-consistency] docs/ADRs/0028-gitlab-support.md:3 — The frontmatter status field remains Deprecated but the body Status section now reads "Deprecated — superseded by ADR 0043." The ADR template defines three valid values: Accepted, Deprecated, Superseded. Since ADR 0028 is being superseded by a newer ADR, Superseded is the semantically correct frontmatter value.
    Remediation: Change the frontmatter status from Deprecated to Superseded.

  • [naming-convention] docs/problems/gitlab-support.md:1 — This file has the identical title ("# GitLab Support Implementation Details") as the existing docs/problems/gitlab-implementation.md, which references the now-deprecated ADR 0028. After merge, readers will find two GitLab implementation documents with the same title and no cross-reference between them.
    Remediation: Either (a) add a deprecation notice to gitlab-implementation.md pointing to gitlab-support.md, or (b) differentiate the new file's title, or (c) replace gitlab-implementation.md if the new doc fully supersedes it.

  • [stale-doc] docs/roadmap.md:78 — References ADR-0028 as "the architectural groundwork" for GitLab support and links only to gitlab-implementation.md. After this PR, ADR 0028 is deprecated/superseded by ADR 0043. Readers following the roadmap will land on a deprecated ADR without knowing the current approach.
    Remediation: Update the roadmap's GitLab section to reference ADR 0043 and add a link to the new gitlab-support.md problem doc.

Low

  • [logic-error] docs/problems/gitlab-support.md:603 — The normalizeClaims code example uses unchecked type assertions (e.g., rawClaims["project_path"].(string)) that will panic if the claim is missing. Since this guides implementation of a security-critical component (the token mint), the example should demonstrate safe patterns.

  • [logic-error] docs/problems/gitlab-support.md:603 — The normalizeClaims code example uses strings.Contains(issuer, "gitlab") to distinguish forges. Self-hosted GitLab instances with non-"gitlab" domains (e.g., code.example.com) would silently fall through to the GitHub path. The doc already mentions ALLOWED_ISSUERS configuration but the code example doesn't use it.

  • [cross-referencing] docs/ADRs/0043-gitlab-support-via-webhook-bridge.md:4 — The relates_to frontmatter lists agent-infrastructure and agent-architecture but omits gitlab-support (the companion problem doc added in this PR). Per the ADR template, relates_to should reference problem doc filenames.

  • [stale-doc] docs/problems/gitlab-implementation.md:3 — Opening line says "see ADR-0028" for the architectural decision without noting that ADR 0028 is now superseded by ADR 0043.

  • [stale-doc] docs/ADRs/0036-agent-execution-sandbox.md:24 — References ADR-0028 as the GitLab support design without noting its superseded status.

Info

  • [inconsistent-validation] docs/problems/gitlab-support.md — The bridge function's Go code (line 496) uses SHA256 pre-hashing before subtle.ConstantTimeCompare for webhook token validation, while the dispatch pipeline's bash validation (line 754) uses openssl dgst hash comparison with == (not constant-time). The two validation implementations have inconsistent security properties.

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Jun 2, 2026
@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 2, 2026
@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 3, 2026
Comment thread docs/problems/gitlab-support.md
Comment thread docs/problems/gitlab-support.md Outdated
Comment thread docs/problems/gitlab-support.md
Comment thread docs/problems/gitlab-support.md
Comment thread docs/problems/gitlab-support.md
rh-hemartin

This comment was marked as off-topic.

@rh-hemartin rh-hemartin self-requested a review June 3, 2026 06:34
Comment thread docs/ADRs/0043-gitlab-support-via-webhook-bridge.md Outdated
Comment thread docs/ADRs/0043-gitlab-support-via-webhook-bridge.md
Comment thread docs/ADRs/0043-gitlab-support-via-webhook-bridge.md Outdated
@ggallen ggallen force-pushed the docs/adr-0043-gitlab-support branch from 3ca0501 to 130d954 Compare June 3, 2026 14:37
@ggallen ggallen force-pushed the docs/adr-0043-gitlab-support branch from 130d954 to de8ee02 Compare June 3, 2026 14:40
@ggallen ggallen requested review from ifireball and maruiz93 June 3, 2026 14:41
@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 3, 2026
@ggallen ggallen force-pushed the docs/adr-0043-gitlab-support branch from de8ee02 to 0fa7e2e Compare June 3, 2026 15:37
@ggallen
Copy link
Copy Markdown
Contributor Author

ggallen commented Jun 3, 2026

All findings from the latest review addressed in the squashed commit:

Medium — fixed:

  • [logic-error] MinimalPayload struct — Added Changes field with nested MinimalChanges and MinimalLabelChanges structs that capture changes.labels.current and changes.labels.previous. The determine-stage script's jq expression for label diffing now has the data it needs.

Low — fixed:

  • [edge-case] getAudience — Now accepts an expected audience parameter and iterates over all elements in the aud array to check if the expected audience (fullsend-mint) is present, rather than blindly returning aud[0]. Falls back to first element if the expected audience isn't found.
  • [scope-coherence] Alternative 3 ambiguity — Removed the sentence in the Credential model section that re-opened Alternative 3 as a "viable alternative." The mint statefulness trade-off paragraph now clearly accepts the per-project storage cost as worthwhile for the centralized credential management benefits.
  • [architectural-coherence] Alternative 4 criteria — Replaced the open-ended "warrants a proof-of-concept" with concrete decision criteria: recommended for multiple self-hosted instances behind VPNs, not recommended for GitLab.com or single-instance deployments. Clarified it may be dropped if the webhook bridge proves viable.
  • [architectural-coherence] PAT blast radius — Added a risk entry documenting that compromised PATs grant persistent role-level access until rotation (up to 1 year), with mitigations: max expiry enforcement, automated rotation, Secret Manager storage, and per-role scoping.

Info — fixed:

  • [cross-reference-style] Removed redundant "(Deprecated)" label from the supersession note — ADR 0028's status is tracked in its own frontmatter.

@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 3, 2026
@Dharit13
Copy link
Copy Markdown

Dharit13 commented Jun 3, 2026

Sorry @ggallen, yesterday I could not review it. But I have reviewed it now, and most of the issues are addressed. feel like I need to extend my working hours :P

@ggallen ggallen force-pushed the docs/adr-0043-gitlab-support branch from 0fa7e2e to 9039e05 Compare June 3, 2026 16:31
@ggallen
Copy link
Copy Markdown
Contributor Author

ggallen commented Jun 3, 2026

Addressing review findings:

Medium — N/A:

  • [error-handling] internal/cli/run.go:141 TraceID — Not applicable. This PR contains no Go code changes. The run.go finding is stale, carried from a previous review when scope-creep commits were present. Those have been removed; the PR is now docs-only.

Low — fixed:

  • [architectural-coherence] ADR Alt 3 — Clarified that the rejection is about the storage mechanism (CI/CD variables), not PATs themselves. The decision section uses PATs stored in Secret Manager through the mint, which is distinct from storing them as CI/CD variables.
  • [architectural-coherence] ADR Alt 4 — Replaced the open-ended "may be dropped entirely" with explicit decision criteria: "will be prototyped if two or more organizations report webhook bridge deployment blocked by network constraints; otherwise dropped."

Low — N/A:

  • [scope-coherence] internal/cli/run.go — Stale finding. All Go code, scaffold changes, and Phase 2 plan have been removed from this PR. It now contains only the ADR 0043 docs, the companion problem doc, and related cross-reference updates.
  • [comment-placement] post-triage.sh — Not in this PR.

Squashed to single signed-off commit, rebased on upstream/main, force-pushed.

@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 3, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Greg Allen <gallen@redhat.com>
@ggallen ggallen force-pushed the docs/adr-0043-gitlab-support branch from 9039e05 to 83c4010 Compare June 3, 2026 17:56
@ggallen
Copy link
Copy Markdown
Contributor Author

ggallen commented Jun 3, 2026

Findings addressed in 83c4010:

  1. [stale-forward-reference] Updated ADR-0007 line 39: replaced the generic "GitLab and Forgejo will need equivalent per-role identity mechanisms" with a specific reference to ADR 0043's Project Access Token model (triage → Reporter, code → Developer, etc.), noting only Forgejo remains open.

  2. Scope cleanup — Removed 4 scaffold sub-agent model pin changes (claude-sonnet-4-6@defaultclaude-sonnet-4-6) that were unrelated to the ADR 0043 GitLab support content.

All Info findings were acknowledged (prior findings resolved, cosmetic timing side-channel).

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels Jun 3, 2026
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.

5 participants