refactor(state): normalize full_state unwrapping in RemoteState once by VascoSch92 · Pull Request #248 · OpenHands/typescript-client

VascoSch92 · 2026-06-29T11:49:56Z

Problem

RemoteState has 7 accessors that each unwrapped the API's full_state wrapper themselves (via a private unwrapState() helper) on top of the result of getConversationInfo().

The original issue framed the per-accessor unwrap as purely redundant. It isn't quite: getConversationInfo() only normalized the fresh REST fetch, not the cache-hit return, and the cache can also be populated by WebSocket state-update events (createStateUpdateCallback → updateStateFromEvent) that still carry a full_state wrapper. So the per-accessor unwrap was load-bearing for the event-driven path.

Closes OpenHands/software-agent-sdk#4733.

Change

Make getConversationInfo() the single unwrap point, normalizing on both the cache-hit and fresh-fetch returns (new private normalizeFullState() helper).
Remove the per-accessor unwrapping from all 7 accessors and delete the old per-accessor helper.

The helper is private, so this is internal-only — no public API or behavior change.

Why the extra care (caught by CI)

My first cut normalized only the fresh fetch (matching the issue's literal proposal). Unit tests passed, but the integration test failed:

execution_status missing in conversation info:
  {"full_state":{...,"execution_status":"idle",...},"last_user_message_id":"..."}
  at RemoteState.getExecutionStatus (src/conversation/remote-state.ts)

i.e. the cached state — populated from state-update events — still had the full_state wrapper, and the cache-hit path returned it un-normalized. Normalizing on the cache-hit path fixes it.

Tests

src/__tests__/remote-state.test.ts:

accessors read correctly from a flat REST payload;
accessors read correctly from a full_state-wrapped REST payload;
legacy agent_status fallback;
missing required field still throws a descriptive error;
regression: a cache populated via an event carrying a full_state wrapper is normalized (this test fails on the naive fresh-fetch-only version and passes here — reproducing the integration failure locally).

Full unit suite green (276 tests), tsc build clean, eslint (no new warnings) + prettier clean. Integration test expected to pass now that the event-populated-cache case is handled.

github-actions · 2026-06-29T11:51:08Z

Endpoint audit

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

Category	Count
❌ Off-contract (not on agent-server)	7
⛔ no known backend	6
↗️ served by `cloud`	1
➕ Missing API (agent-server has, client lacks)	7
agent-server endpoints	112
client endpoints	110

❌ Not on agent-server (gated, 7)

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

DELETE /api/meta-profiles/{}
GET /api/meta-profiles
GET /api/meta-profiles/{}
POST /api/cloud-proxy
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 (7)

GET /
GET /api/conversations
GET /api/conversations/{}/events
GET /api/conversations/{}/workspace
GET /api/conversations/{}/workspace/{}
GET /api/init
POST /api/init

Seven accessors (getExecutionStatus, getConfirmationPolicy, getAgent, getWorkspace, getPersistenceDir, getActivatedKnowledgeSkills, modelDump) each re-ran a per-accessor unwrap of the API's `full_state` wrapper. Make getConversationInfo() the single unwrap point and drop the per-accessor unwrapping. Crucially, getConversationInfo() previously normalized only the fresh REST fetch, not the cache-hit return — and the cache can be populated by state-update events (createStateUpdateCallback -> updateStateFromEvent) that still carry the `full_state` wrapper. So the normalization now runs on the cache-hit path too; otherwise accessors would throw "execution_status missing" against an event-populated cache. Purely internal (the helper is private); public behavior is unchanged. Adds RemoteState unit coverage for flat and full_state-wrapped REST payloads plus a regression test for the event-populated wrapped cache (the case the integration tests exercise). Closes #109

VascoSch92 · 2026-06-30T07:45:18Z

@OpenHands Review this PR. If you find is correct and there are no problem. Accept it.

openhands-ai · 2026-06-30T07:45:33Z

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

VascoSch92 · 2026-06-30T07:49:08Z

🟢 Good taste

I reviewed the PR diff, linked issue OpenHands/software-agent-sdk#4733, and the RemoteState cache/event path. The change looks correct: getConversationInfo() is now the single unwrap point for both REST responses and cache hits, and the accessors remain simple while preserving behavior for event-populated caches.

I verified the targeted unit test locally:

npm test -- --runTestsByPath src/__tests__/remote-state.test.ts --runInBand

Result: passed, 5/5 tests. GitHub also reports all PR checks successful, including CI and integration tests.

[RISK ASSESSMENT]

[Overall PR] ⚠️ Risk Assessment: 🟢 LOW

This is a private internal refactor with focused regression coverage and no public API behavior change.

VERDICT: ✅ Worth merging

I attempted to submit an approving review, but GitHub rejected it with: Review Can not approve your own pull request. So I cannot formally approve with the available token, but from review this PR is acceptable to merge.

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

enyst · 2026-07-03T03:02:07Z

@OpenHands /codereview-roasted and post ypur feedback on pr with gh api

openhands-ai · 2026-07-03T03:02:47Z

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

enyst

🟢 Good taste

This is the right cut: normalize the wire-shape at the state boundary, not seven times in seven accessors. The important part is that getConversationInfo() now handles both fresh REST responses and cache hits, which preserves the event-populated cache path that made the old per-accessor unwrap load-bearing.

I reviewed the PR diff, src/conversation/remote-state.ts, the new src/__tests__/remote-state.test.ts, and linked issue #109. I also ran the targeted regression suite locally:

npm test -- --runTestsByPath src/__tests__/remote-state.test.ts --runInBand

Result: passed, 5/5 tests. npm ci also completed the TypeScript build successfully during install.

No blocking issues found. The regression test covers the exact failure mode: a cached state populated from a state-update event carrying a full_state wrapper must still produce flat accessor results without hitting the network.

[RISK ASSESSMENT]

[Overall PR] ⚠️ Risk Assessment: 🟢 LOW

This is a private-helper refactor with preserved public behavior, narrower duplication, and focused regression coverage around the cache/event path. The main compatibility risk would have been leaving wrapped cache hits unnormalized; this PR explicitly addresses and tests that path.

VERDICT:
✅ Worth merging: Core logic is sound, simpler than the old repeated unwraps, and covered by a regression test that would fail on the naive fresh-fetch-only version.

KEY INSIGHT:
Put shape normalization at the data boundary once; accessors should consume a stable internal representation, not compensate for transport quirks.

This PR review was generated by an AI agent (OpenHands) on behalf of the requester.

openhands-ai · 2026-07-03T03:04:29Z

Done — I reviewed PR #248, verified the relevant RemoteState implementation and tests, ran the targeted regression test successfully (5/5 passed), and posted an approving PR review via the GitHub API.

Review link: #248 (review)

openhands-release-bot · 2026-07-07T07:34:46Z

🚀 Released in v1.32.0.

github-actions Bot added the type: refactor Code refactoring label Jun 29, 2026

VascoSch92 requested a review from all-hands-bot June 29, 2026 11:52

VascoSch92 force-pushed the vasco/normalize-fullstate-unwrap branch from 37c94bd to 9e6ce98 Compare June 29, 2026 11:57

VascoSch92 marked this pull request as ready for review June 30, 2026 06:07

VascoSch92 requested a review from enyst June 30, 2026 07:54

OpenHands deleted a comment from openhands-ai Bot Jul 3, 2026

enyst approved these changes Jul 3, 2026

View reviewed changes

VascoSch92 merged commit bde08a2 into main Jul 5, 2026
13 checks passed

This was referenced Jul 3, 2026

chore(main): release 2.0.0 #236

Closed

chore(main): release 1.32.0 #266

Merged

openhands-release-bot Bot added the released: v1.32.0 Shipped in v1.32.0 label Jul 7, 2026

Navigation Menu

Repository navigation

refactor(state): normalize full_state unwrapping in RemoteState once - #248

refactor(state): normalize full_state unwrapping in RemoteState once#248
VascoSch92 merged 1 commit into
mainfrom
vasco/normalize-fullstate-unwrap

VascoSch92 commented Jun 29, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jun 29, 2026

Uh oh!

VascoSch92 commented Jun 30, 2026

Uh oh!

openhands-ai Bot commented Jun 30, 2026

Uh oh!

VascoSch92 commented Jun 30, 2026

Uh oh!

enyst commented Jul 3, 2026

Uh oh!

openhands-ai Bot commented Jul 3, 2026

Uh oh!

enyst left a comment

Uh oh!

openhands-ai Bot commented Jul 3, 2026

Uh oh!

Uh oh!

openhands-release-bot Bot commented Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

VascoSch92 commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Change

Why the extra care (caught by CI)

Tests

Uh oh!

github-actions Bot commented Jun 29, 2026

Endpoint audit

❌ Not on agent-server (gated, 7)

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

↗️ served by cloud (1)

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

Uh oh!

VascoSch92 commented Jun 30, 2026

Uh oh!

openhands-ai Bot commented Jun 30, 2026

Uh oh!

VascoSch92 commented Jun 30, 2026

Uh oh!

enyst commented Jul 3, 2026

Uh oh!

openhands-ai Bot commented Jul 3, 2026

Uh oh!

enyst left a comment

Choose a reason for hiding this comment

Uh oh!

openhands-ai Bot commented Jul 3, 2026

Uh oh!

Uh oh!

openhands-release-bot Bot commented Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

VascoSch92 commented Jun 29, 2026 •

edited

Loading

↗️ served by `cloud` (1)