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.

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: 6bcaf97d272b14ce6673e54d61818b8c81475304
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/0825fed0-1fc4-4fd5-b69a-03098732aa5c

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Verdict: Approve — no material findings.

Risk: Low. This is a clean, behavior-preserving deduplication that also tightens type safety.

Summary

The PR removes the duplicate ConversationStateUpdateEvent interface from remote-state.ts (which extended Event with value: any and lacked previous_value) and imports the canonical definition from events/types (which extends BaseEvent with value: unknown and includes previous_value). I verified the change is non-breaking and correct:

1. Non-breaking: Nothing imported the remote-state copy. index.ts and local-conversation.ts already reference the canonical type from events/types. The event as ConversationStateUpdateEvent downcast in createStateUpdateCallback remains valid because BaseEvent extends Event.

2. Behavioral equivalence of the unwrap rewrite: The old event.value?.full_state ?? event.value is replaced by a runtime narrowing (typeof event.value === 'object' && event.value !== null && 'full_state' in event.value). I checked all edge cases — null, undefined, non-object values, objects without full_state, and objects with a nullish full_state — and the new code produces identical results to the old expression.

3. Type safety improves: value moves from any to unknown, which is why the runtime narrowing is needed and is the correct pattern.

Validation

• tsc build: clean.
• ESLint: 0 errors; the 3 remaining no-explicit-any warnings in remote-state.ts (lines 49, 64, 91) are pre-existing — this PR actually removes one any (the deleted value: any field).
• New remote-state-events.test.ts: 3/3 passing, exercising the canonical type including previous_value, the __full_state__ snapshot path, single-key updates, and the createStateUpdateCallback dispatch.

No inline comments — no actionable issues were found on the changed lines.