Skip to content

fix(#7769): pin GitLab job identity to CI_JOB_TOKEN record - #7817

Merged
ggallen merged 3 commits into
mainfrom
agent/7769-in-job-gate-hardening
Oct 1, 2026
Merged

ggallen merged 3 commits into
mainfrom
agent/7769-in-job-gate-hardening

Conversation

@fullsend-ai-coder

@fullsend-ai-coder fullsend-ai-coder Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Harden the GitLab poll and agent in-job security gates per ADR 0125.

This PR delivers the in-job hardening portion of #7769:

  • Pins CI job identity to the server-derived GitLab job record.
  • Enforces disjoint poller and agent source allowlists.
  • Verifies the protected default branch.
  • Applies CI_DEBUG_TRACE deny-before-admit behavior.
  • Uses CA-pinned, trigger-safe GitLab gate requests.
  • Fails closed on merge-request identity ambiguity.
  • Includes the related tests and documentation updates.

This PR does not implement pipeline-input transport, opaque dispatch records, or dispatcher fast-path wiring. Those are tracked separately:

The final design must keep the Fullsend runtime credential at Developer level. Owner-level credentials are not required for runtime dispatch. GitLab project pipeline-variable restriction support is an administrative/install-time primitive only; this PR must not make an Owner-level runtime credential part of the dispatch design or break the existing path before #7850/#7851 land.

Closes #7769

Verification

  • Rebased and squashed before final verification.
  • Relevant Go, scaffold, shell, documentation, and security checks must pass.
  • Any CI failures introduced or exposed by this branch must be fixed before merge.

@fullsend-ai-coder
fullsend-ai-coder Bot requested a review from a team as a code owner September 29, 2026 01:29
@fullsend-ai-coder fullsend-ai-coder Bot added the ready-for-review Triggers review agent dispatch label Sep 29, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 1:31 AM UTC · Completed 1:55 AM UTC

Commit: e48480c · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $8.68

@github-actions

github-actions Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Site preview

Preview: https://6206c5ef-site.fullsend-ai.workers.dev

Commit: 08540ef0675cacb227e2b9fb1598d6ec7febfc7b

@codecov

codecov Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

@fullsend-ai-review fullsend-ai-review Bot added the risk/elevated PR risk: elevated label Sep 29, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Risk Assessment: moderate (2/5)

Details

Moderate risk: large security-focused gate-hardening change with elevated churn and regression history is balanced by bot authorship, healthy test coverage, no protected-path edits, opt-in report-only enforcement, and a narrow incremental delta since the prior review.

Previous run

Risk Assessment: moderate (2/5)

Details

Moderate risk: large, security-focused gate-hardening change with elevated churn/regression history is balanced by bot authorship, healthy test coverage, no protected-path edits, and an opt-in (report-only-by-default) enforcement flag; this round's commit is a narrow, well-tested incremental fix (5 files) rather than a fresh large-scope change.

Previous run (2)

Risk Assessment: moderate (2/5)

Details

Moderate risk: large change size and security-focused gate hardening with recent churn and regression history are balanced by bot authorship, healthy test coverage, absence of protected path edits, an opt-in enforcement flag, and zero unresolved issue discussions; score held stable at 2 via re-review anchoring against the prior round.

Previous run (3)

Risk Assessment: moderate (2/5)

Details

Moderate risk: large change size and security-focused gate hardening with recent churn and regression history are balanced by bot authorship, healthy test coverage, absence of protected path edits, an opt-in enforcement flag, and zero unresolved issue discussions; score held stable at 2 via re-review anchoring against the prior round.

Previous run (4)

Risk Assessment: moderate (2/5)

Details

Moderate risk: large change size and security-focused gate hardening with recent churn and regression history are balanced by bot authorship, healthy test coverage, absence of protected path edits, and zero unresolved issue discussions.

Previous run (5)

Risk Assessment: moderate (2/5)

Details

Score preserved at 2 (moderate): re-review anchoring holds across unchanged Tier 1 signals with zero protected path or dependency changes, bot authorship, solid test coverage (0.38), and opt-in rollout safety balancing elevated churn in scaffold scripts.

Previous run (6)

Risk Assessment: moderate (2/5)

Details

Score preserved at 2 (moderate): re-review anchoring holds across the 34-file diff where zero protected path or dependency changes, a strong test ratio (0.38), bot authorship, and opt-in rollout safety keep the composite at moderate despite elevated churn in target scaffold scripts.

Previous run (7)

Risk Assessment: moderate (2/5)

Details

Score preserved at 2 (moderate): anchoring holds across the cumulative 34-file/+3111-231 diff despite a history rewrite precluding clean incremental compare enumeration — zero protected paths or dependency changes, a solid test ratio (0.38), bot authorship, and continued opt-in gating of the newly-added enforcement control keep the composite at moderate.

Previous run (8)

Risk Assessment: moderate (2/5)

Details

Score preserved at 2 (moderate) because cumulative diff characteristics remain stable across review rounds and this round's incremental delta is a narrow, rollback-safe UX refinement to report-only status handling under the default-disabled FULLSEND_GITLAB_PIPELINE_VAR_RESTRICTION flag, backed by solid test coverage and leaving core identity-pinning logic unchanged.

Previous run (9)

Risk Assessment: moderate (2/5)

Details

Score reduced from the prior 3 (elevated) to 2 (moderate) because this round's only new commit adds the ci_pipeline_variables_minimum_override_role control behind a new, default-disabled FULLSEND_GITLAB_PIPELINE_VAR_RESTRICTION opt-in flag, improving rollback safety for the incremental change; the core identity-pin mechanism reviewed across prior rounds is unchanged.

Previous run (10)

Risk Assessment: elevated (3/5)

Details

Security-critical CI job script identity hardening with large blast radius modifies existing behavior without a feature flag, anchored to prior elevated rating following doc-only prose updates with elevated churn and regression history balanced by bot authorship and substantial test coverage.

Previous run (11)

Risk Assessment: elevated (3/5)

Details

Security-critical CI job script identity hardening with a large blast radius modifies existing behavior without a feature flag, anchored to the prior elevated rating with elevated churn, regression history, and coupling balanced by bot authorship and strong test coverage.

Previous run (12)

Risk Assessment: elevated (3/5)

Details

Security-critical CI job script identity hardening with a large blast radius modifies existing behavior without a feature flag, anchored to the prior elevated rating with elevated churn, regression history, and coupling balanced by bot authorship and strong test coverage.

Previous run (13)

Risk Assessment: elevated (3/5)

Details

Security-critical CI job script identity hardening with a large blast radius modifies existing behavior without a feature flag, anchored to the prior elevated rating with elevated churn, regression history, and coupling balanced by bot authorship and strong test coverage.

Previous run (14)

Risk Assessment: elevated (3/5)

Details

Security-critical CI job script identity hardening with a large blast radius modifies existing behavior without a feature flag, anchored to the prior elevated rating with elevated churn, regression history, and coupling balanced by bot authorship and strong test coverage.

Previous run (15)

Risk Assessment: elevated (3/5)

Details

Security-critical CI job script identity hardening with a large blast radius modifies existing behavior without a feature flag, anchored to the prior elevated rating and balanced by bot authorship, strong test ratio, and issue alignment.

Previous run (16)

Risk Assessment: elevated (3/5)

Details

Security-critical CI hardening modifying existing GitLab CI job scripts without a feature flag, anchored to the prior elevated rating and balanced by bot authorship, strong test coverage, and clear issue alignment.

Previous run (17)

Risk Assessment: elevated (3/5)

Details

Security-critical CI hardening modifying existing GitLab CI job scripts without a feature flag, balanced by strong test coverage, a bot author, and clear issue acceptance criteria.

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Review

Findings

Low

Prior findings from the previous review round (SHA 369c87d4) are resolved in this diff:

  • The issue_* EVENT_TYPE arm in run-agent-job.sh now unsets CI_MERGE_REQUEST_IID and exports FULLSEND_NOTE_TARGET=issues, with a regression test asserting the fullsend CLI child does not inherit a pre-set FULLSEND_NOTE_TARGET=merge_requests. Not observed to regress in the current diff.
  • pin-ci-job-identity.sh's post-pin proxy cleanup now also unsets ALL_PROXY/all_proxy alongside the previously-covered proxy variables, with a regression test covering both. Not observed to regress in the current diff.

A candidate finding about the pinned-ref trusted-config git fetch in run-agent-job.sh not stripping the same GIT_* diagnostic/env-override set that checkout-mr-source.sh forces off was considered and withdrawn: that code path and its env handling predate this PR (only the ref source changed, from CI_DEFAULT_BRANCH to FULLSEND_PINNED_REF), the cited helper does not actually cover GIT_PROXY_COMMAND/GIT_CONFIG_* as claimed, and SSLKEYLOGFILE/proxy variables are already unset in the job shell by the time this fetch runs.

Previous run

Review

Findings

Medium

  • [auth-bypass] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/run-agent-job.sh:397 — The issue_* EVENT_TYPE arm (added this round to fix a prior auth-bypass finding) unsets CI_MERGE_REQUEST_IID but does not unset or override FULLSEND_NOTE_TARGET. newGitLabClientFromEnv (internal/cli/reconcilestatus.go:240, unchanged base-tree code) selects the merge_requests note target on CI_MERGE_REQUEST_IID != "" || FULLSEND_NOTE_TARGET == "merge_requests" — an OR. The sibling * arm explicitly export FULLSEND_NOTE_TARGET="merge_requests" for MR events, but the issue_* arm never counters a pre-set value of that same documented, unsigned, outrankable CI/CD variable (docs/guides/getting-started/operations.md:242). The new regression test (TestRunAgentJobScript_UnsetsMergeRequestIIDForIssueEvent) only asserts CI_MERGE_REQUEST_IID=UNSET; it never plants or checks FULLSEND_NOTE_TARGET. A project/group/pipeline CI/CD variable FULLSEND_NOTE_TARGET=merge_requests still causes an issue-event status comment to be sent to merge_requests/{STATUS_IID} instead of issues/{STATUS_IID} — either a silent 404/StepWarn, or a bot comment misdelivered onto an unrelated merge request if one exists with that IID.
    Remediation: In the issue_* arm, also unset FULLSEND_NOTE_TARGET (or export FULLSEND_NOTE_TARGET=issues) before invoking the fullsend CLI. Extend the existing regression test (or add a sibling) to pre-set FULLSEND_NOTE_TARGET=merge_requests in the stub environment and assert the fullsend CLI child does not inherit it.

Low


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (2)

Review

Findings

Medium

  • [api-contract] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/pin-ci-job-identity.sh:281 — After default-branch equality succeeds, fullsend_pin_ci_job_identity GETs /projects/:id/repository/branches/:branch with JOB-TOKEN via fullsend_gate_curl (curl --fail) and treats any non-2xx as fatal ("cannot read pinned branch"). Tests only mock that route (TestPinCIJobIdentity_BranchLookupFailsClosed on a 403). GitLab's classic CI_JOB_TOKEN allowlist documents repository files/commits/changelog and GET /projects/:id, /pipelines/:id, but not the branches endpoint; a 403/404 on a supported GitLab version would abort every poll/agent job after the default-branch check and before HMAC verification, which would violate the PR's own "existing schedule + api paths remain behavior-preserving" acceptance criterion. This remains unverified against a live GitLab instance and has recurred, unchanged in substance, across many prior review rounds of this PR.
    Remediation: Confirm against GitLab's current job-token REST allowlist (and the oldest self-hosted version fullsend supports) whether GET /projects/:id/repository/branches/:branch accepts CI_JOB_TOKEN. If it does not, perform the protected-branch check after a PAT is selected, switch to an allowlisted equivalent such as GET /projects/:id/protected_branches/:branch, or drop the in-job branches check and rely on default-branch equality plus install-time protected-branch enforcement.

  • [auth-bypass] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/run-agent-job.sh:389 — In the issue_* arm of GITLAB_ISSUE_URL construction (lines 387-391), the script builds the issue URL from the signed STATUS_IID but neither unsets CI_MERGE_REQUEST_IID nor exports FULLSEND_NOTE_TARGET=issues. The base-tree newGitLabClientFromEnv (internal/cli/reconcilestatus.go:240-241, unchanged by this PR) sets gl.WithNoteTarget("merge_requests") whenever CI_MERGE_REQUEST_IID is merely non-empty — that OR condition means exporting FULLSEND_NOTE_TARGET=issues cannot override a non-empty CI_MERGE_REQUEST_IID. This job admits source=api only, so GitLab never natively populates CI_MERGE_REQUEST_IID here; any value present is an ordinary, outrankable project/group/pipeline CI/CD variable — the same class of variable this PR's identity-pin hardening otherwise treats as untrusted everywhere else in this file. Since --status-number carries the signed issue IID for issue_* dispatches, a non-empty CI_MERGE_REQUEST_IID would misdirect the fullsend run status-comment API call to merge_requests/{issue_iid} instead of issues/{issue_iid} — either a 404 (status comment fails, StepWarn) or, if a merge request with that IID exists, a bot status comment misdelivered onto an unrelated merge request.
    Remediation: In the issue_* branch, unset CI_MERGE_REQUEST_IID before invoking the fullsend CLI. Exporting FULLSEND_NOTE_TARGET=issues alone is not sufficient, because newGitLabClientFromEnv treats any non-empty CI_MERGE_REQUEST_IID as merge_requests regardless of FULLSEND_NOTE_TARGET.

  • [incomplete-doc] docs/guides/getting-started/configuring-gitlab.md:733 — This doc documents the new CI_DEBUG_TRACE deny-before-admit workflow rule but does not document the runtime identity-pin constraints introduced by pin-ci-job-identity.sh: jobs now fail closed unless they run on the enrolled protected default branch (the YAML workflow:rules still admit any protected ref; the pin additionally rejects protected non-default branches), and unless the pinned pipeline source matches the job's allowlist (poller=schedule, agent=api). These constraints are documented in docs/contributing/gitlab-role-credentials.md, which is not the operator-facing getting-started path — operators on a protected non-default branch will hit a new, undocumented fail-closed abort.
    Remediation: Document in docs/guides/getting-started/configuring-gitlab.md that pin-ci-job-identity.sh requires the enrolled default branch to be protected (not merely any protected ref) and enforces disjoint pipeline sources (schedule for poll jobs, api for agent jobs); note that non-default protected branches fail closed by design.

Low


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (3)

Review

Findings

Medium

  • [api-contract] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/pin-ci-job-identity.sh:280 — After default-branch equality succeeds, the identity pin still GETs /projects/:id/repository/branches/:branch with JOB-TOKEN via fullsend_gate_curl (curl --fail) and treats any non-2xx as fatal ("cannot read pinned branch"). Tests only mock that endpoint (including TestPinCIJobIdentity_BranchLookupFailsClosed on a 403) — they do not prove GitLab's CI_JOB_TOKEN REST allowlist actually accepts this route. If a target GitLab version rejects the call, every legitimate poll/agent job would abort fail-closed after the default-branch check and before HMAC verification, which would violate the PR's own "existing schedule + api paths remain behavior-preserving" acceptance criterion. This is a suspected operational-breakage risk, not a security hole, and remains unverified against a live GitLab instance in this sandbox — unchanged since the prior review round.
    Remediation: Confirm against GitLab's current job-token REST allowlist (and the oldest self-hosted version fullsend supports) whether GET /projects/:id/repository/branches/:branch accepts CI_JOB_TOKEN. If it does not, perform the protected-branch check after a PAT is selected, switch to an allowlisted equivalent such as GET /projects/:id/protected_branches/:branch, or drop the in-job branches check and rely on default-branch equality plus install-time protected-branch enforcement.

Low


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (4)

Review

Findings

Medium

  • [api-contract] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/pin-ci-job-identity.sh:260 — The identity pin always GETs /projects/:id/repository/branches/:branch with JOB-TOKEN and treats any non-2xx as fatal ("cannot read pinned branch"), so if that endpoint is not on the target GitLab version's CI_JOB_TOKEN REST allowlist, every legitimate poll/agent job would abort before HMAC verification or fullsend poll/run. This would be fail-closed operational breakage (violating the PR's own "existing schedule + api paths remain behavior-preserving" acceptance criterion), not a security hole — default-branch equality is already enforced from GET /projects/:id, and this branches call only re-reads .protected. This claim rests on the reviewing model's recollection of GitLab's job-token endpoint allowlist, not a live-verified source (no internet access was available to check GitLab's current API docs), so it may be outdated — GitLab has been incrementally expanding job-token endpoint coverage. TestPinCIJobIdentity_BranchLookupFailsClosed mocks a 403 response and asserts fail-closed, but a mocked test cannot prove real GitLab accepts JOB-TOKEN on this route at all.
    Remediation: Verify against GitLab's current API documentation (or a live target-version instance) whether GET /projects/:id/repository/branches/:branch accepts CI_JOB_TOKEN authentication for a same-project job. If unsupported, perform the protected-branch check after a PAT is selected (PRIVATE-TOKEN), or drop the in-job branches check and rely on the already-performed default-branch equality check plus install-time protections.

Low

  • [edge-case] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/pin-ci-job-identity.sh:170 — After requiring an https://*/api/v4-shaped URL, the helper rejects any CI_API_V4_URL containing square brackets, intended to block curl brace/bracket-range globbing. curl --globoff (already passed in fullsend_gate_curl) already disables that expansion, so the extra bracket ban is redundant for that threat and also rejects legitimate IPv6 API roots (bracketed IPv6 literal hosts) — the pin would fail closed on an IPv6-literal GitLab API root before GET /job even runs.
    Remediation: Rely on --globoff (and the existing brace rejection) for anti-globbing; allow bracket characters specifically when they form a valid IPv6 literal host, or parse the URL and reject glob metacharacters only in a non-IPv6 hostname position.

  • [secret-exposure] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/pin-ci-job-identity.sh:126 — fullsend_gate_curl strips proxy env vars, CURLRC, and CURL_HOME, and the pin unsets SSL_CERT_FILE/GIT_SSL_CAINFO/CURL_CA_BUNDLE/REQUESTS_CA_BUNDLE/NODE_EXTRA_CA_CERTS before re-sourcing trust-ci-server-ca.sh, but neither path unsets SSL_CERT_DIR or SSLKEYLOGFILE. When CI_SERVER_TLS_CA_FILE is unset (the common case on gitlab.com), gate curls skip --cacert, so OpenSSL could honor an attacker-supplied SSL_CERT_DIR. Exploiting this still requires placing a hashed CA file at a path the attacker can also write to before this script runs; under ADR 0125's stated threat model (attacker controls pipeline/trigger CI/CD variables only, not job script content or filesystem writes, since the scaffold scripts are fullsend-committed), that extra precondition makes a pure variable-only MITM impractical — this is a completeness gap relative to the other TLS/CA env vars already handled, not a readily exploitable hole.
    Remediation: For defense-in-depth, add SSL_CERT_DIR and SSLKEYLOGFILE to the stripped-env list in fullsend_gate_curl, and unset them in the pin before sourcing trust-ci-server-ca.sh (leaving them unset for the subsequent fullsend poll/run invocations too).

  • [auth-bypass] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/run-agent-job.sh:105 — After a successful identity pin, several PAT-bearing fullsend_gate_curl calls still interpolate CI_API_V4_URL as the host instead of FULLSEND_PINNED_GITLAB_URL, while correctly using pinned project/pipeline/MR ids in the path. Confirmed sites: run-agent-job.sh resource-group PUT (105), pipeline re-fetch (121), /user (128, 467, 550, 639, 659), members (332), notes (479, 671), MR GET (755), users (790), commits (830); run-poll-job.sh resource-group PUT (~88). This differs from the class of issue ~4 prior review rounds already fixed at other call sites (--gitlab-url/GITLAB_ISSUE_URL/GITLAB_MR_URL, which used an independently-overridable FULLSEND_GITLAB_URL/CI_SERVER_URL): these remaining curls target CI_API_V4_URL, which is exactly the URL the pin already validated and used for GET /job — FULLSEND_PINNED_GITLAB_URL is derived from that same value. Same-process env does not mutate CI_API_V4_URL after the pin succeeds, so this is incomplete "use the pinned value everywhere" hygiene relative to the helper's stated contract rather than an independent new host-redirect vector.
    Remediation: Route every post-pin fullsend_gate_curl URL through FULLSEND_PINNED_GITLAB_URL (plus /api/v4) instead of CI_API_V4_URL, failing closed if FULLSEND_PINNED_GITLAB_URL is empty, for consistency with the pin's stated contract.

  • [intent-alignment] internal/repos/gitlab_pipeline_var_restriction.go:27 — GitLabPipelineVarRestrictionEnforced() returns true only when FULLSEND_GITLAB_PIPELINE_VAR_RESTRICTION equals "enforced"; the default remains report-only. This is an honest, documented deferral (code comments, docs/contributing/gitlab-role-credentials.md:308-326, ADR 0125/ADR 0125: in-job security gate hardening (agent + poll scripts) #7769) pending a separate Owner-identity decision for the poller PAT — not a silent regression, since before this PR the setting was entirely unmanaged. Issue ADR 0125: in-job security gate hardening (agent + poll scripts) #7769's acceptance criteria still call the ci_pipeline_variables_minimum_override_role=owner restriction "a required control, not defense-in-depth" and include a live-verification "Ship gate," neither satisfied by this PR. This round's HEAD commit message correctly says "Relates to ADR 0125: in-job security gate hardening (agent + poll scripts) #7769," but the PR body (fetched via the GitHub API, which drives GitHub's closing-keyword auto-close on merge) still ends with "Closes ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" and cites "(GitLab service-account lifecycle + webhook fast-path via fresh-identity poller handoff (ADR 0125) #7772)" for the install-time restriction — GitLab service-account lifecycle + webhook fast-path via fresh-identity poller handoff (ADR 0125) #7772 is unrelated webhook/trigger-token provisioning; the forge primitive landed in GitLab forge primitive: read/set project ci_pipeline_variables_minimum_override_role #7798/PR refactor(#7798): add GitLab pipeline-variable override-role methods #7813, and the install/converge orchestration is this PR (fix(#7769): pin GitLab job identity to CI_JOB_TOKEN record #7817) itself. In-repo docs already avoid the incorrect #7772 citation; only the PR body metadata is wrong. Merging as-is will auto-close ADR 0125: in-job security gate hardening (agent + poll scripts) #7769 before its acceptance criteria and ship gate are satisfied. This finding (and its facts) are unchanged across roughly 11 prior review rounds; the fix agent has repeatedly stated it lacks tool access to edit the PR title/description directly.
    Remediation: A human operator must edit the PR description via the forge UI to (a) replace "Closes ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" with "Relates to ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" (or "Part of ADR 0125: in-job security gate hardening (agent + poll scripts) #7769") so ADR 0125: in-job security gate hardening (agent + poll scripts) #7769 stays open until the Owner-credential decision and live-verification ship gates are met, and (b) correct or remove the "(GitLab service-account lifecycle + webhook fast-path via fresh-identity poller handoff (ADR 0125) #7772)" citation.

  • [missing-doc] docs/cli/repos.md:82 — This PR adds FULLSEND_GITLAB_PIPELINE_VAR_RESTRICTION (internal/repos/gitlab_pipeline_var_restriction.go), gating whether install/converge enforces ci_pipeline_variables_minimum_override_role=owner or stays report-only. User-facing pages (docs/cli/repos.md:82,93, docs/guides/getting-started/operations.md:87,146) mention "GitLab pipeline-variable override-role inspection, report-only by default" but never name the env var, and operations.md's GitLab CI/CD / CLI variable tables omit it. The canonical contributing doc already documents the name, report-only default, Owner-credential trade-off, and how to set enforced (docs/contributing/gitlab-role-credentials.md:308-326), so this is a discoverability gap on the user-facing CLI/ops pages rather than a missing explanation of the control itself.
    Remediation: Name FULLSEND_GITLAB_PIPELINE_VAR_RESTRICTION in docs/cli/repos.md and in the relevant variable table(s)/an operational note in docs/guides/getting-started/operations.md, pointing at gitlab-role-credentials.md for the full trade-off explanation.

  • [doc-style] docs/cli/repos.md:82 — A comma (rather than the semicolon used on line 93 and in operations.md:87) precedes the newly added "GitLab pipeline-variable override-role inspection, report-only by default" clause, making it read as nested inside the preceding disabled-schedules qualification rather than as an independent list item.
    Remediation: Replace the comma preceding "GitLab pipeline-variable override-role inspection" on line 82 with a semicolon to match the delimiter style used elsewhere in the same list.

  • [backward-compatibility] internal/repos/gitlabci.go:80 — The new CI_DEBUG_TRACE deny-before-admit rule is added to the pipeline-scoped workflow:rules block and backfilled on converge for every already-enrolled repo where fullsend owns the workflow block, with no opt-out. A truthy CI_DEBUG_TRACE therefore refuses to create the entire pipeline, including any non-fullsend jobs in that same run — inherent to workflow:rules and required for the guard (secrets materialize at job init; it cannot be job-scoped), so this is not a code defect. The guides already describe workflow:rules as an allowlist that can stop ordinary pipelines, but do not explicitly say a debug-trace variable set anywhere in a mixed fullsend-owned pipeline will also skip unrelated jobs.
    Remediation: Add a short note to docs/guides/getting-started/configuring-gitlab.md / docs/contributing/gitlab-role-credentials.md clarifying that a truthy CI_DEBUG_TRACE skips the whole pipeline run when fullsend owns workflow:rules, not only fullsend's own jobs. Not observed to need a feature flag — unlike the Owner-gated override-role restriction, there is no credential-cost reason to gate this behind an env var.

  • [backward-compatibility] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/pin-ci-job-identity.sh — This script tightens the pipeline-ref requirement from "any protected ref" (the pre-existing CI_COMMIT_REF_PROTECTED=="true" check) to "must equal the project's exact default branch" — matching ADR 0125/ADR 0125: in-job security gate hardening (agent + poll scripts) #7769's stated contract, so this is intended, not accidental. Content-drift repair (convergeContentDriftFiles) will add this script to every already-enrolled repo on the next repos install/converge run. If any enrolled repo's fullsend dispatch pipelines run against a protected branch other than the registered GitLab default branch, jobs will start failing closed at runtime with no advance drift/status signal calling out the specific incompatibility. This same observation was raised in an earlier round of this PR's review history and has not been addressed since; it is not required to ship the pin.
    Remediation: Consider having repos status/repos install cross-check the manifest's assumed default branch against the live GitLab project's default_branch before rollout, surfacing a warning similar to the report-only pattern used for the pipeline-variable override role, so the incompatibility is visible to operators ahead of a runtime failure. Not blocking.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (5)

Review

Findings

Low

Previous run (6)

Review

Findings

Low

  • [fail-open] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/run-agent-job.sh:447 — This job admits source=api only, so GitLab never natively populates CI_MERGE_REQUEST_IID, and only STATUS_IID is bound into HMAC_MESSAGE. When a non-zero STATUS_IID is present, a disagreeing CI_MERGE_REQUEST_IID now correctly fails closed (lines 439-445) — this fixes the prior round's finding. But when STATUS_IID is empty or "0", MR_IID still falls through to the unsigned CI_MERGE_REQUEST_IID at line 447 for the STAGE=review prior-review lookup (queries the Notes API with a role PAT), and the same empty-STATUS_IID fallback recurs at GITLAB_ISSUE_URL construction (~line 392), the shared code|fix|review MR_IID block (line 587), and --status-number ("${MR_NUMBER:-${STATUS_IID:-${CI_MERGE_REQUEST_IID:-0}}}", line 902). A project/group/pipeline CI/CD variable named CI_MERGE_REQUEST_IID can therefore still key a same-project Notes read, status post, or fix-checkout target on a dispatch whose signed STATUS_IID is empty, without that value being in HMAC_MESSAGE — the same class of outrankable-variable risk this file already refuses to trust for CI_MERGE_REQUEST_TARGET_BRANCH_NAME.
    Remediation: On this api-only job, do not use CI_MERGE_REQUEST_IID as an MR identity source even when STATUS_IID is empty. Treat a non-empty CI_MERGE_REQUEST_IID with empty/zero STATUS_IID as fail-closed disagreement (or ignore it and keep MR_IID=0) at all four sites (lines 447, ~392, 587, 902).

  • [intent-alignment] internal/repos/gitlab_pipeline_var_restriction.go:27 — GitLabPipelineVarRestrictionEnforced() returns true only when FULLSEND_GITLAB_PIPELINE_VAR_RESTRICTION equals "enforced"; the default is report-only. Issue ADR 0125: in-job security gate hardening (agent + poll scripts) #7769's acceptance criteria designate the ci_pipeline_variables_minimum_override_role=owner restriction as "a required control, not defense-in-depth" — this is a deliberate, documented deferral (pending an Owner-identity decision for the poller/dispatcher PAT, tracked in ADR 0125: in-job security gate hardening (agent + poll scripts) #7769), not a silent regression. However, the PR body still ends with "Closes ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" and states the restriction "is applied at install time (GitLab service-account lifecycle + webhook fast-path via fresh-identity poller handoff (ADR 0125) #7772)", while the PR's latest commit message (a861bb7) deliberately uses "Relates to ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" instead, explicitly noting the fix agent lacks tool access to edit the PR description and flagging this as a disagreement for a human to resolve. Merging with the current PR body will auto-close ADR 0125: in-job security gate hardening (agent + poll scripts) #7769 via GitHub's closing-keyword mechanism before the required-control and live-verification ("Ship gate") acceptance criteria are satisfied.
    Remediation: Update the PR description to replace "Closes ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" with "Relates to ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" (or "Part of ADR 0125: in-job security gate hardening (agent + poll scripts) #7769"), matching the latest commit message, so ADR 0125: in-job security gate hardening (agent + poll scripts) #7769 stays open until the Owner-credential decision and live verification ship gates are met.

  • [backward-compatibility] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/pin-ci-job-identity.sh:251 — This script now fails closed unless the CI_JOB_TOKEN job's ref equals the project's default branch, tighter than the pre-existing CI_COMMIT_REF_PROTECTED=="true" gate (any protected ref). Workflow/job rules still admit any protected ref, so a poll/agent job dispatched against a protected non-default ref (e.g. a release branch) would start and then abort in this pin script once a converge rolls the new scripts out. This matches the ADR 0125 / issue ADR 0125: in-job security gate hardening (agent + poll scripts) #7769 contract ("ref must equal the enrolled protected default branch, not merely any protected ref") and converge.go's existing schedule/pipeline creation already targets the default branch, so this is the intended, documented trust contract rather than an accidental incompatibility with the supported install path — but there is no converge-time check that surfaces, rather than silently breaks, an already-enrolled repo where the assumption does not hold.
    Remediation: If any enrolled repo is known to dispatch fullsend jobs against a protected non-default ref, call that out in release notes before rolling this out fleet-wide. A converge pre-flight/report step that surfaces such repos (rather than only failing at job runtime) would make the migration risk visible to operators, though it is not required to ship the pin.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (7)

Review

Findings

Low

  • [logic-error] internal/repos/gitlabci.go:321 — MergeMissingGitLabDebugTraceRule (and the shared mergeWorkflowRules dedup it uses) treats the CI_DEBUG_TRACE deny-before-admit rule as already present whenever any existing workflow.rules entry has an if: textually equal to debugTraceDenyRuleIf, regardless of when: or rule order. GitLab's default when: (when omitted) is always, not never, and rules are first-match — so a same-if: rule with the wrong when:, or one positioned after an admit rule, would be left untouched by the backfill. The common path this PR ships (rule fully absent → prepend with when: never) is correct and tested; the gap is the same-if:/wrong-when:/wrong-order case, which the current tests do not cover.
    Remediation: Require if: equal to debugTraceDenyRuleIf and when: equal to never (and, for the backfill path, that the rule precedes any admit rule) before treating the guard as satisfied, in both MergeMissingGitLabDebugTraceRule and mergeWorkflowRules. Add regression tests for an if:-matching rule with the wrong when:, and for a correctly-shaped rule appearing after an admit rule.

  • [fail-open] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/run-agent-job.sh:425 — The prior round's GITLAB_ISSUE_URL/GITLAB_MR_URL fail-open finding is fixed elsewhere in this file. The STAGE=review prior-review-lookup block still does the opposite at line 425: MR_IID="${CI_MERGE_REQUEST_IID:-${STATUS_IID:-0}}" prefers the unsigned CI_MERGE_REQUEST_IID with no disagreement check before using it to query the GitLab Notes API with a role PAT. HMAC verification of STATUS_IID has already passed by this point, and the later shared code|fix|review block re-resolves MR_IID with the fail-closed, STATUS_IID-preferred check, so a disagreeing unsigned IID cannot reach the agent — but this site still performs a same-project Notes read keyed on the unsigned value first, reintroducing the class of gap this PR otherwise systematically closes in the same file. No existing test covers this lookup site.
    Remediation: Resolve MR_IID for the STAGE=review prior-review lookup using the same STATUS_IID-preferring, fail-closed cross-check against CI_MERGE_REQUEST_IID already used for GITLAB_ISSUE_URL and the shared code|fix|review block, before making the Notes API call.

  • [intent-alignment] internal/repos/gitlab_pipeline_var_restriction.go:27 — The GitLab-side ci_pipeline_variables_minimum_override_role=owner restriction — which issue ADR 0125: in-job security gate hardening (agent + poll scripts) #7769's own acceptance criteria call "a required control, not defense-in-depth" — ships report-only by default (GitLabPipelineVarRestrictionEnforced() is true only on the exact string enforced; the CLI wrapper returns nil after a StepWarn log line rather than structured drift). This is a deliberate, documented deferral pending a separate decision to raise the poller identity to Owner (tracked in ADR 0125: in-job security gate hardening (agent + poll scripts) #7769), not a silent regression. Separately, pin-ci-job-identity.sh's header comment says the restriction "is applied at install/converge time" without the report-only caveat, and the PR body still ends with "Closes ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" even though this PR's own latest commit message explicitly says "Relates to ADR 0125: in-job security gate hardening (agent + poll scripts) #7769 ... Not using a closing keyword here so merging this PR does not prematurely close ADR 0125: in-job security gate hardening (agent + poll scripts) #7769 before those criteria are satisfied." The PR body and the commit message disagree, and merging via the PR body's closing keyword would auto-close ADR 0125: in-job security gate hardening (agent + poll scripts) #7769 while the required-control and foreign-job-token live-verification acceptance criteria remain open. This class of finding has recurred across many prior review rounds on this PR.
    Remediation: Edit the PR body to say "Relates to ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" or "Part of ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" (matching the latest commit message), consistent with the still-open acceptance criteria. Consider surfacing the report-only restriction state as structured drift rather than only a log line, and soften pin-ci-job-identity.sh's "applied at install/converge time" wording to note the report-only default.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (8)

Review

Findings

Medium

  • [fail-open] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/run-agent-job.sh:374 — After pin-ci-job-identity.sh exports FULLSEND_PINNED_GITLAB_URL and FULLSEND_PINNED_PROJECT_PATH, GITLAB_ISSUE_URL is still built from CI_SERVER_URL/CI_PROJECT_PATH (lines 374, 380) instead of the pinned equivalents. For non-issue events it also prefers the unsigned CI_MERGE_REQUEST_IID over the HMAC-signed STATUS_IID (_fs_mr_iid="${CI_MERGE_REQUEST_IID:-${STATUS_IID:-}}") when building that URL — the opposite precedence from the later code|fix|review block, which prefers STATUS_IID and fails closed on disagreement (lines 534-545). GITLAB_MR_URL is likewise built from CI_SERVER_URL/CI_PROJECT_PATH (line 551). Authenticated fullsend run API targeting is already correctly pinned (--status-repo uses FULLSEND_PINNED_PROJECT_PATH, FULLSEND_GITLAB_URL is exported from FULLSEND_PINNED_GITLAB_URL), so this affects only the harness "entity URL" env vars, not credential-bearing calls — but it reintroduces exactly the class of gap this PR otherwise systematically closes elsewhere in the same file.
    Remediation: Build GITLAB_ISSUE_URL and GITLAB_MR_URL from FULLSEND_PINNED_GITLAB_URL and FULLSEND_PINNED_PROJECT_PATH instead of CI_SERVER_URL/CI_PROJECT_PATH. For the IID used in GITLAB_ISSUE_URL, prefer STATUS_IID (with the same fail-closed cross-check against a disagreeing CI_MERGE_REQUEST_IID used in the code|fix|review block) rather than letting CI_MERGE_REQUEST_IID win.

Low


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (9)

Review

Findings

Low

  • [misleading-status-reporting] internal/repos/gitlab_pipeline_var_restriction.go:80 — EnsureGitLabPipelineVariableOverrideRole reports Action: "none" both when ci_pipeline_variables_minimum_override_role is already owner and when active enforcement is off (the FULLSEND_GITLAB_PIPELINE_VAR_RESTRICTION=enforced default) and the role is not owner. This is not a fail-open regression — the setting was entirely unmanaged before this commit, the report-only default is an explicit, documented interim step blocked on a separate poller-credential-elevation decision still tracked in ADR 0125: in-job security gate hardening (agent + poll scripts) #7769 (code comments, gitlab_pipeline_var_restriction.go:12-20,51-61, repos_gitlab.go:406-413, docs/contributing/gitlab-role-credentials.md:321-333), Get/Set API errors still fail the install, and the write itself is fail-closed on the exact string "enforced". Operators aren't fully blind either: ensureGitLabPipelineVariableOverrideRole prints res.Detail via StepDone, and the report-only detail message already states the role is not owner and that enforcement is disabled pending ADR 0125: in-job security gate hardening (agent + poll scripts) #7769. The remaining nit is that both the already-compliant and the not-yet-enforced paths render as the same green Action/StepDone status, so a skimming operator can mistake "control not yet applied" for "control applied."
    Remediation: Consider using a distinct Action value (e.g. "report-only") and printer.StepWarn when the role is non-owner and enforcement is disabled, reserving the green success path for an actually-owner project.

  • [intent-alignment] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/pin-ci-job-identity.sh:43 — The PR body uses "Closes ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" while two acceptance-criteria items remain explicitly unfinished and are documented as such in the code itself: (1) the live GitLab-version Ship gate for a valid foreign job token is deferred until a second job token can be minted (tracked under GitLab webhook: dispatcher CI scaffold for signed pipeline-input dispatch #7771; script header at lines 43-48), with only the invalid-token fail-closed path covered by TestPinCIJobIdentity_InvalidJobTokenFailsClosed; (2) ci_pipeline_variables_minimum_override_role=owner — the issue's own acceptance criteria call this "a required control, not defense-in-depth" — ships as report-only by default in this PR's newest commit (internal/repos/gitlab_pipeline_var_restriction.go), because enforcing it today would break the Developer-level poller/dispatcher's own CreatePipeline call until the poller identity is separately raised to Owner (the same open credential-cost decision ADR 0125: in-job security gate hardening (agent + poll scripts) #7769 still tracks). Merging with "Closes ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" auto-closes the issue that the code and gitlab-role-credentials.md still point back to for this remaining work. This has recurred across prior review rounds; per this round's commit history, the fix-agent explicitly recorded leaving the "Closes ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" wording unchanged, treating it as an issue-tracking question rather than a code defect — the tracking mismatch is real but this is not a product regression in the diff.
    Remediation: Change the PR closer to "Relates to ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" or "Part of ADR 0125: in-job security gate hardening (agent + poll scripts) #7769", or split the remaining Ship-gate and active-enforcement acceptance criteria onto explicit follow-up issues (GitLab webhook: dispatcher CI scaffold for signed pipeline-input dispatch #7771 and a poller-Owner-credential issue) and update ADR 0125: in-job security gate hardening (agent + poll scripts) #7769 accordingly before merge.

  • [missing-doc] docs/cli/repos.md:93 — The repos install component-drift parenthetical (workflow, thin callers, variables, secrets, pipeline schedules, GitLab poller protected-ref pipeline access), repeated in docs/cli/repos.md, docs/guides/getting-started/repo-management.md (~319, ~407), and docs/guides/getting-started/operations.md (88, 147), does not mention the new GitLab ci_pipeline_variables_minimum_override_role inspect/report-only-by-default step this PR's newest commit adds. FULLSEND_GITLAB_PIPELINE_VAR_RESTRICTION itself is documented in docs/contributing/gitlab-role-credentials.md; these curated lists are not exhaustive (they already omit several other ensure* steps), so this is a minor completeness gap rather than an incorrect claim.
    Remediation: If these lists are meant to track GitLab-specific ensure* install steps of the same class as protected-ref pipeline access, add a brief mention of the pipeline-variable override-role inspection/report-only drift check to the repos install reference and guides.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (10)

Review

Findings

Low

The two prior-round doc findings — incomplete converge-migration documentation in docs/guides/getting-started/operations.md and imprecise CI_DEBUG_TRACE truthy-value phrasing in docs/contributing/gitlab-role-credentials.md — are resolved in this diff: both files now accurately describe the CI_DEBUG_TRACE deny-before-admit backfill (including the merge-path-enrollment gap) and the gitlab-runner ParseBool truthy set, matching the implementation in internal/repos/converge.go, internal/repos/gitlabci.go, and the script-level guards.

Previous run (11)

Review

Findings

Low

  • [intent-alignment] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/pin-ci-job-identity.sh:43 — Issue ADR 0125: in-job security gate hardening (agent + poll scripts) #7769's acceptance criteria include an explicit "Ship gate" requiring live verification on the target GitLab version that a CI_JOB_TOKEN override fails closed and that GET /api/v4/job does not mis-resolve with a valid foreign token, then reconciling ADR 0125 / normative claims with the result. The invalid-override fail-closed path is covered by a mocked unit test, not a live GitLab-version check, and this script's own header comment (lines 43-51) states the valid-foreign-token case is deferred to follow-up work once a second job token can be minted (tracked under GitLab webhook: dispatcher CI scaffold for signed pipeline-input dispatch #7771). The PR body says "Closes ADR 0125: in-job security gate hardening (agent + poll scripts) #7769", which will auto-close the issue on merge before its Ship gate criterion is satisfied or formally re-scoped.
    Remediation: Change "Closes ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" to "Relates to ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" or "Part of ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" in the PR description so the issue stays open until the Ship gate verification is completed, or formally move that acceptance criterion onto GitLab webhook: dispatcher CI scaffold for signed pipeline-input dispatch #7771 and update ADR 0125: in-job security gate hardening (agent + poll scripts) #7769 before merge.

  • [incomplete-doc] docs/guides/getting-started/operations.md:88 — The converge paragraph documents two root .gitlab-ci.yml migrations converge performs (stripping the obsolete merge_request_event workflow rule; stripping the leftover empty dispatch stage) but was not updated for the third migration this PR adds: MergeMissingGitLabDebugTraceRule (internal/repos/converge.go:1598, internal/repos/gitlabci.go:266-288), which backfills the CI_DEBUG_TRACE deny-before-admit workflow rule for already-enrolled repos where fullsend owns the workflow: block. The backfill is automatic for fullsend-owned blocks; for merge-path enrollments the same paragraph already says are left untouched by converge, operators aren't told they also need to add this new deny rule manually.
    Remediation: Add a sentence to the converge paragraph noting that converge also backfills the missing CI_DEBUG_TRACE deny-before-admit workflow rule for fullsend-owned workflow: blocks, and that merge-path enrollments need the same manual addition for this rule.

  • [imprecise-doc] docs/contributing/gitlab-role-credentials.md:281 — This PR's added prose states that YAML workflow: and job rules: "deny CI_DEBUG_TRACE=true before any admit rule". The actual rule and the script-level guards match any truthy value in gitlab-runner's ParseBool set (1, t/T, true/TRUE/True), not only the literal string "true". This is inconsistent with the more accurate "deny it before any admit rule" phrasing used elsewhere in the same document (around line 728).
    Remediation: Rephrase to state that the rules deny truthy CI_DEBUG_TRACE values (the gitlab-runner ParseBool truthy set), not the literal string "true".

The medium/low security findings from the prior review round (curl brace-globbing/CURLRC exposure, secrets on curl argv, checkout-mr-source.sh's exact-string CI_DEBUG_TRACE check) and the correctness finding about convergeGitLabRootCIFiles not backfilling the CI_DEBUG_TRACE workflow rule are not observed in the current diff.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (12)

Review

Findings

Medium

  • [auth-bypass] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/pin-ci-job-identity.sh:100 — fullsend_gate_curl is the transport for every JOB-TOKEN identity-pin call and every later PAT-bearing call, but it still runs curl with globbing enabled and without disabling curlrc. The bootstrap URL check only requires a bash glob of an https API root ending in /api/v4 (a case statement matching https://*/api/v4), which accepts brace-expansion hosts such as an https URL of the form https://{attacker.example,gitlab.example}/api/v4 — bash case's * matches any characters including literal {, }, ,. No later check (in this script or in run-agent-job.sh / run-poll-job.sh) rejects braces, userinfo, or extra path segments before the URL reaches curl. Curl performs URL-brace-globbing by default (--globoff is never passed), so it would request both hosts with the JOB-TOKEN header — leaking CI_JOB_TOKEN to an attacker HTTPS endpoint without faking GET /api/v4/job. Subsequent PAT curls interpolate the same raw CI_API_V4_URL, so the glob would also leak the role PAT after a successful pin. Proxy env stripping and --noproxy '*' do not cover this path, and CA pinning does not either (trust-ci-server-ca.sh concatenates the runner CA onto the system bundle, so a public-CA attacker host still verifies). CURLRC/CURL_HOME are also left in the environment.
    Remediation: Invoke curl with --disable first (so curlrc is ignored), add --globoff, and unset CURLRC/CURL_HOME alongside the proxy vars. Tighten the CI_API_V4_URL check to a hostname[:port] API root with no brace/bracket/wildcard/at-sign or userinfo characters, then send gate URLs from that validated value (or FULLSEND_PINNED_GITLAB_URL + /api/v4) rather than raw CI_API_V4_URL.

Low

  • [data-exposure] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/pin-ci-job-identity.sh:123 — Identity-pin and PAT gate curls pass secrets on curl's argv (the JOB-TOKEN and PRIVATE-TOKEN header values are interpolated directly into -H arguments), visible via /proc/<pid>/cmdline, ps, and execve audit logs. This PR's own checkout-mr-source.sh explicitly avoids passing the token on the command line for that reason and uses a credential helper plus env instead; the new pin helper (and the PAT calls now routed through it) does not follow that pattern. This is not a new regression versus base-branch run-agent-job.sh/run-poll-job.sh, which already used a PRIVATE-TOKEN header on curl, but it extends the pattern into new security-sensitive code in the same PR that just closed this class elsewhere.
    Remediation: Write the auth header to a 0600 temp file and pass it via curl's @file header-file syntax (or a netrc file) so the token is not on curl's command line; delete the file in a trap. Keep using env for resolve-mr-source as checkout-mr-source.sh already does.

  • [auth-bypass] internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/checkout-mr-source.sh:126 — This script still denies CI_DEBUG_TRACE only on the exact string "true". run-agent-job.sh and run-poll-job.sh were updated in this same PR to match gitlab-runner's full ParseBool truthy set (1, t/T, true/TRUE/True) via a case statement. checkout-mr-source.sh is sourced from run-agent-job.sh (fix stage) after that broader guard already ran, and run-poll-job.sh never sources this file, so this is a leftover third-layer hole rather than a live bypass — but it is on the credentialed git-fetch path and was one of only four files touched since the prior review round. Tests only assert the exact-string true case.
    Remediation: Replace the exact-string check with the same case pattern (matching 1, t/T, true/TRUE/True) used in run-agent-job.sh / run-poll-job.sh, and extend checkout_mr_source_test.go to cover those truthy variants.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR

fullsend-ai-review[bot]

This comment was marked as outdated.

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 1:57 AM UTC · Completed 2:18 AM UTC

Commit: e48480c · View workflow run →

Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $5.63

fullsend-ai-coder Bot added a commit that referenced this pull request Sep 29, 2026
…overage

Address review feedback on PR #7817:
- Route every PAT-bearing curl call in run-agent-job.sh/run-poll-job.sh
  through fullsend_gate_curl instead of plain curl, so a trigger-supplied
  HTTP_PROXY/HTTPS_PROXY cannot intercept the role PAT on those calls
  (only the identity-pin's own JOB-TOKEN calls were previously routed
  through the proxy-stripping, CA-pinned helper).
- Unset fullsend_gate_curl's curl-args temporary on every return,
  matching the sourced script's convention for its other temporaries.
- Document that fullsend_gate_curl is transport-only and reused by
  callers for PAT-bearing calls after the pin succeeds.
- Add pin-ci-job-identity.sh tests asserting fail-closed behavior when
  the project/branch/pipeline GET routes return a non-200 status.
- Add a run-agent-job.sh test for the user-less job-token pipeline
  payload path, asserting the PAT re-fetch uses the pinned project/
  pipeline IDs rather than the overridable CI_PROJECT_ID/CI_PIPELINE_ID.
- Move the Run*JobScript_* integration tests into gitlab_job_scripts_test.go
  to consolidate with the rest of that script-level coverage.
- Note the new CI_DEBUG_TRACE deny-before-admit workflow rule in the
  GitLab manual-uninstall and install-behavior docs, which omitted it.

Addresses #7817
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 1 (bot-triggered)

Routed every remaining PAT-bearing curl call in the GitLab poll/agent job scripts through the proxy-stripping, CA-pinned fullsend_gate_curl helper; fixed the curl-args temp-var leak in that helper; added fail-closed test coverage for non-200 project/branch/pipeline GET responses and for the user-less pipeline PAT re-fetch path; consolidated the new integration tests into the existing test file; and updated the GitLab uninstall/install docs to mention the new CI_DEBUG_TRACE deny rule. Declined to speculatively rework the identity-pin trust model or broaden the CI_DEBUG_TRACE match without live GitLab/gitlab-runner verification, and declined to edit the PR's closing-keyword wording as out of scope for a branch-code fix.

Fixed (7):

  1. secret-exposure: PAT-bearing curl calls in run-agent-job.sh/run-poll-job.sh bypass the proxy-stripping, CA-pinned gate curl (internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/run-agent-job.sh): Routed every remaining PAT-bearing curl call in run-agent-job.sh (resource-group self-heal PUT, /user GET, members lookup, notes pagination, target-branch lookup, trigger-source lookup, fix-iteration commit count) through fullsend_gate_curl instead of plain curl, so a trigger-supplied HTTP_PROXY/HTTPS_PROXY can no longer intercept these role-PAT calls. Applied the same change to run-poll-job.sh's resource-group PUT.
  2. missing-test: no test exercises the user-less job-token pipeline payload / PAT re-fetch path, and no pin test covers a rejected project/branch/pipeline GET (internal/scaffold/gitlab_job_scripts_test.go): Added TestRunAgentJobScript_UserlessPipelineRefetchesWithPinnedIDs, which mocks a user-less job-token pipeline payload and asserts the PAT re-fetch in run-agent-job.sh keys off the pinned project/pipeline IDs (not the overridable CI_PROJECT_ID/CI_PIPELINE_ID). Added TestPinCIJobIdentity_ProjectLookupFailsClosed, TestPinCIJobIdentity_BranchLookupFailsClosed, and TestPinCIJobIdentity_PipelineLookupFailsClosed to pin_ci_job_identity_test.go, each asserting fail-closed behavior when the corresponding JOB-TOKEN GET route returns a non-200 status.
  3. error-handling-idiom: fullsend_gate_curl's _fs_curl_args array is never unset, unlike the script's other fs temporaries* (internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/pin-ci-job-identity.sh): fullsend_gate_curl now captures curl's exit status into a local variable, unsets _fs_curl_args, and returns the saved status, so the curl-flags array no longer leaks into the caller's shell after the function returns.
  4. api-shape: header comment says only JOB-TOKEN is ever sent by fullsend_gate_curl, but run-agent-job.sh already reuses it with a PAT (internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/pin-ci-job-identity.sh): Reworded the header comment to clarify that fullsend_gate_curl is transport-only (it doesn't set an auth header) and is deliberately reused by callers for PAT-bearing calls after the pin succeeds, pointing to run-agent-job.sh/run-poll-job.sh for that pattern -- consistent with also extending that reuse to the remaining PAT curl calls in this fix.
  5. code-organization: new RunJobScript_ integration tests were added to pin_ci_job_identity_test.go instead of gitlab_job_scripts_test.go, splitting related coverage (internal/scaffold/pin_ci_job_identity_test.go): Moved TestRunPollJobScript_DeniesNonSchedulePinnedSource, TestRunAgentJobScript_DeniesParentPipelinePinnedSource, TestRunAgentJobScript_DebugTraceAbortsBeforePin, and TestRunPollJobScript_DebugTraceAbortsBeforePin from pin_ci_job_identity_test.go into gitlab_job_scripts_test.go, alongside the existing run-agent-job.sh/run-poll-job.sh integration tests.
  6. incomplete-doc: GitLab manual-uninstall instructions omit the new CI_DEBUG_TRACE deny-before-admit workflow rule (docs/guides/getting-started/operations.md): Added the CI_DEBUG_TRACE deny-before-admit rule to the manual-removal workflow-rules list alongside schedule/api/merge_request_event, and to the install-behavior Note describing which workflow rules fullsend adds/writes.
  7. incomplete-doc: operations.md's install-behavior Note and configuring-gitlab.md's equivalent prose both describe the installed workflow rules as only protected-ref schedule/api rules, omitting the new CI_DEBUG_TRACE deny rule (docs/guides/getting-started/configuring-gitlab.md): Updated both operations.md's install-behavior Note and configuring-gitlab.md's workflow:/auto_cancel section to mention the CI_DEBUG_TRACE deny-before-admit rule that fullsend now also installs ahead of the protected-ref schedule/api rules.

Disagreed (3):

  1. logic-error: pin-ci-job-identity.sh fail-closes on three JOB-TOKEN REST calls (Projects/Branches/Pipelines GET) whose acceptance on the target GitLab version is unverified: Added the missing fail-closed test coverage for non-200 responses on all three routes (see the missing-test action above), which was the concretely actionable part of this finding. Whether GitLab's documented CI_JOB_TOKEN-authenticated endpoint allowlist actually covers Projects/Branches/Pipelines GET on the target GitLab version is a live-platform question this sandboxed fix run has no way to verify (no access to a real GitLab instance); the PR body already tracks equivalent live-verification work as a Ship-gate follow-up. Recommend confirming against the target GitLab version during rollout/canary rather than reworking the identity model speculatively.
  2. fail-open: CI_DEBUG_TRACE guards only match the exact string "true"; other truthy values gitlab-runner might accept are unverified: Whether gitlab-runner parses CI_DEBUG_TRACE more permissively than an exact "true" match is external platform behavior that cannot be verified without a live gitlab-runner instance, which this sandboxed fix run does not have access to. Broadening the match to an unconfirmed truthy set risks either missing the real set or over-matching legitimate non-boolean values. GitLab's own CI/CD variable documentation treats this as a boolean compared with "true"/"false", matching the current implementation. Recommend confirming against the target gitlab-runner version during rollout, the same live-verification pattern this PR already uses for the valid-foreign-token case.
  3. intent-alignment: PR states "Closes ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" while the Ship-gate's live-verification acceptance criterion is explicitly deferred to GitLab webhook: dispatcher CI scaffold for signed pipeline-input dispatch #7771: This is a PR-description/issue-tracking wording choice, not a code change. Editing the PR body/closing keyword is forge metadata management outside a fix agent's authorized scope (branch code changes only); the post-script and PR mutations are handled separately. Flagging for the maintainer to decide between completing the deferred verification before closing ADR 0125: in-job security gate hardening (agent + poll scripts) #7769 or changing the keyword to "Relates to ADR 0125: in-job security gate hardening (agent + poll scripts) #7769".

Tests: passed

CI inspections (14):

  1. CI / test (passing, success)
  2. CI / web (passing, success)
  3. CI / build (passing, success)
  4. CI / commit-lint (passing, success)
  5. CI / test-sandbox-darwin (passing, success)
  6. Functional Tests / functional-tests (passing, success)
  7. Functional Tests / gate (passing, success)
  8. E2E Tests / behaviour (passing, success)
  9. E2E Tests / e2e (passing, success)
  10. E2E Tests / gate (passing, success)
  11. Vouch Check / vouch-gate (passing, success)
  12. Build Site / build (passing, success)
  13. codecov/patch (passing, success)
  14. DCO (passing, success)
Decision points
  • Did not attempt to verify GitLab's CI_JOB_TOKEN endpoint allowlist or gitlab-runner's CI_DEBUG_TRACE parsing against a live GitLab instance (alternatives: Rework the identity pin to take default_branch/protected/source from a PAT-authenticated call instead of JOB-TOKEN, Broaden the CI_DEBUG_TRACE match to a guessed truthy set (e.g. 1/yes/on) across YAML rules and script guards, Add the missing fail-closed test coverage and defer the external-platform-behavior questions to a live rollout check; rationale: Both open items depend on unverifiable external platform behavior (GitLab API version, gitlab-runner variable parsing) that this sandboxed fix run cannot check. Speculative code changes based on unconfirmed assumptions risk introducing new correctness issues without actually resolving the reviewer's uncertainty; the PR already has an established pattern (the valid-foreign-token case) of deferring this class of verification to a live rollout/canary step.)

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 2:20 AM UTC · Completed 2:41 AM UTC

Commit: f0a1d74 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $8.28

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 2:43 AM UTC · Completed 3:08 AM UTC

Commit: f0a1d74 · View workflow run →

Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $9.09

fullsend-ai-coder Bot added a commit that referenced this pull request Sep 29, 2026
… PAT calls to job-record project, backfill debug-trace rule on converge

Every deny/admit layer for the debug-trace guard matched only the exact
string "true", missing the truthy variants gitlab-runner also accepts
(Go strconv.ParseBool: "1", "t"/"T", "true"/"TRUE"/"True"). Switch every
YAML rule and script-level guard this PR added/touched
(.gitlab-ci.yml, fullsend-pipeline.yml, fullsend-agent.yml,
fullsend-poll.yml, gitlabci.go's fullsendWorkflowRules/
unmergeWorkflowRules/newGitLabCI, run-agent-job.sh, run-poll-job.sh) to
a case-insensitive regex match, and add coverage for non-"true" truthy
values.

convergeGitLabRootCIFiles only stripped obsolete workflow rules; it
never backfilled the CI_DEBUG_TRACE deny rule onto already-enrolled
repos that go through converge (not the fresh-install merge path).
Add MergeMissingGitLabDebugTraceRule, gated on the same fullsend-
ownership check as the obsolete-rule strip, and wire it into
convergeGitLabRootCIFiles so enrolled repos pick up the guard.

Several PAT-bearing calls in run-agent-job.sh/run-poll-job.sh
(resource-group self-heal, Developer-access members lookup,
prior-review notes GETs, MR GET, commits GET) still interpolated the
overridable CI_PROJECT_ID/CI_MERGE_REQUEST_PROJECT_ID pipeline
variables instead of the project id pinned from the CI_JOB_TOKEN job
record. Route every one through FULLSEND_PINNED_PROJECT_ID, and fail
closed when CI_MERGE_REQUEST_PROJECT_ID disagrees with the pinned
project.

Addresses #7817
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 2 (bot-triggered)

Addressed the review's four findings on PR #7817 (GitLab in-job gate hardening / ADR 0125). Fixed the fail-open CI_DEBUG_TRACE exact-match gap across every YAML rule and shell guard this PR touches by switching to a case-insensitive regex matching gitlab-runner's full truthy set, with new tests for non-"true" truthy values. Fixed the logic-error gap where already-enrolled repos never received the debug-trace rule during converge by adding MergeMissingGitLabDebugTraceRule and wiring it into convergeGitLabRootCIFiles, with updated and new converge tests. Fixed the secret-exposure gap where several PAT-bearing calls used the overridable CI_PROJECT_ID/CI_MERGE_REQUEST_PROJECT_ID instead of the pinned project id, routing them through FULLSEND_PINNED_PROJECT_ID with a fail-closed cross-check and a new full-script regression test. Recorded a disagreement on the low-severity intent-alignment finding (PR closing-keyword mismatch) since it requires a PR-description edit outside this agent's mandate rather than a code change. All new and existing tests pass (go test -race for internal/repos and internal/scaffold), go vet and gofmt are clean, patch coverage on the two modified production Go files (gitlabci.go, converge.go) is at or above the 80% target, and a secret scan found no leaks.

Fixed (3):

  1. [fail-open] CI_DEBUG_TRACE guard only matched exact string "true" (internal/scaffold/fullsend-repo-gitlab/.gitlab-ci.yml): Every deny/admit layer matched only the exact string "true", missing truthy variants gitlab-runner also accepts (Go strconv.ParseBool: "1", "t"/"T", "true"/"TRUE"/"True"); a non-"true" truthy value would bypass every deny rule and admit the job while secrets still materialize at job init. Switched every deny/admit condition this PR added or touched to a case-insensitive regex ($CI_DEBUG_TRACE =~ /^(1|t|true)$/i) matching gitlab-runner's full truthy set: .gitlab-ci.yml, fullsend-pipeline.yml, fullsend-agent.yml, fullsend-poll.yml workflow/job rules; gitlabci.go's fullsendWorkflowRules, unmergeWorkflowRules, and newGitLabCI's embedded template (introduced a shared debugTraceDenyRuleIf constant so all four stay in lockstep); and the script-level guards in run-agent-job.sh and run-poll-job.sh (case statement instead of exact string match). Added TestRunPollJobScript_DebugTraceAbortsOnTruthyVariants and TestRunAgentJobScript_DebugTraceAbortsOnTruthyVariants covering "1" and "TRUE" as required by the finding's remediation, plus updated existing Go test fixtures/assertions that hardcoded the old exact-match literal.
  2. [logic-error] converge never backfills the CI_DEBUG_TRACE rule for already-enrolled repos (internal/repos/gitlabci.go): The CI_DEBUG_TRACE deny-before-admit workflow rule was only applied through MergeGitLabCI (the fresh-install/under-migrated path); convergeGitLabRootCIFiles (the periodic upgrade path for already-enrolled repos) never called mergeWorkflowRules/MergeGitLabCI, so an already-enrolled repo's root .gitlab-ci.yml never got the guard backfilled, leaving a trigger-forged pipeline with CI_DEBUG_TRACE set able to start and run other jobs under debug tracing. Added MergeMissingGitLabDebugTraceRule, gated on the same gitlabCIWorkflowIsFullsendOwned ownership check StripObsoleteGitLabWorkflowRules uses so a user's own hand-written workflow: block is never touched, and wired it into convergeGitLabRootCIFiles (internal/repos/converge.go) as a new gitlab-ci-rules step alongside the existing obsolete-rule strip. Updated TestConverge_GitLab_NoObsoleteWorkflowRuleNoAction (its fixture was missing the debug-trace rule) to assert the backfill now happens, added TestConverge_GitLab_FullyMigratedNoAction to preserve coverage of the true no-op case, and added direct MergeMissingGitLabDebugTraceRule unit tests (adds-missing, already-present, not-fullsend-owned, no-workflow-block, empty-file).
  3. [secret-exposure] Several PAT-bearing calls use overridable CI_PROJECT_ID instead of the pinned project id (internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/run-agent-job.sh): Several PAT-bearing, project-scoped fullsend_gate_curl calls in run-agent-job.sh/run-poll-job.sh (agent resource-group self-heal PUT, Developer-access members lookup, prior-review notes GETs in the review and fix stages, the fix stage's MR GET, and its commits GET) still interpolated the overridable CI_PROJECT_ID / CI_MERGE_REQUEST_PROJECT_ID pipeline variables instead of the project id pinned from the CI_JOB_TOKEN job record, so on a GitLab configuration where pipeline/trigger variables outrank predefined CI_PROJECT_ID, these requests could send the role PAT to an attacker-overridden project. Routed every listed call through FULLSEND_PINNED_PROJECT_ID (from pin-ci-job-identity.sh) in both run-agent-job.sh and run-poll-job.sh's resource-group PUT. For the members lookup (the one call that previously fell back to CI_MERGE_REQUEST_PROJECT_ID), added an explicit fail-closed check that rejects a CI_MERGE_REQUEST_PROJECT_ID disagreeing with the pinned project id before using it, mirroring the existing CI_MERGE_REQUEST_IID/STATUS_IID cross-check pattern already in the script. Added TestRunAgentJobScript_UsesPinnedProjectIDForPATCalls, a full-script regression test that drives the script through identity pinning, bot-identity verification, and HMAC dispatch verification with a mock GitLab API server, asserting the resource-group PUT and members lookup hit the pinned project id (42) and never the deliberately-mismatched CI_PROJECT_ID (1). The notes/MR/commits calls (fix-stage-only, gated behind checkout-mr-source.sh's git checkout) received the same code fix but are not covered by an equivalent full end-to-end test in this run due to the added git-checkout scaffolding that would require; they are simple textual substitutions of the same CI_PROJECT_ID -> FULLSEND_PINNED_PROJECT_ID pattern already regression-tested elsewhere in this diff.

Disagreed (1):

  1. [intent-alignment] PR says "Closes ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" but defers part of its Ship-gate criterion: This is a valid observation about the PR description's closing keyword, not a defect in the shipped code — the invalid-override fail-closed case is unit-tested and the deferral (to future GitLab webhook: dispatcher CI scaffold for signed pipeline-input dispatch #7771 dispatcher work) is transparently documented. The fix agent does not edit PR titles/descriptions (that is forge metadata, not a file in the repository, and is outside this agent's file-editing/commit mandate — PR mutations belong to the post-script or a human). No code or doc-in-repo change applies. Recommend a human maintainer change "Closes ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" to "Relates to ADR 0125: in-job security gate hardening (agent + poll scripts) #7769"/"Part of ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" in the PR description, or complete the deferred live verification and reconcile ADR 0125 before merging.

Tests: passed

CI inspections (12):

  1. CI / test (passing, success)
  2. CI / web (passing, success)
  3. CI / commit-lint (passing, success)
  4. CI / test-sandbox-darwin (passing, success)
  5. Functional Tests / functional-tests (passing, success)
  6. Functional Tests / gate (passing, success)
  7. E2E Tests / e2e (passing, success)
  8. E2E Tests / behaviour (passing, success)
  9. E2E Tests / gate (passing, success)
  10. Build Site / build (passing, success)
  11. codecov/patch (passing, success)
  12. DCO (passing, success)
Decision points
  • Chose a case-insensitive regex ($CI_DEBUG_TRACE =~ /^(1|t|true)$/i) over an explicit enumeration or shell-side normalization for the debug-trace truthy set (alternatives: Enumerate each GitLab-recognized truthy literal as separate OR'd equality checks in YAML rules, Normalize CI_DEBUG_TRACE to lowercase in a pre-step and compare against a fixed set; rationale: GitLab CI/CD if: rules support case-insensitive regex flags natively; a single regex mirrors Go's strconv.ParseBool truthy set ("1", "t"/"T", "true"/"TRUE"/"True") in one readable condition and keeps the YAML and shell script guards visually consistent, and a shared Go constant (debugTraceDenyRuleIf) prevents the three Go call sites from drifting apart.)
  • Gated MergeMissingGitLabDebugTraceRule on the existing gitlabCIWorkflowIsFullsendOwned ownership signal rather than backfilling unconditionally (alternatives: Backfill the debug-trace rule into any workflow: block missing it, regardless of provenance, Only backfill for repos that also have fullsend's schedule/api rules already present; rationale: The root .gitlab-ci.yml is user-owned; unconditionally injecting a rule into a block fullsend cannot prove it installed risks corrupting a repo owner's independently-configured pipeline gate. Reusing the same ownership check the existing obsolete-rule strip relies on keeps the safety invariant consistent across both migration paths.)

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 3:10 AM UTC · Completed 3:33 AM UTC

Commit: d8eed60 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $10.71

fullsend-ai-review[bot]

This comment was marked as outdated.

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 3:35 AM UTC · Completed 3:58 AM UTC

Commit: d8eed60 · View workflow run →

Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $4.93

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 3 (bot-triggered)

Fixed the medium secret-exposure finding by pinning the fullsend poll/run CLI's --project, --gitlab-url, and --status-repo arguments to the CI_JOB_TOKEN job-record identity instead of overridable CI_PROJECT_PATH/FULLSEND_GITLAB_URL/CI_SERVER_URL pipeline variables, with regression tests proving the pinned identity wins even when those variables point elsewhere. Recorded a disagreement on the low intent-alignment finding: it's valid but its remediation (completing live GitLab verification, or editing the PR's closing keyword) is outside this agent's scope. Also inspected project CI: fixed the codecov/patch failure by adding tests for previously-uncovered branches in MergeMissingGitLabDebugTraceRule and the converge dry-run backfill path; the commit-lint failure (an earlier commit's title exceeds gitlint's 100-char limit) needs a human-requested squash/reword since bot-triggered runs cannot rewrite existing commits.

Fixed (1):

  1. secret-exposure: run-poll-job.sh --project/--gitlab-url and run-agent-job.sh --status-repo still use overridable CI_PROJECT_PATH/FULLSEND_GITLAB_URL/CI_SERVER_URL instead of the pinned job-record identity (internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/pin-ci-job-identity.sh): pin-ci-job-identity.sh now also captures path_with_namespace from the already-fetched pinned project record and exports it as FULLSEND_PINNED_PROJECT_PATH, fails closed if it is missing (same fail-closed pattern as default_branch), and exports FULLSEND_PINNED_GITLAB_URL derived from the validated CI_API_V4_URL bootstrap with /api/v4 stripped. run-poll-job.sh's fullsend poll --project/--gitlab-url and run-agent-job.sh's fullsend run --status-repo now consume these pinned values instead of CI_PROJECT_PATH/FULLSEND_GITLAB_URL/CI_SERVER_URL. Registered the two new FULLSEND_PINNED_* names in scaffold_env_consistency_test.go's allowlist. Added TestRunPollJobScript_UsesPinnedIdentityForCLIArgs and TestRunAgentJobScript_UsesPinnedIdentityForStatusRepo, which set CI_PROJECT_PATH/CI_SERVER_URL/FULLSEND_GITLAB_URL to values distinct from the pinned job record and assert the fullsend CLI invocation only received the pinned identity. Updated all existing pin/job-script test fixtures to include path_with_namespace in the mocked GET /projects/:id response so the new fail-closed check does not break them.

Disagreed (1):

  1. intent-alignment: pin-ci-job-identity.sh:39 — PR says "Closes ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" while the Ship-gate's live valid-foreign-token verification is explicitly deferred to GitLab webhook: dispatcher CI scaffold for signed pipeline-input dispatch #7771: The finding is accurate but not something this fix agent can resolve: completing the deferred live verification requires access to a real target GitLab instance/second job token, which is out of scope for an automated code-fix pass, and the alternative remediation (changing the PR's "Closes ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" closing keyword to "Relates to ADR 0125: in-job security gate hardening (agent + poll scripts) #7769"/"Part of ADR 0125: in-job security gate hardening (agent + poll scripts) #7769") is an edit to the PR description itself — forge metadata, not a repository file — which is outside this agent's authorized actions (PR mutations are the post-script's responsibility). Flagging for a human to either complete the live verification or edit the PR description's closing keyword.

Tests: passed

CI inspections (10):

  1. codecov/patch (pr-related, failure): Patch coverage was 70.51% (target 80%, enforced floor 75%): internal/repos/converge.go's new debug-trace-rule-backfill block (mergeErr handling, DryRun branch) and gitlabci.go's new MergeMissingGitLabDebugTraceRule parse-error/non-mapping-root branches, added by this PR's earlier commits, had no test coverage. Added TestMergeMissingGitLabDebugTraceRule_InvalidYAMLReturnsError, _CommentOnlyDocumentNoAction, and _RootNotMappingNoAction in gitlabci_test.go, plus TestConverge_GitLab_DebugTraceRuleBackfillDryRun in converge_test.go, covering the previously-uncovered branches. Recommend rerunning the check on the new commit.
  2. commit-lint (pr-related, failure): Commit d8eed60's title is 140 characters, exceeding gitlint's configured 100-character max (rule T1). Fixing this requires rewording an already-existing commit's message, which is a history rewrite. Bot-triggered fix-agent runs are not authorized to rewrite history (only an explicit human /fs-fix squash/rebase/redo request permits that). Recommend the user run /fs-fix squash (or reword the commit manually) to bring the offending title under 100 characters.
  3. dispatch / Fix (unrelated, pending): This is the current fix-agent dispatch run itself, not a project CI check to fix.
  4. gate (passing, success)
  5. build (passing, success)
  6. test (passing, success)
  7. e2e (passing, success)
  8. behaviour (passing, success)
  9. functional-tests (passing, success)
  10. DCO (passing, success)
Decision points
  • Left the mergeErr-handling block in convergeGitLabRootCIFiles (internal/repos/converge.go, the branch taken when MergeMissingGitLabDebugTraceRule returns a parse error) without a dedicated integration test (alternatives: Force the branch via a converge-level integration test with malformed .gitlab-ci.yml content, Only unit-test MergeMissingGitLabDebugTraceRule's own parse-error return directly; rationale: convergeGitLabRootCIFiles calls StripObsoleteGitLabWorkflowRules on the same raw content before MergeMissingGitLabDebugTraceRule; both parse the bytes the same way, so malformed YAML that reaches the debug-trace merge step deterministically already failed the earlier strip step first, making this specific caller-side branch unreachable through the public Converge() entry point. Covered the actually-reachable parse-error path with a direct unit test on MergeMissingGitLabDebugTraceRule instead.)

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 1:28 AM UTC · Completed 1:53 AM UTC

Commit: a9e3ebc · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $9.81

@fullsend-ai-coder

fullsend-ai-coder Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 1:55 AM UTC · Completed 2:14 AM UTC

Commit: a9e3ebc · View workflow run →

Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $5.49

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 2 (bot-triggered)

Fixed 6 of 9 review findings: allowed a bracketed IPv6-literal CI_API_V4_URL through the identity pin's anti-glob check (with a new real-IPv6-loopback test), stripped SSL_CERT_DIR/SSLKEYLOGFILE alongside the existing CA env vars, routed all post-pin PAT-bearing curls in run-agent-job.sh/run-poll-job.sh through the pinned API root instead of the overridable CI_API_V4_URL, and made three documentation fixes (naming FULLSEND_GITLAB_PIPELINE_VAR_RESTRICTION, a comma/semicolon fix, and a CI_DEBUG_TRACE whole-pipeline-skip note). Disagreed with 3 findings: the branches-endpoint JOB-TOKEN-support concern (requires live GitLab verification unavailable in this sandbox, and the reviewer's own confidence is explicitly low), the PR-description 'Closes #7769' wording (forge metadata edit outside fix-agent scope, per the finding's own remediation), and the default-branch-drift-detection enhancement (reviewer explicitly marked 'Not blocking', a feature request not a defect). Full internal/scaffold and internal/repos test suites pass; go vet and gofmt clean; shellcheck was not available in this sandbox (network-restricted, matching the PR's own note), so bash -n syntax checks were used instead. Inspected project CI: the 'behaviour' E2E job fails on 15/33 scenarios unrelated to this PR's GitLab-only diff (PR review/issue dispatch, hooks, pi runtime) and the same job also failed on other unrelated recent PR merges to main, indicating a pre-existing/infra issue rather than something this PR's changes caused.

Fixed (6):

  1. [edge-case] pin-ci-job-identity.sh:170 rejects any bracket in CI_API_V4_URL, so a legitimate IPv6-literal API root fails closed. (internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/pin-ci-job-identity.sh): Added a narrow regex allow-list for a bracketed IPv6-literal host (optionally with :port) before the general brace/bracket/angle/quote/@/whitespace deny-list, so an IPv6 API root passes validation while gitlab[1-2].example-style bracket-range hosts (and any other bracket use outside a valid IPv6 literal) are still rejected. Added TestPinCIJobIdentity_AllowsIPv6LiteralAPIURL, which stands up the mock GitLab API on the real IPv6 loopback address ([::1]) and asserts a full successful pin, plus verified TestPinCIJobIdentity_RejectsBracketRangeAPIURL still fails closed.
  2. [secret-exposure] pin-ci-job-identity.sh:126 doesn't strip SSL_CERT_DIR/SSLKEYLOGFILE, so a trigger-supplied SSL_CERT_DIR could substitute a CA. (internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/pin-ci-job-identity.sh): Added SSL_CERT_DIR and SSLKEYLOGFILE to the env -u list in fullsend_gate_curl and to the pin's pre-trust unset list (before sourcing trust-ci-server-ca.sh), so both are stripped per-call and left unset for the subsequent fullsend poll/run invocations too. Updated the module docstring to list the full unset set.
  3. [auth-bypass] run-agent-job.sh/run-poll-job.sh: 14 post-pin PAT-bearing curls still use CI_API_V4_URL instead of FULLSEND_PINNED_GITLAB_URL. (internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/run-agent-job.sh): Introduced FULLSEND_PINNED_API_V4_URL = "${FULLSEND_PINNED_GITLAB_URL}/api/v4" once, right after the identity pin succeeds in each script, failing closed with an explicit error if FULLSEND_PINNED_GITLAB_URL is ever unexpectedly unset. Replaced all 13 post-pin ${CI_API_V4_URL} call sites in run-agent-job.sh and the 1 in run-poll-job.sh with ${FULLSEND_PINNED_API_V4_URL}. Added the new script-local var to scaffold_env_consistency_test.go's allowlist so TestGitLabCIScaffoldEnvVarsAreCanonical stays green; full internal/scaffold suite passes.
  4. [missing-doc] docs/cli/repos.md:82 never names FULLSEND_GITLAB_PIPELINE_VAR_RESTRICTION on user-facing CLI/ops pages. (docs/cli/repos.md): Named FULLSEND_GITLAB_PIPELINE_VAR_RESTRICTION inline in the repos install description in docs/cli/repos.md and in the equivalent sentence in docs/guides/getting-started/operations.md, each linking to the GitLab Role-Credential Contract's 'How a job selects its credential' section for the full trade-off explanation.
  5. [doc-style] docs/cli/repos.md:82 uses a comma instead of the semicolon used elsewhere, misreading as nested in the prior clause. (docs/cli/repos.md): Replaced the comma with a semicolon before the clause, matching the delimiter used elsewhere in the same list and in operations.md.
  6. [backward-compatibility] gitlabci.go:80's CI_DEBUG_TRACE rule stops the whole pipeline, not just fullsend jobs, undocumented in the guides. (docs/guides/getting-started/configuring-gitlab.md): Added a short paragraph after the existing workflow:rules/auto_cancel explanation clarifying that workflow:rules gates pipeline creation (not per-job), so a truthy CI_DEBUG_TRACE skips creating the whole run, not only fullsend's own jobs, whenever fullsend owns that workflow: block, and why (CI/CD variables are visible at job init).

Disagreed (3):

  1. [api-contract] pin-ci-job-identity.sh:260's branches-endpoint JOB-TOKEN call may not be on every GitLab version's allowlist, risking fail-closed breakage.: The reviewer itself flags this as unverified ('rests on the reviewing model's recollection... no internet access was available to check GitLab's current API docs'), and I have the same no-network constraint in this sandbox, so I cannot confirm GitLab's current JOB_TOKEN allowlist for this route either. The suggested fix (drop the in-job branches/protected-branch check) would remove a deliberately designed, heavily-tested security control (TestPinCIJobIdentity_DeniesUnprotectedDefaultBranch, TestPinCIJobIdentity_DeniesProtectedNonDefaultRef, TestPinCIJobIdentity_BranchLookupFailsClosed) based on an unconfirmed claim about GitLab's REST surface, which is a materially riskier and larger change than the finding's own confidence supports. ADR 0125 already tracks an analogous live-verification gap (the dispatcher's valid-foreign-token GET /api/v4/job mis-resolution case) as an explicit open, ship-gating follow-up rather than resolving it by guessing at behavior; this finding should be tracked the same way (live-verify against a real target GitLab instance/current docs) rather than acted on speculatively by a fix agent with no network access.
  2. [intent-alignment] PR body still says 'Closes ADR 0125: in-job security gate hardening (agent + poll scripts) #7769' and cites '(GitLab service-account lifecycle + webhook fast-path via fresh-identity poller handoff (ADR 0125) #7772)'; a human must correct this via the forge UI since ADR 0125: in-job security gate hardening (agent + poll scripts) #7769's ship gate isn't fully satisfied.: Editing the PR title/description is forge-metadata mutation reserved for the post-script/human per this agent's scope (it cannot create or mutate PR objects), and the finding itself explicitly directs a human operator to make this edit via the forge UI, noting the fix agent has repeatedly stated across prior rounds that it lacks authorization to do so directly. No code or doc content change is prescribed by this finding — it is a PR-metadata correction outside fix-agent scope.
  3. [backward-compatibility] pin-ci-job-identity.sh: tightening the ref requirement to the exact default branch could break enrolled repos running on a different protected branch; suggests a pre-rollout default-branch drift check.: The reviewer explicitly marks this 'Not blocking' and 'not required to ship the pin' — it is a feature suggestion for a new drift-detection check in repos status/install, not a defect in the code this PR changes. Implementing it would add a new operator-facing feature beyond the scope of addressing review feedback on the existing pin/gate hardening change.

Tests: passed

CI inspections (11):

  1. behaviour (E2E Tests) (unrelated, failure): 15 of 33 behaviour scenarios fail (PR review dispatch, issue label dispatch, fork PR sync/label, SSRF hook, child-trigger inheritance, disabled-harness gating, base-composed harness dispatch, issue label dispatch, pi runtime, CEL trigger dispatch, reaction posting, custom-agent config, URL-sourced+local config mix, bad-integrity-hash handling) via 'after scenario hook failed: harness run ... concluded with failure' against external halfsend-11/test-repo-* dispatch targets. None of the failing scenarios touch GitLab CI scaffolding, gitlabci.go, or the pipeline-var-restriction code this PR changes (diff is entirely GitLab-scoped: docs, internal/repos/gitlabci.go, internal/repos/gitlab_pipeline_var_restriction.go, internal/scaffold/fullsend-repo-gitlab/**). The same 'behaviour' job also failed on other recent, unrelated PR merges to main (e.g. PR feat(#7229)!: move to OpenShell 0.1.2 #7233, feat(#7782)!: require GitLab role credentials at runtime #7811, refactor(#7798): add GitLab pipeline-variable override-role methods #7813 merge commits), indicating a pre-existing/infra-level issue with the external harness-dispatch test fixture rather than a regression from this PR. Recommend rerunning the behaviour job; if it keeps failing on unrelated PRs/main, file an issue against the e2e/behaviour test infra (external halfsend-11 dispatch harness) rather than editing this PR's GitLab-only diff.
  2. codecov/patch (passing, pass)
  3. DCO (passing, pass)
  4. build (passing, pass)
  5. commit-lint (passing, pass)
  6. e2e (passing, pass)
  7. functional-tests (passing, pass)
  8. gate (passing, pass)
  9. test (passing, pass)
  10. test-sandbox-darwin (passing, pass)
  11. web (passing, pass)
Decision points
  • Kept the branches/protected-branch check in pin-ci-job-identity.sh unchanged rather than removing it as one of the finding's suggested remediations (alternatives: Drop the in-job branches check and rely only on default-branch equality plus install-time protections, Move the protected-branch check to run after a PAT is selected, Leave the check as-is and record the live-verification need for tracking; rationale: Removing or restructuring a deliberately designed, well-tested fail-closed security check based on an admittedly unverified claim about GitLab's CI_JOB_TOKEN REST allowlist risked a larger, riskier change than the finding's own confidence level supports; deferred to live verification instead, consistent with how this PR/ADR 0125 already handles an analogous unverifiable case.)
  • Centralized the pinned-API-root fix behind one new script-local variable (FULLSEND_PINNED_API_V4_URL) per script instead of inlining FULLSEND_PINNED_GITLAB_URL + "/api/v4" at each of the 14 call sites (alternatives: Inline the concatenation at each call site, Export FULLSEND_PINNED_API_V4_URL from pin-ci-job-identity.sh itself; rationale: A single derivation point with one fail-closed check is less error-prone than repeating string concatenation at 14 sites, and keeping it local to each caller script (rather than adding it to pin-ci-job-identity.sh's own exports) matches the existing pattern where FULLSEND_PINNED_GITLAB_URL is the pin's contract and callers derive what they need from it.)

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 2:16 AM UTC · Ended 2:39 AM UTC

Commit: fa1553e · View workflow run →

@ggallen

ggallen commented Sep 30, 2026

Copy link
Copy Markdown
Member

/fs-code

Rebase this PR onto the current main branch and squash the branch into one commit before making further changes.

Mold PR #7817 to match the revised least-privilege plan:

  1. Keep and finish the in-job security hardening for ADR 0125: in-job security gate hardening (agent + poll scripts) #7769:

    • server-derived CI job identity pinning;
    • disjoint poller/agent source allowlists;
    • protected-default-branch verification;
    • CI_DEBUG_TRACE deny-before-admit behavior;
    • CA-pinned and trigger-safe GitLab gate requests;
    • fail-closed merge-request identity checks;
    • all associated tests and documentation.
  2. Do not implement pipeline-input transport or opaque dispatch records in this PR. Those belong to GitLab dispatch: replace user-defined pipeline variables with inputs and signed dispatch records #7850 and GitLab forge: add pipeline-input support to pipeline creation #7851.

  3. Remove every requirement or implication that the Fullsend runtime credential must be Owner. The runtime credential must remain D
    eveloper-level.

  4. Keep the GitLab project pipeline-variable restriction support only as an administrative/install-time primitive. Do not enable a runtime Owner credential, do not make Owner the required dispatch design, and do not break the existing dispatch path before GitLab dispatch: replace user-defined pipeline variables with inputs and signed dispatch records #7850/GitLab forge: add pipeline-input support to pipeline creation #7851 land.

  5. Update the implementation and tests so the PR’s scope is the in-job gate hardening only. Do not add speculative dispatcher, webhook, CreatePipeline-input, or opaque-record implementation here.

  6. Rebase and squash before final verification, then run the relevant Go, scaffold, shell, documentation, and security tests. Fix all CI failures, including any CI failures introduced or exposed by this branch. Do not leave known failing checks unexplained.

Do not modify #7767 or #7768, and do not make this PR depend on #7850/#7851 for the core in-job hardening to pass.

Do not overwrite or regress the current PR description; preserve its Developer-level credential scope, #7850/#7851 references, and Closes #7769.

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 2:16 AM UTC · Completed 2:39 AM UTC

Commit: fa1553e · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $5.98

@fullsend-ai-coder

fullsend-ai-coder Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 2:41 AM UTC · Completed 2:48 AM UTC

Commit: fa1553e · View workflow run →

Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.25

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 3 (bot-triggered)

No source changes made this round. Both current findings are process/verification blockers rather than confirmed code defects: the medium finding needs live verification against a real GitLab instance that this sandbox cannot perform, and unilaterally applying any of its code-level remediations would either remove a tested security guarantee or rest on an equally unverified guess; the low finding needs a human edit to the PR description, which is outside this agent's scope (PR mutations belong to the deterministic post-script). Recorded reasoned disagreements for both, consistent with prior rounds. Project CI: all jobs pass except E2E Tests/behaviour, which fails across ~15 scenarios spanning unrelated GitHub-dispatch/harness features this PR's GitLab-only diff never touches; a 3-day scan of E2E Tests runs on main shows frequent failures on unrelated merges, so this is classified as pre-existing flakiness, not caused by this PR.

Disagreed (2):

  1. api-contract: branches-endpoint JOB-TOKEN allowlist support is unverified (pin-ci-job-identity.sh:280): Verified the code matches the finding's description exactly, and confirmed the branches lookup is the only place that checks the default branch is actually protected (TestPinCIJobIdentity_DeniesUnprotectedDefaultBranch depends on it). The three remediation options that do not require live verification all carry real cost: (1) dropping the check removes a tested, ADR-0125-mandated guarantee ('the enrolled protected default branch', not merely a name match) with no equivalent install-time enforcement in this codebase today (internal/repos/gitlab_pipeline_access.go only grants poller access when a branch happens to already be protected; it never enforces protection), so removing it would be a real regression, not a safe simplification; (2) moving the check to run after a PAT is selected requires restructuring run-agent-job.sh and run-poll-job.sh (new call sites, new tests) on the strength of an unverified guess about which endpoint is on the job-token allowlist; (3) switching to /protected_branches/:branch substitutes one unverified endpoint for another with no more evidence either way. This sandbox has no network access to GitLab's API docs or a live instance to resolve the actual allowlist question, which is the live-verification ship gate the finding itself calls out. Per zero-trust, I verified the claim against the code (accurate) but will not guess at a security-relevant behavior change without a way to validate it; the same reasoning was recorded by the fix agent on the immediately preceding review round, and nothing has changed since that would let it be resolved differently now. A human with access to a live target-version GitLab instance (or current GitLab API docs) needs to confirm the allowlist before any of the three code-level remediations can be applied safely.
  2. intent-alignment: PR body 'Closes ADR 0125: in-job security gate hardening (agent + poll scripts) #7769' will auto-close the issue before its acceptance criteria are met: Verified the code: GitLabPipelineVarRestrictionEnforced() does default to report-only (true only when FULLSEND_GITLAB_PIPELINE_VAR_RESTRICTION == "enforced"), matching the finding and the documented, deliberate deferral in this file and docs/contributing/gitlab-role-credentials.md. The remaining gap is metadata (the PR description's closing keyword), not a code defect, and the fix for it is a forge-side edit to the PR description text via the GitHub UI/API — not a source-tree change. This agent's role is scoped to committing code changes on the PR branch; PR body/description mutation is a forge mutation outside that scope (the harness's deterministic post-script owns PR mutations). This matches what has been recorded on every prior round of this PR's review. A human operator should edit the PR description to say 'Relates to ADR 0125: in-job security gate hardening (agent + poll scripts) #7769' (or 'Part of ADR 0125: in-job security gate hardening (agent + poll scripts) #7769') per the finding's own remediation.

Tests: passed

CI inspections (12):

  1. E2E Tests / behaviour (flaky, failure): 15 of 33 behaviour scenarios failed across unrelated GitHub-dispatch/harness features (triage, PR/issue/label dispatch, hooks, PI runtime selection, SSRF hook, fork-PR sync) with generic 'workflow run concluded with failure before producing artifact' errors in ephemeral test repos. This PR's diff only touches GitLab CI scaffold files and repos-install Go code, none of which the failing scenarios exercise. A 3-day scan of E2E Tests runs on main shows multiple failures on unrelated recent merges, indicating pre-existing intermittent flakiness in the behaviour suite/harness infrastructure. Recommend rerunning the E2E Tests workflow; no code change indicated for this PR.
  2. E2E Tests / e2e (passing, success)
  3. E2E Tests / gate (passing, success)
  4. Functional Tests / functional-tests (passing, success)
  5. Functional Tests / gate (passing, success)
  6. CI / test (passing, success)
  7. CI / web (passing, success)
  8. CI / commit-lint (passing, success)
  9. CI / test-sandbox-darwin (passing, success)
  10. Build Site / build (passing, success)
  11. codecov/patch (passing, success)
  12. DCO (passing, success)
Decision points
  • Left the pin-ci-job-identity.sh protected-branch lookup unchanged rather than dropping it, relocating it, or swapping its endpoint (alternatives: Drop the /repository/branches lookup and rely only on default-branch-name equality, Move the protected-branch check into run-agent-job.sh/run-poll-job.sh to run after a role PAT is selected, using PRIVATE-TOKEN instead of JOB-TOKEN, Swap the endpoint to GET /projects/:id/protected_branches/:branch while keeping JOB-TOKEN, Leave the code as-is and record a disagreement pending live GitLab verification; rationale: The first three options each assume a fact about GitLab's CI_JOB_TOKEN REST allowlist that cannot be checked from this sandbox (no network access to GitLab docs or a live instance), and the first option additionally removes a security guarantee an existing regression test depends on with no compensating install-time enforcement elsewhere in the codebase. Making an unverified, security-relevant behavior change is worse than leaving the current fail-closed behavior in place and flagging the open question for a human to resolve against a real GitLab instance.)

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Ended 9:20 PM UTC

Commit: 107f0ad · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 9:22 PM UTC · Ended 9:28 PM UTC

Commit: 561922b · View workflow run →

Pin poll and agent jobs to the server-side CI job identity, enforce disjoint source allowlists, and deny CI debug tracing before admission. Converge GitLab pipeline-variable override restrictions and route gate requests through the pinned API URL with trusted CA handling.

Addresses #7817
Relates to #7769

Signed-off-by: Greg Allen <gallen@redhat.com>
Co-authored-by: Codex <codex@openai.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:30 PM UTC · Completed 9:56 PM UTC

Commit: 0277078 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $8.90

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 9:58 PM UTC · Completed 10:13 PM UTC

Commit: 0277078 · View workflow run →

Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $3.51

Address review findings on PR #7817:

- run-agent-job.sh: unset CI_MERGE_REQUEST_IID in the issue-event
  EVENT_TYPE arm. This job's admit source is api-only, so GitLab never
  natively populates that variable; newGitLabClientFromEnv routes
  status-comment API calls to merge_requests/{iid} whenever it is
  merely non-empty, regardless of FULLSEND_NOTE_TARGET, which could
  misdirect an issue status comment onto an unrelated merge request.
  Added a regression test.
- pin-ci-job-identity.sh: unset HTTP_PROXY/HTTPS_PROXY/http_proxy/
  https_proxy in the parent shell once the pin succeeds, so a
  trigger-influenced proxy setting left over from job init cannot
  steer the subsequent fullsend poll/run Go processes' API traffic
  (fullsend_gate_curl only stripped these for its own curl calls).
  Added a regression test.
- pin-ci-job-identity.sh: documented why the pinned-branch lookup
  (GET /projects/:id/repository/branches/:branch) is a same-project
  CI_JOB_TOKEN call rather than a cross-project one, and that GitLab's
  fine-grained job-token permissions docs separately list this route
  under read_repository -- addressing the long-recurring "confirm this
  against GitLab's allowlist" ask; verification against the oldest
  self-hosted GitLab version fullsend supports remains open.
- docs/guides/getting-started/configuring-gitlab.md: document the
  in-job identity-pin's fail-closed constraints (protected default
  branch only, disjoint schedule/api pipeline-source allowlist) on the
  operator-facing getting-started path.

Not changed (see structured output for reasoning): the report-only
default for the pipeline-variable override-role restriction, and the
PR description's issue-closing keyword -- both are flagged in the
review as requiring a human decision, not a code change in this diff.

Relates to #7769
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 1 (bot-triggered)

Fixed two concrete bugs (issue-event note-target misdirection via a stray CI_MERGE_REQUEST_IID, and proxy env leaking into post-pin fullsend CLI subprocesses) with regression tests, added an operator-facing doc section on the identity-pin's runtime constraints, and documented verification research resolving the long-recurring branches-endpoint allowlist concern. Declined two findings that the review's own remediation assigns to a human/PR-tracking decision rather than a code change.

Fixed (4):

  1. issue_ EVENT_TYPE arm leaves CI_MERGE_REQUEST_IID set, which newGitLabClientFromEnv treats as non-empty regardless of FULLSEND_NOTE_TARGET and routes status-comment calls to merge_requests instead of issues* (internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/run-agent-job.sh): Added unset CI_MERGE_REQUEST_IID in the issue_* EVENT_TYPE case arm before any fullsend CLI invocation, since this api-only job never natively receives that variable and its mere presence (not just a mismatched value) misdirects the note target. Added TestRunAgentJobScript_UnsetsMergeRequestIIDForIssueEvent, which fails on the pre-fix code and passes after the fix.
  2. fullsend_gate_curl strips proxy env vars only for its own curl calls; HTTP_PROXY/HTTPS_PROXY left set in the parent shell after a successful pin are inherited by the subsequent fullsend poll/run Go processes, whose http.Client honors ProxyFromEnvironment (internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/pin-ci-job-identity.sh): Unset HTTP_PROXY/HTTPS_PROXY/http_proxy/https_proxy in the parent shell immediately after a successful pin, before exporting the pinned identity. Added TestPinCIJobIdentity_UnsetsProxyEnvInParentShellOnSuccess, which fails on the pre-fix code and passes after the fix.
  3. branches endpoint's CI_JOB_TOKEN support is unverified against GitLab's allowlist, recurring ~14 review rounds (internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/pin-ci-job-identity.sh): Researched GitLab's documentation (web search, since direct doc fetches were SSRF-blocked in this sandbox): the call is same-project (FULLSEND_PINNED_PROJECT_ID is the job's own pipeline.project_id), and GitLab's classic job-token scope allowlist has only ever restricted cross-project access, not a token's calls into its own project; separately, GitLab's fine-grained job-token permissions documentation explicitly lists GET /projects/:id/repository/branches under the read_repository policy. Added a code comment recording this verification and the specific facts it rests on, addressing the recurring 'confirm against GitLab's allowlist' ask. Live verification against the oldest self-hosted GitLab version fullsend supports is still open and noted as such; behavior was not changed since the evidence supports the current fail-closed design.
  4. getting-started docs omit the in-job identity-pin's runtime constraints (protected-default-branch-only, disjoint schedule/api allowlist), documented only in the contributing docs (docs/guides/getting-started/configuring-gitlab.md): Added an 'In-job identity pin' subsection describing the fail-closed protected-default-branch requirement and the disjoint pipeline-source allowlist, with a cross-reference to gitlab-role-credentials.md for the full trust model.

Disagreed (2):

  1. ci_pipeline_variables_minimum_override_role=owner defaults to report-only unless FULLSEND_GITLAB_PIPELINE_VAR_RESTRICTION=enforced: The review's own remediation for this finding explicitly says not to invert the default in this PR, since doing so would break the Developer-level poller's own CreatePipeline call today; this is a documented, deliberate deferral pending a separate poller Owner-credential decision tracked in ADR 0125: in-job security gate hardening (agent + poll scripts) #7769, not a regression introduced by this diff. No code change made.
  2. PR body ends with a closing keyword ('Closes ADR 0125: in-job security gate hardening (agent + poll scripts) #7769') while ADR 0125: in-job security gate hardening (agent + poll scripts) #7769's acceptance criteria (required-control enforcement, live-verification ship gate) remain unmet, so merging would auto-close the tracking issue prematurely: This requires editing the PR description via the forge UI, which is outside the fix agent's tool access (no PR-description-edit capability) and is explicitly called out in the review's remediation as a task for a human operator. The commit message for this fix uses 'Relates to ADR 0125: in-job security gate hardening (agent + poll scripts) #7769' rather than a closing keyword, consistent with the review's ask, but the PR body itself cannot be changed from this agent.

Tests: passed

CI inspections (11):

  1. functional-tests (transient-infra, failure): The 'Create ephemeral repo and fixture' before_each hook failed (exit 1) uniformly across all four triage eval cases before any agent turn ran, which then made the scoring step crash on a None pass-rate threshold. This is infrastructure for the GitHub-based eval harness (ephemeral repo/fixture provisioning) and is unrelated to this PR's GitLab scaffold scripts and Go changes. Recommend rerunning the functional-tests job; if it fails again with the same ephemeral-repo-creation error, file an issue with the eval-harness owner rather than editing this PR's diff.
  2. e2e (passing, success)
  3. behaviour (passing, success)
  4. gate (passing, success)
  5. test-sandbox-darwin (passing, success)
  6. commit-lint (passing, success)
  7. web (passing, success)
  8. test (passing, success)
  9. build (passing, success)
  10. codecov/patch (passing, success)
  11. DCO (passing, success)
Decision points
  • Left the GET /projects/:id/repository/branches/:branch call's fail-closed behavior unchanged and instead added verification documentation (alternatives: Move the branch-protection check to after a PAT is selected, Switch to a different endpoint, Drop the in-job branches check entirely, Document the verification and keep current behavior; rationale: Web research indicates this is a same-project CI_JOB_TOKEN call (not subject to the classic cross-project job-token scope allowlist the earlier rounds' concern was based on) and that GitLab's current fine-grained permissions documentation explicitly covers this route family; changing the fail-closed design on weaker evidence than what is already documented risked introducing a real security regression to fix an unconfirmed operational one.)

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:15 PM UTC · Completed 10:34 PM UTC

Commit: 369c87d · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $7.35

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 10:36 PM UTC · Completed 10:46 PM UTC

Commit: 369c87d · View workflow run →

Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.27

…nt dispatch

Address review feedback on PR #7817:
- In the issue_* EVENT_TYPE arm of run-agent-job.sh, explicitly export
  FULLSEND_NOTE_TARGET=issues alongside the existing unset of
  CI_MERGE_REQUEST_IID. newGitLabClientFromEnv selects the
  merge_requests note target whenever CI_MERGE_REQUEST_IID is non-empty
  or FULLSEND_NOTE_TARGET is "merge_requests" -- a pre-set
  FULLSEND_NOTE_TARGET CI/CD variable could otherwise survive into an
  issue-event dispatch and misdirect the status comment.
- In pin-ci-job-identity.sh's post-pin proxy cleanup, also unset
  ALL_PROXY/all_proxy alongside HTTP_PROXY/HTTPS_PROXY, matching the
  set fullsend_gate_curl already strips for its own curl calls; git
  (used for later credential-bearing fetches) honors ALL_PROXY as a
  fallback.
- Extended the existing regression tests
  (TestRunAgentJobScript_UnsetsMergeRequestIIDForIssueEvent,
  TestPinCIJobIdentity_UnsetsProxyEnvInParentShellOnSuccess) to cover
  both cases.

The remaining "Closes #7769" PR-description finding requires a human
to edit the PR description via the forge UI and is not resolvable by
an automated commit; recorded as a disagreement in structured output.

Note: pre-commit could not fetch hook repositories (network access is
blocked in this sandbox), so pre-commit itself did not run. Ran the
network-independent equivalents directly instead: gofmt -l and go vet
(clean) for the two changed Go test files, plus manual checks for
trailing whitespace, final newline, and merge-conflict markers on all
four changed files (clean). The shellcheck hook could not be run
directly either, since its shellcheck binary is normally installed via
network fetch and is not preinstalled in this sandbox.

Addresses #7817
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 2 (bot-triggered)

Fixed both code findings from this round's review: pinned FULLSEND_NOTE_TARGET=issues in the issue_* dispatch arm of run-agent-job.sh so a pre-set CI/CD variable cannot misdirect status comments, and extended pin-ci-job-identity.sh's post-pin proxy cleanup to also strip ALL_PROXY/all_proxy. Both existing regression tests were extended to cover the new assertions and pass. The remaining PR-description "Closes #7769" finding is recorded as a disagreement requiring human action, as in prior rounds.

Fixed (2):

  1. issue_ EVENT_TYPE arm unsets CI_MERGE_REQUEST_IID but not FULLSEND_NOTE_TARGET, so a pre-set FULLSEND_NOTE_TARGET=merge_requests CI/CD variable can still misdirect an issue-event status comment to the merge_requests API* (internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/run-agent-job.sh): Added an explicit export FULLSEND_NOTE_TARGET="issues" in the issue_* arm alongside the existing unset CI_MERGE_REQUEST_IID, so newGitLabClientFromEnv's OR condition (CI_MERGE_REQUEST_IID non-empty || FULLSEND_NOTE_TARGET=="merge_requests") can no longer select merge_requests for an issue-event dispatch regardless of any pre-set project/group/pipeline CI/CD variable. Extended TestRunAgentJobScript_UnsetsMergeRequestIIDForIssueEvent to pre-set FULLSEND_NOTE_TARGET=merge_requests in the stub environment and assert the fullsend CLI child observes FULLSEND_NOTE_TARGET=issues instead.
  2. Post-pin proxy env cleanup in pin-ci-job-identity.sh unsets HTTP_PROXY/HTTPS_PROXY (upper/lower) but omits ALL_PROXY/all_proxy, which git honors as a fallback for later credential-bearing fetches (internal/scaffold/fullsend-repo-gitlab/.gitlab/ci/scripts/pin-ci-job-identity.sh): Extended the post-pin unset to also include ALL_PROXY and all_proxy, matching the set fullsend_gate_curl already strips for its own curl calls. Extended TestPinCIJobIdentity_UnsetsProxyEnvInParentShellOnSuccess to set ALL_PROXY/all_proxy to an attacker-controlled value and assert both are unset in the parent shell after a successful pin.

Disagreed (1):

  1. PR body still ends with "Closes ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" while issue ADR 0125: in-job security gate hardening (agent + poll scripts) #7769's acceptance criteria (required control not defense-in-depth, live-verification ship gate) remain unmet, so merging will prematurely auto-close the tracking issue: The finding is valid and verified against the current PR body and issue ADR 0125: in-job security gate hardening (agent + poll scripts) #7769, but it is a PR-metadata/description edit, not a code change. The fix agent has no tool access to edit the PR description; per the review's own remediation, this requires a human operator to edit the PR description via the forge UI (e.g. replace "Closes ADR 0125: in-job security gate hardening (agent + poll scripts) #7769" with "Relates to ADR 0125: in-job security gate hardening (agent + poll scripts) #7769"). Leaving unchanged pending human action, consistent with roughly 15 prior review rounds recording the same disagreement.

Tests: passed

CI inspections (11):

  1. functional-tests (unrelated, failure): Fails in the eval harness's score.py with a TypeError comparing a None threshold to a float during regression detection, unrelated to any code this PR touches (GitLab CI scaffold scripts and Go repos/pipeline-var-restriction code). This PR's diff does not touch eval/ or the harness scoring config. File an issue with the eval-harness/functional-tests owners; rerun is unlikely to help since this looks like a config/threshold gap rather than flakiness, but a rerun can confirm.
  2. build (passing, success)
  3. test (passing, success)
  4. behaviour (passing, success)
  5. e2e (passing, success)
  6. DCO (passing, success)
  7. commit-lint (passing, success)
  8. codecov/patch (passing, success)
  9. gate (passing, success)
  10. test-sandbox-darwin (passing, success)
  11. web (passing, success)

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:48 PM UTC · Completed 11:11 PM UTC

Commit: 08540ef · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $10.88

This branch was successfully deployed

1 active and 1 inactive deployments
site-preview — 08540ef0 Deployed Sep 30, 2026 by github-actions[bot]
dev — 08540ef0 Deployed Sep 30, 2026 by fullsend-ai-coder[bot] via behaviour #14734
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-human Agent loop needs human intervention ready-for-merge All reviewers approved — ready to merge ready-for-review Triggers review agent dispatch risk/moderate PR risk: moderate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ADR 0125: in-job security gate hardening (agent + poll scripts)

1 participant