Repository navigation
refactor(#7782)!: complete GitLab identity migration cleanup - #7846
Conversation
|
🤖 Finished Review · ✅ Success · Started 8:16 PM UTC · Completed 8:38 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $6.78 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
Risk Assessment: moderate (2/5) DetailsModerate 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 runRisk Assessment: moderate (2/5) DetailsModerate 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) DetailsModerate 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. |
ReviewFindingsLow
Next steps:
Previous runLooks good to me Previous run (2)ReviewFindingsLow
Next steps:
Previous run (3)ReviewFindingsMedium
Next steps:
|
24be525 to
226baeb
Compare
|
🤖 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: Runtime: pi · Model: sonnet · Effort: high |
Site previewPreview: https://43a6d9e9-site.fullsend-ai.workers.dev Commit: |
|
/fs-fix Rebase. Squash. Fix the CI test failures. Fix the review issues. Reply to and resolve all threads. |
|
/fs-review |
|
🤖 Finished Fix · ✅ Success · Started 9:32 PM UTC · Completed 9:45 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.28 |
|
🤖 Review · ❌ Terminated · Started 9:33 PM UTC · Ended 9:58 PM UTC Commit: |
🔧 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):
Disagreed (4):
Tests: passed CI inspections (11):
Conflict update: skipped; strategy Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 9:33 PM UTC · Completed 9:58 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $13.44 |
|
/fs-fix Rebase. Squash commits. Resolve and conflicts. Fix all review issues. Reply to and resolve all conversations. |
|
🤖 Finished Fix · ✅ Success · Started 11:56 PM UTC · Completed 12:04 AM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.72 |
🔧 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):
Disagreed (1):
Tests: passed CI inspections (8):
Decision points
Removed a Signed-off-by trailer from 1 agent commit.Next steps:
|
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
226baeb to
0a8e512
Compare
Superseded by updated review
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
0a8e512 to
1a792f9
Compare
|
🤖 Finished Review · ✅ Success · Started 8:36 PM UTC · Completed 9:01 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $12.13 |
There was a problem hiding this comment.
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.
Superseded by updated review
|
🤖 Finished Fix · ✅ Success · Started 9:02 PM UTC · Completed 9:05 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $0.19 |
🔧 Fix agent — iteration 2 (bot-triggered)PR #7846 is already merged; stopped without making changes per the closed/merged PR rule. Disagreed (1):
Tests: failedNext steps:
|
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
FULLSEND_GITLAB_ROLE_MIGRATIONruntime handling and thedisabled,migrating,enforced, androllbackmodes.FULLSEND_FORGE_TOKENfallback paths.--gitlab-role-registry,--gitlab-role-token, own/reuse credentials, and BYOA routing.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_TOKENis no longer selected at runtime. Existing custom-role configurations remain supported through registered role credentials.Verification
make lintgo test -race ./internal/gitlabroles ./internal/repos ./internal/cli ./internal/scaffoldThis PR completes PR3 and closes #7782. The parent rollout issue (#7496) and its separate documentation follow-up (#7502) are tracked independently.
Closes #7782