Skip to content

proxy-router: fix chat-history JSON round-trip + default forward-context off - #804

Closed
cowboycoderhq wants to merge 4 commits into
MorpheusAIs:devfrom
cowboycoderhq:proxy-router-chat-history
Closed

proxy-router: fix chat-history JSON round-trip + default forward-context off#804
cowboycoderhq wants to merge 4 commits into
MorpheusAIs:devfrom
cowboycoderhq:proxy-router-chat-history

Conversation

@cowboycoderhq

Copy link
Copy Markdown

Part 1 of 3, splitting #798 as requested (router first, then UI function, then optional visual). This PR is proxy-router only.

1. Chat-history JSON round-trip fix (+ regression test). AppendChatHistory recovered stored turns with a plain type assertion chat.Prompt.(OpenAiCompletionRequest). Prompt is interface{}, so a history read back from disk is a map[string]interface{} and the assertion always fails after a JSON round-trip — and the discarded ok made it fail silently: every stored turn was dropped and the model got no history. Fixed by re-marshalling the generic map back through the concrete type. Also takes the last stored message (the turn the user took) instead of Messages[0], so a client sending the running transcript doesn't get its first message replayed each turn. history_roundtrip_test.go exercises the marshal/unmarshal path and asserts the prior turn survives.

2. Default PROXY_FORWARD_CHAT_CONTEXT to false when unset (was true). ⚠️ Default change — called out per review. The client owns the transcript and sends the full messages[]; with forwarding on, the router also prepends stored history, duplicating context for any OpenAI-compatible client. PROXY_STORE_CHAT_CONTEXT stays on for the history drawer / /v1/chats/:id; forwarding is now an explicit opt-in for clients that send only the latest turn.

Verified: go build ./... clean; go test -run TestAppendChatHistory passes.

…ext off

AppendChatHistory recovered stored turns with a plain type assertion
(chat.Prompt.(OpenAiCompletionRequest)). Because Prompt is interface{},
a history read back from disk is map[string]interface{}, so the assertion
always failed after a JSON round-trip — and the discarded `ok` made it fail
silently: every stored turn was dropped and the model got no history. Recover
the concrete type by re-marshalling the generic map. Also take the LAST stored
message (the turn the user actually took), not Messages[0], so a client that
sends the running transcript doesn't get its first message replayed each turn.

Adds a regression test that exercises the marshal/unmarshal path and asserts
the prior turn survives.

Default PROXY_FORWARD_CHAT_CONTEXT to false when unset (was true). The client
owns the transcript and sends the full messages[]; with forwarding on the
router ALSO prepends stored history, duplicating context for any
OpenAI-compatible client. Storage stays on for the history drawer; forwarding
is now an explicit opt-in for clients that send only the latest turn.

@alex-sandrk alex-sandrk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Issues

1. Multi-content prompts silently drop the whole turn. openai.ChatCompletionMessage marshals MultiContent as an array; unmarshalling that into the local ChatCompletionMessage.Content string fails, so asCompletionRequest returns false and the turn including its text response is skipped. Same effective behavior as before. Add test for that.

2. The default flip leaves the Desktop UI without chat memory. Bundled desktop client sends only the latest turn:

    const payload = {
      stream: true,
      messages: [incommingMessage],
    };

Nothing in the launcher or shipped env files sets PROXY_FORWARD_CHAT_CONTEXT=true, so the desktop UI runs on the default. Update ui-desktop/orchestrator.config.ts with: PROXY_STORE_CHAT_CONTEXT: 'true',

3. Docs contradict the new default. docs/reference/env-proxy-router.mdx (line 158) documents the default as true. Update docs.

…d-context

Address review on #804.

Multi-content ("content" as an array of typed parts, as openai.
ChatCompletionMessage marshals MultiContent) made the local
ChatCompletionMessage unmarshal fail, so asCompletionRequest rejected the
stored request and the whole turn — text response included — was silently
dropped from replayed history. Accept string/null/array content (an array
flattens to its text parts); genuinely malformed content still errors.
Image-only turns flatten to empty text and are skipped rather than
forwarded as empty-content user messages. Regression tests cover the
round-trip and each content shape.

Desktop UI: the bundled client sends only the latest turn, so set
PROXY_FORWARD_CHAT_CONTEXT=true in the orchestrator env (every platform
config inherits it) and append the key to a pre-existing .env on upgrade —
writeEnvFile never rewrites an existing file, so a key introduced after
first install would otherwise never reach upgraded installs. Only absent
keys are appended; user-edited values are untouched.

Docs: PROXY_FORWARD_CHAT_CONTEXT default documented as false; sample env
flipped to match.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017M9gPEcjZajpp6EtHphaQB
@cowboycoderhq

Copy link
Copy Markdown
Author

Thanks for the catch on all three — addressed in 71445de.

1. Multi-content prompts. Fixed with a custom UnmarshalJSON on the local ChatCompletionMessage: content is accepted as a string, null/absent, or an array of typed parts (flattened to its text parts joined with \n). Genuinely malformed content (e.g. "content":123) still errors, so nothing new is swallowed. Two tests added:

  • TestAppendChatHistory_MultiContentPromptSurvives — stores the prompt exactly as History.Prompt does (*OpenAICompletionRequestExtra with MultiContent), asserts the stored JSON carries array-shaped content as a precondition, round-trips, and asserts the turn survives with its text recovered. Verified to fail without the fix ("got 1 message(s), want 3" — the drop you described) and pass with it.
  • TestChatCompletionMessage_UnmarshalContentShapes — table test over the content shapes, including that Role/Name/ToolCallID survive the custom unmarshal.

One edge this surfaced: an image-only multi-content turn flattens to empty text, and forwarding an empty-content user message can be rejected by strict providers — AppendChatHistory now skips such pairs, matching how they were treated before.

2. Desktop UI memory. PROXY_STORE_CHAT_CONTEXT: 'true' was already in orchestrator.config.ts (and STORE defaults to true router-side); the variable that controls prepending is PROXY_FORWARD_CHAT_CONTEXT, so that's what I've added to the shared env block — every platform config inherits it (Windows spreads the env and overrides only DOCKER_HOST).

That alone only covers fresh installs: writeEnvFile early-returns when .env already exists, so upgraded installs would never receive the new key. Added an append-if-absent migration — keys listed in envKeysAppendedOnUpgrade are appended to an existing .env when missing; user-edited values are never touched. Verified idempotent.

(Related observation, no action taken: the bundled CLI also sends only the latest turn — its messagesContext is initialized empty and never appended to — so it would likewise need forward-on to have memory. Predates this PR.)

3. Docs. docs/reference/env-proxy-router.mdx now documents the default as false with a note on when to enable it, and the sample value in docs/proxy-router.all.env is flipped to false to match the actual default.

Verification: go build ./..., go vet, full go test ./internal/..., and ui-desktop typecheck:node all pass locally. Worth noting while you're here: build.yml's pull_request trigger is commented out, so none of that runs as PR CI — happy to help re-enable it in a separate PR if wanted.

morrpc frames from a contract provider (e.g. a custody contract with no private
key) are signed by the contract's owner(); the consumer previously required the
recovered signer to equal the provider address, rejecting all contract providers
with ErrInvalidSig before any chain call. Add an owner-resolver (eth_getCode +
owner(), cached) mirroring SessionRouter._isValidProviderReceipt's contract-owner
branch, and route the ping-discovery validation through it. EOA providers and the
nil-resolver (mobile) path are byte-identical to before. Opus-reviewed; decodes
the first ABI word to match the on-chain owner() decode.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011SHw2bF3J8JU825xhAqNaf

@alex-sandrk alex-sandrk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@cowboycoderhq, please create a separate MR for other bugs and avoid committing after approval.

mobile/sdk.go never wires the ProviderAuthResolver. The mobile path still rejects contract providers.

The desktop/server entrypoint wires the resolver in cmd/main.go, but mobile/sdk.go constructs its ProxySender (line 198) without a corresponding SetProviderAuthResolver call. With a nil resolver, authorizedSigner() falls back to the provider address itself, so mobile consumers keep failing contract providers with ErrInvalidSig, including EnsureProviderRegistered.

The commit message frames this as intentional ("nil-resolver (mobile) path are byte-identical to before"), which is fine as a compatibility statement, but it means the bug this PR fixes remains unfixed for mobile.

Suggest adding it in this PR.

cmd/main.go already sets the resolver; mobile/sdk.go constructed ProxySender
without it, so authorizedSigner() fell back to the provider address and
rejected contract providers with ErrInvalidSig (including EnsureProviderRegistered).

Co-authored-by: Cursor <cursoragent@cursor.com>
@alex-sandrk

Copy link
Copy Markdown
Collaborator

Wired ProviderAuthResolver on the mobile path in 52dd90f.

mobile/sdk.go now calls SetProviderAuthResolver(NewProviderAuthResolver(ethClient)) after constructing ProxySender, matching cmd/main.go. Contract-provider morrpc frames (including EnsureProviderRegistered) resolve via on-chain owner() instead of falling back to the provider address.

@alex-sandrk
alex-sandrk self-requested a review August 20, 2026 09:42
@cowboycoderhq cowboycoderhq closed this by deleting the head repository Aug 21, 2026
cowboycoderhq added a commit to cowboycoderhq/Morpheus-Lumerin-Node that referenced this pull request Aug 21, 2026
…d-context

Address review on MorpheusAIs#804.

Multi-content ("content" as an array of typed parts, as openai.
ChatCompletionMessage marshals MultiContent) made the local
ChatCompletionMessage unmarshal fail, so asCompletionRequest rejected the
stored request and the whole turn — text response included — was silently
dropped from replayed history. Accept string/null/array content (an array
flattens to its text parts); genuinely malformed content still errors.
Image-only turns flatten to empty text and are skipped rather than
forwarded as empty-content user messages. Regression tests cover the
round-trip and each content shape.

Desktop UI: the bundled client sends only the latest turn, so set
PROXY_FORWARD_CHAT_CONTEXT=true in the orchestrator env (every platform
config inherits it) and append the key to a pre-existing .env on upgrade —
writeEnvFile never rewrites an existing file, so a key introduced after
first install would otherwise never reach upgraded installs. Only absent
keys are appended; user-edited values are untouched.

Docs: PROXY_FORWARD_CHAT_CONTEXT default documented as false; sample env
flipped to match.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.

2 participants