[saphid]

Problem

A failed migration of legacy Bitbucket or GitHub credentials leaves the retired settings keys available for retry. A later settings write could strip those keys and lose the host configuration and token references. Creating a host with only an email could also discard the marker while the old token was still unreadable.

Change

The server retains pending legacy records alongside loaded settings and writes them back until that host has migrated. After the normal reload and migration retry, a guard rejects a requested patch that would create a host with a pending retired record, before that patch's secret-change and persistence phase. The reload itself may still migrate other readable secrets or settings.

Each save retries migration. Once the old token becomes readable and migration succeeds, the same patch can proceed. Existing hosts, hosts without pending retired records, and separate unrelated updates retain their existing behavior. A patch that includes a blocked host is rejected as a whole. Even explicit replacement credentials cannot create that host until the pending legacy migration is resolved; repairing the secret store or deliberately removing the old secret clears this restriction. The existing generic write-secret error is retained, without exposing credentials.

Scope and approval

A focused data-preservation bug fix in one server module and its existing tests. The legacy migration already intends to retry after a failed credential copy. Related #27756 handles secret-provider tokens and #29786 changes observability parsing; neither implements this migration repair.

Verification

• Before the guard, the extended migration test failed: an email-only host patch removed the retired Bitbucket key while the old token read still failed. The repair rejects the patch and preserves both the marker and the retry path. The test also checks that a separate unrelated save retains legacy data.
• After merging 4e0b528, the failing-store fixture was narrowed to the legacy token name so newer secret-normalization reads did not fail for an unrelated reason.
• The focused server-settings suite passed 75 tests before a formatting-only adjustment. Final exact-tree CI=true vp run check:changed passed 77 tests in 2 files, typecheck of 3 packages, format, lint, knip twice and GraphQL in Linux Boat. The local and sandbox trees and both file hashes matched.
• Original red-on-main and mutation checks from the earlier PR revision were not repeated in this pass. No real OS keychain failure or failure-then-recovery runtime flow was exercised; the test injects secret-store failures, and recovery was checked from source and the existing success-path test.

Fresh independent read-only review of the complete two-file candidate by GPT-6.1 Sol (high), through Codex in T3 Code, found no actionable code findings. All 8 required CI checks on 2ef066f pass. CodeRabbit reviewed this head with no actionable comments; the reported migration thread is resolved.

Created with Claude Opus 5.5 in Claude Code through T3 Code. Publication by GPT-6.1 Sol (high) through Codex in T3 Code.

[macroscopeapp]

Approvability

Verdict: Approved at 616e459

Macroscope's review found this PR approvable — Settings saves now retain legacy Bitbucket and GitHub fields when token migration fails, allowing migration to retry without losing configuration. The change is narrowly scoped and retains existing atomic-write safeguards.

^{You can add or adjust custom eligibility rules. Learn more.}

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

Settings migration now retains retired bitbucket and github fields when copying legacy secrets fails. Updates preserve those fields until migration succeeds. Successful migration removes the retired top-level fields.

Changes: Settings migration

Layer / File(s)	Summary
Retired source-control fields in settings encoding apps/server/src/serverSettings.ts	The encoder and writer accept retired source-control fields. The writer includes a field only when the corresponding sourceControlHosts entry is absent.
Migration and update state apps/server/src/serverSettings.ts, apps/server/src/serverSettings.test.ts	Loading and updates retain retired fields when secret copying fails, pass them through writes, and store them in the settings cache. Tests verify that an unrelated update preserves legacy data after a failed migration, that a host update fails while legacy data remains, and that successful migration removes the retired top-level keys.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested labels: bugfix

Suggested reviewers: juliusmarminge, t3dotgg

Merge Risk: Minimal · up to 2ef06

Saving settings no longer discards the pending Bitbucket or GitHub token migration marker. A host patch that would do so is rejected, and the migration can retry on the next load. No actionable merge-blocking risk remains.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title names the primary change: saving settings no longer drops retired Bitbucket and GitHub keys until their tokens are migrated. It matches the summary (load retains retired keys, writes preserv…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description check		The PR description follows the repository template. It has Problem, Change, Scope, and Verification sections, and each section has specific content. The Problem section states the failed legacy Bitbuc…

Full details: Title check

Explanation

The title names the primary change: saving settings no longer drops retired Bitbucket and GitHub keys until their tokens are migrated. It matches the summary (load retains retired keys, writes preserve them until the host exists or secret migration succeeds). The title is long (92 characters) but specific and conventional.

Full details: Description check

Explanation

The PR description follows the repository template. It has Problem, Change, Scope, and Verification sections, and each section has specific content. The Problem section states the failed legacy Bitbucket or GitHub credential migration and the data-loss risk. The Change section explains the retained pending legacy records and the guard that rejects host creation while a retired token is unreadable. The Scope section frames the change as a bug fix in one server module and links related PRs. The Verification section lists the focused test results, the CI status, and the checks that were not run. The template asks for the model and harness used by the agent. The description names Claude Opus 5.5 in Claude Code and GPT-6.1 Sol in Codex, so that item is covered. The description does not state a linked issue that shows the bug. The Scope section cites related PRs but no issue, and it does not say why a fix needs no issue. The template requires this for bug fixes, so the Scope section is partly incomplete. The verification also says the red-on-main and mutation checks were not repeated, so some verification evidence is missing. These gaps are not critical, so the description is mostly complete.

Finishing Touches

Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Recent review info

Run configuration

• Source mode: Auto
• Repository: pingdotgg/t3code
• Branch: main
• File: .coderabbit.config.ts
• Commit: 4e0b528
• Layers: Repository TS config
• Review profile: CHILL
• Plan: Advanced
• Run ID: 4b6cee01-51e1-4ee6-8263-6fa4fa630c6c

Commits

Reviewing files that changed from the base of the PR and between 616e459 and 2ef066f.

Files selected for processing: 2

• apps/server/src/serverSettings.test.ts
• apps/server/src/serverSettings.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

[coderabbitai]

• Fix comments: Fix CodeRabbit comments on this PR