fix: preserve custom secrets during legacy secrets_store migration

[simonrosenberg]

HUMAN:

• A human has tested these changes.

AGENT: Implemented the fix proposed in #102's comment; added regression tests against both FileSecretsStore and the production SaasSecretsStore (encrypted Postgres) path; added migration telemetry. See Notes — this is an OSS/self-hosted-only correctness fix; not connected to Codex auth or any enterprise/SaaS issue.

---

Why

invalidate_legacy_secrets_store (the one-time migration that moves Settings.secrets_store.provider_tokens into the dedicated SecretsStore) built a Secrets document containing only provider_tokens and passed it to secrets_store.store(...). Both SaasSecretsStore.store() and FileSecretsStore.store() treat the passed Secrets object as the entire document — SaasSecretsStore deletes all existing custom-secret rows for the user before inserting whatever is in the payload, and FileSecretsStore overwrites the whole file. Since the migration never populated custom_secrets, every GET /api/v1/settings for a user who still had legacy provider_tokens silently deleted all of that user's custom secrets.

This is fully live today for self-hosted/OSS installs (FileSettingsStore does a full JSON round-trip of Settings, so secrets_store.provider_tokens genuinely persists from before the dedicated SecretsStore existed). On SaaS/enterprise (cloud and replicated alike), SaasSettingsStore has never persisted or restored secrets_store at all, so the migration guard can never trip there — see Notes.

Summary

• Load the existing Secrets document before building the migrated one, and carry its custom_secrets forward instead of dropping them.
• Merge provider_tokens with the dedicated store's existing values taking precedence, so a stale legacy token can't clobber one already reconnected/rotated there.
• Log custom-secret and provider-token counts on every migration, so we can tell whether this path is still firing in production and whether provider tokens are surviving it on stores (like SaaS) that don't durably persist them.
• Regression tests against both FileSecretsStore (tests/unit/app_server/test_invalidate_legacy_secrets_store.py) and the production SaasSecretsStore path — real delete-then-insert + JWT encryption (enterprise/tests/unit/test_invalidate_legacy_secrets_store_saas.py).

Issue Number

Fixes #102

How to Test

• uv run pytest tests/unit/app_server/test_invalidate_legacy_secrets_store.py -q — fails without the fix (confirmed by reverting locally), passes with it.
• PYTHONPATH=".:$PYTHONPATH" poetry run --project=enterprise pytest ./enterprise/tests/unit/test_invalidate_legacy_secrets_store_saas.py ./enterprise/tests/unit/test_saas_secrets_store.py -q — SaaS path, same result.
• uv run pytest tests/unit/app_server/test_settings_api.py tests/unit/app_server/test_secrets_api.py -q — no regressions.
• uv run mypy openhands/app_server/settings/settings_router.py and ruff check/ruff format (both root and enterprise/dev_config/python/ruff.toml configs) — clean.

Type

• Bug fix

Notes

Scope: OSS/self-hosted only, not Codex/enterprise-relevant. #102 originally speculated this bug could explain part of the "no CODEX_AUTH_JSON found" failure bucket from #120. That's now ruled out, not just unlikely — it's timeline-impossible. CODEX_AUTH_JSON and ACP didn't exist until 2026-03/05, five to seven months after the settings table (the only thing that could ever populate secrets_store.provider_tokens and trip this migration's guard) was dropped with no backfill in 523b40dbf / OpenHands/OpenHands#11469 (2025-10-22). Migrations apply strictly in order, so any codebase with Codex ACP support already contains that table drop — the two states can't coexist on any install, cloud or replicated. Full writeup on #102. This PR's actual value is the OSS FileSettingsStore custom-secret-loss fix, full stop.

This lands the credential-agnostic fix recommended in the issue thread, independent of the CODEX_AUTH_JSON-specific allowlist in #101 (which addresses a different scenario — a stale write-back overwriting a rotated credential — not this migration). The residual design flaw (any caller holding one field can still overwrite the whole document via store()) is a separate, larger decision tracked elsewhere in that thread.

Found, not fixed, and closed as not-planned (was #122): SaasSecretsStore.store() never persists provider_tokens (del kwargs['provider_tokens']), and load() never returns them — a real defect in that store's contract. Initially filed as its own issue, but on closer check there is currently no live path that feeds it non-empty provider_tokens for a SaaS user: (1) SaasSettingsStore has never persisted/restored secrets_store, so this migration's guard can't trip for SaaS; (2) the only other caller, POST /api/v1/secrets/git-providers (manual PAT entry), is hidden entirely for SaaS in the frontend ({!isSaas && ...} around every provider's token input in git-settings.tsx) — SaaS connects providers via OAuth App/Keycloak instead. So this is dead code today, not an active production bug; closed #122 rather than track a fix against a path that doesn't exist. Test test_invalidate_legacy_secrets_store_saas_does_not_persist_provider_tokens documents the underlying behavior in case that ever changes (e.g. a future migration or a re-enabled PAT UI for SaaS).

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
openhands/app_server/settings
settings_router.py					390-421
Project Total

This report was generated by python-coverage-comment-action

[simonrosenberg]

Closing. The migration this fixes (invalidate_legacy_secrets_store, and the settings/secrets store split it bridges) shipped 2025-05-03 (ae990d3cb, #8213) — over 15 months ago. It's a one-shot migration: it fires at most once per user, the first time they hit GET /settings on a version containing it.

For anyone who's been actively upgrading OpenHands over that window, whatever was going to happen already happened, however long ago — this fix has no retroactive effect on data already lost. The only population it protects going forward is users still running 15-month-old code who eventually jump straight to a fixed version, or a fresh install that happens to reuse a pre-split config directory with stale legacy secrets. Given the project's release cadence, that population isn't worth carrying an open fix for.

The bug itself was real and correctly diagnosed (see #102) — this isn't a retraction of the finding, just a call that it's too old to be worth landing a fix for now. Leaving #102 open for the record; not implementing further.