Repository navigation
feat(#7782)!: require GitLab role credentials at runtime - #7811
Conversation
Make poll, run, and post-review select the registered Poller, Analyst, Coder, or custom-role secret in every gate mode. Leftover disabled and rollback no longer authenticate with FULLSEND_FORGE_TOKEN or a directly-set GITLAB_TOKEN. Install, converge, status, uninstall, and scaffold still own the migration gate and leftover shared-token state. Custom-role registration, reuse, rotation, and capability checks are unchanged. Note: pre-commit could not fetch remote hook repos (HTTP 403). Local hooks were run directly: gofmt, go vet, trailing-whitespace/EOF, lint-docs-links, lychee, and scan-secrets. BREAKING CHANGE: GitLab runtime jobs require the matching role secret. FULLSEND_FORGE_TOKEN and a pre-set GITLAB_TOKEN are no longer used for authentication, including leftover disabled and rollback gates. Related to #7782
|
🤖 Finished Review · ✅ Success · Started 8:46 PM UTC · Completed 9:09 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $8.12 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Site previewPreview: https://c724dbbd-site.fullsend-ai.workers.dev Commit: |
|
Looks good to me Previous runReviewFindingsLow
Other observations (non-blocking)
Next steps:
Previous run (2)ReviewFindingsHigh
Medium
Low
Other observations (non-blocking)
Next steps:
Previous run (3)ReviewFindingsHigh
Medium
Low
Other observations (non-blocking)
Next steps:
Previous run (4)ReviewFindingsCritical
High
Medium
Low
Other observations (non-blocking)
Next steps:
|
|
🤖 Finished Fix · ✅ Success · Started 9:11 PM UTC · Completed 9:34 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $8.03 |
…h role-only runtime auth The Go-side change in this PR made gitlabroles.Resolve/Select/SelectAgent role-credential-only in every gate mode, but the GitLab CI scaffold templates and docs still assumed disabled/rollback shared one token across every role. That assumption is now false: a forged/unverified STAGE in disabled/rollback could select a specific higher-privileged role token instead of an equally-privileged shared one. - select-gitlab-role-token.sh: drop the disabled/rollback shared-token path; every mode now resolves the registered per-role secret, matching gitlabroles.Resolve. - run-agent-job.sh: require HMAC-verified STAGE before the role-specific re-select in every gate mode, not just migrating/enforced. - run-poll-job.sh: blank sibling role secrets unconditionally, since a sibling secret is now a real higher-privilege credential in every mode. - gitlab-role-credentials.md, configuring-gitlab.md, operations.md, architecture.md, infrastructure-reference.md, ADR 0067: update stale disabled/rollback shared-token prose and fix the self-contradictory paragraph in gitlab-role-credentials.md. - gitlabroles.go: document that Diagnose.Ready is install/converge-state readiness, not a runtime-readiness signal, and add a regression test locking in the known Diagnose/Resolve divergence for leftover modes (status/converge are intentionally out of scope for this PR). - route_test.go: name the unset-mode subtest "unset" instead of "" so go test -run can target it and failure output isn't ambiguous. Updates the existing scaffold/gitlabroles test suites for the new behavior and adds coverage for disabled/rollback role-token resolution. Addresses review feedback on #7811.
🔧 Fix agent — iteration 1 (bot-triggered)Addressed the Critical fail-open finding by removing the disabled/rollback shared-token exception from the GitLab CI scaffold (select-gitlab-role-token.sh, run-agent-job.sh) so STAGE must be cryptographically verified before a role-specific credential is selected in every gate mode, matching the Go-side gitlabroles.Resolve change; also closed the same class of gap in run-poll-job.sh's sibling-secret blanking. Fixed all six stale-doc findings (gitlab-role-credentials.md's self-contradictory paragraph, configuring-gitlab.md, operations.md, architecture.md, infrastructure-reference.md, ADR 0067) to describe role-only runtime authentication in every gate mode. Fixed the Low naming-convention finding in route_test.go. For the Medium Diagnose/Resolve readiness-divergence finding, documented the intentional gap and added a regression test rather than changing status/converge-adjacent code that this PR intentionally leaves unchanged. Updated and extended the Go test suites (internal/gitlabroles, internal/scaffold) for all behavior changes; full build, go vet, and the affected test packages pass, along with docs-link lint, lychee, ADR lint hooks, and gofmt. Fixed (9):
Tests: passed CI inspections (12):
Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 9:36 PM UTC · Completed 9:59 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $9.51 |
|
Risk Assessment: moderate (2/5) DetailsBreaking runtime cleanup across 29 files with moderate churn, anchored to prior assessment as Tier 1 signals remain identical (zero protected paths, no dependency changes, 31% test ratio, bot authorship) with tight alignment to the staged slice of #7782. Previous runRisk Assessment: moderate (2/5) DetailsBreaking runtime cleanup across 29 files with moderate churn, anchored to prior assessment as Tier 1 signals remain in identical ranges (zero protected paths, no dependency changes, 31% test ratio, bot authorship) with tight alignment to the staged slice of #7782. Previous run (2)Risk Assessment: moderate (2/5) DetailsBreaking runtime cleanup across 28 files with moderate churn, anchored to prior assessment as Tier 1 signals remain in identical ranges (zero protected paths, no dependency changes, 29% test ratio, bot authorship) with tight alignment to the staged slice of #7782. Previous run (3)Risk Assessment: moderate (2/5) DetailsBreaking runtime cleanup across 27 files with moderate churn, but no protected paths, no dependency changes, 30% test ratio, bot authorship, and tight alignment with the disclosed staged slice of #7782 keep composite risk moderate. |
|
🤖 Finished Fix · ✅ Success · Started 10:01 PM UTC · Completed 10:14 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $2.33 |
…role-credential docs Addresses review feedback on PR #7811. Diagnose's leftover-mode messages ("legacy shared-token path ready" / "configured but unused" / "not required") implied the shared FULLSEND_FORGE_TOKEN path still works for job execution; they now say plainly that fullsend poll/run require the registered role secret at runtime regardless of converge-state readiness. Diagnose.Ready itself is left as install/converge-state readiness (unchanged, per the disclosed #7782 scope decision already captured in TestDiagnoseReadyDivergesFromResolveOnLeftoverModes). Also brings several docs pages current with role-credential-only runtime auth in every gate mode (including leftover disabled/rollback, which restores install-state only, never runtime auth): docs/cli/repos.md, docs/contributing/bot-identities.md, docs/contributing/gitlab-role-credentials.md, and the Testing/Differences sections of docs/guides/getting-started/configuring-gitlab.md. Addresses #7811
🔧 Fix agent — iteration 2 (bot-triggered)Addressed all 7 findings from the current review round on PR #7811: fixed five stale/incorrect docs pages (docs/cli/repos.md, docs/contributing/bot-identities.md, docs/contributing/gitlab-role-credentials.md twice, docs/guides/getting-started/configuring-gitlab.md) that still scoped role-credential-only runtime auth to migrating/enforced or implied a working shared-token runtime path, reworded gitlabroles.Diagnose's leftover-mode diagnostic messages so they no longer imply a working shared-token runtime path while intentionally leaving the disclosed Ready/Resolve install-vs-runtime divergence unchanged, and flagged (without editing, since PR-description edits are a post-script/maintainer action) that the PR description understates the CI scaffold and approval-capability changes already in the diff. Project CI on the PR head SHA is entirely green (no pr-related failures to fix). Fixed (7):
Tests: passed CI inspections (12):
Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 10:16 PM UTC · Completed 10:33 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $7.65 |
|
🤖 Finished Fix · ✅ Success · Started 10:35 PM UTC · Completed 10:47 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.88 |
…ential PR Update the Free-tier bot token warning and the rollback recovery paragraph in docs/cli/repos.md to stop implying FULLSEND_FORGE_TOKEN or the shared-credential path is used for GitLab runtime authentication -- runtime always selects the registered per-role secret. Also fix TestEnrichGitLabRoleStatusSharedOnlyOmitsRoleReadinessDiagnostics, which started failing CI after the prior fix commit reworded the leftover disabled/rollback diagnostic message to include "the registered role secret"; narrow the assertion to the specific readiness-line pattern it was actually guarding against instead of the broader substring. Addresses #7811
🔧 Fix agent — iteration 3 (bot-triggered)Fixed the two remaining stale-doc findings (Free-tier bot token warning, rollback recovery paragraph) that still implied GitLab runtime auth could fall back to the shared FULLSEND_FORGE_TOKEN. Verified and drafted the corrected text for the PR-summary scope-accuracy finding but could not publish it myself (PR body edits are blocked by sandbox policy / reserved for the post-script). Also fixed a project-CI test failure caused by the previous fix commit's diagnostic-message reword, which broke an over-broad assertion in a downstream test. Fixed (4):
Tests: passed CI inspections (10):
Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 10:49 PM UTC · Completed 11:03 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $5.54 |
|
🤖 Finished Fix · ✅ Success · Started 11:05 PM UTC · Completed 11:12 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $0.78 |
🔧 Fix agent — iteration 4 (bot-triggered)No repository code changes were needed this iteration: all prior review findings (a critical fail-open gap and several stale-doc/logic-error issues) were already resolved in earlier commits, and all project CI checks are green. The one remaining finding asks for an edit to the PR description text itself; that edit is a forge-metadata mutation outside the fix agent's tool access (confirmed by a sandbox policy rejection when attempted), so it is recorded as a disagreement with the exact suggested replacement wording for the post-script or a maintainer to apply. Disagreed (1):
Tests: passed CI inspections (10):
Decision points
Next steps:
|
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 11:27 PM UTC · Completed 11:37 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $3.93 |
Superseded by updated review
|
🤖 Finished Retro · ✅ Success · Started 11:56 PM UTC · Completed 12:04 AM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.53 |
|
PR #7811 (fullsend-ai/fullsend) implements 'PR1' of issue #7782, a GitLab identity-migration cleanup. Two things stand out. Rework before the PR even existed. The human (ggallen) had to dispatch PR review pipeline worked as intended. PR #7811 went through 5 review rounds / 4 fix rounds, but the findings were substantive, not busywork: round 1 caught a real Critical fail-open gap (GitLab CI scaffold scripts not updated in lockstep with the Go-side role-only auth change), plus stale-doc and logic-error findings in later rounds. The final review returned clean ('Looks good to me') and the human approved within minutes. One Low finding (PR description's Summary section contradicting its own Changes section) recurred across all 4 rounds because the fix agent is not permitted to edit PR descriptions itself -- this is evidence for two already-open issues rather than a new one: #3935 (review agent should flag PR-description-vs-diff contradictions) and #2418 (fix agent should update PR title/body when the implementation approach changes). No new proposal filed for this. Discovered agents repo: fullsend-ai/agents (agent defs, skills, harness configs resolved at runtime; both the code and review runs pinned to fullsend-ai/agents@main). Two proposals below target the recurring timeout/discard/re-scope pattern, which is the highest-leverage, non-duplicated finding from this retro. Proposals filed |
Summary
Make GitLab runtime authentication role-credential only.
fullsend poll,fullsend run, andfullsend post-reviewnow select the registered Poller, Analyst, Coder, or custom-role secret in every gate mode, including leftoverdisabledand explicitrollback. There is no shared-token path and no fallback to a directly-setGITLAB_TOKEN.This is PR1 of the GitLab identity-migration cleanup. Install, converge, status, and uninstall behavior remain unchanged. The CI scaffold scripts and approval capability checks now enforce role credentials and HMAC verification unconditionally in every gate mode; custom-role registration, reuse, and rotation remain unchanged.
Related Issue
#7782 (PR1: runtime authentication only)
Changes
gitlabroles.Resolve/Select/SelectAgentalways require the registered role secret; leftoverdisabled/rollbackno longer selectFULLSEND_FORGE_TOKEN.fullsend runfails closed when the role secret is missing instead of leaving a pre-setGITLAB_TOKEN.Testing
go testfor./internal/gitlabroles/and GitLab CLI runtime tests (including leftover disabled/rollback fail-closed cases)go veton changed packagesmake lint—pre-commitcould not fetch remote hook repos in this sandbox (HTTP 403); local equivalents were run insteadChecklist
feat(#7782)!:withBREAKING CHANGEtrailer)Related to #7782
Post-script verification
agent/7782-gitlab-runtime-auth)afa135d45d43450d9ebd14d83c6e7d5f862fe21b..HEAD)