refactor(events): deduplicate ConversationStateUpdateEvent definition

[VascoSch92]

Problem

ConversationStateUpdateEvent was defined in two places with different shapes:

• Canonical — src/events/types.ts: extends BaseEvent, value: unknown, includes previous_value.
• Duplicate — src/conversation/remote-state.ts: extends Event, value: any, missing previous_value.

Different base types, different value types, and a missing field — a source of confusion and drift.

Closes #112.

Change

• Delete the local interface in remote-state.ts.
• Import the canonical ConversationStateUpdateEvent from ../events/types.
• The __full_state__ snapshot path now reads value.full_state through a cast (matching the adjacent (this.cachedState as any) style) since the canonical value is the stricter unknown.

Non-breaking: nothing imported the remote-state copy — index.ts re-exports only the RemoteState class, and the type is already exported from events/types. local-conversation.ts already imports the canonical type. tsc confirms the event as ConversationStateUpdateEvent downcast still holds (BaseEvent extends Event).

Tests

New src/__tests__/remote-state-events.test.ts drives updateStateFromEvent with the canonical type (including the new previous_value field):

• a __full_state__ snapshot event is applied and served from cache (no network);
• a single-key update event is applied;
• events route correctly through createStateUpdateCallback.

Full unit suite green (274 tests), tsc build clean, eslint (no new warnings vs. the removed value: any) + prettier clean.

[github-actions]

Endpoint audit

❌ 6 off-contract call(s) — not on the agent-server · classifiers: cloud

Category	Count
❌ Off-contract (not on agent-server)	6
⛔ no known backend	5
↗️ served by cloud	1
➕ Missing API (agent-server has, client lacks)	9
agent-server endpoints	116
client endpoints	111

❌ Not on agent-server (gated, 6)

⛔ (no known backend) — served by no backend we can see (5)

• DELETE /api/meta-profiles/{}
• GET /api/meta-profiles
• GET /api/meta-profiles/{}
• POST /api/meta-profiles/{}
• POST /api/meta-profiles/{}/activate

↗️ served by cloud (1)

• GET /api/shared-events/search

➕ Missing API — agent-server exposes it, client does not implement (9)

• GET /
• GET /api/conversations
• GET /api/conversations/{}/events
• GET /api/conversations/{}/workspace
• GET /api/conversations/{}/workspace/{}
• GET /api/file/archive
• GET /api/init
• POST /api/conversations/{}/load_plugin
• POST /api/init

[VascoSch92]

@OpenHands /roast-codereview

[openhands-ai]

I'm on it! VascoSch92 can track my progress at all-hands.dev

[openhands-ai]

I’ll re-run the PR fetch using the injected GitHub token.

[VascoSch92]

/roast-codereview

You fixed the root problem (two competing definitions) and you did it the right way: delete the local snowflake, import the canonical type, and add a focused test so this doesn’t silently regress later. Nice.

Now the roast:

1) You eliminated the duplicate type… but still has a stealthy

still does . So the PR message says ‘no more here’, but the file still relies on one. It’s pre-existing, sure, but it undercuts the “we’re being strict about unknown” story.

If you want this to be airtight, mirror the guard you used in :

• check
• only unwrap when that property exists

2) will happily accept garbage

In , is still . If a server ever sends a malformed event (or is not an object), will coerce primitives and do weird things (e.g., strings become indexed properties). It won’t throw, but it will produce nonsense.

A tiny hardening would be:

• only when is a non-null object
• otherwise, ignore and optionally surface an error via

3) The new test is good, but the async callback test is a bit “trust me bro”

works, but it’s the classic flaky pattern when CI is slow. If you can, prefer a deterministic flush (e.g., await the promise returned by , or expose a callback that returns a promise, or use a helper).

4) Minor nit: the test mutates directly

It’s fine in Jest, but it’s global shared state. If another test file assumes is untouched and runs interleaved (parallelism settings change), you get fun nondeterminism. A wrapper is already there, so you’re mostly safe — just be aware this is the kind of thing that causes “only fails on CI” later.

---

Net: the dedupe is a solid refactor and the new guard for is the right direction. I’d just tighten the remaining “unwrap full_state” path and harden to avoid silently corrupting the cache on bad input.

This comment was generated by an AI agent (OpenHands) on behalf of the user.