Skip to content

refactor(#7782)!: complete GitLab identity migration cleanup - #7846

Merged
ggallen merged 1 commit into
mainfrom
agent/7782-drop-dead-migration-compat
Sep 30, 2026
Merged

ggallen merged 1 commit into
mainfrom
agent/7782-drop-dead-migration-compat

Conversation

@fullsend-ai-coder

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

Copy link
Copy Markdown
Contributor

Summary

This PR completes PR3, the final implementation slice of the GitLab identity-migration cleanup tracked by #7782. Building on PR2, it removes the remaining migration-gate compatibility machinery and makes registered role credentials the only supported runtime authentication path.

Custom roles and BYOA behavior are intentionally preserved. This PR does not remove the role registry or custom-role support.

PR3 implementation

  • Removed FULLSEND_GITLAB_ROLE_MIGRATION runtime handling and the disabled, migrating, enforced, and rollback modes.
  • Removed shared-token runtime authentication and FULLSEND_FORGE_TOKEN fallback paths.
  • Removed migration-only cutover/rollback compatibility branches, stale flags, status fields, diagnostics, and API plumbing.
  • Made built-in and registered custom-role selection, readiness, routing, capabilities, reuse, rotation, and lifecycle checks unconditional.
  • Preserved custom-role registration, --gitlab-role-registry, --gitlab-role-token, own/reuse credentials, and BYOA routing.
  • Preserved safe uninstall and retirement cleanup for historical migration variables, shared secrets, legacy role secrets, and obsolete project access tokens.
  • Updated production tests, regression coverage, CLI behavior, and documentation.

Relationship to PR2

PR2 removed the primary migration-gate runtime path. PR3 removes the dead compatibility surface left behind by that change: obsolete mode parsing and reporting, residual cutover inputs, bot-token convergence plumbing, stale status/API fields, and migration-only test/documentation references.

Breaking changes

The GitLab migration/cutover/rollback flags and migration-mode API are removed. FULLSEND_FORGE_TOKEN is no longer selected at runtime. Existing custom-role configurations remain supported through registered role credentials.

Verification

  • make lint
  • go test -race ./internal/gitlabroles ./internal/repos ./internal/cli ./internal/scaffold
  • Changed-function patch coverage checked against the repository’s 80% target / 75% floor

This PR completes PR3 and closes #7782. The parent rollout issue (#7496) and its separate documentation follow-up (#7502) are tracked independently.

Closes #7782

@fullsend-ai-coder
fullsend-ai-coder Bot requested a review from a team as a code owner September 29, 2026 20:14
@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 8:16 PM UTC · Completed 8:38 PM UTC

Commit: 24be525 · View workflow run →

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

@codecov

codecov Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

@ggallen ggallen added the fullsend-no-fix Skip bot-triggered fix agent runs label Sep 29, 2026
@fullsend-ai-review fullsend-ai-review Bot added the risk/moderate PR risk: moderate 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 reflecting a focused, maintainer-directed final slice (PR3 of 3) of a GitLab identity-migration cleanup with good test coverage (0.45 test-file ratio) and no protected-path or CI-workflow changes, balanced against high recent churn/coupling in the GitLab role packages and an explicit breaking-change/security label on the tracking issue; unchanged from the prior review of this same PR (score 2) since Tier 1 signals are materially the same.

Previous run

Risk Assessment: moderate (2/5)

Details

Moderate risk reflecting a focused, maintainer-directed final slice (PR3 of 3) of a GitLab identity-migration cleanup with good test coverage (0.45 test-file ratio) and no protected-path or CI-workflow changes, balanced against high recent churn/coupling in the GitLab role packages and an explicit breaking-change label on the tracking issue; unchanged from the prior review of this same PR (score 2) since Tier 1 signals (file/line count, protected paths, dependency changes, bot author) are materially the same.

Previous run (2)

Risk Assessment: moderate (2/5)

Details

Moderate risk reflecting a focused bot-authored cleanup slice with good test coverage and no protected files, balanced against elevated churn, coupling, and security-labeled breaking changes in GitLab role packages.

@fullsend-ai-review

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

Copy link
Copy Markdown

Review

Findings

Low

  • [incorrect-api-claim] docs/contributing/gitlab-role-credentials.md:351 — Diagnose(present, registry) is documented as setting Ready when every registered role credential is present and satisfies the capability contract. gitlabroles.Diagnose only checks secret presence (Ready = total > 0 && configured == total); it never inspects capabilities, job mappings, or lifecycle. Capability/mapping checks live in CheckBuiltinReadiness / CheckRegisteredReadiness, which repos status AND-combines with Diagnose.Ready. The same bullet list already describes per-role state as presence-only, so this one line overstates the API.
    Remediation: Describe Diagnose.Ready as presence-only (every registered role secret is present). Point readers to CheckBuiltinReadiness / CheckRegisteredReadiness (and repos status) for the capability/mapping contract.

  • [doc-comment-format] internal/gitlabroles/route.go:36 — The doc comment for Select contains an editing artifact: line 36 ends with the dangling clause "It does not read" and line 37 continues with "Migration state is not read.", reading as "It does not read Migration state is not read."
    Remediation: Remove the dangling "It does not read" clause so the comment reads "...then resolves job. Migration state is not read. An unmapped agent name is always rejected...".

  • [naming-convention] internal/gitlabroles/gitlabroles_test.go:90 — TestErrorDoesNotExposeModeOrSecretValue references "Mode" in its name, but the Mode type and Error.Mode field were removed from gitlabroles by this PR. The test only asserts Role/Secret formatting and that token values are not exposed.
    Remediation: Rename the test to TestErrorDoesNotExposeSecretValue (or similar) to match what it actually asserts.

  • [naming-convention] internal/gitlabroles/readiness.go:281 — appendEnforcedResolveReasons retains "Enforced" in its name even though ModeEnforced was removed and the reason string it appends was updated in this same hunk from "enforced resolve..." to "role resolve...".
    Remediation: Rename appendEnforcedResolveReasons (and its call site at line 277) to appendRoleResolveReasons or similar to align with the removal of enforced-mode terminology.


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

Looks good to me

Previous run (2)

Review

Findings

Low

  • [test-weakened] internal/scaffold/select_gitlab_role_token_test.go:43 — The collapsed select-gitlab-role-token.sh unit tests dropped the poison-pill coverage that locked fail-closed behavior when FULLSEND_FORGE_TOKEN is present alongside a role secret. TestSelectGitLabRoleTokenBuiltins only injects the role secret (no leftover shared token), and TestSelectGitLabRoleTokenMissingOrUnregisteredFailsClosed uses a fully empty env, so neither would catch a regression that prefers FULLSEND_FORGE_TOKEN when both are set on the agent path or on a missing-role-secret path. The deleted tests (e.g. TestSelectGitLabRoleToken_DisabledUsesRoleToken, TestSelectGitLabRoleToken_EnforcedMissingRoleSecretFails, TestSelectGitLabRoleToken_MatchesGoRegistryResolution) previously asserted this explicitly and cross-checked the bash reimplementation against internal/gitlabroles.Select/SelectAgent. The production select-gitlab-role-token.sh script is unchanged by this PR and other tests (e.g. TestRunPollJobScript_BlanksSiblingSecretsBeforePoll) still cover the poller path, which is why this is low rather than medium severity.
    Remediation: Restore a compact bash unit case that sets FULLSEND_FORGE_TOKEN without the role secret and asserts fail-closed on the role secret name (not silent success via the shared token), plus a happy-path case with both set that asserts TOKEN is the role secret. Optionally keep one Go/bash TOKEN_NAME agreement assertion so the bash reimplementation cannot silently drift from gitlabroles.Select/SelectAgent.

  • [missing-test] internal/gitlabroles/route_test.go:58 — TestJobForAgentPrefersRegisteredAgentName and TestJobForAgentFallsBackToHarnessRole were deleted and not replaced. JobForAgent (route.go) still contains both branches — a registered custom agent name winning over a different harness role, and an unmapped agent name falling back to a registered harness role — but the only remaining exercise of this path is SelectAgent("unknown", "", ...) in TestSelectFailsClosed, which only covers the doubly-unmapped fail-closed case with an empty harness role. A regression that always used agentName or always used harnessRole in JobForAgent would not be caught by the current suite.
    Remediation: Restore tests for JobForAgent/SelectAgent covering: (1) a registered custom agent name wins over a different harness role, (2) an unmapped agent name falls back to a registered non-empty harness role, (3) neither mapped fails closed as ErrUnregistered.


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

  • [intent-alignment] COMMITS.md:40 — PR title/commit type is feat(#7782)!: for an internal cleanup that removes deprecated compatibility plumbing (ParseMode, ModeVariableName, ErrInvalidMode, UsesSharedOnly, RequiresRoleCredentials, NeedsGitLabBotToken, DrainConfirmed) from unexported/internal Go packages. COMMITS.md reserves feat for end-user-visible new capability and maps internal restructuring / non-user-facing tightening to refactor (or fix): "Restructuring internals... → refactor", "Tightening internal heuristics... that users don't directly control → refactor or fix", "When in doubt, prefer refactor or chore over feat or fix." No new CLI flag, command, or integration is added here. The one operator-visible delta — repos status no longer hiding leftover GitLab role diagnostics — is a display tightening, not a new feature. (The ! marker and BREAKING CHANGE trailer are already correctly present in the commit message, so that part is fine.)
    Remediation: Change the PR title and commit type from feat(#7782)!: to refactor(#7782)!:. Keep the ! marker and BREAKING CHANGE trailer.

  • [incorrect-doc] docs/contributing/gitlab-role-credentials.md:323 — The sentinel table under "Unconfigured vs unregistered vs failed" still maps "Shared secret absent (leftover Token() of a constructed shared Source)" to ErrSharedUnconfigured, noting only that "Resolve never returns it." This PR changes Selection.Token() in internal/gitlabroles/route.go to unconditionally return ErrUnconfigured (removing the if s.Source.Shared { err = ErrSharedUnconfigured } branch), confirmed by the PR's own updated test (internal/gitlabroles/route_test.go::TestTokenMissingAndEmpty, which now asserts ErrorIs(err, ErrUnconfigured) and NotErrorIs(err, ErrSharedUnconfigured) for a constructed shared Source). The doc row is now incorrect about Token(), not only about Resolve. ErrSharedUnconfigured and Source.Shared themselves are intentionally retained (still referenced by tests), so this is a doc-only fix, not a call to remove the sentinel.
    Remediation: Update the table row at docs/contributing/gitlab-role-credentials.md:323 to state that Token() also returns ErrUnconfigured for every unconfigured-secret case, including a constructed shared Source, and that ErrSharedUnconfigured is retained only as a legacy sentinel that current code paths do not return.


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.

@ggallen ggallen changed the title feat(#7782)!: drop GitLab migration-gate compatibility plumbing refactor(#7782)!: complete GitLab identity migration cleanup Sep 29, 2026
@ggallen
ggallen force-pushed the agent/7782-drop-dead-migration-compat branch from 24be525 to 226baeb Compare September 29, 2026 21:21
@fullsend-ai-review

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

Copy link
Copy Markdown

🤖 Finished Review · ❌ Failure (pre-flight connectivity check: GitHub API connectivity check failed (exit 1): Get "https://api.github.com/rate_limit": EOF) · Started 9:23 PM UTC · Completed 9:25 PM UTC

Commit: 226baeb · View workflow run →

Runtime: pi · Model: sonnet · Effort: high

@github-actions

github-actions Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Site preview

Preview: https://43a6d9e9-site.fullsend-ai.workers.dev

Commit: 1a792f9e58191c734ff879cb22951de39f24d599

@ggallen

ggallen commented Sep 29, 2026

Copy link
Copy Markdown
Member

/fs-fix

Rebase. Squash. Fix the CI test failures. Fix the review issues. Reply to and resolve all threads.

@ggallen

ggallen commented Sep 29, 2026

Copy link
Copy Markdown
Member

/fs-review

@fullsend-ai-coder

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

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 9:32 PM UTC · Completed 9:45 PM UTC

Commit: 226baeb · View workflow run →

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

@fullsend-ai-review

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

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 9:33 PM UTC · Ended 9:58 PM UTC

Commit: 226baeb · View workflow run →

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 1 (human-triggered)

Rebase and squash were both no-ops: the branch is already based on main (mergeable=MERGEABLE) and already a single commit. Both bot review findings were checked against the current HEAD and found already resolved by a later push than the one the review analyzed, so no doc/commit-message edits were made. The one failing project CI job (functional-tests) fails during OpenShell/Podman infra setup before any test runs and is unrelated to this PR's Go changes; the actual Go test job in CI already passes. No code changes were made this run; recommend rerunning functional-tests and re-running review to reflect the already-resolved findings.

Fixed (2):

  1. Human /fs-fix instruction: rebase onto main: Checked mergeability (mergeable=MERGEABLE, not CONFLICTING) and ancestry: merge-base(HEAD, origin/main) already equals origin/main, so the branch is already rebased onto the current local origin/main ref. No git rebase was needed; this is the step-3 no-op case.
  2. Human /fs-fix instruction: squash the PR: Computed the whole-PR range (merge-base(HEAD, origin/main)..HEAD) and found it already contains exactly one commit (226baeb). The PR is already a single commit, so squash is a no-op; history_rewritten was not set.

Disagreed (4):

  1. [intent-alignment] PR title/commit type should be refactor(Remove GitLab identity migration machinery and retain only the three built-in roles #7782)! instead of feat(Remove GitLab identity migration machinery and retain only the three built-in roles #7782)!: This finding was raised by the review agent against an earlier revision of the branch (head 24be525, per the review comment's Head SHA marker). The branch was subsequently updated, and the current HEAD (226baeb) plus the current PR title already use refactor(#7782)!: with the ! marker and BREAKING CHANGE trailer intact. No further change is needed.
  2. [incorrect-doc] docs/contributing/gitlab-role-credentials.md:323 should describe Token() returning ErrUnconfigured for a constructed shared Source instead of the stale ErrSharedUnconfigured row: This finding was also raised against the earlier revision (head 24be525). In the current HEAD (226baeb), the 'Shared secret absent (leftover Token() of a constructed shared Source)' table row cited by the finding has already been removed from the doc, and a repo-wide search confirms ErrSharedUnconfigured, Source.Shared, and the referenced TestTokenMissingAndEmpty test no longer exist anywhere in the Go source. The finding's premise (that the sentinel is 'intentionally retained') no longer holds, and no doc change is required.
  3. Human /fs-fix instruction: fix the CI test failures: Inspected all non-Fullsend project CI jobs on the PR head (226baeb). Only one project job failed: 'functional-tests' (workflow 'Functional Tests'). Its log shows the failure is entirely inside the 'Install OpenShell CLI' setup step: the runner's local OpenShell gateway could not reach a Podman API socket ('no responsive Podman API socket found'), so the gateway never started and no test code executed. This is a CI-runner infrastructure problem unrelated to this PR's Go changes. Corroborating evidence: the sibling 'test' job (workflow 'CI'), which runs the actual Go unit-test suite, passed; and locally, go build ./... and go test -race ./internal/gitlabroles ./internal/repos ./internal/scaffold (the packages this PR touches, per the PR's own Verification section) all pass. go test ./internal/cli showed two failures (TestPostScriptEnv_NoSchemaAppendedWhenEmpty, TestPostScriptEnv_NoSchemaAppendedWhenNoValidationLoop), but these are caused by the fix-agent sandbox's own environment already exporting FULLSEND_OUTPUT_SCHEMA (which postScriptEnv/childScriptEnv inherit via os.Environ()) — unrelated to this PR's diff (internal/cli/run.go's only change here is a comment edit) and not reproducible in the real GitHub Actions runner, as confirmed by that job's green result in the PR's own CI run. No code change is warranted for either CI signal; recommend rerunning the functional-tests job.
  4. Human /fs-fix instruction: reply to and resolve all threads: The fix agent has no tool access to post PR comments or resolve review threads — that is the post-script's responsibility. This PR currently has zero inline review comments/threads (only the single bot summary review), and that review's two findings are already stale/resolved as explained above, so there is nothing substantive left to reply to. Recommend a fresh /fs-review (or the post-script's summary comment) to reflect the resolved state; a human or the post-script must perform any actual thread-resolution API calls.

Tests: passed

CI inspections (11):

  1. functional-tests (transient-infra, failure): Failure occurs entirely within the 'Install OpenShell CLI' setup step: the local OpenShell gateway's Podman compute driver found no responsive Podman API socket, so the gateway service crash-looped and never became reachable; no test code executed before the step failed. Rerun the functional-tests job. This is a CI-runner/Podman-socket infrastructure issue, not caused by this PR's Go source changes.
  2. test (passing, success)
  3. gate (passing, success): Functional Tests workflow gate job.
  4. e2e (passing, success)
  5. behaviour (pending, in_progress)
  6. test-sandbox-darwin (passing, success)
  7. commit-lint (passing, success)
  8. web (passing, success)
  9. build (passing, success): Build Site workflow.
  10. codecov/patch (passing, success)
  11. DCO (passing, success)

Conflict update: skipped; strategy none; forge MERGEABLE; target main@cf42bb73c65fe43c6918d86c05b0c41c5463fd53

Decision points
  • Treated both review findings as already resolved rather than re-applying the described edits (alternatives: Re-apply the literal remediations (change commit type; edit the doc table row) even though the described stale state no longer exists, Verify current HEAD against each finding's cited evidence and skip edits that no longer apply; rationale: The review comment's embedded Head SHA (24be525) does not match the current PR head (226baeb) and is not even present in the local repository, confirming the branch was updated after the review ran. Direct inspection of the current title, commit message, doc file, and Go source shows both findings' cited problems no longer exist, so re-applying the remediations would be a no-op or could reintroduce already-removed content.)

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[bot]

This comment was marked as outdated.

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:33 PM UTC · Completed 9:58 PM UTC

Commit: 226baeb · View workflow run →

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

@ggallen ggallen removed the fullsend-no-fix Skip bot-triggered fix agent runs label Sep 29, 2026
@ggallen

ggallen commented Sep 29, 2026

Copy link
Copy Markdown
Member

/fs-fix

Rebase. Squash commits. Resolve and conflicts. Fix all review issues. Reply to and resolve all conversations.

@fullsend-ai-coder

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

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 11:56 PM UTC · Completed 12:04 AM UTC

Commit: 226baeb · View workflow run →

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

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 1 (human-triggered)

Rebased the branch onto main (origin/main had advanced past the branch's prior base), fixed both active low-severity review findings by restoring deleted test coverage for GitLab role routing and fail-closed shared-token behavior, then squashed the whole PR range into a single commit per the human's instruction. Two failing CI jobs (behaviour, functional-tests) were inspected and classified as transient infrastructure failures unrelated to this PR's code changes. Replying to/resolving PR conversation threads is outside the fix agent's authorized tool set and was recorded as a disagreement for the post-script/human to handle.

Fixed (4):

  1. Human /fs-fix: rebase onto main ((repo root)): origin/main had advanced (merge ci(#7751): start the Podman API socket with systemd in functional-tests #7845) since the branch's last rebase, so origin/main was not an ancestor of HEAD. Ran git rebase origin/main non-interactively; it applied cleanly with no conflicts.
  2. Human /fs-fix: squash commits ((repo root)): After the rebase and the review-finding fixes below, the branch had 2 commits ahead of origin/main. Folded both into a single commit via git reset --soft to the merge-base (origin/main tip) plus one new commit, per the squash procedure. Exactly 1 commit now separates origin/main from HEAD. The squash commit message covers the PR's original intent plus the restored test coverage added in this run.
  3. [test-weakened] internal/scaffold/select_gitlab_role_token_test.go:43 — collapsed select-gitlab-role-token.sh tests dropped poison-pill coverage for FULLSEND_FORGE_TOKEN fail-closed behavior (internal/scaffold/select_gitlab_role_token_test.go): Verified against the current test file: TestSelectGitLabRoleTokenBuiltins only sets the role secret and TestSelectGitLabRoleTokenMissingOrUnregisteredFailsClosed uses a fully empty env, so neither would catch a regression that prefers FULLSEND_FORGE_TOKEN. Restored a compact test (TestSelectGitLabRoleTokenIgnoresSharedForgeToken) with two cases: (1) FULLSEND_FORGE_TOKEN set without the role secret fails closed naming the role secret (not the shared token), and (2) both set uses the role secret, not the shared token. Adapted from the pre-cleanup TestSelectGitLabRoleToken_DisabledUsesRoleToken/_EnforcedMissingRoleSecretFails tests found in git history.
  4. [missing-test] internal/gitlabroles/route_test.go:58 — TestJobForAgentPrefersRegisteredAgentName and TestJobForAgentFallsBackToHarnessRole were deleted and not replaced (internal/gitlabroles/route_test.go): Verified JobForAgent (route.go) still has both branches (registered agent name wins; unmapped name falls back to a registered harness role) with only indirect, partial coverage left. Restored both original tests verbatim from git history (commit 35f7227), which still compile and pass against the current JobForAgent signature and mustParseRegistry helper in registry_test.go.

Disagreed (1):

  1. Human /fs-fix: "Reply to and resolve all conversations": Posting replies to PR review comments and resolving conversation threads are forge PR-mutation actions outside the fix agent's authorized tool set (no comment-posting or thread-resolution capability); the agent definition explicitly reserves PR mutations for the deterministic post-script. The two active (non-
    Details) review findings were addressed with code fixes above so the underlying threads are substantively resolved by content; a human or the post-script must perform the actual forge-side reply/resolve action.

Tests: passed

CI inspections (8):

  1. behaviour (transient-infra, failure): 15 of 33 behaviour scenarios failed, spanning unrelated areas (PR/issue dispatch, fork PR sync, SSRF hook, URL-sourced harnesses, etc.) via 'after scenario hook failed: harness run ... concluded with failure' against dispatched GitHub Actions runs in external test repos. Broad, cross-cutting failure pattern unrelated to this PR's GitLab role/credential internals points to external dispatch/API infrastructure flakiness rather than a code regression. Recommend rerunning the behaviour job; do not expect code changes in this PR to affect it.
  2. functional-tests (transient-infra, failure): Failed in the 'Install OpenShell CLI' step: openshell-gateway repeatedly crash-looped with 'no responsive Podman API socket found'. This is a CI runner/environment setup failure unrelated to this PR's Go source changes. Recommend rerunning the functional-tests job; if it persists across reruns, file an infra issue about Podman socket readiness in the functional-tests runner setup.
  3. build (passing, success)
  4. test (passing, success)
  5. e2e (passing, success)
  6. commit-lint (passing, success)
  7. codecov/patch (passing, success)
  8. DCO (passing, success)
Decision points
  • Order of operations: rebase, then implement review fixes, then squash (alternatives: Squash first, then rebase, Rebase, squash immediately (no-op check), then append fixes as a second commit; rationale: The fix-history-rewrite skill requires rebase before squash when both are requested, and requires same-run fix work to be folded into the single squash commit rather than appended afterward, so fixes were implemented before running the squash mechanics.)

Removed a Signed-off-by trailer from 1 agent commit.

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-coder Bot added a commit that referenced this pull request Sep 30, 2026
Make GitLab role authentication unconditional for built-in and
registered custom roles. Remove migration modes, status fields,
runtime shared-token fallback, and migration-only diagnostics while
retaining role rotation, lifecycle checks, custom-role registration,
and legacy shared-credential retirement cleanup.

Also restores two low-severity test-coverage gaps the review agent
flagged after the migration-compat cleanup collapsed several test
files:

- internal/gitlabroles/route_test.go: restore
  TestJobForAgentPrefersRegisteredAgentName and
  TestJobForAgentFallsBackToHarnessRole, covering a registered custom
  agent name winning over a different harness role, and an unmapped
  agent name falling back to a registered harness role.
- internal/scaffold/select_gitlab_role_token_test.go: restore a
  compact case asserting select-gitlab-role-token.sh fails closed on
  the role secret name when only the leftover FULLSEND_FORGE_TOKEN is
  set, and a happy-path case where both are set but the role secret is
  used.

BREAKING CHANGE: GitLab role authentication no longer supports
migration modes or FULLSEND_FORGE_TOKEN runtime fallback; registered
role credentials are required.

Co-authored-by: Codex <codex@openai.com>

Addresses #7846
@fullsend-ai-coder
fullsend-ai-coder Bot force-pushed the agent/7782-drop-dead-migration-compat branch from 226baeb to 0a8e512 Compare September 30, 2026 00:04
@fullsend-ai-review fullsend-ai-review Bot removed the risk/moderate PR risk: moderate label Sep 30, 2026
@fullsend-ai-review
fullsend-ai-review Bot dismissed stale reviews from themself September 30, 2026 00:21

Superseded by updated review

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Sep 30, 2026
Make GitLab role authentication unconditional for built-in and
registered custom roles. Remove migration modes, status fields,
runtime shared-token fallback, and migration-only diagnostics while
retaining role rotation, lifecycle checks, custom-role registration,
and legacy shared-credential retirement cleanup.

Also restores two low-severity test-coverage gaps the review agent
flagged after the migration-compat cleanup collapsed several test
files:

- internal/gitlabroles/route_test.go: restore
  TestJobForAgentPrefersRegisteredAgentName and
  TestJobForAgentFallsBackToHarnessRole, covering a registered custom
  agent name winning over a different harness role, and an unmapped
  agent name falling back to a registered harness role.
- internal/scaffold/select_gitlab_role_token_test.go: restore a
  compact case asserting select-gitlab-role-token.sh fails closed on
  the role secret name when only the leftover FULLSEND_FORGE_TOKEN is
  set, and a happy-path case where both are set but the role secret is
  used.

BREAKING CHANGE: GitLab role authentication no longer supports
migration modes or FULLSEND_FORGE_TOKEN runtime fallback; registered
role credentials are required.

Co-authored-by: Codex <codex@openai.com>

Addresses #7846
@ggallen
ggallen force-pushed the agent/7782-drop-dead-migration-compat branch from 0a8e512 to 1a792f9 Compare September 30, 2026 20:34
@fullsend-ai-review

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

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:36 PM UTC · Completed 9:01 PM UTC

Commit: 1a792f9 · View workflow run →

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

@ggallen
ggallen added this pull request to the merge queue Sep 30, 2026
@fullsend-ai-review fullsend-ai-review Bot added the risk/moderate PR risk: moderate label Sep 30, 2026

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note: The following review comments could not be posted on the diff (GitHub returned 422) and are included here instead:

  • docs/contributing/gitlab-role-credentials.md (file-level): Line 351 · [low] incorrect-api-claim

Diagnose(present, registry) is documented as setting Ready when every registered role credential is present and satisfies the capability contract. gitlabroles.Diagnose only checks secret presence (Ready = total > 0 && configured == total); it never inspects capabilities, job mappings, or lifecycle. Capability/mapping checks live in CheckBuiltinReadiness / CheckRegisteredReadiness, which repos status AND-combines with Diagnose.Ready. The same bullet list already describes per-role state as presence-only, so this one line overstates the API.

Suggested fix: Describe Diagnose.Ready as presence-only (every registered role secret is present). Point readers to CheckBuiltinReadiness / CheckRegisteredReadiness (and repos status) for the capability/mapping contract.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

See the review comment for full details.

Comment thread internal/gitlabroles/route.go
Comment thread internal/gitlabroles/gitlabroles_test.go
Comment thread internal/gitlabroles/readiness.go
@fullsend-ai-review
fullsend-ai-review Bot dismissed their stale review September 30, 2026 21:00

Superseded by updated review

@fullsend-ai-review fullsend-ai-review Bot removed the ready-for-merge All reviewers approved — ready to merge label Sep 30, 2026
@fullsend-ai-coder

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

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 9:02 PM UTC · Completed 9:05 PM UTC

Commit: 1a792f9 · View workflow run →

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

Merged via the queue into main with commit d9bda6a Sep 30, 2026
64 of 66 checks passed
@ggallen
ggallen deleted the agent/7782-drop-dead-migration-compat branch September 30, 2026 21:03
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 2 (bot-triggered)

PR #7846 is already merged; stopped without making changes per the closed/merged PR rule.

Disagreed (1):

  1. Review feedback / fix request for PR refactor(#7782)!: complete GitLab identity migration cleanup #7846: PR refactor(#7782)!: complete GitLab identity migration cleanup #7846 is already merged (state=MERGED); per procedure, no fixes are made once a PR is closed or merged. No code changes were attempted.

Tests: failed

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

This branch was successfully deployed

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

Labels

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.

Remove GitLab identity migration machinery and retain only the three built-in roles

1 participant