fix(#6617): stop flagging legitimate GitLab variables as orphans

[fullsend-ai-coder]

Summary

Fix false-positive orphan variable detection on GitLab repos. repos status was flagging 7 legitimate variables as drifted because the managed variable set was incomplete for GitLab.

Changes

• Seed runtime state variables during install: add FULLSEND_DISPATCHED_KEYS_FAST, FULLSEND_DISPATCHED_KEYS_FULL, FULLSEND_FAILED_KEYS_FAST, FULLSEND_FAILED_KEYS_FULL to managedVarsForForge for GitLab with Dynamic: true and initial value {}
• Exclude required secrets from orphan detection: CheckOrphanVars now adds requiredSecretsForForge names to the managed set, since on GitLab secrets are stored as masked CI/CD variables and ListRepoVariables returns them. FULLSEND_FORGE_TOKEN is added to requiredSecretsForForge for GitLab
• Remove PerRepoGuardVar from managedVarsForForge for both forges per ADR-0044 (per-org mode deprecation). Kept in uninstall lists for cleanup of older installs
• Define all managed variable/secret names as typed constants in internal/forge/forge.go, replacing bare string literals across internal/repos/, internal/poll/, and internal/cli/repos*.go

Testing

• New test TestCheckOrphanVars_GitLabSecretsNotFlagged: verifies all legitimate GitLab variables (runtime state, secrets-as-variables, bot token) are not flagged as orphans
• New test TestCheckOrphanVars_GitLabDetectsGenuineOrphan: verifies a truly unknown variable is still correctly flagged
• New test TestCheckOrphanVars_PerRepoGuardVarFlaggedAsOrphan: verifies the deprecated guard variable is flagged when present from a previous install
• Updated TestInstallVarsForForge_GitLab to assert all 7 GitLab managed variables are present
• Updated GitLab probe and install component tests to include FULLSEND_FORGE_TOKEN as a required secret
• All internal/repos/, internal/poll/, and internal/cli/ tests pass with go test -race

---

Closes #6617

Post-script verification

• Branch is not main/master (agent/6617-fix-gitlab-orphan-vars)
• Secret scan passed (gitleaks — bfa5a2afa6f0e1ae46c6d7d7d142aded36372663..HEAD)
• PR body secret scan passed (gitleaks — no-git)

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:44 AM UTC · Completed 2:04 AM UTC

Commit: 6ad3204 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.76

[codecov]

Codecov Report

❌ Patch coverage is 98.11321% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/repos/status.go	83.33%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Risk Assessment: moderate (2/5)

Details

Score unchanged from prior assessment at 2 (moderate). Tier 1 signals identical to prior run: same file count, similar line count, bot author, zero protected/security/CI/dependency signals. Tier 2 confirms high churn on forge.go and install.go but single-author dominance and no revert history keep contention risk low.

Previous run

Risk Assessment: moderate (2/5)

Details

Tier 1 signals unchanged from prior run (same file count, similar line count 371-406, same bot author, zero protected/security/CI/dependency signals). Tier 2 confirms high churn on install.go and forge.go but single-author dominance and no revert history offset contention risk. No signals warrant deviation from prior score of 2.

Previous run (2)

Risk Assessment: moderate (2/5)

Details

Bug fix in repos management addressing false-positive orphan detection on GitLab. Bot author with no protected paths, CI changes, or dependency modifications. Moderate file count (19 files, 371 lines) with acceptable test coverage. High churn in install.go and multi-author contention on forge.go are offset by very detailed acceptance criteria, no revert history, and safe rollback characteristics. Anchored to prior score of 2 with no signals warranting deviation.

Previous run (3)

Risk Assessment: moderate (2/5)

Details

Bug fix in repos management addressing false-positive orphan detection on GitLab. Bot author with no protected paths, CI changes, or dependency modifications. Moderate file count (19 files, 371 lines) with good test coverage (37% test files). Standard bug fix scope with clear acceptance criteria from linked issue.

Previous run (4)

Risk Assessment: elevated (3/5)

Details

Elevated risk driven by high git churn: changed files are hotspots with heavy 30-day commit activity, many distinct authors, and numerous fix/revert commits. The PR itself is moderate in size (19 files, 346 lines) with no protected paths, no security-sensitive files, and no CI/dependency changes, keeping the metadata tier low. The linked issue is well-scoped and the PR addresses its four-part fix scope with tests.

Previous run (5)

Risk Assessment: elevated (3/5)

Details

Bug fix for false-positive GitLab drift detection with medium blast radius and high churn history in the repos module, partially mitigated by focused scope, comprehensive tests, and bot authorship.

[fullsend-ai-review]

Review

Findings

Low

• [test-coverage-gap] internal/repos/install_test.go:851 — TestRequiredSecretsForForge only tests with ForgeGitHub, never exercising the new ForgeGitLab branch that appends SecretForgeToken. While the new GitLab behavior is indirectly tested via TestCheckOrphanVars_GitLabSecretsNotFlagged and TestProbeComponents_GitLab_SkipsThinCallers, a direct unit test for requiredSecretsForForge(ForgeGitLab) would catch regressions in the function itself.
  Remediation: Add a test case calling requiredSecretsForForge(ForgeGitLab) and asserting the result contains 3 elements including forge.SecretForgeToken.

Previous run

Looks good to me

Previous run (2)

Review

Findings

Low

• [test-coverage-gap] internal/repos/install_test.go — TestCheckInstallComponents_GitLab_MissingSecrets only sets 3 of the now-required 7 GitLab variables (missing the four dispatched/failed keys variables). checkInstallComponents returns false due to missing variables AND missing secrets. The test passes but no longer isolates the "missing secrets" scenario it claims to test.
  Remediation: Add the four dispatched/failed keys variables to the test fixture so the only missing components are the secrets.

• [inconsistent-constant-usage] internal/repos/install_test.go, internal/repos/probe_test.go — TestCheckInstallComponents_GitLab_FullyInstalled, TestCheckInstallComponents_GitLab_MissingSecrets, and TestProbeComponents_GitLab_SkipsThinCallers use bare string literals for the three original GitLab variables and two secrets while using forge.Var*/forge.Secret* constants for the newly-added variables.
  Remediation: Replace the bare literals with forge.VarLastPollAtFast, forge.VarLastPollAtFull, forge.VarLabelState, forge.SecretGCPProjectID, and forge.SecretGCPWIFProvider.

• [awkward-comment-formatting] internal/repos/drift.go — The CheckOrphanVars doc comment has an awkward mid-sentence line break after removing the guard-variable clause: "...that are not in the managed / variable set for the given forge. These / are orphan variables".
  Remediation: Reflow the comment so sentence breaks fall at natural points.

Previous run (3)

Review

Findings

Low

• [test-coverage-gap] internal/repos/install_test.go — TestCheckInstallComponents_GitLab_MissingSecrets no longer isolates the scenario it claims to test. The test fixture only sets 3 of the now-required 7 GitLab variables, so checkInstallComponents returns false due to missing variables AND missing secrets. The test passes but does not verify that missing secrets alone cause a false result.
  Remediation: Add the four dispatched/failed keys variables to the test fixture so the only missing components are the secrets.

• [inconsistent-constant-usage] internal/repos/install_test.go — TestCheckInstallComponents_GitLab_FullyInstalled and TestCheckInstallComponents_GitLab_MissingSecrets use bare string literals for the three original GitLab variables (FULLSEND_LAST_POLL_AT_FAST, etc.) and two original secrets while using forge.Var*/forge.Secret* constants for the newly-added variables.
  Remediation: Replace the bare literals with forge.VarLastPollAtFast, forge.VarLastPollAtFull, forge.VarLabelState, forge.SecretGCPProjectID, and forge.SecretGCPWIFProvider.

• [inconsistent-constant-usage] internal/repos/probe_test.go — TestProbeComponents_GitLab_SkipsThinCallers uses bare string literals for the three original GitLab variables and secrets while using forge.Var*/forge.Secret* constants for the newly-added lines.
  Remediation: Replace the bare literals with the corresponding forge.* constants.

• [awkward-comment-formatting] internal/repos/drift.go — The CheckOrphanVars doc comment has an awkward mid-sentence line break after removing the guard-variable clause: "...that are not in the managed / variable set for the given forge. These / are orphan variables".
  Remediation: Reflow the comment so sentence breaks fall at natural points.

Previous run (4)

Review

Findings

Low

• [consumer-completeness] internal/repos/converge.go:768 — convergeSecrets short-circuits when hasSecrets is true (both GCP secrets exist), reporting ALL secret components as action:"none" with detail "{name} exists". Since ProbeComponents now includes FULLSEND_FORGE_TOKEN in required secrets for GitLab, if the GCP secrets exist but FULLSEND_FORGE_TOKEN does not, hasSecrets returns true (it only checks GCP secrets via secretsPresent), and convergeSecrets reports FULLSEND_FORGE_TOKEN as "exists" even though it is absent. The "none" action is cosmetic (no mutation occurs), but the detail string is misleading for operators inspecting convergence output.
  Remediation: In the hasSecrets branch, check c.Present before emitting the "exists" detail, or omit secret components that are not present from the early-return path.

• [missing-switch-case] internal/repos/converge.go:746 — initialVarValue does not handle the four new GitLab managed variables (VarDispatchedKeysFast, VarDispatchedKeysFull, VarFailedKeysFast, VarFailedKeysFull). These variables are in managedVarsForForge with initial value "{}" but are not in gitlabRequiredVariables, so convergence will never try to seed them via initialVarValue. The poller gracefully handles ErrNotFound by returning empty maps, so this is not a runtime defect. However, it means convergence cannot repair these variables if they are missing — they will only be created by repos install or the poller's first write.
  Remediation: Either add the four variables to gitlabRequiredVariables and add cases in initialVarValue returning "{}", or document that these variables are lazily initialized by the poller.

• [stale-comment] internal/repos/drift.go:161 — The OrphanVar doc comment says "with the FULLSEND_ prefix (or the guard variable)" and the CheckOrphanVars doc comment (line 178) says "(or the guard variable name)". The PR removed the special-case for the guard variable in the filter condition (the guard var has a FULLSEND_ prefix and is caught by that check naturally), so the "(or the guard variable)" parenthetical is now inaccurate.
  Remediation: Update both doc comments to remove the "(or the guard variable)" / "(or the guard variable name)" clause.

Previous run (5)

Review

Findings

Medium

• [slice-mutation] internal/repos/install.go:511 — requiredSecretsForForge uses append(requiredSecrets, forge.SecretForgeToken) on a package-level var slice. Currently safe because len==cap==2 forces a new allocation, but this is a latent defect: uninstall.go aliases the same backing array via var uninstallSecrets = requiredSecrets, and any future change that increases the initial capacity of requiredSecrets would cause append to mutate the shared backing array, silently corrupting uninstallSecrets. The same file already uses slices.Concat for the analogous case in uninstallVariables.
  Remediation: Replace with slices.Concat(requiredSecrets, []string{forge.SecretForgeToken}) or append(slices.Clone(requiredSecrets), forge.SecretForgeToken).

• [stale-doc] docs/guides/getting-started/operations.md:25 — Lists FULLSEND_PER_REPO_INSTALL as a managed repo variable ("auto-set by installer"). This PR removes it from managedVarsForForge — the installer no longer creates this variable.
  Remediation: Remove the FULLSEND_PER_REPO_INSTALL row from the variables table.

• [stale-doc] docs/guides/infrastructure/infrastructure-reference.md:252,260 — FULLSEND_PER_REPO_INSTALL is listed as a managed variable for both GitHub (line 252) and GitLab (line 260). This PR removes it from the managed variable set for both forges.
  Remediation: Remove FULLSEND_PER_REPO_INSTALL from both the GitHub and GitLab variable lists.

• [incomplete-doc] docs/guides/infrastructure/infrastructure-reference.md:264 — The GitLab CI/CD variables section is missing the 4 new managed variables added by this PR: FULLSEND_DISPATCHED_KEYS_FAST, FULLSEND_DISPATCHED_KEYS_FULL, FULLSEND_FAILED_KEYS_FAST, FULLSEND_FAILED_KEYS_FULL.
  Remediation: Add the 4 new variables alongside existing FULLSEND_LAST_POLL_AT_* and FULLSEND_LABEL_STATE entries.

Low

• [incomplete-scope] internal/repos/converge.go:748, internal/repos/status.go:56,306 — These files still use bare string literals ("FULLSEND_LAST_POLL_AT_FAST", "FULLSEND_GCP_REGION", etc.) instead of the new forge.* constants. Both files are within the issue's authorized scope (internal/repos/) and have corresponding constants defined.
  Remediation: Migrate the remaining bare literals to forge.* constants for consistency.

[ggallen]

/fs-fix - Rebase. Fix all the review issues. Resolve all the conversations.

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 2:20 AM UTC · Completed 2:32 AM UTC

Commit: 6ad3204 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.92

[fullsend-ai-coder]

🔧 Fix agent — iteration 1 (human-triggered)

Addressed all 5 review findings: fixed latent slice-mutation bug with slices.Concat, removed stale FULLSEND_PER_REPO_INSTALL from 2 doc files, added 4 missing GitLab variables to infrastructure-reference.md, and migrated remaining bare string literals in converge.go and status.go to forge.* constants.

Fixed (6):

1. slice-mutation in requiredSecretsForForge (internal/repos/install.go): Replaced append(requiredSecrets, forge.SecretForgeToken) with slices.Concat(requiredSecrets, []string{forge.SecretForgeToken}) to prevent latent slice-mutation when uninstallSecrets aliases the same backing array
2. stale-doc: FULLSEND_PER_REPO_INSTALL in operations.md (docs/guides/getting-started/operations.md): Removed FULLSEND_PER_REPO_INSTALL row from the GitHub variables table since the installer no longer creates this variable
3. stale-doc: FULLSEND_PER_REPO_INSTALL in infrastructure-reference.md (docs/guides/infrastructure/infrastructure-reference.md): Removed FULLSEND_PER_REPO_INSTALL from both GitHub and GitLab variable lists, and removed stale FULLSEND_FORGE from the GitLab list
4. incomplete-doc: missing 4 new GitLab managed variables (docs/guides/infrastructure/infrastructure-reference.md): Added FULLSEND_DISPATCHED_KEYS_FAST, FULLSEND_DISPATCHED_KEYS_FULL, FULLSEND_FAILED_KEYS_FAST, FULLSEND_FAILED_KEYS_FULL to the GitLab CI/CD variables section
5. incomplete-scope: bare string literals in converge.go (internal/repos/converge.go): Migrated all remaining bare FULLSEND_* string literals to forge.* constants: secretsPresent(), initialVarValue(), and convergeSecrets()
6. incomplete-scope: bare string literals in status.go (internal/repos/status.go): Migrated all remaining bare FULLSEND_* string literals to forge.* constants in ProbeRepoState() and checkRepoStatus()

Tests: passed

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]

🤖 Review · ❌ Terminated · Started 2:34 AM UTC · Ended 2:53 AM UTC

Commit: 90d2183 · View workflow run →

[github-actions]

Site preview

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

Commit: d757887077c2352efaa3be55cc8892f3e7014b82

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:34 AM UTC · Completed 2:53 AM UTC

Commit: 90d2183 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.51

[ggallen]

/fs-fix - Rebase. Fix all the review issues. Resolve all the conversations.

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 3:03 AM UTC · Completed 3:12 AM UTC

Commit: 90d2183 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.41

[fullsend-ai-coder]

🔧 Fix agent — iteration 2 (human-triggered)

Addressed all 3 review findings from the second review cycle: fixed misleading convergeSecrets detail for absent secrets, added 4 new GitLab variables to gitlabRequiredVariables and initialVarValue for convergence repair support, and removed stale guard-variable references from drift.go doc comments. Updated test fixtures accordingly.

Fixed (3):

1. consumer-completeness: convergeSecrets misleading detail for absent secrets (internal/repos/converge.go): In the hasSecrets early-return branch, added a c.Present check before emitting the detail string. Absent secrets (e.g. FULLSEND_FORGE_TOKEN on GitLab when only GCP secrets exist) now report 'not present (not managed by convergence)' instead of the misleading 'exists'.
2. missing-switch-case: initialVarValue missing 4 new GitLab managed variables (internal/repos/converge.go): Added VarDispatchedKeysFast, VarDispatchedKeysFull, VarFailedKeysFast, VarFailedKeysFull to the initialVarValue switch returning '{}', and added them to gitlabRequiredVariables in install.go so convergence can repair them if missing.
3. stale-comment: OrphanVar and CheckOrphanVars doc comments reference guard variable (internal/repos/drift.go): Removed the '(or the guard variable)' and '(or the guard variable name)' clauses from both doc comments since the guard variable has a FULLSEND_ prefix and is caught by the prefix check naturally.

Tests: passed

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]

🤖 Finished Fix · ✅ Success · Started 4:43 PM UTC · Completed 4:56 PM UTC

Commit: 1ebe991 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.90

[fullsend-ai-coder]

🔧 Fix agent — iteration 3 (human-triggered)

Addressed all 8 review findings. 4 were already fixed in previous iterations (slice-mutation, missing-switch-case, consumer-completeness, stale-comment). Fixed the remaining 4: replaced bare string literals with forge.* constants in install_test.go and probe_test.go for consistency, added missing test variables to TestCheckInstallComponents_GitLab_MissingSecrets to properly isolate the missing-secrets scenario, and reflowed the CheckOrphanVars doc comment.

Fixed (8):

1. inconsistent-constant-usage in install_test.go (internal/repos/install_test.go): Replaced bare string literals with forge.VarLastPollAtFast, forge.VarLastPollAtFull, forge.VarLabelState, forge.SecretGCPProjectID, and forge.SecretGCPWIFProvider in TestCheckInstallComponents_GitLab_FullyInstalled and TestCheckInstallComponents_GitLab_MissingSecrets
2. inconsistent-constant-usage in probe_test.go (internal/repos/probe_test.go): Replaced bare string literals with forge.VarLastPollAtFast, forge.VarLastPollAtFull, forge.VarLabelState, forge.SecretGCPProjectID, and forge.SecretGCPWIFProvider in TestProbeComponents_GitLab_SkipsThinCallers
3. awkward-comment-formatting in drift.go (internal/repos/drift.go): Reflowed the CheckOrphanVars doc comment so sentence breaks fall at natural points, removing the awkward mid-sentence line break
4. test-coverage-gap in install_test.go (internal/repos/install_test.go): Added the four missing dispatched/failed keys variables (VarDispatchedKeysFast, VarDispatchedKeysFull, VarFailedKeysFast, VarFailedKeysFull) to TestCheckInstallComponents_GitLab_MissingSecrets so the test isolates the missing-secrets scenario
5. stale-comment in drift.go (OrphanVar and CheckOrphanVars doc comments) (internal/repos/drift.go): Already addressed in a previous iteration — the guard variable parentheticals were removed from both doc comments
6. consumer-completeness in converge.go (convergeSecrets detail string) (internal/repos/converge.go): Already addressed in a previous iteration — convergeSecrets now checks c.Present before emitting the detail string, showing 'not present (not managed by convergence)' when the secret is absent
7. slice-mutation in install.go (requiredSecretsForForge) (internal/repos/install.go): Already addressed in a previous iteration — requiredSecretsForForge uses slices.Concat instead of append to avoid mutating the shared backing array
8. missing-switch-case in converge.go (initialVarValue) (internal/repos/converge.go): Already addressed in a previous iteration — initialVarValue handles VarDispatchedKeysFast, VarDispatchedKeysFull, VarFailedKeysFast, VarFailedKeysFull returning '{}', and these are included in gitlabRequiredVariables

Tests: passed

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]

🤖 Finished Review · ✅ Success · Started 5:06 PM UTC · Completed 5:22 PM UTC

Commit: 8ad4b0c · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.24

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:14 PM UTC · Completed 8:32 PM UTC

Commit: d757887 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.05

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 9:12 PM UTC · Completed 9:26 PM UTC

Commit: d757887 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.40

[fullsend-ai-retro]

PR #6620 (fix #6617: GitLab orphan variable false positives) completed same-day with a solid 19-file implementation by the code agent. The review agent found 1 medium-severity latent slice-mutation bug and ~10 low-severity findings across 6 review rounds. Three human-triggered /fs-fix cycles addressed all findings before merge.

Cost breakdown: ~$55 total ($10.76 code, $9.23 fixes, $44.30 reviews, $1.45 triage). Review costs were 4.8× fix costs.

Evidence supporting existing open issues:

• Review agent should catch all findings in the first pass to reduce rework cycles #1582 (first-pass thoroughness): Round 2's missing-switch-case finding (incomplete initialVarValue switch in converge.go) was visible in the Round 1 diff but not flagged. Round 3's inconsistent-constant-usage in tests mirrors Round 1's incomplete-scope in production code — same category, different file set.
• Review agent surfaces new low-severity findings across multiple approved passes #1367 / Review agent posts diminishing-value findings after approval #3025 (post-approval low findings): Rounds 3–6 cost $28.03 for exclusively low-severity stylistic findings after the PR was already approved — 63% of total review cost for minimal quality impact.
• Investigate 100% inline comment posting failure (422) on re-review rounds #6039 (422 comment posting): Two inline comments failed to post with HTTP 422 on re-review rounds.
• Review agent should produce consistent verdicts when re-reviewing similar code on the same PR #5107 (inconsistent verdicts): Round 5 found zero issues on commit 8ad4b0c; Round 6 found a new test-coverage-gap on the same code after rebase to d757887.

Other observations: The human reviewer instructed the fix agent not to include Signed-off-by trailers on the third /fs-fix iteration, suggesting the fix agent may have added them on earlier autonomous commits in violation of AGENTS.md policy. This could not be verified from the squash-rebased commit history — see proposal below. E2E test flakiness was also observed — see proposal below.

Proposals filed

• Add exponential backoff to E2E behaviour test dispatch polling (in fullsend-ai/fullsend)
• Add explicit DCO exemption guidance to fix agent definition (in fullsend-ai/agents)