From c7a9e811f9fd242e730a6594123e0514f81417ea Mon Sep 17 00:00:00 2001 From: Joe Laverty Date: Tue, 25 Aug 2026 14:56:44 -0400 Subject: [PATCH] fix: keep member settings rows a true delta over the org defaults --- enterprise/storage/saas_settings_store.py | 69 ++++++++++- .../tests/unit/test_saas_settings_store.py | 109 ++++++++++++++++-- 2 files changed, 164 insertions(+), 14 deletions(-) diff --git a/enterprise/storage/saas_settings_store.py b/enterprise/storage/saas_settings_store.py index 9409a396e..5671f6e33 100644 --- a/enterprise/storage/saas_settings_store.py +++ b/enterprise/storage/saas_settings_store.py @@ -105,6 +105,39 @@ def managed_llm_key_config_from_model( return ManagedLlmKeyConfig(openhands_type=openhands_type) +# Runtime values the SDK regenerates on every construction. They describe the +# moment a request ran, not a setting anyone chose, so they are stripped before +# persisting: storing one pins a stale value and makes every save look like a +# change to the field that carries it. +_VOLATILE_AGENT_SETTINGS_PATHS: tuple[tuple[str, ...], ...] = ( + ('agent_context', 'current_datetime'), +) + +_MISSING = object() + + +def _without_matching_values( + member: dict[str, Any], org: dict[str, Any] +) -> dict[str, Any]: + """Drop entries from ``member`` that already match the org-wide default. + + Recurses into nested dicts so changing one field stores that field alone + rather than the whole block it lives in. What is left is a genuine + override; what is dropped resolves through the org on load, and so keeps + following the org when an admin changes it. + """ + pruned: dict[str, Any] = {} + for key, value in member.items(): + org_value = org.get(key, _MISSING) + if isinstance(value, dict) and isinstance(org_value, dict): + nested = _without_matching_values(value, org_value) + if nested: + pruned[key] = nested + elif org_value is _MISSING or org_value != value: + pruned[key] = value + return pruned + + # ``Settings`` fields that are also ``Org`` columns. The save loop below copies # matching keys onto the ``Org`` row, so these are held back: they are org-wide # defaults, set through the permission-gated ``POST /orgs/app``. @@ -195,6 +228,27 @@ def _get_effective_llm_api_key( return org_member.llm_api_key return None + @staticmethod + def _agent_settings_dump(agent_settings: Any) -> dict[str, Any]: + """Dump agent settings to their persisted shape. + + Both sides of the member-vs-org comparison go through here so they are + built the same way; a key present on one side only would read as a + difference and be stored as an override. + """ + dumped = agent_settings.model_dump(mode='json', exclude={'llm': {'api_key'}}) + # Lives in its own column. + dumped.pop('mcp_config', None) + for *parents, leaf in _VOLATILE_AGENT_SETTINGS_PATHS: + target = dumped + for parent in parents: + target = target.get(parent) if isinstance(target, dict) else None + if not isinstance(target, dict): + break + if isinstance(target, dict): + target.pop(leaf, None) + return dumped + @staticmethod def _get_persisted_agent_settings(item: Settings) -> dict[str, Any]: """Dump the agent settings to persist as this member's override. @@ -203,11 +257,7 @@ def _get_persisted_agent_settings(item: Settings) -> dict[str, Any]: resolves to the org default. An ``agent_kind`` flip fills the new variant from SDK defaults, so persist only what the caller sent. """ - persisted = item.agent_settings.model_dump( - mode='json', - exclude={'llm': {'api_key'}}, - ) - persisted.pop('mcp_config', None) + persisted = SaasSettingsStore._agent_settings_dump(item.agent_settings) sparse_fields = item._agent_kind_changed_fields if sparse_fields is not None: persisted = { @@ -700,6 +750,15 @@ async def store(self, item: Settings): for key, value in effective_agent_settings_diff.items() if key not in MEMBER_PRIVATE_AGENT_KEYS } + # Keep the row a genuine delta: anything matching the org default + # is dropped so it resolves through the org on load, and so follows + # an admin changing that default later. + agent_settings_update = _without_matching_values( + agent_settings_update, + self._agent_settings_dump( + OrgStore.get_agent_settings_from_org(org) + ), + ) effective_conversation_diff = item.conversation_settings.model_dump( mode='json' ) diff --git a/enterprise/tests/unit/test_saas_settings_store.py b/enterprise/tests/unit/test_saas_settings_store.py index 207343663..a0faf7da0 100644 --- a/enterprise/tests/unit/test_saas_settings_store.py +++ b/enterprise/tests/unit/test_saas_settings_store.py @@ -2387,21 +2387,30 @@ async def test_agent_kind_flip_replaces_stale_member_diff( await store.store(loaded) diff = _member_diff(session_maker, fixture) - assert diff.get('agent_kind') == 'openhands' assert 'llm' not in diff assert 'acp_server' not in diff assert 'acp_model' not in diff + # ``openhands`` is the org default, so it needs no member-level entry. + assert 'agent_kind' not in diff + + with ( + patch('storage.saas_settings_store.a_session_maker', async_session_maker), + patch('storage.user_store.a_session_maker', async_session_maker), + patch('storage.org_store.a_session_maker', async_session_maker), + patch.object(SaasSettingsStore, '_ensure_api_key', new_callable=AsyncMock), + ): + loaded = await store.load() + assert loaded.agent_settings.agent_kind == 'openhands' @pytest.mark.asyncio -async def test_non_flip_save_still_persists_full_agent_settings( +async def test_non_flip_save_persists_only_the_changed_field( session_maker, async_session_maker, org_with_multiple_members_fixture ): - """A save that does not change agent_kind writes the full dump. + """An ordinary edit stores that field alone. - The sparse write is scoped to kind flips, where the SDK fabricates the - fields the caller did not send. An ordinary edit fabricates nothing, so it - persists in full. This pins that boundary. + Everything the member did not change matches the org default and resolves + through it on load, so the row stays a delta. """ fixture = org_with_multiple_members_fixture _seed_org_llm_and_clear_member(session_maker, fixture) @@ -2419,9 +2428,9 @@ async def test_non_flip_save_still_persists_full_agent_settings( ) await store.store(loaded) - diff = _member_diff(session_maker, fixture) - assert diff['llm']['model'] == 'openhands/hand-picked' - assert 'condenser' in diff or 'agent_context' in diff + assert _member_diff(session_maker, fixture) == { + 'llm': {'model': 'openhands/hand-picked'} + } @pytest.mark.asyncio @@ -2462,3 +2471,85 @@ async def test_explicit_llm_alongside_kind_flip_is_persisted( 'openhands/hand-picked' ) + + +@pytest.mark.asyncio +async def test_member_keeps_tracking_org_defaults_after_an_edit( + session_maker, async_session_maker, org_with_multiple_members_fixture +): + """Changing one setting must not detach a member from every other default. + + The save path receives the member's composed settings: the org defaults + with their own changes already merged in. Storing that whole view turns + each inherited value into an explicit override, so an admin changing an + org default afterwards reaches only the members who never opened settings. + """ + from sqlalchemy import select + from storage.org import Org + + fixture = org_with_multiple_members_fixture + _seed_org_llm_and_clear_member(session_maker, fixture) + store = SaasSettingsStore(str(fixture['member1_user_id'])) + patches = ( + patch('storage.saas_settings_store.a_session_maker', async_session_maker), + patch('storage.user_store.a_session_maker', async_session_maker), + patch('storage.org_store.a_session_maker', async_session_maker), + patch.object(SaasSettingsStore, '_ensure_api_key', new_callable=AsyncMock), + ) + + # The member changes one thing that has nothing to do with the LLM. + with contextlib.ExitStack() as stack: + for patcher in patches: + stack.enter_context(patcher) + loaded = await store.load() + loaded.update({'agent_settings_diff': {'enable_sub_agents': True}}) + await store.store(loaded) + + assert _member_diff(session_maker, fixture) == {'enable_sub_agents': True} + + # An admin then moves the org onto a different model. + with session_maker() as session: + org = session.execute( + select(Org).where(Org.id == fixture['org_id']) + ).scalar_one() + org.agent_settings = { + 'agent_kind': 'openhands', + 'llm': {'model': 'openhands/new-org-model', 'base_url': ORG_LLM_BASE_URL}, + } + session.commit() + + with contextlib.ExitStack() as stack: + for patcher in patches: + stack.enter_context(patcher) + after = await store.load() + + assert after.agent_settings.llm.model == 'openhands/new-org-model' + assert after.agent_settings.enable_sub_agents is True + + +@pytest.mark.asyncio +async def test_runtime_values_are_not_persisted( + session_maker, async_session_maker, org_with_multiple_members_fixture +): + """``current_datetime`` is regenerated per call and must not be stored. + + It describes when a request ran rather than a setting anyone chose. + Persisting it pins that moment: ``load()`` returns the stored value, so the + agent is handed a stale "now" on every later conversation. + """ + fixture = org_with_multiple_members_fixture + _seed_org_llm_and_clear_member(session_maker, fixture) + store = SaasSettingsStore(str(fixture['member1_user_id'])) + + with ( + patch('storage.saas_settings_store.a_session_maker', async_session_maker), + patch('storage.user_store.a_session_maker', async_session_maker), + patch('storage.org_store.a_session_maker', async_session_maker), + patch.object(SaasSettingsStore, '_ensure_api_key', new_callable=AsyncMock), + ): + loaded = await store.load() + loaded.update({'agent_settings_diff': {'enable_sub_agents': True}}) + await store.store(loaded) + + diff = _member_diff(session_maker, fixture) + assert 'current_datetime' not in diff.get('agent_context', {})