Skip to content

fix(gateway): persist offloaded image refs in transcript for iOS share/node ingress#74625

Closed
Lucenx9 wants to merge 1 commit into
openclaw:mainfrom
Lucenx9:fix/node-events-offloaded-refs-transcript
Closed

fix(gateway): persist offloaded image refs in transcript for iOS share/node ingress#74625
Lucenx9 wants to merge 1 commit into
openclaw:mainfrom
Lucenx9:fix/node-events-offloaded-refs-transcript

Conversation

@Lucenx9

@Lucenx9 Lucenx9 commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Problem: `agent.request` events arriving via the iOS Share extension and `node.event` ingress dropped `parsed.offloadedRefs` after `parseMessageWithAttachments` ran, so the user-turn entry of the session transcript was persisted without `MediaPath` / `MediaPaths` fields even though the media was already on disk.
  • Why it matters: Downstream agents replaying history could not "see" the previously sent images during multi-turn conversations, so iOS share / node-ingress users effectively lost attachments from session memory.
  • What changed: Reused the existing `chat.send` media-persistence contract — `persistChatSendImages` + `rewriteChatSendUserTurnMediaPaths` — by exporting those helpers (and `resolveTranscriptPath`) from `server-methods/chat.ts` and wiring them into the `agent.request` branch of `server-node-events.ts`. `persistChatSendImages`'s `logGateway` parameter was widened to `Pick<..., "warn">` since only `.warn` is consumed; existing call sites remain compatible.
  • What did NOT change (scope boundary): No changes to `chat.send` behavior, `agentCommandFromIngress`, the agent runtime, or the parser. Non-image attachments on the node path remain rejected (`acceptNonImage: false`). The over-limit cleanup branch is untouched.

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor required for the fix
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration

Linked Issue/PR

Root Cause

  • Root cause: `server-node-events.ts` referenced `parsed.offloadedRefs` only inside the over-limit cleanup branch and never carried it through to a transcript media-field rewrite. `chat.send` already had a fix for this contract via `persistChatSendImages` + `rewriteChatSendUserTurnMediaPaths`, but those helpers were private to `chat.ts`, so the iOS share / node path could not reuse them.
  • Missing detection / guardrail: No regression test asserted that `agent.request` with an offloaded image produced a transcript user turn with `MediaPaths`. Existing coverage for `chat.send`'s rewrite (`chat.directive-tags.test.ts` — `preserves offloaded attachment media paths in transcript order`) only locked in the web/chat path.
  • Contributing context: Surfaced after PR fix(gateway): prevent OOM and socket hang up via Claim Check pattern for large attachments [media://inbound/<id> contract] #55513 introduced the claim-check offload pattern (per the issue discussion).

Regression Test Plan

  • Coverage level that should have caught this:
    • Unit test
  • Target test or file: `src/gateway/server-node-events.test.ts` — new test `persists offloaded image refs in transcript user turn for agent.request`.
  • Scenario the test locks in: When `parseMessageWithAttachments` returns `offloadedRefs`, `agent.request` invokes `persistChatSendImages` with those refs and then `rewriteChatSendUserTurnMediaPaths` with the resolved `transcriptPath` + persisted `savedImages`.
  • Why this is the smallest reliable guardrail: Mocks at the same `./server-node-events.runtime.js` barrel boundary the existing test file already mocks, so failures in either the wire-up or the persistence-rewrite contract surface in the same lane as the other agent-request unit tests, without a full integration harness.
  • Existing test that already covers this (if any): Only `chat.send` had similar coverage; that test continues to pass with this PR's logger-type widening.

AI assistance

This PR was prepared with Claude (Opus 4.7).

  • AI-assisted (noted here per CONTRIBUTING.md)
  • Testing degree: lightly tested locally:
    • `pnpm tsgo`: green
    • `pnpm check`: green
    • `pnpm test src/gateway/server-node-events.test.ts`: 37/37 passed (including the new regression)
    • `pnpm test src/gateway/server-methods/chat.directive-tags.test.ts -t "preserves offloaded attachment media paths"`: green (sanity-check that the `logGateway` widening does not regress chat.send's path)
  • I understand what the code does and the contract it mirrors from `chat.send`.

@openclaw-barnacle openclaw-barnacle Bot added app: web-ui App: web-ui gateway Gateway runtime size: S labels Apr 29, 2026
@Lucenx9
Lucenx9 force-pushed the fix/node-events-offloaded-refs-transcript branch 2 times, most recently from e7fefef to b7d27b1 Compare April 29, 2026 21:29
@Lucenx9
Lucenx9 force-pushed the fix/node-events-offloaded-refs-transcript branch from b7d27b1 to 0c4ec29 Compare April 29, 2026 21:35
@clawsweeper clawsweeper Bot mentioned this pull request Apr 30, 2026
Closed
@clawsweeper

clawsweeper Bot commented Apr 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge.

What this changes:

The PR exports the existing chat.send media persistence/transcript rewrite helpers, reuses them from the agent.request node-event path, and adds a server-node-events regression test for offloaded image refs becoming transcript media paths.

Required change before merge:

The remaining repair is a narrow changelog-only update; the gateway implementation still needs ordinary maintainer review and validation.

Security review:

Security review cleared: The diff only reuses existing gateway media transcript persistence helpers and adds tests; it does not add dependencies, workflows, secrets handling, package metadata, or external code execution paths.

Review findings:

  • [P3] Add the required changelog entry — src/gateway/server-node-events.ts:582
Review details

Best possible solution:

Add a single-line entry under Unreleased/Fixes crediting @Lucenx9, then land this PR or an equivalent narrow gateway change after the targeted server-node-events/chat-send tests and changed gate pass so the linked issue closes from the merge.

Do we have a high-confidence way to reproduce the issue?

Yes. Current main has a static high-confidence reproduction: an agent.request with an offloaded image gets parsed into offloadedRefs, but the node path drops those refs before transcript MediaPath/MediaPaths can be written.

Is this the best way to solve the issue?

Yes for the gateway code path. Reusing the existing chat.send media persistence and transcript rewrite helpers is the narrowest maintainable fix; the branch is not merge-ready until the required changelog entry is added.

Full review comments:

  • [P3] Add the required changelog entry — src/gateway/server-node-events.ts:582
    This is a user-facing bug fix for iOS share/node attachments regaining transcript media, but the branch only changes gateway code/tests and has no CHANGELOG.md entry. OpenClaw policy requires user-facing fixes to add a single-line Unreleased/Fixes entry with contributor credit before merge.
    Confidence: 0.9

Overall correctness: patch is correct
Overall confidence: 0.86

Acceptance criteria:

  • git diff --check
  • pnpm test src/gateway/server-node-events.test.ts
  • pnpm test src/gateway/server-methods/chat.directive-tags.test.ts -t "preserves offloaded attachment media paths"
  • pnpm check:changed

What I checked:

  • Current main drops node-path offloaded refs: Current main assigns parsed.message, parsed.images, and parsed.imageOrder for agent.request, but only references parsed.offloadedRefs inside the over-limit cleanup branch before dispatching agentCommandFromIngress without transcript media rewrite context. (src/gateway/server-node-events.ts:497, 4cbd1b53cf86)
  • Parser preserves the metadata the node path loses: parseMessageWithAttachments records each offloaded media item as an OffloadedRef with id, mediaRef, path, mimeType, label, and sizeBytes, and returns those refs to callers. (src/gateway/chat-attachments.ts:384, 4cbd1b53cf86)
  • Existing chat.send contract has the needed persistence/rewrite logic: chat.send already converts inline/offloaded attachments into SavedMedia and rewrites the matching user transcript turn with MediaPath/MediaPaths fields. (src/gateway/server-methods/chat.ts:701, 4cbd1b53cf86)
  • PR reuses the chat.send helper path: The PR exports the helper functions, imports them through server-node-events.runtime, persists parsed offloaded refs before dispatch, and runs rewriteChatSendUserTurnMediaPaths after the ingress agent run settles. (src/gateway/server-node-events.ts:582, 0c4ec29f63c5)
  • Regression coverage is targeted: The PR adds a server-node-events unit test that mocks parseMessageWithAttachments returning an offloaded image ref, asserts persistChatSendImages receives it, and waits for transcript rewrite with the saved media path. (src/gateway/server-node-events.test.ts:1019, 0c4ec29f63c5)
  • Required changelog entry is missing: CHANGELOG.md has an Unreleased/Fixes section, and a search found no entry for this PR, the linked issue, or the offloadedRefs/node-ingress transcript fix. (CHANGELOG.md:17, 4cbd1b53cf86)

Likely related people:

  • Syysean: Merged fix(gateway): prevent OOM and socket hang up via Claim Check pattern for large attachments [media://inbound/<id> contract] #55513 introduced the media://inbound claim-check/offloadedRefs flow that the node-event transcript path now needs to preserve. (role: introduced related claim-check behavior; confidence: medium; commits: c6f2db150687; files: src/gateway/chat-attachments.ts, src/gateway/server-node-events.ts, src/media/store.ts)
  • steipete: Recent main history shows repeated maintenance of the gateway node-event, chat media, and transcript-adjacent surfaces, including iOS background node events and image routing. (role: recent maintainer and adjacent owner; confidence: high; commits: bdba90a20b2b, 0113248d917c, 86f69ba5a06a; files: src/gateway/server-node-events.ts, src/gateway/server-methods/chat.ts, src/gateway/chat-attachments.ts)
  • samzong: Recent merged chat attachment work modified the chat.send offload path and persistence contract that this PR reuses. (role: recent adjacent contributor; confidence: medium; commits: 25ef9c0c419d; files: src/gateway/server-methods/chat.ts, src/gateway/chat-attachments.ts)

Remaining risk / open question:

  • This read-only review did not run the PR tests or changed gate; the targeted server-node-events/chat tests and normal changed validation should run before merge.
  • The PR base SHA is older than current main, so maintainers should confirm the branch rebases cleanly before landing.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 4cbd1b53cf86.

@Lucenx9 Lucenx9 closed this May 1, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 19, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

app: web-ui App: web-ui gateway Gateway runtime size: S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: bug(gateway): offloadedRefs metadata lost in transcript for iOS share/node path

1 participant

@Lucenx9