🧹 chore: Tighten code-execution attachment polish per audit feedback

Resolves the eight actionable findings from the comprehensive audit:

- Scope `displayFilename` out of `FileContainer`: opt-in via a new
  `displayName` prop. User-uploaded chips (input area, persisted
  message files) keep their raw filename, eliminating the false-positive
  class where `report-abc123.pdf` was silently rewritten to `report.pdf`.
  Code-execution artifact paths in `Attachment.tsx` explicitly compute
  the de-suffixed name and pass it through.
- Tighten `TRAILING_NOTES_PATTERN` to anchor on the two known boilerplate
  openings (`Files from previous executions`, `Files in "Available files"`),
  so a user-authored `Note:` line preceded by a blank line in stdout no
  longer gets eaten along with everything after it.
- `ToolMermaidArtifact`: compute `visibleFilename` once and reuse for
  title, content, and the download `aria-label` (was using the raw
  `attachment.filename` for the aria-label, creating a screen-reader
  inconsistency).
- `ToolArtifactCard`: read `isSubmittingFamily(0)` once via a
  non-subscribing `useRecoilCallback`, instead of subscribing for the
  full lifetime to a value the ref only ever needs at first render.
- Extract `bySalience` and `byEntrySalience` comparators from
  `attachmentTypes.ts`, replacing the ten duplicated sort lambdas in
  `Attachment.tsx` and `LogContent.tsx`.
- Treat `attachmentSalience({ bytes: undefined })` as neutral (`0`)
  rather than empty (`1`); only an explicit `bytes === 0` sinks. Stops
  non-code-exec sources (web-search inline results, files where the
  schema omits the byte count) from silently sinking past real content.
- Pin the click-history test to the panel-open button by name instead
  of relying on `getByRole('button', { pressed: false })`, which
  matched by DOM order.
- Add the missing blank line between adjacent `it(...)` blocks.
- Drop the verbose narrating comments in `FileContainer` along with the
  removed `displayFilename` import.

Adds three regression tests for the new behavior (FileContainer raw
filename, artifact-context displayName flow, user-authored `Note:` line
preserved through cleanup) and updates the salience test for the new
neutral-undefined semantics.
This commit is contained in:
Danny Avila 2026-04-28 23:31:16 -04:00
parent 97517f04c5
commit 4d9cc89af4
11 changed files with 206 additions and 48 deletions

View file

@ -1,6 +1,5 @@
import type { TFile } from 'librechat-data-provider';
import type { ExtendedFile } from '~/common';
import { displayFilename } from '~/components/Chat/Messages/Content/Parts/attachmentTypes';
import { getFileType, cn } from '~/utils';
import FilePreview from './FilePreview';
import RemoveFile from './RemoveFile';
@ -8,6 +7,7 @@ import RemoveFile from './RemoveFile';
const FileContainer = ({
file,
overrideType,
displayName,
buttonClassName,
containerClassName,
onDelete,
@ -15,17 +15,19 @@ const FileContainer = ({
}: {
file: Partial<ExtendedFile | TFile>;
overrideType?: string;
/**
* Optional pre-computed label for the chip. Callers in code-execution
* artifact contexts pass the de-suffixed name; upload chips and
* persisted user files leave this undefined and render the raw filename.
*/
displayName?: string;
buttonClassName?: string;
containerClassName?: string;
onDelete?: () => void;
onClick?: React.MouseEventHandler<HTMLButtonElement>;
}) => {
const fileType = getFileType(overrideType ?? file.type);
// The on-disk filename can carry a `-<6 hex>` collision suffix that
// `sanitizeArtifactPath` adds when sanitization mutated the raw input
// (`.dirkeep` → `_.dirkeep-88b30b`). Show the canonical name in the
// chip; downloads still use `file.filename` so lookup is unaffected.
const visibleName = displayFilename(file.filename);
const visibleName = displayName ?? file.filename ?? '';
return (
<div

View file

@ -0,0 +1,57 @@
import React from 'react';
import { render, screen } from '@testing-library/react';
import '@testing-library/jest-dom';
import type { TFile } from 'librechat-data-provider';
import FileContainer from '../FileContainer';
jest.mock('~/utils', () => ({
cn: (...classes: Array<string | false | null | undefined>) => classes.filter(Boolean).join(' '),
getFileType: () => ({ paths: [], color: '', title: 'Plain' }),
}));
jest.mock('../FilePreview', () => ({
__esModule: true,
default: () => <div data-testid="file-preview" />,
}));
jest.mock('../RemoveFile', () => ({
__esModule: true,
default: () => <button data-testid="remove-file" />,
}));
const baseFile = (overrides: Partial<TFile> = {}): Partial<TFile> => ({
file_id: 'f1',
filename: 'report.pdf',
type: 'application/pdf',
...overrides,
});
describe('FileContainer chip label', () => {
it('shows the raw filename when no `displayName` is supplied (upload context)', () => {
/** A user-uploaded file whose name happens to look like the
* code-execution collision suffix (`-<6 hex>` before extension) must
* not have its name silently rewritten historically a global
* `displayFilename(file.filename)` call here would strip the suffix
* and turn `report-abc123.pdf` into `report.pdf`. Stripping is now
* opt-in via `displayName`, so upload chips show the raw name. */
render(<FileContainer file={baseFile({ filename: 'report-abc123.pdf' })} />);
expect(screen.getByText('report-abc123.pdf')).toBeInTheDocument();
});
it('uses `displayName` when supplied (artifact context opts in)', () => {
render(
<FileContainer
file={baseFile({ filename: 'archive-deadbe.zip', type: 'application/zip' })}
displayName="archive.zip"
/>,
);
expect(screen.getByText('archive.zip')).toBeInTheDocument();
expect(screen.queryByText(/-deadbe/)).not.toBeInTheDocument();
});
it('falls back to empty string when neither `displayName` nor `filename` is set', () => {
const { container } = render(<FileContainer file={{ file_id: 'noname' } as Partial<TFile>} />);
/** Title element exists but is empty — no crash, no `undefined`. */
expect(container.querySelector('.font-medium')?.textContent).toBe('');
});
});

View file

@ -4,7 +4,9 @@ import type { TAttachment, TFile, TAttachmentMetadata } from 'librechat-data-pro
import type { ToolArtifactType } from '~/utils/artifacts';
import {
artifactTypeForAttachment,
attachmentSalience,
bySalience,
byEntrySalience,
displayFilename,
isImageAttachment,
isInternalSandboxArtifact,
isTextAttachment,
@ -58,6 +60,7 @@ const FileAttachment = memo(({ attachment }: { attachment: Partial<TAttachment>
file={attachment}
onClick={handleDownload}
overrideType={extension}
displayName={displayFilename(attachment.filename)}
containerClassName="max-w-fit"
buttonClassName="bg-surface-secondary hover:cursor-pointer hover:bg-surface-hover active:bg-surface-secondary focus:bg-surface-hover hover:border-border-heavy active:border-border-heavy"
/>
@ -119,6 +122,7 @@ const TextAttachment = memo(({ attachment }: { attachment: Partial<TAttachment>
file={attachment}
onClick={handleDownload}
overrideType={extension}
displayName={displayFilename(attachment.filename)}
containerClassName="max-w-fit"
buttonClassName="bg-surface-secondary hover:cursor-pointer hover:bg-surface-hover active:bg-surface-secondary focus:bg-surface-hover hover:border-border-heavy active:border-border-heavy"
/>
@ -296,13 +300,11 @@ export function AttachmentGroup({ attachments }: { attachments?: TAttachment[] }
// Sink empty / placeholder-shaped files in each bucket so the user's
// eye lands on the real artifact first. `sort` is stable in modern
// engines (V8 ≥ 7.0) so equal-weight entries keep their input order.
fileAttachments.sort((a, b) => attachmentSalience(a) - attachmentSalience(b));
textAttachments.sort((a, b) => attachmentSalience(a) - attachmentSalience(b));
panelArtifacts.sort(
(a, b) => attachmentSalience(a.attachment) - attachmentSalience(b.attachment),
);
mermaidArtifacts.sort((a, b) => attachmentSalience(a) - attachmentSalience(b));
imageAttachments.sort((a, b) => attachmentSalience(a) - attachmentSalience(b));
fileAttachments.sort(bySalience);
textAttachments.sort(bySalience);
panelArtifacts.sort(byEntrySalience);
mermaidArtifacts.sort(bySalience);
imageAttachments.sort(bySalience);
return (
<>

View file

@ -5,7 +5,8 @@ import type { TFile, TAttachment, TAttachmentMetadata } from 'librechat-data-pro
import type { Artifact } from '~/common';
import {
artifactTypeForAttachment,
attachmentSalience,
bySalience,
byEntrySalience,
displayFilename,
isInternalSandboxArtifact,
isTextAttachment,
@ -121,11 +122,11 @@ const LogContent: React.FC<LogContentProps> = ({ output = '', renderImages, atta
// Sink empty / placeholder files in each bucket so the user's eye
// lands on the real artifact first. Stable sort preserves the
// arrival order among non-empty entries.
imageAtts.sort((a, b) => attachmentSalience(a) - attachmentSalience(b));
textAtts.sort((a, b) => attachmentSalience(a) - attachmentSalience(b));
panelAtts.sort((a, b) => attachmentSalience(a.attachment) - attachmentSalience(b.attachment));
mermaidAtts.sort((a, b) => attachmentSalience(a.attachment) - attachmentSalience(b.attachment));
otherAtts.sort((a, b) => attachmentSalience(a) - attachmentSalience(b));
imageAtts.sort(bySalience);
textAtts.sort(bySalience);
panelAtts.sort(byEntrySalience);
mermaidAtts.sort(byEntrySalience);
otherAtts.sort(bySalience);
return {
imageAttachments: renderImages === true ? imageAtts : null,

View file

@ -1,6 +1,12 @@
import { memo, useEffect, useId, useLayoutEffect, useRef } from 'react';
import { Download } from 'lucide-react';
import { useRecoilState, useRecoilValue, useResetRecoilState, useSetRecoilState } from 'recoil';
import {
useRecoilCallback,
useRecoilState,
useRecoilValue,
useResetRecoilState,
useSetRecoilState,
} from 'recoil';
import type { TAttachment, TFile, TAttachmentMetadata } from 'librechat-data-provider';
import type { Artifact } from '~/common';
import FilePreview from '~/components/Chat/Input/Files/FilePreview';
@ -66,15 +72,27 @@ const ToolArtifactCard = memo(({ attachment, artifact }: ToolArtifactCardProps)
const resetCurrentArtifactId = useResetRecoilState(store.currentArtifactId);
const currentArtifactId = useRecoilValue(store.currentArtifactId);
const existingEntry = useRecoilValue(store.artifactByIdSelector(artifact.id));
const isSubmitting = useRecoilValue(store.isSubmittingFamily(0));
const [claim, setClaim] = useRecoilState(store.toolArtifactClaim(artifact.id));
const isSelected = artifact.id === currentArtifactId;
const isMyClaim = claim === claimKey;
// Captured at first render only — cards that mount mid-stream stay
// "fresh" for the rest of their lifetime even after streaming ends,
// and cards that mount post-stream stay "history" even if the user
// sends another message while the same card stays mounted.
const mountedDuringStreamRef = useRef(isSubmitting);
/**
* Captured at first render via a non-subscribing snapshot read so the
* downstream effect doesn't re-fire (and the component doesn't
* re-render) every time `isSubmittingFamily(0)` flips. Cards that mount
* mid-stream stay "fresh" for the rest of their lifetime; cards that
* mount post-stream stay "history" even if the user sends a new
* message while this card stays mounted.
*/
const readInitialIsSubmitting = useRecoilCallback(
({ snapshot }) =>
() =>
snapshot.getLoadable(store.isSubmittingFamily(0)).getValue(),
[],
);
const mountedDuringStreamRef = useRef<boolean | null>(null);
if (mountedDuringStreamRef.current === null) {
mountedDuringStreamRef.current = readInitialIsSubmitting();
}
useLayoutEffect(() => {
// Always (re)claim on mount — a later card for the same id displaces

View file

@ -50,6 +50,8 @@ const ToolMermaidArtifact = memo(({ attachment, text }: ToolMermaidArtifactProps
return null;
}
const visibleFilename = displayFilename(attachment.filename);
return (
<div className="my-2 flex w-full flex-col gap-1">
{(attachment.filename || attachment.filepath) && (
@ -57,16 +59,16 @@ const ToolMermaidArtifact = memo(({ attachment, text }: ToolMermaidArtifactProps
{attachment.filename && (
<div
className="truncate text-[10px] font-medium uppercase tracking-wide text-text-secondary"
title={displayFilename(attachment.filename)}
title={visibleFilename}
>
{displayFilename(attachment.filename)}
{visibleFilename}
</div>
)}
{attachment.filepath && (
<button
type="button"
onClick={handleDownload}
aria-label={`${localize('com_ui_download')} ${attachment.filename ?? ''}`}
aria-label={`${localize('com_ui_download')} ${visibleFilename}`}
title={localize('com_ui_download')}
className={cn(
'inline-flex shrink-0 items-center gap-1 rounded-md px-2 py-1 text-xs',

View file

@ -19,8 +19,8 @@ jest.mock('../LogLink', () => ({
jest.mock('~/components/Chat/Input/Files/FileContainer', () => ({
__esModule: true,
default: ({ file }: { file: { filename?: string } }) => (
<div data-testid="file-container">{file.filename ?? ''}</div>
default: ({ file, displayName }: { file: { filename?: string }; displayName?: string }) => (
<div data-testid="file-container">{displayName ?? file.filename ?? ''}</div>
),
}));
@ -473,8 +473,11 @@ describe('ToolArtifactCard click behaviour', () => {
streaming: false,
});
expect(getSnapshot().currentArtifactId).toBeNull();
// Open button is unpressed initially (no auto-focus).
const openButton = screen.getByRole('button', { pressed: false });
/** Pin to the panel-open button by name the download button has no
* `aria-pressed`, but `getByRole('button', { pressed: false })`
* relies on DOM order, which silently shifts if the chip's button
* order changes. */
const openButton = screen.getByRole('button', { name: /com_ui_artifact_click/i });
act(() => {
fireEvent.click(openButton);
});
@ -534,6 +537,25 @@ describe('AttachmentGroup routing', () => {
expect(filenames[0]).toMatch(/archive\.zip/);
expect(filenames[1]).toMatch(/placeholder\.zip/);
});
it('passes the de-suffixed name to FileContainer in the code-execution artifact bucket', () => {
/** `displayFilename` is now scoped to artifact-rendering call sites:
* `FileContainer` itself never strips the `-<6 hex>` suffix, so a
* user-uploaded `report-abc123.pdf` rendered through the upload chip
* stays intact. The artifact path explicitly opts into stripping by
* computing `displayName` and passing it down verify that flow
* here so the chip shows `archive.zip`, not `archive-deadbe.zip`. */
const sandboxFile = baseAttachment({
file_id: 'sandbox-zip',
filename: 'archive-deadbe.zip',
type: 'application/zip',
bytes: 1024,
} as Partial<TAttachment>);
const { container } = renderWith(<AttachmentGroup attachments={[sandboxFile]} />);
const chip = container.querySelector('[data-testid="file-container"]');
expect(chip?.textContent).toBe('archive.zip');
});
it('renders separate buckets for panel artifacts, mermaid, text, and plain files', () => {
const attachments = [
baseAttachment({

View file

@ -234,12 +234,15 @@ describe('attachmentSalience', () => {
expect(attachmentSalience({ bytes: 47 })).toBe(0);
});
it('returns 1 for zero-byte content (sinks last)', () => {
it('returns 1 only for an explicit zero-byte entry (sinks last)', () => {
expect(attachmentSalience({ bytes: 0 })).toBe(1);
});
it('treats undefined `bytes` as empty', () => {
expect(attachmentSalience({})).toBe(1);
it('treats undefined `bytes` as neutral so non-code-exec sources do not silently sink', () => {
/** Web-search results, uploaded files where the schema omits `bytes`,
* etc. should keep their input order only an explicit `bytes === 0`
* counts as the empty-placeholder shape we want to demote. */
expect(attachmentSalience({})).toBe(0);
});
it('produces a stable bucket sort when used as `(a,b) => salience(a) - salience(b)`', () => {

View file

@ -78,22 +78,42 @@ export const displayFilename = (filename: string | undefined): string => {
/**
* Salience weight for sorting attachments within a bucket. `0` for
* non-empty content (renders first), `1` for empty / placeholder
* (sinks to the bottom). Single-arg so callers compose it inline as
* `arr.sort((a, b) => attachmentSalience(a) - attachmentSalience(b))`
* a two-arg comparator hits TypeScript's contravariance check on
* `TAttachment`'s union branches that don't carry `bytes`.
* normal entries (renders first), `1` only for entries that explicitly
* report `bytes === 0` (empty placeholders sink to the bottom).
*
* Treating an absent `bytes` field as neutral (`0`) keeps non-code-exec
* sources (web-search inline results, uploaded files where the schema
* omits the byte count) from silently sinking past real content. The
* filter is intentionally narrow: only an explicit zero counts as empty.
*
* Accepts the broad `TAttachment` union (some branches lack `bytes`)
* plus the bare `{ bytes?: number }` shape the unit tests use. The
* `bytes` read goes through a defensive cast since not every branch
* declares the property.
* plus the bare `{ bytes?: number }` shape the unit tests use.
*/
export const attachmentSalience = (item: TAttachment | { bytes?: number }): number => {
const bytes = (item as { bytes?: number }).bytes ?? 0;
return bytes > 0 ? 0 : 1;
const bytes = (item as { bytes?: number }).bytes;
return bytes === 0 ? 1 : 0;
};
/**
* Stable comparator for arrays of `TAttachment`-like values. Equivalent
* to `(a, b) => attachmentSalience(a) - attachmentSalience(b)` but
* exported once so the lambda doesn't need to be repeated at every
* call site.
*/
export const bySalience = (
a: TAttachment | { bytes?: number },
b: TAttachment | { bytes?: number },
): number => attachmentSalience(a) - attachmentSalience(b);
/**
* Comparator variant for buckets that wrap the attachment in a record
* (e.g. `{ attachment, type }` panel entries). Reads salience off the
* inner `attachment` field so wrapped buckets sort the same way the
* bare ones do.
*/
export const byEntrySalience = <T extends { attachment: TAttachment }>(a: T, b: T): number =>
attachmentSalience(a.attachment) - attachmentSalience(b.attachment);
/**
* An attachment is treated as an image only when it has the dimensions and
* filepath needed to render via `<Image>`. Without width/height the image

View file

@ -98,6 +98,28 @@ describe('cleanCodeToolOutput', () => {
expect(output).toMatch(/Note: this is real content/);
});
it('preserves a user-authored "Note:" line that follows a blank line in stdout', () => {
/** Tightened anchor: the trailing-notes regex now requires one of the
* known boilerplate openings (`Files from previous executions`, etc.)
* after `Note:`. A user `print()` that happens to output a blank
* line followed by `Note: ...something else...` must survive
* untouched, along with everything that follows it. */
const input = [
'stdout:',
'first line',
'',
"Note: this is the user's measurement output",
'more output',
'',
'Generated files:',
'- /mnt/data/foo.txt',
].join('\n');
const output = cleanCodeToolOutput(input);
expect(output).toMatch(/Note: this is the user's measurement output/);
expect(output).toMatch(/Generated files:/);
expect(output).toMatch(/- \/mnt\/data\/foo\.txt$/);
});
it('drops trailing blank lines after stripping', () => {
const input =
'stdout:\nresult\n\n\nNote: Files from previous executions are automatically available and can be modified.';

View file

@ -41,7 +41,16 @@
const PER_FILE_ANNOTATION_PATTERN =
/\s*\|\s*(?:File is already downloaded by the user|Image is already displayed to the user|Available as an input[^,\n]*?)(?=,|\n|$)/g;
const TRAILING_NOTES_PATTERN = /\n\s*\n\s*Note:[\s\S]*$/;
/**
* Matches the bash executor's trailing `Note:` paragraphs. Anchored to
* the two known boilerplate openings (`Files from previous executions`,
* `Files in "Available files"`) so a user-authored `Note:` line even
* one preceded by a blank line in stdout is not eaten by the
* `[\s\S]*$` tail. New upstream variants need to be added here
* explicitly; that's the trade-off for not silently dropping content.
*/
const TRAILING_NOTES_PATTERN =
/\n\s*\n\s*Note:\s*(?:Files from previous executions|Files in "Available files")[\s\S]*$/;
/**
* Returns `content` with the bash-executor boilerplate removed. Safe