https://docs.all-hands.dev/ logo
Join Slack
Powered by
Have there been any discussions about an intended ...
# proj-agent-server
c
Have there been any discussions about an intended boundary between 
ConversationState
and 
StoredConversation
? I've been running into some friction while working on the new endpoints for conversation tags, but there are at least two latent bugs lurking right now: 
confirmation_policy
, 
security_analyzer
, and 
secrets
are all tracked by both stores and read from 
stored
on startup (e.g. lines 1090–1102 of 
event_service.py
), but API mutations only write to 
ConversationState
. That means those changes are silently lost on conversation eviction or server restart. 
tags
(what my PR addresses) is actually the only field where the API correctly keeps both stores in sync. Is this a known gap? And more broadly, is there a design doc or shared understanding of which store is authoritative for which fields? The current behavior looks like it accreted field by field rather than following a deliberate boundary.
c
The intended design here is like: • 
ConversationState
should be the only one in use for the fields that belong in it • IOW, those fields shouldn’t be duplicated elsewhere
I'd consider these duplications bugs
We should not try to keep two sources of truth, in sync or otherwise; I mean, the existence of two sources of truth in some parts of the code are leftovers that I’d consider bugs
👍🏼 1
c
I understand moving 
confirmation_policy
, 
security_analyzer
, and 
secrets
out of 
StoredConversation
. but what about tags?
Tags are currently read from 
stored
when building listing/search results, so if they move to 
ConversationState
only, every tag-based query would have to deserialize each conversation's 
base_state.json
— which is what 
meta.json
was designed to avoid. Is a cached projection in 
StoredConversation
acceptable (source of truth in 
ConversationState
, derived copy in 
meta.json
for listing), or should the listing use case be handled a different way? Sent using @kind-afternoon-35376
c
Ah, there are a few fields that are not in 
ConversationState
and they’re about lists of conversations - those belong in 
meta.json
Not sure, but maybe this should be the case of tags?
Fields like 
updated_at
c
what i've discovered is that the standalone SDK doesn't use 
meta.json
so i wasn't sure if that could lead to issues with people that use the SDK
c
Yes… is it necessary?
c
it depends on how people use the SDK which idk. technically it could be a breaking change
c
I can use python to create a conversation or more; the SDK doesn’t exactly keep a list of them
We can look at the details, but if I were to guess, there’s no breaking change
c
Grepped for usages of 
state.tags
in the SDK — two internal paths depend on it: 1. Observability (
base.py:172
) — tags are forwarded to the observability span as 
conversation.tags.{key}
attributes at conversation start 2. Fork (
local_conversation.py:878
) — when forking with a title, the title is written into the forked conversation's 
_state.tags
by merging it in Neither is a public-facing API, so no external breaking change, but both would need updating if tags move out of 
ConversationState
. The fork one in particular is a bit odd anyway — it sets a title by injecting it into the tags dict, which might be worth cleaning up regardless. Sent using @kind-afternoon-35376
c
Ah 🤔
c
yeah the observability span snag in particular feels tricky
i'm asking claude about the potential fix now
One constraint with reading from config at span start: you'd lose the ability to reflect tag mutations on the observability span mid-conversation. If tags change after start, the trace wouldn't know about it. That makes observability the real blocker for moving tags out of 
ConversationState
entirely — it probably needs a more considered solution. Happy to dig into that separately. The fork quirk we can fix independently regardless. Sent using @kind-afternoon-35376
c
Yes, the fork is probably a bug
but it’s different
Thank you!
np 1
c
yeah, tags in observability is the remaining question
c
I think this is such a good question, actually. How do we conceptualize tags? • groupings of conversations, interweaved; or • properties of a conversation, to be picked up, interpreted, searched by etc
It seems that if we put it this way, the second sounds better to me
c
i agree. i think the question is whether it justifies keeping the tags in both places. Claude seems to think so. it feels kinda wrong to me but... i guess i can accept it haha
c
Maybe this is worth considering, the cached projection for performance. I feel it’s important for the LLM to know it’s an optimization of sorts, not “the ground truth”
c
ok, i can update my PR to make sure that's clear in AGENTS.md
❤️ 1
c
That may work!
c
ok, it'll be documented with the rest of my changes then: github.com/OpenHands/software-agent-sdk/pull/4617/changes#…
and then I can see about working on the other 2 issues when i get the chance.
Opened #4813 for #4810. The approach I went with: override 
confirmation_policy
, 
security_analyzer
, 
secrets
in 
StoredConversation
with 
Field(exclude=True)
so they're never written to 
meta.json
, and fixed 
update_secrets
to trigger autosave. But I want to check — when you said "those fields shouldn't be duplicated elsewhere", did you mean they also shouldn't exist on 
StoredConversation
in memory at all (i.e. removed from 
ConversationConfig
and only on 
StartConversationRequest
)? We looked at that and ran into the fork path and telemetry helper both reading from 
stored
, plus a CODEX credential injection pattern that needs 
stored.secrets
before 
ConversationState
is loaded. Happy to go further if that's the intent, just wanted to confirm before we commit to the approach. Sent using @kind-afternoon-35376
c
If you want, you could point your agent to find a PR merged relatively recently in the sdk, by enyst, aiming to solve the two sources of truth. Should be easy to recognize from commit title ^^ it was something like that. Just for context, really. And in that context, I’m… not sure offhand why these fields are in ConversationConfig
c
interesting. i'll take a look.
ok, that was a great help. thank you.
l
Thanks both! Great to see this discussion
c
github.com/OpenHands/software-agent-sdk/pull/4617 and github.com/OpenHands/software-agent-sdk/pull/4814 are ready, both relatively short/simple. github.com/OpenHands/software-agent-sdk/pull/4813 is also theoretically ready but it's a more hefty change so i'm gonna look through it before adding my HUMAN note and opening it for review.
2 Views
Open in Slack
Previous
Powered by