🥢 fix: Exclude Prompt Inputs From Terminal Events - #16112
Conversation
Terminal events carried prompt-building inputs into every place a FINAL is kept: the runtime cache, the durable Redis job hash, Pub/Sub publication and late/cross-replica replay. Neither terminal producer projected the payload, so a 706 KiB attachment-heavy FINAL was stored and published at full size. sanitizeMessageForTransmit removed fileContext and files[].text from requestMessage at the controller, but never image_urls, and responseMessage was not sanitized at all. projectTerminalEvent is one shared, schema-aware projection applied at both GenerationJobManager.emitDone() and publishTerminalClaim(), and on read where a stored record may predate it. It excludes fileContext, image_urls and embedded file bodies from requestMessage, responseMessage and every runMessages entry, while preserving message content, attachment references and display metadata, and the terminal/reconciliation protocol fields. publishTerminalClaim compared publicationEvent !== finalEvent to decide persistenceFailed. Projection allocates a new object, so that identity check now compares against the projected intendedEvent; a successfully persisted FINAL would otherwise have been reported as a persistence failure.
|
Ready for review at head That head carries:
Measured before/after at each boundary, and the limits of the claim, are in the PR description. Notably |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e5f6f8ced
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const MESSAGE_SLOTS = ['requestMessage', 'responseMessage'] as const; | ||
|
|
||
| /** Nested collections of file-like entries on a message. */ | ||
| const FILE_COLLECTIONS = ['files', 'attachments'] as const; |
There was a problem hiding this comment.
Do not apply the text denylist indiscriminately to attachments: unlike requestMessage.files[].text, an assistant-generated attachment's text is authoritative display content consumed by TextAttachment, MermaidArtifact, and the HTML/Office artifact renderers. A late or cross-replica subscriber may receive only the stored FINAL rather than the earlier attachment chunk, and a preserved status: 'ready' prevents preview polling from recovering the removed text, leaving inline previews blank or reduced to download chips. Restrict stripping to prompt-input file bodies, or otherwise preserve output attachment text.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and a user-visible regression. Fixed in 537ad2d90.
Confirmed the mechanism end to end before changing anything:
BaseClient.js:1218setsresponseMessage.attachments = (await Promise.all(this.artifactPromises)).filter(...), soattachmentsis generated output, not prompt input.TextAttachmentrenders it directly:const text = file.text ?? ''.- Your recovery point is exact.
useAttachmentPreviewSynccomputesbaseStatus = file?.status ?? 'ready'andenabled = !!fileId && baseStatus === 'pending', so an attachment that arrivesreadynever polls the preview endpoint. Stripping its text was unrecoverable, not merely lossy.
Each collection now carries its own exclusion set: files excludes text plus _id/__v (its text is the body extracted for the prompt), attachments excludes only _id/__v. Storage bookkeeping still goes from both.
Coverage: three unit tests (generated text preserved while a sibling prompt body is excluded; same on a runMessages entry; bookkeeping still excluded from both collections) and the boundary spec now asserts text and status: 'ready' survive in the actual Redis hash and published frame. Negative control: restoring the shared denylist fails exactly those three.
This also corrected a measurement error of mine. My original 706 KiB figure used a responseMessage.files[].text fixture, and response.files is never assigned on the agents path. Re-measured on a realistic shape the leak is 307.0 KiB to 6.9 KiB, all of it image_urls. The PR description now says that instead.
| /** Records written before projection shipped, or by an older replica, | ||
| * are projected on read so replay never re-delivers or re-caches an | ||
| * oversized payload. */ | ||
| finalEvent = projectTerminalEvent(JSON.parse(jobData.finalEvent) as t.ServerSentEvent); |
There was a problem hiding this comment.
The mixed-deployment protection here covers terminal events read from the job store, but not a live Pub/Sub FINAL produced by an older replica: the transport onDone callback still passes that event directly to queueDone, after which deliverDone caches it and forwards it to the browser. During a rolling deployment, an old generation owner and a new subscriber replica therefore still deliver and cache the oversized fileContext, image_urls, and embedded bodies; apply the projection to transport-delivered terminal events before queueing them as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Fixed in 537ad2d90.
You are right that I only covered the store-read paths. The transport callback at onDone passed its event straight to queueDone, which caches it on the runtime and forwards it, so during a rolling deploy an old generation owner's published FINAL reached a new subscriber intact with nothing read from the job store to project instead.
The projection now runs at the top of queueDone. That is the single choke point every terminal delivery to a subscriber passes through, both the deferred branch that sets runtime.finalEvent and the immediate deliverDone, so it covers the transport frame, the stored-terminal replay and the reconcile frame in one place. It is idempotent and returns the identical reference for an already-projected frame, so the paths that were already projected pay nothing.
Coverage: a new Redis test publishes an unprojected FINAL through a separate RedisEventTransport at the live generation while a subscriber is attached, with no durable finalEvent written, then asserts both the delivered event and the cached runtime.finalEvent are safe.
Worth noting what the negative control showed: with the queueDone projection removed, the new test fails while the existing legacy-record replay test still passes. The two mixed-deployment paths really were independent, which is what your finding identified.
| * Non-terminal events (chunks, `created`) carry no message payload and are | ||
| * returned unchanged. | ||
| */ | ||
| export function projectTerminalEvent<T extends ServerSentEvent>(event: T): T { |
There was a problem hiding this comment.
The function deletes properties while declaring that it returns the exact input type T. If a caller passes a structural subtype where requestMessage.fileContext, image_urls, or a nested file body is required, TypeScript still permits that caller to dereference the field after projection even though it is now undefined, which can produce a runtime exception. The newly exported ProjectedFinalEvent is therefore not actually enforced at the projection boundary; use an overload or conditional return type that preserves identity only for non-final events and returns the projected contract for FINAL events.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and worse than stated. Fixed in 537ad2d90.
ProjectedFinalEvent was not merely unenforced at the boundary, it was vacuous as a type. FinalMessageFields carries [key: string]: unknown, so keyof is string | number and Omit<FinalMessageFields, 'fileContext' | 'image_urls'> removes nothing at all. The exported contract was structurally identical to the unprojected one.
Two changes, taking the overload half of your suggestion:
export type ProjectedMessageFields = Omit<FinalMessageFields, TransientMessageField> & {
[K in TransientMessageField]?: never;
};
export function projectTerminalEvent(event: FinalEvent): ProjectedFinalEvent;
export function projectTerminalEvent<T extends StreamEvent | CreatedEvent>(event: T): T;
export function projectTerminalEvent(event: ServerSentEvent): ServerSentEvent;Manager call sites all pass the union and are unaffected.
I checked what this actually enforces rather than assuming, and my first attempt was wrong in a way worth recording. I initially asserted that reading an excluded field errors; tsc reported the @ts-expect-error directives as unused, because the field reads as undefined and undefined is assignable to string | undefined. What is genuinely enforced is that the field types as undefined rather than unknown, so it cannot be treated as live data, and that constructing a ProjectedFinalEvent carrying one is an error (Type 'string' is not assignable to type 'undefined').
Coverage: two tests pinning exactly those, the second with a @ts-expect-error on the construction so tsc --noEmit fails if the enforcement regresses. packages/api typechecks to a byte-identical error set against a pristine worktree at the same base commit.
Addresses three review findings at 9e5f6f8. Output attachment text was excluded as if it were a prompt input. `files` carries the user's uploads, whose `text` is the body extracted for the prompt, but `attachments` holds resolved artifactPromises that BaseClient assigns onto responseMessage, so its `text` is model-generated output rendered inline by TextAttachment from `file.text ?? ''`. Excluding it was also unrecoverable: useAttachmentPreviewSync polls the preview endpoint only while status is 'pending', and an absent status reads as 'ready', so a late or cross-replica subscriber whose only source is the stored FINAL rendered a blank preview. Each collection now carries its own exclusion set; storage bookkeeping (_id, __v) is still excluded from both. A live Pub/Sub FINAL from an older replica bypassed projection. The store-read paths were covered, but during a rolling deploy an old generation owner publishes an unprojected frame that a new subscriber cached on its runtime and forwarded to the browser. queueDone is the one choke point every terminal delivery to a subscriber passes through, so the projection now runs there. The declared return type was the input type T, so the exported projected contract was never enforced. FinalMessageFields carries an index signature, which makes Omit alone a no-op, so ProjectedMessageFields now maps the transient fields to `?: never` and overloads return the projected contract for a FinalEvent, the exact type for a non-terminal event, and ServerSentEvent for the union every manager call site passes.
|
Round 1 addressed. Ready for review at head All three findings were valid and are fixed in
Two things reviewers should know, both consequences of the first finding:
Verified at this head: 30 stream suites / 965 passed (1 skipped) on single-node; the two projection specs 29/29 on single-node and on a 3-master cluster; |
tsdown builds packages/api with --isolatedDeclarations, which rejects a spread element in an inferred array type (TS9018), so composing TRANSIENT_FILE_FIELDS from a shared bookkeeping tuple broke Build packages and every lane that depends on a built package. tsc --noEmit does not use that flag and passed, which is why this reached CI. Both tuples are now spelled out, with a comment recording the constraint.
|
@codex review the latest head |
|
Head moved. Now at
No behavior changed between Review state at this head: the three P2 findings from |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Pull Request
Summary
A terminal (
final: true) event is kept in four places: the manager's runtime cache, the durablefinalEventfield of the Redis generation job hash, the Pub/Sub frame published at completion, andlate/cross-replica replay. Neither terminal producer projected the payload before any of that, so
whatever a controller handed over was stored and published at full size.
The event carries
requestMessage,responseMessageandconversation, and those messages alsocarry data assembled only to build the prompt.
sanitizeMessageForTransmitremovedfileContextandfiles[].textfromrequestMessageat the controller call site, but neverimage_urls, and nothingsanitized the event centrally. Attachment-heavy conversations therefore put base64 image input into
Redis memory and into completion-time Pub/Sub traffic.
Measured on this branch against a single-node Redis, driving the real
publishTerminalClaimwith themessage shape
request.jsactually builds (a 400 KiB extracted document, a 300 KiB base64 image, anda legitimate answer plus a generated attachment held constant):
finalEventprojectTerminalEventis one shared projection applied at every terminal boundary. It excludesfileContext,image_urlsandfiles[].textfromrequestMessage,responseMessageand everyrunMessagesentry, and preserves message text and structured content, attachment references andtheir display metadata, generated attachment text, and the terminal/reconciliation protocol fields.
Construction is unchanged by design: the controller still builds the full object, and the boundary is
what refuses to store it.
Two scope notes, because the reported numbers do not all reproduce:
image_urlsstill leaked.fileContextandrequestMessage.files[].textwere already removed by the caller-level sanitizer. The projectioncovers them anyway, as defense in depth for the producers that do not call it.
the terminal
finalEvent. The reporter's 19-364 MB has not been reproduced at that scale; whatis reproduced is the boundary weakness and one of the three field origins.
How it works
Every producer projects before anything is cached, persisted or published, and the read and delivery
paths project because a frame or record may come from a replica that predates this change.
The projection selects rather than clones: only the containers on a path that actually holds excluded
data are rebuilt, excluded values are never copied, and the caller's objects are never mutated
because prompt processing and message persistence still own them. Projecting an already-safe event
returns the identical reference, which makes it idempotent and free on the common path.
filesandattachmentsare not the same thingBoth are collections of file-like entries, and only one of them carries prompt input.
TextAttachmentrenders an attachment fromfile.text ?? '', anduseAttachmentPreviewSyncpollsGET /api/files/:file_id/previewonly whilestatus === 'pending', with an absent status reading as'ready'. Excluding generated attachment text would therefore blank the inline preview for a late orcross-replica subscriber with no path back to it. Each collection carries its own exclusion set;
storage bookkeeping (
_id,__v) is excluded from both.Persistence-success bookkeeping
publishTerminalClaimdecided success partly by object identity:That check distinguishes publishing the intended payload from publishing one recovered out of durable
state or a conservative reconciliation. Projection allocates a new object, so comparing against the
caller's reference would have classified every successfully persisted FINAL as a persistence failure.
The comparison now uses the projected event and the other three distinctions are untouched. A
regression test pins each, and reverting this one line turns three of them red.
The projected type is enforced, not decorative
FinalMessageFieldscarries an index signature, soOmitalone removes nothing from it and the firstversion of
ProjectedFinalEventwas structurally identical to the unprojected contract. Thetransient fields are now mapped to
?: never, and overloads return the projected contract for aFinalEvent, the exact type for a non-terminal event, andServerSentEventfor the union everymanager call site passes. An excluded field types as
undefinedrather thanunknown, andconstructing a projected event that carries one is a type error.
Type of change
Testing
Reproduced first, then fixed. A scratch harness built an attachment-heavy FINAL, drove it through the
real
publishTerminalClaimagainst a real Redis, and measured the durable hash and the publishedframe at each boundary; those are the numbers in the table. It confirmed
fileContextandrequestMessage.files[].textwere already stripped by the caller and thatimage_urls(300 KiB) wasnot. The harness was then replaced by the committed tests.
Tested environments/configuration:
USE_REDIS=true USE_REDIS_STREAMS=true, bothUSE_REDIS_CLUSTER=falseandtrueAutomated tests:
packages/api/src/stream/__tests__/terminalProjection.spec.ts, 19 tests: exclusion, preservation ofcontent and attachment metadata, generated attachment text surviving, no input mutation,
idempotence,
nullversus absent slots, the two type-enforcement assertions, and payload scalingthat holds the answer constant while the excluded bodies grow.
packages/api/src/stream/__tests__/terminalProjectionBoundary.stream_integration.spec.ts, 10 tests:the actual Redis hash and published frame, in-memory parity, durable size not scaling with excluded
bodies, a live unprojected frame published by an older replica, a legacy stored record replayed by a
second replica, and the three
persistenceFailedoutcomes. Green on single-node and on cluster.src/stream/__tests__/*.spec.tsincluding every stream integration spec: 30 suites, 965 passed, 1skipped.
npx tsc --noEmitinpackages/api: 62 pre-existing errors in unrelated modules (skills,langfuse,code,agents/triggers) from a staledata-provider/data-schemasdist in thisenvironment. A pristine worktree at the same base commit produces a byte-identical error set, so
this branch adds none.
api/app/clients/specs/BaseClient.test.js, which cannot load here because thebuilt
@librechat/apibarrel is stale (needsRetentionConversation is not a function). It failsidentically on an unmodified worktree at the same base commit; CI's
Tests: apishards cover it.Negative controls, each verified by temporarily breaking the mechanism:
publishTerminalClaimfails 3 leak regressions while theemitDonecoverage still passes, so the two producers are covered independently.
publicationEvent !== finalEventfails 3 tests including "reports success for a normallyprojected and persisted FINAL", and leaves the recovery and reconciliation cases passing.
queueDoneprojection fails only the live-frame test while the stored-record replaytest still passes, so the two mixed-deployment paths are covered independently.
filesandattachmentsfails the three attachment-preservationtests.
Risk / compatibility
No wire-format change: the terminal contract is the same shape with excluded fields absent, and
absence was already possible for every one of them.
Mixed deployments are covered in both directions. A new producer publishes and stores projected
events that an old subscriber reads as an ordinary FINAL. An old producer's frames are projected by a
new subscriber on the live
queueDonepath and on all three store-read paths.Legacy records already in Redis are not rewritten. They are projected on read, so replay never
re-delivers an oversized payload, but parsing a large stored JSON string still costs memory and CPU
until the record expires by TTL. That is eventual expiry, not remediation; no destructive cleanup or
migration is included.
Two limits worth stating plainly.
conversationis deliberately left unprojected, since nothing inthe measurement pointed at it and narrowing it risks title and endpoint state. And a field exclusion
contract does not bound event size: legitimate response content can still make a projected event
large, which is why the size-safety fallback is deliberately out of scope here.
Checklist