Skip to content

fix(agent-server): persist confirmation policy, analyzer and secrets to meta.json - #4859

Closed
alanhuangyoo wants to merge 2 commits into
OpenHands:mainfrom
alanhuangyoo:fix/persist-session-mutations-to-meta
Closed

fix(agent-server): persist confirmation policy, analyzer and secrets to meta.json#4859
alanhuangyoo wants to merge 2 commits into
OpenHands:mainfrom
alanhuangyoo:fix/persist-session-mutations-to-meta

Conversation

@alanhuangyoo

Copy link
Copy Markdown
Contributor

HUMAN:

Turned on confirmation prompts and an analyzer through the API, restarted the server, and they were both back to what the conversation started with. They stick now.


AGENT:

Why

Closes #4810.

set_confirmation_policy, set_security_analyzer and update_secrets delegate to the conversation, which writes base_state.json, and never touch StoredConversation:

async def set_confirmation_policy(self, policy: ConfirmationPolicyBase):
    ...
    await loop.run_in_executor(None, self._conversation.set_confirmation_policy, policy)
    # nothing updates self.stored, nothing calls save_meta()

Startup reads all three off StoredConversation (event_service.py:1105-1117), so after eviction or a restart the conversation reverts to the values it was created with. For two of the three that means the security analyzer and confirmation policy silently regress to the creation-time setting.

It is worse than the issue describes: stored is stale in process as well, so it is not only a reload problem.

$ python repro.py
in-process confirmation_policy : NeverConfirm       <- set to AlwaysConfirm
in-process security_analyzer   : NoneType           <- set to LLMSecurityAnalyzer
in-process secrets keys        : []                 <- MY_TOKEN was set
meta.json confirmation_policy  : {'kind': 'NeverConfirm'}
meta.json security_analyzer    : None
after restart confirmation_policy: NeverConfirm

Summary

Why the normalisation is not incidental

update_secrets is typed dict[str, SecretValue], and SecretValue = str | SecretSource. StoredConversation.secrets is dict[str, SecretSource]. model_copy(update=...) does not validate, so merging a raw string in produces a model that looks fine and then fails at the point of writing meta.json:

PydanticSerializationError: Error calling function `_serialize_by_kind`:
AttributeError: 'str' object has no attribute '_is_handler_for_current_class'

The API route validates through UpdateSecretsRequest, so the HTTP path never hits this, but the method accepts strings and apply_resume_secrets merges the same way. test_a_plain_string_secret_is_stored_as_a_secret_source passes a bare string so this cannot regress.

How to Test

uv run pytest tests/agent_server/test_conversation_service.py -k "mutation or plain_string" -q

Full suite:

$ uv run pytest tests/agent_server/ -q
2094 passed, 13 deselected in 180.44s

ruff check and ruff format --check clean on both changed files.

End-to-end, not just unit tests

Start a conversation, mutate all three through the service, drop the ConversationService entirely, and bring a fresh one up against the same directory:

in-process confirmation_policy : AlwaysConfirm
in-process security_analyzer   : LLMSecurityAnalyzer
in-process secrets keys        : ['MY_TOKEN']
meta.json confirmation_policy  : {'kind': 'AlwaysConfirm'}
meta.json security_analyzer    : {'kind': 'LLMSecurityAnalyzer'}
meta.json secrets keys         : ['MY_TOKEN']
after restart confirmation_policy: AlwaysConfirm
after restart security_analyzer  : LLMSecurityAnalyzer
after restart secret value       : 'shh'

What happens to the secret at rest

I checked this rather than assuming, since the change is what starts routing API secret updates into meta.json.

With a cipher configured, the value is encrypted and round-trips:

{"MY_TOKEN": {"value": "gAAAAABqmvmIuMS90CqPhDCi_Xo-...", "kind": "StaticSecret"}}

'shh' in meta.json is False, and the reloaded source returns 'shh'.

Without a cipher, the serialiser masks it, so the name persists and the value loads back as Nonenot as the ********** placeholder, which would otherwise reach a tool call. Both behaviours are what apply_resume_secrets already produced; this change does not alter them.

Tests

Three added. All three fail on the parent commit:

FAILED test_api_mutations_survive_a_server_restart
FAILED test_mutations_are_written_to_meta_json
FAILED test_a_plain_string_secret_is_stored_as_a_secret_source

test_mutations_are_written_to_meta_json asserts against the file rather than the object, because meta.json is what startup actually reads.

Issue Number

Closes #4810

Type

  • Bug fix
  • Feature
  • Refactor
  • Breaking change

…to meta.json

set_confirmation_policy, set_security_analyzer and update_secrets delegate to the
conversation, which writes base_state.json, and never touch StoredConversation.
Startup reads all three off StoredConversation, so after eviction or a restart
the conversation silently reverts to the values it was created with -- including
its security analyzer and confirmation policy.

The divergence is immediate rather than only on reload: stored is stale in
process as well, so anything reading it before the reload sees the old value too.

    in-process confirmation_policy : NeverConfirm      (set to AlwaysConfirm)
    in-process security_analyzer   : None              (set to LLMSecurityAnalyzer)
    meta.json confirmation_policy  : {'kind': 'NeverConfirm'}
    after restart                  : NeverConfirm

All three now write through to meta.json, using model_copy and save_meta as
apply_resume_secrets already does for the resume path. Rollback when save_meta
itself fails is a separate concern tracked in OpenHands#4810's sibling issue and is not
changed here.

update_secrets is typed for SecretValue (str | SecretSource) while
StoredConversation.secrets holds SecretSource, and model_copy does not validate,
so a plain string would have been stored unvalidated and only failed later when
meta.json was serialised. Strings are normalised on the way in.

Secrets keep the serialisation they already had: encrypted with the server's
cipher where one is configured, and where one is not the name persists while the
value loads back as None rather than as the mask placeholder.
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

📁 PR Artifacts Notice

This PR contains a .pr/ directory with temporary PR-specific documents. Because this is a fork PR, the directory will be automatically removed from main immediately after merge.

_get_or_load_event_service returns EventService | None, so pyright rejected
reading .stored off it directly.

@enyst enyst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR, @alanhuangyoo ! I think maybe the right solution is rather to read them from base_state.json?

I believe we have another PR working through this… let me find it 🙏

Edit: #4813

@alanhuangyoo

Copy link
Copy Markdown
Contributor Author

You are right, and #4813 is the better fix. Closing this.

Mine adds a sync; #4813 removes the need for one by taking the three fields off StoredConversation entirely, so base_state.json is the only place they live — the same shape #4440 used for agent. Two sources kept in step is a thing someone has to remember; one source is not.

I had reached for the sync because apply_resume_secrets already does exactly that and I matched it. That was the wrong precedent to match.

Before closing I checked one thing #4813 depends on and one thing it could have broken, since I had the reproduction already set up. Posting the results on #4813 rather than here.

Thanks @enyst.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: confirmation_policy, security_analyzer, and secrets mutations not persisted to meta.json

2 participants